On Mon, May 22, 2017 at 09:09:42PM +0100, Sean Young wrote: >On Mon, May 01, 2017 at 06:04:42PM +0200, David Härdeman wrote: >> Using the kernel ida facilities, we can avoid a lot of unnecessary code and at the same >> time get rid of lirc_dev_lock in favour of per-device locks (the irctls array was used >> throughout lirc_dev so this patch necessarily touches a lot of code). >> >> Signed-off-by: David Härdeman <david@xxxxxxxxxxx> >> --- >> drivers/media/rc/ir-lirc-codec.c | 9 - >> drivers/media/rc/lirc_dev.c | 258 ++++++++++++------------------- >> drivers/staging/media/lirc/lirc_zilog.c | 16 +- >> include/media/lirc_dev.h | 14 -- >> 4 files changed, 115 insertions(+), 182 deletions(-) >> >> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c >> index a30af91710fe..2c1221a61ea1 100644 >> --- a/drivers/media/rc/ir-lirc-codec.c >> +++ b/drivers/media/rc/ir-lirc-codec.c >> @@ -382,7 +382,6 @@ static int ir_lirc_register(struct rc_dev *dev) >> >> snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)", >> dev->driver_name); >> - drv->minor = -1; >> drv->features = features; >> drv->data = &dev->raw->lirc; >> drv->rbuf = NULL; >> @@ -394,11 +393,9 @@ static int ir_lirc_register(struct rc_dev *dev) >> drv->rdev = dev; >> drv->owner = THIS_MODULE; >> >> - drv->minor = lirc_register_driver(drv); >> - if (drv->minor < 0) { >> - rc = -ENODEV; >> + rc = lirc_register_driver(drv); >> + if (rc < 0) >> goto out; >> - } >> >> dev->raw->lirc.drv = drv; >> dev->raw->lirc.dev = dev; >> @@ -413,7 +410,7 @@ static int ir_lirc_unregister(struct rc_dev *dev) >> { >> struct lirc_codec *lirc = &dev->raw->lirc; >> >> - lirc_unregister_driver(lirc->drv->minor); >> + lirc_unregister_driver(lirc->drv); >> kfree(lirc->drv); >> lirc->drv = NULL; >> >> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c >> index e44e0b23b883..7db7d4c57991 100644 >> --- a/drivers/media/rc/lirc_dev.c >> +++ b/drivers/media/rc/lirc_dev.c >> @@ -31,23 +31,35 @@ >> #include <linux/bitops.h> >> #include <linux/device.h> >> #include <linux/cdev.h> >> +#include <linux/idr.h> >> >> #include <media/rc-core.h> >> #include <media/lirc.h> >> #include <media/lirc_dev.h> >> >> #define IRCTL_DEV_NAME "BaseRemoteCtl" >> -#define NOPLUG -1 >> #define LOGHEAD "lirc_dev (%s[%d]): " >> >> static dev_t lirc_base_dev; >> >> +/** >> + * struct irctl - lirc per-device structure >> + * @d: internal copy of the &struct lirc_driver for the device >> + * @dead: if the device has gone away >> + * @users: the number of users of the lirc chardev (currently limited to 1) >> + * @mutex: synchronizes open()/close()/ioctl()/etc calls >> + * @buf: used to store lirc RX data >> + * @buf_internal: whether @buf was allocated internally or not >> + * @chunk_size: @buf stores and read() returns data chunks of this size >> + * @dev: the &struct device for the lirc device >> + * @cdev: the &struct device for the lirc chardev >> + */ >> struct irctl { >> struct lirc_driver d; >> - int attached; >> - int open; >> + bool dead; >> + unsigned users; >> >> - struct mutex irctl_lock; >> + struct mutex mutex; >> struct lirc_buffer *buf; >> bool buf_internal; >> unsigned int chunk_size; >> @@ -56,9 +68,9 @@ struct irctl { >> struct cdev cdev; >> }; >> >> -static DEFINE_MUTEX(lirc_dev_lock); >> - >> -static struct irctl *irctls[MAX_IRCTL_DEVICES]; >> +/* Used to keep track of allocated lirc devices */ >> +#define LIRC_MAX_DEVICES 256 >> +static DEFINE_IDA(lirc_ida); >> >> /* Only used for sysfs but defined to void otherwise */ >> static struct class *lirc_class; >> @@ -72,9 +84,6 @@ static void lirc_release(struct device *ld) >> kfree(ir->buf); >> } >> >> - mutex_lock(&lirc_dev_lock); >> - irctls[ir->d.minor] = NULL; >> - mutex_unlock(&lirc_dev_lock); >> kfree(ir); >> } >> >> @@ -117,7 +126,18 @@ static int lirc_allocate_buffer(struct irctl *ir) >> return err; >> } >> >> -int lirc_register_driver(struct lirc_driver *d) >> +/** >> + * lirc_register_driver() - create a new lirc device by registering a driver >> + * @d: the &struct lirc_driver to register >> + * >> + * This function allocates and registers a lirc device for a given >> + * &lirc_driver. The &lirc_driver structure is updated to contain >> + * information about the allocated device (minor, buffer, etc). >> + * >> + * Return: zero on success or a negative error value. >> + */ >> +int >> +lirc_register_driver(struct lirc_driver *d) >> { >> struct irctl *ir; >> int minor; >> @@ -138,12 +158,6 @@ int lirc_register_driver(struct lirc_driver *d) >> return -EINVAL; >> } >> >> - if (d->minor >= MAX_IRCTL_DEVICES) { >> - dev_err(d->dev, "minor must be between 0 and %d!\n", >> - MAX_IRCTL_DEVICES - 1); >> - return -EBADRQC; >> - } >> - >> if (d->code_length < 1 || d->code_length > (BUFLEN * 8)) { >> dev_err(d->dev, "code length must be less than %d bits\n", >> BUFLEN * 8); >> @@ -156,49 +170,30 @@ int lirc_register_driver(struct lirc_driver *d) >> return -EBADRQC; >> } >> >> - mutex_lock(&lirc_dev_lock); >> - >> - minor = d->minor; >> + d->name[sizeof(d->name)-1] = '\0'; >> + if (d->features == 0) >> + d->features = LIRC_CAN_REC_LIRCCODE; >> >> - if (minor < 0) { >> - /* find first free slot for driver */ >> - for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++) >> - if (!irctls[minor]) >> - break; >> - if (minor == MAX_IRCTL_DEVICES) { >> - dev_err(d->dev, "no free slots for drivers!\n"); >> - err = -ENOMEM; >> - goto out_lock; >> - } >> - } else if (irctls[minor]) { >> - dev_err(d->dev, "minor (%d) just registered!\n", minor); >> - err = -EBUSY; >> - goto out_lock; >> - } >> + minor = ida_simple_get(&lirc_ida, 0, LIRC_MAX_DEVICES, GFP_KERNEL); >> + if (minor < 0) >> + return minor; >> + d->minor = minor; >> >> ir = kzalloc(sizeof(struct irctl), GFP_KERNEL); >> if (!ir) { >> err = -ENOMEM; >> - goto out_lock; >> + goto out_minor; >> } >> >> - mutex_init(&ir->irctl_lock); >> - irctls[minor] = ir; >> - d->minor = minor; >> - >> - /* some safety check 8-) */ >> - d->name[sizeof(d->name)-1] = '\0'; >> - >> - if (d->features == 0) >> - d->features = LIRC_CAN_REC_LIRCCODE; >> + mutex_init(&ir->mutex); >> >> ir->d = *d; > >Here we copy lirc_driver. > >> >> if (LIRC_CAN_REC(d->features)) { >> - err = lirc_allocate_buffer(irctls[minor]); >> + err = lirc_allocate_buffer(ir); >> if (err) { >> kfree(ir); >> - goto out_lock; >> + goto out_minor; >> } >> d->rbuf = ir->buf; >> } >> @@ -218,107 +213,82 @@ int lirc_register_driver(struct lirc_driver *d) >> if (err) >> goto out_free_dev; >> >> - ir->attached = 1; >> - >> err = device_add(&ir->dev); >> if (err) >> goto out_cdev; >> >> - mutex_unlock(&lirc_dev_lock); >> - >> dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n", >> ir->d.name, ir->d.minor); >> >> - return minor; >> + d->lirc_internal = ir; > >And now a store a pointer to the copy in the original lirc_driver. > >It would be much better to not copy lirc_driver and the lirc_internal >member would be unnecessary. I know, but this is a more minimal fix. The struct copy is already in the current code and I'd prefer removing it in a separate patch once the use of lirc_driver has been vetted. >> + return 0; >> >> out_cdev: >> cdev_del(&ir->cdev); >> out_free_dev: >> put_device(&ir->dev); >> -out_lock: >> - mutex_unlock(&lirc_dev_lock); >> +out_minor: >> + ida_simple_remove(&lirc_ida, minor); >> >> return err; >> } >> EXPORT_SYMBOL(lirc_register_driver); >> >> -int lirc_unregister_driver(int minor) >> +/** >> + * lirc_unregister_driver() - remove the lirc device for a given driver >> + * @d: the &struct lirc_driver to unregister >> + * >> + * This function unregisters the lirc device for a given &lirc_driver. >> + */ >> +void >> +lirc_unregister_driver(struct lirc_driver *d) >> { >> struct irctl *ir; >> >> - if (minor < 0 || minor >= MAX_IRCTL_DEVICES) { >> - pr_err("minor (%d) must be between 0 and %d!\n", >> - minor, MAX_IRCTL_DEVICES - 1); >> - return -EBADRQC; >> - } >> + if (!d->lirc_internal) >> + return; >> >> - ir = irctls[minor]; >> - if (!ir) { >> - pr_err("failed to get irctl\n"); >> - return -ENOENT; >> - } >> + ir = d->lirc_internal; > >This is done to find a our copy again. > >> - mutex_lock(&lirc_dev_lock); >> - >> - if (ir->d.minor != minor) { >> - dev_err(ir->d.dev, "lirc_dev: minor %d device not registered\n", >> - minor); >> - mutex_unlock(&lirc_dev_lock); >> - return -ENOENT; >> - } >> + mutex_lock(&ir->mutex); >> >> dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n", >> ir->d.name, ir->d.minor); >> >> - ir->attached = 0; >> - if (ir->open) { >> + ir->dead = true; >> + if (ir->users) { >> dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n", >> ir->d.name, ir->d.minor); >> wake_up_interruptible(&ir->buf->wait_poll); >> } >> >> - mutex_unlock(&lirc_dev_lock); >> + mutex_unlock(&ir->mutex); >> >> device_del(&ir->dev); >> cdev_del(&ir->cdev); >> - put_device(&ir->dev); >> - >> - return 0; >> + ida_simple_remove(&lirc_ida, d->minor); >> + d->minor = -1; >> + d->lirc_internal = NULL; >> } >> EXPORT_SYMBOL(lirc_unregister_driver); >> >> int lirc_dev_fop_open(struct inode *inode, struct file *file) >> { >> - struct irctl *ir; >> + struct irctl *ir = container_of(inode->i_cdev, struct irctl, cdev); >> int retval; >> >> - if (iminor(inode) >= MAX_IRCTL_DEVICES) { >> - pr_err("open result for %d is -ENODEV\n", iminor(inode)); >> - return -ENODEV; >> - } >> - >> - if (mutex_lock_interruptible(&lirc_dev_lock)) >> - return -ERESTARTSYS; >> + mutex_lock(&ir->mutex); >> >> - ir = irctls[iminor(inode)]; >> - mutex_unlock(&lirc_dev_lock); >> - >> - if (!ir) { >> - retval = -ENODEV; >> + if (ir->users) { >> + retval = -EBUSY; >> + mutex_unlock(&ir->mutex); >> goto error; >> } >> + ir->users++; >> >> - dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor); >> - >> - if (ir->d.minor == NOPLUG) { >> - retval = -ENODEV; >> - goto error; >> - } >> + mutex_unlock(&ir->mutex); >> >> - if (ir->open) { >> - retval = -EBUSY; >> - goto error; >> - } >> + dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor); >> >> if (ir->d.rdev) { >> retval = rc_open(ir->d.rdev); >> @@ -329,8 +299,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file) >> if (ir->buf) >> lirc_buffer_clear(ir->buf); >> >> - ir->open++; >> - >> + file->private_data = ir; >> nonseekable_open(inode, file); >> >> return 0; >> @@ -342,22 +311,14 @@ EXPORT_SYMBOL(lirc_dev_fop_open); >> >> int lirc_dev_fop_close(struct inode *inode, struct file *file) >> { >> - struct irctl *ir = irctls[iminor(inode)]; >> - int ret; >> - >> - if (!ir) { >> - pr_err("called with invalid irctl\n"); >> - return -EINVAL; >> - } >> + struct irctl *ir = file->private_data; >> >> - ret = mutex_lock_killable(&lirc_dev_lock); >> - WARN_ON(ret); >> + mutex_lock(&ir->mutex); >> >> rc_close(ir->d.rdev); >> + ir->users--; >> >> - ir->open--; >> - if (!ret) >> - mutex_unlock(&lirc_dev_lock); >> + mutex_unlock(&ir->mutex); >> >> return 0; >> } >> @@ -365,26 +326,21 @@ EXPORT_SYMBOL(lirc_dev_fop_close); >> >> unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait) >> { >> - struct irctl *ir = irctls[iminor(file_inode(file))]; >> + struct irctl *ir = file->private_data; >> unsigned int ret; >> >> - if (!ir) { >> - pr_err("called with invalid irctl\n"); >> - return POLLERR; >> - } >> - >> - if (!ir->attached) >> + if (ir->dead) >> return POLLHUP | POLLERR; >> >> - if (ir->buf) { >> - poll_wait(file, &ir->buf->wait_poll, wait); >> + if (!ir->buf) >> + return POLLERR; >> + >> + poll_wait(file, &ir->buf->wait_poll, wait); >> >> - if (lirc_buffer_empty(ir->buf)) >> - ret = 0; >> - else >> - ret = POLLIN | POLLRDNORM; >> - } else >> - ret = POLLERR; >> + if (lirc_buffer_empty(ir->buf)) >> + ret = 0; >> + else >> + ret = POLLIN | POLLRDNORM; >> >> dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n", >> ir->d.name, ir->d.minor, ret); >> @@ -395,25 +351,20 @@ EXPORT_SYMBOL(lirc_dev_fop_poll); >> >> long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg) >> { >> + struct irctl *ir = file->private_data; >> __u32 mode; >> int result = 0; >> - struct irctl *ir = irctls[iminor(file_inode(file))]; >> - >> - if (!ir) { >> - pr_err("no irctl found!\n"); >> - return -ENODEV; >> - } >> >> dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n", >> ir->d.name, ir->d.minor, cmd); >> >> - if (ir->d.minor == NOPLUG || !ir->attached) { >> + if (ir->dead) { >> dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n", >> ir->d.name, ir->d.minor); >> return -ENODEV; >> } >> >> - mutex_lock(&ir->irctl_lock); >> + mutex_lock(&ir->mutex); >> >> switch (cmd) { >> case LIRC_GET_FEATURES: >> @@ -468,7 +419,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg) >> result = -ENOTTY; >> } >> >> - mutex_unlock(&ir->irctl_lock); >> + mutex_unlock(&ir->mutex); >> >> return result; >> } >> @@ -479,16 +430,11 @@ ssize_t lirc_dev_fop_read(struct file *file, >> size_t length, >> loff_t *ppos) >> { >> - struct irctl *ir = irctls[iminor(file_inode(file))]; >> + struct irctl *ir = file->private_data; >> unsigned char *buf; >> int ret = 0, written = 0; >> DECLARE_WAITQUEUE(wait, current); >> >> - if (!ir) { >> - pr_err("called with invalid irctl\n"); >> - return -ENODEV; >> - } >> - >> if (!LIRC_CAN_REC(ir->d.features)) >> return -EINVAL; >> >> @@ -498,11 +444,12 @@ ssize_t lirc_dev_fop_read(struct file *file, >> if (!buf) >> return -ENOMEM; >> >> - if (mutex_lock_interruptible(&ir->irctl_lock)) { >> + if (mutex_lock_interruptible(&ir->mutex)) { >> ret = -ERESTARTSYS; >> goto out_unlocked; >> } >> - if (!ir->attached) { >> + >> + if (ir->dead) { >> ret = -ENODEV; >> goto out_locked; >> } >> @@ -541,18 +488,18 @@ ssize_t lirc_dev_fop_read(struct file *file, >> break; >> } >> >> - mutex_unlock(&ir->irctl_lock); >> + mutex_unlock(&ir->mutex); >> set_current_state(TASK_INTERRUPTIBLE); >> schedule(); >> set_current_state(TASK_RUNNING); >> >> - if (mutex_lock_interruptible(&ir->irctl_lock)) { >> + if (mutex_lock_interruptible(&ir->mutex)) { >> ret = -ERESTARTSYS; >> remove_wait_queue(&ir->buf->wait_poll, &wait); >> goto out_unlocked; >> } >> >> - if (!ir->attached) { >> + if (ir->dead) { >> ret = -ENODEV; >> goto out_locked; >> } >> @@ -570,7 +517,7 @@ ssize_t lirc_dev_fop_read(struct file *file, >> remove_wait_queue(&ir->buf->wait_poll, &wait); >> >> out_locked: >> - mutex_unlock(&ir->irctl_lock); >> + mutex_unlock(&ir->mutex); >> >> out_unlocked: >> kfree(buf); >> @@ -581,7 +528,8 @@ EXPORT_SYMBOL(lirc_dev_fop_read); >> >> void *lirc_get_pdata(struct file *file) >> { >> - return irctls[iminor(file_inode(file))]->d.data; >> + struct irctl *ir = file->private_data; >> + return ir->d.data; >> } >> EXPORT_SYMBOL(lirc_get_pdata); >> >> @@ -596,7 +544,7 @@ static int __init lirc_dev_init(void) >> return PTR_ERR(lirc_class); >> } >> >> - retval = alloc_chrdev_region(&lirc_base_dev, 0, MAX_IRCTL_DEVICES, >> + retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES, >> IRCTL_DEV_NAME); >> if (retval) { >> class_destroy(lirc_class); >> @@ -613,7 +561,7 @@ static int __init lirc_dev_init(void) >> static void __exit lirc_dev_exit(void) >> { >> class_destroy(lirc_class); >> - unregister_chrdev_region(lirc_base_dev, MAX_IRCTL_DEVICES); >> + unregister_chrdev_region(lirc_base_dev, LIRC_MAX_DEVICES); >> pr_info("module unloaded\n"); >> } >> >> diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c >> index 59e05aa1bc55..ffb70dee4547 100644 >> --- a/drivers/staging/media/lirc/lirc_zilog.c >> +++ b/drivers/staging/media/lirc/lirc_zilog.c >> @@ -183,11 +183,7 @@ static void release_ir_device(struct kref *ref) >> * ir->open_count == 0 - happens on final close() >> * ir_lock, tx_ref_lock, rx_ref_lock, all released >> */ >> - if (ir->l.minor >= 0) { >> - lirc_unregister_driver(ir->l.minor); >> - ir->l.minor = -1; >> - } >> - >> + lirc_unregister_driver(&ir->l); >> if (kfifo_initialized(&ir->rbuf.fifo)) >> lirc_buffer_free(&ir->rbuf); >> list_del(&ir->list); >> @@ -1382,7 +1378,6 @@ static const struct file_operations lirc_fops = { >> >> static struct lirc_driver lirc_template = { >> .name = "lirc_zilog", >> - .minor = -1, >> .code_length = 13, >> .buffer_size = BUFLEN / 2, >> .chunk_size = 2, >> @@ -1597,14 +1592,13 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id) >> } >> >> /* register with lirc */ >> - ir->l.minor = lirc_register_driver(&ir->l); >> - if (ir->l.minor < 0) { >> + ret = lirc_register_driver(&ir->l); >> + if (ret < 0) { >> dev_err(tx->ir->l.dev, >> - "%s: lirc_register_driver() failed: %i\n", >> - __func__, ir->l.minor); >> - ret = -EBADRQC; >> + "%s: lirc_register_driver failed: %i\n", __func__, ret); >> goto out_put_xx; >> } >> + >> dev_info(ir->l.dev, >> "IR unit on %s (i2c-%d) registered as lirc%d and ready\n", >> adap->name, adap->nr, ir->l.minor); >> diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h >> index 1f327e25a9be..f7629ff116a9 100644 >> --- a/include/media/lirc_dev.h >> +++ b/include/media/lirc_dev.h >> @@ -9,7 +9,6 @@ >> #ifndef _LINUX_LIRC_DEV_H >> #define _LINUX_LIRC_DEV_H >> >> -#define MAX_IRCTL_DEVICES 8 >> #define BUFLEN 16 >> >> #define mod(n, div) ((n) % (div)) >> @@ -164,6 +163,8 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf, >> * device. >> * >> * @owner: the module owning this struct >> + * >> + * @lirc_internal: lirc_dev bookkeeping data, don't touch. > >It's not a great name or comment :) > >Otherwise, it's a good idea. > > >Sean > >> */ >> struct lirc_driver { >> char name[40]; >> @@ -182,19 +183,12 @@ struct lirc_driver { >> const struct file_operations *fops; >> struct device *dev; >> struct module *owner; >> + void *lirc_internal; >> }; >> >> -/* following functions can be called ONLY from user context >> - * >> - * returns negative value on error or minor number >> - * of the registered device if success >> - * contents of the structure pointed by p is copied >> - */ >> extern int lirc_register_driver(struct lirc_driver *d); >> >> -/* returns negative value on error or 0 if success >> -*/ >> -extern int lirc_unregister_driver(int minor); >> +extern void lirc_unregister_driver(struct lirc_driver *d); >> >> /* Returns the private data stored in the lirc_driver >> * associated with the given device file pointer. -- David Härdeman