Re: [PATCH net-next v22 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 new uAPI macro MDIO_PMA_10GBT_TESTMODE_MASK in
  include/uapi/linux/mdio.h is defined as GENMASK(15, 13).
- [Low] net: phy: 'template' is a misnomer for standard 10GBASE-T test
  mode

--

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

This commit adds genphy_c45_template_testmode(), which programs the
10GBASE-T PMA template test mode field. It also adds new defines for
the register (132) and for the Normal and TestMode1..TestMode7 values
to include/uapi/linux/mdio.h.

> 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
> @@ -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 */
>  #define MDIO_PMA_10GBT_SNR	133	/* 10GBASE-T SNR margin, lane A.
>  					 * Lanes B-D are numbered 134-136. */

[ ... ]

> @@ -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]
Is "template" the right name for this helper? IEEE 802.3 calls PMA/PMD
register 1.132 the 10GBASE-T test mode register. As far as I can tell,
the word template is not part of the register's name in the standard.

The same wording shows up in several places:

  - the exported symbol genphy_c45_template_testmode()
  - its kerneldoc
  - the "PMA 10GBASE-T Template Test Mode Register" comment and the
    "Template ..." value comments in include/uapi/linux/mdio.h
  - the subject line ("Template Control register")

The new defines themselves are called MDIO_PMA_10GBT_TESTMODE_*, so the
function name no longer matches the register it programs. Someone
reading the code or grepping for the 10GBASE-T test mode register won't
easily find this helper.

This has no functional impact. Still, once the symbol is exported from
the core PHY library, other drivers may start using it and the name
becomes harder to change.

If template refers to something in the Motorcomm datasheet or in the
test procedure, could you say so in the commit message? Otherwise, would
something like genphy_c45_10gbt_testmode() be closer to the standard's
naming, with the comments and subject line updated to match?

[Severity: Low]
Will userspace be able to use this macro? GENMASK() comes from the
kernel-internal include/linux/bits.h. This uapi header only includes:

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

So a userspace program that includes <linux/mdio.h> and uses
MDIO_PMA_10GBT_TESTMODE_MASK will not compile unless it defines GENMASK
itself.

The MDIO_AN_C73_*_MASK defines in this header already have the same
problem, but this change adds another one to the exported header.

Would __GENMASK(15, 13) from include/uapi/linux/bits.h, or a plain 0xe000,
work better here?

This isn't a bug, but the new register define above uses spaces:

#define MDIO_PMA_10GBT_TESTMODE 132 /* Test mode control */

while the lines around it use tabs to line up the values and comments. The
same goes for the new MDIO_PMA_10GBT_TESTMODE_* value defines.

> +#define MDIO_PMA_10GBT_TESTMODE_NORMAL 0x0 /* Template Normal */
> +#define MDIO_PMA_10GBT_TESTMODE_1 0x1 /* Template TestMode1 */
> +#define MDIO_PMA_10GBT_TESTMODE_2 0x2 /* Template TestMode2 */
> +#define MDIO_PMA_10GBT_TESTMODE_3 0x3 /* Template TestMode3 */
> +#define MDIO_PMA_10GBT_TESTMODE_4 0x4 /* Template TestMode4 */
> +#define MDIO_PMA_10GBT_TESTMODE_5 0x5 /* Template TestMode5 */
> +#define MDIO_PMA_10GBT_TESTMODE_6 0x6 /* Template TestMode6 */
> +#define MDIO_PMA_10GBT_TESTMODE_7 0x7 /* Template TestMode7 */
> +

[ ... ]

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