[Nouveau] [bug report] drm/nouveau/mmu/r535: initial support

[Danilo Krummrich]

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