Re: [PATCH net-next v6 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

[netdev-bot+sashiko@kernel.org]

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

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

- [Low] drivers/phy/rockchip/phy-rockchip-naneng-combphy.c:380-381 reads
  'rockchip,sgmii-mac-sel' with device_property_read_u32() for every…

--

phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

This adds a "rockchip,sgmii-mac-sel" DT property so the RK3568 SGMII
interface can be routed to GMAC0 or GMAC1 via the PIPE_GRF_XPCS_CON0
pipe_sgmii_mac_sel bit, defaulting to GMAC1 to match the hardware reset
value.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 229)

> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> index 7843356a4dd47..7b867e7520064 100644
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c

[ ... ]

> @@ -375,6 +377,9 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
>  
>  	priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
>  
> +	priv->sgmii_mac_sel = 1;
> +	device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
> +

[Severity: Low]
This isn't a bug for any schema-checked device tree, but would it be worth
reacting to the return value here?

The binding restricts the property to two values and to one compatible:

  rockchip,sgmii-mac-sel:
    $ref: /schemas/types.yaml#/definitions/uint32
    enum: [0, 1]
    default: 1

  ...
        rockchip,sgmii-mac-sel: true
    else:
      properties:
        rockchip,sgmii-mac-sel: false

In rockchip_combphy_parse_dt() the result of device_property_read_u32() is
dropped, so a malformed property (for example a zero-length or wrong-sized
cell, which returns -ENODATA/-EOVERFLOW and leaves the output untouched)
ends up indistinguishable from an explicit request for GMAC1.

The read also happens for every compatible, including rk3528/rk3562/rk3576/
rk3588 where the binding declares the property invalid; there the value is
parsed and then never used, since only rk3568_combphy_cfg() consumes it.

[ ... ]

> @@ -873,6 +878,8 @@ static int rk3568_combphy_cfg(struct rockchip_combphy_priv *priv)
>  		break;
>  
>  	case PHY_TYPE_SGMII:
> +		rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
> +					     priv->sgmii_mac_sel > 0);

[Severity: Low]
Should out-of-range values be rejected rather than folded into GMAC1?

The "priv->sgmii_mac_sel > 0" test collapses the u32 to a boolean, so a
device tree with rockchip,sgmii-mac-sel = <2> (or 0xffffffff) that was never
run through dtbs_check gets the GMAC1 routing and rk3568_combphy_cfg() still
returns success, with nothing logged.

A range check in rockchip_combphy_parse_dt() returning -EINVAL, or at least
a dev_warn(), would make such a device tree visible instead of silently
selecting the default route.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915123802.1561724-1-coiaprant%40gmail.com