Re: [PATCH net-next v3 4/9] net: stmmac: mediatek: simplify TX/RX delay handling in mt2712_set_delay

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] stmmac: mediatek: swapped RGMII and RMII/MII delay stage divisors
--

commit 83d585b88ab8ffcfd715b793f8dc33929760603d
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

net: stmmac: mediatek: simplify TX/RX delay handling in mt2712_set_delay

This commit simplifies TX/RX delay handling for MT2712 by calculating
delay stage values locally instead of modifying the driver data. It
also introduces macro definitions for the stage divisors and maximums.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> index f7eb85110df08..596411d4a5b2b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> @@ -36,6 +36,14 @@
>  #define ETH_FINE_DLY_GTXC	BIT(1)
>  #define ETH_FINE_DLY_RXC	BIT(0)
>  
> +#define MT2712_RMII_DLY_RXC_STAGE_DIV	170 /* 170ps per stage */
> +#define MT2712_RMII_DLY_TXC_STAGE_DIV	170 /* 170ps per stage */
> +
> +#define MT2712_RGMII_DLY_RXC_STAGE_DIV	550 /* 550ps per stage */
> +#define MT2712_RGMII_DLY_RXC_MAX	17600 /* 32 x 550ps */
> +#define MT2712_RGMII_DLY_TXC_STAGE_DIV	550 /* 550ps per stage */
> +#define MT2712_RGMII_DLY_TXC_MAX	17600 /* 32 x 550ps */
> +

[Severity: High]
Are these stage divisors swapped compared to the original code?

In the original code, the divisors were:
/* 550ps per stage for MII/RMII */
mac_delay->tx_delay /= 550;
...
/* 170ps per stage for RGMII */
mac_delay->tx_delay /= 170;

With the new definitions, RMII gets divided by 170 and RGMII by 550, which
reverses the intended delay values for both interfaces.

[ ... ]

> @@ -129,71 +137,36 @@ static int mt2712_set_interface(struct mediatek_dwmac_plat_data *plat,
>  static int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat)
>  {
>  	struct mac_delay_struct *mac_delay = &plat->mac_delay;
> -	u32 delay_val = 0, fine_val = 0;
> +	u32 rx_delay_stage_val, tx_delay_stage_val;
> +	u16 rx_delay_stage_div, tx_delay_stage_div;
> +	u32 delay_val = 0;
> +	u32 fine_val = 0;
>  
> -	mt2712_delay_ps2stage(plat);
> +	if (phy_interface_mode_is_rgmii(plat->phy_mode)) {
> +		rx_delay_stage_div = MT2712_RGMII_DLY_RXC_STAGE_DIV;
> +		tx_delay_stage_div = MT2712_RGMII_DLY_TXC_STAGE_DIV;
> +	} else {
> +		rx_delay_stage_div = MT2712_RMII_DLY_RXC_STAGE_DIV;
> +		tx_delay_stage_div = MT2712_RMII_DLY_TXC_STAGE_DIV;
> +	}
> +	rx_delay_stage_val = mac_delay->rx_delay / rx_delay_stage_div;
> +	tx_delay_stage_val = mac_delay->tx_delay / tx_delay_stage_div;
>  
>  	switch (plat->phy_mode) {
>  	case PHY_INTERFACE_MODE_MII:
> -		delay_val |= FIELD_PREP(ETH_DLY_TXC_ENABLE, !!mac_delay->tx_delay);
> -		delay_val |= FIELD_PREP(ETH_DLY_TXC_STAGES, mac_delay->tx_delay);
> +		delay_val |= FIELD_PREP(ETH_DLY_TXC_ENABLE,
> +					!!tx_delay_stage_val);
> +		delay_val |= FIELD_PREP(ETH_DLY_TXC_STAGES,
> +					tx_delay_stage_val);

[Severity: High]
Because the divisors are swapped above, could the resulting stage value
exceed the bitfield boundaries?

Since RMII is now incorrectly divided by 170 instead of 550, valid device
tree delay values might produce a stage count that exceeds the 5-bit width
of the ETH_DLY_TXC_STAGES hardware field. When this happens, FIELD_PREP()
would silently truncate the stages value, leading to misconfigured hardware
timings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-dwmac-mediatek-mt8189-v3-0-25daf2bbcf35@collabora.com?part=4