Re: [PATCH net-next 03/12] net: pcs: pcs-xpcs: Preserve BMCR_ANENBLE during link up

On 5/2/26 10:56 AM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Unconditionally forcing BMCR_ANENABLE for SGMII interfaces during out-of-band link configuration breaks forced and fixed links.
> --
> 
> commit 3fde24f4d6c6c8c6b563418e32cec40953863f8a
> Author: Daniel Thompson <[email protected]>
> 
> net: pcs: pcs-xpcs: Preserve BMCR_ANENBLE during link up
> 
> This commit unconditionally sets the BMCR_ANENABLE bit for SGMII interfaces
> during link up. This change was introduced to resolve an issue on a specific
> hardware platform where the MAC fails to transmit packets despite negotiating
> the speed correctly.
> 
>> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
>> index b2c84b7e1e11..1d62d5b31c61 100644
>> --- a/drivers/net/pcs/pcs-xpcs.c
>> +++ b/drivers/net/pcs/pcs-xpcs.c
>> @@ -1263,11 +1263,14 @@ static void xpcs_link_up_sgmii_1000basex(struct dw_xpcs *xpcs,
>>   					 phy_interface_t interface,
>>   					 int speed, int duplex)
>>   {
>> +	u16 an_enable;
>>   	int ret;
>>   
>>   	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
>>   		return;
>>   
>> +	an_enable = (interface == PHY_INTERFACE_MODE_SGMII ? BMCR_ANENABLE : 0);
>> +
> 
> Will this break forced and fixed links by enabling auto-negotiation?

The next version of this code will handle this differently,
setting the plat_stmmacenet_data->default_an_inband flag to
true instead.

					-Alex

> 
> This function handles forcing the link state when in-band auto-negotiation
> is explicitly disabled, as indicated by the early return for
> PHYLINK_PCS_NEG_INBAND_ENABLED. When this function continues, phylink is
> requesting out-of-band negotiation or a fixed link.
> 
> By unconditionally setting BMCR_ANENABLE here, the hardware might ignore
> the fixed speed and duplex bits, instead waiting for auto-negotiation to
> complete. If the system uses a fixed link (e.g., MAC-to-MAC) or connects
> to a PHY with in-band signaling disabled, SGMII AN base pages will never
> arrive.
> 
> Could this cause the MAC PCS to wait indefinitely, failing to establish a
> link and dropping all traffic?
> 
> [ ... ]
> 
>>   	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
>> -			 mii_bmcr_encode_fixed(speed, duplex));
>> +			 mii_bmcr_encode_fixed(speed, duplex) | an_enable);
>