[PATCH net-next v4 0/2] net: phy: fix PHY interface mask initialization and XPCS usage

[PATCH net-next v4 0/2] net: phy: fix PHY interface mask initialization and XPCS usage

[Coia Prant]

This series fixes issues related to the handling of the PHY interface mask
(supported_interfaces) at the core level and in the XPCS driver.

Patch 1 fixes a local macro in the txgbe driver.
DECLARE_PHY_INTERFACE_MASK_ZERO() incorrectly uses PHY_INTERFACE_MODE_MAX
as the array length instead of BITS_TO_LONGS(), which wastes stack space.
Move it to include/linux/phy.h and base it on
DECLARE_PHY_INTERFACE_MASK() so the bitmap is correctly sized while
still being zero-initialized.

Patch 2 makes the XPCS supported_interfaces mask authoritative. The
generic compat table advertises modes that a specific integration may
not support. The Rockchip glue now builds a platform mask (1000BASE-X,
SGMII, QSGMII) and intersects it with the inherited core mask using
phy_interface_and(). In addition, xpcs_find_compat() now checks
supported_interfaces before looking up the compat table, ensuring the
core respects the narrowed mask.

Note: checkpatch.pl reports "Macros with complex values should be
enclosed in parentheses" for the new macro in Patch 1. This is a known
false positive for declarative macros that expand to variable
declarations; wrapping it would break compilation.

Changes since v3:
- Rebased onto net-next as suggested by Paolo.
- Patch 1: drop the Fixes tag, as this is a cleanup/optimization rather
  than a bug fix.
- Patch 2: move DECLARE_PHY_INTERFACE_MASK_ZERO() to the top of the
  function to comply with the reverse Christmas tree order.
- Patch 2: clarify in the commit message that QSGMII is currently a
  no-op because the core does not support it, demonstrating the
  intersection behavior.
---
Coia Prant (2):
  net: phy: move DECLARE_PHY_INTERFACE_MASK_ZERO to core
  net: pcs: xpcs: make supported_interfaces authoritative

 drivers/net/ethernet/wangxun/txgbe/txgbe_type.h |  3 ---
 drivers/net/pcs/pcs-xpcs-rk.c                   | 10 ++++++++++
 drivers/net/pcs/pcs-xpcs.c                      |  3 +++
 include/linux/phy.h                             |  3 +++
 4 files changed, 16 insertions(+), 3 deletions(-)

-- 
2.47.3

[PATCH net-next v4 1/2] net: phy: move DECLARE_PHY_INTERFACE_MASK_ZERO to core

[Coia Prant]

The txgbe driver currently defines DECLARE_PHY_INTERFACE_MASK_ZERO()
locally, but incorrectly uses PHY_INTERFACE_MODE_MAX as the array
length instead of BITS_TO_LONGS(). This allocates significantly more
space on the stack than necessary.

Move the macro to include/linux/phy.h and build it on top of
DECLARE_PHY_INTERFACE_MASK() to correctly size the bitmap, while still
initializing the stack variable to zero.

Signed-off-by: Coia Prant <coiaprant@gmail.com>
Cc: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/txgbe/txgbe_type.h | 3 ---
 include/linux/phy.h                             | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 757d1ce8d3ad..8071464c7b90 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -315,9 +315,6 @@ void txgbe_up(struct wx *wx);
 int txgbe_setup_tc(struct net_device *dev, u8 tc);
 void txgbe_do_reset(struct net_device *netdev, bool reinit);
 
-#define DECLARE_PHY_INTERFACE_MASK_ZERO(name) \
-	unsigned long name[PHY_INTERFACE_MODE_MAX] = { 0, }
-
 #define TXGBE_LINK_SPEED_UNKNOWN        0
 #define TXGBE_LINK_SPEED_10GB_FULL      4
 #define TXGBE_LINK_SPEED_25GB_FULL      0x10
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 21afc2082b17..43aba23109fa 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -159,6 +159,9 @@ typedef enum {
 #define DECLARE_PHY_INTERFACE_MASK(name) \
 	DECLARE_BITMAP(name, PHY_INTERFACE_MODE_MAX)
 
+#define DECLARE_PHY_INTERFACE_MASK_ZERO(name) \
+	DECLARE_PHY_INTERFACE_MASK(name) = { 0, }
+
 static inline void phy_interface_zero(unsigned long *intf)
 {
 	bitmap_zero(intf, PHY_INTERFACE_MODE_MAX);
-- 
2.47.3

[PATCH net-next v4 2/2] net: pcs: xpcs: make supported_interfaces authoritative

[Coia Prant]

pcs.supported_interfaces is the mask of interfaces a given xpcs
instance actually supports. The compat table describes what the
Synopsys IP can do in general, but a specific integration may support
a different subset. For example, the RK3568 XPCS supports 1000BASE-X,
SGMII and QSGMII in hardware, but does not implement the C73,
10GBASE-R or 2500BASE-X modes that the Synopsys compat table
advertises.

However, xpcs_find_compat() only walks desc->compat[] and does not look
at pcs.supported_interfaces. If a caller ever passes an interface
outside the mask, the core would still find and configure a compat
entry for it, contradicting the mask the glue already set. This was a
known gap: the mask was written but never consulted by the core.

Fix both sides:

- In xpcs_find_compat(), return NULL when the interface is not in
  pcs.supported_interfaces, so the mask is authoritative for what the
  core will configure.
- In the Rockchip glue, build a platform mask with the three modes
  supported by RK3568 (1000BASE-X, SGMII, QSGMII) and intersect it
  with the core's supported_interfaces using phy_interface_and().
  This narrows the mask inherited from the generic compat table so it
  reflects the actual SoC capabilities.

Note that QSGMII is currently filtered out (a no-op) because the core's
compat table does not support it yet, which demonstrates the intended
filtering behavior of this intersection. If the core adds QSGMII
support in the future, the glue will automatically pick it up.

This is a logical inconsistency rather than a user-visible bug in a
released kernel: the affected Rockchip platform is not present in any
released version, and the MAC side forces the phy mode, so phylink does
not normally pass an interface outside the mask. The patch makes the
mask the single source of truth for both the core and the glue.

Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 drivers/net/pcs/pcs-xpcs-rk.c | 10 ++++++++++
 drivers/net/pcs/pcs-xpcs.c    |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
index ad187fba4e9d..e29f473776b2 100644
--- a/drivers/net/pcs/pcs-xpcs-rk.c
+++ b/drivers/net/pcs/pcs-xpcs-rk.c
@@ -480,6 +480,7 @@ MODULE_DEVICE_TABLE(of, xpcs_rk_of_ids);
 
 struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *pcs, int port)
 {
+	DECLARE_PHY_INTERFACE_MASK_ZERO(supported_interfaces);
 	struct platform_device *pdev;
 	struct dw_xpcs_rk *pxpcs;
 	struct dw_xpcs *xpcs;
@@ -536,6 +537,15 @@ struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *pcs, int
 		return xpcs;
 	}
 
+	/*
+	 * The RK3568 XPCS supports 1000BASE-X, SGMII and QSGMII.
+	 */
+	__set_bit(PHY_INTERFACE_MODE_1000BASEX, supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_SGMII, supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_QSGMII, supported_interfaces);
+	phy_interface_and(xpcs->pcs.supported_interfaces, xpcs->pcs.supported_interfaces,
+			  supported_interfaces);
+
 	xpcs_config_eee_mult_fact(xpcs, pxpcs->eee_mult_fact);
 	put_device(&pdev->dev);
 	return xpcs;
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 6466e0ff2a98..46878a66064e 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -169,6 +169,9 @@ xpcs_find_compat(struct dw_xpcs *xpcs, phy_interface_t interface)
 {
 	const struct dw_xpcs_compat *compat;
 
+	if (!test_bit(interface, xpcs->pcs.supported_interfaces))
+		return NULL;
+
 	for (compat = xpcs->desc->compat; compat->supported; compat++)
 		if (compat->interface == interface)
 			return compat;
-- 
2.47.3

Re: [PATCH net-next v4 2/2] net: pcs: xpcs: make supported_interfaces authoritative

[netdev-bot+sashiko]

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] The commit message overstates what the core hunk fixes.
- [Low] The commit message leaves out the behaviour change that actually
  matters.
- [Low] xpcs_rk_create() puts PHY_INTERFACE_MODE_QSGMII into the platform
  mask, and its comment says "The RK3568 XPCS supports 1000BASE-X, SGMII…

--

net: pcs: xpcs: make supported_interfaces authoritative

This commit makes xpcs_find_compat() return NULL for any interface that
is not set in pcs.supported_interfaces. It also narrows the Rockchip
XPCS mask in xpcs_rk_create(). It does this by intersecting the mask
with a platform mask of 1000BASE-X, SGMII and QSGMII.

> However, xpcs_find_compat() only walks desc->compat[] and does not look
> at pcs.supported_interfaces. If a caller ever passes an interface
> outside the mask, the core would still find and configure a compat
> entry for it, contradicting the mask the glue already set. This was a
> known gap: the mask was written but never consulted by the core.

[Severity: Medium]
Does this describe the code as it was before this patch?

In the core, only xpcs_create() writes pcs.supported_interfaces. It does
so through xpcs_get_interfaces(), which builds the mask from the same
table that xpcs_find_compat() walks:

drivers/net/pcs/pcs-xpcs.c:xpcs_get_interfaces() {
	for (compat = xpcs->desc->compat; compat->supported; compat++)
		__set_bit(compat->interface, interfaces);
}

For every existing user, a set bit and a compat entry mean the same
thing. The new test_bit() in xpcs_find_compat() therefore can't reject
anything.

Before this patch, xpcs_rk_create() didn't touch pcs.supported_interfaces
either. So no mask had been "already set" by the glue. This same patch
adds the narrowing.

phylink also already checks the mask before it uses the PCS:

drivers/net/phy/phylink.c:phylink_validate_mac_and_pcs() {
    ...
		if (!phy_interface_empty(pcs->supported_interfaces) &&
		    !test_bit(state->interface, pcs->supported_interfaces)) {
			phylink_err(pl, "MAC returned PCS which does not support %s\n",
				    phy_modes(state->interface));
			return -EINVAL;
		}
    ...
}

The only path that skips phylink seems to be the exported
xpcs_get_an_mode(). It is called from stmmac_init_phy() with
plat->phy_interface. On RK that is always SGMII, which stays in the
mask.

Could the commit message say that the xpcs_find_compat() change is
defensive, and that the behaviour change comes from the glue narrowing
the mask?

> diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> index 6f0a110e32d24..b14b55dbd26cc 100644
> --- a/drivers/net/pcs/pcs-xpcs-rk.c
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c

[ ... ]

> @@ -536,6 +537,15 @@ struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *pcs, int
>  		return xpcs;
>  	}
>  
> +	/*
> +	 * The RK3568 XPCS supports 1000BASE-X, SGMII and QSGMII.
> +	 */
> +	__set_bit(PHY_INTERFACE_MODE_1000BASEX, supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_SGMII, supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_QSGMII, supported_interfaces);

[Severity: Low]
Should QSGMII be in this mask?

synopsys_xpcs_compat[] has no QSGMII entry. phy_interface_and() always
clears this bit, so it is dead code today.

The commit message says the glue "will automatically pick it up" once
the core supports QSGMII. However, nothing in the RK integration seems
to handle QSGMII:

- pcs-xpcs-rk.c only calls phy_init() and phy_power_on() on the SerDes.
  It never sets a SerDes mode for each interface.
- rk_gmac_probe() only installs rk_pcs_init() and rk_select_pcs() when
  plat_dat->phy_interface is PHY_INTERFACE_MODE_SGMII.
- rk_gmac_check_ops() and rk_gmac_powerup() only accept RGMII*, RMII
  and SGMII.

Another integration might later need a QSGMII entry in
synopsys_xpcs_compat[]. In that case the bit would survive here, and
stmmac_phylink_setup() would OR it into phylink's supported_interfaces
for dwmac-rk.

Would phylink then be able to select QSGMII, even though neither the
SerDes nor the MAC glue is set up for it?

> +	phy_interface_and(xpcs->pcs.supported_interfaces, xpcs->pcs.supported_interfaces,
> +			  supported_interfaces);

[Severity: Low]
Should the commit message mention that this narrowing changes the
phylink configuration of dwmac-rk?

rk_pcs_init() stores this XPCS in priv->hw->xpcs. stmmac_phylink_setup()
then ORs its mask into phylink's interface masks:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_phylink_setup() {
    ...
	if (pcs)
		phy_interface_or(config->supported_interfaces,
				 config->supported_interfaces,
				 pcs->supported_interfaces);
    ...
		if (pcs)
			phy_interface_or(config->lpi_interfaces,
					 config->lpi_interfaces,
					 pcs->supported_interfaces);
    ...
}

The lpi_interfaces update only happens when priv->dma_cap.eee is set
and STMMAC_FLAG_EEE_DISABLE is not.

Before this patch, the full synopsys_xpcs_compat[] set was advertised to
phylink for dwmac-rk as both supported and LPI-capable. That set is
USXGMII, 10GKR, 25GBASER, XLGMII, 50GBASER, LAUI, 100GBASEP, 10GBASER,
SGMII, 1000BASEX and 2500BASEX. After this patch, both masks shrink to
SGMII and 1000BASEX.

rk_select_pcs() returns the XPCS for any interface. That makes this
narrowing the part of the patch that actually changes behaviour.

The commit message only describes the xpcs_find_compat() change, and
says:

  the MAC side forces the phy mode, so phylink does not normally pass an
  interface outside the mask.

Could the message also describe the change to the dwmac-rk phylink
supported_interfaces and lpi_interfaces?

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