Re: [PATCH net-next v1 2/3] net: phy: realtek: add firmware for RTL8261D

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

- [High] net: phy: realtek: making rtl_nic/rtl8261d.bin a hard
  requirement silently breaks RTL8261D hardware that works today.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the RTL8261D
  path newly enabled here inherits it: priv->fw_loaded is never…

--

net: phy: realtek: add firmware for RTL8261D

This adds a firmware file name and a per-part sub magic value for the
RTL8261D, and makes rtl8261x_verify_firmware() compare the header sub
magic against the value stored in the driver private data instead of the
hardcoded RTL8261C value.

> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index d0d09e8e8f5c5..61e6029489313 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c
> @@ -286,11 +286,15 @@
>  
>  #define FW_MAIN_MAGIC			0x52544C38
>  #define FW_SUB_MAGIC_8261C		0x32363143
> +#define FW_SUB_MAGIC_8261D		0x32363144
>  #define RTL8261X_POLL_TIMEOUT_MS	100
>  #define RTL8261X_MAX_MMD_DEV		31
>  
>  #define RTL8261C_CE_FW_NAME	"rtl_nic/rtl8261c.bin"
> +#define RTL8261D_FW_NAME	"rtl_nic/rtl8261d.bin"
> +
>  MODULE_FIRMWARE(RTL8261C_CE_FW_NAME);
> +MODULE_FIRMWARE(RTL8261D_FW_NAME);

[ ... ]

> @@ -411,10 +416,13 @@ static int rtl8261x_probe(struct phy_device *phydev)
>  	switch (sub_phy_id) {
>  	case RTL8261C_CE_MODEL:
>  		priv->fw_name = RTL8261C_CE_FW_NAME;
> +		priv->fw_sub_magic = FW_SUB_MAGIC_8261C;
>  		phydev_info(phydev, "RTL8261C detected (sub_id 0x%02x)\n", sub_phy_id);
>  		break;
>  
>  	case RTL8261D_MODEL:
> +		priv->fw_name = RTL8261D_FW_NAME;
> +		priv->fw_sub_magic = FW_SUB_MAGIC_8261D;
>  		phydev_info(phydev, "RTL8261D detected (sub_id 0x%02x)\n", sub_phy_id);
>  		break;

[Severity: High]
Does setting priv->fw_name for RTL8261D turn rtl_nic/rtl8261d.bin into a
hard requirement for the part to work at all?

Before this patch this switch arm only printed the info line, so fw_name
stayed NULL from the devm_kzalloc() in rtl8261x_probe() and
rtl8261x_config_init() returned 0 without touching the firmware loader:

	/* The firmware parameters are preserved across IEEE soft resets and
	 * suspend/resume cycles. ...
	 */
	if (priv->fw_name && !priv->fw_loaded)
		return rtl8261x_fw_load(phydev);

	return 0;

With fw_name set, rtl8261x_fw_load() returns the request_firmware() error
verbatim:

	ret = request_firmware(&fw, priv->fw_name, &phydev->mdio.dev);
	if (ret) {
		phydev_err(phydev, "Failed to load firmware %s: %d\n", priv->fw_name, ret);
		return ret;
	}

and that error is fatal all the way up:

rtl8261x_config_init() -> phy_init_hw() -> phy_attach_direct()

phy_init_hw() does "ret = phydev->drv->config_init(phydev); if (ret < 0)
return ret;" and phy_attach_direct() does "err = phy_init_hw(phydev); if
(err) goto error;".

So on a board with an RTL8261D that links today, is a missing or unreadable
rtl8261d.bin (-ENOENT, or -EAGAIN when the loader/rootfs is not ready yet)
now enough to fail PHY attach so the interface never comes up?  MODULE_FIRMWARE()
only advertises the name to initramfs tooling, it does not make the blob exist.

Two follow-ups:

Has rtl8261d.bin been accepted into linux-firmware?  The commit message
("Add support for RTL8261D to load firmware") does not mention it.

Would firmware_request_nowarn() plus treating -ENOENT as non-fatal be
preferable here, so the part keeps falling back to its power-on/OTP defaults
as it does today?

For reference, the later patch in the series ("net: phy: realtek: add support
for RTL8261D_VM") does not change this behaviour: the RTL8261D_FW_NAME
assignment stays unconditional and config_init() still returns the loader
error.

[Severity: Medium]
This isn't a bug introduced by this patch, but routing RTL8261D through the
same mechanism extends it to the D part: priv->fw_loaded is never
invalidated across a PHY hardware reset, so the reload that the comment in
rtl8261x_config_init() calls for never happens.

rtl8261x_fw_load() sets the flag on success:

	priv->fw_loaded = true;

and nothing clears it again; the only zeroing is the devm_kzalloc() in
rtl8261x_probe(), i.e. once per driver bind.  Meanwhile
rtl8261x_config_init() states:

	/* The firmware parameters are preserved across IEEE soft resets and
	 * suspend/resume cycles. Reloading is only necessary after a power
	 * cycle or hard reset.
	 */
	if (priv->fw_name && !priv->fw_loaded)
		return rtl8261x_fw_load(phydev);

On boards that describe a PHY reset line, phy_detach_internal() only calls
device_release_driver() when phydev->is_genphy_driven is set, so a dedicated
driver such as this one stays bound with its private data intact, and then:

phy_detach_internal()
	phy_device_reset(phydev, 1);	/* reset asserted, fw_loaded survives */

A later re-attach goes:

phy_attach_direct() -> phy_init_hw()
	phy_device_reset(phydev, 0);
	ret = phydev->drv->config_init(phydev);

and config_init() sees fw_loaded still true and skips the load, so the PHY
runs on power-on defaults while initialization is reported as successful.

Should fw_loaded be cleared where the reset is asserted, or dropped in favour
of always loading in config_init()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916081444.497-1-javen_xu%40realsil.com.cn