Re: [PATCH net-next] net: phy: marvell: add support for active-low and active-high LEDs
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-15 20:26:36
Also in:
lkml
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. --
quoted hunk ↗ jump to hunk
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