On Mon, 2026-06-08 at 21:52 +0800, Dawei Feng wrote:
> Hi Timur,
>
> On Fri, Jun 05, 2026 at 06:22:41PM +0000, Timur Tabi wrote:
> > I think it would be cleaner to instead delete this
> > nvkm_firmware_put(blob) call here, and just rely on the call to
> > nvkm_firmware_put() at the end of nvkm_falcon_fw_ctor_hs(). Then you
> > won't need "blob = NULL".
>
> Thanks for your review.
>
> I don't think we can drop the nvkm_firmware_put(blob) here. At this
> point, blob still points to the image firmware loaded at the beginning of
> nvkm_falcon_fw_ctor_hs(). The later nvkm_firmware_load_name(..., &blob)
> call overwrites blob with the bootloader firmware pointer on success.
>
> If we only rely on the final nvkm_firmware_put(blob), the success path
> would release the bootloader firmware, but the original image firmware
> pointer would be lost and leaked.
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;
This way, there is no confusion between the two blobs, and the bootloader blob
exists only inside
the if-block that needs it.
