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

[David Yang]

On Wed, Sep 2, 2026 at 8:56 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> > @@ -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?
>
> > @@ -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.

> > +     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).

> > +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.