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

[Jakub Kicinski <kuba@kernel.org>]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 5

- [Medium] `yt921x_led_blink_set()` (via the `blink_set` callback
  `yt921x_cled_blink_set()`) special-cases only the both-delays-zero…
- [Medium] LED class devices are removed only from the DSA `.teardown`
  callback (`yt921x_dsa_teardown()` → `yt921x_leds_remove()`), but…
- [Low] Dead error-handling plumbing in the LED setup chain:
  `yt921x_leds_setup()` (drivers/net/dsa/motorcomm/leds.c) ends with an…
- [Low] Concern (assessed false positive): the driver programs only per-
  port LED registers (YT921X_LED0/1/2_PORTn) plus YT921X_LED_PAR_INV and…
- [Low] Concern (assessed resolved later in the series): the driver
  introduces an undocumented DT contract — a per-port `leds` child node,…
- [Low] `yt921x_led_trigger_get()` (hw_control_get) is not the inverse of
  `yt921x_led_trigger_set()` (hw_control_set): TRIGGER_NETDEV_LINK is…
- [Low] The `revoke`/`revoke_led2` rollback in `yt921x_led_setup()`
  writes back a whole-register snapshot of YT921X_LED2_PORTn(port)…

--

net: dsa: motorcomm: Add LED support

This adds LED class device support to the Motorcomm yt921x DSA driver, with
up to three LEDs per port described by the standard device tree LEDs
structure, plus hardware offload of the netdev trigger. Only the hardware
default (parallel mode, 1:1 mapping) is handled, and the LEDs are torn down
from a new DSA teardown callback.

A few questions on the new code below.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 6dee25b6754a..d663af010f43 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c  

[ ... ]

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

[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?

> diff --git a/drivers/net/dsa/motorcomm/leds.c b/drivers/net/dsa/motorcomm/leds.c
> new file mode 100644
> index 000000000000..7d3c0f68d8ff
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/leds.c
> @@ -0,0 +1,641 @@  

[ ... ]

> +static int
> +yt921x_led_blink_select(const struct yt921x_priv *priv, unsigned long on,
> +			unsigned long off, unsigned short *cyclep,
> +			unsigned char *dutyp)
> +{
> +	static const unsigned char dutys[] = {
> +		YT921X_LED_DUTY(1, 6),
> +		YT921X_LED_DUTY(1, 4),
> +		YT921X_LED_DUTY(1, 3),
> +		YT921X_LED_DUTY(1, 2),
> +	};  

[ ... ]

> +	duty_req = DIV_ROUND_CLOSEST(YT921X_LED_DUTY_DENOM *
> +				     (on > off ? off : on), cycle_req);
> +	for (unsigned int i = ARRAY_SIZE(dutys) - 1;; i--)
> +		if (i == 0 || duty_req >= (dutys[i - 1] + dutys[i]) / 2) {
> +			duty = dutys[i];
> +			break;
> +		}  

[ ... ]

> +	if (!*onp && !*offp) {
> +		cycle = YT921X_LED_BLINK_DEF;
> +		duty = (pp->led_duty_mask & ~BIT(group)) ? pp->led_duty :
> +		       YT921X_LED_DUTY(1, 2);
> +
> +		use_cycle = false;
> +	} else {
> +		res = yt921x_led_blink_select(priv, *onp, *offp, &cycle, &duty);
> +		if (res)
> +			return res;  

[Severity: Medium]

What happens when exactly one of *delay_on / *delay_off is zero? The LED
core defines delay_on == 0 as steady off and delay_off == 0 as steady on,
but drivers/leds/led-core.c led_blink_setup() returns as soon as
blink_set() succeeds:

	if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
	    led_cdev->blink_set &&
	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))
		return;

so led_set_software_blink() and its zero-delay handling never run.

Only the both-zero case is special-cased here. For *onp == 0 and
*offp == 500, yt921x_led_blink_select() computes duty_req == 0, the search
bottoms out at dutys[0] (1/6), and yt921x_led_blink_set() then programs
YT921X_LED1_PORT_BLINK_DUTY_5_6 with _COMP plus
YT921X_LED2_PORT_FORCEn_BLINK(group) and returns 0.

Does the LED then blink at roughly 1/6 duty where a steady off was
requested (and roughly 5/6 for delay_off == 0)?

> +	*onp = DIV_ROUND_CLOSEST(duty * cycle, YT921X_LED_DUTY_DENOM);
> +	*offp = cycle - *onp;  

These two lines also replace the requested zero with a non-zero value, so
the timer trigger's delay_on / delay_off files report timings user space
never asked for. Would driving the steady states through
YT921X_LED2_PORT_FORCEn_ON / _OFF, or returning -EOPNOTSUPP for a single
zero delay, be preferable?

[ ... ]

> +static const struct yt921x_led_trigger_map yt921x_led_trigger_maps[] = {
> +	{BIT(TRIGGER_NETDEV_LINK),
> +	 YT921X_LEDx_PORT_ACT_DUPLEX_HALF | YT921X_LEDx_PORT_ACT_DUPLEX_FULL},  

[ ... ]

> +	{BIT(TRIGGER_NETDEV_HALF_DUPLEX), YT921X_LEDx_PORT_ACT_DUPLEX_HALF},
> +	{BIT(TRIGGER_NETDEV_FULL_DUPLEX), YT921X_LEDx_PORT_ACT_DUPLEX_FULL},  

[ ... ]

> +	*flagsp = 0;
> +	for (unsigned int i = 0; i < ARRAY_SIZE(yt921x_led_trigger_maps); i++) {
> +		const struct yt921x_led_trigger_map *map = &yt921x_led_trigger_maps[i];
> +
> +		if ((val & map->mask) == map->mask)
> +			*flagsp |= map->flags;
> +	}  

[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?

[ ... ]

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

[ ... ]

> +	led2_val = U32_MAX;
> +	res = yt921x_reg_read(priv, YT921X_LED2_PORTn(port), &led2_val);
> +	if (res)
> +		goto revoke_led2;  

[ ... ]

> +	mutex_unlock(&priv->reg_lock);  

[ ... ]

> +	res = devm_led_classdev_register_ext(dev, led_cdev, &init_data);
> +	if (res)
> +		goto revoke;
> +
> +	return 0;
> +
> +revoke:
> +	mutex_lock(&priv->reg_lock);
> +revoke_led2:
> +	if (led2_val != U32_MAX) {
> +		ret = yt921x_reg_write(priv, YT921X_LED2_PORTn(port), led2_val);  

[Severity: Low]

Can this rollback clobber the other LED groups of the same port?

led2_val is a snapshot of the whole YT921X_LED2_PORTn(port) word, taken
before mutex_unlock(&priv->reg_lock) and before
devm_led_classdev_register_ext() runs. That word holds the FORCE and
FORCE_BLINK fields of all three groups, and the groups set up in earlier
loop iterations are already registered class devices reachable from sysfs.

If devm_led_classdev_register_ext() fails (for example -ENOMEM from
devres_alloc()) while a concurrent brightness or delay_on write for a
sibling LED has gone through yt921x_cled_brightness_set_blocking() or
yt921x_cled_blink_set(), does the revoke path write the stale word back and
silently revert that sibling's force/blink state, leaving pp->led_duty,
pp->led_cycle and the masks out of sync with the hardware?

Would re-reading the register under reg_lock and using
yt921x_reg_update_bits() with
YT921X_LED2_PORT_FORCEn_M(group) | YT921X_LED2_PORT_FORCE_BLINKn_M(group)
be safer here?

[ ... ]

> +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?
-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831044818.4103477-1-mmyangfl%40gmail.com