Re: [PATCH net-next v2 5/5] net: phy: mxl-gpy: support MxL86211C lane mapping

[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 binding and the driver disagree about which nodes
  `maxlinear,lane-asp-map` is valid on and how long it may be.

--

net: phy: mxl-gpy: support MxL86211C lane mapping

This commit lets the MxL86211C take an optional maxlinear,lane-asp-map DT
property. The property programs the mapping from physical TPI lanes to
analog front ends in VSPEC1_LANE_ASP_MAP. When the property is absent, the
hardware reset mapping is left alone.

> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> index 8bc119655f104..acd5ad01a285d 100644
> --- a/drivers/net/phy/mxl-gpy.c
> +++ b/drivers/net/phy/mxl-gpy.c

[ ... ]

> @@ -423,6 +430,42 @@ static int mxl86211c_configure_mdio_voltage(struct phy_device *phydev,
>  			      VSPEC1_PM_CTRL_MDIO_VOL, val);
>  }
>  
> +static int mxl86211c_configure_lane_asp_map(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	u32 lane_asp_map[4];
> +	u16 val;
> +	unsigned int seen = 0;
> +	int i, ret;
> +
> +	if (!device_property_present(dev, "maxlinear,lane-asp-map"))
> +		return 0;
> +
> +	ret = device_property_read_u32_array(dev, "maxlinear,lane-asp-map",
> +					     lane_asp_map,
> +					     ARRAY_SIZE(lane_asp_map));

[Severity: Low]
Should a maxlinear,lane-asp-map with more than 4 cells be rejected here?

With the OF backend, device_property_read_u32_array() ends up in
of_property_read_variable_u32_array() with sz_max set to 0.
of_find_property_value_of_size() treats that as "no upper bound":

drivers/of/property.c:of_find_property_value_of_size() {
    ...
	if (max && prop->length > max)
		return ERR_PTR(-EOVERFLOW);
    ...
}

So a value like <0 1 3 2 1> is accepted. Only the first 4 cells are
used, and nothing is logged. The binding says the property has exactly
4 items.

The range and duplicate checks below still keep bad values out of
VSPEC1_LANE_ASP_MAP. The problem is that the driver and the binding
accept different things.

The schema may not catch this case either. The driver binds by PHY ID
through PHY_ID_MATCH_MODEL(PHY_ID_MXL86211C), whatever the node's
compatible is. However, maxlinear,gpy2xx.yaml (from the earlier
"dt-bindings: net: maxlinear: add MxL86211C VDDP supply" patch) only
selects nodes that carry this exact compatible:

select:
  properties:
    compatible:
      contains:
        const: ethernet-phy-idc133.5400
  required:
    - compatible

An ethernet-phy@N node without that compatible is only checked against
ethernet-phy.yaml, and that schema allows additional properties. So a
malformed maxlinear,lane-asp-map on such a node would pass dtbs_check,
and the driver would still read it.

There is a related issue in "dt-bindings: net: maxlinear: add MxL86211C
lane mapping support". The allOf if/then/else tests the same compatible
that select already requires:

    else:
      properties:
        vddp-supply: false
        maxlinear,lane-asp-map: false

Can this else branch ever be evaluated? No other schema references this
one, so the ban on these properties for other GPY PHYs never seems to
take effect.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016112033.949026-1-robert.marko%40sartura.hr