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

Simon Horman
2025-10-14T08:35:03Z

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,

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.

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

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

...