Re: [PATCH v2] net: dsa: rtl8366rb: Fix compilation problem

[Vladimir Oltean <olteanv@gmail.com>]

On Thu, Feb 20, 2025 at 05:02:21PM +0100, Linus Walleij wrote:
> diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
> index 35491dc20d6d6ed54855cbf62a0107b13b2a8fda..2fb362bbbc183584317b4bc7792ee638c40fa6a1 100644
> --- a/drivers/net/dsa/realtek/Makefile
> +++ b/drivers/net/dsa/realtek/Makefile
> @@ -12,4 +12,7 @@ endif
>  
>  obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
>  rtl8366-objs 				:= rtl8366-core.o rtl8366rb.o
> +ifdef CONFIG_LEDS_CLASS
> +rtl8366-objs 				+= rtl8366rb-leds.o
> +endif
>  obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o

I was expecting to see even more than this. What happens if CONFIG_LEDS_CLASS
is a module but CONFIG_NET_DSA_REALTEK_RTL8366RB is built-in? Built-in
code can't call symbols exported by modules, but I don't see that
restriction imposed, that CONFIG_NET_DSA_REALTEK_RTL8366RB should also
be a module.

> +	init_data.devicename = kasprintf(GFP_KERNEL, "Realtek-%d:0%d:%d",
> +					 dp->ds->index, dp->index, led_group);
> +	if (!init_data.devicename)
> +		return -ENOMEM;
> +
> +	ret = devm_led_classdev_register_ext(priv->dev, &led->cdev, &init_data);
> +	if (ret) {
> +		dev_warn(priv->dev, "Failed to init LED %d for port %d",
> +			 led_group, dp->index);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

I know this is just moving the code around, but I was looking at it anyway.

Doesn't init_data.devicename need to be kfree()d after devm_led_classdev_register_ext(),
regardless of whether it succeeds or fails?

IMHO, the code could use further simplification if "realtek,disable-leds" were
honored only with the LED subsystem enabled. I understand the property exists
prior to that, but it can be ignored if convenient. It seems reasonable to
leave LEDs as they are if CONFIG_LEDS_CLASS is disabled. But let me know
if you disagree, it's not a strong opinion.

Also, I'm not sure there's any reason to set RTL8366RB_LED_BLINKRATE_56MS from
within the common code instead of from rtl8366rb-leds.c, since the only
thing that the common code does is disable the LEDs anyway (so why would
the blink rate matter). But that's again unrelated to this specific change,
and something which can be handled later in net-next.