lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7175bcaf-f588-41ec-afe6-117eceffc28d@t-8ch.de>
Date: Mon, 13 Oct 2025 17:39:55 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Jonathan Brophy <professorjonny98@...il.com>
Cc: lee Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>, 
	Jonathan Brophy <professor_jonny@...mail.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Radoslav Tsvetkov <rtsvetkov@...dotech.eu>, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-leds@...r.kernel.org
Subject: Re: [PATCH v2 4/4] leds: Add Virtual Color LED Group driver

On 2025-10-14 01:09:48+1300, Jonathan Brophy wrote:
> From: Jonathan Brophy <professor_jonny@...mail.com>
> 
> Introduces a new driver that implements virtual LED groups,
> aggregating multiple monochromatic LEDs into virtual groups and providing
> priority-based control for concurrent state management.
> 
> Author: Radoslav Tsvetkov <rtsvetkov@...dotech.eu>
> Co Author: Jonathan Brophy <professor_jonny@...mail.com>
> Copyright (C) 2024 Jonathan Brophy <professor_jonny@...mail.com>
> 
> Co-developed-by: Radoslav Tsvetkov <rtsvetkov@...dotech.eu>
> Signed-off-by: Radoslav Tsvetkov <rtsvetkov@...dotech.eu>
> Tested-by: Jonathan Brophy <professor_jonny@...mail.com>
> Signed-off-by: Jonathan Brophy <professor_jonny@...mail.com>
> ---
>  drivers/leds/rgb/Kconfig                   |  17 +
>  drivers/leds/rgb/Makefile                  |   1 +
>  drivers/leds/rgb/leds-group-virtualcolor.c | 439 +++++++++++++++++++++
>  3 files changed, 457 insertions(+)
>  create mode 100644 drivers/leds/rgb/leds-group-virtualcolor.c
> 
(...)

> diff --git a/drivers/leds/rgb/leds-group-virtualcolor.c b/drivers/leds/rgb/leds-group-virtualcolor.c
> new file mode 100644
> index 000000000000..e11ad155d3b4
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-group-virtualcolor.c
> @@ -0,0 +1,439 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtual LED Group Driver with Priority Control
> + *
> + * Implements virtual LED groups by aggregating multiple
> + * monochromatic LEDs. Provides priority-based control for managing
> + * concurrent LED activation requests, ensuring only the highest-priority
> + * LED state is active at any given time.
> + *
> + * Code created by Radoslav Tsvetkov <rtsvetkov@...dotech.eu>
> + * Copyright (C) 2024 Jonathan Brophy <professor_jonny@...mail.com>
> + *
> + */
> +
> +#include <linux/gpio/consumer.h>

Looks unnecessary.

> +#include <linux/leds.h>

#include <linux/list.h>

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +struct virtual_led {
> +	struct led_classdev cdev;
> +	struct led_classdev **monochromatics;
> +	struct leds_virtualcolor *vc_data;
> +	int num_monochromatics;
> +	int priority;
> +	unsigned long blink_delay_on;
> +	unsigned long blink_delay_off;
> +	struct list_head list;
> +};
> +
> +struct leds_virtualcolor {
> +	struct virtual_led *vleds;
> +	int num_vleds;
> +	struct list_head active_leds;
> +	struct mutex lock; // Protects access to active LEDs
> +};
> +
> +static void virtual_set_monochromatic_brightness(struct virtual_led *vled,
> +						 enum led_brightness brightness)
> +{
> +	int i;
> +
> +	if (vled->blink_delay_on || vled->blink_delay_off) {
> +		unsigned long blink_mask = (BIT(LED_BLINK_SW) | BIT(LED_BLINK_ONESHOT) |
> +					    BIT(LED_SET_BLINK));
> +
> +		/*
> +		 * Make sure the LED is not already blinking.
> +		 * We don't want to call led_blink_set multiple times.
> +		 */
> +		if (!(vled->cdev.work_flags & blink_mask))

work_flags don't look they are meant to be used by drivers.

> +			led_blink_set(&vled->cdev, &vled->blink_delay_on, &vled->blink_delay_off);
> +
> +		/* Update the blink delays if they have changed */
> +		if (vled->blink_delay_on != vled->cdev.blink_delay_on ||
> +		    vled->blink_delay_off != vled->cdev.blink_delay_off) {
> +			vled->cdev.blink_delay_on = vled->blink_delay_on;
> +			vled->cdev.blink_delay_off = vled->blink_delay_off;
> +		}
> +	}
> +
> +	for (i = 0; i < vled->num_monochromatics; i++)
> +		led_set_brightness(vled->monochromatics[i], brightness);
> +}
> +
> +static ssize_t priority_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct virtual_led *vled = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", vled->priority);

sysfs_emit();

> +}
> +
> +static ssize_t priority_store(struct device *dev, struct device_attribute *attr, const char *buf,
> +			      size_t count)
> +{
> +	struct virtual_led *vled = dev_get_drvdata(dev);
> +	int new_priority;
> +	int ret;
> +
> +	ret = kstrtoint(buf, 10, &new_priority);
> +	if (ret < 0)
> +		return ret;
> +
> +	vled->priority = new_priority;

No locking?

> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(priority);
> +
> +static ssize_t blink_delay_on_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct virtual_led *vled = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%lu\n", vled->blink_delay_on);
> +}

Why does this have a custom blinking UAPI?
Shouldn't it be generic?

(...)

> +static int leds_virtualcolor_init_vled(struct device *dev, struct device_node *child,
> +				       struct virtual_led *vled, struct leds_virtualcolor *vc_data)
> +{
> +	struct led_init_data init_data = {};
> +	u32 blink_interval;
> +	u32 phandle_count;
> +	u32 max_brightness;
> +	int ret;
> +	int i;

INIT_LIST_HEAD(&vled->list);

> +
> +	ret = of_property_read_u32(child, "priority", &vled->priority);
> +	if (ret)
> +		vled->priority = 0;
> +
> +	ret = of_property_read_u32(child, "blink", &blink_interval);
> +	if (!ret) {
> +		vled->blink_delay_on = blink_interval;
> +		vled->blink_delay_off = blink_interval;
> +	}
> +
> +	phandle_count = of_property_count_elems_of_size(child, "leds", sizeof(u32));
> +	if (phandle_count <= 0) {
> +		dev_err(dev, "No monochromatic LEDs specified for virtual LED %s\n",
> +			vled->cdev.name);
> +		return -EINVAL;
> +	}
> +
> +	vled->num_monochromatics = phandle_count;
> +	vled->monochromatics = devm_kcalloc(dev, vled->num_monochromatics,
> +					    sizeof(*vled->monochromatics), GFP_KERNEL);
> +	if (!vled->monochromatics)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < vled->num_monochromatics; i++) {
> +		struct led_classdev *led_cdev;
> +
> +		led_cdev = devm_of_led_get_optional(dev, i);
> +		if (IS_ERR(led_cdev)) {
> +			/*
> +			 * If the LED is not available yet, defer the probe.
> +			 * The probe will be retried when it becomes available.
> +			 */
> +			if (PTR_ERR(led_cdev) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +
> +			ret = PTR_ERR(led_cdev);
> +			dev_err(dev, "Failed to get monochromatic LED for %s, error %d\n",
> +				vled->cdev.name, ret);
> +			return ret;
> +		}

Just use dev_err_probe(), it will properly handle -EPROBE_DEFER.

> +
> +		vled->monochromatics[i] = led_cdev;
> +	}
> +
> +	ret = of_property_read_u32(child, "max-brightness", &max_brightness);
> +	if (ret)
> +		vled->cdev.max_brightness = LED_FULL;
> +	else
> +		vled->cdev.max_brightness = max_brightness;
> +
> +	vled->cdev.brightness_set_blocking = virtual_led_brightness_set;
> +	vled->cdev.flags = LED_CORE_SUSPENDRESUME;
> +
> +	init_data.fwnode = NULL; // Use OF, not fwnode

Why?

> +	ret = devm_led_classdev_register_ext(dev, &vled->cdev, &init_data);
> +	if (ret) {
> +		dev_err(dev, "Failed to register virtual LED %s\n", vled->cdev.name);
> +		return ret;
> +	}

if (ret)
	return dev_err_probe() ...

> +
> +	ret = device_create_file(vled->cdev.dev, &dev_attr_priority);
> +	if (ret) {
> +		dev_err(dev, "Failed to create sysfs attribute for priority\n");
> +		return ret;
> +	}

Use 'struct platform_driver::driver::dev_groups' to let the driver core
manage your sysfs attributes instead of doing it manually.

> +
> +	ret = device_create_file(vled->cdev.dev, &dev_attr_blink_delay_on);
> +	if (ret) {
> +		dev_err(dev, "Failed to create sysfs attribute for blink_delay_on\n");
> +		return ret;
> +	}
> +
> +	ret = device_create_file(vled->cdev.dev, &dev_attr_blink_delay_off);
> +	if (ret) {
> +		dev_err(dev, "Failed to create sysfs attribute for blink_delay_off\n");
> +		return ret;
> +	}
> +
> +	vled->vc_data = vc_data;
> +
> +	return 0;
> +}
> +
> +static int leds_virtualcolor_disable_sysfs_access(struct device *dev, struct virtual_led *vled)
> +{
> +	int i;
> +
> +	for (i = 0; i < vled->num_monochromatics; i++) {
> +		struct led_classdev *led_cdev = vled->monochromatics[i];
> +
> +		mutex_lock(&led_cdev->led_access);

This mutex looks unnecessary.

> +		led_sysfs_disable(led_cdev);
> +		mutex_unlock(&led_cdev->led_access);
> +
> +		devm_add_action_or_reset(dev, restore_sysfs_write_access, led_cdev);

Check for errors.

> +	}
> +
> +	return 0;
> +}
> +
> +static int leds_virtualcolor_probe(struct platform_device *pdev)
> +{
> +	struct leds_virtualcolor *vc_data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *child;
> +	int count = 0;
> +	int ret;
> +
> +	vc_data = devm_kzalloc(dev, sizeof(*vc_data), GFP_KERNEL);
> +	if (!vc_data)
> +		return -ENOMEM;
> +
> +	mutex_init(&vc_data->lock);

Use devm_mutex_init(), then you can get rid of all the mutex_destroy()
calls below.

> +	INIT_LIST_HEAD(&vc_data->active_leds);
> +
> +	vc_data->num_vleds = of_get_child_count(dev->of_node);
> +	if (vc_data->num_vleds == 0) {
> +		dev_err(dev, "No virtual LEDs defined in device tree\n");

return dev_err_probe();

> +		ret = -EINVAL;
> +		goto err_mutex_destroy;
> +	}
> +
> +	vc_data->vleds = devm_kcalloc(dev, vc_data->num_vleds, sizeof(*vc_data->vleds), GFP_KERNEL);
> +	if (!vc_data->vleds) {
> +		ret = -ENOMEM;
> +		goto err_mutex_destroy;
> +	}
> +
> +	for_each_child_of_node(dev->of_node, child) {

for_each_child_of_node_scoped() should be nicer.
Also I think you should check for available, or better yet use
for_each_available_child_of_node_scoped().

> +		struct virtual_led *vled = &vc_data->vleds[count];
> +
> +		ret = leds_virtualcolor_init_vled(dev, child, vled, vc_data);
> +		if (ret) {
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to initialize virtual LED %d\n", count);
> +
> +			of_node_put(child);
> +			goto err_node_put;
> +		}
> +
> +		count++;
> +	}
> +
> +	platform_set_drvdata(pdev, vc_data);
> +
> +	if (of_property_read_bool(dev->of_node, "monochromatics-ro")) {

The property should be documented.

> +		int i;
> +
> +		for (i = 0; i < count; i++) {
> +			struct virtual_led *vled = &vc_data->vleds[i];
> +
> +			ret = leds_virtualcolor_disable_sysfs_access(dev, vled);
> +			if (ret)
> +				goto err_node_put;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_node_put:
> +	mutex_destroy(&vc_data->lock);
> +	return ret;
> +
> +err_mutex_destroy:

Both labels have the same code behind them.

> +	mutex_destroy(&vc_data->lock);
> +
> +	return ret;
> +}
> +
> +static void leds_virtualcolor_remove(struct platform_device *pdev)
> +{
> +	struct leds_virtualcolor *vc_data = platform_get_drvdata(pdev);
> +	int i, j;
> +
> +	for (i = 0; i < vc_data->num_vleds; i++) {
> +		struct virtual_led *vled = &vc_data->vleds[i];
> +
> +		device_remove_file(vled->cdev.dev, &dev_attr_priority);
> +		device_remove_file(vled->cdev.dev, &dev_attr_blink_delay_on);
> +		device_remove_file(vled->cdev.dev, &dev_attr_blink_delay_off);

No need to clean up sysfs files, they will be cleaned up automatically.

> +
> +		for (j = 0; j < vled->num_monochromatics; j++) {
> +			if (vled->monochromatics[j]) {
> +				led_put(vled->monochromatics[j]);

This is dropping the reference acquired by devm_of_led_get_optional(),
correct? Then it is unnecessary, as the reference will be dropped
automatically by the devres framework.

> +				vled->monochromatics[j] = NULL;

This looks unnecessary, the memory is going to be freed anyways.

> +			}
> +		}
> +	}
> +
> +	mutex_destroy(&vc_data->lock);
> +}

(...)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ