Re: [PATCH net-next v5 06/10] net: stmmac: mediatek: use standard internal delay properties

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In mediatek_dwmac_config_dt(), the new RGMII fallback caps the
  vendor value with min(vendor, rgmii_delay_max).
- [Low] The DT binding and the driver disagree.
- [Low] In mediatek_dwmac_config_dt(), the new 'Invalid RGMII TX/RX clock
  delay' messages, and the moved 'Invalid TX/RX clock delay' messages,…

--

net: stmmac: mediatek: use standard internal delay properties

This commit makes the mediatek-dwmac driver keep separate MII/RMII and
RGMII delay values. The RGMII delays now come from the standard
rx/tx-internal-delay-ps properties. If those are absent, the driver falls
back to the mediatek,rx/tx-delay-ps vendor properties.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> index cda3f3edde39f..ee43d694099bd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c

[ ... ]

> @@ -427,24 +432,58 @@ static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat)
>  		return PTR_ERR(plat->peri_regmap);
>  	}
>  
> -	if (!of_property_read_u32(plat->np, "mediatek,tx-delay-ps", &tx_delay_ps)) {
> -		if (tx_delay_ps < plat->variant->tx_delay_max) {
> -			mac_delay->tx_delay = tx_delay_ps;
> -		} else {
> -			dev_err(plat->dev, "Invalid TX clock delay: %dps\n", tx_delay_ps);
> +	of_property_read_u32(plat->np, "mediatek,tx-delay-ps", &tx_delay_ps);

[Severity: Low]
This one is about the binding change in the same series, not this hunk
directly. The commit "dt-bindings: net: mediatek-dwmac: deprecate rx/tx
delay vendor properties" adds "deprecated: true" to both
mediatek,tx-delay-ps and mediatek,rx-delay-ps with no condition. Its
commit message also says they "duplicate standard properties".

In this patch, though, mediatek_dwmac_config_dt() still uses these vendor
properties as the only source for the MII/RMII delays in
mac_delay->tx_delay and mac_delay->rx_delay. This commit message also
says:

    There is no equivalent standard properties for RMII and MII case at the
    moment.

ethernet-controller.yaml only defines rx/tx-internal-delay-ps inside an
if/then on rgmii phy-modes. That leaves an MII/RMII node with no
non-deprecated way to describe these delays.

Should the schema limit the deprecation to RGMII, the way the description
text already does?

The binding example also still uses the deprecated property on an RGMII
node:

    phy-mode = "rgmii-rxid";
    ...
    mediatek,tx-delay-ps = <1530>;

Also, the stage granularity and ranges are documented only for the
deprecated properties, not for the standard ones. The driver now checks
those standard properties against rgmii_delay_max.

> +	if (tx_delay_ps >= plat->variant->mii_delay_max) {
> +		dev_err(plat->dev, "Invalid TX clock delay: %dps\n",
> +			tx_delay_ps);
> +		return -EINVAL;
> +	}
> +
> +	/* Read RGMII TX internal delay standard property */
> +	if (!of_property_read_u32(plat->np, "tx-internal-delay-ps",
> +				  &rgmii_tx_delay_ps)) {
> +		if (rgmii_tx_delay_ps >= plat->variant->rgmii_delay_max) {
> +			dev_err(plat->dev, "Invalid RGMII TX clock delay: %dps\n",
> +				rgmii_tx_delay_ps);

[Severity: Low]
This isn't a bug, but rgmii_tx_delay_ps is a u32 printed with %d. A
malformed DT value above INT_MAX, such as 0xffffffff, would be logged as
"-1ps". Should this be %u?

The same applies to the "Invalid RGMII RX clock delay" message. It also
applies to the moved "Invalid TX/RX clock delay" messages for tx_delay_ps
and rx_delay_ps.

> +			return -EINVAL;
>  		}
> +	} else {
> +		/*
> +		 * standard property value not available, fallback on vendor
> +		 * value instead and cap it to max RGMII delay value
> +		 */
> +		rgmii_tx_delay_ps = min(tx_delay_ps,
> +					plat->variant->rgmii_delay_max);
>  	}

[Severity: Medium]
Can this cap produce a value that the stage field can't hold?
rgmii_delay_max is used as an exclusive bound elsewhere, including in the
standard-property branch just above:

    if (rgmii_tx_delay_ps >= plat->variant->rgmii_delay_max) {

It is also defined as one stage past the largest value that can be
programmed:

    #define MT2712_RGMII_DELAY_MAX		5440 /* 32 x 170ps */

On MT2712, mii_delay_max is 17600. A mediatek,tx-delay-ps value in
[5440, 17600) therefore passes the earlier check. If the node has no
tx-internal-delay-ps, min() caps the value to exactly 5440.

Then the RGMII case in mt2712_set_delay() does:

    tx_delay_stage_val = mac_delay->rgmii_tx_delay
                         / MT2712_RGMII_DLY_STAGE_DIV;

That gives 5440 / 170 = 32. For a non-constant value, FIELD_PREP() masks
32 down to 0 in the 5-bit ETH_DLY_GTXC_STAGES and ETH_DLY_RXC_STAGES
fields. ETH_DLY_GTXC_ENABLE and ETH_DLY_RXC_ENABLE are still set from
!!32.

So the RGMII delay is programmed to zero stages with the enable bit set,
not to the maximum the comment describes, and nothing is logged. The
binding documents the MT2712 RGMII range as 0~31*170, which matches an
exclusive bound. The commit message only says "fallback to vendor
values" and doesn't mention any capping.

Should the cap be rgmii_delay_max - 1? Or should the fallback return
-EINVAL, like the standard-property branch does?

The rgmii_rx_delay_ps fallback below has the same problem. MT8195 and
MT8189 aren't affected, because mii_delay_max equals rgmii_delay_max
there. The behavior seems unchanged later in the series, where
get_delay_stage() divides by rgmii_delay_stage_div (170 on MT2712).

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-dwmac-mediatek-mt8189-v5-0-7d99d6292962%40collabora.com