Re: [PATCH 3/3] phy: qcom: sgmii-eth: add Nord USXGMII support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] phy: qcom: sgmii: incorrect hardware offsets written on mode change
--

commit eb8579d974b41d6c98fd95052875c10ccf3e31fa
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

    phy: qcom: sgmii-eth: add Nord USXGMII support
    
    Nord uses a QMP v7 SerDes block which is an updated version of the
    SGMII SerDes PHY IP. The driver currently handles SGMII and 2500BASE-X
    configurations only. This patch adds support for configuring the SerDes
    to operate at 10.3125 Gbaud, enabling USXGMII and 10GBASE-R PHY modes
    for the Nord compatible.

> diff --git a/drivers/phy/qualcomm/phy-qcom-sgmii-eth.c b/drivers/phy/qualcomm/phy-qcom-sgmii-eth.c
> index 029dafacee785..3c68bfe0ea443 100644
> --- a/drivers/phy/qualcomm/phy-qcom-sgmii-eth.c
> +++ b/drivers/phy/qualcomm/phy-qcom-sgmii-eth.c
[ ... ]
> @@ -275,16 +419,19 @@ static int qcom_dwmac_sgmii_phy_calibrate(struct phy *phy)
>  	case PHY_INTERFACE_MODE_1000BASEX:
>  		/* 1.25Gbps mode */
>  		qcom_dwmac_sgmii_phy_init_1g(data->regmap);
> -		break;
> +		return qcom_dwmac_sgmii_phy_poll_sgmii_ready(dev, data->regmap);
>  	case PHY_INTERFACE_MODE_2500BASEX:
>  		/* 3.125Gbps mode */
>  		qcom_dwmac_sgmii_phy_init_2p5g(data->regmap);
> -		break;
> +		return qcom_dwmac_sgmii_phy_poll_sgmii_ready(dev, data->regmap);
> +	case PHY_INTERFACE_MODE_USXGMII:
> +	case PHY_INTERFACE_MODE_10GBASER:
> +		/* 10.3125Gbps mode */
> +		qcom_dwmac_sgmii_phy_init_10g(data->regmap);
> +		return qcom_dwmac_sgmii_phy_poll_usxgmii_ready(dev, data->regmap);

[Severity: High]
Does this logic incorrectly route SGMII and 2500BASE-X requests directly to
V5 hardware initialization sequences, regardless of whether the underlying
hardware is V5 or V7?

If a MAC driver using phylink dynamically falls back to SGMII, 1000BASE-X, or
2500BASE-X on the Nord (v7) platform during link state changes (e.g., when
plugging in a different SFP module), it appears this will write completely
wrong offsets to the PHY SerDes block.

Similarly, if phylink falls back to USXGMII or 10GBASE-R on the sa8775p (v5)
platform, won't it run the V7 initialization sequence instead?

Writing incorrect register offsets into the hardware IP can lead to
unpredictable behavior or system crashes.

>  	default:
>  		return -EINVAL;
>  	}
> -
> -	return qcom_dwmac_sgmii_phy_poll_sgmii_ready(dev, data->regmap);
>  }
[ ... ]
> @@ -340,7 +487,9 @@ static int qcom_dwmac_sgmii_phy_validate(struct phy *phy, enum phy_mode mode,
>  
>  	if (submode == PHY_INTERFACE_MODE_SGMII ||
>  	    submode == PHY_INTERFACE_MODE_1000BASEX ||
> -	    submode == PHY_INTERFACE_MODE_2500BASEX)
> +	    submode == PHY_INTERFACE_MODE_2500BASEX ||
> +	    submode == PHY_INTERFACE_MODE_USXGMII ||
> +	    submode == PHY_INTERFACE_MODE_10GBASER)
>  		return 0;

[Severity: High]
Does this validation incorrectly advertise that all PHY interface modes
are supported by all hardware instances?

By returning 0 for all these modes, this allows phylink to believe the platform
supports them unconditionally.

This is what permits consumers to switch into incompatible modes during
standard PHY mode negotiation, ultimately triggering the incorrect hardware IP
initialization sequences in qcom_dwmac_sgmii_phy_calibrate() mentioned above.

>  
>  	return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-nord_ethernet_serdes-v1-0-f8ac1c487882@oss.qualcomm.com?part=3