Re: [PATCH net-next] net: phy: marvell: add support for active-low and active-high LEDs

[netdev-bot+sashiko]

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

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

- [Medium] Incomplete mask for 2-bit LED polarity field in
  phy_modify_paged.

--

> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -3585,6 +3589,41 @@ static int m88e1318_led_hw_control_get(struct phy_device *phydev, u8 index,
>  	return marvell_get_led_rules(index, rules, mode);
>  }
>  
> +static int m88e1318_led_polarity_set(struct phy_device *phydev, int index,
> +				     unsigned long modes)
> +{
> +	bool force_active_low = false, force_active_high = false;
> +	u32 mode;
> +
> +	if (index >= M88E1318S_MAX_LEDS)
> +		return -EINVAL;
> +
> +	for_each_set_bit(mode, &modes, __PHY_LED_MODES_NUM) {
> +		switch (mode) {
> +		case PHY_LED_ACTIVE_LOW:
> +			force_active_low = true;
> +			break;
> +		case PHY_LED_ACTIVE_HIGH:
> +			force_active_high = true;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (force_active_low)
> +		return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE,
> +					MII_88E1318S_PHY_LED_POL,
> +					MII_88E1318S_PHY_LED_POL_ACT_HIGH(index), 0);
> +
> +	if (force_active_high)
> +		return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE,
> +					MII_88E1318S_PHY_LED_POL, 0,
> +					MII_88E1318S_PHY_LED_POL_ACT_HIGH(index));
> +
> +	return -EINVAL;
> +}
> +
>  static int marvell_probe(struct phy_device *phydev)
>  {
>  	struct marvell_priv *priv;

[Severity: Medium]
The LED polarity field in page 3 register 0x11 is two bits wide per
LED, but m88e1318_led_polarity_set() only ever touches the lower bit
of the field. MII_88E1318S_PHY_LED_POL_ACT_HIGH(idx) expands to
1 << 2 * idx, so for the active-low case the phy_modify_paged() call
clears just that one bit and leaves the upper bit of the pair
untouched; for the active-high case the mask argument is 0, so no
bits are cleared at all and only the low bit is set.

That means the result depends on whatever the field held before. If
the LED was left in one of the encodings that has the upper bit set
(e.g. the tri-state/high-Z variant configured by the bootloader or by
strapping), asking for active-low leaves the pair as 0b10 - i.e. the
requested polarity is silently not applied - and asking for
active-high produces 0b11, which is not one of the documented
encodings. Since this callback is reached from the LED polarity
plumbing on any DT/sysfs configuration, the LED can end up in a state
that neither the user nor a later led_hw_control_get() agrees with.

The fix is to read-modify-write the whole two-bit field: pass a mask
covering both bits (something like GENMASK(2 * index + 1, 2 * index),
or a dedicated MII_88E1318S_PHY_LED_POL_MASK(idx) macro) and the
desired encoding as the set value, so both the active-low and
active-high paths fully define the field. Since you have the 88E1318
hardware and I do not have the TRM in front of me, could you confirm
from the datasheet what the upper bit of each pair means and what the
correct encodings for plain active-low and active-high are? If the
upper bit selects tri-state-when-off behaviour that we deliberately
want to preserve, please say so and add a comment, but then the
active-high path still must not be able to produce the reserved 0b11
combination.

While you are there, please also use BIT()/GENMASK() and parenthesise
the shift in the new macro rather than open-coding 1 << 2 * (idx).

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914200847.445375-1-olek2%40wp.pl