diff mbox series

[v2,10/15] leds: trigger: blkdev: Add LED trigger activate function

Message ID 20210909222513.2184795-11-arequipeno@gmail.com
State New
Headers show
Series Introduce block device LED trigger | expand

Commit Message

Ian Pilcher Sept. 9, 2021, 10:25 p.m. UTC
Allocate per-LED data structure and initialize with default values

Create /sys/class/leds/<led>/block_devices directory

Increment module reference count.  Module can only be removed when no LEDs
are associated with the trigger.

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 57 +++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

Comments

Greg KH Sept. 10, 2021, 6:47 a.m. UTC | #1
On Thu, Sep 09, 2021 at 05:25:08PM -0500, Ian Pilcher wrote:
> Allocate per-LED data structure and initialize with default values

> 

> Create /sys/class/leds/<led>/block_devices directory

> 

> Increment module reference count.  Module can only be removed when no LEDs

> are associated with the trigger.

> 

> Signed-off-by: Ian Pilcher <arequipeno@gmail.com>

> ---

>  drivers/leds/trigger/ledtrig-blkdev.c | 57 +++++++++++++++++++++++++++

>  1 file changed, 57 insertions(+)

> 

> diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c

> index 40dc55e5d4f3..6f78a9515976 100644

> --- a/drivers/leds/trigger/ledtrig-blkdev.c

> +++ b/drivers/leds/trigger/ledtrig-blkdev.c

> @@ -164,6 +164,62 @@ static void blkdev_process(struct work_struct *const work)

>  }

>  

>  

> +/*

> + *

> + *	Associate an LED with the blkdev trigger

> + *

> + */

> +

> +static int blkdev_activate(struct led_classdev *const led_dev)

> +{

> +	struct ledtrig_blkdev_led *led;

> +	int ret;

> +

> +	/* Don't allow module to be removed while any LEDs are linked */

> +	if (WARN_ON(!try_module_get(THIS_MODULE))) {


That pattern is racy and broken and never ever ever add it to the kernel
again please.  All existing in-kernel users of it are also wrong, we
have been removing them for decades now.

> +		ret = -ENODEV;		/* Shouldn't ever happen */

> +		goto exit_return;

> +	}

> +

> +	led = kmalloc(sizeof(*led), GFP_KERNEL);

> +	if (led == NULL) {

> +		ret = -ENOMEM;

> +		goto exit_put_module;

> +	}

> +

> +	led->led_dev = led_dev;

> +	led->blink_msec = LEDTRIG_BLKDEV_BLINK_MSEC;

> +	led->mode = LEDTRIG_BLKDEV_MODE_RW;

> +	INIT_HLIST_HEAD(&led->disks);

> +

> +	ret = mutex_lock_interruptible(&ledtrig_blkdev_mutex);

> +	if (ret != 0)

> +		goto exit_free;

> +

> +	led->dir = kobject_create_and_add("linked_devices",

> +					  &led_dev->dev->kobj);


You have created a "raw" kobject in the device tree now, which means
that userspace will not be notified of it and will have a "hole" in it's
knowledge.  Why not just create a named attribute group to this device
instead?

> +	if (led->dir == NULL) {

> +		ret = -ENOMEM;

> +		goto exit_unlock;

> +	}

> +

> +	hlist_add_head(&led->leds_node, &ledtrig_blkdev_leds);

> +	led_set_trigger_data(led_dev, led);

> +	ret = 0;

> +

> +exit_unlock:

> +	mutex_unlock(&ledtrig_blkdev_mutex);

> +exit_free:

> +	if (ret != 0)

> +		kfree(led);

> +exit_put_module:

> +	if (ret != 0)

> +		module_put(THIS_MODULE);


Again, racy and broken, please do not do this.

thanks,

greg k-h
Ian Pilcher Sept. 10, 2021, 4:10 p.m. UTC | #2
On 9/10/21 1:47 AM, Greg KH wrote:
>> +	/* Don't allow module to be removed while any LEDs are linked */

>> +	if (WARN_ON(!try_module_get(THIS_MODULE))) {

> 

> That pattern is racy and broken and never ever ever add it to the kernel

> again please.  All existing in-kernel users of it are also wrong, we

> have been removing them for decades now.


OK.  (I was misled by the instances that you haven't gotten to yet.)

> You have created a "raw" kobject in the device tree now, which means

> that userspace will not be notified of it and will have a "hole" in it's

> knowledge.  Why not just create a named attribute group to this device

> instead?


What would I pass as the first argument to sysfs_create_link() in that
case?

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================
diff mbox series

Patch

diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
index 40dc55e5d4f3..6f78a9515976 100644
--- a/drivers/leds/trigger/ledtrig-blkdev.c
+++ b/drivers/leds/trigger/ledtrig-blkdev.c
@@ -164,6 +164,62 @@  static void blkdev_process(struct work_struct *const work)
 }
 
 
+/*
+ *
+ *	Associate an LED with the blkdev trigger
+ *
+ */
+
+static int blkdev_activate(struct led_classdev *const led_dev)
+{
+	struct ledtrig_blkdev_led *led;
+	int ret;
+
+	/* Don't allow module to be removed while any LEDs are linked */
+	if (WARN_ON(!try_module_get(THIS_MODULE))) {
+		ret = -ENODEV;		/* Shouldn't ever happen */
+		goto exit_return;
+	}
+
+	led = kmalloc(sizeof(*led), GFP_KERNEL);
+	if (led == NULL) {
+		ret = -ENOMEM;
+		goto exit_put_module;
+	}
+
+	led->led_dev = led_dev;
+	led->blink_msec = LEDTRIG_BLKDEV_BLINK_MSEC;
+	led->mode = LEDTRIG_BLKDEV_MODE_RW;
+	INIT_HLIST_HEAD(&led->disks);
+
+	ret = mutex_lock_interruptible(&ledtrig_blkdev_mutex);
+	if (ret != 0)
+		goto exit_free;
+
+	led->dir = kobject_create_and_add("linked_devices",
+					  &led_dev->dev->kobj);
+	if (led->dir == NULL) {
+		ret = -ENOMEM;
+		goto exit_unlock;
+	}
+
+	hlist_add_head(&led->leds_node, &ledtrig_blkdev_leds);
+	led_set_trigger_data(led_dev, led);
+	ret = 0;
+
+exit_unlock:
+	mutex_unlock(&ledtrig_blkdev_mutex);
+exit_free:
+	if (ret != 0)
+		kfree(led);
+exit_put_module:
+	if (ret != 0)
+		module_put(THIS_MODULE);
+exit_return:
+	return ret;
+}
+
+
 /*
  *
  *	Initialization - register the trigger
@@ -185,5 +241,6 @@  static const struct attribute_group *ledtrig_blkdev_attr_groups[] = {
 
 struct led_trigger ledtrig_blkdev_trigger = {
 	.name		= "blkdev",
+	.activate	= blkdev_activate,
 	.groups		= ledtrig_blkdev_attr_groups,
 };