search for: 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 a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c b/drivers/gpu/...

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 the behavior of the caller of gm20b_clk_new(). If th...

...mail.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 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...

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 clean things up. Ben. > > Signed-off-by: Dinghao Liu <dinghao.liu at zju.edu.cn> > --- > drivers/gpu/d...

> When gk20a_clk_ctor() returns an error code, pointer "clk" > should be released. Such an information is reasonable. > 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? Would you like to add the tag ?Fixes? to the commit mes...

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 > > returns a pointer, the core will call the destructor to clean things > > up. > > > > I'm not familiar with the behavior of the...

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

> > > 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 expe...

> 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 you notice opportunities to improve source code analysis (or software documentation) accordingly? Regards, Markus

> > > 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 for reminding me! Regards, Dinghao

> > 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 caller of gm20b_clk_new() always expec...

.... You're right. The patch is freeing "clk" on the success path so > that doesn't work. > 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. Regards, Dinghao

> 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

> 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. Just add a Fixes tag and resend. regards, dan carpenter

> 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

...() returns -ENOMEM, all of its callers (and callers of callers) will be influenced if there is a failed kzalloc inside which. In this case, maybe we should check the return value of gk20a_clk_ctor() and release clk if it returns -ENOMEM. And many other functions also have the same issue (e.g., gm20b_clk_new_speedo0). Do you have any idea about this problem? Regards, Dinghao

After observing random crashes on Tegra devices when patch "fb/gf100: defer DMA mapping of scratch page to oneinit() hook" was applied, I noticed that moving the 100c10 page allocation to the oneinit() hook resulted in that page being now allocated for Tegra as well, and accesses be made to members of gf100_fb which were not accessible because gk20a_fb_new() called nvkm_fb_new_()

.../nouveau/nvkm/engine/device/base.c b/drm/nouveau/nvkm/engine/device/base.c index 53d171729353..52d932d9df7c 100644 --- a/drm/nouveau/nvkm/engine/device/base.c +++ b/drm/nouveau/nvkm/engine/device/base.c @@ -2131,7 +2131,7 @@ nv12b_chipset = { .bar = gk20a_bar_new, .bus = gf100_bus_new, .clk = gm20b_clk_new, - .fb = gk20a_fb_new, + .fb = gm20b_fb_new, .fuse = gm107_fuse_new, .ibus = gk20a_ibus_new, .imem = gk20a_instmem_new, diff --git a/drm/nouveau/nvkm/subdev/fb/Kbuild b/drm/nouveau/nvkm/subdev/fb/Kbuild index edcc157e6ac8..ef47d57fcb87 100644 --- a/drm/nouveau/nvkm/subdev/fb/Kbuild +++ b/drm/...