Re: [PATCH net-next v3 2/3] net: airoha: npu: Add airoha_npu_soc_data struct

Lorenzo Bianconi
2025-10-14T09:23:45Z

[-- Attachment #1: Type: text/plain, Size: 3669 bytes --]

> On Mon, Oct 13, 2025 at 03:58:50PM +0200, Lorenzo Bianconi wrote:
>
> ...
>
> > @@ -182,49 +192,53 @@ static int airoha_npu_send_msg(struct airoha_npu *npu, int func_id,
> > return ret;
> > }
> >
> > -static int airoha_npu_run_firmware(struct device *dev, void __iomem *base,
> > - struct resource *res)
> > +static int airoha_npu_load_firmware(struct device *dev, void __iomem *addr,
> > + const struct airoha_npu_fw *fw_info)
> > {
> > const struct firmware *fw;
> > - void __iomem *addr;
> > int ret;
> >
> > - ret = request_firmware(&fw, NPU_EN7581_FIRMWARE_RV32, dev);
> > + ret = request_firmware(&fw, fw_info->name, dev);
> > if (ret)
> > return ret == -ENOENT ? -EPROBE_DEFER : ret;
> >
> > - if (fw->size > NPU_EN7581_FIRMWARE_RV32_MAX_SIZE) {
> > + if (fw->size > fw_info->max_size) {
> > dev_err(dev, "%s: fw size too overlimit (%zu)\n",
> > - NPU_EN7581_FIRMWARE_RV32, fw->size);
> > + fw_info->name, fw->size);
> > ret = -E2BIG;
> > goto out;
> > }
> >
> > - addr = devm_ioremap_resource(dev, res);
> > - if (IS_ERR(addr)) {
> > - ret = PTR_ERR(addr);
> > - goto out;
> > - }
> > -
> > memcpy_toio(addr, fw->data, fw->size);
> > +out:
> > release_firmware(fw);
> >
> > - ret = request_firmware(&fw, NPU_EN7581_FIRMWARE_DATA, dev);
> > - if (ret)
> > - return ret == -ENOENT ? -EPROBE_DEFER : ret;
> > + return ret;
> > +}
> >
> > - if (fw->size > NPU_EN7581_FIRMWARE_DATA_MAX_SIZE) {
> > - dev_err(dev, "%s: fw size too overlimit (%zu)\n",
> > - NPU_EN7581_FIRMWARE_DATA, fw->size);
> > - ret = -E2BIG;
> > - goto out;
> > - }
> > +static int airoha_npu_run_firmware(struct device *dev, void __iomem *base,
> > + struct resource *res)
> > +{
> > + const struct airoha_npu_soc_data *soc;
> > + void __iomem *addr;
> > + int ret;
> >
> > - memcpy_toio(base + REG_NPU_LOCAL_SRAM, fw->data, fw->size);
> > -out:
> > - release_firmware(fw);
> > + soc = of_device_get_match_data(dev);
> > + if (!soc)
> > + return -EINVAL;
> >
> > - return ret;
> > + addr = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(addr))
> > + return PTR_ERR(addr);
> > +
> > + /* Load rv32 npu firmware */
> > + ret = airoha_npu_load_firmware(dev, addr, &soc->fw_rv32);
> > + if (ret)
> > + return ret;
> > +
> > + /* Load data npu firmware */
> > + return airoha_npu_load_firmware(dev, base + REG_NPU_LOCAL_SRAM,
> > + &soc->fw_data);
>
> Hi Lorenzo,

Hi Simon,

>
> There are two calls to airoha_npu_load_firmware() above.
> And, internally, airoha_npu_load_firmware() will call release_firmware()
> if an error is encountered.
>
> But should release_firmware() be called for the firmware requested
> by the first call to airoha_npu_load_firmware() if the second call fails?
> Such clean-up seems to have been the case prior to this patch.

release_firmware() is intended to release the resources allocated by the
corresponding call to request_firmware() in airoha_npu_load_firmware().
According to my understanding we always run release_firmware() in
airoha_npu_load_firmware() before returning to the caller. Even before this
patch we run release_firmware() on the 'first' firmware image before requesting
the second one. Am I missing something?

>
> Also, not strictly related. Should release_firmware() be called (twice)
> when the driver is removed?

For the above reasons, it is not important to call release_firmware() removing
the module. Agree?

Regards,
Lorenzo

>
> > }
> >
> > static irqreturn_t airoha_npu_mbox_handler(int irq, void *npu_instance)
>
> ...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]