Re: [PATCH net-next v24 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 concern: genphy_c45_template_testmode() in
  drivers/net/phy/phy-c45.c and the new MDIO_PMA_10GBT_TESTMODE_* UAPI…
- [Low] genphy_c45_template_testmode() returns -EOPNOTSUPP when test_mode
  > MDIO_PMA_10GBT_TESTMODE_7.
- [Low] The new UAPI macro MDIO_PMA_10GBT_TESTMODE_MASK in
  include/uapi/linux/mdio.h is defined as GENMASK(15, 13).
- [Low] MDIO_PMA_10GBT_TESTMODE_NORMAL through MDIO_PMA_10GBT_TESTMODE_7
  are exported in include/uapi/linux/mdio.h as raw, unshifted field…
- [Low] The concern: genphy_c45_template_testmode() writes PMA register
  1.132 without checking phydev->pma_extable for 10GBASE-T or NBASE-T…
- [Low] The concern: genphy_c45_template_testmode() puts the PMA into a
  link-disrupting test mode without forcing speed, disabling AN, setting…

--

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

This commit adds genphy_c45_template_testmode(), an exported helper that
programs the 10GBASE-T PMA template test mode field (bits 15:13 of PMA
register 132). It also adds the register number and the field and value
definitions to include/uapi/linux/mdio.h.

> 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]
In this patch by itself, nothing in the tree uses this exported helper or
the new MDIO_PMA_10GBT_TESTMODE_* defines.

The next patch in the series, "net: phy: Add driver for Motorcomm Quad
2.5GbE phy", adds the callers in drivers/net/phy/motorcomm.c. They are
yt8824_utp_set_template_test_mode() and the YT8824 soft reset sequence.
That looks like the usual split where the API comes before its user.

Would it help to say in the commit message that the user comes in the
following patch?

> +{
> +	u16 ctrl;
> +
> +	if (test_mode > MDIO_PMA_10GBT_TESTMODE_7)
> +		return -EOPNOTSUPP;

[Severity: Low]
Should this be -EINVAL rather than -EOPNOTSUPP?

A test_mode above 7 is a bad argument from the caller. In phylib,
-EOPNOTSUPP usually means the PHY does not support the operation, and
callers often check for it so they can fall back.

The helper also never checks PMA capability. So it never returns
-EOPNOTSUPP in the case that errno normally describes.

The kernel-doc Return: section above gives only register read/write
failures as examples. Could it also mention this range check and the errno
it returns?

Every in-series caller passes a constant between 0 and 7, so this cannot
be hit today.

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

[Severity: Low]
This writes PMA register 1.132 without checking phydev->pma_extable for
10GBASE-T or NBASE-T ability. On a PMA without BASE-T ability, could this
write a reserved or vendor register and still return 0?

This may be intended. genphy_c45_fast_retrain() above also writes
MDIO_PMA_10GBR_FSRT_CSR without a capability check and leaves that to the
calling driver.

The only caller in the series is motorcomm.c, which uses it for its own
YT8824 PHY, and that PHY implements the register.

[Severity: Low]
A non-normal template test mode disrupts the link. This helper does not
coordinate with the phylib state machine, and it does not record the mode
so it can be restored later.

Is the intent that callers handle all of the sequencing, including
restoring MDIO_PMA_10GBT_TESTMODE_NORMAL?

That would match genphy_c45_loopback() and genphy_c45_fast_retrain(). The
in-series caller in motorcomm.c does restore NORMAL after TESTMODE_1, on
both the success path and the err_restore_normal path.

Could the kernel-doc state that callers must do this?

> +}
> +EXPORT_SYMBOL_GPL(genphy_c45_template_testmode);

[ ... ]

> 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]
GENMASK() is defined only for the kernel, in include/linux/bits.h. The
exported include/uapi/linux/bits.h provides only __GENMASK(),
__GENMASK_ULL() and __GENMASK_U128(). Also, mdio.h includes only
<linux/types.h> and <linux/mii.h>.

If userspace includes the installed <linux/mdio.h> and uses
MDIO_PMA_10GBT_TESTMODE_MASK, will it fail to compile because GENMASK is
undefined?

The existing MDIO_AN_C73_* defines in this header have the same problem,
but this adds another one. Could this use __GENMASK(15, 13) with
<linux/bits.h> included, as include/uapi/linux/pci_regs.h does, or just
0xe000?

> +#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 */

[Severity: Low]
These are unshifted field values. Most other value constants in this
header are pre-shifted register bits. Examples are
MDIO_PMA_10GBR_FSRT_ENABLE just above, the MDIO_PMA_10GBT_SWAPPOL_* bits,
and MDIO_USXGMII_SPD_* under the 0x0e00 mask.

genphy_c45_template_testmode() shifts these values with FIELD_PREP(), but
the header does not say they need shifting. If someone writes
MDIO_PMA_10GBT_TESTMODE_3 directly into register 1.132, wouldn't bits
15:13 be left at 0 (Normal)?

These names become permanent UAPI. Would it be clearer to pre-shift them,
or to say in the comment that they are field values for
MDIO_PMA_10GBT_TESTMODE_MASK?

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