Re: [PATCH net-next 3/5] net: rnpgbe: Support copper link configuration

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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 the forced (autoneg disabled) branch of
  rnpgbe_set_link_ksettings(), `mode` is computed from…
- [Low] The commit message says only that set_link_ksettings support is
  added for copper ports.
- [Low] rnpgbe_set_link_ksettings() never checks the requested
  cmd->base.port or cmd->base.phy_address.
- [Low] When the requested autoneg is disabled,
  rnpgbe_set_link_ksettings() never reads or validates…

--

net: rnpgbe: Support copper link configuration

This adds a set_link_ksettings ethtool callback for copper ports in the
rnpgbe driver. It covers autonegotiation, advertised link modes, forced
10/100 Mb/s speed and duplex, and MDI/MDI-X selection. It also switches
mucse_mbx_set_link() to send PHY_LINK_SET through
mucse_write_mbx_coalesce_event().

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_ethtool.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_ethtool.c
> index 3ff01a15961e3..cd3cca1474ec1 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_ethtool.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_ethtool.c
> @@ -131,6 +131,93 @@ static int rnpgbe_get_link_ksettings(struct net_device *netdev,

[ ... ]

> +static int rnpgbe_set_link_ksettings(struct net_device *netdev,
> +				     const struct ethtool_link_ksettings *cmd)
> +{

[ ... ]

> +	err = rnpgbe_get_link_ksettings(netdev, &settings);
> +	if (err)
> +		return err;
> +	if (settings.base.port != PORT_TP)
> +		return -EOPNOTSUPP;

[Severity: Low]
This tests the port in the driver's own freshly fetched settings, not the
requested cmd->base.port. Should cmd->base.port and cmd->base.phy_address
also be checked here?

ethnl_set_linkinfo() in net/ethtool/linkinfo.c loads the current settings,
applies the user's changes, and passes the result to the driver:

    ethnl_update_u8(&lsettings->port, tb[ETHTOOL_A_LINKINFO_PORT], &mod);
    ethnl_update_u8(&lsettings->phy_address, tb[ETHTOOL_A_LINKINFO_PHYADDR],
                    &mod);

So "ethtool -s ethX port fibre" or "ethtool -s ethX phyad N" on a copper
port seems to return success. The requested port and PHY address are
silently dropped, and PHY_LINK_SET is still sent with the other fields. A
later get still reports PORT_TP and the old phy_addr.

For comparison, phy_ethtool_ksettings_set() rejects a phy_address mismatch
with -EINVAL.

> +
> +	supported = rnpgbe_fw_link_modes(settings.link_modes.supported);
> +	advertising = rnpgbe_fw_link_modes(settings.link_modes.advertising);
> +	if (autoneg) {

[ ... ]

> +	} else {
> +		if (cmd->base.duplex != DUPLEX_FULL &&
> +		    cmd->base.duplex != DUPLEX_HALF)
> +			return -EINVAL;
> +		switch (cmd->base.speed) {
> +		case SPEED_10:
> +			mode = cmd->base.duplex == DUPLEX_FULL ?
> +				RNPGBE_FW_ADV_10_FULL : RNPGBE_FW_ADV_10_HALF;
> +			break;
> +		case SPEED_100:
> +			mode = cmd->base.duplex == DUPLEX_FULL ?
> +				RNPGBE_FW_ADV_100_FULL : RNPGBE_FW_ADV_100_HALF;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		if (!(supported & mode))
> +			return -EINVAL;
> +	}

[Severity: Medium]
In the forced branch, mode is used only for the supported check and is
never assigned to advertising. Is it intended that advertising keeps the
old cached mask from settings.link_modes.advertising?

That mask can still contain RNPGBE_FW_ADV_1000_FULL and other modes. It is
sent to firmware as adv_speed_mask along with autoneg=0 and a forced speed
of 10 or 100.

The existing forced-mode caller narrows the mask to the forced mode before
sending PHY_LINK_SET:

rnpgbe_chip.c:rnpgbe_setup_default_link() {
    ...
	if (!autoneg) {
		advertising &= RNPGBE_FW_ADV_1000_FULL;
		speed = SPEED_1000;
		duplex = DUPLEX_FULL;
	}
    ...
}

The mucse_mbx_set_link() kernel-doc says only that firmware ignores speed
and duplex when autoneg is true. Nothing says adv_speed_mask is ignored
when autoneg is false.

For example, "ethtool -s ethX autoneg off speed 100 duplex full" on a port
that was autonegotiating with a 1000/100/10 advertisement would send the
whole stale mask. On success, that mask is also cached into
hw->link_config.advertising. A later get_link_ksettings then reports
1000baseT_Full as advertised while the link is forced to 100.

Should advertising be set to mode in this branch?

> +
> +	err = mucse_mbx_set_link(&mucse->hw, advertising, autoneg,
> +				 autoneg ? 0 : cmd->base.speed,
> +				 autoneg ? 0 : cmd->base.duplex, mdix);
> +	if (!err) {
> +		mucse->hw.mdix_ctrl = mdix;
> +		mucse->hw.link_config.advertising = advertising;

[Severity: Low]
When autoneg is disabled, cmd->link_modes.advertising is never read or
validated. The old cached mask is written back here and the call returns
success. Can a user-requested advertising mask be silently lost this way?

ethnl_update_linkmodes() applies ETHTOOL_A_LINKMODES_OURS to
ksettings->link_modes.advertising whatever the autoneg state is. So
"ethtool -s ethX advertise <mask>" on a port in forced mode returns 0, but
the new mask is thrown away.

A later "ethtool -s ethX autoneg on" builds cmd from
rnpgbe_get_link_ksettings(), which returns the old cached mask. The
requested advertisement therefore never takes effect.

Also, bits outside the supported set get -EINVAL in autoneg mode but are
silently accepted in forced mode.

> +		mucse->hw.link_config.speed = autoneg ?

[ ... ]

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> index 469818b83d70f..7af56fce2e229 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> @@ -236,7 +236,7 @@ int mucse_mbx_set_link(struct mucse_hw *hw, u32 advertising, bool autoneg,
>  
>  	len = le16_to_cpu(req.r.datalen);
>  	mutex_lock(&hw->mbx.lock);
> -	err = mucse_write_and_wait_ack_mbx(hw, req.dwords, len);
> +	err = mucse_write_mbx_coalesce_event(hw, req.dwords, len);

[Severity: Low]
This isn't a bug, but the commit message only describes adding
set_link_ksettings. It doesn't mention this change to the mailbox
transport used for PHY_LINK_SET.

mucse_write_mbx_coalesce_event() goes through
mucse_write_mbx_pf_coalesce_event(), which calls mucse_ack_pending_event()
before writing the command:

    err = mucse_ack_pending_event(hw);
    if (err)
        goto release;

This acknowledges any pending firmware LINK_CHANGE_EVT without consuming
its snapshot. It also adds a new -EIO return path for when the firmware
request counter reads 0.

It also changes the behaviour of the existing probe-time caller,
rnpgbe_setup_default_link().

The change is probably needed. PHY_LINK_SET can now be issued at runtime
while link event reporting is enabled, which is the same case as
mucse_mbx_phyup() and mucse_mbx_link_report(); both already use the
coalescing variant.

Could the reason be stated in the commit message, or the change split into
its own patch?

>  	mutex_unlock(&hw->mbx.lock);
>  
>  	return err;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/FB722C2F25FE9005%2B20261106112420.43050-1-dong100%40mucse.com