Re: [PATCH net-next v9 06/11] net: ethernet: oa_tc6: Support for vendor specific MMS

[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] Documentation/networking/oa-tc6-framework.rst describes
  oa_tc6_set_vend1_mms() as mapping VEND1 to a vendor MMS 'between 10…

--

net: ethernet: oa_tc6: Support for vendor specific MMS

This commit adds an exported helper, oa_tc6_set_vend1_mms(), so a vendor
driver can map MDIO_MMD_VEND1 to a vendor-specific MMS value. With this
change, oa_tc6_get_phy_c45_mms() returns the stored value for
MDIO_MMD_VEND1. The value defaults to -EOPNOTSUPP in oa_tc6_init().

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> index 0b6e216de8a1b..0c4f7d3f978fd 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> @@ -248,6 +248,18 @@ static int oa_tc6_update_standard_capability(struct oa_tc6 *tc6)
>  	return 0;
>  }
>  
> +/**
> + * oa_tc6_set_vend1_mms - Add vendor specific MDIO_MMD to OA TC6 MMS
> + * mapper value.
> + * @tc6: oa_tc6 struct.
> + * @mms: vendor defined MMS value for VEND1 mdio device.
> + */
> +void oa_tc6_set_vend1_mms(struct oa_tc6 *tc6, int mms)
> +{
> +	tc6->vend1_mms = mms;
> +}

[Severity: Low]
Should this setter reject values outside the 10..15 range?

Documentation/networking/oa-tc6-framework.rst says the helper is for
vendors that "may have used any MMS value between 10 and 15 as allowed by
the specification". However, the setter stores any int as-is and returns
void.

oa_tc6_mdiobus_read_c45() and oa_tc6_mdiobus_write_c45() only reject
mms < 0. The value is then passed as a u8 to oa_tc6_read_register_mms() and
oa_tc6_write_register_mms(), which do:

    mms_reg = FIELD_PREP(OA_TC6_REG_MMS_MASK, mms) | address;

OA_TC6_REG_MMS_MASK is GENMASK(19, 16). That means a value of 0..9 selects
a standard memory map directly, and 16 or 17 wraps to MMS0 or MMS1. VEND1
MDIO reads and writes would then go to a different register space without
any error.

The only caller in the series, s2500_probe(), passes
OA_TC6_PHY_C45_VS_MMS12, which is in range. Nothing in the tree hits this
today.

Would it make sense to add a range check that returns -EINVAL? Another
option is kernel-doc text stating that the caller must pass a value in
10..15.

> +EXPORT_SYMBOL_GPL(oa_tc6_set_vend1_mms);
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-s2500-mac-phy-support-v9-0-dcefe1d0bf0d%40onsemi.com