Re: [PATCH net-next v2 5/7] net: phy: adin1100: Skip the soft reset on ADIN1110/ADIN2111

[netdev-bot+sashiko@kernel.org]

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

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

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] The commit message says the whole-chip CRSM_SFT_RST is "not an
  issue with the generic SPI protocol" because adin1110 configures the…
- [Low] The patch justifies dropping .soft_reset with "The MAC driver
  already resets the chip before attaching the PHY".

--

net: phy: adin1100: Skip the soft reset on ADIN1110/ADIN2111

This patch removes .soft_reset from the ADIN1110 and ADIN2111 entries in
the adin1100 PHY driver. On these MAC-PHYs, CRSM_SFT_RST resets the whole
chip, which would undo the configuration that the upcoming OPEN Alliance
TC6 support does in oa_tc6_init() before the PHY is attached.

> This is not an issue with the generic SPI protocol, where the adin1110
> driver configures the MAC in ndo_open, after phy_connect(). The OPEN
> Alliance TC6 protocol support, added in a following patch, configures
> the device in oa_tc6_init() before the PHY is attached, and the
> configuration would be lost.

[Severity: Medium]
Does this hold on the resume path as well? phy_init_hw() is not only
called at attach time. mdio_bus_phy_resume() also calls it when the PHY
was suspended by the MDIO bus and mac_managed_pm is not set:

drivers/net/phy/phy_device.c:mdio_bus_phy_resume() {
    ...
	ret = phy_init_hw(phydev);
    ...
}

adin1110.c has no dev_pm_ops and never sets mac_managed_pm. The adin1100
driver provides .suspend, so on system resume with the interface up, the
sequence before this patch was:

mdio_bus_phy_resume()
  phy_init_hw()
    adin_soft_reset()
      CRSM_SFT_RST (whole-chip reset)

Would that wipe what adin1110_net_open() programmed: CONFIG2 CRC_APPEND,
the IMASK1 interrupt mask, the MAC address filter slots and CONFIG1 SYNC?
Nothing in adin1110 writes them back. The link can then come up while
the MAC is still at reset defaults, and it stays that way until the
interface is taken down and up.

If so, this patch also fixes an existing resume regression on the generic
SPI path. Could the commit message describe that? And should it carry a
Fixes: tag so stable picks it up? The tag would probably be:

Fixes: 875b718ac380 ("net: phy: adin1100: add PHY IDs of adin1110/adin2111")

> diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
> index b946cb86b6c41..ddb91b0e7081f 100644
> --- a/drivers/net/phy/adin1100.c
> +++ b/drivers/net/phy/adin1100.c
> @@ -341,7 +341,6 @@ static struct phy_driver adin_driver[] = {
>  		PHY_ID_MATCH_MODEL(PHY_ID_ADIN1110),
>  		.name			= "ADIN1110",
>  		.get_features		= adin_get_features,
> -		.soft_reset		= adin_soft_reset,

[Severity: Low]
With .soft_reset gone, does anything on the generic SPI path still wait
for the chip reset to finish before the PHY is accessed?

The commit message says "The MAC driver already resets the chip before
attaching the PHY". However, adin1110_probe() writes the reset and goes
straight to MDIO bus registration:

drivers/net/ethernet/adi/adin1110.c:adin1110_probe() {
    ...
	ret = adin1110_write_reg(priv, ADIN1110_RESET, ADIN1110_SWRESET);
	if (ret < 0)
		return ret;

	ret = adin1110_register_mdiobus(priv, dev);
    ...
}

There is no sleep and no poll of STATUS0.RESETC there. The commit message
itself says MDIO reads return stale data until the reset completes. The
oa_tc6 patch in this series ("net: ethernet: oa_tc6: Allow custom reset
settle time values") says the ADIN1110 can need up to 10 ms after a
software reset.

Before this patch, phy_connect()->phy_attach_direct()->phy_init_hw()->
adin_soft_reset() polled for SYS_RDY:

	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, ADIN_CRSM_STAT, ret,
					 (ret & ADIN_CRSM_SYS_RDY),
					 10000, 30000, true);

That was the only readiness check on this path. phy_init_hw() now goes
straight to config_intr(), which does a read-modify-write of
ADIN_PHY_SUBSYS_IRQ_MASK. phy_attach_direct() then calls adin_resume().
Either of these can run inside the reset window.

The practical impact looks small, since the MDIO scan already raced this
reset before the patch.

The TC6 path handles this in oa_tc6_sw_reset_macphy(): it calls fsleep()
for the settle time and then polls RESETC. Could adin1110_probe() do the
same before calling adin1110_register_mdiobus()?

>  		.probe			= adin_probe,
>  		.config_aneg		= adin_config_aneg,
>  		.read_status		= adin_read_status,

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109085952.105331-1-iustin.stolniceanu%40analog.com