Re: [PATCH net-next v2] net: phy: marvell: add support for PHY LEDs polarity modes

[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

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] The polarity and tristate settings written to page 3 reg 17
  (MII_88E1318S_PHY_LED_POL) are programmed only once.

--

net: phy: marvell: add support for PHY LEDs polarity modes

This commit adds a led_polarity_set callback for the 88E1318 and 88E1510
PHYs. The callback programs the per-LED active-high and high-impedance
bits in page 3, register 0x11.

> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f71cffa884062..d57fcabfec2b9 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c

[ ... ]

> @@ -3585,6 +3590,36 @@ 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)
> +{

[ ... ]

> +	return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE,
> +				MII_88E1318S_PHY_LED_POL,
> +				MII_88E1318S_PHY_LED_POL_ACT_HIGH(index) |
> +				MII_88E1318S_PHY_LED_POL_HIGH_IMP(index), val);
> +}

[Severity: Medium]
Can the polarity and high-impedance settings be lost after the PHY goes
through a hardware reset?

The register is only written once, from probe:

phy_probe()
  of_phy_leds()
    of_phy_led()
      m88e1318_led_polarity_set()

The requested modes are not saved in struct marvell_priv. Neither
m88e1318_config_init() nor m88e1510_config_init() (through
marvell_config_init()) rewrites MII_88E1318S_PHY_LED_POL.

phy_detach() puts the PHY into reset:

drivers/net/phy/phy_device.c:phy_detach() {
    ...
	/* Assert the reset signal */
	phy_device_reset(phydev, 1);
    ...
}

On the next attach, phy_attach_direct()->phy_init_hw() releases reset and
runs config_init, but of_phy_leds() is not called again.

Take a board with a PHY reset GPIO and a MAC driver that connects the PHY
in ndo_open and disconnects it in ndo_stop. Wouldn't each "ip link set
down" followed by "ip link set up" put the LED pins back to their reset
default polarity and drive state?

The same thing looks possible after resume if the PHY lost power, via
mdio_bus_phy_resume()->phy_init_hw().

qca808x avoids this. It keeps led_polarity_mode in its priv structure and
reapplies it from qca808x_config_init(). Would a similar approach work
here, with the per-LED modes saved in marvell_priv and restored from
config_init?

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