Re: [PATCH net v3 3/3] net: phy: qca808x: handle the active-high LED polarity mode

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

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

- [High] Incomplete fix in drivers/net/phy/qcom/qca808x.c: the new `case
  PHY_LED_ACTIVE_HIGH: active_low = false;` in…

--

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

This adds a PHY_LED_ACTIVE_HIGH case to qca808x_led_polarity_set() so that
a device tree LED node carrying the 'active-high' property no longer makes
the switch fall into the default case and return -EINVAL, which previously
failed phy_probe().

[Severity: High]
Does this new case leave the LED driven active-low, i.e. the inverse of
what the device tree asked for?

Before this change priv->led_polarity_mode could only hold -1 (never
requested) or 1 (active-low requested).  With the new case an explicit
'active-high' node now stores 0:

drivers/net/phy/qcom/qca808x.c:qca808x_led_polarity_set() {
	...
	/* Save the last PHY polarity mode */
	priv->led_polarity_mode = active_low;

	return phy_modify_mmd(phydev, MDIO_MMD_AN,
			      QCA808X_MMD7_LED_POLARITY_CTRL,
			      QCA808X_LED_ACTIVE_HIGH,
			      active_low ? 0 : QCA808X_LED_ACTIVE_HIGH);
}

The only other place QCA808X_LED_ACTIVE_HIGH gets programmed tests for -1
specifically:

drivers/net/phy/qcom/qca808x.c:qca808x_config_init() {
	/* Default to LED Active High if active-low not in DT */
	if (priv->led_polarity_mode == -1) {
		ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
				       QCA808X_MMD7_LED_POLARITY_CTRL,
				       QCA808X_LED_ACTIVE_HIGH);
	...
}

so the newly reachable value 0 skips that write.

phy_init_hw() runs drv->soft_reset (qca808x_soft_reset ->
genphy_soft_reset, BMCR_RESET) before drv->config_init, and it is called
from phy_attach_direct() and again from mdio_bus_phy_resume().  Since
BIT(6) has to be set explicitly for active high:

drivers/net/phy/qcom/qca808x.c:
/* QSDK sets by default 0x46 to this reg that sets BIT 6 for
 * LED to active high. It's not clear what BIT 3 and BIT 4 does.
 */
#define QCA808X_LED_ACTIVE_HIGH			BIT(6)

isn't the single probe-time phy_modify_mmd() write discarded by that reset
and then never restored, leaving the LED active-low?

That would make a device tree that explicitly states 'active-high' behave
worse than one that omits the property entirely, since the omitted case
keeps led_polarity_mode == -1 and does get the bit re-asserted from
config_init.

There is a second effect of the overloaded value: with
priv->led_polarity_mode == 0 the driver's global-polarity bookkeeping
records "active high" while the register says active low, so a second LED
also requesting active-high passes the mismatch check against state that
does not reflect the hardware.

Should qca808x_config_init() also cover the explicit active-high case, for
example by re-asserting the bit whenever led_polarity_mode != 1, or by
using a distinct sentinel for "not requested" rather than overloading -1
and 0?

Related to this, could the commit message paragraph:

  "active_low is already false when no polarity mode is requested, which
  programs QCA808X_LED_ACTIVE_HIGH, so the request is already satisfied and
  only the case label is missing."

be revisited?  In the no-property case led_polarity_set is not called at
all (of_phy_led() only calls it when modes is non-zero), so what programs
QCA808X_LED_ACTIVE_HIGH there is the led_polarity_mode == -1 branch of
qca808x_config_init(), which the new case label makes unreachable for an
explicit active-high request.

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