Re: [PATCH net-next v3 2/2] net: macb: add support for 1000BASE-X autonegotiation to PCS

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/
---
--- Patch [2]: [PATCH net-next v3 2/2] net: macb: add support for 1000BASE-X autonegotiation to PCS ---
commit 3a10158740d1fea523a081adf59e6bcdf1cd55d1
Author: Nathan Whitehorn <nwhitehorn@pa.msu.edu>

    net: macb: add support for 1000BASE-X autonegotiation to PCS
    
    The current PCS code unconditionally uses SGMII autonegotiation, though
    the hardware supports both SGMII and 1000BASE-X modes. Decouple the
    choice of PCS enablement from use of the SGMII mode when running at
    gigabit rates and announce to phylink that 1000BASE-X is a supported
    operating mode. This enables direct attachment of the PCS to e.g. an
    SFP.
    
    The 1000BASE-X code in phylink also sometimes calls the autonegotiation
    restart method, so add an implementation of autonegotiation restart.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index c15a9c7e69d37..310ad8eac21fc 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -583,7 +583,12 @@ static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
>  
>  static void macb_pcs_an_restart(struct phylink_pcs *pcs)
>  {
> -	/* Not supported */
> +	struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
> +	u32 old, new;
> +
> +	old = gem_readl(bp, PCSCNTRL);
> +	new = old | BMCR_ANRESTART;
> +	gem_writel(bp, PCSCNTRL, new);
>  }

[Severity: Medium]
Since 1000BASE-X utilizes base page advertisements that can dynamically change,
should the adjacent macb_pcs_config() function be updated to return 1 when
changes are made?

Looking at macb_pcs_config():

static int macb_pcs_config(...)
{
...
	if (old != new)
		gem_writel(bp, PCSCNTRL, new);

	return 0;
}

Returning 0 unconditionally appears to prevent phylink from restarting
autonegotiation when the advertisement changes (like flow control or PAUSE
settings), meaning the new configuration will not take effect until the link
is bounced.

[ ... ]

> @@ -750,7 +755,9 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
>  		ctrl &= ~(GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
>  		ncr &= ~GEM_BIT(ENABLE_HS_MAC);
>  
> -		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> +		if (state->interface == PHY_INTERFACE_MODE_1000BASEX) {
> +			ctrl |= GEM_BIT(PCSSEL);
> +		} else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
>  			ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);

[Severity: High]
This is a pre-existing issue, but could macb_mac_config() race with
macb_set_rx_mode() over the NCFGR register?

macb_mac_config() modifies NCFGR while holding bp->lock:

	spin_lock_irqsave(&bp->lock, flags);
	old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
	...
	if (old_ctrl ^ ctrl)
		macb_or_gem_writel(bp, NCFGR, ctrl);

However, macb_set_rx_mode() performs a lockless read-modify-write on NCFGR
protected only by netif_addr_lock_bh:

	cfg = macb_readl(bp, NCFGR);
	if (dev->flags & IFF_PROMISC) {
	...

Could this concurrent modification lead to lost configuration updates, such as
overwriting PCSSEL/SGMIIEN bits or dropping promiscuous/multicast flags?

[ ... ]

> @@ -1061,9 +1070,12 @@ static int macb_mii_probe(struct net_device *dev)
>  			  bp->phylink_config.supported_interfaces);
>  		phy_interface_set_rgmii(bp->phylink_config.supported_interfaces);
>  
> -		if (bp->caps & MACB_CAPS_PCS)
> +		if (bp->caps & MACB_CAPS_PCS) {
> +			__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +				  bp->phylink_config.supported_interfaces);
>  			__set_bit(PHY_INTERFACE_MODE_SGMII,
>  				  bp->phylink_config.supported_interfaces);
> +		}

[Severity: High]
With the addition of 1000BASE-X support here, does the init_reset_optional()
function also need updating to prevent a regression on platforms using a SerDes?

Looking at init_reset_optional() later in the file:

	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
		/* Ensure PHY device used in SGMII mode is ready */
		bp->phy = devm_phy_optional_get(&pdev->dev, NULL);

Since this explicitly checks only for SGMII mode, won't the hardware
initialization for the SerDes lane fail when the MAC is configured for
1000BASE-X, leaving the generic PHY uninitialized and preventing the hardware
from linking up?
-- 
This is an AI-generated review.