Re: [PATCH net] net: airoha: npu: load the firmware without the sysfs fallback

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
--- Patch [1]: [PATCH net] net: airoha: npu: load the firmware without the sysfs fallback ---
commit 1732abc5f78412a3a794eba72e4e0fbf37cf4fa7
Author: Vitaliy Sochnev <sochnev.v.74@gmail.com>

net: airoha: npu: load the firmware without the sysfs fallback

This switches airoha_npu_load_firmware() from request_firmware() to
request_firmware_direct() so that a missing firmware file is always
reported as -ENOENT and mapped to -EPROBE_DEFER, instead of running into
the sysfs/usermode-helper fallback and returning -ETIMEDOUT after the
loading timeout.

> diff --git a/drivers/net/ethernet/airoha/airoha_npu.c b/drivers/net/ethernet/airoha/airoha_npu.c
> index b679bed952de3..ca33e615a3f09 100644
> --- a/drivers/net/ethernet/airoha/airoha_npu.c
> +++ b/drivers/net/ethernet/airoha/airoha_npu.c
> @@ -202,7 +202,7 @@ static int airoha_npu_load_firmware(struct device *dev, void __iomem *addr,
>  	const struct firmware *fw;
>  	int ret;
>  
> -	ret = request_firmware(&fw, fw_name, dev);
> +	ret = request_firmware_direct(&fw, fw_name, dev);

[Severity: Low]
[Finding: main-1-0]
[Sources: opus-5]
Does this also drop the only message that names the firmware file when it
is genuinely absent or mis-pathed?

request_firmware_direct() passes FW_OPT_NO_WARN along with
FW_OPT_NOFALLBACK_SYSFS in drivers/base/firmware_loader/main.c:

	ret = _request_firmware(firmware_p, name, device, NULL, 0, 0,
				FW_OPT_UEVENT | FW_OPT_NO_WARN |
				FW_OPT_NOFALLBACK_SYSFS);

and _request_firmware() gates the file-naming warning on that flag:

		if (ret) {
			if (!(opt_flags & FW_OPT_NO_WARN))
				dev_warn(device,
					 "Direct firmware load for %s failed with error %d\n",
					 name, ret);

Inside fw_get_filesystem_firmware() the per-path -ENOENT report is only a
dev_dbg(), so nothing at default level mentions
airoha/en7581_npu_rv32.bin any more.

The driver side does not make up for it either: airoha_npu_probe() reports
through dev_err_probe(dev, err, "failed to run npu firmware\n"), and
dev_err_probe() logs at debug level for -EPROBE_DEFER, so a permanently
missing file leaves the device unbound with no default-level diagnostic
naming the file.

With CONFIG_MODULES=y the deferred probe timeout still emits one
"deferred probe pending: ... failed to run npu firmware" from
deferred_probe_timeout_work_func(), but that message does not name the
firmware file, and with driver_deferred_probe_timeout == 0 (the default
when !MODULES, or with deferred_probe_timeout=0 on the command line) even
that is absent.

Also worth noting: the kernel-doc for request_firmware_direct() scopes it
to "optional firmwares, which aren't always present", while this firmware
is mandatory for the probe to succeed. Would it be worth mentioning the
lost warning in the changelog, given the Cc: stable tag?

[Severity: Medium]
[Finding: main-1-1]
[Sources: opus-5]
Can this permanently break systems where the NPU firmware is only
delivered through the usermode helper?

fw_run_sysfs_fallback() bails out unconditionally on the flag that
request_firmware_direct() sets, in
drivers/base/firmware_loader/fallback.c:

	if ((opt_flags & FW_OPT_NOFALLBACK_SYSFS))
		return false;

So for this driver's only firmware request the helper delivery path is
gone for every invocation, not just for the boot-before-rootfs case the
change targets. If the file is not in the filesystem,
fw_get_filesystem_firmware() returns its initial -ENOENT,
airoha_npu_load_firmware() turns that into -EPROBE_DEFER, and
airoha_npu_probe() -> airoha_npu_run_firmware() ->
airoha_npu_load_firmware() re-defers forever, since the driver has no
other firmware source.

The configuration in question is exactly the one the changelog describes,
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y or force_sysfs_fallback armed,
with a helper that answers through the sysfs loading interface.

The changelog says:

  "The fallback is compiled in and forced in both cases; the patch does
   not disable it, it only keeps the driver from falling into it."

For this driver the helper path is disabled outright, including where it
is the only way the firmware arrives. Could that trade-off be spelled out
in the changelog, since the patch carries Cc: stable?
-- 
This is an AI-generated review.