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

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Nathan,

On Tue Aug 4, 2026 at 5:08 PM CEST, Nathan Whitehorn wrote:
>
>
> On 8/4/26 04:37, Paolo Abeni wrote:
>> 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.
>>
>> [ ... ]
>
> 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).

Sashiko loves raising such pre-existing issues! macb_set_rx_mode() is at
fault and you have no relation to it, so don't feel obliged to address
that finding.

Also, it does no distinction on the frequency of the codepaths (here two
functions called approx once per boot). It would be a proper severity
high if it was phylink_get_state versus IRQ for example...

Fix only if you want.

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

Agreed with you. It's ZyncMQ quirks, and if it was done that way most
likely the HW only supports SGMII and they have two-three wrapper
registers that must be written.

I asked an LLM to make a scan and upstream devicetrees seem to confirm,
with only RGMII or SGMII PHYs.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com