Re: [PATCH net-next v2 2/2] net: phy: realtek: support unaligned RTL8224 PHY packages

[netdev-bot+sashiko]

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

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

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] The RTL8224 text added to realtek,rtl82xx.yaml (preceding
  commit "dt-bindings: net: realtek,rtl82xx: document RTL8224 packages")…
- [Low] The new RTL8224 package example in realtek,rtl82xx.yaml
  (preceding commit "dt-bindings: net: realtek,rtl82xx: document RTL8224…

--

net: phy: realtek: support unaligned RTL8224 PHY packages

This commit makes the RTL8224 driver take the package base address from
the reg property of an ethernet-phy-package parent node, rather than
always rounding the PHY address down to a multiple of four. The port
index relative to the package is stored in a new struct rtl8224_priv and
used to pick the MDI pair order and polarity fields. When there is no
package node, the old aligned-address assumption still applies.

> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index e14960192e791..91cf34d134202 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c

[ ... ]

> @@ -2783,9 +2789,42 @@ static int rtl8224_config_init(struct phy_device *phydev)
>  
>  static int rtl8224_probe(struct phy_device *phydev)
>  {
> -	/* Chip exposes 4 ports, join all of them in the same package */
> -	return devm_phy_package_join(&phydev->mdio.dev, phydev,
> -				     phydev->mdio.addr & ~3, 0);
> +	struct device *dev = &phydev->mdio.dev;
> +	u32 base_addr = phydev->mdio.addr & ~3;
> +	struct rtl8224_priv *priv;
> +	struct device_node *np;
> +	int ret;

[ ... ]

> +	np = of_get_parent(dev->of_node);
> +	if (of_node_name_eq(np, "ethernet-phy-package")) {
> +		ret = of_property_read_u32(np, "reg", &base_addr);
> +		of_node_put(np);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Missing or invalid PHY package reg\n");
> +
> +		if (phydev->mdio.addr < base_addr ||
> +		    phydev->mdio.addr - base_addr >= 4)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "PHY address outside RTL8224 package\n");
> +
> +		ret = devm_of_phy_package_join(dev, phydev, 0);
> +	} else {
> +		of_node_put(np);
> +		/* Without a package node, assume four aligned PHY addresses. */
> +		ret = devm_phy_package_join(dev, phydev, base_addr, 0);

[Severity: Medium]
What happens in this fallback when an RTL8224 is unaligned and has no
ethernet-phy-package parent?

The binding text added in the preceding commit "dt-bindings: net:
realtek,rtl82xx: document RTL8224 packages" says the package node is
optional:

  Its PHY nodes may be grouped under an ethernet-phy-package node ...
  This address need not be a multiple of four.

That text never says the package node is required when the first PHY
address is not a multiple of four. So a DT for the Hasivo
S1100WP-8GT-2S+ layout (chips at 1-4 and 5-8) that has no package nodes
still passes the schema.

For that DT, this branch uses base_addr = addr & ~3 with no check:

  PHYs 1-3 join base 0, which is not part of either chip
  PHY 4 joins base 4 with port_offset 0
  PHYs 5-7 (ports 0-2 of chip B) also join base 4, with offsets 1-3

rtl8224_package_modify_mmd() then does a read-modify-write of
RTL8224_VND1_MDI_PAIR_SWAP and RTL8224_VND1_MDI_POLARITY_SWAP at package
address 0:

	val = __phy_package_read_mmd(phydev, 0, devad, regnum);
	...
	ret = __phy_package_write_mmd(phydev, 0, devad, regnum, val);

Wouldn't chip B's pair order and polarity settings end up in chip A's
registers? They would also overwrite the settings for chip A port 3, and
nothing reports an error or warning.

Commit 330296ea9e15 ("net: phy: realtek: add RTL8224 pair order
support") says the swap "has to be set correctly for each of the 4 ports
before the chip can detect a link". The affected copper ports may
therefore fail to link.

Could the binding require the package node when the first PHY address
is not a multiple of four? Alternatively, could this fallback reject or
warn about the ambiguous case?

> +	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to join PHY package\n");

[Severity: Low]
This isn't a bug, but I have a question about the RTL8224 package
example added to realtek,rtl82xx.yaml in the preceding commit
"dt-bindings: net: realtek,rtl82xx: document RTL8224 packages".

The only compatible on each child PHY in that example is:

	compatible = "ethernet-phy-ieee802.3-c45";

The schema has no explicit select, and its compatible enum lists only
ethernet-phy-id001c.xxxx values.

Doesn't that mean dt-schema never applies realtek,rtl82xx.yaml to these
nodes? They would then be checked only against ethernet-phy-package.yaml
and ethernet-phy.yaml.

If so, the example never exercises the RTL8224-specific rules in this
file: unevaluatedProperties: false, the leds: false rule and the vendor
properties. A wrong property in the example would still pass
dt_binding_check.

This is partly an existing limitation. The first example has no
compatible either, and cameo-rtl9302c-2x-rtl8224-2xge.dts also uses only
the c45 compatible for RTL8224.

> +
> +	priv->port_offset = phydev->mdio.addr - base_addr;
> +	phydev->priv = priv;
> +
> +	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005201540.913977-1-oskari%40lemmela.net