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
