Re: [PATCH net-next v12 1/6] net: pcs: xpcs: add ANRESTART support for SGMII link recovery

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In xpcs_get_state_c37_sgmii() (drivers/net/pcs/pcs-xpcs.c lines
  1110-1121), the ANCMPLT latch is cleared with…

--

net: pcs: xpcs: add ANRESTART support for SGMII link recovery

This commit sets BMCR_ANRESTART along with BMCR_ANENABLE in
xpcs_config_aneg_c37_sgmii() and clears DW_VR_MII_AN_INTR_STS before
starting AN. It also makes xpcs_get_state_c37_sgmii() clear the ANCMPLT
latch and trigger an AN restart when the link is down and AN complete is
latched. This works around RK3568 XPCS, which does not restart AN on its
own.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index b415b93d77c15..6466e0ff2a98b 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c

[ ... ]

> @@ -1093,9 +1107,18 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
>  		return 0;
>  	}
>  
> -	/* Clear AN complete status or interrupt */
> -	if (state->an_complete)
> -		xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +	if (state->an_complete) {
> +		/* Clear AN complete status or interrupt */
> +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Initiate the next round of AN */
> +		ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
> +				  BMCR_ANRESTART);
> +		if (ret < 0)
> +			return ret;

[Severity: Low]
What happens if the DW_VR_MII_AN_INTR_STS write succeeds but the
xpcs_modify() of MII_BMCR fails?

At that point the ANCMPLT latch is already cleared. If the read fails,
__mdiobus_c45_modify_changed() returns without writing anything:

drivers/net/phy/mdio_bus.c:__mdiobus_c45_modify_changed() {
    ...
	ret = __mdiobus_c45_read(bus, addr, devad, regnum);
	if (ret < 0)
		return ret;
    ...
}

On hardware that needs this workaround, the latched ANCMPLT seems to be
the only thing that triggers the restart. Later calls to
xpcs_get_state_c37_sgmii() would see the link down with no ANCMPLT, so
the restart would never be retried.

xpcs_get_state() only logs the error:

	case DW_AN_C37_SGMII:
		ret = xpcs_get_state_c37_sgmii(xpcs, state);
		if (ret)
			dev_err(&xpcs->mdiodev->dev, "%s returned %pe\n",
				"xpcs_get_state_c37_sgmii", ERR_PTR(ret));
		break;

phylink_resolve() only reconfigures the PCS when the interface changes
or force_major_config is set:

	if ((mac_config && link_state.interface != pl->link_config.interface) ||
	    pl->force_major_config) {

Would the link then stay down until pcs_config runs again, for example
after an interface down/up?

The commit message says:

    If an MDIO access fails at this point, it indicates an
    unrecoverable hardware condition until reset.

Does that hold for RK3568? In the later patch "net: pcs: xpcs: add
Rockchip RK3568 platform glue driver", xpcs_rk_read_reg() and
xpcs_rk_write_reg() appear to fail only through
pm_runtime_resume_and_get(). A failed runtime resume callback does not
set the sticky power.runtime_error, so the failure could be transient
rather than permanent.

> +	}
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017054406.1828061-1-coiaprant%40gmail.com