Re: [PATCH net-next 2/5] net: rnpgbe: Report link settings through ethtool

[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 mucse_mbx_fw_req_handler() (rnpgbe_mbx_fw.c:465-466), the
  patch takes the 2-bit firmware field FIELD_GET(ST_STATUS_MDIX_MASK /*…

--

net: rnpgbe: Report link settings through ethtool

This commit adds get_link_ksettings support to rnpgbe. At probe time it
queries firmware with GET_LANE_STATUS and reports the supported and
advertised link modes, autoneg, port type and PHY address from that
reply. It also caches the MDI-X state from LINK_CHANGE_EVT, and sets
the default link to autoneg on TP ports and to fixed 1000/full on fibre
and backplane ports.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> index 4a18937927514..469818b83d70f 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c

[ ... ]

> @@ -435,6 +462,8 @@ static void mucse_mbx_fw_req_handler(struct mucse_hw *hw,
>  
>  		hw->speed = le16_to_cpu(req->link_stat.st.speed);
>  		hw->duplex = req->link_stat.st.flags & DUPLEX_BIT;
> +		hw->mdix = FIELD_GET(ST_STATUS_MDIX_MASK,
> +				     le16_to_cpu(req->link_stat.st.status));

[Severity: Low]
This stores the raw 2-bit firmware field (0..3) in hw->mdix. Is that
value guaranteed to use the ETH_TP_MDI_* encoding?

When the carrier is up on a TP port, rnpgbe_get_link_ksettings() copies
the value straight into the ethtool status field:

	if (status & MUCSE_LINK_TP)
		cmd->base.eth_tp_mdix = hw->mdix;

The ethtool uAPI only allows ETH_TP_MDI_INVALID, ETH_TP_MDI or
ETH_TP_MDI_X in eth_tp_mdix. ETH_TP_MDI_AUTO (3) is only valid for
control. Both linkinfo_fill_reply() and the legacy ioctl path pass the
value to userspace without checking it.

If firmware reports 3, userspace gets an invalid status. If firmware
reports 0 while the link is up, userspace sees "Unknown". If firmware
uses a different encoding, userspace sees the wrong MDI/MDI-X state.

The control path already sends ETH_TP_MDI_* values to firmware in
phy_link_set.tp_mdix_ctrl, so values 1 and 2 may well line up. Nothing
here documents or enforces that, though, and later patches in the
series don't change the parsing or the reporting code.

Could the firmware value be mapped to ETH_TP_MDI_* explicitly, with
anything unexpected reported as ETH_TP_MDI_INVALID?

The patch also adds a second MDI-X field definition, for the
GET_LANE_STATUS status word in rnpgbe_mbx_fw.h, and nothing uses it:

	#define MUCSE_LINK_MDIX		GENMASK(14, 13)

Neither this define nor ST_STATUS_MDIX_MASK documents its value
encoding. Is MUCSE_LINK_MDIX meant to be used, or can it be dropped?

>  		rnpgbe_set_link(hw, mucse_link_is_up(hw, req));
>  		/* update regs to notify link info is received */
>  		mucse_update_link_status_reg(hw, req);

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