Re: [PATCH net-next 3/3] net: phy: realtek: add LED support for RTL8261C/D
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-24 10:21:16
Also in:
linux-devicetree, lkml
Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin. Low priority / nit picks for your consideration: - [Low] Asymmetric LED register programming: rtl8261x_led_hw_control_set() (drivers/net/phy/realtek/realtek_main.c,… - [Low] rtl8261x_led_hw_control_get() (drivers/net/phy/realtek/realtek_main.c ~2570-2620) reports the… - [Low] rtl8261x_led_hw_is_supported() (drivers/net/phy/realtek/realtek_main.c ~2542-2568) accepts asymmetric… - [Low] Out-of-range shift evaluated before the bounds check: in both rtl8261x_led_hw_control_set() and rtl8261x_led_brightness_set() the… Pre-existing issues: - [Low] Claimed non-atomic hardware programming sequence: rtl8261x_led_hw_control_set() and rtl8261x_led_brightness_set() each… -- net: phy: realtek: add LED support for RTL8261C/D Adds LED callbacks for the RTL8261C/D reusing the RTL8221B LED register block, with four LEDs, extra 5G/10G link speed bits, a new per-LED output enable bit and caching of the configured polarity in the private data. The callbacks are wired into the RTL8261C_CG and RTL8261CE_CG driver entries. A few questions below.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c index 04d397f0a2d80..b1f020aa77604 100644 --- a/drivers/net/phy/realtek/realtek_main.c +++ b/drivers/net/phy/realtek/realtek_main.c@@ -183,12 +183,15 @@ #define RTL822X_VND2_LCR_LINK_100 BIT(1) #define RTL822X_VND2_LCR_LINK_1000 BIT(2) #define RTL822X_VND2_LCR_LINK_2500 BIT(5) +#define RTL822X_VND2_LCR_LINK_5000 BIT(6) +#define RTL822X_VND2_LCR_LINK_10000 BIT(4) #define RTL822X_VND2_LCR6 0xd040 #define RTL822X_VND2_LED_ACT(x) BIT(x) #define RTL822X_VND2_LCR7 0xd044 #define RTL822X_VND2_LED_POLAR(x) BIT(x) +#define RTL822X_VND2_LED_EN(x) BIT((x) + 4)
[ ... ]
quoted hunk ↗ jump to hunk
@@ -305,6 +308,7 @@ #define FW_SUB_MAGIC_8261D 0x32363144 #define RTL8261X_POLL_TIMEOUT_MS 100 #define RTL8261X_MAX_MMD_DEV 31 +#define RTL8261X_LED_COUNT 4
[ ... ]
quoted hunk ↗ jump to hunk
@@ -2530,6 +2539,215 @@ static int rtl822xb_led_brightness_set(struct phy_device *phydev, u8 index, +static int rtl8261x_led_hw_is_supported(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + const unsigned long act_mask = BIT(TRIGGER_NETDEV_RX) | + BIT(TRIGGER_NETDEV_TX);
[ ... ]
quoted hunk ↗ jump to hunk
+ /* RX and TX are not differentiated, they are not possible + * without combination with a link trigger. + */ + if ((rules & act_mask) && !(rules & link_mask)) + return -EOPNOTSUPP; + + return 0; +}
[Severity: Low]
Should rtl8261x_led_hw_is_supported() also reject asymmetric activity
requests? As written, LINK|RX without TX (and LINK|TX without RX) passes
validation, but the hardware bit those map to is a single combined
RX/TX activity bit:
rtl8261x_led_hw_control_set()
act = test_bit(TRIGGER_NETDEV_RX, &rules) ||
test_bit(TRIGGER_NETDEV_TX, &rules);
rtl8261x_led_hw_control_get()
if (val & RTL822X_VND2_LED_ACT(index)) {
__set_bit(TRIGGER_NETDEV_RX, rules);
__set_bit(TRIGGER_NETDEV_TX, rules);
}
Since hw_is_supported() returns 0, the netdev trigger offloads the request
instead of falling back to software blinking, so an rx=1 tx=0 configuration
blinks on transmit as well, which differs from what
Documentation/ABI/testing/sysfs-class-led-trigger-netdev describes for
writing 0 to tx.
rtl8211f_led_hw_is_supported() in the same file handles the same hardware
limitation by rejecting the mismatch:
/* RX and TX are not differentiated, either both are set or not set. */
if (!(rules & BIT(TRIGGER_NETDEV_RX)) ^ !(rules & BIT(TRIGGER_NETDEV_TX)))
return -EOPNOTSUPP;
Would following that variant instead of the rtl822xb one be preferable
here?
quoted hunk ↗ jump to hunk
+ +static int rtl8261x_led_hw_control_get(struct phy_device *phydev, u8 index, + unsigned long *rules) +{ + const u16 all_links = RTL822X_VND2_LCR_LINK_10 | + RTL822X_VND2_LCR_LINK_100 | + RTL822X_VND2_LCR_LINK_1000 | + RTL822X_VND2_LCR_LINK_2500 | + RTL822X_VND2_LCR_LINK_5000 | + RTL822X_VND2_LCR_LINK_10000; + int val; + + if (index >= RTL8261X_LED_COUNT) + return -EINVAL; + + val = phy_read_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_LED(index)); + if (val < 0) + return val; + + if (val & RTL822X_VND2_LCR_LINK_10) + __set_bit(TRIGGER_NETDEV_LINK_10, rules);
[ ... ]
quoted hunk ↗ jump to hunk
+ if (val & RTL822X_VND2_LCR_LINK_10000) + __set_bit(TRIGGER_NETDEV_LINK_10000, rules); + + if ((val & all_links) == all_links) + __set_bit(TRIGGER_NETDEV_LINK, rules);
[Severity: Low] Can this report a rule combination that the netdev trigger itself refuses? When all six speed bits are set, the per-speed bits stay set and TRIGGER_NETDEV_LINK is added on top. netdev_trig_activate() in drivers/leds/trigger/ledtrig-netdev.c takes the bitmap verbatim: rc = led_cdev->hw_control_get(led_cdev, &mode); if (!rc) trigger_data->mode = mode; and netdev_led_attr_store() rejects that same combination: if (test_bit(TRIGGER_NETDEV_LINK, &mode) && (test_bit(TRIGGER_NETDEV_LINK_10, &mode) || ... )) return -EINVAL; So with an all-speeds hardware configuration, would every subsequent write to link_10 ... rx/tx/full_duplex fail with -EINVAL until the user clears link? Should the getter clear the per-speed bits when it sets TRIGGER_NETDEV_LINK? The same pattern already exists in rtl822xb_led_hw_control_get() and rtl8211e_led_hw_control_get(), so any canonicalisation probably belongs there too, but this patch adds a new instance.
quoted hunk ↗ jump to hunk
+ + val = phy_read_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_LCR6); + if (val < 0) + return val; + + if (val & RTL822X_VND2_LED_ACT(index)) { + __set_bit(TRIGGER_NETDEV_RX, rules); + __set_bit(TRIGGER_NETDEV_TX, rules); + } + + return 0; +}
[Severity: Low] Should rtl8261x_led_hw_control_get() also look at LCR7? It reads only RTL822X_VND2_LED(index) and the LCR6 activity bit, while rtl8261x_led_hw_control_set() is the only place that programs RTL822X_VND2_LED_EN(index), so the get path cannot tell whether the output driver is actually enabled. The commit message says the enable bit is one "which firmware normally leaves set already". If a board boots with link condition bits set but the enable bit clear, netdev_trig_activate() imports those rules and also does: trigger_data->hw_control = true; ... rc = led_cdev->hw_control_get(led_cdev, &mode); if (!rc) trigger_data->mode = mode; without calling hw_control_set(), so nothing re-asserts the enable bit. Would the LED then stay dark while userspace is told the triggers are offloaded, with software blinking suppressed as well? Should a clear enable bit be reported as not offloaded?
quoted hunk ↗ jump to hunk
+ +static int rtl8261x_led_hw_control_set(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + struct rtl8261x_priv *priv = phydev->priv; + u16 lcr7 = RTL822X_VND2_LED_EN(index); + u16 val = 0; + bool act; + int ret; + + if (index >= RTL8261X_LED_COUNT) + return -EINVAL;
[Severity: Low] Is the shift in the initialiser evaluated before index is validated? RTL822X_VND2_LED_EN(index) expands to UL(1) << (index + 4), and it runs ahead of the index >= RTL8261X_LED_COUNT test. The same ordering appears in rtl8261x_led_brightness_set(). index comes from the DT LED node reg property, and of_phy_led() in drivers/net/phy/phy_device.c only bounds it at U8_MAX: err = of_property_read_u32(led, "reg", &index); if (err) return err; if (index > U8_MAX) return -EINVAL; So a device tree with led@3c or higher registers a LED classdev whose brightness write reaches this initialiser with a shift count of 64 or more on an unsigned long, which is undefined behaviour and should produce a UBSAN shift-out-of-bounds report. Can the initialisation be moved below the bounds check? The existing rtl822xb_* and rtl8211e_* callbacks compute their masks only after validating index. [ ... ]
quoted hunk ↗ jump to hunk
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, + RTL822X_VND2_LED(index), val); + if (ret < 0) + return ret; + + act = test_bit(TRIGGER_NETDEV_RX, &rules) || + test_bit(TRIGGER_NETDEV_TX, &rules); + + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_LCR6, + RTL822X_VND2_LED_ACT(index), act ? + RTL822X_VND2_LED_ACT(index) : 0); + if (ret < 0) + return ret; + + /* Enable the output and restore the configured polarity, which + * led_brightness_set() may have used as a level. + */ + if (priv->led_active_low & BIT(index)) + lcr7 |= RTL822X_VND2_LED_POLAR(index); + + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, RTL822X_VND2_LCR7, + RTL822X_VND2_LED_EN(index) | + RTL822X_VND2_LED_POLAR(index), lcr7); +}
[Severity: Low] This isn't a bug introduced by this patch, but the three MDIO transactions here (per-LED link conditions, LCR6 activity, LCR7 enable/polarity) return on the first failure without undoing the earlier writes, so a mid-sequence failure leaves the LED enabled with new link conditions, stale activity state and the manual-mode polarity level. On the success path the LED is also briefly inverted between the first and the last write because LCR7's polarity bit doubles as the manual level. rtl822xb_led_hw_control_set() and rtl822xb_led_brightness_set() already use the same unwound sequence on the same register block, so this is the existing convention in the driver rather than something new, and the same applies to rtl8261x_led_brightness_set() here. Worth noting only in case a rollback or a quiesce-first ordering is wanted for the new callbacks. [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921161817.565185-1-spolack%2Bgit%40mailbox.org