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

On 8/4/26 04:37, Paolo Abeni wrote:
> From: AI Reviewer <[email protected]>
>
> 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 <[email protected]>
>
>      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.
>
> [ ... ]

Thank you! This is a good catch of a pre-existing bug in 
macb_pcs_config() that matters a lot more with removable transceivers 
and I will fix it in an updated patch.

>
>> @@ -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?
>
> [ ... ]

It probably could. I am a bit hesitant to fix this as part of this patch 
series for scope reasons, though, and would like to defer to the driver 
maintainers on the proper fix for this one. I don't think things are any 
worse with this patch than they were (1000BASE-X vs. SGMII 
autonegotiation modes are essentially static, except on physical SFP 
replacement, unlike promiscuous/multicast).

>
>> @@ -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 block of code is specific to an onboard PHY -- which does not 
generally (ever?) exist in 1000BASE-X mode -- and what looks like some 
kind of Xilinx Zynq-specific quirks that I don't know enough details 
about to understand whether it is relevant in this case or test. As 
such, I had decided not to touch this code in this patch, but would 
welcome any feedback from people more familiar with the Xilinx case than 
I am and am happy to make changes if appropriate.

Thanks,
Nathan

-- 
Nathan Whitehorn (he/him)
Associate Professor
Department of Physics and Astronomy
Michigan State University
Biomedical and Physical Sciences 3225
East Lansing, MI 48824
(517) 884-5563