Re: [PATCH net-next v27 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] include/uapi/linux/mdio.h line 325 defines the new exported uAPI
  macro MDIO_PMA_10GBT_TESTMODE_MASK as GENMASK(15, 13).

--

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

This commit adds definitions for the 10GBASE-T PMA Template Test Mode
register (PMA register 132, bits 15:13) to the mdio uAPI header. It also
adds a genphy_c45_template_testmode() helper, which selects one of the
eight test modes (Normal through TestMode7).

> 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

[ ... ]

> @@ -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 MDIO_PMA_10GBT_TESTMODE_MASK compile when userspace uses it?

GENMASK() is defined in the kernel-only include/linux/bits.h. The
exported include/uapi/linux/bits.h only provides these:

    #define __GENMASK(h, l) ...
    #define __GENMASK_ULL(h, l) ...
    #define __GENMASK_U128(h, l) ...

Also, include/uapi/linux/mdio.h only includes <linux/types.h> and
<linux/mii.h>.

So a userspace program that includes the installed <linux/mdio.h> and
uses MDIO_PMA_10GBT_TESTMODE_MASK would fail with GENMASK undeclared. The
in-tree build works because phy-c45.c gets GENMASK() from the kernel
headers. CONFIG_UAPI_HEADER_TEST would not catch this either, because
the macro is never expanded there.

Could this use __GENMASK(15, 13) with the uapi <linux/bits.h>, or the
literal 0xe000?

The header already has the same problem in MDIO_AN_C73_0_S_MASK,
MDIO_AN_C73_0_E_MASK and MDIO_AN_C73_1_T_MASK, but this patch adds a new
exported symbol with it. The later patches in the series leave this
definition unchanged.

> +#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/20261020021929.3028490-1-kyle.switch%40motor-comm.com