| Message ID | 20210909222513.2184795-11-arequipeno@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce block device LED trigger | expand |
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
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?
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, };
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(+)