[PATCH net] net: phy: dp83867: restore the LED polarity after a soft reset

[PATCH net] net: phy: dp83867: restore the LED polarity after a soft reset

[Donggeun Yoo]

dp83867_led_polarity_set() programs the LEDCR2 polarity bit for a
DT-configured LED. It runs once, from phy_probe() via of_phy_leds().
Every phy_init_hw() afterwards (phy_attach_direct(), mdio_bus_phy_resume(),
and MAC drivers such as fec_main.c) calls .soft_reset first, and
dp83867_phy_reset() issues DP83867_SW_RESET, which restores register
defaults. config_init does not touch LEDCR2 and the requested value is
not cached, so a DT-configured LED polarity is lost from the first
attach onward.

The soft reset is the only path that clears LEDCR2, and phy_init_hw()
always follows it with config_init. Cache the requested bits and reapply
them from dp83867_config_init(). aqr_gen1_config_init() recovers from the
same reset the same way (61578f679378).

Fixes: 447b80a9330e ("net: phy: dp83867: Add support for active-low LEDs")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
---
The soft-reset regression was flagged by an automated analysis of an
earlier posting of the active-high series. Compile-tested and
call-chain-traced only; I have no affected hardware.

 drivers/net/phy/dp83867.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 88255e92b4cd..c9c3b0062cc7 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -196,6 +196,8 @@ struct dp83867_private {
 	bool set_clk_output;
 	u32 clk_output_sel;
 	bool sgmii_ref_clk_en;
+	u16 led_polarity;
+	u16 led_polarity_mask;
 };
 
 static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -896,6 +898,15 @@ static int dp83867_config_init(struct phy_device *phydev)
 			       mask, val);
 	}
 
+	/* Restore the LED polarity dropped by the soft reset in phy_init_hw() */
+	if (dp83867->led_polarity_mask) {
+		ret = phy_modify(phydev, DP83867_LEDCR2,
+				 dp83867->led_polarity_mask,
+				 dp83867->led_polarity);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -1141,8 +1152,10 @@ static int dp83867_led_hw_control_get(struct phy_device *phydev, u8 index,
 static int dp83867_led_polarity_set(struct phy_device *phydev, int index,
 				    unsigned long modes)
 {
+	struct dp83867_private *dp83867 = phydev->priv;
+	u16 mask = DP83867_LED_POLARITY(index);
 	/* Default active high */
-	u16 polarity = DP83867_LED_POLARITY(index);
+	u16 polarity = mask;
 	u32 mode;
 
 	for_each_set_bit(mode, &modes, __PHY_LED_MODES_NUM) {
@@ -1154,8 +1167,11 @@ static int dp83867_led_polarity_set(struct phy_device *phydev, int index,
 			return -EINVAL;
 		}
 	}
-	return phy_modify(phydev, DP83867_LEDCR2,
-			  DP83867_LED_POLARITY(index), polarity);
+
+	dp83867->led_polarity_mask |= mask;
+	dp83867->led_polarity = (dp83867->led_polarity & ~mask) | polarity;
+
+	return phy_modify(phydev, DP83867_LEDCR2, mask, polarity);
 }
 
 static unsigned int dp83867_inband_caps(struct phy_device *phydev,
-- 
2.53.0

Re: [PATCH net] net: phy: dp83867: restore the LED polarity after a soft reset

[Andrew Lunn]

On Tue, Sep 08, 2026 at 08:49:01PM +0900, Donggeun Yoo wrote:
> dp83867_led_polarity_set() programs the LEDCR2 polarity bit for a
> DT-configured LED. It runs once, from phy_probe() via of_phy_leds().
> Every phy_init_hw() afterwards (phy_attach_direct(), mdio_bus_phy_resume(),
> and MAC drivers such as fec_main.c) calls .soft_reset first, and
> dp83867_phy_reset() issues DP83867_SW_RESET, which restores register
> defaults. config_init does not touch LEDCR2 and the requested value is
> not cached, so a DT-configured LED polarity is lost from the first
> attach onward.

What about the other bits in LEDCR2? dp83867_led_brightness_set()
might of been used to turn the LED on/off in order to show the state
of my caps lock key, etc.

	Andrew

Re: [PATCH net] net: phy: dp83867: restore the LED polarity after a soft reset

[Donggeun Yoo]

On Tue, Sep 08, 2026 at 04:14:58PM +0200, Andrew Lunn wrote:
> What about the other bits in LEDCR2? dp83867_led_brightness_set()
> might of been used to turn the LED on/off in order to show the state
> of my caps lock key, etc.

You're right. LEDCR2 is cleared by SW_RESET -- v1 already showed the
polarity there is lost, and the DRV_EN/DRV_VAL on/off bits from
led_brightness_set() live in the same register, so a manually driven LED
loses its state too. Polarity-only was a partial fix.

For v2 I'll shadow what the driver programs into LEDCR2 (value plus a
written-bits mask, updated in the setters) and replay it from
config_init(), which runs right after the soft reset. That restores both
polarity and the software-driven on/off state. The runtime setters run
under phydev->lock but the resume-path phy_init_hw() does not, so v2 will
define how the shadow read is serialized against a concurrent setter
rather than leaving it racy.

The function nibble in LEDCR1 is lost the same way. The DP83867IR/CR
datasheet (Rev J) section 7.5.5.3 says the global software reset resets
all internal circuits including the IEEE-defined and extended registers
to defaults, and LEDCR1 (0x18) is an extended register, so SW_RESET
clears it too. The netdev trigger only re-issues the function on its next
set_baseline_state(), i.e. the next link or activity event, not on the
reset, so an offloaded LED whose link stays down after a resume would sit
at the reset-default function indefinitely -- the same failure this patch
fixes, one register up. So v2 shadows and replays LEDCR1 as well:
config_init() rewrites the last function the driver programmed, which is
the trigger's own last intent, and the trigger's next update writes the
identical value.

I did consider pushing this into phylib so every soft-resetting PHY would
benefit. The core could reapply the DT polarity, and that alone would let
aquantia drop its leds_active_low/high cache (61578f679378). But it can't
restore the software-driven on/off state -- the core has no way to tell a
software-driven LED from one offloaded to a hw trigger without reaching
into the trigger's private state -- so that half stays in the driver
regardless, and splitting the restore across two layers seemed worse than
keeping it in one. So I dropped the core route and kept it all in the
driver. I may well have missed a core mechanism that would change that; if
so, let me know and I'll rework it.

Thanks,
Donggeun