Re: [PATCH net-next v21 2/3] net: phy: Add support for Template Control register for PMA

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The naming doesn't match the register.
- [Low] The patch adds a new UAPI constant, `#define
  MDIO_PMA_10GBT_TESTMODE_MASK GENMASK(15, 13)`, at…
- [Low] phy-c45: genphy_c45_template_testmode() returns -EOPNOTSUPP for
  invalid argument

--

net: phy: Add support for Template Control register for PMA

This adds definitions to the uapi mdio.h header for the 10GBASE-T PMA
test mode register (1.132) and its test mode field. It also adds a
genphy_c45_template_testmode() helper in drivers/net/phy/phy-c45.c,
which writes one of the modes Normal or TestMode1..TestMode7 into that
field.

> diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
> index 06f4bc3c20c7..353ca13c64fc 100644
> --- a/include/uapi/linux/mdio.h
> +++ b/include/uapi/linux/mdio.h
> @@ -63,6 +63,7 @@
>  /* Media-dependent registers. */
>  #define MDIO_PMA_10GBT_SWAPPOL	130	/* 10GBASE-T pair swap & polarity */
>  #define MDIO_PMA_10GBT_TXPWR	131	/* 10GBASE-T TX power control */
> +#define MDIO_PMA_10GBT_TESTMODE 132 /* Test mode control */

[Severity: Low]
This isn't a bug, but is "Template" the right name here?

The word "Template" appears in three places:

  - The subject line: "Add support for Template Control register for
    PMA".
  - The new exported helper: genphy_c45_template_testmode() in
    drivers/net/phy/phy-c45.c.
  - The comment on every new field define.

IEEE 802.3 calls register 1.132 the 10GBASE-T test mode register, and
bits 15:13 "Test mode control". The define above already uses that
wording.

"Template" looks like vendor wording, and it becomes part of a generic
exported phylib symbol. Would names based on the standard terminology
work better for the helper and the subject line?

There are also a few small style nits:

  - This define uses spaces where the defines around it are
    tab-aligned.
  - The new MDIO_PMA_10GBT_TESTMODE_* field defines are not
    tab-aligned either.
  - The comment "/* PMA 10GBASE-T Template Test Mode Register*/" has no
    space before the terminator.

[ ... ]

> @@ -320,6 +321,17 @@
>  /* PMA 10GBASE-R Fast Retrain status and control register. */
>  #define MDIO_PMA_10GBR_FSRT_ENABLE	0x0001	/* Fast retrain enable */
>  
> +/* PMA 10GBASE-T Template Test Mode Register*/
> +#define MDIO_PMA_10GBT_TESTMODE_MASK GENMASK(15, 13) /* Template test mode */

[Severity: Low]
Can userspace actually use this new mask?

GENMASK() is kernel-internal and comes from include/linux/bits.h. The
exported include/uapi/linux/bits.h only provides __GENMASK(),
__GENMASK_ULL() and __GENMASK_U128().

This header only includes <linux/types.h> and <linux/mii.h>, and
neither of them provides GENMASK.

So a userspace program that includes <linux/mdio.h> and uses
MDIO_PMA_10GBT_TESTMODE_MASK would fail to build, because GENMASK is
undeclared. The UAPI header compile test won't catch this, since the
macro is only expanded where it is used.

The MDIO_AN_C73_* masks in this header already have the same problem.
This patch adds another exported symbol that depends on it, though.

Could this use a literal 0xe000, or __GENMASK(15, 13) from
<linux/bits.h>, instead?

[ ... ]

> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -1408,6 +1408,29 @@ int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable)
>  }
>  EXPORT_SYMBOL_GPL(genphy_c45_fast_retrain);
>  
> +/**
> + * genphy_c45_template_testmode - configure template testmode registers
> + * @phydev: target phy_device struct
> + * @test_mode: testmode includes Normal to Test mode 7
> + *
> + * Description: Set template testmode include Normal to Test mode 7
> + *
> + * Return: 0 on success, or a negative error code on failure (e.g. register
> + * read/write error).
> + */
> +int genphy_c45_template_testmode(struct phy_device *phydev, u16 test_mode)
> +{
> +	u16 ctrl;
> +
> +	if (test_mode > MDIO_PMA_10GBT_TESTMODE_7)
> +		return -EOPNOTSUPP;
> +
> +	ctrl = FIELD_PREP(MDIO_PMA_10GBT_TESTMODE_MASK, test_mode);
> +	return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_10GBT_TESTMODE,
> +			      MDIO_PMA_10GBT_TESTMODE_MASK, ctrl);
> +}
> +EXPORT_SYMBOL_GPL(genphy_c45_template_testmode);
> +
>  /**
>   * genphy_c45_plca_get_cfg - get PLCA configuration from standard registers
>   * @phydev: target phy_device struct

[Severity: Low]
Should this return -EINVAL rather than -EOPNOTSUPP?

A test_mode above MDIO_PMA_10GBT_TESTMODE_7 cannot be encoded in the
3-bit MDIO_PMA_10GBT_TESTMODE_MASK field at all. That makes it a bad
argument from the caller of genphy_c45_template_testmode(), not a
feature the PHY lacks.

-EOPNOTSUPP reads as "this PHY does not support test modes". Suppose
a driver logs the error or passes it up towards userspace. The person
debugging it would then look at the hardware instead of at the caller
that passed the bad value.

Elsewhere in phylib, -EOPNOTSUPP is generally used for a missing
capability and -EINVAL for out-of-range arguments. Would -EINVAL fit
better here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924075048.4060414-1-kyle.switch%40motor-comm.com