lkml.org 
[lkml]   [2021]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 10/15] leds: trigger: blkdev: Add LED trigger activate function
    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

    \
     
     \ /
      Last update: 2021-09-10 08:48    [W:4.551 / U:0.204 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site