Nouveau - Nov 2023 - [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

On Tue, 2023-11-07 at 19:11 +0100, Danilo Krummrich wrote:
I think the problem already starts with gf100_bar_new_() not setting its
pbar argument to NULL on failure, but this code assuming that.

Isn't it standard procedure for functions not to write to output parameters
on failure?

Generally, I think it would be better if all those functions would return
an ERR_PTR on failure.

Agreed.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20231107/f193b19b/attachment-0001.htm>