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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing mutex unlock on error path causes permanent deadlock
- [Low] Build failure due to dynamic array index in `container_of_const`
--

commit 6e291330945ad21e3fadada5285deeff7e1563e6
Author: David Yang <mmyangfl@gmail.com>
Subject: net: dsa: motorcomm: Add LED support

This patch adds LED support for the Motorcomm YT9215 ethernet switch chips.
It introduces standard LED class devices for each port, supporting parallel
mode and mapping to the hardware default state.

> diff --git a/drivers/net/dsa/motorcomm/leds.c b/drivers/net/dsa/motorcomm/leds.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/leds.c
> @@ -0,0 +1,666 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2026 David Yang
> + */
> +
> +#include <linux/uleds.h>
> +
> +#include "chip.h"
> +#include "leds.h"
> +#include "smi.h"
> +
> +#define to_yt921x_led(led_cdev) \
> +	container_of_const((led_cdev), struct yt921x_led, cdev)
> +#define to_yt921x_port(led) ((led)->port)
> +#define to_yt921x_priv(pp) \
> +	container_of_const((pp), struct yt921x_priv, ports[(pp)->index])

[Severity: Low]
Does this macro definition cause a build failure? `container_of_const` uses
`offsetof()`, which requires array indices to be integer constant expressions.
Since `(pp)->index` is evaluated at runtime, this might result in a
"non-constant integer expression in offsetof" compiler error when compiling
with CONFIG_NET_DSA_YT921X_LEDS enabled.

[ ... ]

> +static int
> +yt921x_led_setup(struct yt921x_priv *priv, int port,
> +		 struct fwnode_handle *fwnode)
> +{
[ ... ]
> +	state = led_init_default_state_get(fwnode);
> +
> +	mutex_lock(&priv->reg_lock);
> +
> +	/* Inversion is internal - force on will give low logic.
> +	 * In the rest of the file, treat LEDs as if active-low.
> +	 */
> +	polarity_orig = 0;
> +	if (polarity) {
> +		res = yt921x_reg_read(priv, YT921X_LED_PAR_INV, &val);
> +		if (res)
> +			goto err_inv;
> +
> +		mask = YT921X_LED_PAR_INV_INVnm(group, port);
> +		ctrl = polarity > 0 ? val | mask : val & ~mask;
> +		if (ctrl != val) {
> +			res = yt921x_reg_write(priv, YT921X_LED_PAR_INV, ctrl);
> +			if (res)
> +				goto err_inv;
> +
> +			polarity_orig = -polarity;
> +		}
> +	}

[ ... ]

> +err_led2:
> +	if (polarity_orig) {
> +		ret = yt921x_reg_toggle_bits(priv, YT921X_LED_PAR_INV,
> +					     YT921X_LED_PAR_INV_INVnm(group, port),
> +					     polarity_orig > 0);
> +		if (ret)
> +			dev_err(dev,
> +				"Failed to restore %s for LED %02d:%02u: %d\n",
> +				"LED_PAR_INV", port, group, ret);
> +	}
> +	mutex_unlock(&priv->reg_lock);
> +
> +err_inv:
> +	pp->leds[group] = NULL;
> +	devm_kfree(dev, led);

[Severity: High]
Can jumping to `err_inv` cause a permanent deadlock? The lock
`priv->reg_lock` is acquired earlier in `yt921x_led_setup()`. If the read
or write to `YT921X_LED_PAR_INV` fails, the code jumps to the `err_inv`
label, which is located after the `mutex_unlock(&priv->reg_lock)` call.
This bypasses the unlock entirely and leaves the lock permanently acquired,
potentially hanging the device's control path on subsequent operations.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903031630.356840-1-mmyangfl@gmail.com?part=4