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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] net: phy: mxl-gpy: potential overwrite of upper bits in
  VSPEC1_LANE_ASP_MAP

--

> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> --- a/drivers/net/phy/mxl-gpy.c
> +++ b/drivers/net/phy/mxl-gpy.c
> @@ -440,6 +447,50 @@ static const struct regulator_desc mxl86211c_mdio_desc = {
>  	.owner = THIS_MODULE,
>  };
>  
> +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));
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(lane_asp_map); i++) {
> +		if (lane_asp_map[i] > 3 || seen & BIT(lane_asp_map[i]))
> +			return -EINVAL;
> +
> +		seen |= BIT(lane_asp_map[i]);
> +	}
> +
> +	val = FIELD_PREP(VSPEC1_LANE_ASP_MAP_A, lane_asp_map[0]) |
> +	      FIELD_PREP(VSPEC1_LANE_ASP_MAP_B, lane_asp_map[1]) |
> +	      FIELD_PREP(VSPEC1_LANE_ASP_MAP_C, lane_asp_map[2]) |
> +	      FIELD_PREP(VSPEC1_LANE_ASP_MAP_D, lane_asp_map[3]);
> +
> +	return phy_write_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_LANE_ASP_MAP,
> +			     val);
> +}
> +
> +static int mxl86211c_config_init(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = mxl86211c_configure_lane_asp_map(phydev);
> +	if (ret)
> +		return ret;
> +
> +	return gpy21x_config_init(phydev);
> +}
> +
>  static int mxl86211c_probe(struct phy_device *phydev)
>  {
>  	struct device *dev = &phydev->mdio.dev;

[Severity: Medium]
mxl86211c_configure_lane_asp_map() writes VSPEC1_LANE_ASP_MAP with
a plain phy_write_mmd(). That means bits 15:8 of the register are
always written as zero, even though the patch only defines the four
2-bit lane fields in bits 7:0.

Those upper bits might hold something: reserved bits with a non-zero
reset value, or an unrelated control field. If so, they get cleared
whenever maxlinear,lane-asp-map is present in DT. The commit message
says the reset configuration is left alone when the property is
absent. When the property is present, this write also changes
whatever else lives in that register, not just the lane mapping.

Does the datasheet say bits 15:8 of VSPEC1 0x14 are unused and safe
to write as zero? If not, would phy_modify_mmd() with a mask built
from VSPEC1_LANE_ASP_MAP_A..D be safer? mxl86211c_probe() already
uses phy_modify_mmd() for VSPEC1_PM_CTRL, so that would also match
the rest of the driver.

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