similar to: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

When gk20a_clk_ctor() returns an error code, pointer "clk" should be released. It's the same when gm20b_clk_new() returns from elsewhere following this call. Signed-off-by: Dinghao Liu <dinghao.liu at zju.edu.cn> --- drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git

Hi Ben, > > When gk20a_clk_ctor() returns an error code, pointer "clk" > > should be released. It's the same when gm20b_clk_new() > > returns from elsewhere following this call. > This shouldn't be necessary. If a subdev constructor fails, and > returns a pointer, the core will call the destructor to clean things > up. > I'm not familiar with

On Mon, 1 Jun 2020 at 13:37, Ben Skeggs <skeggsb at gmail.com> wrote: > > On Mon, 1 Jun 2020 at 13:27, <dinghao.liu at zju.edu.cn> wrote: > > > > > > Hi Ben, > > > > > > When gk20a_clk_ctor() returns an error code, pointer "clk" > > > > should be released. It's the same when gm20b_clk_new() > > > > returns

On Tue, Jun 02, 2020 at 01:10:34PM +0200, Markus Elfring wrote: > > The original patch was basically fine. > > I propose to reconsider the interpretation of the software situation once more. > > * Should the allocated clock object be kept usable even after > a successful return from this function? Heh. You're right. The patch is freeing "clk" on the

> > I just found that clk is referenced by pclk in this function. When clk is freed, > > pclk will be allocated in gm20b_clk_new_speedo0(). Thus we should not release clk > > in this function and there is no bug here. > > Can there be a need to release a clock object after a failed gk20a_clk_ctor() call? > I think this mainly depends on pclk pointer. It seems that the

> > > For security, I will release this pointer only on error paths in this function. > > Do you tend to release objects (which are referenced by pointers)? > I just found that clk is referenced by pclk in this function. When clk is freed, pclk will be allocated in gm20b_clk_new_speedo0(). Thus we should not release clk in this function and there is no bug here. Thank you

> > > It's the same when gm20b_clk_new() returns from elsewhere following this call. > > I suggest to reconsider the interpretation of the software situation once more. > Can it be that the allocated clock object should be kept usable even after > a successful return from this function? > It's possible that we expect an usable clk pointer, though I could not

On Sat, 30 May 2020 at 19:42, Dinghao Liu <dinghao.liu at zju.edu.cn> wrote: > > When gk20a_clk_ctor() returns an error code, pointer "clk" > should be released. It's the same when gm20b_clk_new() > returns from elsewhere following this call. This shouldn't be necessary. If a subdev constructor fails, and returns a pointer, the core will call the destructor to

On Mon, 1 Jun 2020 at 13:27, <dinghao.liu at zju.edu.cn> wrote: > > > Hi Ben, > > > > When gk20a_clk_ctor() returns an error code, pointer "clk" > > > should be released. It's the same when gm20b_clk_new() > > > returns from elsewhere following this call. > > This shouldn't be necessary. If a subdev constructor fails, and >

> > If gk20a_clk_ctor() never returns such an error code, > > we may need not to release this clock object. > > Would you like to achieve complete exception handling > also for this function implementation? > It seems that it's possible to get -ENOMEM from gk20a_clk_ctor(). The call chain is as follows: gk20a_clk_ctor() <- nvkm_clk_ctor() <- nvkm_notify_init()

> On Tue, Jun 02, 2020 at 01:10:34PM +0200, Markus Elfring wrote: > > > The original patch was basically fine. > > > > I propose to reconsider the interpretation of the software situation once more. > > > > * Should the allocated clock object be kept usable even after > > a successful return from this function? > > Heh. You're right. The

The original patch was basically fine. Just add a Fixes tag and resend. regards, dan carpenter

> I just found that clk is referenced by pclk in this function. When clk is freed, > pclk will be allocated in gm20b_clk_new_speedo0(). Thus we should not release clk > in this function and there is no bug here. Can there be a need to release a clock object after a failed gk20a_clk_ctor() call? Regards, Markus

> If gk20a_clk_ctor() never returns such an error code, > we may need not to release this clock object. Would you like to achieve complete exception handling also for this function implementation? Regards, Markus

> The original patch was basically fine. I propose to reconsider the interpretation of the software situation once more. * Should the allocated clock object be kept usable even after a successful return from this function? * How much do ?destructor? calls matter here for (sub)devices? > Just add a Fixes tag and resend. This tag can help also. Regards, Markus

> Ben has explained this problem: > https://lore.kernel.org/patchwork/patch/1249592/ > Since the caller will check "pclk" on failure, we don't need to free > "clk" in gm20b_clk_new() and I think this patch is no longer needed. * I am curious if it can become easier to see the relationships for these variables according to mentioned ?destructor? calls. * Did

> It's possible that we expect an usable clk pointer, though I could not find > the exact usage yet. I am curious if another developer would like to add helpful background information. > For security, I will release this pointer only on error paths in this function. Do you tend to release objects (which are referenced by pointers)? Regards, Markus

> In this case, maybe we should check the return value of > gk20a_clk_ctor() and release clk if it returns -ENOMEM. All error situations (including failed memory allocations) can matter here. > And many other functions also have the same issue > (e.g. gm20b_clk_new_speedo0). I recommend to increase the error detection and improve the desired exception handling accordingly.

This series does some refactoring in the GK20A's volt and clk drivers (fixing a few things while we are at it) to let GM20B benefit from the GK20A's logic with which it is compatible. GM20B is capable of more sophisticated (and power-efficient) reclocking which will follow later. Even after this more fancy reclocking is merged, the present logic will remain used in the lowest speedo of

This series adds support for GM20B PLL's Maxwell features, namely glitchless switch and (more importantly) DFS support. DFS lets the PLL lower its output speed according to input current variations, making the clock more stable and allowing it to run safely at lower voltage. All GM20B additions are done in the last patch, which consequently ends up being considerably big ; fortunately, it