Re: [PATCH net-next v26 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 new UAPI macro MDIO_PMA_10GBT_TESTMODE_MASK in
  include/uapi/linux/mdio.h is defined as GENMASK(15, 13).

--

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

This adds the 10GBASE-T PMA Template Test Mode register definitions to
the UAPI mdio.h header. It also adds a genphy_c45_template_testmode()
helper that selects one of the eight test modes (Normal and
TestMode1..TestMode7) through phy_modify_mmd().

> 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 userspace that uses MDIO_PMA_10GBT_TESTMODE_MASK from the installed
<linux/mdio.h> still build?

GENMASK is defined in include/linux/bits.h, and that header is not
exported. Under include/uapi, include/uapi/linux/bits.h only provides
__GENMASK, __GENMASK_ULL and __GENMASK_U128. Also, include/uapi/linux/mdio.h
only includes <linux/types.h> and <linux/mii.h>.

A userspace program that expands this macro, for example a tool that
uses SIOCGMIIREG/SIOCSMIIREG on a C45 PHY, would hit an undeclared
GENMASK unless it defines one itself. The kernel build is fine because
kernel code gets GENMASK from include/linux/bits.h.

This header already does the same thing in MDIO_AN_C73_0_S_MASK,
MDIO_AN_C73_0_E_MASK and MDIO_AN_C73_1_T_MASK. This patch adds one more
macro that userspace can't use, though.

Most masks in this file are hex literals. Would 0xe000 work here instead?

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