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

[netdev-bot+sashiko]

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] DISCARDED (resolved later in the series):
  genphy_c45_template_testmode() has no in-tree caller at this commit,…
- [Low] include/uapi/linux/mdio.h:325 defines the newly exported macro
  MDIO_PMA_10GBT_TESTMODE_MASK as GENMASK(15, 13).

--

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

This adds a new phylib helper, genphy_c45_template_testmode(), which writes
the 10GBASE-T PMA template test mode field, along with the register and
field definitions in the exported mdio.h header.  Eight values are defined,
Normal plus TestMode1 through TestMode7.

Two small observations below.

> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index 870920311f9a0..c5f5753f7194b 100644
> --- 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);

[Severity: Low]
This isn't a bug, just a note for the record.  At this commit
genphy_c45_template_testmode() has no in-tree caller.

The following patch in the series, "net: phy: Add driver for Motorcomm Quad
2.5GbE phy", adds the consumers, for example in
yt8824_utp_set_template_test_mode():

	return genphy_c45_template_testmode(phydev, test_mode);

along with uses of MDIO_PMA_10GBT_TESTMODE_1 and
MDIO_PMA_10GBT_TESTMODE_NORMAL in yt8824_soft_reset() and
yt8824_restore_working_status(), so the transient state resolves within the
series.

> diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
> index 06f4bc3c20c71..353ca13c64fca 100644
> --- a/include/uapi/linux/mdio.h
> +++ b/include/uapi/linux/mdio.h

[ ... ]

> @@ -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 expand this new mask?  GENMASK() comes from the kernel-only
include/linux/bits.h, and the exported header only pulls in:

	#include <linux/types.h>
	#include <linux/mii.h>

Neither uapi mii.h nor uapi ethtool.h defines GENMASK or BIT, so a
userspace translation unit that includes <linux/mdio.h> and actually uses
MDIO_PMA_10GBT_TESTMODE_MASK, for instance in a constant initializer, would
fail to compile with GENMASK undeclared.

Would __GENMASK() from include/uapi/linux/bits.h, or a plain 0xe000, be
preferable here?

The in-kernel user in phy-c45.c is unaffected, since it reaches GENMASK and
FIELD_PREP through the in-kernel linux/mdio.h chain.  Note the header
already carries a few bare GENMASK() uses that predate this patch, so this
is an existing wart in the file rather than newly broken userspace, but new
uapi additions probably should not repeat it.

> +#define MDIO_PMA_10GBT_TESTMODE_NORMAL 0x0 /* Template Normal */
> +#define MDIO_PMA_10GBT_TESTMODE_1 0x1 /* Template TestMode1 */

[ ... ]

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