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 <redacted>
---
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
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
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