Re: [PATCH net-next v10 3/4] net: dsa: motorcomm: Add LED support

[David Yang]

On Wed, Aug 19, 2026 at 1:10 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> > @@ -4581,6 +4580,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
> > +
> >       return 0;
> >  }
>
> [Severity: Low]
> Can this dev_warn() ever be reached?  yt921x_leds_setup() discards the
> result of every yt921x_leds_setup_port() call and has a single
>
>         return 0;
>
> as its only exit, so res is always zero here.

yt921x_leds_setup() is allowed to fail (not now, but in the future),
like all other init callbacks.

> > +static int
> > +yt921x_led_force_set(struct yt921x_priv *priv, int port, int group, bool on)
> > +{
> > +     struct yt921x_port *pp = &priv->ports[port];
> > +     struct yt921x_led *led = pp->leds[group];
> > +     u32 ctrl;
> > +     u32 mask;
> > +
> > +     led->use_cycle = false;
> > +     led->use_duty = false;
> > +
> > +     mask = YT921X_LED2_PORT_FORCEn_M(group);
> > +     ctrl = on ? YT921X_LED2_PORT_FORCEn_ON(group) :
> > +            YT921X_LED2_PORT_FORCEn_OFF(group);
> > +     return yt921x_reg_update_bits(priv, YT921X_LED2_PORTn(port), mask,
> > +                                   ctrl);
> > +}
>
> [Severity: Low]
> Should use_cycle/use_duty only be cleared once the register update has
> succeeded?  If the SMI/MDIO transfer in yt921x_reg_update_bits() fails,
> the LED keeps hardware-blinking while the driver records that this group no
> longer uses the shared per-port duty and cycle fields.
>
> The same pattern appears in yt921x_led_trigger_set(), which clears both
> flags before two fallible yt921x_reg_update_bits() calls and never
> restores them on failure.

IO transfers are not expected to fail. If that does happen, we won't
have much control over HW consistency, nor can safely revert the
register changes. In that case, sacrificing an LED seems a better
solution.

>
> > +     /* The chip seems to jam a while if changing duty directly */
> > +     res = yt921x_reg_read(priv, YT921X_LED2_PORTn(port), &val);
> > +     if (res)
> > +             return res;
> > +
> > +     ctrl = val & ~YT921X_LED2_PORT_FORCEn_M(group);
> > +     ctrl |= YT921X_LED2_PORT_FORCEn_DONTCARE(group);
> > +     if (val != ctrl) {
> > +             res = yt921x_reg_write(priv, YT921X_LED2_PORTn(port), ctrl);
> > +             if (res)
> > +                     return res;
> > +     }
>
> [ ... ]
>
> > +     res = yt921x_reg_update_bits(priv, YT921X_LED1_PORTn(port), mask, ctrl);
> > +     if (res)
> > +             return res;
>
> [Severity: Low]
> This is a multi-register sequence: FORCE is first switched to DONTCARE, so
> the LED immediately follows whatever activity bits happen to be programmed,
> then the shared YT921X_LED1_PORTn duty and OTHER_BLINK fields are updated,
> then forced blinking is selected.  Both intermediate error returns leave
> the earlier writes in place with no unwind.
>
> Since the conflict check earlier in this function trusts use_cycle and
> use_duty to decide whether another group owns the shared timing fields, can
> a later request silently retime a sibling LED that is still blinking, or
> leave an LED driven by stale activity bits?

Again register reversion dilemma.

> > +             yt921x_leds_setup_port(priv, port);
> > +             /* Allow partial configuration: LEDs are optional */
> > +     }
> > +
> > +     return 0;
> > +}
>
> [Severity: Low]
> These two functions are the ones that make the dev_warn() in
> yt921x_dsa_setup() unreachable, as noted above.  Is the "LEDs are optional"
> policy meant to coexist with that warning, or should one of them go?

dev_warn() in yt921x_dsa_setup() is for interface consistency.