[PATCH 00/16] lirc_dev spring cleaning

[PATCH 00/16] lirc_dev spring cleaning

[David Härdeman]

lirc_dev has lots of functionality which is unused and the code isn't exactly
up-to-date with current kernel practices. This patchset removes the unused bits
and also simplifies the locking by moving lirc_dev over to only use
per-device mutexes rather than a big lirc lock in addition to per-device locks.

I think this is about as much as can be done right now before lirc_zilog is
either removed or ported to rc-core.

---

David Härdeman (16):
      lirc_dev: remove pointless functions
      lirc_dev: remove unused set_use_inc/set_use_dec
      lirc_dev: correct error handling
      lirc_dev: remove sampling kthread
      lirc_dev: clarify error handling
      lirc_dev: make fops mandatory
      lirc_dev: merge lirc_register_driver() and lirc_allocate_driver()
      lirc_zilog: remove module parameter minor
      lirc_dev: remove lirc_irctl_init() and lirc_cdev_add()
      lirc_dev: remove superfluous get/put_device() calls
      lirc_dev: remove unused module parameter
      lirc_dev: return POLLHUP and POLLERR when device is gone
      lirc_dev: use an ida instead of a hand-rolled array to keep track of minors
      lirc_dev: cleanup includes
      lirc_dev: remove name from struct lirc_driver
      lirc_dev: cleanup header


 drivers/media/rc/ir-lirc-codec.c        |   23 -
 drivers/media/rc/lirc_dev.c             |  516 ++++++++-----------------------
 drivers/staging/media/lirc/lirc_zilog.c |   33 --
 include/media/lirc_dev.h                |   53 ---
 4 files changed, 149 insertions(+), 476 deletions(-)

--
David Härdeman

[PATCH 01/16] lirc_dev: remove pointless functions

[David Härdeman]

drv->set_use_inc and drv->set_use_dec are already optional so we can remove all
dummy functions.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/ir-lirc-codec.c        |   14 ++------------
 drivers/staging/media/lirc/lirc_zilog.c |   11 -----------
 2 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 8f0669c9894c..fc58745b26b8 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -327,16 +327,6 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 	return ret;
 }
 
-static int ir_lirc_open(void *data)
-{
-	return 0;
-}
-
-static void ir_lirc_close(void *data)
-{
-	return;
-}
-
 static const struct file_operations lirc_fops = {
 	.owner		= THIS_MODULE,
 	.write		= ir_lirc_transmit_ir,
@@ -396,8 +386,8 @@ static int ir_lirc_register(struct rc_dev *dev)
 	drv->features = features;
 	drv->data = &dev->raw->lirc;
 	drv->rbuf = NULL;
-	drv->set_use_inc = &ir_lirc_open;
-	drv->set_use_dec = &ir_lirc_close;
+	drv->set_use_inc = NULL;
+	drv->set_use_dec = NULL;
 	drv->code_length = sizeof(struct ir_raw_event) * 8;
 	drv->chunk_size = sizeof(int);
 	drv->buffer_size = LIRCBUF_SIZE;
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index e4a533b6beb3..436cf1b6a70a 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -497,15 +497,6 @@ static int lirc_thread(void *arg)
 	return 0;
 }
 
-static int set_use_inc(void *data)
-{
-	return 0;
-}
-
-static void set_use_dec(void *data)
-{
-}
-
 /* safe read of a uint32 (always network byte order) */
 static int read_uint32(unsigned char **data,
 				     unsigned char *endp, unsigned int *val)
@@ -1396,8 +1387,6 @@ static struct lirc_driver lirc_template = {
 	.buffer_size	= BUFLEN / 2,
 	.sample_rate	= 0, /* tell lirc_dev to not start its own kthread */
 	.chunk_size	= 2,
-	.set_use_inc	= set_use_inc,
-	.set_use_dec	= set_use_dec,
 	.fops		= &lirc_fops,
 	.owner		= THIS_MODULE,
 };

[PATCH 02/16] lirc_dev: remove unused set_use_inc/set_use_dec

[David Härdeman]

Since there are no users of this functionality, it can be removed altogether.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/ir-lirc-codec.c |    2 --
 drivers/media/rc/lirc_dev.c      |   24 ++++++------------------
 include/media/lirc_dev.h         |    6 ------
 3 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index fc58745b26b8..a30af91710fe 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -386,8 +386,6 @@ static int ir_lirc_register(struct rc_dev *dev)
 	drv->features = features;
 	drv->data = &dev->raw->lirc;
 	drv->rbuf = NULL;
-	drv->set_use_inc = NULL;
-	drv->set_use_dec = NULL;
 	drv->code_length = sizeof(struct ir_raw_event) * 8;
 	drv->chunk_size = sizeof(int);
 	drv->buffer_size = LIRCBUF_SIZE;
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 42704552b005..05f600bd6c67 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -418,12 +418,6 @@ int lirc_unregister_driver(int minor)
 		wake_up_interruptible(&ir->buf->wait_poll);
 	}
 
-	mutex_lock(&ir->irctl_lock);
-
-	if (ir->d.set_use_dec)
-		ir->d.set_use_dec(ir->d.data);
-
-	mutex_unlock(&ir->irctl_lock);
 	mutex_unlock(&lirc_dev_lock);
 
 	device_del(&ir->dev);
@@ -473,17 +467,13 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
 			goto error;
 	}
 
+	if (ir->buf)
+		lirc_buffer_clear(ir->buf);
+
+	if (ir->task)
+		wake_up_process(ir->task);
+
 	ir->open++;
-	if (ir->d.set_use_inc)
-		retval = ir->d.set_use_inc(ir->d.data);
-	if (retval) {
-		ir->open--;
-	} else {
-		if (ir->buf)
-			lirc_buffer_clear(ir->buf);
-		if (ir->task)
-			wake_up_process(ir->task);
-	}
 
 error:
 	nonseekable_open(inode, file);
@@ -508,8 +498,6 @@ int lirc_dev_fop_close(struct inode *inode, struct file *file)
 	rc_close(ir->d.rdev);
 
 	ir->open--;
-	if (ir->d.set_use_dec)
-		ir->d.set_use_dec(ir->d.data);
 	if (!ret)
 		mutex_unlock(&lirc_dev_lock);
 
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index cec7d35602d1..71c1c11950fe 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -165,10 +165,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
  *			have to write to the buffer by other means, like irq's
  *			(see also lirc_serial.c).
  *
- * @set_use_inc:	set_use_inc will be called after device is opened
- *
- * @set_use_dec:	set_use_dec will be called after device is closed
- *
  * @rdev:		Pointed to struct rc_dev associated with the LIRC
  *			device.
  *
@@ -198,8 +194,6 @@ struct lirc_driver {
 	int max_timeout;
 	int (*add_to_buf)(void *data, struct lirc_buffer *buf);
 	struct lirc_buffer *rbuf;
-	int (*set_use_inc)(void *data);
-	void (*set_use_dec)(void *data);
 	struct rc_dev *rdev;
 	const struct file_operations *fops;
 	struct device *dev;

[PATCH 03/16] lirc_dev: correct error handling

[David Härdeman]

If an error is generated, nonseekable_open() shouldn't be called.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 05f600bd6c67..7f13ed479e1c 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -431,7 +431,7 @@ EXPORT_SYMBOL(lirc_unregister_driver);
 int lirc_dev_fop_open(struct inode *inode, struct file *file)
 {
 	struct irctl *ir;
-	int retval = 0;
+	int retval;
 
 	if (iminor(inode) >= MAX_IRCTL_DEVICES) {
 		pr_err("open result for %d is -ENODEV\n", iminor(inode));
@@ -475,9 +475,11 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
 
 	ir->open++;
 
-error:
 	nonseekable_open(inode, file);
 
+	return 0;
+
+error:
 	return retval;
 }
 EXPORT_SYMBOL(lirc_dev_fop_open);

[PATCH 04/16] lirc_dev: remove sampling kthread

[David Härdeman]

There are no drivers which use this functionality.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c             |   94 +------------------------------
 drivers/staging/media/lirc/lirc_zilog.c |    1 
 include/media/lirc_dev.h                |   16 -----
 3 files changed, 2 insertions(+), 109 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 7f13ed479e1c..5c2b009b6d50 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -28,7 +28,6 @@
 #include <linux/mutex.h>
 #include <linux/wait.h>
 #include <linux/unistd.h>
-#include <linux/kthread.h>
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/cdev.h>
@@ -57,9 +56,6 @@ struct irctl {
 
 	struct device dev;
 	struct cdev cdev;
-
-	struct task_struct *task;
-	long jiffies_to_wait;
 };
 
 static DEFINE_MUTEX(lirc_dev_lock);
@@ -95,59 +91,6 @@ static void lirc_release(struct device *ld)
 	kfree(ir);
 }
 
-/*  helper function
- *  reads key codes from driver and puts them into buffer
- *  returns 0 on success
- */
-static int lirc_add_to_buf(struct irctl *ir)
-{
-	int res;
-	int got_data = -1;
-
-	if (!ir->d.add_to_buf)
-		return 0;
-
-	/*
-	 * service the device as long as it is returning
-	 * data and we have space
-	 */
-	do {
-		got_data++;
-		res = ir->d.add_to_buf(ir->d.data, ir->buf);
-	} while (!res);
-
-	if (res == -ENODEV)
-		kthread_stop(ir->task);
-
-	return got_data ? 0 : res;
-}
-
-/* main function of the polling thread
- */
-static int lirc_thread(void *irctl)
-{
-	struct irctl *ir = irctl;
-
-	do {
-		if (ir->open) {
-			if (ir->jiffies_to_wait) {
-				set_current_state(TASK_INTERRUPTIBLE);
-				schedule_timeout(ir->jiffies_to_wait);
-			}
-			if (kthread_should_stop())
-				break;
-			if (!lirc_add_to_buf(ir))
-				wake_up_interruptible(&ir->buf->wait_poll);
-		} else {
-			set_current_state(TASK_INTERRUPTIBLE);
-			schedule();
-		}
-	} while (!kthread_should_stop());
-
-	return 0;
-}
-
-
 static const struct file_operations lirc_dev_fops = {
 	.owner		= THIS_MODULE,
 	.read		= lirc_dev_fop_read,
@@ -252,18 +195,8 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 		return -EBADRQC;
 	}
 
-	if (d->sample_rate) {
-		if (2 > d->sample_rate || HZ < d->sample_rate) {
-			dev_err(d->dev, "invalid %d sample rate\n",
-							d->sample_rate);
-			return -EBADRQC;
-		}
-		if (!d->add_to_buf) {
-			dev_err(d->dev, "add_to_buf not set\n");
-			return -EBADRQC;
-		}
-	} else if (!d->rbuf && !(d->fops && d->fops->read &&
-				d->fops->poll && d->fops->unlocked_ioctl)) {
+	if (!d->rbuf && !(d->fops && d->fops->read &&
+			  d->fops->poll && d->fops->unlocked_ioctl)) {
 		dev_err(d->dev, "undefined read, poll, ioctl\n");
 		return -EBADRQC;
 	}
@@ -312,22 +245,6 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 	dev_set_name(&ir->dev, "lirc%d", ir->d.minor);
 	device_initialize(&ir->dev);
 
-	if (d->sample_rate) {
-		ir->jiffies_to_wait = HZ / d->sample_rate;
-
-		/* try to fire up polling thread */
-		ir->task = kthread_run(lirc_thread, (void *)ir, "lirc_dev");
-		if (IS_ERR(ir->task)) {
-			dev_err(d->dev, "cannot run thread for minor = %d\n",
-								d->minor);
-			err = -ECHILD;
-			goto out_sysfs;
-		}
-	} else {
-		/* it means - wait for external event in task queue */
-		ir->jiffies_to_wait = 0;
-	}
-
 	err = lirc_cdev_add(ir);
 	if (err)
 		goto out_sysfs;
@@ -404,10 +321,6 @@ int lirc_unregister_driver(int minor)
 		return -ENOENT;
 	}
 
-	/* end up polling thread */
-	if (ir->task)
-		kthread_stop(ir->task);
-
 	dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
 		ir->d.name, ir->d.minor);
 
@@ -470,9 +383,6 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
 	if (ir->buf)
 		lirc_buffer_clear(ir->buf);
 
-	if (ir->task)
-		wake_up_process(ir->task);
-
 	ir->open++;
 
 	nonseekable_open(inode, file);
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index 436cf1b6a70a..8d8fd8b164e2 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1385,7 +1385,6 @@ static struct lirc_driver lirc_template = {
 	.minor		= -1,
 	.code_length	= 13,
 	.buffer_size	= BUFLEN / 2,
-	.sample_rate	= 0, /* tell lirc_dev to not start its own kthread */
 	.chunk_size	= 2,
 	.fops		= &lirc_fops,
 	.owner		= THIS_MODULE,
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 71c1c11950fe..01649b009922 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -133,12 +133,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
  * @buffer_size:	Number of FIFO buffers with @chunk_size size. If zero,
  *			creates a buffer with BUFLEN size (16 bytes).
  *
- * @sample_rate:	if zero, the device will wait for an event with a new
- *			code to be parsed. Otherwise, specifies the sample
- *			rate for polling. Value should be between 0
- *			and HZ. If equal to HZ, it would mean one polling per
- *			second.
- *
  * @features:		lirc compatible hardware features, like LIRC_MODE_RAW,
  *			LIRC_CAN\_\*, as defined at include/media/lirc.h.
  *
@@ -153,14 +147,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
  * @max_timeout:	Maximum timeout for record. Valid only if
  *			LIRC_CAN_SET_REC_TIMEOUT is defined.
  *
- * @add_to_buf:		add_to_buf will be called after specified period of the
- *			time or triggered by the external event, this behavior
- *			depends on value of the sample_rate this function will
- *			be called in user context. This routine should return
- *			0 if data was added to the buffer and -ENODATA if none
- *			was available. This should add some number of bits
- *			evenly divisible by code_length to the buffer.
- *
  * @rbuf:		if not NULL, it will be used as a read buffer, you will
  *			have to write to the buffer by other means, like irq's
  *			(see also lirc_serial.c).
@@ -184,7 +170,6 @@ struct lirc_driver {
 	int minor;
 	__u32 code_length;
 	unsigned int buffer_size; /* in chunks holding one code each */
-	int sample_rate;
 	__u32 features;
 
 	unsigned int chunk_size;
@@ -192,7 +177,6 @@ struct lirc_driver {
 	void *data;
 	int min_timeout;
 	int max_timeout;
-	int (*add_to_buf)(void *data, struct lirc_buffer *buf);
 	struct lirc_buffer *rbuf;
 	struct rc_dev *rdev;
 	const struct file_operations *fops;

[PATCH 05/16] lirc_dev: clarify error handling

[David Härdeman]

out_sysfs is misleading, sysfs only comes into play after device_add(). Also,
calling device_init() before the rest of struct dev is filled out is clearer.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 5c2b009b6d50..fb487c39b834 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -238,16 +238,16 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 
 	ir->d = *d;
 
+	device_initialize(&ir->dev);
 	ir->dev.devt = MKDEV(MAJOR(lirc_base_dev), ir->d.minor);
 	ir->dev.class = lirc_class;
 	ir->dev.parent = d->dev;
 	ir->dev.release = lirc_release;
 	dev_set_name(&ir->dev, "lirc%d", ir->d.minor);
-	device_initialize(&ir->dev);
 
 	err = lirc_cdev_add(ir);
 	if (err)
-		goto out_sysfs;
+		goto out_free_dev;
 
 	ir->attached = 1;
 
@@ -264,7 +264,7 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 	return minor;
 out_cdev:
 	cdev_del(&ir->cdev);
-out_sysfs:
+out_free_dev:
 	put_device(&ir->dev);
 out_lock:
 	mutex_unlock(&lirc_dev_lock);

[PATCH 06/16] lirc_dev: make fops mandatory

[David Härdeman]

Every caller of lirc_register_driver() passes their own fops and there are no
users of lirc_dev_fop_write() in the kernel tree. Thus we can make fops
mandatory and remove lirc_dev_fop_write().

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |   41 +++++------------------------------------
 include/media/lirc_dev.h    |    3 ---
 2 files changed, 5 insertions(+), 39 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index fb487c39b834..29eecddbd371 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -91,17 +91,6 @@ static void lirc_release(struct device *ld)
 	kfree(ir);
 }
 
-static const struct file_operations lirc_dev_fops = {
-	.owner		= THIS_MODULE,
-	.read		= lirc_dev_fop_read,
-	.write		= lirc_dev_fop_write,
-	.poll		= lirc_dev_fop_poll,
-	.unlocked_ioctl	= lirc_dev_fop_ioctl,
-	.open		= lirc_dev_fop_open,
-	.release	= lirc_dev_fop_close,
-	.llseek		= noop_llseek,
-};
-
 static int lirc_cdev_add(struct irctl *ir)
 {
 	struct lirc_driver *d = &ir->d;
@@ -110,13 +99,11 @@ static int lirc_cdev_add(struct irctl *ir)
 
 	cdev = &ir->cdev;
 
-	if (d->fops) {
-		cdev_init(cdev, d->fops);
-		cdev->owner = d->owner;
-	} else {
-		cdev_init(cdev, &lirc_dev_fops);
-		cdev->owner = THIS_MODULE;
-	}
+	if (!d->fops)
+		return -EINVAL;
+
+	cdev_init(cdev, d->fops);
+	cdev->owner = d->owner;
 	retval = kobject_set_name(&cdev->kobj, "lirc%d", d->minor);
 	if (retval)
 		return retval;
@@ -640,24 +627,6 @@ void *lirc_get_pdata(struct file *file)
 EXPORT_SYMBOL(lirc_get_pdata);
 
 
-ssize_t lirc_dev_fop_write(struct file *file, const char __user *buffer,
-			   size_t length, loff_t *ppos)
-{
-	struct irctl *ir = irctls[iminor(file_inode(file))];
-
-	if (!ir) {
-		pr_err("called with invalid irctl\n");
-		return -ENODEV;
-	}
-
-	if (!ir->attached)
-		return -ENODEV;
-
-	return -EINVAL;
-}
-EXPORT_SYMBOL(lirc_dev_fop_write);
-
-
 static int __init lirc_dev_init(void)
 {
 	int retval;
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 01649b009922..1f327e25a9be 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -210,7 +210,4 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait);
 long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 ssize_t lirc_dev_fop_read(struct file *file, char __user *buffer, size_t length,
 			  loff_t *ppos);
-ssize_t lirc_dev_fop_write(struct file *file, const char __user *buffer,
-			   size_t length, loff_t *ppos);
-
 #endif

[PATCH 07/16] lirc_dev: merge lirc_register_driver() and lirc_allocate_driver()

[David Härdeman]

Merging the two means that lirc_allocate_buffer() is called before device_add()
and cdev_add() which makes more sense. This also simplifies the locking
slightly because lirc_allocate_buffer() will always be called with lirc_dev_lock
held.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |   41 +++++++++++++----------------------------
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 29eecddbd371..fcc88a09b108 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -120,8 +120,6 @@ static int lirc_allocate_buffer(struct irctl *ir)
 	unsigned int buffer_size;
 	struct lirc_driver *d = &ir->d;
 
-	mutex_lock(&lirc_dev_lock);
-
 	bytes_in_key = BITS_TO_LONGS(d->code_length) +
 						(d->code_length % 8 ? 1 : 0);
 	buffer_size = d->buffer_size ? d->buffer_size : BUFLEN / bytes_in_key;
@@ -145,16 +143,15 @@ static int lirc_allocate_buffer(struct irctl *ir)
 		}
 
 		ir->buf_internal = true;
+		d->rbuf = ir->buf;
 	}
 	ir->chunk_size = ir->buf->chunk_size;
 
 out:
-	mutex_unlock(&lirc_dev_lock);
-
 	return err;
 }
 
-static int lirc_allocate_driver(struct lirc_driver *d)
+int lirc_register_driver(struct lirc_driver *d)
 {
 	struct irctl *ir;
 	int minor;
@@ -225,6 +222,15 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 
 	ir->d = *d;
 
+	if (LIRC_CAN_REC(d->features)) {
+		err = lirc_allocate_buffer(irctls[minor]);
+		if (err) {
+			kfree(ir);
+			goto out_lock;
+		}
+		d->rbuf = ir->buf;
+	}
+
 	device_initialize(&ir->dev);
 	ir->dev.devt = MKDEV(MAJOR(lirc_base_dev), ir->d.minor);
 	ir->dev.class = lirc_class;
@@ -248,7 +254,9 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 
 	dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
 		 ir->d.name, ir->d.minor);
+
 	return minor;
+
 out_cdev:
 	cdev_del(&ir->cdev);
 out_free_dev:
@@ -258,29 +266,6 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 
 	return err;
 }
-
-int lirc_register_driver(struct lirc_driver *d)
-{
-	int minor, err = 0;
-
-	minor = lirc_allocate_driver(d);
-	if (minor < 0)
-		return minor;
-
-	if (LIRC_CAN_REC(d->features)) {
-		err = lirc_allocate_buffer(irctls[minor]);
-		if (err)
-			lirc_unregister_driver(minor);
-		else
-			/*
-			 * This is kind of a hack but ir-lirc-codec needs
-			 * access to the buffer that lirc_dev allocated.
-			 */
-			d->rbuf = irctls[minor]->buf;
-	}
-
-	return err ? err : minor;
-}
 EXPORT_SYMBOL(lirc_register_driver);
 
 int lirc_unregister_driver(int minor)

[PATCH 08/16] lirc_zilog: remove module parameter minor

[David Härdeman]

Remove the "minor" module parameter in preparation for the next patch.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/staging/media/lirc/lirc_zilog.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index 8d8fd8b164e2..59e05aa1bc55 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -156,7 +156,6 @@ static struct mutex tx_data_lock;
 /* module parameters */
 static bool debug;	/* debug output */
 static bool tx_only;	/* only handle the IR Tx function */
-static int minor = -1;	/* minor number */
 
 
 /* struct IR reference counting */
@@ -184,10 +183,11 @@ 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 && ir->l.minor < MAX_IRCTL_DEVICES) {
+	if (ir->l.minor >= 0) {
 		lirc_unregister_driver(ir->l.minor);
-		ir->l.minor = MAX_IRCTL_DEVICES;
+		ir->l.minor = -1;
 	}
+
 	if (kfifo_initialized(&ir->rbuf.fifo))
 		lirc_buffer_free(&ir->rbuf);
 	list_del(&ir->list);
@@ -1597,12 +1597,11 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	}
 
 	/* register with lirc */
-	ir->l.minor = minor; /* module option: user requested minor number */
 	ir->l.minor = lirc_register_driver(&ir->l);
-	if (ir->l.minor < 0 || ir->l.minor >= MAX_IRCTL_DEVICES) {
+	if (ir->l.minor < 0) {
 		dev_err(tx->ir->l.dev,
-			"%s: \"minor\" must be between 0 and %d (%d)!\n",
-			__func__, MAX_IRCTL_DEVICES-1, ir->l.minor);
+			"%s: lirc_register_driver() failed: %i\n",
+			__func__, ir->l.minor);
 		ret = -EBADRQC;
 		goto out_put_xx;
 	}
@@ -1673,9 +1672,6 @@ MODULE_LICENSE("GPL");
 /* for compat with old name, which isn't all that accurate anymore */
 MODULE_ALIAS("lirc_pvr150");
 
-module_param(minor, int, 0444);
-MODULE_PARM_DESC(minor, "Preferred minor device number");
-
 module_param(debug, bool, 0644);
 MODULE_PARM_DESC(debug, "Enable debugging messages");
 

[PATCH 09/16] lirc_dev: remove lirc_irctl_init() and lirc_cdev_add()

[David Härdeman]

These two functions only make the logic in lirc_register_driver() harder to
follow.

(Note that almost no other driver calls kobject_set_name() on their cdev so I
simply removed that part).

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |   44 ++++++++++++-------------------------------
 1 file changed, 12 insertions(+), 32 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index fcc88a09b108..574f4dd416b8 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -65,15 +65,6 @@ static struct irctl *irctls[MAX_IRCTL_DEVICES];
 /* Only used for sysfs but defined to void otherwise */
 static struct class *lirc_class;
 
-/*  helper function
- *  initializes the irctl structure
- */
-static void lirc_irctl_init(struct irctl *ir)
-{
-	mutex_init(&ir->irctl_lock);
-	ir->d.minor = NOPLUG;
-}
-
 static void lirc_release(struct device *ld)
 {
 	struct irctl *ir = container_of(ld, struct irctl, dev);
@@ -91,27 +82,6 @@ static void lirc_release(struct device *ld)
 	kfree(ir);
 }
 
-static int lirc_cdev_add(struct irctl *ir)
-{
-	struct lirc_driver *d = &ir->d;
-	struct cdev *cdev;
-	int retval;
-
-	cdev = &ir->cdev;
-
-	if (!d->fops)
-		return -EINVAL;
-
-	cdev_init(cdev, d->fops);
-	cdev->owner = d->owner;
-	retval = kobject_set_name(&cdev->kobj, "lirc%d", d->minor);
-	if (retval)
-		return retval;
-
-	cdev->kobj.parent = &ir->dev.kobj;
-	return cdev_add(cdev, ir->dev.devt, 1);
-}
-
 static int lirc_allocate_buffer(struct irctl *ir)
 {
 	int err = 0;
@@ -167,6 +137,11 @@ int lirc_register_driver(struct lirc_driver *d)
 		return -EINVAL;
 	}
 
+	if (!d->fops) {
+		pr_err("fops pointer not filled in!\n");
+		return -EINVAL;
+	}
+
 	if (d->minor >= MAX_IRCTL_DEVICES) {
 		dev_err(d->dev, "minor must be between 0 and %d!\n",
 						MAX_IRCTL_DEVICES - 1);
@@ -210,7 +185,8 @@ int lirc_register_driver(struct lirc_driver *d)
 		err = -ENOMEM;
 		goto out_lock;
 	}
-	lirc_irctl_init(ir);
+
+	mutex_init(&ir->irctl_lock);
 	irctls[minor] = ir;
 	d->minor = minor;
 
@@ -238,7 +214,11 @@ int lirc_register_driver(struct lirc_driver *d)
 	ir->dev.release = lirc_release;
 	dev_set_name(&ir->dev, "lirc%d", ir->d.minor);
 
-	err = lirc_cdev_add(ir);
+	cdev_init(&ir->cdev, d->fops);
+	ir->cdev.owner = ir->d.owner;
+	ir->cdev.kobj.parent = &ir->dev.kobj;
+
+	err = cdev_add(&ir->cdev, ir->dev.devt, 1);
 	if (err)
 		goto out_free_dev;
 

[PATCH 10/16] lirc_dev: remove superfluous get/put_device() calls

[David Härdeman]

device_add() and friends alredy manage the references to the parent device so
these calls aren't necessary.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 574f4dd416b8..34bd3f8bf30d 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -69,8 +69,6 @@ static void lirc_release(struct device *ld)
 {
 	struct irctl *ir = container_of(ld, struct irctl, dev);
 
-	put_device(ir->dev.parent);
-
 	if (ir->buf_internal) {
 		lirc_buffer_free(ir->buf);
 		kfree(ir->buf);
@@ -230,8 +228,6 @@ int lirc_register_driver(struct lirc_driver *d)
 
 	mutex_unlock(&lirc_dev_lock);
 
-	get_device(ir->dev.parent);
-
 	dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
 		 ir->d.name, ir->d.minor);
 

[PATCH 11/16] lirc_dev: remove unused module parameter

[David Härdeman]

The "debug" parameter isn't actually used anywhere.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 34bd3f8bf30d..57d21201ff93 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -36,8 +36,6 @@
 #include <media/lirc.h>
 #include <media/lirc_dev.h>
 
-static bool debug;
-
 #define IRCTL_DEV_NAME	"BaseRemoteCtl"
 #define NOPLUG		-1
 #define LOGHEAD		"lirc_dev (%s[%d]): "
@@ -625,6 +623,3 @@ module_exit(lirc_dev_exit);
 MODULE_DESCRIPTION("LIRC base driver module");
 MODULE_AUTHOR("Artur Lipowski");
 MODULE_LICENSE("GPL");
-
-module_param(debug, bool, S_IRUGO | S_IWUSR);
-MODULE_PARM_DESC(debug, "Enable debugging messages");

[PATCH 12/16] lirc_dev: return POLLHUP and POLLERR when device is gone

[David Härdeman]

Most drivers return both values when the device is gone.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 57d21201ff93..e44e0b23b883 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -374,7 +374,7 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
 	}
 
 	if (!ir->attached)
-		return POLLERR;
+		return POLLHUP | POLLERR;
 
 	if (ir->buf) {
 		poll_wait(file, &ir->buf->wait_poll, wait);

[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 <david@hardeman.nu>
---
 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;
 
 	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;
+	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;
 
-	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.
  */
 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.

[PATCH 14/16] lirc_dev: cleanup includes

[David Härdeman]

Remove superfluous includes and defines.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |   12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 7db7d4c57991..4ba6c7e2d41b 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -15,20 +15,11 @@
  *
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/module.h>
-#include <linux/kernel.h>
 #include <linux/sched/signal.h>
-#include <linux/errno.h>
 #include <linux/ioctl.h>
-#include <linux/fs.h>
 #include <linux/poll.h>
-#include <linux/completion.h>
 #include <linux/mutex.h>
-#include <linux/wait.h>
-#include <linux/unistd.h>
-#include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/cdev.h>
 #include <linux/idr.h>
@@ -37,7 +28,6 @@
 #include <media/lirc.h>
 #include <media/lirc_dev.h>
 
-#define IRCTL_DEV_NAME	"BaseRemoteCtl"
 #define LOGHEAD		"lirc_dev (%s[%d]): "
 
 static dev_t lirc_base_dev;
@@ -545,7 +535,7 @@ static int __init lirc_dev_init(void)
 	}
 
 	retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
-				     IRCTL_DEV_NAME);
+				     "BaseRemoteCtl");
 	if (retval) {
 		class_destroy(lirc_class);
 		pr_err("alloc_chrdev_region failed\n");

[PATCH 15/16] lirc_dev: remove name from struct lirc_driver

[David Härdeman]

The name is only used for a few debug messages and the name of the parent
device as well as the name of the lirc device (e.g. "lirc0") are sufficient
anyway.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/ir-lirc-codec.c        |    2 --
 drivers/media/rc/lirc_dev.c             |   25 ++++++++-----------------
 drivers/staging/media/lirc/lirc_zilog.c |    1 -
 include/media/lirc_dev.h                |    3 ---
 4 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 2c1221a61ea1..74ce27f92901 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -380,8 +380,6 @@ static int ir_lirc_register(struct rc_dev *dev)
 	if (dev->max_timeout)
 		features |= LIRC_CAN_SET_REC_TIMEOUT;
 
-	snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
-		 dev->driver_name);
 	drv->features = features;
 	drv->data = &dev->raw->lirc;
 	drv->rbuf = NULL;
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 4ba6c7e2d41b..10783ef75a25 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -28,8 +28,6 @@
 #include <media/lirc.h>
 #include <media/lirc_dev.h>
 
-#define LOGHEAD		"lirc_dev (%s[%d]): "
-
 static dev_t lirc_base_dev;
 
 /**
@@ -160,7 +158,6 @@ lirc_register_driver(struct lirc_driver *d)
 		return -EBADRQC;
 	}
 
-	d->name[sizeof(d->name)-1] = '\0';
 	if (d->features == 0)
 		d->features = LIRC_CAN_REC_LIRCCODE;
 
@@ -207,8 +204,7 @@ lirc_register_driver(struct lirc_driver *d)
 	if (err)
 		goto out_cdev;
 
-	dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
-		 ir->d.name, ir->d.minor);
+	dev_info(ir->d.dev, "lirc device registered as lirc%d\n", minor);
 
 	d->lirc_internal = ir;
 	return 0;
@@ -242,13 +238,11 @@ lirc_unregister_driver(struct lirc_driver *d)
 
 	mutex_lock(&ir->mutex);
 
-	dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
-		ir->d.name, ir->d.minor);
+	dev_dbg(&ir->dev, "unregistered\n");
 
 	ir->dead = true;
 	if (ir->users) {
-		dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
-			ir->d.name, ir->d.minor);
+		dev_dbg(&ir->dev, "releasing opened driver\n");
 		wake_up_interruptible(&ir->buf->wait_poll);
 	}
 
@@ -278,7 +272,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
 
 	mutex_unlock(&ir->mutex);
 
-	dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
+	dev_dbg(&ir->dev, "open called\n");
 
 	if (ir->d.rdev) {
 		retval = rc_open(ir->d.rdev);
@@ -332,8 +326,7 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
 	else
 		ret = POLLIN | POLLRDNORM;
 
-	dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n",
-		ir->d.name, ir->d.minor, ret);
+	dev_dbg(&ir->dev, "poll result = %d\n", ret);
 
 	return ret;
 }
@@ -345,12 +338,10 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	__u32 mode;
 	int result = 0;
 
-	dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
-		ir->d.name, ir->d.minor, cmd);
+	dev_dbg(&ir->dev, "ioctl called (0x%x)\n", cmd);
 
 	if (ir->dead) {
-		dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
-			ir->d.name, ir->d.minor);
+		dev_err(&ir->dev, "ioctl result = -ENODEV\n");
 		return -ENODEV;
 	}
 
@@ -428,7 +419,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
 	if (!LIRC_CAN_REC(ir->d.features))
 		return -EINVAL;
 
-	dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
+	dev_dbg(&ir->dev, "read called\n");
 
 	buf = kzalloc(ir->chunk_size, GFP_KERNEL);
 	if (!buf)
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index ffb70dee4547..131d87a04aab 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1377,7 +1377,6 @@ static const struct file_operations lirc_fops = {
 };
 
 static struct lirc_driver lirc_template = {
-	.name		= "lirc_zilog",
 	.code_length	= 13,
 	.buffer_size	= BUFLEN / 2,
 	.chunk_size	= 2,
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index f7629ff116a9..11f455a34090 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -120,8 +120,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
 /**
  * struct lirc_driver - Defines the parameters on a LIRC driver
  *
- * @name:		this string will be used for logs
- *
  * @minor:		indicates minor device (/dev/lirc) number for
  *			registered driver if caller fills it with negative
  *			value, then the first free minor number will be used
@@ -167,7 +165,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
  * @lirc_internal:	lirc_dev bookkeeping data, don't touch.
  */
 struct lirc_driver {
-	char name[40];
 	int minor;
 	__u32 code_length;
 	unsigned int buffer_size; /* in chunks holding one code each */

[PATCH 16/16] lirc_dev: cleanup header

[David Härdeman]

Remove some stuff from lirc_dev.h which is not used anywhere.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 include/media/lirc_dev.h |   11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 11f455a34090..af738d522dec 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -9,10 +9,6 @@
 #ifndef _LINUX_LIRC_DEV_H
 #define _LINUX_LIRC_DEV_H
 
-#define BUFLEN            16
-
-#define mod(n, div) ((n) % (div))
-
 #include <linux/slab.h>
 #include <linux/fs.h>
 #include <linux/ioctl.h>
@@ -20,6 +16,8 @@
 #include <linux/kfifo.h>
 #include <media/lirc.h>
 
+#define BUFLEN            16
+
 struct lirc_buffer {
 	wait_queue_head_t wait_poll;
 	spinlock_t fifo_lock;
@@ -89,11 +87,6 @@ static inline int lirc_buffer_empty(struct lirc_buffer *buf)
 	return !lirc_buffer_len(buf);
 }
 
-static inline int lirc_buffer_available(struct lirc_buffer *buf)
-{
-	return buf->size - (lirc_buffer_len(buf) / buf->chunk_size);
-}
-
 static inline unsigned int lirc_buffer_read(struct lirc_buffer *buf,
 					    unsigned char *dest)
 {

Re: [PATCH 15/16] lirc_dev: remove name from struct lirc_driver

[Sean Young]

On Mon, May 01, 2017 at 06:04:52PM +0200, David Härdeman wrote:
> The name is only used for a few debug messages and the name of the parent
> device as well as the name of the lirc device (e.g. "lirc0") are sufficient
> anyway.
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/ir-lirc-codec.c        |    2 --
>  drivers/media/rc/lirc_dev.c             |   25 ++++++++-----------------
>  drivers/staging/media/lirc/lirc_zilog.c |    1 -
>  include/media/lirc_dev.h                |    3 ---
>  4 files changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> index 2c1221a61ea1..74ce27f92901 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -380,8 +380,6 @@ static int ir_lirc_register(struct rc_dev *dev)
>  	if (dev->max_timeout)
>  		features |= LIRC_CAN_SET_REC_TIMEOUT;
>  
> -	snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
> -		 dev->driver_name);
>  	drv->features = features;
>  	drv->data = &dev->raw->lirc;
>  	drv->rbuf = NULL;
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 4ba6c7e2d41b..10783ef75a25 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -28,8 +28,6 @@
>  #include <media/lirc.h>
>  #include <media/lirc_dev.h>
>  
> -#define LOGHEAD		"lirc_dev (%s[%d]): "
> -
>  static dev_t lirc_base_dev;
>  
>  /**
> @@ -160,7 +158,6 @@ lirc_register_driver(struct lirc_driver *d)
>  		return -EBADRQC;
>  	}
>  
> -	d->name[sizeof(d->name)-1] = '\0';
>  	if (d->features == 0)
>  		d->features = LIRC_CAN_REC_LIRCCODE;
>  
> @@ -207,8 +204,7 @@ lirc_register_driver(struct lirc_driver *d)
>  	if (err)
>  		goto out_cdev;
>  
> -	dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
> -		 ir->d.name, ir->d.minor);
> +	dev_info(ir->d.dev, "lirc device registered as lirc%d\n", minor);

I'm not so sure this is a good idea. First of all, the documentation says
you can use "dmesg |grep lirc_dev" to find your lirc devices and you've
just replaced lirc_dev with lirc.

https://linuxtv.org/downloads/v4l-dvb-apis/uapi/rc/lirc-dev-intro.html

It's useful having the driver name in the message. For example, I have
two rc devices connected usually:

[sean@bigcore ~]$ dmesg | grep lirc_dev
[    5.938284] lirc_dev: IR Remote Control driver registered, major 239
[    5.945324] rc rc0: lirc_dev: driver ir-lirc-codec (winbond-cir) registered at minor = 0
[ 5111.830118] rc rc1: lirc_dev: driver ir-lirc-codec (mceusb) registered at minor = 1

With the driver name I know which one is which.

Maybe lirc_driver.name should be a "const char*" so no strcpy is needed
(the ir-lirc-codec does not seem necessary).

Thanks

Sean

>  
>  	d->lirc_internal = ir;
>  	return 0;
> @@ -242,13 +238,11 @@ lirc_unregister_driver(struct lirc_driver *d)
>  
>  	mutex_lock(&ir->mutex);
>  
> -	dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
> -		ir->d.name, ir->d.minor);
> +	dev_dbg(&ir->dev, "unregistered\n");
>  
>  	ir->dead = true;
>  	if (ir->users) {
> -		dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
> -			ir->d.name, ir->d.minor);
> +		dev_dbg(&ir->dev, "releasing opened driver\n");
>  		wake_up_interruptible(&ir->buf->wait_poll);
>  	}
>  
> @@ -278,7 +272,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
>  
>  	mutex_unlock(&ir->mutex);
>  
> -	dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
> +	dev_dbg(&ir->dev, "open called\n");
>  
>  	if (ir->d.rdev) {
>  		retval = rc_open(ir->d.rdev);
> @@ -332,8 +326,7 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
>  	else
>  		ret = POLLIN | POLLRDNORM;
>  
> -	dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n",
> -		ir->d.name, ir->d.minor, ret);
> +	dev_dbg(&ir->dev, "poll result = %d\n", ret);
>  
>  	return ret;
>  }
> @@ -345,12 +338,10 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	__u32 mode;
>  	int result = 0;
>  
> -	dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
> -		ir->d.name, ir->d.minor, cmd);
> +	dev_dbg(&ir->dev, "ioctl called (0x%x)\n", cmd);
>  
>  	if (ir->dead) {
> -		dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
> -			ir->d.name, ir->d.minor);
> +		dev_err(&ir->dev, "ioctl result = -ENODEV\n");
>  		return -ENODEV;
>  	}
>  
> @@ -428,7 +419,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
>  	if (!LIRC_CAN_REC(ir->d.features))
>  		return -EINVAL;
>  
> -	dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
> +	dev_dbg(&ir->dev, "read called\n");
>  
>  	buf = kzalloc(ir->chunk_size, GFP_KERNEL);
>  	if (!buf)
> diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
> index ffb70dee4547..131d87a04aab 100644
> --- a/drivers/staging/media/lirc/lirc_zilog.c
> +++ b/drivers/staging/media/lirc/lirc_zilog.c
> @@ -1377,7 +1377,6 @@ static const struct file_operations lirc_fops = {
>  };
>  
>  static struct lirc_driver lirc_template = {
> -	.name		= "lirc_zilog",
>  	.code_length	= 13,
>  	.buffer_size	= BUFLEN / 2,
>  	.chunk_size	= 2,
> diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
> index f7629ff116a9..11f455a34090 100644
> --- a/include/media/lirc_dev.h
> +++ b/include/media/lirc_dev.h
> @@ -120,8 +120,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
>  /**
>   * struct lirc_driver - Defines the parameters on a LIRC driver
>   *
> - * @name:		this string will be used for logs
> - *
>   * @minor:		indicates minor device (/dev/lirc) number for
>   *			registered driver if caller fills it with negative
>   *			value, then the first free minor number will be used
> @@ -167,7 +165,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
>   * @lirc_internal:	lirc_dev bookkeeping data, don't touch.
>   */
>  struct lirc_driver {
> -	char name[40];
>  	int minor;
>  	__u32 code_length;
>  	unsigned int buffer_size; /* in chunks holding one code each */

Re: [PATCH 15/16] lirc_dev: remove name from struct lirc_driver

[David Härdeman]

On Tue, May 02, 2017 at 06:04:18PM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:04:52PM +0200, David Härdeman wrote:
>> The name is only used for a few debug messages and the name of the parent
>> device as well as the name of the lirc device (e.g. "lirc0") are sufficient
>> anyway.
>>...
>> @@ -207,8 +204,7 @@ lirc_register_driver(struct lirc_driver *d)
>>  	if (err)
>>  		goto out_cdev;
>>  
>> -	dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
>> -		 ir->d.name, ir->d.minor);
>> +	dev_info(ir->d.dev, "lirc device registered as lirc%d\n", minor);
>
>I'm not so sure this is a good idea. First of all, the documentation says
>you can use "dmesg |grep lirc_dev" to find your lirc devices and you've
>just replaced lirc_dev with lirc.

Sure, no strong preferences here, you could change the line to say
"lirc_dev device...", or drop the patch.

>https://linuxtv.org/downloads/v4l-dvb-apis/uapi/rc/lirc-dev-intro.html
>
>It's useful having the driver name in the message. For example, I have
>two rc devices connected usually:
>
>[sean@bigcore ~]$ dmesg | grep lirc_dev
>[    5.938284] lirc_dev: IR Remote Control driver registered, major 239
>[    5.945324] rc rc0: lirc_dev: driver ir-lirc-codec (winbond-cir) registered at minor = 0
>[ 5111.830118] rc rc1: lirc_dev: driver ir-lirc-codec (mceusb) registered at minor = 1

winbond-cir....good man :)

How about "dmesg | grep lirc -A2 -B2"?

I don't think the situation is that different from how you'd know which
input dev is allocated to any given rc_dev? With this patch applied the
relevant output will be:

[    0.393494] rc rc0: rc-core loopback device as /devices/virtual/rc/rc0
[    0.394458] input: rc-core loopback device as /devices/virtual/rc/rc0/input2
[    0.395717] rc rc0: lirc device registered as lirc0
[   12.612313] rc rc1: mceusb device as /devices/virtual/rc/rc1
[   12.612768] input: mceusb device as /devices/virtual/rc/rc1/input4
[   12.613112] rc rc1: lirc device registered as lirc1

(and we might want to change the lirc line to include the sysfs path?)

But realistically, how much dmesg grepping are we expecting normal
end-users to be doing?

Anyway, as I said, this patch isn't crucial, and we can revisit printk's
later (I'm looking at the ioctl locking right now and I think an
ir-lirc-codec and lirc_dev merger might be a good idea once the fate of
lirc_zilog has been decided).

>With the driver name I know which one is which.
>
>Maybe lirc_driver.name should be a "const char*" so no strcpy is needed
>(the ir-lirc-codec does not seem necessary).

Not that it really pertains to whether d->name should be kept or not,
but I think that lirc_dev shouldn't copy the lirc_driver struct into an
irctl struct internal copy at all, but just keep a normal pointer. I
haven't gotten around to vetting the (ab)use of the lirc_driver struct
yet though.

Regards,
David

Re: [PATCH 14/16] lirc_dev: cleanup includes

[Sean Young]

On Mon, May 01, 2017 at 06:04:47PM +0200, David Härdeman wrote:
> Remove superfluous includes and defines.
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/lirc_dev.c |   12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 7db7d4c57991..4ba6c7e2d41b 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -15,20 +15,11 @@
>   *
>   */
>  
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
>  #include <linux/module.h>
> -#include <linux/kernel.h>
>  #include <linux/sched/signal.h>
> -#include <linux/errno.h>
>  #include <linux/ioctl.h>
> -#include <linux/fs.h>
>  #include <linux/poll.h>
> -#include <linux/completion.h>
>  #include <linux/mutex.h>
> -#include <linux/wait.h>
> -#include <linux/unistd.h>
> -#include <linux/bitops.h>
>  #include <linux/device.h>
>  #include <linux/cdev.h>
>  #include <linux/idr.h>
> @@ -37,7 +28,6 @@
>  #include <media/lirc.h>
>  #include <media/lirc_dev.h>
>  
> -#define IRCTL_DEV_NAME	"BaseRemoteCtl"
>  #define LOGHEAD		"lirc_dev (%s[%d]): "
>  
>  static dev_t lirc_base_dev;
> @@ -545,7 +535,7 @@ static int __init lirc_dev_init(void)
>  	}
>  
>  	retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
> -				     IRCTL_DEV_NAME);
> +				     "BaseRemoteCtl");

This has always surprised/annoyed me. Why is this called BaseRemoteCtl? As
far as I know, this is only used for /proc/devices, where it says:

$ grep 239 /proc/devices 
239 BaseRemoteCtl

And not lirc, as you would expect.

Sean

Re: [PATCH 14/16] lirc_dev: cleanup includes

[David Härdeman]

On Fri, May 19, 2017 at 07:21:23PM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:04:47PM +0200, David Härdeman wrote:
>> Remove superfluous includes and defines.
>> 
>> Signed-off-by: David Härdeman <david@hardeman.nu>
>> ---
>>  drivers/media/rc/lirc_dev.c |   12 +-----------
>>  1 file changed, 1 insertion(+), 11 deletions(-)
>> 
>> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
>> index 7db7d4c57991..4ba6c7e2d41b 100644
>> --- a/drivers/media/rc/lirc_dev.c
>> +++ b/drivers/media/rc/lirc_dev.c
>> @@ -15,20 +15,11 @@
>>   *
>>   */
>>  
>> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> -
>>  #include <linux/module.h>
>> -#include <linux/kernel.h>
>>  #include <linux/sched/signal.h>
>> -#include <linux/errno.h>
>>  #include <linux/ioctl.h>
>> -#include <linux/fs.h>
>>  #include <linux/poll.h>
>> -#include <linux/completion.h>
>>  #include <linux/mutex.h>
>> -#include <linux/wait.h>
>> -#include <linux/unistd.h>
>> -#include <linux/bitops.h>
>>  #include <linux/device.h>
>>  #include <linux/cdev.h>
>>  #include <linux/idr.h>
>> @@ -37,7 +28,6 @@
>>  #include <media/lirc.h>
>>  #include <media/lirc_dev.h>
>>  
>> -#define IRCTL_DEV_NAME	"BaseRemoteCtl"
>>  #define LOGHEAD		"lirc_dev (%s[%d]): "
>>  
>>  static dev_t lirc_base_dev;
>> @@ -545,7 +535,7 @@ static int __init lirc_dev_init(void)
>>  	}
>>  
>>  	retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
>> -				     IRCTL_DEV_NAME);
>> +				     "BaseRemoteCtl");
>
>This has always surprised/annoyed me. Why is this called BaseRemoteCtl? As
>far as I know, this is only used for /proc/devices, where it says:
>
>$ grep 239 /proc/devices 
>239 BaseRemoteCtl
>
>And not lirc, as you would expect.

Yeah, I also find it a bit of an ugly wart. I didn't dare to change it
though since userspace might rely on "BaseRemoteCtl". For example:
https://build.opensuse.org/package/view_file/openSUSE:12.2/lirc/rc.lirc?expand=1)

-- 
David Härdeman

Re: [PATCH 03/16] lirc_dev: correct error handling

[Sean Young]

On Mon, May 01, 2017 at 06:03:51PM +0200, David Härdeman wrote:
> If an error is generated, nonseekable_open() shouldn't be called.

There is no harm in calling nonseekable_open(), so this commit is
misleading.

Sean

> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/lirc_dev.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 05f600bd6c67..7f13ed479e1c 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -431,7 +431,7 @@ EXPORT_SYMBOL(lirc_unregister_driver);
>  int lirc_dev_fop_open(struct inode *inode, struct file *file)
>  {
>  	struct irctl *ir;
> -	int retval = 0;
> +	int retval;
>  
>  	if (iminor(inode) >= MAX_IRCTL_DEVICES) {
>  		pr_err("open result for %d is -ENODEV\n", iminor(inode));
> @@ -475,9 +475,11 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
>  
>  	ir->open++;
>  
> -error:
>  	nonseekable_open(inode, file);
>  
> +	return 0;
> +
> +error:
>  	return retval;
>  }
>  EXPORT_SYMBOL(lirc_dev_fop_open);

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 <david@hardeman.nu>
> ---
>  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.

> +	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.

Re: [PATCH 03/16] lirc_dev: correct error handling

[David Härdeman]

On Sun, May 21, 2017 at 09:57:13AM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:03:51PM +0200, David Härdeman wrote:
>> If an error is generated, nonseekable_open() shouldn't be called.
>
>There is no harm in calling nonseekable_open(), so this commit is
>misleading.

I'm not sure why you consider it misleading? If there's an error, the
logic thing to do is to error out immediately and not do any further
work?

>> Signed-off-by: David Härdeman <david@hardeman.nu>
>> ---
>>  drivers/media/rc/lirc_dev.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
>> index 05f600bd6c67..7f13ed479e1c 100644
>> --- a/drivers/media/rc/lirc_dev.c
>> +++ b/drivers/media/rc/lirc_dev.c
>> @@ -431,7 +431,7 @@ EXPORT_SYMBOL(lirc_unregister_driver);
>>  int lirc_dev_fop_open(struct inode *inode, struct file *file)
>>  {
>>  	struct irctl *ir;
>> -	int retval = 0;
>> +	int retval;
>>  
>>  	if (iminor(inode) >= MAX_IRCTL_DEVICES) {
>>  		pr_err("open result for %d is -ENODEV\n", iminor(inode));
>> @@ -475,9 +475,11 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
>>  
>>  	ir->open++;
>>  
>> -error:
>>  	nonseekable_open(inode, file);
>>  
>> +	return 0;
>> +
>> +error:
>>  	return retval;
>>  }
>>  EXPORT_SYMBOL(lirc_dev_fop_open);

-- 
David Härdeman

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 <david@hardeman.nu>
>> ---
>>  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

Re: [PATCH 03/16] lirc_dev: correct error handling

[Sean Young]

On Sun, May 28, 2017 at 10:23:37AM +0200, David Härdeman wrote:
> On Sun, May 21, 2017 at 09:57:13AM +0100, Sean Young wrote:
> >On Mon, May 01, 2017 at 06:03:51PM +0200, David Härdeman wrote:
> >> If an error is generated, nonseekable_open() shouldn't be called.
> >
> >There is no harm in calling nonseekable_open(), so this commit is
> >misleading.
> 
> I'm not sure why you consider it misleading? If there's an error, the
> logic thing to do is to error out immediately and not do any further
> work?

The commit message says that nonseekable_open() should not be called,
suggesting there is a bug which is not the case.


Sean

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 <david@hardeman.nu>
> >> ---
> >>  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.

Your patch adds the lirc_internal pointer to work around this, which makes
the code even worse than it is.


Sean

> 
> >> +	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

Re: [PATCH 03/16] lirc_dev: correct error handling

[David Härdeman]

On Sun, May 28, 2017 at 04:04:30PM +0100, Sean Young wrote:
>On Sun, May 28, 2017 at 10:23:37AM +0200, David Härdeman wrote:
>> On Sun, May 21, 2017 at 09:57:13AM +0100, Sean Young wrote:
>> >On Mon, May 01, 2017 at 06:03:51PM +0200, David Härdeman wrote:
>> >> If an error is generated, nonseekable_open() shouldn't be called.
>> >
>> >There is no harm in calling nonseekable_open(), so this commit is
>> >misleading.
>> 
>> I'm not sure why you consider it misleading? If there's an error, the
>> logic thing to do is to error out immediately and not do any further
>> work?
>
>The commit message says that nonseekable_open() should not be called,
>suggesting there is a bug which is not the case.

I'll do another version with an updated commit message then :)

-- 
David Härdeman