Re: [PATCH net-next v25 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] The patch adds a uAPI constant, MDIO_PMA_10GBT_TESTMODE_MASK, to
  include/uapi/linux/mdio.h (line 325) and defines it as GENMASK(15, 13).
- [Low] Several names in this patch use 'Template', which is not the IEEE
  802.3 term: the subject ('Template Control register'), the body…

--

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

This commit adds an exported helper, genphy_c45_template_testmode(). The
helper writes the test mode field (bits 15:13) of the 10GBASE-T PMA
register 1.132. It also adds the register and field definitions to the
uAPI mdio.h header.

> 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)

[Severity: Low]
This isn't a bug, but is "template" the right name here? The register
define in this same patch calls it test mode control:

#define MDIO_PMA_10GBT_TESTMODE 132 /* Test mode control */

IEEE 802.3 calls register 1.132 the 10GBASE-T test mode register, and
bits 15:13 are the test mode control field. In 802.3, "template" means
the masks that test mode output is measured against, not the register.

The "Template" wording shows up in the subject and commit message, in
the exported genphy_c45_template_testmode() name and its kerneldoc, and
in the uAPI comments ("Template TestMode1" .. "Template TestMode7").

The exported genphy_c45_* name and the uAPI text are hard to change
after merge. Could they use the standard test mode naming instead?

[ ... ]

> 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]
Will this build for userspace? GENMASK() is only defined in the
kernel-internal include/linux/bits.h. The exported uAPI headers only
provide __GENMASK(), in include/uapi/linux/bits.h.

include/uapi/linux/mdio.h only includes:

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

So a userspace program that includes the installed <linux/mdio.h> and
uses MDIO_PMA_10GBT_TESTMODE_MASK would fail to compile, because
GENMASK is undefined. The kernel build still works because phy-c45.c
gets GENMASK() from kernel headers.

The same file already has this problem with MDIO_AN_C73_0_S_MASK,
MDIO_AN_C73_0_E_MASK and MDIO_AN_C73_1_T_MASK. This patch adds one more
constant of that kind.

Would a literal 0xe000, or __GENMASK() with the matching include, keep
the header self-contained?

[ ... ]

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