Re: [PATCH net] net: mii: Check speed and duplex only when autoneg is off

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] This fix only covers the autoneg case of the read-modify-write
  (RMW) breakage caused by f6f2e946aa4d.
- [Medium] After this patch, mii_ethtool_set_link_ksettings() accepts any
  base.speed and base.duplex when autoneg is AUTONEG_ENABLE.

--

net: mii: Check speed and duplex only when autoneg is off

This patch moves the speed and duplex checks in
mii_ethtool_set_link_ksettings() under the AUTONEG_DISABLE case. With
that change, enabling autoneg on a port with no link no longer fails
with -EINVAL. Before, it failed because the getter reports
SPEED_UNKNOWN.

> diff --git a/drivers/net/mii.c b/drivers/net/mii.c
> index 37bc3131d31a8..4394446375373 100644
> --- a/drivers/net/mii.c
> +++ b/drivers/net/mii.c
> @@ -333,12 +333,6 @@ int mii_ethtool_set_link_ksettings(struct mii_if_info *mii,
>  	struct net_device *dev = mii->dev;
>  	u32 speed = cmd->base.speed;
>  
> -	if (speed != SPEED_10 &&
> -	    speed != SPEED_100 &&
> -	    speed != SPEED_1000)
> -		return -EINVAL;
> -	if (cmd->base.duplex != DUPLEX_HALF && cmd->base.duplex != DUPLEX_FULL)
> -		return -EINVAL;
>  	if (cmd->base.port != PORT_MII)
>  		return -EINVAL;
>  	if (cmd->base.phy_address != mii->phy_id)
> @@ -346,8 +340,19 @@ int mii_ethtool_set_link_ksettings(struct mii_if_info *mii,
>  	if (cmd->base.autoneg != AUTONEG_DISABLE &&
>  	    cmd->base.autoneg != AUTONEG_ENABLE)
>  		return -EINVAL;
> -	if ((speed == SPEED_1000) && (!mii->supports_gmii))
> -		return -EINVAL;
> +
> +	/* Speed is SPEED_UNKNOWN while the link is down; autoneg ignores it */
> +	if (cmd->base.autoneg == AUTONEG_DISABLE) {
> +		if (speed != SPEED_10 &&
> +		    speed != SPEED_100 &&
> +		    speed != SPEED_1000)
> +			return -EINVAL;

[Severity: High]

Do forced-media ports with no link still hit the same -EINVAL?

mii_ethtool_get_link_ksettings() sets the speed to SPEED_UNKNOWN after
both branches. In forced mode, that replaces the speed the
AUTONEG_DISABLE branch has just read from BMCR:

drivers/net/mii.c:mii_ethtool_get_link_ksettings() {
    ...
	} else {
		cmd->base.autoneg = AUTONEG_DISABLE;

		cmd->base.speed = ((bmcr & BMCR_SPEED1000 &&
				    (bmcr & BMCR_SPEED100) == 0) ?
				   SPEED_1000 :
				   ((bmcr & BMCR_SPEED100) ?
				    SPEED_100 : SPEED_10));
		...
	}

	if (!(bmsr & BMSR_LSTATUS))
		cmd->base.speed = SPEED_UNKNOWN;
    ...
}

So a request that leaves the speed alone sends SPEED_UNKNOWN back. One
example is "ethtool -s eth0 duplex half" on a forced-media port with no
cable.

On the netlink path, ethnl_update_linkmodes() only updates
lsettings->speed when ETHTOOL_A_LINKMODES_SPEED is present. The ioctl
read-modify-write path behaves the same way.

That SPEED_UNKNOWN then fails the check above and returns -EINVAL.
Before f6f2e946aa4d the same command worked, because the getter reported
the BMCR speed in forced mode.

Since this carries the f6f2e946aa4d Fixes: tag and Cc: stable, should
the forced case be covered too? phylib reports the forced speed and
duplex even when the link is down, and only reports SPEED_UNKNOWN under
autoneg. Would doing the same in the getter's AUTONEG_DISABLE branch fix
the remaining case?

> +		if (cmd->base.duplex != DUPLEX_HALF &&
> +		    cmd->base.duplex != DUPLEX_FULL)
> +			return -EINVAL;
> +		if ((speed == SPEED_1000) && (!mii->supports_gmii))
> +			return -EINVAL;
> +	}

[Severity: Medium]

With AUTONEG_ENABLE, any base.speed and base.duplex now pass this
function. mii_ethtool_sset() in the same file still rejects those values
in every mode:

drivers/net/mii.c:mii_ethtool_sset() {
    ...
	if (speed != SPEED_10 &&
	    speed != SPEED_100 &&
	    speed != SPEED_1000)
		return -EINVAL;
	if (ecmd->duplex != DUPLEX_HALF && ecmd->duplex != DUPLEX_FULL)
		return -EINVAL;
    ...
}

Can this leave the pch_gbe PHY without the requested advertisement?
pch_gbe_set_link_ksettings() only rewrites speed == UINT_MAX. It then
stores whatever this function accepted:

drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c:pch_gbe_set_link_ksettings() {
    ...
	ret = mii_ethtool_set_link_ksettings(&adapter->mii, &copy_ecmd);
	...
	hw->mac.link_speed = speed;
	hw->mac.link_duplex = copy_ecmd.base.duplex;
    ...
}

Some requests used to get -EINVAL and now succeed. One is
"ethtool -s ethX autoneg on speed 2500 advertise 0x2f", where the
explicit advertise mask skips ethnl_auto_linkmodes(). Another is an
out-of-range duplex sent through raw netlink or ioctl.

When the interface is not running, the path is:

pch_gbe_set_link_ksettings()
  pch_gbe_reset()
    pch_gbe_phy_init_setting()
      pch_gbe_phy_write_reg_miic(hw, MII_BMCR, BMCR_RESET);
      mii_ethtool_sset()   <- returns -EINVAL

pch_gbe_phy_init_setting() passes the stored hw->mac.link_speed and
hw->mac.link_duplex to mii_ethtool_sset() right after the PHY reset.
mii_ethtool_sset() rejects them and "Error: mii_ethtool_sset" is logged.

The requested advertisement is never written to the freshly reset PHY,
even though the ethtool call has already returned success.

The stored link_speed can also reach pch_gbe_set_rgmii_ctrl() and
pch_gbe_set_mode() in __pch_gbe_suspend(). This happens if the watchdog
never overwrites it, and the watchdog only does that once the link comes
up.

Should mii_ethtool_set_link_ksettings() and mii_ethtool_sset() stay
consistent here?

>  
>  	/* ignore supported, maxtxpkt, maxrxpkt */
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106195544.4146568-1-anthony.pighin%40nokia.com