Dan Carpenter
2023-Nov-07 15:17 UTC
[Nouveau] [bug report] drm/nouveau/mmu/r535: initial support
On Tue, Nov 07, 2023 at 03:06:27PM +0000, Timur Tabi wrote:> On Tue, 2023-11-07 at 17:32 +0300, Dan Carpenter wrote: > > ??? 170???????? ret = gf100_bar_new_(rm, device, type, inst, &bar); > > --> 171???????? *pbar = bar; > > ??? 172???????? if (ret) { > > ??? 173???????????????? if (!bar) > > ??????????????????????????? ^^^^ > > If gf100_bar_new_() fails then bar isn't initialized.? Do we really > > need to initialize bar to NULL on error?? If so then we should do it > > before the "rm = kzalloc()". > > We can just do this: > > struct nvkm_bar *bar = NULL;I mean that will silence the warning, but why are we setting *pbar to NULL? If it's necessary then there is still a bug because the first error path doesn't do it. If not, then just do: ret = gf100_bar_new_(rm, device, type, inst, &bar); if (ret) { kfree(rm); return ret; } *pbar = bar; It really depends on what we're doing with *pbar. I looked at the context before I sent the bug report and it kind of looked like this function is dead code honestly... regards, dan carpenter
Danilo Krummrich
2023-Nov-07 18:11 UTC
[Nouveau] [bug report] drm/nouveau/mmu/r535: initial support
On 11/7/23 16:17, Dan Carpenter wrote:> On Tue, Nov 07, 2023 at 03:06:27PM +0000, Timur Tabi wrote: >> On Tue, 2023-11-07 at 17:32 +0300, Dan Carpenter wrote: >>> ??? 170???????? ret = gf100_bar_new_(rm, device, type, inst, &bar); >>> --> 171???????? *pbar = bar; >>> ??? 172???????? if (ret) { >>> ??? 173???????????????? if (!bar) >>> ??????????????????????????? ^^^^ >>> If gf100_bar_new_() fails then bar isn't initialized.? Do we really >>> need to initialize bar to NULL on error?? If so then we should do it >>> before the "rm = kzalloc()". >> >> We can just do this: >> >> struct nvkm_bar *bar = NULL; > > I mean that will silence the warning, but why are we setting *pbar to > NULL? If it's necessary then there is still a bug because the first > error path doesn't do it.I think the problem already starts with gf100_bar_new_() not setting its pbar argument to NULL on failure, but this code assuming that. Generally, I think it would be better if all those functions would return an ERR_PTR on failure. However, the common pattern in nvkm seems to be that one can't trust those pointer arguments on failure. Hence, your code below seems to be the correct fix.> If not, then just do: > > ret = gf100_bar_new_(rm, device, type, inst, &bar); > if (ret) { > kfree(rm); > return ret; > } > *pbar = bar; > > It really depends on what we're doing with *pbar. I looked at the > context before I sent the bug report and it kind of looked like this > function is dead code honestly...It's used by tu102_bar_new() which is used by the chipset structures in drivers/gpu/drm/nouveau/nvkm/engine/device/base.c. On driver initialization there are a few magic macros calling all those function pointers from the chipset structures. [1] [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c#L3220> > regards, > dan carpenter