Re: [PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors

[Sean Young]

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

[David Härdeman]

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

[Sean Young]

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

[David Härdeman]

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