[PATCH net] net: phy: dp83867: handle the active-high LED polarity mode

[PATCH net] net: phy: dp83867: handle the active-high LED polarity mode

[Donggeun Yoo]

Commit a274465cc3be ("net: phy: support 'active-high' property for PHY
LEDs") added PHY_LED_ACTIVE_HIGH and made of_phy_led() set the matching
bit in the modes mask when a LED node carries the 'active-high'
property. dp83867 was not part of that series.

dp83867_led_polarity_set() only recognizes PHY_LED_ACTIVE_LOW, so
PHY_LED_ACTIVE_HIGH falls through to the default case and returns -EINVAL.
of_phy_led() propagates the error, of_phy_leds() drops the LEDs registered
so far and passes it on, and phy_probe() fails. A device tree marking a
DP83867 LED as 'active-high', which leds/common.yaml allows and
ethernet-phy.yaml references for led@N nodes, thus stops the PHY from
probing.

Active high is what the function programs when no polarity mode is
requested at all, so the initial value of polarity already satisfies the
request and only the case label is missing.

The same series updated mxl-gpy in commit eb89c79c1b8f ("net: phy:
mxl-gpy: correctly describe LED polarity") and aquantia in
commit 9d55e68b19f2 ("net: phy: aquantia: correctly describe LED
polarity override").

Fixes: a274465cc3be ("net: phy: support 'active-high' property for PHY LEDs")
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
---
Compile tested only. I have no DP83867 hardware, so the failure path was
traced by inspection through of_phy_led(), of_phy_leds() and phy_probe().

The polarity bit sense assumed here matches the datasheet: LEDCR2 bits
2, 6, 10 and 14 are LED_x_POLARITY with 1 = active high, 0 = active low,
and they reset to 1 (SNLS484J, Table 8-24).

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

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 88255e92b4cd..61a941aa02d9 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -1150,6 +1150,9 @@ static int dp83867_led_polarity_set(struct phy_device *phydev, int index,
 		case PHY_LED_ACTIVE_LOW:
 			polarity = 0;
 			break;
+		case PHY_LED_ACTIVE_HIGH:
+			polarity = DP83867_LED_POLARITY(index);
+			break;
 		default:
 			return -EINVAL;
 		}

base-commit: 5a3f7a683aee56e1f15c9d53041f3236767eaae7
-- 
2.53.0

Re: [PATCH net] net: phy: dp83867: handle the active-high LED polarity mode

[netdev-bot+sashiko]

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

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

- [Medium] Incomplete fix of the a274465cc3be regression class: after
  this patch, two other in-tree ->led_polarity_set() implementers still…

Pre-existing issues:
- [Medium] The LEDCR2 polarity bit programmed from DT is written during
  phy_probe() and is then discarded by the driver's own software reset,…
- [Low] dp83867_led_polarity_set() (drivers/net/phy/dp83867.c:1141) uses
  the DT-supplied LED index in DP83867_LED_POLARITY(index) ==…

--

net: phy: dp83867: handle the active-high LED polarity mode

This adds a PHY_LED_ACTIVE_HIGH case to dp83867_led_polarity_set() so that
a device tree LED node carrying the 'active-high' property no longer makes
the callback return -EINVAL and abort phy_probe().  The new case programs
the same polarity bit that the function already uses as its default.

> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 88255e92b4cd..61a941aa02d9 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -1150,6 +1150,9 @@ static int dp83867_led_polarity_set(struct phy_device *phydev, int index,
>  		case PHY_LED_ACTIVE_LOW:
>  			polarity = 0;
>  			break;
> +		case PHY_LED_ACTIVE_HIGH:
> +			polarity = DP83867_LED_POLARITY(index);
> +			break;
>  		default:
>  			return -EINVAL;
>  		}

[Severity: Medium]
Are there other drivers left with the same problem after this patch?

The commit message lists mxl-gpy and aquantia as the drivers the original
series updated, and presents dp83867 as the one that "was not part of that
series".  Two more ->led_polarity_set() implementers still reject
PHY_LED_ACTIVE_HIGH.

drivers/net/phy/dp83td510.c:dp83td510_led_polarity_set() {
	u16 polarity = DP83TD510E_LED_POLARITY(index);
	...
		case PHY_LED_ACTIVE_LOW:
			polarity = 0;
			break;
		default:
			return -EINVAL;
	...
}

drivers/net/phy/qcom/qca808x.c:qca808x_led_polarity_set() {
	bool active_low = false;
	...
		case PHY_LED_ACTIVE_LOW:
			active_low = true;
			break;
		default:
			return -EINVAL;
	...
}

In both cases the pre-initialised value is already active high, exactly as
in dp83867, so the missing case label is the only difference.  Since
of_phy_led() sets PHY_LED_ACTIVE_HIGH from the device tree property and
of_phy_leds() -> phy_probe() propagate the error, boards with an
'active-high' LED on a DP83TD510 or QCA808x PHY still fail to probe.  All
other implementers (air_an8801, as21xxx, intel-xway, mxl-gpy,
aquantia_leds, mtk-ge-soc) already have the case.

Would it make sense to send this as a small series covering those two as
well, so the class of failures the Fixes: tag refers to is fully closed?

[Severity: Low]
This isn't a bug introduced by this patch, but should
dp83867_led_polarity_set() validate index the way the other LED callbacks
in this driver do?

DP83867_LED_POLARITY(index) expands to BIT((index) * 4 + 2) and is
evaluated three times here (the initialiser, the new active-high case, and
the phy_modify() mask) with no bound check, while the neighbouring
callbacks start with:

drivers/net/phy/dp83867.c:dp83867_led_brightness_set() {
	if (index >= DP83867_LED_COUNT)
		return -EINVAL;
	...
}

drivers/net/phy/dp83867.c:dp83867_led_mode() {
	if (index >= DP83867_LED_COUNT)
		return -EINVAL;
	...
}

The core does not filter this either.  of_phy_led() reads led@N's 'reg'
and only rejects values above U8_MAX before calling led_polarity_set(),
and ethernet-phy.yaml puts no numeric limit on the LED index.

For reg values 4 to 7 the computed bit does not fit in the u16 polarity
variable or the u16 phy_modify() mask, so the read-modify-write becomes a
no-op that still returns success, and the LED classdev is registered as if
the polarity had been programmed.  For larger reg values the shift count in
BIT() reaches the width of the shifted type, which is undefined behaviour
and reported by UBSAN.

Since the patch touches this function anyway, would adding
'if (index >= DP83867_LED_COUNT) return -EINVAL;' at the top be a
reasonable companion change?

[Severity: Medium]
This is a pre-existing issue, but does the polarity programmed here survive
the driver's own soft reset?

This phy_modify() is the only write to the LEDCR2 polarity bits in the
driver, and it runs from phy_probe() -> of_phy_leds() -> of_phy_led().
Afterwards, phy_init_hw() (called from phy_attach_direct() and from MDIO
bus resume) runs the registered .soft_reset:

drivers/net/phy/dp83867.c:dp83867_phy_reset() {
	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESET);
	if (err < 0)
		return err;
	...
}

That reset clears registers, which is why dp83867_phy_reset() re-programs
PHYCTRL and DSP_FFE_CFG right after it, and why dp83867_config_init()
re-programs CFG2, the FIFO depth and so on.  Neither of those functions
touches DP83867_LEDCR1 or DP83867_LEDCR2, and struct dp83867_private does
not cache the requested polarity, so an 'active-low' LED ends up running
with the reset default the commit message describes as active high.

The aquantia driver keeps the requested polarity and re-applies it:

drivers/net/phy/aquantia/aquantia_main.c:aqr107_config_init() {
	/* Restore LED polarity state after reset */
	for_each_set_bit(led_idx, &priv->leds_active_low, AQR_MAX_LEDS) {
		ret = aqr_phy_led_active_low_set(phydev, led_idx, true);
	...
}

Would latching the requested polarity in struct dp83867_private and
re-applying it from dp83867_config_init() be the right fix here?  The newly
added active-high case happens to be unaffected only because it programs
the reset default.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903022839.4006614-1-donggeunyoo.kernel%40gmail.com

Re: [PATCH net] net: phy: dp83867: handle the active-high LED polarity mode

[Paolo Abeni]

On 9/3/26 4:28 AM, Donggeun Yoo wrote:
> Commit a274465cc3be ("net: phy: support 'active-high' property for PHY
> LEDs") added PHY_LED_ACTIVE_HIGH and made of_phy_led() set the matching
> bit in the modes mask when a LED node carries the 'active-high'
> property. dp83867 was not part of that series.
> 
> dp83867_led_polarity_set() only recognizes PHY_LED_ACTIVE_LOW, so
> PHY_LED_ACTIVE_HIGH falls through to the default case and returns -EINVAL.
> of_phy_led() propagates the error, of_phy_leds() drops the LEDs registered
> so far and passes it on, and phy_probe() fails. A device tree marking a
> DP83867 LED as 'active-high', which leds/common.yaml allows and
> ethernet-phy.yaml references for led@N nodes, thus stops the PHY from
> probing.
> 
> Active high is what the function programs when no polarity mode is
> requested at all, so the initial value of polarity already satisfies the
> request and only the case label is missing.
> 
> The same series updated mxl-gpy in commit eb89c79c1b8f ("net: phy:
> mxl-gpy: correctly describe LED polarity") and aquantia in
> commit 9d55e68b19f2 ("net: phy: aquantia: correctly describe LED
> polarity override").
> 
> Fixes: a274465cc3be ("net: phy: support 'active-high' property for PHY LEDs")
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
> ---
> Compile tested only. I have no DP83867 hardware, so the failure path was
> traced by inspection through of_phy_led(), of_phy_leds() and phy_probe().
> 
> The polarity bit sense assumed here matches the datasheet: LEDCR2 bits
> 2, 6, 10 and 14 are LED_x_POLARITY with 1 = active high, 0 = active low,
> and they reset to 1 (SNLS484J, Table 8-24).
There are more drivers with the same issue, and Sashiko suggests to fix
all of them in the same series. I think instead this fix can be applied
standalone, to avoid more traffic on the ML.

Still an explict ack from someone actually owing the H/W would be nice.
Waiting a little more for that.

/P

Re: [PATCH net] net: phy: dp83867: handle the active-high LED polarity mode

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  3 Sep 2026 11:28:38 +0900 you wrote:
> Commit a274465cc3be ("net: phy: support 'active-high' property for PHY
> LEDs") added PHY_LED_ACTIVE_HIGH and made of_phy_led() set the matching
> bit in the modes mask when a LED node carries the 'active-high'
> property. dp83867 was not part of that series.
> 
> dp83867_led_polarity_set() only recognizes PHY_LED_ACTIVE_LOW, so
> PHY_LED_ACTIVE_HIGH falls through to the default case and returns -EINVAL.
> of_phy_led() propagates the error, of_phy_leds() drops the LEDs registered
> so far and passes it on, and phy_probe() fails. A device tree marking a
> DP83867 LED as 'active-high', which leds/common.yaml allows and
> ethernet-phy.yaml references for led@N nodes, thus stops the PHY from
> probing.
> 
> [...]

Here is the summary with links:
  - [net] net: phy: dp83867: handle the active-high LED polarity mode
    https://git.kernel.org/netdev/net/c/36a45facedd5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html