Re: [PATCH net-next v11 4/5] net: dsa: motorcomm: Add LED support

On Wed, Sep 2, 2026 at 8:56 AM Jakub Kicinski [off-list ref] wrote:

quoted

@@ -4225,6 +4224,15 @@ static int yt921x_edata_read(struct yt921x_priv *priv, u8 addr, u8 *valp)
      return yt921x_edata_read_cont(priv, addr, valp);
 }

+static void yt921x_dsa_teardown(struct dsa_switch *ds)
+{
+     struct yt921x_priv *priv = to_yt921x_priv(ds);
+
+#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
+     yt921x_leds_remove(priv);
+#endif
+}
+

[Severity: Medium]

Can removing the LEDs only from the DSA teardown callback leave the LED
class devices registered after the port netdevs have been freed?

net/dsa/dsa.c tears the tree down in this order:

dsa_tree_teardown()
    dsa_tree_teardown_ports()
        dsa_port_teardown()
            dsa_user_destroy(dp->user);   /* ends in free_netdev() */
            dp->user = NULL;
    dsa_tree_teardown_switches()
        dsa_switch_teardown()
            ds->ops->teardown(ds)        /* yt921x_leds_remove() */

So between free_netdev() and the dp->user = NULL assignment, dp->user is a
dangling pointer while the LEDs are still registered and reachable from
sysfs.

yt921x_cled_hw_control_get_device() reads dp->user / dp->conduit with no
rtnl_lock, no RCU and takes no reference:

        if (dsa_port_is_user(dp))
                return !dp->user ? NULL : &dp->user->dev;

and ledtrig-netdev dereferences the returned device immediately, with
dev_name() in netdev_trig_activate() and to_net_dev() in
validate_net_dev().

Would an "echo netdev > /sys/class/leds/<led>/trigger" racing with switch
unbind, or with the dsa_tree_setup() error path, touch a freed
net_device here? Would removing the LEDs per port before the netdevs go
away, or validating dp->user under rtnl_lock, avoid that?

quoted

@@ -4581,6 +4589,12 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
      if (res)
              return res;

+#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
+     res = yt921x_leds_setup(priv);
+     if (res)
+             dev_warn(dev, "Failed to setup LEDs: %d\n", res);
+#endif
+

Just a question: I don't see any protection against this in other DSA
led drivers, like mv88e6 or qca8k.

[Severity: Low]

Can this dev_warn() ever be reached? yt921x_leds_setup() in leds.c ends
with an unconditional return 0 and drops the result of
yt921x_leds_setup_port():

        dsa_switch_for_each_port(dp, ds) {
                ...
                yt921x_leds_setup_port(priv, port);
                /* Allow partial configuration: LEDs are optional */
        }

        return 0;

yt921x_leds_setup_port() in turn discards every yt921x_led_setup() return
value, so the -EINVAL it produces for port >= YT921X_LED_PORT_NUM is also
thrown away.

Should the error be propagated, or should this branch in yt921x_dsa_setup()
be dropped?

Intended, explained in the response for the previous version.

[Severity: Low]

Is yt921x_led_trigger_get() intended to be the inverse of
yt921x_led_trigger_set()?

TRIGGER_NETDEV_LINK is encoded as the union of the two duplex masks, and
this read-back loop ORs in the flags of every entry whose mask bits are all
set, with no early break. yt921x_led_trigger_set() instead matches the LINK
entry first and breaks once the flags are consumed.

So a LED programmed with only BIT(TRIGGER_NETDEV_LINK) appears to read back
as LINK | HALF_DUPLEX | FULL_DUPLEX. ledtrig-netdev keeps that value as the
trigger state at activation:

        rc = led_cdev->hw_control_get(led_cdev, &mode);
        if (!rc)
                trigger_data->mode = mode;

Would /sys/class/leds/<led>/half_duplex and full_duplex then report 1 even
though only link indication was requested?

Yes.

quoted

+     if (group >= YT921X_LED_GROUP_NUM) {
+             dev_err(dev, "Invalid LED reg %u for port %d\n", group, port);
+             return -EINVAL;
+     }

[Severity: Low]

This isn't a bug, but the device tree contract introduced here (a per-port
leds child node, a per-LED reg in 0..2, LEDs only on ports below
YT921X_LED_PORT_NUM) is not described in
Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml by this
commit.

The follow-on patch in the same series, "dt-bindings: net: dsa: yt921x: Add
LEDs definition example", adds the description text and a worked leds
example, so this is only a question about ordering within the series.

What the schema still does not express is a maximum for reg, so a device
tree with led@3 validates cleanly and is rejected only at probe time by the
dev_err() above. Would adding that constraint to the binding be worthwhile?

I don't see a good notation to impose the constraint (from sibling DSA
bindings).

quoted

+int yt921x_leds_setup(struct yt921x_priv *priv)
+{
+     struct dsa_switch *ds = &priv->ds;
+     struct dsa_port *dp;
+
+     /* LEDs are always enabled. There is no way to disable them altogether
+      * (as far as I know).
+      */

[Severity: Low]

This isn't a bug, just a question about scope: the driver programs only the
per-port registers (YT921X_LED0/1/2_PORTn) and YT921X_LED_PAR_INV, and
never reads YT921X_LED_CTRL, YT921X_LED_PAR_PORTS or the parallel/serial
mapping registers declared in leds.h.

As far as I can tell that is fine, since yt921x_dsa_setup() runs
yt921x_chip_reset() (which writes YT921X_RST_HW, described in the code as
almost the same as a GPIO hard reset) before yt921x_leds_setup(), so the
LED block is back at the parallel mode plus 1:1 mapping default that the
commit message names.

Is a check of YT921X_LED_CTRL_MODE_M worth adding anyway, or is the reset
considered sufficient?

Reset is sufficient.