Hi, Timur,
On Fri, Jun 05, 2026 at 06:22:41PM +0000, Timur Tabi wrote:
> Ah yes, you're right.
>
> So now I think a better fix might be to have two different `blob`
> variables, so that there is no longer any confusion. Because right now,
> the nvkm_firmware_put() call at the end of the function releases a
> different `blob` depending on whether `bl` is NULL or not.
>
> What do you think about this:
>
> nvkm_firmware_put(blob);
> if (bl) {
> const struct firmware *blob_bl;
>
> ret = nvkm_firmware_load_name(subdev, bl, "", ver, &blob_bl);
> if (ret)
> goto done;
> ...
> nvkm_firmware_put(blob_bl);
> if (!fw->boot)
> ret = -ENOMEM;
> } else {
> fw->boot_addr = fw->nmem_base;
> }
>
> done:
> if (ret)
> nvkm_falcon_fw_dtor(fw);
>
> return ret;
Yes, using a separate pointer `blob_bl` for the bootloader firmware is a
cleaner approach.
However, we must keep the final nvkm_firmware_put(blob) under the done
label. Moving it earlier would cause memory leaks in prior error paths
like nvkm_falcon_fw_ctor(), which jump directly to done.
A safer approach is to manage blob_bl locally inside the if (bl) block,
while leaving the original blob cleanup at the end.
What do you think about this:
if (bl) {
const struct firmware *blob_bl;
ret = nvkm_firmware_load_name(subdev, bl, "", ver, &blob_bl);
if (ret)
goto done;
...
nvkm_firmware_put(blob_bl);
if (!fw->boot)
ret = -ENOMEM;
} else {
fw->boot_addr = fw->nmem_base;
}
done:
if (ret)
nvkm_falcon_fw_dtor(fw);
nvkm_firmware_put(blob);
return ret;
Regards,
Dawei
