Re: [PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors
On Sun, May 28, 2017 at 10:26:59AM +0200, David Härdeman wrote: > 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> >> --- > >> 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 = >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 = >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 > >> #include > >> #include > >> +#include > >> > >> #include > >> #include > >> #include > >> > >> #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 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 device for the lirc device > >> + * @cdev: the 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(_dev_lock); > >> - irctls[ir->d.minor] = NULL; > >> - mutex_unlock(_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 lirc_driver to register > >> + * > >> + * This function allocates and registers a lirc device for a given > >> + * _driver. The _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
Re: [PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors
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>> --- >> 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 = >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 = >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 >> #include >> #include >> +#include >> >> #include >> #include >> #include >> >> #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 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 device for the lirc device >> + * @cdev: the 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(_dev_lock); >> -irctls[ir->d.minor] = NULL; >> -mutex_unlock(_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 lirc_driver to register >> + * >> + * This function allocates and registers a lirc device for a given >> + * _driver. The _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", >> -
Re: [PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors
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> --- > 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 = >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 = >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 > #include > #include > +#include > > #include > #include > #include > > #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 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 device for the lirc device > + * @cdev:the 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(_dev_lock); > - irctls[ir->d.minor] = NULL; > - mutex_unlock(_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 lirc_driver to register > + * > + * This function allocates and registers a lirc device for a given > + * _driver. The _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 ||
[PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors
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--- 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 = >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 = >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 #include #include +#include #include #include #include #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 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 device for the lirc device + * @cdev: the 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(_dev_lock); - irctls[ir->d.minor] = NULL; - mutex_unlock(_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 lirc_driver to register + * + * This function allocates and registers a lirc device for a given + * _driver. The _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