Re: [PATCH net-next v2] net: phy: aquantia: Poll global status register for provision ID

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In aqr_wait_reset_complete()
  (drivers/net/phy/aquantia/aquantia_main.c:708-721), the new second…
- [Medium] net: phy: aquantia: propagate MDIO read error in
  aqr_wait_reset_complete()
- [Low] The comment above aqr_wait_reset_complete()
  (aquantia_main.c:684-690) was not updated.
- [Low] On a provisioning-ID timeout, aqr_wait_reset_complete()
  (aquantia_main.c:717-719) prints 'Firmware provisioning ID not ready;…
- [Low] net: phy: aquantia: missing Fixes trailer for
  aqr_wait_reset_complete() fix

--

net: phy: aquantia: Poll global status register for provision ID

This commit adds a second poll to aqr_wait_reset_complete(). Once the
firmware ID shows up, the poll waits for the provisioning ID field in
VEND1_GLOBAL_RSVD_STAT1 to become non-zero. If the poll times out, it
logs a warning and the function still returns 0. The stated reasons
are link failures seen on AQR113 and correct fingerprinting of AQR412C
MUSX firmware.

> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> index e95d391a6dc23..bed891cfaaa1b 100644
> --- a/drivers/net/phy/aquantia/aquantia_main.c
> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> @@ -702,7 +702,23 @@ int aqr_wait_reset_complete(struct phy_device *phydev)
>  		return val;
>  	}
>  
> -	return ret;
> +	if (ret)
> +		return ret;
> +
> +	ret = read_poll_timeout(phy_read_mmd, val,
> +				val < 0 ||
> +				FIELD_GET(VEND1_GLOBAL_RSVD_STAT1_PROV_ID, val),
> +				AQR_FW_WAIT_SLEEP_US, AQR_FW_WAIT_TIMEOUT_US,
> +				false, phydev, MDIO_MMD_VEND1,
> +				VEND1_GLOBAL_RSVD_STAT1);

[Severity: Low]
The commit message describes two problems: link failures seen during
AQR113 bring-up, and an AQR412C MUSX fingerprint that can't be built
correctly. Both come from aqr_wait_reset_complete() treating a
non-zero VEND1_GLOBAL_FW_ID as the end of chip initialization, before
the provisioning ID in VEND1_GLOBAL_RSVD_STAT1 is set.

If this is meant as a bug fix, should it carry a Fixes tag? The
FW_ID-only poll appears to go back to commit 43429a0353af ("net: phy:
aquantia: report PHY details like firmware version"). That commit
added aqr107_wait_reset_complete(), which was later renamed to
aqr_wait_reset_complete().

Without the tag, stable maintainers and backporters have no easy way
to tell that this change addresses a problem in released kernels. And
if it is a fix, should it go to net rather than net-next?

[Severity: Medium]
This poll only exits on a non-zero PROV_ID or a read error. If a
firmware image reports PROV_ID == 0, will every call sleep the full
AQR_FW_WAIT_TIMEOUT_US (2 s) and then return 0 anyway?

The commit message says "not all PHYs require a valid provision ID",
which suggests PROV_ID can stay 0 on some images.

In the driver, prov_id only matters for one fingerprint:

  AQR_G3_V4_3_C_AQR_NXP_SPF_30841_MUSX_ID40019_VER1198
      AQR_FW_FINGERPRINT(4, 3, 0xc, 1, 40019, 1198)

That fingerprint is only checked for PHY_ID_AQR412C, in
aqr_translate_interface().

aqr_wait_reset_complete() has two callers, and both now pay for this
wait:

  aqr107_probe()
    aqr_firmware_load()
      aqr_wait_reset_complete()

  phy_attach_direct() / mdio_bus_phy_resume()
    phy_init_hw()
      aqr_gen1_config_init()
        aqr_wait_reset_complete()

With such an image, would each port lose about 4 s across probe and
attach, and then 2 s plus a warning on every later attach and resume?
On quad-port parts like AQR412/AQR412C, that cost applies to each port.

Before this patch, both paths returned as soon as FW_ID was non-zero.
Could the PROV_ID wait be limited to the PHYs or firmware images that
need it?

> +	if (val < 0)
> +		phydev_warn(phydev, "Failed to read VEND1_GLOBAL_RSVD_STAT1: %pe\n",
> +			    ERR_PTR(val));
> +	else if (ret)
> +		phydev_warn(phydev, "Firmware provisioning ID not ready; "
> +			    "interface mode detection may be incorrect\n");

[Severity: Medium]
If phy_read_mmd() fails while polling VEND1_GLOBAL_RSVD_STAT1, the
val < 0 term ends read_poll_timeout() right away with ret == 0. The
code then logs a warning and returns 0, so the MDIO error is lost.

The VEND1_GLOBAL_FW_ID poll just above handles the same case with
phydev_err() and returns val. Is it intended that a read error here
is non-fatal? The commit message only says that the timeout case
should not fail probe.

Both callers act on the return value:

  aqr107_probe()
    aqr_firmware_load()
      aqr_wait_reset_complete()

aqr_firmware_load() treats 0 as "Some firmware is loaded => do
nothing". Any other negative value goes to the default branch, which
passes the PHY read error back to aqr107_probe(). With this change, an
MDIO failure on RSVD_STAT1 looks like ready firmware, and probe goes
on.

  phy_init_hw()
    aqr_gen1_config_init()
      aqr_wait_reset_complete()

aqr_gen1_config_init() only builds the fingerprint when the return
value is 0. After a read error it now goes on to
aqr_build_fingerprint() and the rest of the PHY configuration, even
though the provisioning ID was never seen.

Could the val < 0 case return val, the same way the FW_ID poll does,
and leave only the timeout as a warning?

[Severity: Low]
This warning can fire on every model that uses aqr107_probe() and
aqr_gen1_config_init(), such as AQR105, AQR107, AQCS109, AQR111,
AQR112, AQR113, AQR114C, AQR115 and AQR813. Is "interface mode
detection may be incorrect" accurate for those?

The provisioning ID only affects the interface mode in
aqr_translate_interface():

	if (phy_id_compare(phydev->drv->phy_id, PHY_ID_AQR412C, phydev->drv->phy_id_mask) &&
	    priv->fingerprint == AQR_G3_V4_3_C_AQR_NXP_SPF_30841_MUSX_ID40019_VER1198 &&
	    interface == PHY_INTERFACE_MODE_USXGMII)
		return PHY_INTERFACE_MODE_10G_QXGMII;

So on other models, a missing provisioning ID can't cause wrong
interface detection.

The message also doesn't mention the link failures that the commit
message describes for AQR113.

> +
> +	return 0;
>  }

[Severity: Low]
This isn't a bug, but the comment above aqr_wait_reset_complete() was
not updated:

/* If we configure settings whilst firmware is still initializing the chip,
 * then these settings may be overwritten. Therefore make sure chip
 * initialization has completed. Use presence of the firmware ID as
 * indicator for initialization having completed.
 * The chip also provides a "reset completed" bit, but it's cleared after
 * read. Therefore function would time out if called again.
 */

It still names FW_ID as the only sign that initialization is complete.
It doesn't mention the second, non-fatal wait on the provisioning ID.

Also, on PHYs where PROV_ID stays 0, every call from
aqr_firmware_load() and aqr_gen1_config_init() now times out again.
Isn't that the repeated-call timeout this comment says the design
avoids?

Could the comment be updated to match the new behavior?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-aquantia-poll-global-status-register-v2-1-b602fc7e80a9%40oss.qualcomm.com