[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aO5VqDegyOY9fVEK@lore-desk>
Date: Tue, 14 Oct 2025 15:52:40 +0200
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Simon Horman <horms@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net-next v3 2/3] net: airoha: npu: Add
airoha_npu_soc_data struct
On Oct 14, Simon Horman wrote:
> On Tue, Oct 14, 2025 at 11:23:37AM +0200, Lorenzo Bianconi wrote:
> > > 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?
>
> Thanks, agreed.
>
> For some reason I missed that release_firmware() is called in
> airoha_npu_load_firmware() regardless of error - I thought it was only
> in error paths for some reason.
>
> So I agree that the firmware is always released by the time
> airoha_npu_load_firmware() is returned. As thus there is never
> a need to release it afterwards.
>
> Reviewed-by: Simon Horman <horms@...nel.org>
>
>
ack, thx for the review.
Regards,
Lorenzo
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists