Re: [PATCH 07/19] lirc_dev: remove kmalloc in lirc_dev_fop_read()

[David Härdeman]

October 4, 2017 6:57 PM, "Mauro Carvalho Chehab" <mche...@s-opensource.com> 
wrote:

> Em Sun, 25 Jun 2017 14:31:50 +0200
> David Härdeman <da...@hardeman.nu> escreveu:
> 
>> lirc_zilog uses a chunk_size of 2 and ir-lirc-codec uses sizeof(int).
>> 
>> Therefore, using stack memory should be perfectly fine.
>> 
>> Signed-off-by: David Härdeman <da...@hardeman.nu>
>> ---
>> drivers/media/rc/lirc_dev.c | 8 +---
>> 1 file changed, 1 insertion(+), 7 deletions(-)
>> 
>> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
>> index 1773a2934484..92048d945ba7 100644
>> --- a/drivers/media/rc/lirc_dev.c
>> +++ b/drivers/media/rc/lirc_dev.c
>> @@ -376,7 +376,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
>> loff_t *ppos)
>> {
>> struct irctl *ir = file->private_data;
>> - unsigned char *buf;
>> + unsigned char buf[ir->buf->chunk_size];
> 
> No. We don't do dynamic buffer allocation on stak at the Kernel,
> as this could cause the Linux stack to overflow without notice.

While the general policy is to avoid large stack allocations (in order to not 
overflow the 4K stack), I'm not aware of a general ban on *any* stack 
allocations - that sounds like an overly dogmatic approach. If such a generic 
ban exists, could you please point me to some kind of discussion/message to 
that effect?

The commit message clearly explained what kind of stack allocations we're 
talking about here (i.e. sizeof(int) is the maximum), so the stack allocation 
is clearly not able to cause stack overflows. And once the last lirc driver is 
gone, this can be changed to a simple int (which would also be allocated on 
stack).

Regards,
David

Re: [PATCH 2/2] rc-main: remove input events for repeat messages

[David Härdeman]

On Sat, Jul 01, 2017 at 01:20:50PM +0100, Sean Young wrote:
>On Thu, Jun 22, 2017 at 09:24:00PM +0200, David Härdeman wrote:
>> Protocols like NEC generate around 10 repeat events per second.
>> 
>> The input events are not very useful for userspace but still waste power
>> by waking up every listener. So let's remove them (MSC_SCAN events
>> are still generated for the initial keypress).
>> 
>> Signed-off-by: David Härdeman <da...@hardeman.nu>
>> ---
>>  drivers/media/rc/rc-main.c |   13 -
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>> index 7387bd4d75b0..9f490aa11bc4 100644
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -616,16 +616,11 @@ void rc_repeat(struct rc_dev *dev)
>>  
>>  spin_lock_irqsave(>keylock, flags);
>>  
>> -if (!dev->keypressed)
>> -goto out;
>> -
>> -input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
>> -input_sync(dev->input_dev);
>
>I don't agree with this. It's good to see something in user space when
>a repeat received. This is useful for debugging purposes.

Not going to press the issue, but dev_dbg might be another option if
debugging is the intended use-case?

-- 
David Härdeman

[PATCH] lirc_dev: sanitize locking (v2)

[David Härdeman]

Use the irctl mutex for all device operations and only use lirc_dev_lock to
protect the irctls array. Also, make sure that the device is alive early in
each fops function before doing anything else.

Since this patch touches nearly every line where the irctl mutex is
taken/released, it also renames the mutex at the same time (the name
irctl_lock will be misleading once struct irctl goes away in later
patches).

V2: make sure ir->d.minor is set as well once allocated
(found by Fengguang Wu <fengguang...@intel.com>)

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index aece6b619796..55ed65621bbd 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -38,7 +38,7 @@ struct irctl {
bool attached;
int open;
 
-   struct mutex irctl_lock;
+   struct mutex mutex;
struct lirc_buffer *buf;
bool buf_internal;
 
@@ -46,6 +46,7 @@ struct irctl {
struct cdev cdev;
 };
 
+/* This mutex protects the irctls array */
 static DEFINE_MUTEX(lirc_dev_lock);
 
 static struct irctl *irctls[MAX_IRCTL_DEVICES];
@@ -53,18 +54,23 @@ static struct irctl *irctls[MAX_IRCTL_DEVICES];
 /* Only used for sysfs but defined to void otherwise */
 static struct class *lirc_class;
 
-static void lirc_release(struct device *ld)
+static void lirc_free_buffer(struct irctl *ir)
 {
-   struct irctl *ir = container_of(ld, struct irctl, dev);
-
if (ir->buf_internal) {
lirc_buffer_free(ir->buf);
kfree(ir->buf);
+   ir->buf = NULL;
}
+}
+
+static void lirc_release(struct device *ld)
+{
+   struct irctl *ir = container_of(ld, struct irctl, dev);
 
mutex_lock(_dev_lock);
irctls[ir->d.minor] = NULL;
mutex_unlock(_dev_lock);
+   lirc_free_buffer(ir);
kfree(ir);
 }
 
@@ -141,6 +147,28 @@ int lirc_register_driver(struct lirc_driver *d)
return -EBADRQC;
}
 
+   /* some safety check 8-) */
+   d->name[sizeof(d->name)-1] = '\0';
+
+   if (d->features == 0)
+   d->features = LIRC_CAN_REC_LIRCCODE;
+
+   ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
+   if (!ir)
+   return -ENOMEM;
+
+   mutex_init(>mutex);
+   ir->d = *d;
+
+   if (LIRC_CAN_REC(d->features)) {
+   err = lirc_allocate_buffer(ir);
+   if (err) {
+   kfree(ir);
+   return err;
+   }
+   d->rbuf = ir->buf;
+   }
+
mutex_lock(_dev_lock);
 
/* find first free slot for driver */
@@ -150,37 +178,18 @@ int lirc_register_driver(struct lirc_driver *d)
 
if (minor == MAX_IRCTL_DEVICES) {
dev_err(d->dev, "no free slots for drivers!\n");
-   err = -ENOMEM;
-   goto out_lock;
-   }
-
-   ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
-   if (!ir) {
-   err = -ENOMEM;
-   goto out_lock;
+   mutex_unlock(_dev_lock);
+   lirc_free_buffer(ir);
+   kfree(ir);
+   return -ENOMEM;
}
 
-   mutex_init(>irctl_lock);
irctls[minor] = ir;
d->irctl = ir;
d->minor = 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;
-
-   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;
-   }
+   mutex_unlock(_dev_lock);
 
device_initialize(>dev);
ir->dev.devt = MKDEV(MAJOR(lirc_base_dev), ir->d.minor);
@@ -194,22 +203,15 @@ int lirc_register_driver(struct lirc_driver *d)
ir->attached = true;
 
err = cdev_device_add(>cdev, >dev);
-   if (err)
-   goto out_dev;
-
-   mutex_unlock(_dev_lock);
+   if (err) {
+   put_device(>dev);
+   return err;
+   }
 
dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
 ir->d.name, ir->d.minor);
 
return 0;
-
-out_dev:
-   put_device(>dev);
-out_lock:
-   mutex_unlock(_dev_lock);
-
-   return err;
 }
 EXPORT_SYMBOL(lirc_register_driver);
 
@@ -222,11 +224,13 @@ void lirc_unregister_driver(struct lirc_driver *d)
 
ir = d->irctl;
 
-   mutex_lock(_dev_lock);
-
dev_dbg(ir->d.dev, &q

[PATCH 18/19] ir-lirc-codec: move the remaining fops over from lirc_dev

[David Härdeman]

ir-lirc-codec is the only user of these functions, so instead of exporting them
from lirc_dev, move all of them over to ir-lirc-codec.

This means that all ir-lirc-codec fops can be found next to each other in
ir-lirc-codec.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ir-lirc-codec.c |  181 -
 drivers/media/rc/lirc_dev.c  |  187 --
 include/media/lirc_dev.h |8 --
 3 files changed, 178 insertions(+), 198 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index f914a3d5a468..05f88401f694 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -13,6 +13,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -21,6 +22,7 @@
 #include "rc-core-priv.h"
 
 #define LIRCBUF_SIZE 256
+#define LOGHEAD"lirc_dev (%s[%d]): "
 
 /**
  * ir_lirc_decode() - Send raw IR data to lirc_dev to be relayed to the
@@ -369,6 +371,177 @@ static long ir_lirc_ioctl(struct file *filep, unsigned 
int cmd,
return ret;
 }
 
+static ssize_t ir_lirc_read(struct file *file, char __user *buffer,
+   size_t length, loff_t *ppos)
+{
+   struct lirc_dev *d = file->private_data;
+   unsigned char buf[d->buf->chunk_size];
+   int ret, written = 0;
+   DECLARE_WAITQUEUE(wait, current);
+
+   dev_dbg(>dev, LOGHEAD "read called\n", d->name, d->minor);
+
+   ret = mutex_lock_interruptible(>mutex);
+   if (ret)
+   return ret;
+
+   if (!d->attached) {
+   ret = -ENODEV;
+   goto out_locked;
+   }
+
+   if (!LIRC_CAN_REC(d->features)) {
+   ret = -EINVAL;
+   goto out_locked;
+   }
+
+   if (length % d->buf->chunk_size) {
+   ret = -EINVAL;
+   goto out_locked;
+   }
+
+   /*
+* we add ourselves to the task queue before buffer check
+* to avoid losing scan code (in case when queue is awaken somewhere
+* between while condition checking and scheduling)
+*/
+   add_wait_queue(>buf->wait_poll, );
+
+   /*
+* while we didn't provide 'length' bytes, device is opened in blocking
+* mode and 'copy_to_user' is happy, wait for data.
+*/
+   while (written < length && ret == 0) {
+   if (lirc_buffer_empty(d->buf)) {
+   /* According to the read(2) man page, 'written' can be
+* returned as less than 'length', instead of blocking
+* again, returning -EWOULDBLOCK, or returning
+* -ERESTARTSYS
+*/
+   if (written)
+   break;
+   if (file->f_flags & O_NONBLOCK) {
+   ret = -EWOULDBLOCK;
+   break;
+   }
+   if (signal_pending(current)) {
+   ret = -ERESTARTSYS;
+   break;
+   }
+
+   mutex_unlock(>mutex);
+   set_current_state(TASK_INTERRUPTIBLE);
+   schedule();
+   set_current_state(TASK_RUNNING);
+
+   ret = mutex_lock_interruptible(>mutex);
+   if (ret) {
+   remove_wait_queue(>buf->wait_poll, );
+   goto out_unlocked;
+   }
+
+   if (!d->attached) {
+   ret = -ENODEV;
+   goto out_locked;
+   }
+   } else {
+   lirc_buffer_read(d->buf, buf);
+   ret = copy_to_user((void __user *)buffer+written, buf,
+  d->buf->chunk_size);
+   if (!ret)
+   written += d->buf->chunk_size;
+   else
+   ret = -EFAULT;
+   }
+   }
+
+   remove_wait_queue(>buf->wait_poll, );
+
+out_locked:
+   mutex_unlock(>mutex);
+
+out_unlocked:
+   return ret ? ret : written;
+}
+
+static unsigned int ir_lirc_poll(struct file *file, poll_table *wait)
+{
+   struct lirc_dev *d = file->private_data;
+   unsigned int ret;
+
+   if (!d->attached)
+   return POLLHUP | POLLERR;
+
+   if (d->buf) {
+   poll_wait(file, >buf->wait_poll, wait);
+
+   if (lirc_buffer_empty(d->buf))
+   ret = 0;
+   else
+   ret = POLLIN | POLLRDNORM;
+   } 

[PATCH 19/19] lirc_dev: consistent device registration printk

[David Härdeman]

This patch changes the message that is printed on lirc device registration to
make it more consistent with the input and rc subsystems.

Before:
  rc rc0: rc-core loopback device as /devices/virtual/rc/rc0
  input: rc-core loopback device as /devices/virtual/rc/rc0/input43
  lirc lirc0: lirc_dev: driver ir-lirc-codec (rc-loopback) registered at minor 
= 0

After:
  rc rc0: rc-core loopback device as /devices/virtual/rc/rc0
  input: rc-core loopback device as /devices/virtual/rc/rc0/input23
  lirc lirc0: rc-core loopback device as /devices/virtual/rc/rc0/lirc0

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

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 05f88401f694..4f33516a95a3 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -595,8 +595,7 @@ static int ir_lirc_register(struct rc_dev *dev)
if (dev->max_timeout)
features |= LIRC_CAN_SET_REC_TIMEOUT;
 
-   snprintf(ldev->name, sizeof(ldev->name), "ir-lirc-codec (%s)",
-dev->driver_name);
+   snprintf(ldev->name, sizeof(ldev->name), "%s", dev->input_name);
ldev->features = features;
ldev->data = >raw->lirc;
ldev->buf = NULL;
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index c1c917932f7e..03430a1fb192 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -105,6 +105,7 @@ int lirc_register_device(struct lirc_dev *d)
 {
int minor;
int err;
+   const char *path;
 
if (!d) {
pr_err("driver pointer must be not NULL!\n");
@@ -171,8 +172,9 @@ int lirc_register_device(struct lirc_dev *d)
return err;
}
 
-   dev_info(>dev, "lirc_dev: driver %s registered at minor = %d\n",
-d->name, d->minor);
+   path = kobject_get_path(>dev.kobj, GFP_KERNEL);
+   dev_info(>dev, "%s as %s\n", d->name, path ?: "N/A");
+   kfree(path);
 
return 0;
 }

[PATCH 17/19] ir-lirc-codec: merge lirc_dev_fop_ioctl into ir_lirc_ioctl

[David Härdeman]

ir_lirc_ioctl() is the only caller of lirc_dev_fop_ioctl() so merging the
latter into the former makes the code more readable. At the same time, this
allows the locking situation in ir_lirc_ioctl() to be fixed by holding
the lirc_dev mutex during the whole ioctl call.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ir-lirc-codec.c |  168 --
 drivers/media/rc/lirc_dev.c  |   59 -
 include/media/lirc_dev.h |1 
 3 files changed, 105 insertions(+), 123 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index ba20a4ce9cbc..f914a3d5a468 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -178,12 +178,13 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, 
const char __user *buf,
 }
 
 static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
-   unsigned long arg)
+ unsigned long arg)
 {
struct lirc_codec *lirc;
struct rc_dev *dev;
+   struct lirc_dev *d;
u32 __user *argp = (u32 __user *)(arg);
-   int ret = 0;
+   int ret;
__u32 val = 0, tmp;
 
lirc = lirc_get_pdata(filep);
@@ -194,10 +195,23 @@ static long ir_lirc_ioctl(struct file *filep, unsigned 
int cmd,
if (!dev)
return -EFAULT;
 
+   d = lirc->ldev;
+   if (!d)
+   return -EFAULT;
+
+   ret = mutex_lock_interruptible(>mutex);
+   if (ret)
+   return ret;
+
+   if (!d->attached) {
+   ret = -ENODEV;
+   goto out;
+   }
+
if (_IOC_DIR(cmd) & _IOC_WRITE) {
ret = get_user(val, argp);
if (ret)
-   return ret;
+   goto out;
}
 
switch (cmd) {
@@ -205,125 +219,153 @@ static long ir_lirc_ioctl(struct file *filep, unsigned 
int cmd,
/* legacy support */
case LIRC_GET_SEND_MODE:
if (!dev->tx_ir)
-   return -ENOTTY;
-
-   val = LIRC_MODE_PULSE;
+   ret = -ENOTTY;
+   else
+   val = LIRC_MODE_PULSE;
break;
 
case LIRC_SET_SEND_MODE:
if (!dev->tx_ir)
-   return -ENOTTY;
-
-   if (val != LIRC_MODE_PULSE)
-   return -EINVAL;
-   return 0;
+   ret = -ENOTTY;
+   else if (val != LIRC_MODE_PULSE)
+   ret = -EINVAL;
+   break;
 
/* TX settings */
case LIRC_SET_TRANSMITTER_MASK:
if (!dev->s_tx_mask)
-   return -ENOTTY;
-
-   return dev->s_tx_mask(dev, val);
+   ret = -ENOTTY;
+   else
+   ret = dev->s_tx_mask(dev, val);
+   break;
 
case LIRC_SET_SEND_CARRIER:
if (!dev->s_tx_carrier)
-   return -ENOTTY;
-
-   return dev->s_tx_carrier(dev, val);
+   ret = -ENOTTY;
+   else
+   ret = dev->s_tx_carrier(dev, val);
+   break;
 
case LIRC_SET_SEND_DUTY_CYCLE:
if (!dev->s_tx_duty_cycle)
-   return -ENOTTY;
-
-   if (val <= 0 || val >= 100)
-   return -EINVAL;
-
-   return dev->s_tx_duty_cycle(dev, val);
+   ret = -ENOTTY;
+   else if (val <= 0 || val >= 100)
+   ret = -EINVAL;
+   else
+   ret = dev->s_tx_duty_cycle(dev, val);
+   break;
 
/* RX settings */
case LIRC_SET_REC_CARRIER:
if (!dev->s_rx_carrier_range)
-   return -ENOTTY;
-
-   if (val <= 0)
-   return -EINVAL;
-
-   return dev->s_rx_carrier_range(dev,
-  dev->raw->lirc.carrier_low,
-  val);
+   ret = -ENOTTY;
+   else if (val <= 0)
+   ret = -EINVAL;
+   else
+   ret = dev->s_rx_carrier_range(dev,
+ 
dev->raw->lirc.carrier_low,
+ val);
+   break;
 
case LIRC_SET_REC_CARRIER_RANGE:
if (!dev->s_rx_carrier_range)
-   return -ENOTTY;
-
-   if (val <= 0)
-   return -EINVAL;
-
-   dev->raw->lirc.carrier_low = val;
-   return 0;
+   ret = -ENOTTY;
+   els

[PATCH 16/19] lirc_dev: merge struct irctl into struct lirc_dev

[David Härdeman]

The use of two separate structs (lirc_dev aka lirc_driver and irctl) makes
it much harder to follow the proper lifetime of the various structs and
necessitates hacks such as keeping a copy of struct lirc_dev inside
struct irctl.

Merging the two structs means that lirc_dev can properly manage the lifetime
of the resulting struct and simplifies the code at the same time.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ir-lirc-codec.c|   15 +-
 drivers/media/rc/lirc_dev.c |  288 +--
 drivers/staging/media/lirc/lirc_zilog.c |   20 +-
 include/media/lirc_dev.h|   26 ++-
 4 files changed, 161 insertions(+), 188 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index f276c4f56fb5..ba20a4ce9cbc 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -35,7 +35,7 @@ static int ir_lirc_decode(struct rc_dev *dev, struct 
ir_raw_event ev)
struct lirc_codec *lirc = >raw->lirc;
int sample;
 
-   if (!dev->raw->lirc.ldev || !dev->raw->lirc.ldev->rbuf)
+   if (!dev->raw->lirc.ldev || !dev->raw->lirc.ldev->buf)
return -EINVAL;
 
/* Packet start */
@@ -84,7 +84,7 @@ static int ir_lirc_decode(struct rc_dev *dev, struct 
ir_raw_event ev)
(u64)LIRC_VALUE_MASK);
 
gap_sample = LIRC_SPACE(lirc->gap_duration);
-   lirc_buffer_write(dev->raw->lirc.ldev->rbuf,
+   lirc_buffer_write(dev->raw->lirc.ldev->buf,
(unsigned char *) _sample);
lirc->gap = false;
}
@@ -95,9 +95,9 @@ static int ir_lirc_decode(struct rc_dev *dev, struct 
ir_raw_event ev)
   TO_US(ev.duration), TO_STR(ev.pulse));
}
 
-   lirc_buffer_write(dev->raw->lirc.ldev->rbuf,
+   lirc_buffer_write(dev->raw->lirc.ldev->buf,
  (unsigned char *) );
-   wake_up(>raw->lirc.ldev->rbuf->wait_poll);
+   wake_up(>raw->lirc.ldev->buf->wait_poll);
 
return 0;
 }
@@ -384,12 +384,12 @@ static int ir_lirc_register(struct rc_dev *dev)
 dev->driver_name);
ldev->features = features;
ldev->data = >raw->lirc;
-   ldev->rbuf = NULL;
+   ldev->buf = NULL;
ldev->code_length = sizeof(struct ir_raw_event) * 8;
ldev->chunk_size = sizeof(int);
ldev->buffer_size = LIRCBUF_SIZE;
ldev->fops = _fops;
-   ldev->dev = >dev;
+   ldev->dev.parent = >dev;
ldev->rdev = dev;
ldev->owner = THIS_MODULE;
 
@@ -402,7 +402,7 @@ static int ir_lirc_register(struct rc_dev *dev)
return 0;
 
 out:
-   kfree(ldev);
+   lirc_free_device(ldev);
return rc;
 }
 
@@ -411,7 +411,6 @@ static int ir_lirc_unregister(struct rc_dev *dev)
struct lirc_codec *lirc = >raw->lirc;
 
lirc_unregister_device(lirc->ldev);
-   kfree(lirc->ldev);
lirc->ldev = NULL;
 
return 0;
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 80944c2f7e91..4ad08d3d4e2f 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -34,19 +34,6 @@
 
 static dev_t lirc_base_dev;
 
-struct irctl {
-   struct lirc_dev d;
-   bool attached;
-   int open;
-
-   struct mutex mutex;
-   struct lirc_buffer *buf;
-   bool buf_internal;
-
-   struct device dev;
-   struct cdev cdev;
-};
-
 /* Used to keep track of allocated lirc devices */
 #define LIRC_MAX_DEVICES 256
 static DEFINE_IDA(lirc_ida);
@@ -54,69 +41,72 @@ static DEFINE_IDA(lirc_ida);
 /* Only used for sysfs but defined to void otherwise */
 static struct class *lirc_class;
 
-static void lirc_free_buffer(struct irctl *ir)
+static void lirc_release_device(struct device *ld)
 {
-   if (ir->buf_internal) {
-   lirc_buffer_free(ir->buf);
-   kfree(ir->buf);
-   ir->buf = NULL;
+   struct lirc_dev *d = container_of(ld, struct lirc_dev, dev);
+
+   if (d->buf_internal) {
+   lirc_buffer_free(d->buf);
+   kfree(d->buf);
+   d->buf = NULL;
}
+   kfree(d);
+   module_put(THIS_MODULE);
 }
 
-static void lirc_release(struct device *ld)
+static int lirc_allocate_buffer(struct lirc_dev *d)
 {
-   struct irctl *ir = container_of(ld, struct irctl, dev);
-
-   lirc_free_buffer(ir);
-   kfree(ir);
-}
+   int err;
 
-static int lirc_allocate_buffer(struct irctl *ir)
-{
-   int err = 0;
-   struct lirc_dev *d = >d;
-
-   if (d->rbuf) {
-   ir->buf = d->

[PATCH 15/19] lirc_zilog: use a dynamically allocated lirc_dev

[David Härdeman]

lirc_zilog currently embeds a struct lirc_dev in its own struct IR, but
subsequent patches will make the lifetime of struct lirc_dev dynamic (i.e.
it will be free():d once lirc_dev is sure there are no users of the struct).

Therefore, change lirc_zilog to use a pointer to a dynamically allocated
struct lirc_dev.

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

diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index 51512ba7f5b8..a25ae574 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -102,8 +102,8 @@ struct IR {
struct kref ref;
struct list_head list;
 
-   /* FIXME spinlock access to l.features */
-   struct lirc_dev l;
+   /* FIXME spinlock access to l->features */
+   struct lirc_dev *l;
struct lirc_buffer rbuf;
 
struct mutex ir_lock;
@@ -187,7 +187,10 @@ 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
 */
-   lirc_unregister_device(>l);
+   if (ir->l) {
+   lirc_unregister_device(ir->l);
+   lirc_free_device(ir->l);
+   }
 
if (kfifo_initialized(>rbuf.fifo))
lirc_buffer_free(>rbuf);
@@ -244,7 +247,7 @@ static void release_ir_rx(struct kref *ref)
 * and releasing the ir reference can cause a sleep.  That work is
 * performed by put_ir_rx()
 */
-   ir->l.features &= ~LIRC_CAN_REC_LIRCCODE;
+   ir->l->features &= ~LIRC_CAN_REC_LIRCCODE;
/* Don't put_ir_device(rx->ir) here; lock can't be freed yet */
ir->rx = NULL;
/* Don't do the kfree(rx) here; we still need to kill the poll thread */
@@ -289,7 +292,7 @@ static void release_ir_tx(struct kref *ref)
struct IR_tx *tx = container_of(ref, struct IR_tx, ref);
struct IR *ir = tx->ir;
 
-   ir->l.features &= ~LIRC_CAN_SEND_PULSE;
+   ir->l->features &= ~LIRC_CAN_SEND_PULSE;
/* Don't put_ir_device(tx->ir) here, so our lock doesn't get freed */
ir->tx = NULL;
kfree(tx);
@@ -318,7 +321,7 @@ static int add_to_buf(struct IR *ir)
int ret;
int failures = 0;
unsigned char sendbuf[1] = { 0 };
-   struct lirc_buffer *rbuf = ir->l.rbuf;
+   struct lirc_buffer *rbuf = ir->l->rbuf;
struct IR_rx *rx;
struct IR_tx *tx;
 
@@ -464,7 +467,7 @@ static int add_to_buf(struct IR *ir)
 static int lirc_thread(void *arg)
 {
struct IR *ir = arg;
-   struct lirc_buffer *rbuf = ir->l.rbuf;
+   struct lirc_buffer *rbuf = ir->l->rbuf;
 
dev_dbg(ir->dev, "poll thread started\n");
 
@@ -885,7 +888,7 @@ static ssize_t read(struct file *filep, char __user 
*outbuf, size_t n,
 {
struct IR *ir = lirc_get_pdata(filep);
struct IR_rx *rx;
-   struct lirc_buffer *rbuf = ir->l.rbuf;
+   struct lirc_buffer *rbuf = ir->l->rbuf;
int ret = 0, written = 0, retries = 0;
unsigned int m;
DECLARE_WAITQUEUE(wait, current);
@@ -1203,7 +1206,7 @@ static unsigned int poll(struct file *filep, poll_table 
*wait)
 {
struct IR *ir = lirc_get_pdata(filep);
struct IR_rx *rx;
-   struct lirc_buffer *rbuf = ir->l.rbuf;
+   struct lirc_buffer *rbuf = ir->l->rbuf;
unsigned int ret;
 
dev_dbg(ir->dev, "%s called\n", __func__);
@@ -1239,7 +1242,7 @@ static long ioctl(struct file *filep, unsigned int cmd, 
unsigned long arg)
int result;
unsigned long mode, features;
 
-   features = ir->l.features;
+   features = ir->l->features;
 
switch (cmd) {
case LIRC_GET_LENGTH:
@@ -1349,13 +1352,6 @@ static const struct file_operations lirc_fops = {
.release= close
 };
 
-static struct lirc_dev lirc_template = {
-   .name   = "lirc_zilog",
-   .code_length= 13,
-   .fops   = _fops,
-   .owner  = THIS_MODULE,
-};
-
 static int ir_remove(struct i2c_client *client)
 {
if (strncmp("ir_tx_z8", client->name, 8) == 0) {
@@ -1446,22 +1442,35 @@ static int ir_probe(struct i2c_client *client, const 
struct i2c_device_id *id)
spin_lock_init(>rx_ref_lock);
 
/* set lirc_dev stuff */
-   memcpy(>l, _template, sizeof(struct lirc_dev));
+   ir->l = lirc_allocate_device();
+   if (!ir->l) {
+   ret = -ENOMEM;
+   goto out_put_ir;
+   }
+
+   snprintf(ir->l->name, sizeof(ir->l->name), &qu

[PATCH 12/19] lirc_dev: introduce lirc_allocate_device and lirc_free_device

[David Härdeman]

Introduce two new functions so that the API for lirc_dev matches that
of the rc-core and input subsystems.

This means that lirc_dev structs are managed using the usual four functions:
lirc_allocate_device
lirc_free_device
lirc_register_device
lirc_unregister_device

The functions are pretty simplistic at this point, later patches will put
more flesh on the bones of both.

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

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 2349630ed383..f276c4f56fb5 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -347,7 +347,7 @@ static int ir_lirc_register(struct rc_dev *dev)
int rc = -ENOMEM;
unsigned long features = 0;
 
-   ldev = kzalloc(sizeof(struct lirc_dev), GFP_KERNEL);
+   ldev = lirc_allocate_device();
if (!ldev)
return rc;
 
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 1c2f1a07bdaa..d107ed6b634b 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -101,6 +101,19 @@ static int lirc_allocate_buffer(struct irctl *ir)
return err;
 }
 
+struct lirc_dev *
+lirc_allocate_device(void)
+{
+   return kzalloc(sizeof(struct lirc_dev), GFP_KERNEL);
+}
+EXPORT_SYMBOL(lirc_allocate_device);
+
+void lirc_free_device(struct lirc_dev *d)
+{
+   kfree(d);
+}
+EXPORT_SYMBOL(lirc_free_device);
+
 int lirc_register_device(struct lirc_dev *d)
 {
struct irctl *ir;
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 567959e9524f..3f8edabfef88 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -150,11 +150,10 @@ struct lirc_dev {
struct irctl *irctl;
 };
 
-/* following functions can be called ONLY from user context
- *
- * returns negative value on error or zero
- * contents of the structure pointed by p is copied
- */
+extern struct lirc_dev *lirc_allocate_device(void);
+
+extern void lirc_free_device(struct lirc_dev *d);
+
 extern int lirc_register_device(struct lirc_dev *d);
 
 extern void lirc_unregister_device(struct lirc_dev *d);

[PATCH 14/19] lirc_zilog: add a pointer to the parent device to struct IR

[David Härdeman]

lirc_zilog stashes a pointer to the parent device in struct lirc_dev and uses
it for logging. It makes more sense to let lirc_zilog keep track of that
pointer in its own struct (this is in preparation for subsequent patches
which will remodel struct lirc_dev).

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

diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index f54b66de4a27..51512ba7f5b8 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -109,6 +109,7 @@ struct IR {
struct mutex ir_lock;
atomic_t open_count;
 
+   struct device *dev;
struct i2c_adapter *adapter;
 
spinlock_t rx_ref_lock; /* struct IR_rx kref get()/put() */
@@ -322,7 +323,7 @@ static int add_to_buf(struct IR *ir)
struct IR_tx *tx;
 
if (lirc_buffer_full(rbuf)) {
-   dev_dbg(ir->l.dev, "buffer overflow\n");
+   dev_dbg(ir->dev, "buffer overflow\n");
return -EOVERFLOW;
}
 
@@ -368,17 +369,17 @@ static int add_to_buf(struct IR *ir)
 */
ret = i2c_master_send(rx->c, sendbuf, 1);
if (ret != 1) {
-   dev_err(ir->l.dev, "i2c_master_send failed with %d\n",
+   dev_err(ir->dev, "i2c_master_send failed with %d\n",
ret);
if (failures >= 3) {
mutex_unlock(>ir_lock);
-   dev_err(ir->l.dev,
+   dev_err(ir->dev,
"unable to read from the IR chip after 
3 resets, giving up\n");
break;
}
 
/* Looks like the chip crashed, reset it */
-   dev_err(ir->l.dev,
+   dev_err(ir->dev,
"polling the IR receiver chip failed, trying 
reset\n");
 
set_current_state(TASK_UNINTERRUPTIBLE);
@@ -405,14 +406,14 @@ static int add_to_buf(struct IR *ir)
ret = i2c_master_recv(rx->c, keybuf, sizeof(keybuf));
mutex_unlock(>ir_lock);
if (ret != sizeof(keybuf)) {
-   dev_err(ir->l.dev,
+   dev_err(ir->dev,
"i2c_master_recv failed with %d -- keeping last 
read buffer\n",
ret);
} else {
rx->b[0] = keybuf[3];
rx->b[1] = keybuf[4];
rx->b[2] = keybuf[5];
-   dev_dbg(ir->l.dev,
+   dev_dbg(ir->dev,
"key (0x%02x/0x%02x)\n",
rx->b[0], rx->b[1]);
}
@@ -465,7 +466,7 @@ static int lirc_thread(void *arg)
struct IR *ir = arg;
struct lirc_buffer *rbuf = ir->l.rbuf;
 
-   dev_dbg(ir->l.dev, "poll thread started\n");
+   dev_dbg(ir->dev, "poll thread started\n");
 
while (!kthread_should_stop()) {
set_current_state(TASK_INTERRUPTIBLE);
@@ -493,7 +494,7 @@ static int lirc_thread(void *arg)
wake_up_interruptible(>wait_poll);
}
 
-   dev_dbg(ir->l.dev, "poll thread ended\n");
+   dev_dbg(ir->dev, "poll thread ended\n");
return 0;
 }
 
@@ -646,10 +647,10 @@ static int send_data_block(struct IR_tx *tx, unsigned 
char *data_block)
buf[0] = (unsigned char)(i + 1);
for (j = 0; j < tosend; ++j)
buf[1 + j] = data_block[i + j];
-   dev_dbg(tx->ir->l.dev, "%*ph", 5, buf);
+   dev_dbg(tx->ir->dev, "%*ph", 5, buf);
ret = i2c_master_send(tx->c, buf, tosend + 1);
if (ret != tosend + 1) {
-   dev_err(tx->ir->l.dev,
+   dev_err(tx->ir->dev,
"i2c_master_send failed with %d\n", ret);
return ret < 0 ? ret : -EFAULT;
}
@@ -674,7 +675,7 @@ static int send_boot_data(struct IR_tx *tx)
buf[1] = 0x20;
ret = i2c_master_send(tx->c, buf, 2);
if (ret != 2) {
-   dev_err(tx->ir->l.dev, "i2c_master_send failed with %d\n", ret);
+   dev_err(tx->ir->dev, "i2c_master_send failed with %d\n", ret);
return ret < 0 ? ret : -EFAULT;

[PATCH 13/19] lirc_dev: remove the BUFLEN define

[David Härdeman]

The define is only used in the lirc_zilog driver and once in lirc_dev.

In lirc_dev it rather serves to make the limits on d->code_length less clear,
so move the define to lirc_zilog.

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index d107ed6b634b..80944c2f7e91 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -145,9 +145,8 @@ int lirc_register_device(struct lirc_dev *d)
return -EINVAL;
}
 
-   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);
+   if (d->code_length < 1 || d->code_length > 128) {
+   dev_err(d->dev, "invalid code_length!\n");
return -EBADRQC;
}
 
diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index a8aefd033ad9..f54b66de4a27 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -64,6 +64,9 @@
 /* Max transfer size done by I2C transfer functions */
 #define MAX_XFER_SIZE  64
 
+/* LIRC buffer size */
+#define BUFLEN16
+
 struct IR;
 
 struct IR_rx {
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 3f8edabfef88..21aac9494678 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -9,8 +9,6 @@
 #ifndef _LINUX_LIRC_DEV_H
 #define _LINUX_LIRC_DEV_H
 
-#define BUFLEN16
-
 #include 
 #include 
 #include 

[PATCH 10/19] lirc_dev: use an IDA instead of an array to keep track of registered devices

[David Härdeman]

Using the kernel-provided IDA simplifies the code and makes it possible
to remove the lirc_dev_lock mutex.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |   36 
 include/media/lirc_dev.h|1 -
 2 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 6bd21609a719..996cb5e778dc 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -46,10 +47,9 @@ struct irctl {
struct cdev cdev;
 };
 
-/* This mutex protects the irctls array */
-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;
@@ -67,9 +67,6 @@ static void lirc_release(struct device *ld)
 {
struct irctl *ir = container_of(ld, struct irctl, dev);
 
-   mutex_lock(_dev_lock);
-   irctls[ir->d.minor] = NULL;
-   mutex_unlock(_dev_lock);
lirc_free_buffer(ir);
kfree(ir);
 }
@@ -107,7 +104,7 @@ static int lirc_allocate_buffer(struct irctl *ir)
 int lirc_register_driver(struct lirc_driver *d)
 {
struct irctl *ir;
-   unsigned minor;
+   int minor;
int err;
 
if (!d) {
@@ -169,27 +166,16 @@ int lirc_register_driver(struct lirc_driver *d)
d->rbuf = ir->buf;
}
 
-   mutex_lock(_dev_lock);
-
-   /* 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");
-   mutex_unlock(_dev_lock);
+   minor = ida_simple_get(_ida, 0, LIRC_MAX_DEVICES, GFP_KERNEL);
+   if (minor < 0) {
lirc_free_buffer(ir);
kfree(ir);
-   return -ENOMEM;
+   return minor;
}
 
-   irctls[minor] = ir;
d->irctl = ir;
d->minor = minor;
 
-   mutex_unlock(_dev_lock);
-
device_initialize(>dev);
ir->dev.devt = MKDEV(MAJOR(lirc_base_dev), ir->d.minor);
ir->dev.class = lirc_class;
@@ -203,6 +189,7 @@ int lirc_register_driver(struct lirc_driver *d)
 
err = cdev_device_add(>cdev, >dev);
if (err) {
+   ida_simple_remove(_ida, minor);
put_device(>dev);
return err;
}
@@ -239,6 +226,7 @@ void lirc_unregister_driver(struct lirc_driver *d)
 
mutex_unlock(>mutex);
 
+   ida_simple_remove(_ida, d->minor);
put_device(>dev);
 }
 EXPORT_SYMBOL(lirc_unregister_driver);
@@ -509,7 +497,7 @@ static int __init lirc_dev_init(void)
return PTR_ERR(lirc_class);
}
 
-   retval = alloc_chrdev_region(_base_dev, 0, MAX_IRCTL_DEVICES,
+   retval = alloc_chrdev_region(_base_dev, 0, LIRC_MAX_DEVICES,
 "BaseRemoteCtl");
if (retval) {
class_destroy(lirc_class);
@@ -526,7 +514,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/include/media/lirc_dev.h b/include/media/lirc_dev.h
index a01fe5433bb7..2629c40e8a39 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 BUFLEN16
 
 #include 

[PATCH 11/19] lirc_dev: rename struct lirc_driver to struct lirc_dev

[David Härdeman]

This is in preparation for the later patches which do away with
struct irctl entirely.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ir-lirc-codec.c|   50 ---
 drivers/media/rc/lirc_dev.c |   12 ---
 drivers/media/rc/rc-core-priv.h |2 +
 drivers/staging/media/lirc/lirc_zilog.c |   12 ---
 include/media/lirc_dev.h|   46 -
 5 files changed, 51 insertions(+), 71 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 2c1221a61ea1..2349630ed383 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -35,7 +35,7 @@ static int ir_lirc_decode(struct rc_dev *dev, struct 
ir_raw_event ev)
struct lirc_codec *lirc = >raw->lirc;
int sample;
 
-   if (!dev->raw->lirc.drv || !dev->raw->lirc.drv->rbuf)
+   if (!dev->raw->lirc.ldev || !dev->raw->lirc.ldev->rbuf)
return -EINVAL;
 
/* Packet start */
@@ -84,7 +84,7 @@ static int ir_lirc_decode(struct rc_dev *dev, struct 
ir_raw_event ev)
(u64)LIRC_VALUE_MASK);
 
gap_sample = LIRC_SPACE(lirc->gap_duration);
-   lirc_buffer_write(dev->raw->lirc.drv->rbuf,
+   lirc_buffer_write(dev->raw->lirc.ldev->rbuf,
(unsigned char *) _sample);
lirc->gap = false;
}
@@ -95,9 +95,9 @@ static int ir_lirc_decode(struct rc_dev *dev, struct 
ir_raw_event ev)
   TO_US(ev.duration), TO_STR(ev.pulse));
}
 
-   lirc_buffer_write(dev->raw->lirc.drv->rbuf,
+   lirc_buffer_write(dev->raw->lirc.ldev->rbuf,
  (unsigned char *) );
-   wake_up(>raw->lirc.drv->rbuf->wait_poll);
+   wake_up(>raw->lirc.ldev->rbuf->wait_poll);
 
return 0;
 }
@@ -343,12 +343,12 @@ static const struct file_operations lirc_fops = {
 
 static int ir_lirc_register(struct rc_dev *dev)
 {
-   struct lirc_driver *drv;
+   struct lirc_dev *ldev;
int rc = -ENOMEM;
unsigned long features = 0;
 
-   drv = kzalloc(sizeof(struct lirc_driver), GFP_KERNEL);
-   if (!drv)
+   ldev = kzalloc(sizeof(struct lirc_dev), GFP_KERNEL);
+   if (!ldev)
return rc;
 
if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
@@ -380,29 +380,29 @@ 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)",
+   snprintf(ldev->name, sizeof(ldev->name), "ir-lirc-codec (%s)",
 dev->driver_name);
-   drv->features = features;
-   drv->data = >raw->lirc;
-   drv->rbuf = NULL;
-   drv->code_length = sizeof(struct ir_raw_event) * 8;
-   drv->chunk_size = sizeof(int);
-   drv->buffer_size = LIRCBUF_SIZE;
-   drv->fops = _fops;
-   drv->dev = >dev;
-   drv->rdev = dev;
-   drv->owner = THIS_MODULE;
-
-   rc = lirc_register_driver(drv);
+   ldev->features = features;
+   ldev->data = >raw->lirc;
+   ldev->rbuf = NULL;
+   ldev->code_length = sizeof(struct ir_raw_event) * 8;
+   ldev->chunk_size = sizeof(int);
+   ldev->buffer_size = LIRCBUF_SIZE;
+   ldev->fops = _fops;
+   ldev->dev = >dev;
+   ldev->rdev = dev;
+   ldev->owner = THIS_MODULE;
+
+   rc = lirc_register_device(ldev);
if (rc < 0)
goto out;
 
-   dev->raw->lirc.drv = drv;
+   dev->raw->lirc.ldev = ldev;
dev->raw->lirc.dev = dev;
return 0;
 
 out:
-   kfree(drv);
+   kfree(ldev);
return rc;
 }
 
@@ -410,9 +410,9 @@ static int ir_lirc_unregister(struct rc_dev *dev)
 {
struct lirc_codec *lirc = >raw->lirc;
 
-   lirc_unregister_driver(lirc->drv);
-   kfree(lirc->drv);
-   lirc->drv = NULL;
+   lirc_unregister_device(lirc->ldev);
+   kfree(lirc->ldev);
+   lirc->ldev = NULL;
 
return 0;
 }
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 996cb5e778dc..1c2f1a07bdaa 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -35,7 +35,7 @@
 static dev_t lirc_base_dev;
 
 struct irctl {
-   struct lirc_driver d;
+   struct lirc_dev d;
bool attached;
int open;
 
@@ -74,7 +74,7 @@ static void lirc_release(struct device *ld)
 static int lirc_allocate_buffer(struct irctl *ir)
 {
int err = 0;
-   struct lirc_driver *d = &

[PATCH 09/19] lirc_dev: sanitize locking

[David Härdeman]

Use the irctl mutex for all device operations and only use lirc_dev_lock to
protect the irctls array. Also, make sure that the device is alive early in
each fops function before doing anything else.

Since this patch touches nearly every line where the irctl mutex is
taken/released, it also renames the mutex at the same time (the name
irctl_lock will be misleading once struct irctl goes away in later
patches).

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index aece6b619796..6bd21609a719 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -38,7 +38,7 @@ struct irctl {
bool attached;
int open;
 
-   struct mutex irctl_lock;
+   struct mutex mutex;
struct lirc_buffer *buf;
bool buf_internal;
 
@@ -46,6 +46,7 @@ struct irctl {
struct cdev cdev;
 };
 
+/* This mutex protects the irctls array */
 static DEFINE_MUTEX(lirc_dev_lock);
 
 static struct irctl *irctls[MAX_IRCTL_DEVICES];
@@ -53,18 +54,23 @@ static struct irctl *irctls[MAX_IRCTL_DEVICES];
 /* Only used for sysfs but defined to void otherwise */
 static struct class *lirc_class;
 
-static void lirc_release(struct device *ld)
+static void lirc_free_buffer(struct irctl *ir)
 {
-   struct irctl *ir = container_of(ld, struct irctl, dev);
-
if (ir->buf_internal) {
lirc_buffer_free(ir->buf);
kfree(ir->buf);
+   ir->buf = NULL;
}
+}
+
+static void lirc_release(struct device *ld)
+{
+   struct irctl *ir = container_of(ld, struct irctl, dev);
 
mutex_lock(_dev_lock);
irctls[ir->d.minor] = NULL;
mutex_unlock(_dev_lock);
+   lirc_free_buffer(ir);
kfree(ir);
 }
 
@@ -141,6 +147,28 @@ int lirc_register_driver(struct lirc_driver *d)
return -EBADRQC;
}
 
+   /* some safety check 8-) */
+   d->name[sizeof(d->name)-1] = '\0';
+
+   if (d->features == 0)
+   d->features = LIRC_CAN_REC_LIRCCODE;
+
+   ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
+   if (!ir)
+   return -ENOMEM;
+
+   mutex_init(>mutex);
+   ir->d = *d;
+
+   if (LIRC_CAN_REC(d->features)) {
+   err = lirc_allocate_buffer(ir);
+   if (err) {
+   kfree(ir);
+   return err;
+   }
+   d->rbuf = ir->buf;
+   }
+
mutex_lock(_dev_lock);
 
/* find first free slot for driver */
@@ -150,37 +178,17 @@ int lirc_register_driver(struct lirc_driver *d)
 
if (minor == MAX_IRCTL_DEVICES) {
dev_err(d->dev, "no free slots for drivers!\n");
-   err = -ENOMEM;
-   goto out_lock;
-   }
-
-   ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
-   if (!ir) {
-   err = -ENOMEM;
-   goto out_lock;
+   mutex_unlock(_dev_lock);
+   lirc_free_buffer(ir);
+   kfree(ir);
+   return -ENOMEM;
}
 
-   mutex_init(>irctl_lock);
irctls[minor] = ir;
d->irctl = 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;
-
-   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;
-   }
+   mutex_unlock(_dev_lock);
 
device_initialize(>dev);
ir->dev.devt = MKDEV(MAJOR(lirc_base_dev), ir->d.minor);
@@ -194,22 +202,15 @@ int lirc_register_driver(struct lirc_driver *d)
ir->attached = true;
 
err = cdev_device_add(>cdev, >dev);
-   if (err)
-   goto out_dev;
-
-   mutex_unlock(_dev_lock);
+   if (err) {
+   put_device(>dev);
+   return err;
+   }
 
dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
 ir->d.name, ir->d.minor);
 
return 0;
-
-out_dev:
-   put_device(>dev);
-out_lock:
-   mutex_unlock(_dev_lock);
-
-   return err;
 }
 EXPORT_SYMBOL(lirc_register_driver);
 
@@ -222,11 +223,13 @@ void lirc_unregister_driver(struct lirc_driver *d)
 
ir = d->irctl;
 
-   mutex_lock(_dev_lock);
-
dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
d->name, d->minor);
 
+   cdev_device_del(>cdev, >dev);
+
+  

[PATCH 08/19] lirc_dev: change irctl->attached to be a boolean

[David Härdeman]

The "attached" member of struct irctl is a boolean value, so let the code
reflect that.

Signed-off-by: David Härdeman <da...@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 92048d945ba7..aece6b619796 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -35,7 +35,7 @@ static dev_t lirc_base_dev;
 
 struct irctl {
struct lirc_driver d;
-   int attached;
+   bool attached;
int open;
 
struct mutex irctl_lock;
@@ -191,7 +191,7 @@ int lirc_register_driver(struct lirc_driver *d)
 
cdev_init(>cdev, d->fops);
ir->cdev.owner = ir->d.owner;
-   ir->attached = 1;
+   ir->attached = true;
 
err = cdev_device_add(>cdev, >dev);
if (err)
@@ -227,7 +227,7 @@ void lirc_unregister_driver(struct lirc_driver *d)
dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
d->name, d->minor);
 
-   ir->attached = 0;
+   ir->attached = false;
if (ir->open) {
dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
d->name, d->minor);

[PATCH 05/19] lirc_dev: make better use of file->private_data

[David Härdeman]

By making better use of file->private_data in lirc_dev we can avoid
digging around in the irctls[] array, thereby simplifying the code.

External drivers need to use lirc_get_pdata() instead of mucking around
in file->private_data.

The newly introduced lirc_init_pdata() function isn't very elegant, but
it's a stopgap measure which can be removed once lirc_zilog is converted
to rc-core.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |   70 +--
 drivers/staging/media/lirc/lirc_zilog.c |   53 ---
 include/media/lirc_dev.h|3 +
 3 files changed, 33 insertions(+), 93 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 61ed90a975ad..2de840dd829d 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -243,36 +243,18 @@ 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(_dev_lock))
-   return -ERESTARTSYS;
-
-   ir = irctls[iminor(inode)];
-   mutex_unlock(_dev_lock);
-
-   if (!ir) {
-   retval = -ENODEV;
-   goto error;
-   }
-
dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
 
-   if (ir->open) {
-   retval = -EBUSY;
-   goto error;
-   }
+   if (ir->open)
+   return -EBUSY;
 
if (ir->d.rdev) {
retval = rc_open(ir->d.rdev);
if (retval)
-   goto error;
+   return retval;
}
 
if (ir->buf)
@@ -280,25 +262,18 @@ int lirc_dev_fop_open(struct inode *inode, struct file 
*file)
 
ir->open++;
 
+   lirc_init_pdata(inode, file);
nonseekable_open(inode, file);
 
return 0;
-
-error:
-   return retval;
 }
 EXPORT_SYMBOL(lirc_dev_fop_open);
 
 int lirc_dev_fop_close(struct inode *inode, struct file *file)
 {
-   struct irctl *ir = irctls[iminor(inode)];
+   struct irctl *ir = file->private_data;
int ret;
 
-   if (!ir) {
-   pr_err("called with invalid irctl\n");
-   return -EINVAL;
-   }
-
ret = mutex_lock_killable(_dev_lock);
WARN_ON(ret);
 
@@ -314,14 +289,9 @@ 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)
return POLLHUP | POLLERR;
 
@@ -344,14 +314,9 @@ 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);
@@ -410,16 +375,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;
 
@@ -510,9 +470,19 @@ ssize_t lirc_dev_fop_read(struct file *file,
 }
 EXPORT_SYMBOL(lirc_dev_fop_read);
 
+void lirc_init_pdata(struct inode *inode, struct file *file)
+{
+   struct irctl *ir = container_of(inode->i_cdev, struct irctl, cdev);
+
+   file->private_data = ir;
+}
+EXPORT_SYMBOL(lirc_init_pdata);
+
 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);
 
diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index 10594aea2a5c..6d4c5a957ab4 100644

[PATCH 04/19] lirc_dev: use cdev_device_add() helper function

[David Härdeman]

Replace calls to cdev_add() and device_add() with the cdev_device_add()
helper function.

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 591dee9f6ba2..61ed90a975ad 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -191,17 +191,11 @@ int lirc_register_driver(struct lirc_driver *d)
 
cdev_init(>cdev, d->fops);
ir->cdev.owner = ir->d.owner;
-   ir->cdev.kobj.parent = >dev.kobj;
-
-   err = cdev_add(>cdev, ir->dev.devt, 1);
-   if (err)
-   goto out_free_dev;
-
ir->attached = 1;
 
-   err = device_add(>dev);
+   err = cdev_device_add(>cdev, >dev);
if (err)
-   goto out_cdev;
+   goto out_dev;
 
mutex_unlock(_dev_lock);
 
@@ -210,9 +204,7 @@ int lirc_register_driver(struct lirc_driver *d)
 
return 0;
 
-out_cdev:
-   cdev_del(>cdev);
-out_free_dev:
+out_dev:
put_device(>dev);
 out_lock:
mutex_unlock(_dev_lock);
@@ -244,8 +236,7 @@ void lirc_unregister_driver(struct lirc_driver *d)
 
mutex_unlock(_dev_lock);
 
-   device_del(>dev);
-   cdev_del(>cdev);
+   cdev_device_del(>cdev, >dev);
put_device(>dev);
 }
 EXPORT_SYMBOL(lirc_unregister_driver);

[PATCH 06/19] lirc_dev: make chunk_size and buffer_size mandatory

[David Härdeman]

Make setting chunk_size and buffer_size mandatory for drivers which
expect lirc_dev to allocate the lirc_buffer (i.e. ir-lirc-codec) and
don't set them in lirc-zilog (which creates its own buffer).

Also remove an unnecessary copy of chunk_size in struct irctl (the
same information is already available from struct lirc_buffer).

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |   26 +-
 drivers/staging/media/lirc/lirc_zilog.c |5 +
 include/media/lirc_dev.h|9 +
 3 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 2de840dd829d..1773a2934484 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -41,7 +41,6 @@ struct irctl {
struct mutex irctl_lock;
struct lirc_buffer *buf;
bool buf_internal;
-   unsigned int chunk_size;
 
struct device dev;
struct cdev cdev;
@@ -72,16 +71,8 @@ static void lirc_release(struct device *ld)
 static int lirc_allocate_buffer(struct irctl *ir)
 {
int err = 0;
-   int bytes_in_key;
-   unsigned int chunk_size;
-   unsigned int buffer_size;
struct lirc_driver *d = >d;
 
-   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;
-   chunk_size  = d->chunk_size  ? d->chunk_size  : bytes_in_key;
-
if (d->rbuf) {
ir->buf = d->rbuf;
ir->buf_internal = false;
@@ -92,7 +83,7 @@ static int lirc_allocate_buffer(struct irctl *ir)
goto out;
}
 
-   err = lirc_buffer_init(ir->buf, chunk_size, buffer_size);
+   err = lirc_buffer_init(ir->buf, d->chunk_size, d->buffer_size);
if (err) {
kfree(ir->buf);
ir->buf = NULL;
@@ -102,7 +93,6 @@ static int lirc_allocate_buffer(struct irctl *ir)
ir->buf_internal = true;
d->rbuf = ir->buf;
}
-   ir->chunk_size = ir->buf->chunk_size;
 
 out:
return err;
@@ -129,6 +119,16 @@ int lirc_register_driver(struct lirc_driver *d)
return -EINVAL;
}
 
+   if (!d->rbuf && d->chunk_size < 1) {
+   pr_err("chunk_size must be set!\n");
+   return -EINVAL;
+   }
+
+   if (!d->rbuf && d->buffer_size < 1) {
+   pr_err("buffer_size must be set!\n");
+   return -EINVAL;
+   }
+
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);
@@ -385,7 +385,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
 
dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
 
-   buf = kzalloc(ir->chunk_size, GFP_KERNEL);
+   buf = kzalloc(ir->buf->chunk_size, GFP_KERNEL);
if (!buf)
return -ENOMEM;
 
@@ -398,7 +398,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
goto out_locked;
}
 
-   if (length % ir->chunk_size) {
+   if (length % ir->buf->chunk_size) {
ret = -EINVAL;
goto out_locked;
}
diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index 6d4c5a957ab4..c6a2fe2ad210 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1348,8 +1348,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,
.fops   = _fops,
.owner  = THIS_MODULE,
 };
@@ -1456,8 +1454,7 @@ static int ir_probe(struct i2c_client *client, const 
struct i2c_device_id *id)
ir->l.dev  = >dev;
/* This will be returned by lirc_get_pdata() */
ir->l.data = ir;
-   ret = lirc_buffer_init(ir->l.rbuf,
-  ir->l.chunk_size, ir->l.buffer_size);
+   ret = lirc_buffer_init(ir->l.rbuf, 2, BUFLEN / 2);
if (ret)
goto out_put_ir;
}
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 20c5c5d6f101..a01fe5433bb7 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -121,13 +121,14 @@ static inline unsigned int lirc_buffer_write(str

[PATCH 07/19] lirc_dev: remove kmalloc in lirc_dev_fop_read()

[David Härdeman]

lirc_zilog uses a chunk_size of 2 and ir-lirc-codec uses sizeof(int).

Therefore, using stack memory should be perfectly fine.

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 1773a2934484..92048d945ba7 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -376,7 +376,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
  loff_t *ppos)
 {
struct irctl *ir = file->private_data;
-   unsigned char *buf;
+   unsigned char buf[ir->buf->chunk_size];
int ret = 0, written = 0;
DECLARE_WAITQUEUE(wait, current);
 
@@ -385,10 +385,6 @@ ssize_t lirc_dev_fop_read(struct file *file,
 
dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
 
-   buf = kzalloc(ir->buf->chunk_size, GFP_KERNEL);
-   if (!buf)
-   return -ENOMEM;
-
if (mutex_lock_interruptible(>irctl_lock)) {
ret = -ERESTARTSYS;
goto out_unlocked;
@@ -464,8 +460,6 @@ ssize_t lirc_dev_fop_read(struct file *file,
mutex_unlock(>irctl_lock);
 
 out_unlocked:
-   kfree(buf);
-
return ret ? ret : written;
 }
 EXPORT_SYMBOL(lirc_dev_fop_read);

[PATCH 01/19] lirc_dev: clarify error handling

[David Härdeman]

If an error is generated, it is more logical to error out ASAP.

Signed-off-by: David Härdeman <da...@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 db1e7b70c998..9c1d55e41e34 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -282,7 +282,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));
@@ -323,9 +323,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 03/19] lirc_dev: remove min_timeout and max_timeout

[David Härdeman]

There are no users of this functionality (ir-lirc-codec.c has its own
implementation and lirc_zilog.c doesn't use it) so remove it.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |   18 --
 include/media/lirc_dev.h|8 
 2 files changed, 26 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index c9afaf5e64a9..591dee9f6ba2 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -404,24 +404,6 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
case LIRC_GET_LENGTH:
result = put_user(ir->d.code_length, (__u32 __user *)arg);
break;
-   case LIRC_GET_MIN_TIMEOUT:
-   if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||
-   ir->d.min_timeout == 0) {
-   result = -ENOTTY;
-   break;
-   }
-
-   result = put_user(ir->d.min_timeout, (__u32 __user *)arg);
-   break;
-   case LIRC_GET_MAX_TIMEOUT:
-   if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||
-   ir->d.max_timeout == 0) {
-   result = -ENOTTY;
-   break;
-   }
-
-   result = put_user(ir->d.max_timeout, (__u32 __user *)arg);
-   break;
default:
result = -ENOTTY;
}
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 1419d64e2e59..53eef86e07a0 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -132,12 +132,6 @@ static inline unsigned int lirc_buffer_write(struct 
lirc_buffer *buf,
  * @data:  it may point to any driver data and this pointer will
  * be passed to all callback functions.
  *
- * @min_timeout:   Minimum timeout for record. Valid only if
- * LIRC_CAN_SET_REC_TIMEOUT is defined.
- *
- * @max_timeout:   Maximum timeout for record. Valid only if
- * LIRC_CAN_SET_REC_TIMEOUT is defined.
- *
  * @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).
@@ -168,8 +162,6 @@ struct lirc_driver {
unsigned int chunk_size;
 
void *data;
-   int min_timeout;
-   int max_timeout;
struct lirc_buffer *rbuf;
struct rc_dev *rdev;
const struct file_operations *fops;

[PATCH 02/19] lirc_dev: remove support for manually specifying minor number

[David Härdeman]

All users of lirc_register_driver() uses dynamic minor allocation, therefore
we can remove the ability to explicitly request a given number.

This changes the function prototype of lirc_unregister_driver() to also
take a struct lirc_driver pointer as the sole argument.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ir-lirc-codec.c|9 +---
 drivers/media/rc/lirc_dev.c |   68 ---
 drivers/staging/media/lirc/lirc_zilog.c |   14 ++
 include/media/lirc_dev.h|   18 
 4 files changed, 33 insertions(+), 76 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index a30af91710fe..2c1221a61ea1 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -382,7 +382,6 @@ static int ir_lirc_register(struct rc_dev *dev)
 
snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
 dev->driver_name);
-   drv->minor = -1;
drv->features = features;
drv->data = >raw->lirc;
drv->rbuf = NULL;
@@ -394,11 +393,9 @@ static int ir_lirc_register(struct rc_dev *dev)
drv->rdev = dev;
drv->owner = THIS_MODULE;
 
-   drv->minor = lirc_register_driver(drv);
-   if (drv->minor < 0) {
-   rc = -ENODEV;
+   rc = lirc_register_driver(drv);
+   if (rc < 0)
goto out;
-   }
 
dev->raw->lirc.drv = drv;
dev->raw->lirc.dev = dev;
@@ -413,7 +410,7 @@ static int ir_lirc_unregister(struct rc_dev *dev)
 {
struct lirc_codec *lirc = >raw->lirc;
 
-   lirc_unregister_driver(lirc->drv->minor);
+   lirc_unregister_driver(lirc->drv);
kfree(lirc->drv);
lirc->drv = NULL;
 
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 9c1d55e41e34..c9afaf5e64a9 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -29,7 +29,6 @@
 #include 
 #include 
 
-#define NOPLUG -1
 #define LOGHEAD"lirc_dev (%s[%d]): "
 
 static dev_t lirc_base_dev;
@@ -112,7 +111,7 @@ static int lirc_allocate_buffer(struct irctl *ir)
 int lirc_register_driver(struct lirc_driver *d)
 {
struct irctl *ir;
-   int minor;
+   unsigned minor;
int err;
 
if (!d) {
@@ -130,12 +129,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);
@@ -150,21 +143,14 @@ int lirc_register_driver(struct lirc_driver *d)
 
mutex_lock(_dev_lock);
 
-   minor = d->minor;
+   /* find first free slot for driver */
+   for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++)
+   if (!irctls[minor])
+   break;
 
-   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;
+   if (minor == MAX_IRCTL_DEVICES) {
+   dev_err(d->dev, "no free slots for drivers!\n");
+   err = -ENOMEM;
goto out_lock;
}
 
@@ -176,6 +162,7 @@ int lirc_register_driver(struct lirc_driver *d)
 
mutex_init(>irctl_lock);
irctls[minor] = ir;
+   d->irctl = ir;
d->minor = minor;
 
/* some safety check 8-) */
@@ -221,7 +208,7 @@ int lirc_register_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;
+   return 0;
 
 out_cdev:
cdev_del(>cdev);
@@ -234,38 +221,24 @@ int lirc_register_driver(struct lirc_driver *d)
 }
 EXPORT_SYMBOL(lirc_register_driver);
 
-int lirc_unregister_driver(int minor)
+void lirc_unregister_driver(struct lirc_driver *d)
 {
struct irctl *ir;
 
-   if (minor < 0 || minor >= MAX_IRCTL_DEVIC

[PATCH 00/19] lirc_dev modernisation

[David Härdeman]

This patch series reworks lirc_dev to use a single struct lirc_dev to
keep track of registered lirc devices rather than the current situation
where a combination of a struct lirc_driver and a struct irctl are used.
The fact that two structs are currently used per device makes the current
code harder to read and to analyse (e.g. wrt locking correctness).

The idea started out with this patch:
http://www.mail-archive.com/linux-media@vger.kernel.org/msg112159.html

Which was rejected due to the struct copying. In fixing that issue and
at the same time trying to split up the patch in smaller pieces, I ended
up with quite a bit larger patch series than first expected.

The end result is that struct lirc_dev (which is maintained by lirc_dev)
has proper lifecycle management and that we can avoid the current struct
copying that is performed between struct lirc_driver and struct irctl.

The locking in lirc_dev is also much improved by only having one mutex per
struct lirc_dev which is used to synchronize all operations.

The modifications to lirc_dev and ir-lirc-codec have been tested using
rc-loopback and mceusb. The changes to lirc_zilog are only compile tested.

---

David Härdeman (19):
  lirc_dev: clarify error handling
  lirc_dev: remove support for manually specifying minor number
  lirc_dev: remove min_timeout and max_timeout
  lirc_dev: use cdev_device_add() helper function
  lirc_dev: make better use of file->private_data
  lirc_dev: make chunk_size and buffer_size mandatory
  lirc_dev: remove kmalloc in lirc_dev_fop_read()
  lirc_dev: change irctl->attached to be a boolean
  lirc_dev: sanitize locking
  lirc_dev: use an IDA instead of an array to keep track of registered 
devices
  lirc_dev: rename struct lirc_driver to struct lirc_dev
  lirc_dev: introduce lirc_allocate_device and lirc_free_device
  lirc_dev: remove the BUFLEN define
  lirc_zilog: add a pointer to the parent device to struct IR
  lirc_zilog: use a dynamically allocated lirc_dev
  lirc_dev: merge struct irctl into struct lirc_dev
  ir-lirc-codec: merge lirc_dev_fop_ioctl into ir_lirc_ioctl
  ir-lirc-codec: move the remaining fops over from lirc_dev
  lirc_dev: consistent device registration printk


 drivers/media/rc/ir-lirc-codec.c|  404 -
 drivers/media/rc/lirc_dev.c |  587 ++-
 drivers/media/rc/rc-core-priv.h |2 
 drivers/staging/media/lirc/lirc_zilog.c |  234 +---
 include/media/lirc_dev.h|  111 ++
 5 files changed, 570 insertions(+), 768 deletions(-)

--
David Härdeman

[PATCH 2/2] rc-main: remove input events for repeat messages

[David Härdeman]

Protocols like NEC generate around 10 repeat events per second.

The input events are not very useful for userspace but still waste power
by waking up every listener. So let's remove them (MSC_SCAN events
are still generated for the initial keypress).

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/rc-main.c |   13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 7387bd4d75b0..9f490aa11bc4 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -616,16 +616,11 @@ void rc_repeat(struct rc_dev *dev)
 
spin_lock_irqsave(>keylock, flags);
 
-   if (!dev->keypressed)
-   goto out;
-
-   input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
-   input_sync(dev->input_dev);
-
-   dev->keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT);
-   mod_timer(>timer_keyup, dev->keyup_jiffies);
+   if (dev->keypressed) {
+   dev->keyup_jiffies = jiffies + 
msecs_to_jiffies(IR_KEYPRESS_TIMEOUT);
+   mod_timer(>timer_keyup, dev->keyup_jiffies);
+   }
 
-out:
spin_unlock_irqrestore(>keylock, flags);
 }
 EXPORT_SYMBOL_GPL(rc_repeat);

[PATCH 0/2] rc-core: consistent rc_repeat() usage

[David Härdeman]

These two patches are a reworked version of these two patches:
http://www.mail-archive.com/linux-media@vger.kernel.org/msg112170.html
http://www.mail-archive.com/linux-media@vger.kernel.org/msg112163.html

The first patch changes the NEC and Sanyo decoders as discussed in those
threads, moving the keydown check into rc_repeat() where the proper
locking is done.

The second patch I'd consider optional, it removes the input events for
repeat messages which I consider to be rather pointless.

---

David Härdeman (2):
  rc-core: consistent use of rc_repeat()
  rc-main: remove input events for repeat messages


 drivers/media/rc/ir-nec-decoder.c   |   10 +++---
 drivers/media/rc/ir-sanyo-decoder.c |   10 +++---
 drivers/media/rc/rc-main.c  |   13 -
 3 files changed, 10 insertions(+), 23 deletions(-)

--
David Härdeman

[PATCH 1/2] rc-core: consistent use of rc_repeat()

[David Härdeman]

The NEC decoder and the Sanyo decoders check if dev->keypressed is true
before calling rc_repeat (without holding dev->keylock).

Meanwhile, the XMP and JVC decoders do no such checks.

This patch makes sure all users of rc_repeat() do so consistently by removing
extra checks in NEC/Sanyo and modifying the check a bit in rc_repeat() so that
no input event is generated if the key isn't pressed.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ir-nec-decoder.c   |   10 +++---
 drivers/media/rc/ir-sanyo-decoder.c |   10 +++---
 drivers/media/rc/rc-main.c  |6 +++---
 3 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/media/rc/ir-nec-decoder.c 
b/drivers/media/rc/ir-nec-decoder.c
index 3ce850314dca..75b9137f6faf 100644
--- a/drivers/media/rc/ir-nec-decoder.c
+++ b/drivers/media/rc/ir-nec-decoder.c
@@ -88,13 +88,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct 
ir_raw_event ev)
data->state = STATE_BIT_PULSE;
return 0;
} else if (eq_margin(ev.duration, NEC_REPEAT_SPACE, NEC_UNIT / 
2)) {
-   if (!dev->keypressed) {
-   IR_dprintk(1, "Discarding last key repeat: 
event after key up\n");
-   } else {
-   rc_repeat(dev);
-   IR_dprintk(1, "Repeat last key\n");
-   data->state = STATE_TRAILER_PULSE;
-   }
+   rc_repeat(dev);
+   IR_dprintk(1, "Repeat last key\n");
+   data->state = STATE_TRAILER_PULSE;
return 0;
}
 
diff --git a/drivers/media/rc/ir-sanyo-decoder.c 
b/drivers/media/rc/ir-sanyo-decoder.c
index 520bb77dcb62..e6a906a34f90 100644
--- a/drivers/media/rc/ir-sanyo-decoder.c
+++ b/drivers/media/rc/ir-sanyo-decoder.c
@@ -110,13 +110,9 @@ static int ir_sanyo_decode(struct rc_dev *dev, struct 
ir_raw_event ev)
break;
 
if (!data->count && geq_margin(ev.duration, SANYO_REPEAT_SPACE, 
SANYO_UNIT / 2)) {
-   if (!dev->keypressed) {
-   IR_dprintk(1, "SANYO discarding last key 
repeat: event after key up\n");
-   } else {
-   rc_repeat(dev);
-   IR_dprintk(1, "SANYO repeat last key\n");
-   data->state = STATE_INACTIVE;
-   }
+   rc_repeat(dev);
+   IR_dprintk(1, "SANYO repeat last key\n");
+   data->state = STATE_INACTIVE;
return 0;
}
 
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index a9eba0013525..7387bd4d75b0 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -616,12 +616,12 @@ void rc_repeat(struct rc_dev *dev)
 
spin_lock_irqsave(>keylock, flags);
 
-   input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
-   input_sync(dev->input_dev);
-
if (!dev->keypressed)
goto out;
 
+   input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
+   input_sync(dev->input_dev);
+
dev->keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT);
mod_timer(>timer_keyup, dev->keyup_jiffies);
 

Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[David Härdeman]

On Sun, Jun 11, 2017 at 05:17:40PM +0100, Sean Young wrote:
>On Sat, Apr 29, 2017 at 10:44:58AM +0200, David Härdeman wrote:
>> >This can be implemented without breaking userspace.
>> 
>> How?
>
>The current keymaps we have do not specify the protocol variant, only
>the protocol (rc6 vs rc6-mce). So to support this, we have to be able
>to specify multiple protocols at the same time. So I think the protocol
>should be a bitmask.
>
>Also, in your example you re-used RC_TYPE_OTHER to match any protocol;
>I don't think that is a good solution since there are already keymaps
>which use other.
>
>So if we have an "struct rc_scancode" which looks like:
>
>struct rc_scancode {
>   u64 protocol;
>   u64 scancode;
>};
>
>Then if the keymap protocol is rc6, ir-keytable should set the protocol
>to RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | RC_BIT_RC6_6A_32
> | RC_BIT_RC6_MCE.
>
>If the old ioctl is used, then the protocol should be set to RC_BIT_ALL.
>
>I can't think of anything what would break with this scheme.

I've tried coding up that solution before and the problem is that it'll
still require heuristics in the presence of a mix of old and new style
ioctl():s.

For example:

old_ioctl_set(0x1234 = KEY_A);
new_ioctl_set(PROTOCOL_NEC, 0x1234 = KEY_B);
new_ioctl_set(PROTOCOL_JVC, 0x1234 = KEY_C);
old_ioctl_get(0x1234) = ?
old_ioctl_set(0x1234 = KEY_D);
new_ioctl_get(PROTOCOL_NEC, 0x1234) = ?

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

Re: [PATCH 3/7] rc-core: img-nec-decoder - leave the internals of rc_dev alone

[David Härdeman]

On Sun, Jun 11, 2017 at 05:02:24PM +0100, Sean Young wrote:
>On Sun, May 28, 2017 at 10:28:44AM +0200, David Härdeman wrote:
>> On Mon, May 22, 2017 at 09:40:30PM +0100, Sean Young wrote:
>> >On Mon, May 01, 2017 at 06:10:06PM +0200, David Härdeman wrote:
>> >> Obvious fix, leave repeat handling to rc-core
>> >> 
>> >> Signed-off-by: David Härdeman <da...@hardeman.nu>
>> >> ---
>> >>  drivers/media/rc/ir-nec-decoder.c |   10 +++---
>> >>  1 file changed, 3 insertions(+), 7 deletions(-)
>> >> 
>> >> diff --git a/drivers/media/rc/ir-nec-decoder.c 
>> >> b/drivers/media/rc/ir-nec-decoder.c
>> >> index 3ce850314dca..75b9137f6faf 100644
>> >> --- a/drivers/media/rc/ir-nec-decoder.c
>> >> +++ b/drivers/media/rc/ir-nec-decoder.c
>> >> @@ -88,13 +88,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct 
>> >> ir_raw_event ev)
>> >>   data->state = STATE_BIT_PULSE;
>> >>   return 0;
>> >>   } else if (eq_margin(ev.duration, NEC_REPEAT_SPACE, NEC_UNIT / 
>> >> 2)) {
>> >> - if (!dev->keypressed) {
>> >> - IR_dprintk(1, "Discarding last key repeat: 
>> >> event after key up\n");
>> >> - } else {
>> >> - rc_repeat(dev);
>> >> - IR_dprintk(1, "Repeat last key\n");
>> >> - data->state = STATE_TRAILER_PULSE;
>> >> - }
>> >> + rc_repeat(dev);
>> >> + IR_dprintk(1, "Repeat last key\n");
>> >> + data->state = STATE_TRAILER_PULSE;
>> >
>> >This is not correct. This means that whenever a nec repeat is received,
>> >the last scancode is sent to the input device, irrespective of whether
>> >there has been no IR for hours. The original code is stricter.
>> 
>> I think that'd be an argument for moving the check to rc_repeat().
>
>It is.

And I just realised that the check is anyway incorrect since
dev->keylock isn't held when dev->keypressed is checked. rc_repeat() on
the other hand does the right thing, which is another argument for
moving the check.

>> But, on the other hand, sending an input scancode for each repeat event
>> seems kind of pointless, doesn't it? If so, it might make more sense to
>> just remove the input event generation from rc_repeat() altogether...
>
>At least there is something visible in user-space saying that a repeat
>has been received.

Yes, but I'm not sure what the value of that information is? The
MSC_SCAN events are mostly useful to create new keymaps for unknown
controls, additional repeat messages won't really be of any use there.

And generating repeat events has the disadvantage that userspace will be
woken up (how often is protocol dependent, but once every 110ms for NEC,
as an example) repeatedly while a button is pressed.

Anyway, I'll spin a new patch in two parts, one which makes the
behaviour consistent between decoders and one which removes the input
event altogether, then you can decide whether you want to merge the
second patch or not.

-- 
David Härdeman

Re: [PATCH 5/7] rc-core: ir-raw - leave the internals of rc_dev alone

[David Härdeman]

On Tue, May 23, 2017 at 10:20:27AM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:10:17PM +0200, David Härdeman wrote:
>> Replace the REP_DELAY value with a static value, which makes more sense.
>> Automatic repeat handling in the input layer has no relevance for the drivers
>> idea of "a long time".
>> 
>> Signed-off-by: David Härdeman <da...@hardeman.nu>
>> ---
>>  drivers/media/rc/rc-ir-raw.c |4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
>> index ae7785c4fbe7..967ab9531e0a 100644
>> --- a/drivers/media/rc/rc-ir-raw.c
>> +++ b/drivers/media/rc/rc-ir-raw.c
>> @@ -102,20 +102,18 @@ int ir_raw_event_store_edge(struct rc_dev *dev, enum 
>> raw_event_type type)
>>  s64 delta; /* ns */
>>  DEFINE_IR_RAW_EVENT(ev);
>>  int rc = 0;
>> -int delay;
>>  
>>  if (!dev->raw)
>>  return -EINVAL;
>>  
>>  now = ktime_get();
>>  delta = ktime_to_ns(ktime_sub(now, dev->raw->last_event));
>> -delay = MS_TO_NS(dev->input_dev->rep[REP_DELAY]);
>>  
>>  /* Check for a long duration since last event or if we're
>>   * being called for the first time, note that delta can't
>>   * possibly be negative.
>>   */
>> -if (delta > delay || !dev->raw->last_type)
>> +if (delta > MS_TO_NS(500) || !dev->raw->last_type)
>>  type |= IR_START_EVENT;
>
>So this is just a fail-safe to ensure that the IR decoders are reset after
>a period of IR silence. The decoders should reset themselves anyway if they
>receive a long space, so it's just belt and braces.

Not 100% sure but it also checks for the first call to
ir_raw_event_store_edge()...

>Why is a static value better? At least REP_DELAY can be changed from
>user space.

REP_DELAY serves a completely different purpose. It controls how long a
key should be pressed before the input layer should start generating
soft-repeat events.

The timeout here is related to the IR protocol handling...which is a
quite different matter.

>Maybe we should do away with it.

Maybe...but that'd be a different patch...I think this fix should go in
nevertheless.

-- 
David Härdeman

Re: [PATCH 3/7] rc-core: img-nec-decoder - leave the internals of rc_dev alone

[David Härdeman]

On Mon, May 22, 2017 at 09:40:30PM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:10:06PM +0200, David Härdeman wrote:
>> Obvious fix, leave repeat handling to rc-core
>> 
>> Signed-off-by: David Härdeman <da...@hardeman.nu>
>> ---
>>  drivers/media/rc/ir-nec-decoder.c |   10 +++---
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/media/rc/ir-nec-decoder.c 
>> b/drivers/media/rc/ir-nec-decoder.c
>> index 3ce850314dca..75b9137f6faf 100644
>> --- a/drivers/media/rc/ir-nec-decoder.c
>> +++ b/drivers/media/rc/ir-nec-decoder.c
>> @@ -88,13 +88,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct 
>> ir_raw_event ev)
>>  data->state = STATE_BIT_PULSE;
>>  return 0;
>>  } else if (eq_margin(ev.duration, NEC_REPEAT_SPACE, NEC_UNIT / 
>> 2)) {
>> -if (!dev->keypressed) {
>> -IR_dprintk(1, "Discarding last key repeat: 
>> event after key up\n");
>> -} else {
>> -rc_repeat(dev);
>> -IR_dprintk(1, "Repeat last key\n");
>> -data->state = STATE_TRAILER_PULSE;
>> -}
>> +rc_repeat(dev);
>> +IR_dprintk(1, "Repeat last key\n");
>> +data->state = STATE_TRAILER_PULSE;
>
>This is not correct. This means that whenever a nec repeat is received,
>the last scancode is sent to the input device, irrespective of whether
>there has been no IR for hours. The original code is stricter.

I think that'd be an argument for moving the check to rc_repeat().

But, on the other hand, sending an input scancode for each repeat event
seems kind of pointless, doesn't it? If so, it might make more sense to
just remove the input event generation from rc_repeat() altogether...

-- 
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 <da...@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 = >raw->lirc;
>>  drv->rbuf = NULL;
>> @@ -394,11 +393,9 @@ static int ir_lirc_register(struct rc_dev *dev)
>>  drv->rdev = dev;
>>  drv->owner = THIS_MODULE;
>>  
>> -drv->minor = lirc_register_driver(drv);
>> -if (drv->minor < 0) {
>> -rc = -ENODEV;
>> +rc = lirc_register_driver(drv);
>> +if (rc < 0)
>>  goto out;
>> -}
>>  
>>  dev->raw->lirc.drv = drv;
>>  dev->raw->lirc.dev = dev;
>> @@ -413,7 +410,7 @@ static int ir_lirc_unregister(struct rc_dev *dev)
>>  {
>>  struct lirc_codec *lirc = >raw->lirc;
>>  
>> -lirc_unregister_driver(lirc->drv->minor);
>> +lirc_unregister_driver(lirc->drv);
>>  kfree(lirc->drv);
>>  lirc->drv = NULL;
>>  
>> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
>> index e44e0b23b883..7db7d4c57991 100644
>> --- a/drivers/media/rc/lirc_dev.c
>> +++ b/drivers/media/rc/lirc_dev.c
>> @@ -31,23 +31,35 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include 
>>  #include 
>>  #include 
>>  
>>  #define IRCTL_DEV_NAME  "BaseRemoteCtl"
>> -#define NOPLUG  -1
>>  #define LOGHEAD "lirc_dev (%s[%d]): "
>>  
>>  static dev_t lirc_base_dev;
>>  
>> +/**
>> + * struct irctl - lirc per-device structure
>> + * @d:  internal copy of the  lirc_driver for 
>> the device
>> + * @dead:   if the device has gone away
>> + * @users:  the number of users of the lirc chardev (currently 
>> limited to 1)
>> + * @mutex:  synchronizes open()/close()/ioctl()/etc calls
>> + * @buf:used to store lirc RX data
>> + * @buf_internal:   whether @buf was allocated internally or not
>> + * @chunk_size: @buf stores and read() returns data chunks of 
>> this size
>> + * @dev:the  device for the lirc device
>> + * @cdev:   the  device for the lirc chardev
>> + */
>>  struct irctl {
>>  struct lirc_driver d;
>> -int attached;
>> -int open;
>> +bool dead;
>> +unsigned users;
>>  
>> -struct mutex irctl_lock;
>> +struct mutex mutex;
>>  struct lirc_buffer *buf;
>>  bool buf_internal;
>>  unsigned int chunk_size;
>> @@ -56,9 +68,9 @@ struct irctl {
>>  struct cdev cdev;
>>  };
>>  
>> -static DEFINE_MUTEX(lirc_dev_lock);
>> -
>> -static struct irctl *irctls[MAX_IRCTL_DEVICES];
>> +/* Used to keep track of allocated lirc devices */
>> +#define LIRC_MAX_DEVICES 256
>> +static DEFINE_IDA(lirc_ida);
>>  
>>  /* Only used for sysfs but defined to void otherwise */
>>  static struct class *lirc_class;
>> @@ -72,9 +84,6 @@ static void lirc_release(struct device *ld)
>>  kfree(ir->buf);
>>  }
>>  
>> -mutex_lock(_dev_lock);
>> -irctls[ir->d.minor] = NULL;
>> -mutex_unlock(_dev_lock);
>>  kfree(ir);
>>  }
>>  
>> @@ -117,7 +126,18 @@ static int lirc_allocat

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 <da...@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 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 <da...@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 
>> -#include 
>>  #include 
>> -#include 
>>  #include 
>> -#include 
>>  #include 
>> -#include 
>>  #include 
>> -#include 
>> -#include 
>> -#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -37,7 +28,6 @@
>>  #include 
>>  #include 
>>  
>> -#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(_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] rc-core: cleanup rc_register_device (v2)

[David Härdeman]

On Sat, May 20, 2017 at 12:10:40PM +0100, Sean Young wrote:
>On Wed, May 03, 2017 at 12:04:00PM +0200, David Härdeman wrote:
>> The device core infrastructure is based on the presumption that
>> once a driver calls device_add(), it must be ready to accept
>> userspace interaction.
>> 
>> This requires splitting rc_setup_rx_device() into two functions
>> and reorganizing rc_register_device() so that as much work
>> as possible is performed before calling device_add().
>> 
>> Version 2: switch the order in which rc_prepare_rx_device() and
>> ir_raw_event_prepare() gets called so that dev->change_protocol()
>> gets called before device_add().
>
>With this patch applied, when I plug in an iguanair usb device, I get.

I'm not surprised that changes to rc_register_device() might require
some driver-specific fixes as well.

I haven't looked at this yet, and I'm going on vacation in a few hours,
so I'll probably take a look in a week...


-- 
David Härdeman

Re: [PATCH] rc-core: cleanup rc_register_device (v2)

[David Härdeman]

On Wed, May 17, 2017 at 09:09:57PM +0100, Sean Young wrote:
>Hi David,
>
>On Wed, May 03, 2017 at 12:04:00PM +0200, David Härdeman wrote:
>> The device core infrastructure is based on the presumption that
>> once a driver calls device_add(), it must be ready to accept
>> userspace interaction.
>> 
>> This requires splitting rc_setup_rx_device() into two functions
>> and reorganizing rc_register_device() so that as much work
>> as possible is performed before calling device_add().
>> 
>> Version 2: switch the order in which rc_prepare_rx_device() and
>> ir_raw_event_prepare() gets called so that dev->change_protocol()
>> gets called before device_add().
>
>I've looked at this patch and I don't see what problem it solves;
>what user-space interaction is problematic?

It's a preparatory patch, the next patch ("rc-core: cleanup
rc_register_device pt2") is the one which removes the dev->initialized
hack (which currently papers over the fact that device_add() is called
before userspace is actually ready to accept sysfs interaction).

Does that answer your question?

//David

[PATCH] rc-core: cleanup rc_register_device (v2)

[David Härdeman]

The device core infrastructure is based on the presumption that
once a driver calls device_add(), it must be ready to accept
userspace interaction.

This requires splitting rc_setup_rx_device() into two functions
and reorganizing rc_register_device() so that as much work
as possible is performed before calling device_add().

Version 2: switch the order in which rc_prepare_rx_device() and
ir_raw_event_prepare() gets called so that dev->change_protocol()
gets called before device_add().

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/rc-core-priv.h |2 +
 drivers/media/rc/rc-ir-raw.c|   34 --
 drivers/media/rc/rc-main.c  |   75 +--
 3 files changed, 73 insertions(+), 38 deletions(-)

diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index 0455b273c2fc..b3e7cac2c3ee 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -263,7 +263,9 @@ int ir_raw_gen_pl(struct ir_raw_event **ev, unsigned int 
max,
  * Routines from rc-raw.c to be used internally and by decoders
  */
 u64 ir_raw_get_allowed_protocols(void);
+int ir_raw_event_prepare(struct rc_dev *dev);
 int ir_raw_event_register(struct rc_dev *dev);
+void ir_raw_event_free(struct rc_dev *dev);
 void ir_raw_event_unregister(struct rc_dev *dev);
 int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler);
 void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler);
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 90f66dc7c0d7..ae7785c4fbe7 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -486,14 +486,18 @@ EXPORT_SYMBOL(ir_raw_encode_scancode);
 /*
  * Used to (un)register raw event clients
  */
-int ir_raw_event_register(struct rc_dev *dev)
+int ir_raw_event_prepare(struct rc_dev *dev)
 {
-   int rc;
-   struct ir_raw_handler *handler;
+   static bool raw_init; /* 'false' default value, raw decoders loaded? */
 
if (!dev)
return -EINVAL;
 
+   if (!raw_init) {
+   request_module("ir-lirc-codec");
+   raw_init = true;
+   }
+
dev->raw = kzalloc(sizeof(*dev->raw), GFP_KERNEL);
if (!dev->raw)
return -ENOMEM;
@@ -502,6 +506,13 @@ int ir_raw_event_register(struct rc_dev *dev)
dev->change_protocol = change_protocol;
INIT_KFIFO(dev->raw->kfifo);
 
+   return 0;
+}
+
+int ir_raw_event_register(struct rc_dev *dev)
+{
+   struct ir_raw_handler *handler;
+
/*
 * raw transmitters do not need any event registration
 * because the event is coming from userspace
@@ -510,10 +521,8 @@ int ir_raw_event_register(struct rc_dev *dev)
dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
   "rc%u", dev->minor);
 
-   if (IS_ERR(dev->raw->thread)) {
-   rc = PTR_ERR(dev->raw->thread);
-   goto out;
-   }
+   if (IS_ERR(dev->raw->thread))
+   return PTR_ERR(dev->raw->thread);
}
 
mutex_lock(_raw_handler_lock);
@@ -524,11 +533,15 @@ int ir_raw_event_register(struct rc_dev *dev)
mutex_unlock(_raw_handler_lock);
 
return 0;
+}
+
+void ir_raw_event_free(struct rc_dev *dev)
+{
+   if (!dev)
+   return;
 
-out:
kfree(dev->raw);
dev->raw = NULL;
-   return rc;
 }
 
 void ir_raw_event_unregister(struct rc_dev *dev)
@@ -547,8 +560,7 @@ void ir_raw_event_unregister(struct rc_dev *dev)
handler->raw_unregister(dev);
mutex_unlock(_raw_handler_lock);
 
-   kfree(dev->raw);
-   dev->raw = NULL;
+   ir_raw_event_free(dev);
 }
 
 /*
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 802e559cc30e..f3bc9f4e2b96 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1663,7 +1663,7 @@ struct rc_dev *devm_rc_allocate_device(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_rc_allocate_device);
 
-static int rc_setup_rx_device(struct rc_dev *dev)
+static int rc_prepare_rx_device(struct rc_dev *dev)
 {
int rc;
struct rc_map *rc_map;
@@ -1708,10 +1708,22 @@ static int rc_setup_rx_device(struct rc_dev *dev)
dev->input_dev->phys = dev->input_phys;
dev->input_dev->name = dev->input_name;
 
+   return 0;
+
+out_table:
+   ir_free_table(>rc_map);
+
+   return rc;
+}
+
+static int rc_setup_rx_device(struct rc_dev *dev)
+{
+   int rc;
+
/* rc_open will be called here */
rc = input_register_device(dev->input_dev);
if (rc)
-   goto out_table;
+   return rc;
 
/*
 * Default delay of 250ms is too 

Re: [PATCH 2/6] rc-core: cleanup rc_register_device

[David Härdeman]

On Tue, May 02, 2017 at 09:48:26PM +0100, Sean Young wrote:
>On Tue, May 02, 2017 at 08:53:02PM +0200, David Härdeman wrote:
>> On Mon, May 01, 2017 at 07:47:25PM +0200, David Härdeman wrote:
>> >On Mon, May 01, 2017 at 05:49:53PM +0100, Sean Young wrote:
>> >>On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote:
>> >>> The device core infrastructure is based on the presumption that
>> >>> once a driver calls device_add(), it must be ready to accept
>> >>> userspace interaction.
>> >>> 
>> >>> This requires splitting rc_setup_rx_device() into two functions
>> >>> and reorganizing rc_register_device() so that as much work
>> >>> as possible is performed before calling device_add().
>> >>> 
>> >>
>> >>With this patch applied, I'm no longer getting any scancodes from
>> >>my rc devices.
>> >>
>> >>David, please can you test your patches before submitting. I have to go
>> >>over them meticulously because I cannot assume you've tested them.
>> >
>> >I did test this patch and I just redid the tests, both with rc-loopback
>> >and with a mceusb receiver. I'm seeing scancodes on the input device as
>> >well as pulse-space readings on the lirc device in both tests.
>> >
>> >I did the tests with only this patch applied and the lirc-use-after-free
>> >(v3). What hardware did you test with?
>> >
>> >Meanwhile, I'll try rebasing my patches to the latest version of the
>> >media-master git tree and test again.
>> 
>> I rebased the patches onto media-master (commit
>> 3622d3e77ecef090b5111e3c5423313f11711dfa) and tested again. I still
>> can't reproduce the problems you're having :/
>
>The protocol is not set properly. In rc_prepare_rx_device(), 
>dev->change_protocol() is call if not null. At this point it still is
>null, since it will only be set up in ir_raw_event_prepare(), which
>is called afterwards.

Ah, good catch. Since ir_raw_event_prepare() only does a kmalloc() and
sets the change_protocol pointer it should be fine to call
ir_raw_event_prepare() before rc_prepare_rx_device(). I'll prepare a v2
of the patch.

>Presumably you have udev set up to execute ir-keytable, which sets the
>protocol up (again).

Well, kind of. In the automated testing I use rc-loopback which has the
"rc-empty" keytable so it doesn't set the protocols. In my manual
testing I used mceusb with a NEC remote so I anyway needed to set the
protocols manually and I missed the fact that "[rc-6]" was no longer set
in sysfs.

>There is another problem with your patches, you've introduced a race
>condition. When device_add() is called, the protocol is not set up yet.
>So anyone reading the protocols sysfs attribute early enough will get
>false information. Is it not possible to make sure that it is all setup
>correctly at the point of device_add()?

Isn't this the same problem? If dev->change_protocol() isn't NULL when
rc_prepare_rx_device() is called then the protocol will be set up (i.e.
dev->enabled_protcols will be set to the right value). Or did I
misunderstand you?

-- 
David Härdeman

Re: [PATCH 2/6] rc-core: cleanup rc_register_device

[David Härdeman]

On Mon, May 01, 2017 at 07:47:25PM +0200, David Härdeman wrote:
>On Mon, May 01, 2017 at 05:49:53PM +0100, Sean Young wrote:
>>On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote:
>>> The device core infrastructure is based on the presumption that
>>> once a driver calls device_add(), it must be ready to accept
>>> userspace interaction.
>>> 
>>> This requires splitting rc_setup_rx_device() into two functions
>>> and reorganizing rc_register_device() so that as much work
>>> as possible is performed before calling device_add().
>>> 
>>
>>With this patch applied, I'm no longer getting any scancodes from
>>my rc devices.
>>
>>David, please can you test your patches before submitting. I have to go
>>over them meticulously because I cannot assume you've tested them.
>
>I did test this patch and I just redid the tests, both with rc-loopback
>and with a mceusb receiver. I'm seeing scancodes on the input device as
>well as pulse-space readings on the lirc device in both tests.
>
>I did the tests with only this patch applied and the lirc-use-after-free
>(v3). What hardware did you test with?
>
>Meanwhile, I'll try rebasing my patches to the latest version of the
>media-master git tree and test again.

I rebased the patches onto media-master (commit
3622d3e77ecef090b5111e3c5423313f11711dfa) and tested again. I still
can't reproduce the problems you're having :/

-- 
David Härdeman

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-cirgood 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 2/6] rc-core: cleanup rc_register_device

[David Härdeman]

On Mon, May 01, 2017 at 05:49:53PM +0100, Sean Young wrote:
>On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote:
>> The device core infrastructure is based on the presumption that
>> once a driver calls device_add(), it must be ready to accept
>> userspace interaction.
>> 
>> This requires splitting rc_setup_rx_device() into two functions
>> and reorganizing rc_register_device() so that as much work
>> as possible is performed before calling device_add().
>> 
>
>With this patch applied, I'm no longer getting any scancodes from
>my rc devices.
>
>David, please can you test your patches before submitting. I have to go
>over them meticulously because I cannot assume you've tested them.

I did test this patch and I just redid the tests, both with rc-loopback
and with a mceusb receiver. I'm seeing scancodes on the input device as
well as pulse-space readings on the lirc device in both tests.

I did the tests with only this patch applied and the lirc-use-after-free
(v3). What hardware did you test with?

Meanwhile, I'll try rebasing my patches to the latest version of the
media-master git tree and test again.

-- 
David Härdeman

[PATCH 6/7] rc-core: cx231xx - leave the internals of rc_dev alone

[David Härdeman]

Just some debug statements to change.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/usb/cx231xx/cx231xx-input.c |5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/cx231xx/cx231xx-input.c 
b/drivers/media/usb/cx231xx/cx231xx-input.c
index 6e80f3c573f3..eecf074b0a48 100644
--- a/drivers/media/usb/cx231xx/cx231xx-input.c
+++ b/drivers/media/usb/cx231xx/cx231xx-input.c
@@ -30,7 +30,7 @@ static int get_key_isdbt(struct IR_i2c *ir, enum rc_type 
*protocol,
int rc;
u8  cmd, scancode;
 
-   dev_dbg(>rc->input_dev->dev, "%s\n", __func__);
+   dev_dbg(>rc->dev, "%s\n", __func__);
 
/* poll IR chip */
rc = i2c_master_recv(ir->c, , 1);
@@ -48,8 +48,7 @@ static int get_key_isdbt(struct IR_i2c *ir, enum rc_type 
*protocol,
 
scancode = bitrev8(cmd);
 
-   dev_dbg(>rc->input_dev->dev, "cmd %02x, scan = %02x\n",
-   cmd, scancode);
+   dev_dbg(>rc->dev, "cmd %02x, scan = %02x\n", cmd, scancode);
 
*protocol = RC_TYPE_OTHER;
*pscancode = scancode;

[PATCH 7/7] rc-core: tm6000 - leave the internals of rc_dev alone

[David Härdeman]

Not sure what the driver is trying to do, however, IR handling seems incomplete
ATM so deleting the offending parts shouldn't affect functionality

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

diff --git a/drivers/media/usb/tm6000/tm6000-input.c 
b/drivers/media/usb/tm6000/tm6000-input.c
index 39c15bb2b20c..1a033f57fcc1 100644
--- a/drivers/media/usb/tm6000/tm6000-input.c
+++ b/drivers/media/usb/tm6000/tm6000-input.c
@@ -63,7 +63,6 @@ struct tm6000_IR {
u8  wait:1;
u8  pwled:2;
u8  submit_urb:1;
-   u16 key_addr;
struct urb  *int_urb;
 
/* IR device properties */
@@ -321,9 +320,6 @@ static int tm6000_ir_change_protocol(struct rc_dev *rc, u64 
*rc_type)
 
dprintk(2, "%s\n",__func__);
 
-   if ((rc->rc_map.scan) && (*rc_type == RC_BIT_NEC))
-   ir->key_addr = ((rc->rc_map.scan[0].scancode >> 8) & 0x);
-
ir->rc_type = *rc_type;
 
tm6000_ir_config(ir);

[PATCH 3/7] rc-core: img-nec-decoder - leave the internals of rc_dev alone

[David Härdeman]

Obvious fix, leave repeat handling to rc-core

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ir-nec-decoder.c |   10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/media/rc/ir-nec-decoder.c 
b/drivers/media/rc/ir-nec-decoder.c
index 3ce850314dca..75b9137f6faf 100644
--- a/drivers/media/rc/ir-nec-decoder.c
+++ b/drivers/media/rc/ir-nec-decoder.c
@@ -88,13 +88,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct 
ir_raw_event ev)
data->state = STATE_BIT_PULSE;
return 0;
} else if (eq_margin(ev.duration, NEC_REPEAT_SPACE, NEC_UNIT / 
2)) {
-   if (!dev->keypressed) {
-   IR_dprintk(1, "Discarding last key repeat: 
event after key up\n");
-   } else {
-   rc_repeat(dev);
-   IR_dprintk(1, "Repeat last key\n");
-   data->state = STATE_TRAILER_PULSE;
-   }
+   rc_repeat(dev);
+   IR_dprintk(1, "Repeat last key\n");
+   data->state = STATE_TRAILER_PULSE;
return 0;
}
 

[PATCH 5/7] rc-core: ir-raw - leave the internals of rc_dev alone

[David Härdeman]

Replace the REP_DELAY value with a static value, which makes more sense.
Automatic repeat handling in the input layer has no relevance for the drivers
idea of "a long time".

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/rc-ir-raw.c |4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index ae7785c4fbe7..967ab9531e0a 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -102,20 +102,18 @@ int ir_raw_event_store_edge(struct rc_dev *dev, enum 
raw_event_type type)
s64 delta; /* ns */
DEFINE_IR_RAW_EVENT(ev);
int rc = 0;
-   int delay;
 
if (!dev->raw)
return -EINVAL;
 
now = ktime_get();
delta = ktime_to_ns(ktime_sub(now, dev->raw->last_event));
-   delay = MS_TO_NS(dev->input_dev->rep[REP_DELAY]);
 
/* Check for a long duration since last event or if we're
 * being called for the first time, note that delta can't
 * possibly be negative.
 */
-   if (delta > delay || !dev->raw->last_type)
+   if (delta > MS_TO_NS(500) || !dev->raw->last_type)
type |= IR_START_EVENT;
else
ev.duration = delta;

[PATCH 2/7] rc-core: img-ir - leave the internals of rc_dev alone

[David Härdeman]

Changing the protocol does not imply that the keymap changes.

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

diff --git a/drivers/media/rc/img-ir/img-ir-hw.c 
b/drivers/media/rc/img-ir/img-ir-hw.c
index 431d33b36fb0..8d1439622533 100644
--- a/drivers/media/rc/img-ir/img-ir-hw.c
+++ b/drivers/media/rc/img-ir/img-ir-hw.c
@@ -702,10 +702,6 @@ static void img_ir_set_protocol(struct img_ir_priv *priv, 
u64 proto)
 {
struct rc_dev *rdev = priv->hw.rdev;
 
-   spin_lock_irq(>rc_map.lock);
-   rdev->rc_map.rc_type = __ffs64(proto);
-   spin_unlock_irq(>rc_map.lock);
-
mutex_lock(>lock);
rdev->enabled_protocols = proto;
rdev->allowed_wakeup_protocols = proto;

[PATCH 0/7] rc: don't poke around in rc_dev internals

[David Härdeman]

The following patch series fixes up various drivers which go
poking around in struct rc_dev for no good reason.

---

David Härdeman (7):
  rc-core: ati_remote - leave the internals of rc_dev alone
  rc-core: img-ir - leave the internals of rc_dev alone
  rc-core: img-nec-decoder - leave the internals of rc_dev alone
  rc-core: sanyo - leave the internals of rc_dev alone
  rc-core: ir-raw - leave the internals of rc_dev alone
  rc-core: cx231xx - leave the internals of rc_dev alone
  rc-core: tm6000 - leave the internals of rc_dev alone


 drivers/media/rc/ati_remote.c |3 ---
 drivers/media/rc/img-ir/img-ir-hw.c   |4 
 drivers/media/rc/ir-nec-decoder.c |   10 +++---
 drivers/media/rc/ir-sanyo-decoder.c   |   10 +++---
 drivers/media/rc/rc-ir-raw.c  |4 +---
 drivers/media/usb/cx231xx/cx231xx-input.c |5 ++---
 drivers/media/usb/tm6000/tm6000-input.c   |4 
 7 files changed, 9 insertions(+), 31 deletions(-)

--
David Härdeman

[PATCH 1/7] rc-core: ati_remote - leave the internals of rc_dev alone

[David Härdeman]

The REP_DELAY setting on the input device is independent of hardware. This
change should not change how to driver works (as it does a keydown/keyup and
has no real repeat handling).

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

diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
index 9cf3e69de16a..a4c6ad4f67c1 100644
--- a/drivers/media/rc/ati_remote.c
+++ b/drivers/media/rc/ati_remote.c
@@ -904,9 +904,6 @@ static int ati_remote_probe(struct usb_interface *interface,
if (err)
goto exit_kill_urbs;
 
-   /* use our delay for rc_dev */
-   ati_remote->rdev->input_dev->rep[REP_DELAY] = repeat_delay;
-
/* Set up and register mouse input device */
if (mouse) {
input_dev = input_allocate_device();

[PATCH 4/7] rc-core: sanyo - leave the internals of rc_dev alone

[David Härdeman]

Leave repeat handling to rc-core.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ir-sanyo-decoder.c |   10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/media/rc/ir-sanyo-decoder.c 
b/drivers/media/rc/ir-sanyo-decoder.c
index 520bb77dcb62..e6a906a34f90 100644
--- a/drivers/media/rc/ir-sanyo-decoder.c
+++ b/drivers/media/rc/ir-sanyo-decoder.c
@@ -110,13 +110,9 @@ static int ir_sanyo_decode(struct rc_dev *dev, struct 
ir_raw_event ev)
break;
 
if (!data->count && geq_margin(ev.duration, SANYO_REPEAT_SPACE, 
SANYO_UNIT / 2)) {
-   if (!dev->keypressed) {
-   IR_dprintk(1, "SANYO discarding last key 
repeat: event after key up\n");
-   } else {
-   rc_repeat(dev);
-   IR_dprintk(1, "SANYO repeat last key\n");
-   data->state = STATE_INACTIVE;
-   }
+   rc_repeat(dev);
+   IR_dprintk(1, "SANYO repeat last key\n");
+   data->state = STATE_INACTIVE;
return 0;
}
 

[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 <da...@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, >buf->wait_poll, wait);

[PATCH 14/16] lirc_dev: cleanup includes

[David Härdeman]

Remove superfluous includes and defines.

Signed-off-by: David Härdeman <da...@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 
-#include 
 #include 
-#include 
 #include 
-#include 
 #include 
-#include 
 #include 
-#include 
-#include 
-#include 
 #include 
 #include 
 #include 
@@ -37,7 +28,6 @@
 #include 
 #include 
 
-#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(_base_dev, 0, LIRC_MAX_DEVICES,
-IRCTL_DEV_NAME);
+"BaseRemoteCtl");
if (retval) {
class_destroy(lirc_class);
pr_err("alloc_chrdev_region failed\n");

[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 <da...@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 BUFLEN16
-
-#define mod(n, div) ((n) % (div))
-
 #include 
 #include 
 #include 
@@ -20,6 +16,8 @@
 #include 
 #include 
 
+#define BUFLEN16
+
 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)
 {

[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 <da...@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 = >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 
 #include 
 
-#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(>mutex);
 
-   dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
-   ir->d.name, ir->d.minor);
+   dev_dbg(>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(>dev, "releasing opened driver\n");
wake_up_interruptible(>buf->wait_poll);
}
 
@@ -278,7 +272,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file 
*file)
 
mutex_unlock(>mutex);
 
-   dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
+   dev_dbg(>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(>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(>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(>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(>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 f7629ff116a

[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 <da...@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 = >raw->lirc;
drv->rbuf = NULL;
@@ -394,11 +393,9 @@ static int ir_lirc_register(struct rc_dev *dev)
drv->rdev = dev;
drv->owner = THIS_MODULE;
 
-   drv->minor = lirc_register_driver(drv);
-   if (drv->minor < 0) {
-   rc = -ENODEV;
+   rc = lirc_register_driver(drv);
+   if (rc < 0)
goto out;
-   }
 
dev->raw->lirc.drv = drv;
dev->raw->lirc.dev = dev;
@@ -413,7 +410,7 @@ static int ir_lirc_unregister(struct rc_dev *dev)
 {
struct lirc_codec *lirc = >raw->lirc;
 
-   lirc_unregister_driver(lirc->drv->minor);
+   lirc_unregister_driver(lirc->drv);
kfree(lirc->drv);
lirc->drv = NULL;
 
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index e44e0b23b883..7db7d4c57991 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -31,23 +31,35 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
 #include 
 
 #define IRCTL_DEV_NAME "BaseRemoteCtl"
-#define NOPLUG -1
 #define LOGHEAD"lirc_dev (%s[%d]): "
 
 static dev_t lirc_base_dev;
 
+/**
+ * struct irctl - lirc per-device structure
+ * @d: internal copy of the  lirc_driver for the device
+ * @dead:  if the device has gone away
+ * @users: the number of users of the lirc chardev (currently 
limited to 1)
+ * @mutex: synchronizes open()/close()/ioctl()/etc calls
+ * @buf:   used to store lirc RX data
+ * @buf_internal:  whether @buf was allocated internally or not
+ * @chunk_size:@buf stores and read() returns data chunks of 
this size
+ * @dev:   the  device for the lirc device
+ * @cdev:  the  device for the lirc chardev
+ */
 struct irctl {
struct lirc_driver d;
-   int attached;
-   int open;
+   bool dead;
+   unsigned users;
 
-   struct mutex irctl_lock;
+   struct mutex mutex;
struct lirc_buffer *buf;
bool buf_internal;
unsigned int chunk_size;
@@ -56,9 +68,9 @@ struct irctl {
struct cdev cdev;
 };
 
-static DEFINE_MUTEX(lirc_dev_lock);
-
-static struct irctl *irctls[MAX_IRCTL_DEVICES];
+/* Used to keep track of allocated lirc devices */
+#define LIRC_MAX_DEVICES 256
+static DEFINE_IDA(lirc_ida);
 
 /* Only used for sysfs but defined to void otherwise */
 static struct class *lirc_class;
@@ -72,9 +84,6 @@ static void lirc_release(struct device *ld)
kfree(ir->buf);
}
 
-   mutex_lock(_dev_lock);
-   irctls[ir->d.minor] = NULL;
-   mutex_unlock(_dev_lock);
kfree(ir);
 }
 
@@ -117,7 +126,18 @@ static int lirc_allocate_buffer(struct irctl *ir)
return err;
 }
 
-int lirc_register_driver(struct lirc_driver *d)
+/**
+ * lirc_register_driver() - create a new lirc device by registering a driver
+ * @d: the  lirc_driver to register
+ *
+ * This function allocates and registers a lirc device for a given
+ * _driver. The _driver structure is updated to contain
+ * information about the allocated device (minor, buffer, etc).
+ *
+ * Return: zero on success or a negative error value.
+ */
+int
+lirc_register_driver(struct lirc_driver *d)
 {
struct irctl *ir;
int minor;
@@ -138,12 +158,6 @@ int lirc_register_driver(struct lirc_driver *d)
return -EINVAL;
}
 
-   if (d->minor >= MAX_IRCTL_DEVICES) {
-   dev_err(d->dev, "minor must be between 0 and %d!\n",
-   MAX_IRCTL_DEVICES - 1);
-   return -EBADRQC;
-   }
-
if (d->code_length < 1 || d->code_length > (BUFLEN * 8)) {
dev_err(d->dev, "code length must be less than %d bits\n",
 

[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 <da...@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 
 #include 
 
-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 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 <da...@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(_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 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 <da...@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(>rbuf.fifo))
lirc_buffer_free(>rbuf);
list_del(>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(>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 <da...@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(>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 = >d;
-   struct cdev *cdev;
-   int retval;
-
-   cdev = >cdev;
-
-   if (!d->fops)
-   return -EINVAL;
-
-   cdev_init(cdev, d->fops);
-   cdev->owner = d->owner;
-   retval = kobject_set_name(>kobj, "lirc%d", d->minor);
-   if (retval)
-   return retval;
-
-   cdev->kobj.parent = >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(>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(>dev, "lirc%d", ir->d.minor);
 
-   err = lirc_cdev_add(ir);
+   cdev_init(>cdev, d->fops);
+   ir->cdev.owner = ir->d.owner;
+   ir->cdev.kobj.parent = >dev.kobj;
+
+   err = cdev_add(>cdev, ir->dev.devt, 1);
if (err)
goto out_free_dev;
 

[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 <da...@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 = >d;
 
-   mutex_lock(_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(_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(>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(>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 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 <da...@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 = >d;
@@ -110,13 +99,11 @@ static int lirc_cdev_add(struct irctl *ir)
 
cdev = >cdev;
 
-   if (d->fops) {
-   cdev_init(cdev, d->fops);
-   cdev->owner = d->owner;
-   } else {
-   cdev_init(cdev, _dev_fops);
-   cdev->owner = THIS_MODULE;
-   }
+   if (!d->fops)
+   return -EINVAL;
+
+   cdev_init(cdev, d->fops);
+   cdev->owner = d->owner;
retval = kobject_set_name(>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 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 <da...@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(>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(>dev, "lirc%d", ir->d.minor);
-   device_initialize(>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(>cdev);
-out_sysfs:
+out_free_dev:
put_device(>dev);
 out_lock:
mutex_unlock(_dev_lock);

[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 <da...@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 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -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(>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(>dev, "lirc%d", ir->d.minor);
device_initialize(>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, s

[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 <da...@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 = >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(>buf->wait_poll);
}
 
-   mutex_lock(>irctl_lock);
-
-   if (ir->d.set_use_dec)
-   ir->d.set_use_dec(ir->d.data);
-
-   mutex_unlock(>irctl_lock);
mutex_unlock(_dev_lock);
 
device_del(>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(_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 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 <da...@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 = >raw->lirc;
drv->rbuf = NULL;
-   drv->set_use_inc = _lirc_open;
-   drv->set_use_dec = _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   = _fops,
.owner  = THIS_MODULE,
 };

[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 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 <da...@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] ir-lirc-codec: let lirc_dev handle the lirc_buffer (v3)

[David Härdeman]

ir_lirc_register() currently creates its own lirc_buffer before
passing the lirc_driver to lirc_register_driver().

When a module is later unloaded, ir_lirc_unregister() gets called
which performs a call to lirc_unregister_driver() and then free():s
the lirc_buffer.

The problem is that:

a) there can still be a userspace app holding an open lirc fd
   when lirc_unregister_driver() returns; and

b) the lirc_buffer contains "wait_queue_head_t wait_poll" which
   is potentially used as long as any userspace app is still around.

The result is an oops which can be triggered quite easily by a
userspace app monitoring its lirc fd using epoll() and not closing
the fd promptly on device removal.

The minimalistic fix is to let lirc_dev create the lirc_buffer since
lirc_dev will then also free the buffer once it believes it is safe to
do so.

Version 2: make sure that the allocated buffer is communicated back to
ir-lirc-codec so that ir_lirc_decode() can use it.

Version 3: set chunk_size and buffer_size in ir-lirc-codec.

CC: sta...@vger.kernel.org
Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ir-lirc-codec.c |   25 +++--
 drivers/media/rc/lirc_dev.c  |   13 -
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index de85f1d7ce43..8f0669c9894c 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -354,7 +354,6 @@ static const struct file_operations lirc_fops = {
 static int ir_lirc_register(struct rc_dev *dev)
 {
struct lirc_driver *drv;
-   struct lirc_buffer *rbuf;
int rc = -ENOMEM;
unsigned long features = 0;
 
@@ -362,19 +361,12 @@ static int ir_lirc_register(struct rc_dev *dev)
if (!drv)
return rc;
 
-   rbuf = kzalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
-   if (!rbuf)
-   goto rbuf_alloc_failed;
-
-   rc = lirc_buffer_init(rbuf, sizeof(int), LIRCBUF_SIZE);
-   if (rc)
-   goto rbuf_init_failed;
-
if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
features |= LIRC_CAN_REC_MODE2;
if (dev->rx_resolution)
features |= LIRC_CAN_GET_REC_RESOLUTION;
}
+
if (dev->tx_ir) {
features |= LIRC_CAN_SEND_PULSE;
if (dev->s_tx_mask)
@@ -403,10 +395,12 @@ static int ir_lirc_register(struct rc_dev *dev)
drv->minor = -1;
drv->features = features;
drv->data = >raw->lirc;
-   drv->rbuf = rbuf;
+   drv->rbuf = NULL;
drv->set_use_inc = _lirc_open;
drv->set_use_dec = _lirc_close;
drv->code_length = sizeof(struct ir_raw_event) * 8;
+   drv->chunk_size = sizeof(int);
+   drv->buffer_size = LIRCBUF_SIZE;
drv->fops = _fops;
drv->dev = >dev;
drv->rdev = dev;
@@ -415,19 +409,15 @@ static int ir_lirc_register(struct rc_dev *dev)
drv->minor = lirc_register_driver(drv);
if (drv->minor < 0) {
rc = -ENODEV;
-   goto lirc_register_failed;
+   goto out;
}
 
dev->raw->lirc.drv = drv;
dev->raw->lirc.dev = dev;
return 0;
 
-lirc_register_failed:
-rbuf_init_failed:
-   kfree(rbuf);
-rbuf_alloc_failed:
+out:
kfree(drv);
-
return rc;
 }
 
@@ -436,9 +426,8 @@ static int ir_lirc_unregister(struct rc_dev *dev)
struct lirc_codec *lirc = >raw->lirc;
 
lirc_unregister_driver(lirc->drv->minor);
-   lirc_buffer_free(lirc->drv->rbuf);
-   kfree(lirc->drv->rbuf);
kfree(lirc->drv);
+   lirc->drv = NULL;
 
return 0;
 }
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 8d60c9f00df9..42704552b005 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -52,6 +52,7 @@ struct irctl {
 
struct mutex irctl_lock;
struct lirc_buffer *buf;
+   bool buf_internal;
unsigned int chunk_size;
 
struct device dev;
@@ -83,7 +84,7 @@ static void lirc_release(struct device *ld)
 
put_device(ir->dev.parent);
 
-   if (ir->buf != ir->d.rbuf) {
+   if (ir->buf_internal) {
lirc_buffer_free(ir->buf);
kfree(ir->buf);
}
@@ -198,6 +199,7 @@ static int lirc_allocate_buffer(struct irctl *ir)
 
if (d->rbuf) {
ir->buf = d->rbuf;
+   ir->buf_internal = false;
} else {
ir->buf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
if (!ir->buf) {
@@ -208,8 +210,11 @@ static int lirc_allocate_buffer(struct irctl *ir)
err = lirc_buffer_init(ir->buf, chunk_size, buffer_size);
 

Re: [PATCH] ir-lirc-codec: let lirc_dev handle the lirc_buffer (v2)

[David Härdeman]

On Mon, May 01, 2017 at 01:22:21PM +0100, Sean Young wrote:
>On Sat, Apr 29, 2017 at 11:22:28PM +0200, David Härdeman wrote:
>> ir_lirc_register() currently creates its own lirc_buffer before
>> passing the lirc_driver to lirc_register_driver().
>> 
>> When a module is later unloaded, ir_lirc_unregister() gets called
>> which performs a call to lirc_unregister_driver() and then free():s
>> the lirc_buffer.
>> 
>> The problem is that:
>> 
>> a) there can still be a userspace app holding an open lirc fd
>>when lirc_unregister_driver() returns; and
>> 
>> b) the lirc_buffer contains "wait_queue_head_t wait_poll" which
>>is potentially used as long as any userspace app is still around.
>> 
>> The result is an oops which can be triggered quite easily by a
>> userspace app monitoring its lirc fd using epoll() and not closing
>> the fd promptly on device removal.
>
>You're right, the rbuf is freed too early. Good catch! I missed this one.
>
>However, when I test your patch it does not work.
>
>[sean@bigcore bin]$ ./ir-ctl -d /dev/lirc1 -r
>/dev/lirc1: read returned 2 bytes
>
>The lirc_buffer is no longer has a chunk size of 4.

Thanks. I just tested that /dev/lirc0 returned something, but not the
actual data which was returned. I'll spin a v3.

-- 
David Härdeman

Re: [PATCH] [RFC] rc-core: report protocol information to userspace

[David Härdeman]

On Mon, May 01, 2017 at 11:38:30AM +0100, Sean Young wrote:
>On Sat, Apr 29, 2017 at 12:52:12PM +0200, David Härdeman wrote:
>> Whether we decide to go for any new keytable ioctl():s or not in rc-core, we
>> should provide the protocol information of keypresses to userspace.
>> 
>> Note that this means that the RC_TYPE_* definitions become part of the
>> userspace <-> kernel API/ABI (meaning a full patch should maybe move those
>> defines under include/uapi).
>> 
>> This would also need to be ack:ed by the input maintainers.
>
>This was already NACKed in the past.
>
>http://www.spinics.net/lists/linux-input/msg46941.html
>

Didn't know that, thanks for the pointer. I still think we should
revisit this though. Even if we don't add protocol-aware EVIOC[SG]KEY_V2
ioctls, that information is useful for a configuration tool when
creating keymaps for a new remote.

And examining the parent hardware device (as Dmitry seemed to suggest)
doesn't help with protocol identification.

Another option if we don't want to touch the input layer would be to
export the last_* members from struct rc_dev in sysfs (and I'm guessing
a timestamp would be necessary then). Seems like a lot of work to
accomplish what would otherwise be a one-line change in the input layer
though (one-line since I'm assuming we could provide the protocol
defines in a separate header, other than input-event-codes.h as the
protocols are subsystem-specific).

-- 
David Härdeman

Re: [PATCH] rc-core: export the hardware type to sysfs

[David Härdeman]

On Mon, May 01, 2017 at 11:36:13AM +0100, Sean Young wrote:
>On Sat, Apr 29, 2017 at 12:03:29PM +0200, David Härdeman wrote:
>> Exporting the hardware type makes it possible for userspace applications
>> to know what to expect from the hardware.
>> 
>> This makes it possible to write more user-friendly userspace apps.
>
>This duplicates lirc features (LIRC_GET_FEATURES ioctl); the one exception
>is that the scancode-only devices which have no lirc device, but there
>are patches which change that.

The intention was to let userspace have a way of knowing whether to
expect any lirc device to show up at all. If some class of devices can't
have lirc devices (looking at the patch you linked to that'd still be
CEC?) then I think it's still useful?

-- 
David Härdeman

[PATCH] ir-lirc-codec: let lirc_dev handle the lirc_buffer (v2)

[David Härdeman]

ir_lirc_register() currently creates its own lirc_buffer before
passing the lirc_driver to lirc_register_driver().

When a module is later unloaded, ir_lirc_unregister() gets called
which performs a call to lirc_unregister_driver() and then free():s
the lirc_buffer.

The problem is that:

a) there can still be a userspace app holding an open lirc fd
   when lirc_unregister_driver() returns; and

b) the lirc_buffer contains "wait_queue_head_t wait_poll" which
   is potentially used as long as any userspace app is still around.

The result is an oops which can be triggered quite easily by a
userspace app monitoring its lirc fd using epoll() and not closing
the fd promptly on device removal.

The minimalistic fix is to let lirc_dev create the lirc_buffer since
lirc_dev will then also free the buffer once it believes it is safe to
do so.

Version 2: make sure that the allocated buffer is communicated back to
ir-lirc-codec so that ir_lirc_decode() can use it.

CC: sta...@vger.kernel.org
Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ir-lirc-codec.c |   23 +--
 drivers/media/rc/lirc_dev.c  |   13 -
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index de85f1d7ce43..7b961357d333 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -354,7 +354,6 @@ static const struct file_operations lirc_fops = {
 static int ir_lirc_register(struct rc_dev *dev)
 {
struct lirc_driver *drv;
-   struct lirc_buffer *rbuf;
int rc = -ENOMEM;
unsigned long features = 0;
 
@@ -362,19 +361,12 @@ static int ir_lirc_register(struct rc_dev *dev)
if (!drv)
return rc;
 
-   rbuf = kzalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
-   if (!rbuf)
-   goto rbuf_alloc_failed;
-
-   rc = lirc_buffer_init(rbuf, sizeof(int), LIRCBUF_SIZE);
-   if (rc)
-   goto rbuf_init_failed;
-
if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
features |= LIRC_CAN_REC_MODE2;
if (dev->rx_resolution)
features |= LIRC_CAN_GET_REC_RESOLUTION;
}
+
if (dev->tx_ir) {
features |= LIRC_CAN_SEND_PULSE;
if (dev->s_tx_mask)
@@ -403,7 +395,7 @@ static int ir_lirc_register(struct rc_dev *dev)
drv->minor = -1;
drv->features = features;
drv->data = >raw->lirc;
-   drv->rbuf = rbuf;
+   drv->rbuf = NULL;
drv->set_use_inc = _lirc_open;
drv->set_use_dec = _lirc_close;
drv->code_length = sizeof(struct ir_raw_event) * 8;
@@ -415,19 +407,15 @@ static int ir_lirc_register(struct rc_dev *dev)
drv->minor = lirc_register_driver(drv);
if (drv->minor < 0) {
rc = -ENODEV;
-   goto lirc_register_failed;
+   goto out;
}
 
dev->raw->lirc.drv = drv;
dev->raw->lirc.dev = dev;
return 0;
 
-lirc_register_failed:
-rbuf_init_failed:
-   kfree(rbuf);
-rbuf_alloc_failed:
+out:
kfree(drv);
-
return rc;
 }
 
@@ -436,9 +424,8 @@ static int ir_lirc_unregister(struct rc_dev *dev)
struct lirc_codec *lirc = >raw->lirc;
 
lirc_unregister_driver(lirc->drv->minor);
-   lirc_buffer_free(lirc->drv->rbuf);
-   kfree(lirc->drv->rbuf);
kfree(lirc->drv);
+   lirc->drv = NULL;
 
return 0;
 }
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 8d60c9f00df9..42704552b005 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -52,6 +52,7 @@ struct irctl {
 
struct mutex irctl_lock;
struct lirc_buffer *buf;
+   bool buf_internal;
unsigned int chunk_size;
 
struct device dev;
@@ -83,7 +84,7 @@ static void lirc_release(struct device *ld)
 
put_device(ir->dev.parent);
 
-   if (ir->buf != ir->d.rbuf) {
+   if (ir->buf_internal) {
lirc_buffer_free(ir->buf);
kfree(ir->buf);
}
@@ -198,6 +199,7 @@ static int lirc_allocate_buffer(struct irctl *ir)
 
if (d->rbuf) {
ir->buf = d->rbuf;
+   ir->buf_internal = false;
} else {
ir->buf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
if (!ir->buf) {
@@ -208,8 +210,11 @@ static int lirc_allocate_buffer(struct irctl *ir)
err = lirc_buffer_init(ir->buf, chunk_size, buffer_size);
if (err) {
kfree(ir->buf);
+   ir->buf = NULL;
goto out;
}
+
+   ir->buf_internal = true;
}
ir->chunk_size = ir->buf->

Re: [PATCH] ir-lirc-codec: let lirc_dev handle the lirc_buffer

[David Härdeman]

On Fri, Apr 28, 2017 at 07:04:09PM +0200, David Härdeman wrote:
>ir_lirc_register() currently creates its own lirc_buffer before
>passing the lirc_driver to lirc_register_driver().
>
>When a module is later unloaded, ir_lirc_unregister() gets called
>which performs a call to lirc_unregister_driver() and then free():s
>the lirc_buffer.
>
>The problem is that:
>
>a) there can still be a userspace app holding an open lirc fd
>   when lirc_unregister_driver() returns; and
>
>b) the lirc_buffer contains "wait_queue_head_t wait_poll" which
>   is potentially used as long as any userspace app is still around.
>
>The result is an oops which can be triggered quite easily by a
>userspace app monitoring its lirc fd using epoll() and not closing
>the fd promptly on device removal.
>
>The minimalistic fix is to let lirc_dev create the lirc_buffer since
>lirc_dev will then also free the buffer once it believes it is safe to
>do so.

Ignore this patch. I missed that ir_lirc_decode() checks
dev->raw->lirc.drv->rbuf, so this needs to be reworked.

-- 
David Härdeman

[PATCH] [RFC] rc-core: report protocol information to userspace

[David Härdeman]

Whether we decide to go for any new keytable ioctl():s or not in rc-core, we
should provide the protocol information of keypresses to userspace.

Note that this means that the RC_TYPE_* definitions become part of the
userspace <-> kernel API/ABI (meaning a full patch should maybe move those
defines under include/uapi).

This would also need to be ack:ed by the input maintainers.
---
 drivers/media/rc/rc-main.c |1 +
 include/uapi/linux/input-event-codes.h |1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index e0f9b322ab02..a38c1f3569ee 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -773,6 +773,7 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_type 
protocol,
if (new_event && dev->keypressed)
ir_do_keyup(dev, false);
 
+   input_event(dev->input_dev, EV_MSC, MSC_PROTOCOL, protocol);
input_event(dev->input_dev, EV_MSC, MSC_SCAN, scancode);
 
if (new_event && keycode != KEY_RESERVED) {
diff --git a/include/uapi/linux/input-event-codes.h 
b/include/uapi/linux/input-event-codes.h
index 3af60ee69053..1a8c3554cbcb 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -794,6 +794,7 @@
 #define MSC_RAW0x03
 #define MSC_SCAN   0x04
 #define MSC_TIMESTAMP  0x05
+#define MSC_PROTOCOL   0x06
 #define MSC_MAX0x07
 #define MSC_CNT(MSC_MAX+1)
 

[PATCH] rc-core: export the hardware type to sysfs

[David Härdeman]

Exporting the hardware type makes it possible for userspace applications
to know what to expect from the hardware.

This makes it possible to write more user-friendly userspace apps.

Note that the size of sysfs_groups[] in struct rc_dev is not changed
by this patch because it was already large enough for one more group.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/rc-main.c |   43 +++
 1 file changed, 43 insertions(+)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 85f95441b85b..e0f9b322ab02 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1294,6 +1294,38 @@ static ssize_t store_protocols(struct device *device,
 }
 
 /**
+ * show_hwtype() - shows the hardware type in sysfs
+ * @device:the  device descriptor
+ * @attr:  the  device_attribute
+ * @buf:   a pointer to the output buffer
+ *
+ * This callback function is used to get the hardware type of an rc device.
+ * It is triggered by reading /sys/class/rc/rc?/hwtype.
+ *
+ * Return: the number of bytes read or a negative error code.
+ */
+static ssize_t show_hwtype(struct device *device,
+  struct device_attribute *attr,
+  char *buf)
+{
+   struct rc_dev *dev = to_rc_dev(device);
+
+   switch (dev->driver_type) {
+   case RC_DRIVER_SCANCODE:
+   return sprintf(buf, "scancode\n");
+   case RC_DRIVER_IR_RAW_TX:
+   return sprintf(buf, "ir-tx\n");
+   case RC_DRIVER_IR_RAW:
+   if (dev->tx_ir)
+   return sprintf(buf, "ir-tx-rx\n");
+   else
+   return sprintf(buf, "ir-rx\n");
+   default:
+   return sprintf(buf, "\n");
+   }
+}
+
+/**
  * show_filter() - shows the current scancode filter value or mask
  * @device:the device descriptor
  * @attr:  the device attribute struct
@@ -1613,6 +1645,7 @@ static int rc_dev_uevent(struct device *device, struct 
kobj_uevent_env *env)
  * Static device attribute struct with the sysfs attributes for IR's
  */
 static DEVICE_ATTR(protocols, 0644, show_protocols, store_protocols);
+static DEVICE_ATTR(hwtype, 0444, show_hwtype, NULL);
 static DEVICE_ATTR(wakeup_protocols, 0644, show_wakeup_protocols,
   store_wakeup_protocols);
 static RC_FILTER_ATTR(filter, S_IRUGO|S_IWUSR,
@@ -1633,6 +1666,15 @@ static struct attribute_group rc_dev_protocol_attr_grp = 
{
.attrs  = rc_dev_protocol_attrs,
 };
 
+static struct attribute *rc_dev_hwtype_attrs[] = {
+   _attr_hwtype.attr,
+   NULL,
+};
+
+static struct attribute_group rc_dev_hwtype_attr_grp = {
+   .attrs = rc_dev_hwtype_attrs,
+};
+
 static struct attribute *rc_dev_filter_attrs[] = {
_attr_filter.attr.attr,
_attr_filter_mask.attr.attr,
@@ -1863,6 +1905,7 @@ int rc_register_device(struct rc_dev *dev)
dev->sysfs_groups[attr++] = _dev_filter_attr_grp;
if (dev->s_wakeup_filter)
dev->sysfs_groups[attr++] = _dev_wakeup_filter_attr_grp;
+   dev->sysfs_groups[attr++] = _dev_hwtype_attr_grp;
dev->sysfs_groups[attr++] = NULL;
 
if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {

Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[David Härdeman]

On Fri, Apr 28, 2017 at 08:42:13PM +0100, Sean Young wrote:
>On Fri, Apr 28, 2017 at 06:59:11PM +0200, David Härdeman wrote:
>> On Fri, Apr 28, 2017 at 08:31:33AM -0300, Mauro Carvalho Chehab wrote:
>> >Em Thu, 27 Apr 2017 22:34:23 +0200
>> >David Härdeman <da...@hardeman.nu> escreveu:
>> ...
>> >> This patch changes how the "input_keymap_entry" struct is interpreted
>> >> by rc-core by casting it to "rc_keymap_entry":
>> >> 
>> ...
>> >
>> >Nack.
>> 
>> That's not a very constructive approach. If you have a better approach
>> in mind I'm all ears. Because you're surely not suggesting that we stay
>> with the current protocol-less approach forever?
>
>Well, what problem are we trying to solve actually?

I'm not sure what the confusion is? Last time around we discussed this
there seemed to be general agreement that protocol information is
useful?

>Looking at the keymaps we have already, there are many scancodes which
>overlap and only a few of them use a different protocol. So having this
>feature will not suddenly make it possible to load all our keymaps, it
>will just make it possible to simultaneously load a few more.

That's not really the point, overlaps in scancode && protocol cannot be
distinguished. But overlaps in scancode can be. I have remotes which
overlap in scancode even though they have different protocols.

>> 
>> That's a gross oversimplification.
>
>This can be implemented without breaking userspace.

How?

-- 
David Härdeman

Re: [PATCH] ir-lirc-codec: let lirc_dev handle the lirc_buffer

[David Härdeman]

On Fri, Apr 28, 2017 at 07:04:09PM +0200, David Härdeman wrote:
>ir_lirc_register() currently creates its own lirc_buffer before
>passing the lirc_driver to lirc_register_driver().
>
>When a module is later unloaded, ir_lirc_unregister() gets called
>which performs a call to lirc_unregister_driver() and then free():s
>the lirc_buffer.
>
>The problem is that:
>
>a) there can still be a userspace app holding an open lirc fd
>   when lirc_unregister_driver() returns; and
>
>b) the lirc_buffer contains "wait_queue_head_t wait_poll" which
>   is potentially used as long as any userspace app is still around.
>
>The result is an oops which can be triggered quite easily by a
>userspace app monitoring its lirc fd using epoll() and not closing
>the fd promptly on device removal.
>
>The minimalistic fix is to let lirc_dev create the lirc_buffer since
>lirc_dev will then also free the buffer once it believes it is safe to
>do so.
>
>I'm pretty certain that any driver which creates its own lirc_buffer
>is quite likely to be buggy as well, but that seems to only concern
>staging.
>
>Signed-off-by: David Härdeman <da...@hardeman.nu>

And there should probably be a CC: sta...@vger.kernel.org here...

>---
> drivers/media/rc/ir-lirc-codec.c |   23 +--
> 1 file changed, 5 insertions(+), 18 deletions(-)

[PATCH] ir-lirc-codec: let lirc_dev handle the lirc_buffer

[David Härdeman]

ir_lirc_register() currently creates its own lirc_buffer before
passing the lirc_driver to lirc_register_driver().

When a module is later unloaded, ir_lirc_unregister() gets called
which performs a call to lirc_unregister_driver() and then free():s
the lirc_buffer.

The problem is that:

a) there can still be a userspace app holding an open lirc fd
   when lirc_unregister_driver() returns; and

b) the lirc_buffer contains "wait_queue_head_t wait_poll" which
   is potentially used as long as any userspace app is still around.

The result is an oops which can be triggered quite easily by a
userspace app monitoring its lirc fd using epoll() and not closing
the fd promptly on device removal.

The minimalistic fix is to let lirc_dev create the lirc_buffer since
lirc_dev will then also free the buffer once it believes it is safe to
do so.

I'm pretty certain that any driver which creates its own lirc_buffer
is quite likely to be buggy as well, but that seems to only concern
staging.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ir-lirc-codec.c |   23 +--
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index de85f1d7ce43..7b961357d333 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -354,7 +354,6 @@ static const struct file_operations lirc_fops = {
 static int ir_lirc_register(struct rc_dev *dev)
 {
struct lirc_driver *drv;
-   struct lirc_buffer *rbuf;
int rc = -ENOMEM;
unsigned long features = 0;
 
@@ -362,19 +361,12 @@ static int ir_lirc_register(struct rc_dev *dev)
if (!drv)
return rc;
 
-   rbuf = kzalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
-   if (!rbuf)
-   goto rbuf_alloc_failed;
-
-   rc = lirc_buffer_init(rbuf, sizeof(int), LIRCBUF_SIZE);
-   if (rc)
-   goto rbuf_init_failed;
-
if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
features |= LIRC_CAN_REC_MODE2;
if (dev->rx_resolution)
features |= LIRC_CAN_GET_REC_RESOLUTION;
}
+
if (dev->tx_ir) {
features |= LIRC_CAN_SEND_PULSE;
if (dev->s_tx_mask)
@@ -403,7 +395,7 @@ static int ir_lirc_register(struct rc_dev *dev)
drv->minor = -1;
drv->features = features;
drv->data = >raw->lirc;
-   drv->rbuf = rbuf;
+   drv->rbuf = NULL;
drv->set_use_inc = _lirc_open;
drv->set_use_dec = _lirc_close;
drv->code_length = sizeof(struct ir_raw_event) * 8;
@@ -415,19 +407,15 @@ static int ir_lirc_register(struct rc_dev *dev)
drv->minor = lirc_register_driver(drv);
if (drv->minor < 0) {
rc = -ENODEV;
-   goto lirc_register_failed;
+   goto out;
}
 
dev->raw->lirc.drv = drv;
dev->raw->lirc.dev = dev;
return 0;
 
-lirc_register_failed:
-rbuf_init_failed:
-   kfree(rbuf);
-rbuf_alloc_failed:
+out:
kfree(drv);
-
return rc;
 }
 
@@ -436,9 +424,8 @@ static int ir_lirc_unregister(struct rc_dev *dev)
struct lirc_codec *lirc = >raw->lirc;
 
lirc_unregister_driver(lirc->drv->minor);
-   lirc_buffer_free(lirc->drv->rbuf);
-   kfree(lirc->drv->rbuf);
kfree(lirc->drv);
+   lirc->drv = NULL;
 
return 0;
 }

[PATCH] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl (alternative approach)

[David Härdeman]

It is currently impossible to distinguish between scancodes which have
been generated using different protocols (and scancodes can, and will,
overlap).

For example:
RC5 message to address 0x00, command 0x03 has scancode 0x0503
JVC message to address 0x00, command 0x03 has scancode 0x0503

It is only possible to distinguish (and parse) scancodes by knowing the
scancode *and* the protocol.

Setting and getting keycodes in the input subsystem used to be done via
the EVIOC[GS]KEYCODE ioctl and "unsigned int[2]" (one int for scancode
and one for the keycode).

The interface has now been extended to use the EVIOC[GS]KEYCODE_V2 ioctl
which uses the following struct:

struct input_keymap_entry {
__u8  flags;
__u8  len;
__u16 index;
__u32 keycode;
__u8  scancode[32];
};

(scancode can of course be even bigger, thanks to the len member).

This patch changes how the "input_keymap_entry" struct is interpreted
by rc-core by casting it to "rc_keymap_entry":

struct rc_scancode {
__u16 protocol;
__u16 reserved[3];
__u64 scancode;
}

struct rc_keymap_entry {
__u8  flags;
__u8  len;
__u16 index;
__u32 keycode;
union {
struct rc_scancode rc;
__u8 raw[32];
};
};

The u64 scancode member is large enough for all current protocols and it
would be possible to extend it in the future should it be necessary for
some exotic protocol.

The main advantage with this change is that the protocol is made explicit,
which means that we're not throwing away data (the protocol type).

This also means that struct rc_map no longer hardcodes the protocol, meaning
that keytables with mixed entries are possible.

Heuristics are also added to hopefully do the right thing with older
ioctls in order to preserve backwards compatibility.

This is an alternative approach to the other one that I posted here:
http://www.spinics.net/lists/linux-media/msg114898.html

It replaces patches 4-6 of that patchset.

The difference is that RC_TYPE_UNKNOWN is repurposed to serve as a marker
for ioctl() requests and keytable entries which lack protocol information.

ioctl():s which lack a protocol will match on any matching scancode,
regardless of protocol. Keytable entries of type RC_TYPE_UNKNOWN
will match on any protocol.

Note that these heuristics are also not 100% guaranteed to get things
right. For example, consider the following sequence of events:

EVIOCSKEYCODE_V2: <RC_TYPE_RC5, 0x01fe> = KEY_NUMERIC_1
EVIOCSKEYCODE_V2: <RC_TYPE_SANYO, 0x01fe> = KEY_NUMERIC_2
EVIOCGKEYCODE: <0x01fe> = ?
EVIOCSKEYCODE: <0x01fe> = KEY_RESERVED
EVIOCGKEYCODE_V2: <RC_TYPE_RC5, 0x01fe> = ?
EVIOCGKEYCODE_V2: <RC_TYPE_SANYO, 0x01fe> = ?

However, mixing and matching ioctl() types is a pretty pathological
case.

Also, that is somewhat mitigated by the fact that the "only"
userspace binary which might need to change is ir-keytable. Userspace
programs which simply consume input events (i.e. the vast majority)
won't have to change.

I haven't tested the patch yet, consider it a basis for discussion.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ati_remote.c |1 
 drivers/media/rc/imon.c   |7 +
 drivers/media/rc/rc-main.c|  219 -
 include/media/rc-core.h   |   26 -
 include/media/rc-map.h|5 +
 5 files changed, 202 insertions(+), 56 deletions(-)

diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
index 9cf3e69de16a..cc81b938471f 100644
--- a/drivers/media/rc/ati_remote.c
+++ b/drivers/media/rc/ati_remote.c
@@ -546,6 +546,7 @@ static void ati_remote_input_report(struct urb *urb)
 * set, assume this is a scrollwheel up/down event.
 */
wheel_keycode = rc_g_keycode_from_table(ati_remote->rdev,
+   RC_TYPE_OTHER,
scancode & 0x78);
 
if (wheel_keycode == KEY_RESERVED) {
diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 3489010601b5..c724a1a5e9cd 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -1274,14 +1274,15 @@ static u32 imon_remote_key_lookup(struct imon_context 
*ictx, u32 scancode)
bool is_release_code = false;
 
/* Look for the initial press of a button */
-   keycode = rc_g_keycode_from_table(ictx->rdev, scancode);
+   keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type, scancode);
ictx->rc_toggle = 0x0;
ictx->rc_scancode = scancode;
 
/* Look for the release of a button */
if (keycode == KEY_RESERVED) {
release = scancode & ~0x4000;
-   keycode = rc_g_keycode_

Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[David Härdeman]

On Fri, Apr 28, 2017 at 08:31:33AM -0300, Mauro Carvalho Chehab wrote:
>Em Thu, 27 Apr 2017 22:34:23 +0200
>David Härdeman <da...@hardeman.nu> escreveu:
...
>> This patch changes how the "input_keymap_entry" struct is interpreted
>> by rc-core by casting it to "rc_keymap_entry":
>> 
>> struct rc_scancode {
>>  __u16 protocol;
>>  __u16 reserved[3];
>>  __u64 scancode;
>> }
>> 
>> struct rc_keymap_entry {
>>  __u8  flags;
>>  __u8  len;
>>  __u16 index;
>>  __u32 keycode;
>>  union {
>>  struct rc_scancode rc;
>>  __u8 raw[32];
>>  };
>> };
>> 
...
>
>Nack.

That's not a very constructive approach. If you have a better approach
in mind I'm all ears. Because you're surely not suggesting that we stay
with the current protocol-less approach forever?

>No userspace breakages are allowed.

That's a gross oversimplification. A cursory glance at the linux-api
mailing list shows plenty of examples of changes that might not be 100%
backwards-compatible. Here's an example:
http://marc.info/?l=linux-fsdevel=149089166918069

That's the kind of discussion we need to have - i.e. the best way to go
about this and to minimize the damage to userspace. In that vein, I'll
post an alternative approach shortly as the basis for further
discussion.

>There's no way to warrant that
>ir-keytable version is compatible with a certain Kernel version.

I know. But we know when an ioctl() is made whether it is a
protocol-aware one or not.

-- 
David Härdeman

Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[David Härdeman]

On Fri, Apr 28, 2017 at 12:40:53PM +0100, Sean Young wrote:
>On Thu, Apr 27, 2017 at 10:34:23PM +0200, David Härdeman wrote:
...
>> This patch changes how the "input_keymap_entry" struct is interpreted
>> by rc-core by casting it to "rc_keymap_entry":
>> 
>> struct rc_scancode {
>>  __u16 protocol;
>>  __u16 reserved[3];
>>  __u64 scancode;
>> }
>> 
>> struct rc_keymap_entry {
>>  __u8  flags;
>>  __u8  len;
>>  __u16 index;
>>  __u32 keycode;
>>  union {
>>  struct rc_scancode rc;
>>  __u8 raw[32];
>>  };
>> };
>> 
>> The u64 scancode member is large enough for all current protocols and it
>> would be possible to extend it in the future should it be necessary for
>> some exotic protocol.
>> 
>> The main advantage with this change is that the protocol is made explicit,
>> which means that we're not throwing away data (the protocol type).
>> 
>> This also means that struct rc_map no longer hardcodes the protocol, meaning
>> that keytables with mixed entries are possible.
>> 
>> Heuristics are also added to hopefully do the right thing with older
>> ioctls in order to preserve backwards compatibility.
>
>The current ioctls do not provide any protocol information, so they should
>continue to match any protocol. Those heuristics aren't good enough.
>
>Another way of doing is to have a bitmask of protocols, and default to
>RC_BIT_ALL for current ioctls.

I've been mulling that approach as well, but slightly different. My
alternative approach is based on repurposing RC_TYPE_UNKNOWN as a kind
of catch-all which will match any scancode. I'll post a patch showing
the alternative approach straight away.

>> Note that the heuristics are not 100% guaranteed to get things right.
>> That is unavoidable since the protocol information simply isn't there
>> when userspace calls the previous ioctl() types.
>> 
>> However, that is somewhat mitigated by the fact that the "only"
>> userspace binary which might need to change is ir-keytable. Userspace
>> programs which simply consume input events (i.e. the vast majority)
>> won't have to change.
>
>For this to be accepted we would need ir-keytable changes too so it can
>be tested.

I know. But I'll postpone those patches until we have more of a
consensus on the right approach to take.

-- 
David Härdeman

Re: [PATCH 5/6] rc-core: use the full 32 bits for NEC scancodes

[David Härdeman]

On Fri, Apr 28, 2017 at 12:58:32PM +0100, Sean Young wrote:
>On Thu, Apr 27, 2017 at 10:34:18PM +0200, David Härdeman wrote:
>> Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
>> and the nec decoder without any loss of functionality. At the same time
>> it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and
>> removes lots of duplication (as you can see from the patch, the same NEC
>> disambiguation logic is contained in several different drivers).
>> 
>> Using NEC32 also removes ambiguity. For example, consider these two NEC
>> messages:
>> NEC16 message to address 0x05, command 0x03
>> NEC24 message to address 0x0005, command 0x03
>> 
>> They'll both have scancode 0x0503, and there's no way to tell which
>> message was received.
>
>It's not ambiguous, the protocol is different (RC_TYPE_NEC vs RC_TYPE_NECX).

It's ambigous in any context where the protocol is not known (e.g. when
old-style ioctl():s are performed) or in contexts where protocols are
bundled (i.e. when the only information about protocols come from the
sysfs file).

Anyway, I'm very open to leaving NEC well alone if that means we can
make some progress on the more important issue of protocol-enabled
keytables. :)

>> In order to maintain backwards compatibility, some heuristics are added
>> in rc-main.c to convert scancodes to NEC32 as necessary when userspace
>> adds entries to the keytable using the regular input ioctls. These
>> heuristics are essentially the same as the ones that are currently in
>> drivers/media/rc/img-ir/img-ir-nec.c (which are rendered unecessary
>> with this patch).
>
>There are issues with the patch which breaks userspace, as discussed
>in the previous patch. None of those issues have been addressed.

It is impossible to add protocol information without affecting
userspace, what we should be focusing on is the best way to ameliorate
the effects.

As a simple example, consider new-style ioctls doing:

EVIOCSKEYCODE_V2 (SONY, 0x001f) = KEY_NUMERIC_2
EVIOCSKEYCODE_V2 (SANYO, 0x001f) = KEY_NUMERIC_1

After that, what should these two ioctl():s perform/return?:

EVIOCGKEYCODE (0x001f) -> ?
EVIOCSKEYCODE (0x001f) = KEY_*

-- 
David Härdeman

[PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl

[David Härdeman]

It is currently impossible to distinguish between scancodes which have
been generated using different protocols (and scancodes can, and will,
overlap).

For example:
RC5 message to address 0x00, command 0x03 has scancode 0x0503
JVC message to address 0x00, command 0x03 has scancode 0x0503

It is only possible to distinguish (and parse) scancodes by known the
scancode *and* the protocol.

Setting and getting keycodes in the input subsystem used to be done via
the EVIOC[GS]KEYCODE ioctl and "unsigned int[2]" (one int for scancode
and one for the keycode).

The interface has now been extended to use the EVIOC[GS]KEYCODE_V2 ioctl
which uses the following struct:

struct input_keymap_entry {
__u8  flags;
__u8  len;
__u16 index;
__u32 keycode;
__u8  scancode[32];
};

(scancode can of course be even bigger, thanks to the len member).

This patch changes how the "input_keymap_entry" struct is interpreted
by rc-core by casting it to "rc_keymap_entry":

struct rc_scancode {
__u16 protocol;
__u16 reserved[3];
__u64 scancode;
}

struct rc_keymap_entry {
__u8  flags;
__u8  len;
__u16 index;
__u32 keycode;
union {
struct rc_scancode rc;
__u8 raw[32];
};
};

The u64 scancode member is large enough for all current protocols and it
would be possible to extend it in the future should it be necessary for
some exotic protocol.

The main advantage with this change is that the protocol is made explicit,
which means that we're not throwing away data (the protocol type).

This also means that struct rc_map no longer hardcodes the protocol, meaning
that keytables with mixed entries are possible.

Heuristics are also added to hopefully do the right thing with older
ioctls in order to preserve backwards compatibility.

Note that the heuristics are not 100% guaranteed to get things right.
That is unavoidable since the protocol information simply isn't there
when userspace calls the previous ioctl() types.

However, that is somewhat mitigated by the fact that the "only"
userspace binary which might need to change is ir-keytable. Userspace
programs which simply consume input events (i.e. the vast majority)
won't have to change.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/ati_remote.c |1 
 drivers/media/rc/imon.c   |7 +-
 drivers/media/rc/rc-main.c|  188 +
 include/media/rc-core.h   |   26 +-
 include/media/rc-map.h|5 +
 5 files changed, 165 insertions(+), 62 deletions(-)

diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
index 9cf3e69de16a..cc81b938471f 100644
--- a/drivers/media/rc/ati_remote.c
+++ b/drivers/media/rc/ati_remote.c
@@ -546,6 +546,7 @@ static void ati_remote_input_report(struct urb *urb)
 * set, assume this is a scrollwheel up/down event.
 */
wheel_keycode = rc_g_keycode_from_table(ati_remote->rdev,
+   RC_TYPE_OTHER,
scancode & 0x78);
 
if (wheel_keycode == KEY_RESERVED) {
diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 3489010601b5..c724a1a5e9cd 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -1274,14 +1274,15 @@ static u32 imon_remote_key_lookup(struct imon_context 
*ictx, u32 scancode)
bool is_release_code = false;
 
/* Look for the initial press of a button */
-   keycode = rc_g_keycode_from_table(ictx->rdev, scancode);
+   keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type, scancode);
ictx->rc_toggle = 0x0;
ictx->rc_scancode = scancode;
 
/* Look for the release of a button */
if (keycode == KEY_RESERVED) {
release = scancode & ~0x4000;
-   keycode = rc_g_keycode_from_table(ictx->rdev, release);
+   keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type,
+ release);
if (keycode != KEY_RESERVED)
is_release_code = true;
}
@@ -1310,7 +1311,7 @@ static u32 imon_mce_key_lookup(struct imon_context *ictx, 
u32 scancode)
scancode = scancode | MCE_KEY_MASK | MCE_TOGGLE_BIT;
 
ictx->rc_scancode = scancode;
-   keycode = rc_g_keycode_from_table(ictx->rdev, scancode);
+   keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type, scancode);
 
/* not used in mce mode, but make sure we know its false */
ictx->release_code = false;
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 881af208a19a..ad5545301588 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers

[PATCH 4/6] rc-core: use the full 32 bits for NEC scancodes in wakefilters

[David Härdeman]

The new sysfs wakefilter API will expose the difference between the NEC
protocols to userspace for no good reason and once exposed, it will be much
more difficult to change the logic.

By only allowing full NEC32 scancodes to be set, any heuristics in the kernel
can be avoided.

This is a minor preparation for the next patch which moves the rest of
rc-core over to only using NEC32.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/rc-main.c |   17 -
 drivers/media/rc/winbond-cir.c |   32 ++--
 2 files changed, 6 insertions(+), 43 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 0acc8f27abeb..8355f86a460b 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -741,8 +741,6 @@ static int rc_validate_filter(struct rc_dev *dev,
[RC_TYPE_SONY15] = 0xff007f,
[RC_TYPE_SONY20] = 0x1fff7f,
[RC_TYPE_JVC] = 0x,
-   [RC_TYPE_NEC] = 0x,
-   [RC_TYPE_NECX] = 0xff,
[RC_TYPE_NEC32] = 0x,
[RC_TYPE_SANYO] = 0x1f,
[RC_TYPE_MCIR2_KBD] = 0x,
@@ -758,14 +756,9 @@ static int rc_validate_filter(struct rc_dev *dev,
enum rc_type protocol = dev->wakeup_protocol;
 
switch (protocol) {
+   case RC_TYPE_NEC:
case RC_TYPE_NECX:
-   if s >> 16) ^ ~(s >> 8)) & 0xff) == 0)
-   return -EINVAL;
-   break;
-   case RC_TYPE_NEC32:
-   if s >> 24) ^ ~(s >> 16)) & 0xff) == 0)
-   return -EINVAL;
-   break;
+   return -EINVAL;
case RC_TYPE_RC6_MCE:
if ((s & 0x) != 0x800f)
return -EINVAL;
@@ -1301,7 +1294,7 @@ static ssize_t store_filter(struct device *device,
 /*
  * This is the list of all variants of all protocols, which is used by
  * the wakeup_protocols sysfs entry. In the protocols sysfs entry some
- * some protocols are grouped together (e.g. nec = nec + necx + nec32).
+ * some protocols are grouped together.
  *
  * For wakeup we need to know the exact protocol variant so the hardware
  * can be programmed exactly what to expect.
@@ -1316,9 +1309,7 @@ static const char * const proto_variant_names[] = {
[RC_TYPE_SONY12] = "sony-12",
[RC_TYPE_SONY15] = "sony-15",
[RC_TYPE_SONY20] = "sony-20",
-   [RC_TYPE_NEC] = "nec",
-   [RC_TYPE_NECX] = "nec-x",
-   [RC_TYPE_NEC32] = "nec-32",
+   [RC_TYPE_NEC32] = "nec",
[RC_TYPE_SANYO] = "sanyo",
[RC_TYPE_MCIR2_KBD] = "mcir2-kbd",
[RC_TYPE_MCIR2_MSE] = "mcir2-mse",
diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 5a4d4a611197..6ef0e7232356 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -714,34 +714,6 @@ wbcir_shutdown(struct pnp_dev *device)
proto = IR_PROTOCOL_RC5;
break;
 
-   case RC_TYPE_NEC:
-   mask[1] = bitrev8(mask_sc);
-   mask[0] = mask[1];
-   mask[3] = bitrev8(mask_sc >> 8);
-   mask[2] = mask[3];
-
-   match[1] = bitrev8(wake_sc);
-   match[0] = ~match[1];
-   match[3] = bitrev8(wake_sc >> 8);
-   match[2] = ~match[3];
-
-   proto = IR_PROTOCOL_NEC;
-   break;
-
-   case RC_TYPE_NECX:
-   mask[1] = bitrev8(mask_sc);
-   mask[0] = mask[1];
-   mask[2] = bitrev8(mask_sc >> 8);
-   mask[3] = bitrev8(mask_sc >> 16);
-
-   match[1] = bitrev8(wake_sc);
-   match[0] = ~match[1];
-   match[2] = bitrev8(wake_sc >> 8);
-   match[3] = bitrev8(wake_sc >> 16);
-
-   proto = IR_PROTOCOL_NEC;
-   break;
-
case RC_TYPE_NEC32:
mask[0] = bitrev8(mask_sc);
mask[1] = bitrev8(mask_sc >> 8);
@@ -1087,8 +1059,8 @@ wbcir_probe(struct pnp_dev *device, const struct 
pnp_device_id *dev_id)
data->dev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
data->dev->rx_resolution = US_TO_NS(2);
data->dev->allowed_protocols = RC_BIT_ALL_IR_DECODER;
-   data->dev->allowed_wakeup_protocols = RC_BIT_NEC | RC_BIT_NECX |
-   RC_BIT_NEC32 | RC_BIT_RC5 | RC_BIT_RC6_0 |
+   data->dev->allowed_wakeup_protocols =
+   RC_BIT_NEC | RC_BIT_RC5 | RC_BIT_RC6_0 |
RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 |
RC_BIT_RC6_6A_32 | RC_BIT_RC6_MCE;
data->dev->wakeup_protocol = RC_TYPE_RC6_MCE;

[PATCH 5/6] rc-core: use the full 32 bits for NEC scancodes

[David Härdeman]

Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
and the nec decoder without any loss of functionality. At the same time
it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and
removes lots of duplication (as you can see from the patch, the same NEC
disambiguation logic is contained in several different drivers).

Using NEC32 also removes ambiguity. For example, consider these two NEC
messages:
NEC16 message to address 0x05, command 0x03
NEC24 message to address 0x0005, command 0x03

They'll both have scancode 0x0503, and there's no way to tell which
message was received.

In order to maintain backwards compatibility, some heuristics are added
in rc-main.c to convert scancodes to NEC32 as necessary when userspace
adds entries to the keytable using the regular input ioctls. These
heuristics are essentially the same as the ones that are currently in
drivers/media/rc/img-ir/img-ir-nec.c (which are rendered unecessary
with this patch).

While this is a change which cannot be 100% sure to be backwards
compatible, it should be noted that heuristics and such breakage cannot
be avoided when we introduce the protocol into the keytable
(see next patch).

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/pci/cx88/cx88-input.c   |4 +
 drivers/media/pci/saa7134/saa7134-input.c |4 +
 drivers/media/rc/igorplugusb.c|4 +
 drivers/media/rc/img-ir/img-ir-nec.c  |   92 ++---
 drivers/media/rc/ir-nec-decoder.c |   63 +++-
 drivers/media/rc/rc-main.c|   65 +---
 drivers/media/rc/winbond-cir.c|2 -
 drivers/media/usb/au0828/au0828-input.c   |3 -
 drivers/media/usb/dvb-usb-v2/af9015.c |   30 ++---
 drivers/media/usb/dvb-usb-v2/af9035.c |   27 ++---
 drivers/media/usb/dvb-usb-v2/az6007.c |   25 ++--
 drivers/media/usb/dvb-usb-v2/lmedm04.c|5 +-
 drivers/media/usb/dvb-usb-v2/rtl28xxu.c   |   29 ++---
 drivers/media/usb/dvb-usb/dib0700_core.c  |   25 ++--
 drivers/media/usb/dvb-usb/dtt200u.c   |   25 ++--
 drivers/media/usb/em28xx/em28xx-input.c   |   22 ++-
 include/media/rc-map.h|   48 ---
 17 files changed, 162 insertions(+), 311 deletions(-)

diff --git a/drivers/media/pci/cx88/cx88-input.c 
b/drivers/media/pci/cx88/cx88-input.c
index 01f2e472a2a0..61c46763ac97 100644
--- a/drivers/media/pci/cx88/cx88-input.c
+++ b/drivers/media/pci/cx88/cx88-input.c
@@ -146,7 +146,7 @@ static void cx88_ir_handle_key(struct cx88_IR *ir)
scancode = RC_SCANCODE_NECX(addr, cmd);
 
if (0 == (gpio & ir->mask_keyup))
-   rc_keydown_notimeout(ir->dev, RC_TYPE_NECX, scancode,
+   rc_keydown_notimeout(ir->dev, RC_TYPE_NEC, scancode,
 0);
else
rc_keyup(ir->dev);
@@ -348,7 +348,7 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev 
*pci)
 * 002-T mini RC, provided with newer PV hardware
 */
ir_codes = RC_MAP_PIXELVIEW_MK12;
-   rc_type = RC_BIT_NECX;
+   rc_type = RC_BIT_NEC;
ir->gpio_addr = MO_GP1_IO;
ir->mask_keyup = 0x80;
ir->polling = 10; /* ms */
diff --git a/drivers/media/pci/saa7134/saa7134-input.c 
b/drivers/media/pci/saa7134/saa7134-input.c
index 78849c19f68a..47d8e055ddd3 100644
--- a/drivers/media/pci/saa7134/saa7134-input.c
+++ b/drivers/media/pci/saa7134/saa7134-input.c
@@ -338,7 +338,7 @@ static int get_key_beholdm6xx(struct IR_i2c *ir, enum 
rc_type *protocol,
if (data[9] != (unsigned char)(~data[8]))
return 0;
 
-   *protocol = RC_TYPE_NECX;
+   *protocol = RC_TYPE_NEC;
*scancode = RC_SCANCODE_NECX(data[11] << 8 | data[10], data[9]);
*toggle = 0;
return 1;
@@ -1028,7 +1028,7 @@ void saa7134_probe_i2c_ir(struct saa7134_dev *dev)
dev->init_data.name = "BeholdTV";
dev->init_data.get_key = get_key_beholdm6xx;
dev->init_data.ir_codes = RC_MAP_BEHOLD;
-   dev->init_data.type = RC_BIT_NECX;
+   dev->init_data.type = RC_BIT_NEC;
info.addr = 0x2d;
break;
case SAA7134_BOARD_AVERMEDIA_CARDBUS_501:
diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
index cb6d4f1247da..9e3119c94368 100644
--- a/drivers/media/rc/igorplugusb.c
+++ b/drivers/media/rc/igorplugusb.c
@@ -203,8 +203,8 @@ static int igorplugusb_probe(struct usb_interface *intf,
 * for the NEC protocol and many others.
 */
rc->allowed_protocols = RC_BIT_ALL_IR_DECODER & ~(RC_BIT_NEC |
-   RC_BIT_NECX | RC_BIT_NEC32 | RC_BIT_RC6_6A_20 |
-  

[PATCH 0/6] rc-core - protocol in keytables

[David Härdeman]

The first three patches are just some cleanups that I noticed
while working on the other patches.

The fourth and fifth patch change rc-core over to use NEC32
scancodes everywhere. That might seem like a recipe for
breaking userspace...but, as you'll see with the sixth patch,
we can't really avoid it if we want to improve the
EVIOC[GS]KEYCODE_V2 ioctl():s to support protocols.

And since it was requested last time around, I have written
a much longer explanation of the NEC32/ioctl patches and
posted here (with pretty pictures):
https://david.hardeman.nu/rccore/

Most of you might want to skip the introduction part :)

---

David Härdeman (6):
  rc-core: fix input repeat handling
  rc-core: cleanup rc_register_device
  rc-core: cleanup rc_register_device pt2
  rc-core: use the full 32 bits for NEC scancodes in wakefilters
  rc-core: use the full 32 bits for NEC scancodes
  rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl


 drivers/media/pci/cx88/cx88-input.c   |4 
 drivers/media/pci/saa7134/saa7134-input.c |4 
 drivers/media/rc/ati_remote.c |1 
 drivers/media/rc/igorplugusb.c|4 
 drivers/media/rc/img-ir/img-ir-nec.c  |   92 +--
 drivers/media/rc/imon.c   |7 -
 drivers/media/rc/ir-nec-decoder.c |   63 +
 drivers/media/rc/rc-core-priv.h   |2 
 drivers/media/rc/rc-ir-raw.c  |   34 ++
 drivers/media/rc/rc-main.c|  406 ++---
 drivers/media/rc/winbond-cir.c|   32 --
 drivers/media/usb/au0828/au0828-input.c   |3 
 drivers/media/usb/dvb-usb-v2/af9015.c |   30 --
 drivers/media/usb/dvb-usb-v2/af9035.c |   27 --
 drivers/media/usb/dvb-usb-v2/az6007.c |   25 --
 drivers/media/usb/dvb-usb-v2/lmedm04.c|5 
 drivers/media/usb/dvb-usb-v2/rtl28xxu.c   |   29 +-
 drivers/media/usb/dvb-usb/dib0700_core.c  |   25 --
 drivers/media/usb/dvb-usb/dtt200u.c   |   25 +-
 drivers/media/usb/em28xx/em28xx-input.c   |   22 --
 include/media/rc-core.h   |   28 ++
 include/media/rc-map.h|   53 ++--
 22 files changed, 412 insertions(+), 509 deletions(-)

--
David Härdeman

[PATCH 1/6] rc-core: fix input repeat handling

[David Härdeman]

The call to input_register_device() needs to take place
before the repeat parameters are set or the input subsystem
repeat handling will be disabled (as was already noted in
the comments in that function).

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/rc-main.c |   20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 6ec73357fa47..802e559cc30e 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1703,6 +1703,16 @@ static int rc_setup_rx_device(struct rc_dev *dev)
if (dev->close)
dev->input_dev->close = ir_close;
 
+   dev->input_dev->dev.parent = >dev;
+   memcpy(>input_dev->id, >input_id, sizeof(dev->input_id));
+   dev->input_dev->phys = dev->input_phys;
+   dev->input_dev->name = dev->input_name;
+
+   /* rc_open will be called here */
+   rc = input_register_device(dev->input_dev);
+   if (rc)
+   goto out_table;
+
/*
 * Default delay of 250ms is too short for some protocols, especially
 * since the timeout is currently set to 250ms. Increase it to 500ms,
@@ -1718,16 +1728,6 @@ static int rc_setup_rx_device(struct rc_dev *dev)
 */
dev->input_dev->rep[REP_PERIOD] = 125;
 
-   dev->input_dev->dev.parent = >dev;
-   memcpy(>input_dev->id, >input_id, sizeof(dev->input_id));
-   dev->input_dev->phys = dev->input_phys;
-   dev->input_dev->name = dev->input_name;
-
-   /* rc_open will be called here */
-   rc = input_register_device(dev->input_dev);
-   if (rc)
-   goto out_table;
-
return 0;
 
 out_table:

[PATCH 3/6] rc-core: cleanup rc_register_device pt2

[David Härdeman]

Now that rc_register_device() is reorganised, the dev->initialized
hack can be removed. Any driver which calls rc_register_device()
must be prepared for the device to go live immediately.

The dev->initialized commits that are relevant are:
c73bbaa4ec3eb225ffe468f80d45724d0496bf03
08aeb7c9a42ab7aa8b53c8f7779ec58f860a565c

The original problem was that show_protocols() would access
dev->rc_map.* and various other bits which are now properly
initialized before device_add() is called.

At the same time, remove  the bogus "device is being removed"
check (quiz: when would container_of give a NULL value???).

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/rc-main.c |   67 +++-
 include/media/rc-core.h|2 -
 2 files changed, 10 insertions(+), 59 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 44189366f232..0acc8f27abeb 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -15,7 +15,6 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -934,8 +933,8 @@ static bool lirc_is_present(void)
  * It returns the protocol names of supported protocols.
  * Enabled protocols are printed in brackets.
  *
- * dev->lock is taken to guard against races between device
- * registration, store_protocols and show_protocols.
+ * dev->lock is taken to guard against races between
+ * store_protocols and show_protocols.
  */
 static ssize_t show_protocols(struct device *device,
  struct device_attribute *mattr, char *buf)
@@ -945,13 +944,6 @@ static ssize_t show_protocols(struct device *device,
char *tmp = buf;
int i;
 
-   /* Device is being removed */
-   if (!dev)
-   return -EINVAL;
-
-   if (!atomic_read(>initialized))
-   return -ERESTARTSYS;
-
mutex_lock(>lock);
 
enabled = dev->enabled_protocols;
@@ -1106,8 +1098,8 @@ static void ir_raw_load_modules(u64 *protocols)
  * See parse_protocol_change() for the valid commands.
  * Returns @len on success or a negative error code.
  *
- * dev->lock is taken to guard against races between device
- * registration, store_protocols and show_protocols.
+ * dev->lock is taken to guard against races between
+ * store_protocols and show_protocols.
  */
 static ssize_t store_protocols(struct device *device,
   struct device_attribute *mattr,
@@ -1119,13 +,6 @@ static ssize_t store_protocols(struct device *device,
u64 old_protocols, new_protocols;
ssize_t rc;
 
-   /* Device is being removed */
-   if (!dev)
-   return -EINVAL;
-
-   if (!atomic_read(>initialized))
-   return -ERESTARTSYS;
-
IR_dprintk(1, "Normal protocol change requested\n");
current_protocols = >enabled_protocols;
filter = >scancode_filter;
@@ -1200,7 +1185,7 @@ static ssize_t store_protocols(struct device *device,
  * Bits of the filter value corresponding to set bits in the filter mask are
  * compared against input scancodes and non-matching scancodes are discarded.
  *
- * dev->lock is taken to guard against races between device registration,
+ * dev->lock is taken to guard against races between
  * store_filter and show_filter.
  */
 static ssize_t show_filter(struct device *device,
@@ -1212,13 +1197,6 @@ static ssize_t show_filter(struct device *device,
struct rc_scancode_filter *filter;
u32 val;
 
-   /* Device is being removed */
-   if (!dev)
-   return -EINVAL;
-
-   if (!atomic_read(>initialized))
-   return -ERESTARTSYS;
-
mutex_lock(>lock);
 
if (fattr->type == RC_FILTER_NORMAL)
@@ -1251,7 +1229,7 @@ static ssize_t show_filter(struct device *device,
  * Bits of the filter value corresponding to set bits in the filter mask are
  * compared against input scancodes and non-matching scancodes are discarded.
  *
- * dev->lock is taken to guard against races between device registration,
+ * dev->lock is taken to guard against races between
  * store_filter and show_filter.
  */
 static ssize_t store_filter(struct device *device,
@@ -1265,13 +1243,6 @@ static ssize_t store_filter(struct device *device,
unsigned long val;
int (*set_filter)(struct rc_dev *dev, struct rc_scancode_filter 
*filter);
 
-   /* Device is being removed */
-   if (!dev)
-   return -EINVAL;
-
-   if (!atomic_read(>initialized))
-   return -ERESTARTSYS;
-
ret = kstrtoul(buf, 0, );
if (ret < 0)
return ret;
@@ -1372,8 +1343,8 @@ static const char * const proto_variant_names[] = {
  * It returns the protocol names of supported protocols.
  * The enabled protocols are printed in brackets.
  *
- * dev->lock is 

[PATCH 2/6] rc-core: cleanup rc_register_device

[David Härdeman]

The device core infrastructure is based on the presumption that
once a driver calls device_add(), it must be ready to accept
userspace interaction.

This requires splitting rc_setup_rx_device() into two functions
and reorganizing rc_register_device() so that as much work
as possible is performed before calling device_add().

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/rc-core-priv.h |2 +
 drivers/media/rc/rc-ir-raw.c|   34 --
 drivers/media/rc/rc-main.c  |   75 +--
 3 files changed, 73 insertions(+), 38 deletions(-)

diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index 0455b273c2fc..b3e7cac2c3ee 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -263,7 +263,9 @@ int ir_raw_gen_pl(struct ir_raw_event **ev, unsigned int 
max,
  * Routines from rc-raw.c to be used internally and by decoders
  */
 u64 ir_raw_get_allowed_protocols(void);
+int ir_raw_event_prepare(struct rc_dev *dev);
 int ir_raw_event_register(struct rc_dev *dev);
+void ir_raw_event_free(struct rc_dev *dev);
 void ir_raw_event_unregister(struct rc_dev *dev);
 int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler);
 void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler);
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 90f66dc7c0d7..ae7785c4fbe7 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -486,14 +486,18 @@ EXPORT_SYMBOL(ir_raw_encode_scancode);
 /*
  * Used to (un)register raw event clients
  */
-int ir_raw_event_register(struct rc_dev *dev)
+int ir_raw_event_prepare(struct rc_dev *dev)
 {
-   int rc;
-   struct ir_raw_handler *handler;
+   static bool raw_init; /* 'false' default value, raw decoders loaded? */
 
if (!dev)
return -EINVAL;
 
+   if (!raw_init) {
+   request_module("ir-lirc-codec");
+   raw_init = true;
+   }
+
dev->raw = kzalloc(sizeof(*dev->raw), GFP_KERNEL);
if (!dev->raw)
return -ENOMEM;
@@ -502,6 +506,13 @@ int ir_raw_event_register(struct rc_dev *dev)
dev->change_protocol = change_protocol;
INIT_KFIFO(dev->raw->kfifo);
 
+   return 0;
+}
+
+int ir_raw_event_register(struct rc_dev *dev)
+{
+   struct ir_raw_handler *handler;
+
/*
 * raw transmitters do not need any event registration
 * because the event is coming from userspace
@@ -510,10 +521,8 @@ int ir_raw_event_register(struct rc_dev *dev)
dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
   "rc%u", dev->minor);
 
-   if (IS_ERR(dev->raw->thread)) {
-   rc = PTR_ERR(dev->raw->thread);
-   goto out;
-   }
+   if (IS_ERR(dev->raw->thread))
+   return PTR_ERR(dev->raw->thread);
}
 
mutex_lock(_raw_handler_lock);
@@ -524,11 +533,15 @@ int ir_raw_event_register(struct rc_dev *dev)
mutex_unlock(_raw_handler_lock);
 
return 0;
+}
+
+void ir_raw_event_free(struct rc_dev *dev)
+{
+   if (!dev)
+   return;
 
-out:
kfree(dev->raw);
dev->raw = NULL;
-   return rc;
 }
 
 void ir_raw_event_unregister(struct rc_dev *dev)
@@ -547,8 +560,7 @@ void ir_raw_event_unregister(struct rc_dev *dev)
handler->raw_unregister(dev);
mutex_unlock(_raw_handler_lock);
 
-   kfree(dev->raw);
-   dev->raw = NULL;
+   ir_raw_event_free(dev);
 }
 
 /*
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 802e559cc30e..44189366f232 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1663,7 +1663,7 @@ struct rc_dev *devm_rc_allocate_device(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_rc_allocate_device);
 
-static int rc_setup_rx_device(struct rc_dev *dev)
+static int rc_prepare_rx_device(struct rc_dev *dev)
 {
int rc;
struct rc_map *rc_map;
@@ -1708,10 +1708,22 @@ static int rc_setup_rx_device(struct rc_dev *dev)
dev->input_dev->phys = dev->input_phys;
dev->input_dev->name = dev->input_name;
 
+   return 0;
+
+out_table:
+   ir_free_table(>rc_map);
+
+   return rc;
+}
+
+static int rc_setup_rx_device(struct rc_dev *dev)
+{
+   int rc;
+
/* rc_open will be called here */
rc = input_register_device(dev->input_dev);
if (rc)
-   goto out_table;
+   return rc;
 
/*
 * Default delay of 250ms is too short for some protocols, especially
@@ -1729,27 +1741,23 @@ static int rc_setup_rx_device(struct rc_dev *dev)
dev->input_dev->rep[REP_PERIOD] = 125;
 
r

Re: [PATCH] rc-core: use the full 32 bits for NEC scancodes

[David Härdeman]

April 24, 2017 5:58 PM, "Sean Young" <s...@mess.org> wrote:
> On Tue, Apr 18, 2017 at 05:50:27PM +0200, David Härdeman wrote:
>> Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
>> and the nec decoder without any loss of functionality. At the same time
>> it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and
>> removes lots of duplication (as you can see from the patch, the same NEC
>> disambiguation logic is contained in several different drivers).
>> 
>> Using NEC32 also removes ambiguity. For example, consider these two NEC
>> messages:
>> NEC16 message to address 0x05, command 0x03
>> NEC24 message to address 0x0005, command 0x03
>> 
>> They'll both have scancode 0x0503, and there's no way to tell which
>> message was received.
> 
> More precisely, there is no way to tell which protocol variant it was sent
> with.

Oh, but there is. The driver/rc-core will know. It's just that userspace cannot 
ever know.

> With the Sony and rc6 protocols, you can also get the same scancode from
> different protocol variants. I think the right solution is to pass the 
> protocol
> variant to user space (and the keymap mapper).

Yes, I'm working on refreshing my patches to add a new 
EVIOCGKEYCODE_V2/EVIOCSKEYCODE_V2 ioctl which includes the protocol. And 
actually, those patches are greatly simplified by only using NEC32.
 
> This also solves some other problems, e.g. rc6_6a_20:0x75460 is also decoded
> by the sony protocol decoder (as scancode 0).

I know. And it also makes it possible to make /sys/class/rc/rc0/protocols fully 
automatic. And we could theoretically also refuse to set unsupported protocols 
in the keytable (not sure yet if that's something we should do).

>> In order to maintain backwards compatibility, some heuristics are added
>> in rc-main.c to convert scancodes to NEC32 as necessary when userspace
>> adds entries to the keytable using the regular input ioctls.
> 
> This is where it falls apart. In the patch below, you guess the protocol
> variant from the scancode value. By your own example above, nec24 with
> an address of 0x0005 would be not be possible in a keymap since it would
> guessed as nec16 (see to_nec32() below) and expanded to 0x05fb03fc. An
> actual nec24 would be 0x000503fc.

It's not 100% bulletproof. There's no way to fix this issue in a 100% backwards 
compatible manner. But the future EVIOCGKEYCODE_V2/EVIOCSKEYCODE_V2 ioctl would 
make the heuristics unnecessary.

>> These
>> heuristics are essentially the same as the ones that are currently in
>> drivers/media/rc/img-ir/img-ir-nec.c (which are rendered unecessary
>> with this patch).
> 
> Rendered unnecessary since you moved it to core code. You've changed the
> img-ir filter functionality in the process, breaking userspace.

I doubt it can be fixed in a way which doesn't involve some userspace changes. 
But the breakage can be minimized.

> This is the scancode filter in the img-ir which admittedly isn't great,
> so I don't think we should introduce it elsewhere. What would be much
> better would be if we could specify the protocol variants for ir decoding,
> rather than just "nec" or "sony" or "rc6". I'm not sure how to do this
> without breaking userspace though.

I think the best way is by introducing the new ioctls.

Anyway, I'll try to post the whole patchset later today, then it should be 
clearer why this is a good change.

>> The reason this has to be done now is that the newer sysfs wakefilter API
>> will expose the difference between the NEC protocols to userspace for no
>> good reason and once exposed, it will be much more difficult to change the
>> logic.
>> 
>> Signed-off-by: David Härdeman <da...@hardeman.nu>
>> ---
>> drivers/media/rc/igorplugusb.c | 4 +
>> drivers/media/rc/img-ir/img-ir-nec.c | 92 +++---
>> drivers/media/rc/ir-nec-decoder.c | 63 ---
>> drivers/media/rc/rc-main.c | 74 ---
>> drivers/media/rc/winbond-cir.c | 32 +---
>> 5 files changed, 89 insertions(+), 176 deletions(-)
>> 
>> diff --git a/drivers/media/pci/cx88/cx88-input.c 
>> b/drivers/media/pci/cx88/cx88-input.c
>> index 01f2e472a2a0..61c46763ac97 100644
>> --- a/drivers/media/pci/cx88/cx88-input.c
>> +++ b/drivers/media/pci/cx88/cx88-input.c
>> @@ -146,7 +146,7 @@ static void cx88_ir_handle_key(struct cx88_IR *ir)
>> scancode = RC_SCANCODE_NECX(addr, cmd);
>> 
>> if (0 == (gpio & ir->mask_keyup))
>> - rc_keydown_notimeout(ir->dev, RC_TYPE_NECX, scancode,
>> + rc_keydown_notimeout(ir->dev, RC_TYPE_NEC, scancode,
>> 0);

Re: [PATCH] rc-core: use the full 32 bits for NEC scancodes in wakefilters

[David Härdeman]

April 24, 2017 6:02 PM, "Sean Young" <s...@mess.org> wrote:

> On Tue, Apr 18, 2017 at 10:31:04PM +0200, David Härdeman wrote:
> 
>> The new sysfs wakefilter API will expose the difference between the NEC
>> protocols to userspace for no good reason and once exposed, it will be much
>> more difficult to change the logic.
>> 
>> By only allowing full NEC32 scancodes to be set, any heuristics in the kernel
>> can be avoided.
> 
> No heuristics are being removed in this patch or the other patch for nec32,
> if anything it gets worse.

It avoids having to add heuristics in the future if we move to always use nec32 
in the kernel<->userspace API. That, IMHO, is the only sane default. Explicitly 
differentiating between NEC16/24/32 in the API provides no benefits whatsoever.

> This patch depends on the other patch, which needs work.

It's a minimal version of the other patch, not dependent on it. It just makes 
sure that the wakeup logic only supports nec32 before that API becomes 
official. 

>> This is the minimalistic version of the full NEC32 patch posted here:
>> http://www.spinics.net/lists/linux-media/msg114603.html
>> 
>> Signed-off-by: David Härdeman <da...@hardeman.nu>
>> ---
>> drivers/media/rc/rc-main.c | 17 -
>> drivers/media/rc/winbond-cir.c | 32 ++--
>> 2 files changed, 6 insertions(+), 43 deletions(-)
>> 
>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>> index 6ec73357fa47..8a2a2973e718 100644
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -742,8 +742,6 @@ static int rc_validate_filter(struct rc_dev *dev,
>> [RC_TYPE_SONY15] = 0xff007f,
>> [RC_TYPE_SONY20] = 0x1fff7f,
>> [RC_TYPE_JVC] = 0x,
>> - [RC_TYPE_NEC] = 0x,
>> - [RC_TYPE_NECX] = 0xff,
>> [RC_TYPE_NEC32] = 0x,
>> [RC_TYPE_SANYO] = 0x1f,
>> [RC_TYPE_MCIR2_KBD] = 0x,
>> @@ -759,14 +757,9 @@ static int rc_validate_filter(struct rc_dev *dev,
>> enum rc_type protocol = dev->wakeup_protocol;
>> 
>> switch (protocol) {
>> + case RC_TYPE_NEC:
>> case RC_TYPE_NECX:
>> - if s >> 16) ^ ~(s >> 8)) & 0xff) == 0)
>> - return -EINVAL;
>> - break;
>> - case RC_TYPE_NEC32:
>> - if s >> 24) ^ ~(s >> 16)) & 0xff) == 0)
>> - return -EINVAL;
>> - break;
>> + return -EINVAL;
>> case RC_TYPE_RC6_MCE:
>> if ((s & 0x) != 0x800f)
>> return -EINVAL;
>> @@ -1330,7 +1323,7 @@ static ssize_t store_filter(struct device *device,
>> /*
>> * This is the list of all variants of all protocols, which is used by
>> * the wakeup_protocols sysfs entry. In the protocols sysfs entry some
>> - * some protocols are grouped together (e.g. nec = nec + necx + nec32).
>> + * some protocols are grouped together.
>> *
>> * For wakeup we need to know the exact protocol variant so the hardware
>> * can be programmed exactly what to expect.
>> @@ -1345,9 +1338,7 @@ static const char * const proto_variant_names[] = {
>> [RC_TYPE_SONY12] = "sony-12",
>> [RC_TYPE_SONY15] = "sony-15",
>> [RC_TYPE_SONY20] = "sony-20",
>> - [RC_TYPE_NEC] = "nec",
>> - [RC_TYPE_NECX] = "nec-x",
>> - [RC_TYPE_NEC32] = "nec-32",
>> + [RC_TYPE_NEC32] = "nec",
>> [RC_TYPE_SANYO] = "sanyo",
>> [RC_TYPE_MCIR2_KBD] = "mcir2-kbd",
>> [RC_TYPE_MCIR2_MSE] = "mcir2-mse",
>> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
>> index 5a4d4a611197..6ef0e7232356 100644
>> --- a/drivers/media/rc/winbond-cir.c
>> +++ b/drivers/media/rc/winbond-cir.c
>> @@ -714,34 +714,6 @@ wbcir_shutdown(struct pnp_dev *device)
>> proto = IR_PROTOCOL_RC5;
>> break;
>> 
>> - case RC_TYPE_NEC:
>> - mask[1] = bitrev8(mask_sc);
>> - mask[0] = mask[1];
>> - mask[3] = bitrev8(mask_sc >> 8);
>> - mask[2] = mask[3];
>> -
>> - match[1] = bitrev8(wake_sc);
>> - match[0] = ~match[1];
>> - match[3] = bitrev8(wake_sc >> 8);
>> - match[2] = ~match[3];
>> -
>> - proto = IR_PROTOCOL_NEC;
>> - break;
>> -
>> - case RC_TYPE_NECX:
>> - mask[1] = bitrev8(mask_sc);
>> - mask[0] = mask[1];
>> - mask[2] = bitrev8(mask_sc >> 8);
>> - mask[3] = bitrev8(mask_sc >> 16);
>> -
>> - match[1] = bitrev8(wake_sc);
>> - match[0] = ~match[1];
>> - match[2] = bitrev8(wake_sc >> 8);
>> - match[3] = bitrev8(wake_sc >> 16);
>> -
>> - proto = IR_PROTOCOL_NEC;
>> - break;
>> -
>> case RC_TYPE_NEC32:
>> mask[0] = bitrev8(mask_sc);
>> mask[1] = bitrev8(mask_sc >> 8);
>> @@ -1087,8 +1059,8 @@ wbcir_probe(struct pnp_dev *device, const struct 
>> pnp_device_id *dev_id)
>> data->dev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
>> data->dev->rx_resolution = US_TO_NS(2);
>> data->dev->allowed_protocols = RC_BIT_ALL_IR_DECODER;
>> - data->dev->allowed_wakeup_protocols = RC_BIT_NEC | RC_BIT_NECX |
>> - RC_BIT_NEC32 | RC_BIT_RC5 | RC_BIT_RC6_0 |
>> + data->dev->allowed_wakeup_protocols =
>> + RC_BIT_NEC | RC_BIT_RC5 | RC_BIT_RC6_0 |
>> RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 |
>> RC_BIT_RC6_6A_32 | RC_BIT_RC6_MCE;
>> data->dev->wakeup_protocol = RC_TYPE_RC6_MCE;

[PATCH] rc-core: use the full 32 bits for NEC scancodes in wakefilters

[David Härdeman]

The new sysfs wakefilter API will expose the difference between the NEC
protocols to userspace for no good reason and once exposed, it will be much
more difficult to change the logic.

By only allowing full NEC32 scancodes to be set, any heuristics in the kernel
can be avoided.

This is the minimalistic version of the full NEC32 patch posted here:
http://www.spinics.net/lists/linux-media/msg114603.html

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/rc-main.c |   17 -
 drivers/media/rc/winbond-cir.c |   32 ++--
 2 files changed, 6 insertions(+), 43 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 6ec73357fa47..8a2a2973e718 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -742,8 +742,6 @@ static int rc_validate_filter(struct rc_dev *dev,
[RC_TYPE_SONY15] = 0xff007f,
[RC_TYPE_SONY20] = 0x1fff7f,
[RC_TYPE_JVC] = 0x,
-   [RC_TYPE_NEC] = 0x,
-   [RC_TYPE_NECX] = 0xff,
[RC_TYPE_NEC32] = 0x,
[RC_TYPE_SANYO] = 0x1f,
[RC_TYPE_MCIR2_KBD] = 0x,
@@ -759,14 +757,9 @@ static int rc_validate_filter(struct rc_dev *dev,
enum rc_type protocol = dev->wakeup_protocol;
 
switch (protocol) {
+   case RC_TYPE_NEC:
case RC_TYPE_NECX:
-   if s >> 16) ^ ~(s >> 8)) & 0xff) == 0)
-   return -EINVAL;
-   break;
-   case RC_TYPE_NEC32:
-   if s >> 24) ^ ~(s >> 16)) & 0xff) == 0)
-   return -EINVAL;
-   break;
+   return -EINVAL;
case RC_TYPE_RC6_MCE:
if ((s & 0x) != 0x800f)
return -EINVAL;
@@ -1330,7 +1323,7 @@ static ssize_t store_filter(struct device *device,
 /*
  * This is the list of all variants of all protocols, which is used by
  * the wakeup_protocols sysfs entry. In the protocols sysfs entry some
- * some protocols are grouped together (e.g. nec = nec + necx + nec32).
+ * some protocols are grouped together.
  *
  * For wakeup we need to know the exact protocol variant so the hardware
  * can be programmed exactly what to expect.
@@ -1345,9 +1338,7 @@ static const char * const proto_variant_names[] = {
[RC_TYPE_SONY12] = "sony-12",
[RC_TYPE_SONY15] = "sony-15",
[RC_TYPE_SONY20] = "sony-20",
-   [RC_TYPE_NEC] = "nec",
-   [RC_TYPE_NECX] = "nec-x",
-   [RC_TYPE_NEC32] = "nec-32",
+   [RC_TYPE_NEC32] = "nec",
[RC_TYPE_SANYO] = "sanyo",
[RC_TYPE_MCIR2_KBD] = "mcir2-kbd",
[RC_TYPE_MCIR2_MSE] = "mcir2-mse",
diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 5a4d4a611197..6ef0e7232356 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -714,34 +714,6 @@ wbcir_shutdown(struct pnp_dev *device)
proto = IR_PROTOCOL_RC5;
break;
 
-   case RC_TYPE_NEC:
-   mask[1] = bitrev8(mask_sc);
-   mask[0] = mask[1];
-   mask[3] = bitrev8(mask_sc >> 8);
-   mask[2] = mask[3];
-
-   match[1] = bitrev8(wake_sc);
-   match[0] = ~match[1];
-   match[3] = bitrev8(wake_sc >> 8);
-   match[2] = ~match[3];
-
-   proto = IR_PROTOCOL_NEC;
-   break;
-
-   case RC_TYPE_NECX:
-   mask[1] = bitrev8(mask_sc);
-   mask[0] = mask[1];
-   mask[2] = bitrev8(mask_sc >> 8);
-   mask[3] = bitrev8(mask_sc >> 16);
-
-   match[1] = bitrev8(wake_sc);
-   match[0] = ~match[1];
-   match[2] = bitrev8(wake_sc >> 8);
-   match[3] = bitrev8(wake_sc >> 16);
-
-   proto = IR_PROTOCOL_NEC;
-   break;
-
case RC_TYPE_NEC32:
mask[0] = bitrev8(mask_sc);
mask[1] = bitrev8(mask_sc >> 8);
@@ -1087,8 +1059,8 @@ wbcir_probe(struct pnp_dev *device, const struct 
pnp_device_id *dev_id)
data->dev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
data->dev->rx_resolution = US_TO_NS(2);
data->dev->allowed_protocols = RC_BIT_ALL_IR_DECODER;
-   data->dev->allowed_wakeup_protocols = RC_BIT_NEC | RC_BIT_NECX |
-   RC_BIT_NEC32 | RC_BIT_RC5 | RC_BIT_RC6_0 |
+   data->dev->allowed_wakeup_protocols =
+   RC_BIT_NEC | RC_BIT_RC5 | RC_BIT_RC6_0 |
RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 |
RC_BIT_RC6_6A_32 | RC_BIT_RC6_MCE;
data->dev->wakeup_protocol = RC_TYPE_RC6_MCE;

[PATCH] rc-core: use the full 32 bits for NEC scancodes

[David Härdeman]

Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
and the nec decoder without any loss of functionality. At the same time
it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and
removes lots of duplication (as you can see from the patch, the same NEC
disambiguation logic is contained in several different drivers).

Using NEC32 also removes ambiguity. For example, consider these two NEC
messages:
NEC16 message to address 0x05, command 0x03
NEC24 message to address 0x0005, command 0x03

They'll both have scancode 0x0503, and there's no way to tell which
message was received.

In order to maintain backwards compatibility, some heuristics are added
in rc-main.c to convert scancodes to NEC32 as necessary when userspace
adds entries to the keytable using the regular input ioctls. These
heuristics are essentially the same as the ones that are currently in
drivers/media/rc/img-ir/img-ir-nec.c (which are rendered unecessary
with this patch).

The reason this has to be done now is that the newer sysfs wakefilter API
will expose the difference between the NEC protocols to userspace for no
good reason and once exposed, it will be much more difficult to change the
logic.

Signed-off-by: David Härdeman <da...@hardeman.nu>
---
 drivers/media/rc/igorplugusb.c   |4 +
 drivers/media/rc/img-ir/img-ir-nec.c |   92 +++---
 drivers/media/rc/ir-nec-decoder.c|   63 ---
 drivers/media/rc/rc-main.c   |   74 ---
 drivers/media/rc/winbond-cir.c   |   32 +---
 5 files changed, 89 insertions(+), 176 deletions(-)

diff --git a/drivers/media/pci/cx88/cx88-input.c 
b/drivers/media/pci/cx88/cx88-input.c
index 01f2e472a2a0..61c46763ac97 100644
--- a/drivers/media/pci/cx88/cx88-input.c
+++ b/drivers/media/pci/cx88/cx88-input.c
@@ -146,7 +146,7 @@ static void cx88_ir_handle_key(struct cx88_IR *ir)
scancode = RC_SCANCODE_NECX(addr, cmd);
 
if (0 == (gpio & ir->mask_keyup))
-   rc_keydown_notimeout(ir->dev, RC_TYPE_NECX, scancode,
+   rc_keydown_notimeout(ir->dev, RC_TYPE_NEC, scancode,
 0);
else
rc_keyup(ir->dev);
@@ -348,7 +348,7 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev 
*pci)
 * 002-T mini RC, provided with newer PV hardware
 */
ir_codes = RC_MAP_PIXELVIEW_MK12;
-   rc_type = RC_BIT_NECX;
+   rc_type = RC_BIT_NEC;
ir->gpio_addr = MO_GP1_IO;
ir->mask_keyup = 0x80;
ir->polling = 10; /* ms */
diff --git a/drivers/media/pci/saa7134/saa7134-input.c 
b/drivers/media/pci/saa7134/saa7134-input.c
index 78849c19f68a..47d8e055ddd3 100644
--- a/drivers/media/pci/saa7134/saa7134-input.c
+++ b/drivers/media/pci/saa7134/saa7134-input.c
@@ -338,7 +338,7 @@ static int get_key_beholdm6xx(struct IR_i2c *ir, enum 
rc_type *protocol,
if (data[9] != (unsigned char)(~data[8]))
return 0;
 
-   *protocol = RC_TYPE_NECX;
+   *protocol = RC_TYPE_NEC;
*scancode = RC_SCANCODE_NECX(data[11] << 8 | data[10], data[9]);
*toggle = 0;
return 1;
@@ -1028,7 +1028,7 @@ void saa7134_probe_i2c_ir(struct saa7134_dev *dev)
dev->init_data.name = "BeholdTV";
dev->init_data.get_key = get_key_beholdm6xx;
dev->init_data.ir_codes = RC_MAP_BEHOLD;
-   dev->init_data.type = RC_BIT_NECX;
+   dev->init_data.type = RC_BIT_NEC;
info.addr = 0x2d;
break;
case SAA7134_BOARD_AVERMEDIA_CARDBUS_501:
diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
index cb6d4f1247da..9e3119c94368 100644
--- a/drivers/media/rc/igorplugusb.c
+++ b/drivers/media/rc/igorplugusb.c
@@ -203,8 +203,8 @@ static int igorplugusb_probe(struct usb_interface *intf,
 * for the NEC protocol and many others.
 */
rc->allowed_protocols = RC_BIT_ALL_IR_DECODER & ~(RC_BIT_NEC |
-   RC_BIT_NECX | RC_BIT_NEC32 | RC_BIT_RC6_6A_20 |
-   RC_BIT_RC6_6A_24 | RC_BIT_RC6_6A_32 | RC_BIT_RC6_MCE |
+   RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 |
+   RC_BIT_RC6_6A_32 | RC_BIT_RC6_MCE |
RC_BIT_SONY20 | RC_BIT_SANYO);
 
rc->priv = ir;
diff --git a/drivers/media/rc/img-ir/img-ir-nec.c 
b/drivers/media/rc/img-ir/img-ir-nec.c
index 044fd42b22a0..56159bb44f9c 100644
--- a/drivers/media/rc/img-ir/img-ir-nec.c
+++ b/drivers/media/rc/img-ir/img-ir-nec.c
@@ -28,28 +28,15 @@ static int img_ir_nec_scancode(int len, u64 raw, u64 
enabled_protocols,
addr_inv = (raw >>  8) & 0xff;
data = (raw >> 

Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices

[David Härdeman]

October 27, 2016 4:36 PM, "Sean Young"  wrote:
> Since we have to be able to switch between waiting and not waiting,
> we need some sort of ABI for this. I think this warrants a new ioctl;
> I'm not sure how else it can be done. I'll be sending out a patch
> shortly.

Hi Sean,

have you considered using a module param for the LIRC bridge module instead? As 
far as I understand it, this is an issue which is entirely internal to LIRC, 
and it's also not something which really needs to be changed on a per-device 
level (either you have a modern LIRC daemon or you don't, and all drivers 
should behave the same, no?).

Another advantage is that the parameter would then go away if and when the lirc 
bridge ever goes away (yes, I can still dream, can't I?).

Regards,
David
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [media] winbond-cir: Move assignments for three variables in wbcir_shutdown()

[David Härdeman]

October 19, 2016 3:38 PM, "SF Markus Elfring"  
wrote:
>>> Move the setting for the local variables "mask", "match" and "rc6_csl"
>>> behind the source code for a condition check by this function
>>> at the beginning.
>> 
>> Again, I can't see what the point is?
> 
> * How do you think about to set these variables only after the initial
> check succeded?

I prefer setting variables early so that no thinking about whether they're 
initialized or not is necessary later. 

> * Do you care for data access locality?

Not unless you can show measurable performance improvements?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] [media] winbond-cir: Move a variable assignment in wbcir_tx()

[David Härdeman]

October 14, 2016 1:42 PM, "SF Markus Elfring"  
wrote:
> From: Markus Elfring 
> Date: Fri, 14 Oct 2016 07:34:46 +0200
> 
> Move the assignment for the local variable "data" behind the source code
> for a memory allocation by this function.

Sorry, I can't see what the point is?


> Signed-off-by: Markus Elfring 
> ---
> drivers/media/rc/winbond-cir.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index 59050f5..fd997f0 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -655,7 +655,7 @@ wbcir_txmask(struct rc_dev *dev, u32 mask)
> static int
> wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count)
> {
> - struct wbcir_data *data = dev->priv;
> + struct wbcir_data *data;
> unsigned *buf;
> unsigned i;
> unsigned long flags;
> @@ -668,6 +668,7 @@ wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count)
> for (i = 0; i < count; i++)
> buf[i] = DIV_ROUND_CLOSEST(b[i], 10);
> 
> + data = dev->priv;
> /* Not sure if this is possible, but better safe than sorry */
> spin_lock_irqsave(>spinlock, flags);
> if (data->txstate != WBCIR_TXSTATE_INACTIVE) {
> -- 
> 2.10.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] [media] winbond-cir: Use kmalloc_array() in wbcir_tx()

[David Härdeman]

October 14, 2016 1:41 PM, "SF Markus Elfring" <elfr...@users.sourceforge.net> 
wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Fri, 14 Oct 2016 07:19:00 +0200
> 
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>

Sure...why not...

Signed-off-by: David Härdeman <da...@hardeman.nu>


> ---
> drivers/media/rc/winbond-cir.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index 95ae60e..59050f5 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -660,7 +660,7 @@ wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count)
> unsigned i;
> unsigned long flags;
> 
> - buf = kmalloc(count * sizeof(*b), GFP_KERNEL);
> + buf = kmalloc_array(count, sizeof(*b), GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
> 
> -- 
> 2.10.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] [media] winbond-cir: Move a variable assignment in two functions

[David Härdeman]

October 14, 2016 1:45 PM, "SF Markus Elfring"  
wrote:
> From: Markus Elfring 
> Date: Fri, 14 Oct 2016 13:13:11 +0200
> 
> Move the assignment for the local variable "data" behind the source code
> for condition checks by these functions.

Why?

> Signed-off-by: Markus Elfring 
> ---
> drivers/media/rc/winbond-cir.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index 3d286b9..716b1fe 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -566,7 +566,7 @@ wbcir_set_carrier_report(struct rc_dev *dev, int enable)
> static int
> wbcir_txcarrier(struct rc_dev *dev, u32 carrier)
> {
> - struct wbcir_data *data = dev->priv;
> + struct wbcir_data *data;
> unsigned long flags;
> u8 val;
> u32 freq;
> @@ -592,6 +592,7 @@ wbcir_txcarrier(struct rc_dev *dev, u32 carrier)
> break;
> }
> 
> + data = dev->priv;
> spin_lock_irqsave(>spinlock, flags);
> if (data->txstate != WBCIR_TXSTATE_INACTIVE) {
> spin_unlock_irqrestore(>spinlock, flags);
> @@ -611,7 +612,7 @@ wbcir_txcarrier(struct rc_dev *dev, u32 carrier)
> static int
> wbcir_txmask(struct rc_dev *dev, u32 mask)
> {
> - struct wbcir_data *data = dev->priv;
> + struct wbcir_data *data;
> unsigned long flags;
> u8 val;
> 
> @@ -637,6 +638,7 @@ wbcir_txmask(struct rc_dev *dev, u32 mask)
> return -EINVAL;
> }
> 
> + data = dev->priv;
> spin_lock_irqsave(>spinlock, flags);
> if (data->txstate != WBCIR_TXSTATE_INACTIVE) {
> spin_unlock_irqrestore(>spinlock, flags);
> -- 
> 2.10.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] [media] winbond-cir: One variable and its check less in wbcir_shutdown() after error detection

[David Härdeman]

October 15, 2016 3:30 PM, "Sean Young"  wrote:
> On Fri, Oct 14, 2016 at 01:44:02PM +0200, SF Markus Elfring wrote:
> 
>> From: Markus Elfring 
>> Date: Fri, 14 Oct 2016 12:48:41 +0200
>> 
>> The local variable "do_wake" was set to "false" after an invalid system
>> setting was detected so that a bit of error handling was triggered.
>> 
>> * Replace these assignments by direct jumps to the source code with the
>> desired exception handling.
>> 
>> * Delete this status variable and a corresponding check which became
>> unnecessary with this refactoring.
>> 
>> Signed-off-by: Markus Elfring 
>> ---
>> drivers/media/rc/winbond-cir.c | 78 
>> ++
>> 1 file changed, 34 insertions(+), 44 deletions(-)
>> 
>> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
>> index 9d05e17..3d286b9 100644
>> --- a/drivers/media/rc/winbond-cir.c
>> +++ b/drivers/media/rc/winbond-cir.c
>> @@ -699,16 +699,13 @@ wbcir_shutdown(struct pnp_dev *device)
>> {
>> struct device *dev = >dev;
>> struct wbcir_data *data = pnp_get_drvdata(device);
>> - bool do_wake = true;
>> u8 match[11];
>> u8 mask[11];
>> u8 rc6_csl;
>> int i;
>> 
>> - if (wake_sc == INVALID_SCANCODE || !device_may_wakeup(dev)) {
>> - do_wake = false;
>> - goto finish;
>> - }
>> + if (wake_sc == INVALID_SCANCODE || !device_may_wakeup(dev))
>> + goto clear_bits;
>> 
>> rc6_csl = 0;
>> memset(match, 0, sizeof(match));
>> @@ -716,9 +713,8 @@ wbcir_shutdown(struct pnp_dev *device)
>> switch (protocol) {
>> case IR_PROTOCOL_RC5:
>> if (wake_sc > 0xFFF) {
>> - do_wake = false;
>> dev_err(dev, "RC5 - Invalid wake scancode\n");
>> - break;
>> + goto clear_bits;
>> }
>> 
>> /* Mask = 13 bits, ex toggle */
>> @@ -735,9 +731,8 @@ wbcir_shutdown(struct pnp_dev *device)
>> 
>> case IR_PROTOCOL_NEC:
>> if (wake_sc > 0xFF) {
>> - do_wake = false;
>> dev_err(dev, "NEC - Invalid wake scancode\n");
>> - break;
>> + goto clear_bits;
>> }
>> 
>> mask[0] = mask[1] = mask[2] = mask[3] = 0xFF;
>> @@ -757,9 +752,8 @@ wbcir_shutdown(struct pnp_dev *device)
>> 
>> if (wake_rc6mode == 0) {
>> if (wake_sc > 0x) {
>> - do_wake = false;
>> dev_err(dev, "RC6 - Invalid wake scancode\n");
>> - break;
>> + goto clear_bits;
>> }
>> 
>> /* Command */
>> @@ -813,9 +807,8 @@ wbcir_shutdown(struct pnp_dev *device)
>> } else if (wake_sc <= 0x007F) {
>> rc6_csl = 60;
>> } else {
>> - do_wake = false;
>> dev_err(dev, "RC6 - Invalid wake scancode\n");
>> - break;
>> + goto clear_bits;
>> }
>> 
>> /* Header */
>> @@ -825,49 +818,38 @@ wbcir_shutdown(struct pnp_dev *device)
>> mask[i++] = 0x0F;
>> 
>> } else {
>> - do_wake = false;
>> dev_err(dev, "RC6 - Invalid wake mode\n");
>> + goto clear_bits;
>> }
>> 
>> break;
>> 
>> default:
>> - do_wake = false;
>> - break;
>> + goto clear_bits;
>> }
>> 
>> -finish:
>> - if (do_wake) {
>> - /* Set compare and compare mask */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
>> - WBCIR_REGSEL_COMPARE | WBCIR_REG_ADDR0,
>> - 0x3F);
>> - outsb(data->wbase + WBCIR_REG_WCEIR_DATA, match, 11);
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
>> - WBCIR_REGSEL_MASK | WBCIR_REG_ADDR0,
>> - 0x3F);
>> - outsb(data->wbase + WBCIR_REG_WCEIR_DATA, mask, 11);
>> -
>> - /* RC6 Compare String Len */
>> - outb(rc6_csl, data->wbase + WBCIR_REG_WCEIR_CSL);
>> -
>> - /* Clear status bits NEC_REP, BUFF, MSG_END, MATCH */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_STS, 0x17, 0x17);
>> + /* Set compare and compare mask */
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
>> + WBCIR_REGSEL_COMPARE | WBCIR_REG_ADDR0,
>> + 0x3F);
>> + outsb(data->wbase + WBCIR_REG_WCEIR_DATA, match, 11);
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
>> + WBCIR_REGSEL_MASK | WBCIR_REG_ADDR0,
>> + 0x3F);
>> + outsb(data->wbase + WBCIR_REG_WCEIR_DATA, mask, 11);
>> 
>> - /* Clear BUFF_EN, Clear END_EN, Set MATCH_EN */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x01, 0x07);
>> + /* RC6 Compare String Len */
>> + outb(rc6_csl, data->wbase + WBCIR_REG_WCEIR_CSL);
>> 
>> - /* Set CEIR_EN */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
>> -
>> - } else {
>> - /* Clear BUFF_EN, Clear END_EN, Clear MATCH_EN */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x00, 0x07);
>> + /* Clear status bits NEC_REP, BUFF, MSG_END, MATCH */
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_STS, 0x17, 0x17);
>> 
>> - /* Clear CEIR_EN */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x00, 0x01);
>> - }
>> + /* Clear BUFF_EN, Clear END_EN, Set MATCH_EN */
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x01, 0x07);
>> 
>> + /* Set CEIR_EN */
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
>> +set_irqmask:
>> /*
>> * ACPI will set the HW disable bit for SP3 which means that the
>> * output signals are left in an undefined state which may cause
>> @@ -876,6 +858,14 @@ 

Re: [PATCH 4/5] [media] winbond-cir: One variable and its check less in wbcir_shutdown() after error detection

[David Härdeman]

October 15, 2016 6:42 PM, "SF Markus Elfring"  
wrote:
>>> + /* Set CEIR_EN */
>>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
>>> +set_irqmask:
>>> /*
>>> * ACPI will set the HW disable bit for SP3 which means that the
>>> * output signals are left in an undefined state which may cause
>>> @@ -876,6 +858,14 @@ wbcir_shutdown(struct pnp_dev *device)
>>> */
>>> wbcir_set_irqmask(data, WBCIR_IRQ_NONE);
>>> disable_irq(data->irq);
>>> + return;
>>> +clear_bits:
>>> + /* Clear BUFF_EN, Clear END_EN, Clear MATCH_EN */
>>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x00, 0x07);
>>> +
>>> + /* Clear CEIR_EN */
>>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x00, 0x01);
>>> + goto set_irqmask;
>> 
>> I'm not convinced that adding a goto which goes backwards is making this
>> code any more readible, just so that a local variable can be dropped.
> 
> Thanks for your feedback.
> 
> Is such a "backward jump" usual and finally required when you would like
> to move a bit of common error handling code to the end without using extra
> local variables and a few statements should still be performed after it?
> 

I'm sorry, I can't parse this.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] [media] winbond-cir: Move assignments for three variables in wbcir_shutdown()

[David Härdeman]

October 14, 2016 1:43 PM, "SF Markus Elfring"  
wrote:
> From: Markus Elfring 
> Date: Fri, 14 Oct 2016 10:40:12 +0200
> 
> Move the setting for the local variables "mask", "match" and "rc6_csl"
> behind the source code for a condition check by this function
> at the beginning.
 
Again, I can't see what the point is?

> Signed-off-by: Markus Elfring 
> ---
> drivers/media/rc/winbond-cir.c | 8 
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index fd997f0..9d05e17 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -702,17 +702,17 @@ wbcir_shutdown(struct pnp_dev *device)
> bool do_wake = true;
> u8 match[11];
> u8 mask[11];
> - u8 rc6_csl = 0;
> + u8 rc6_csl;
> int i;
> 
> - memset(match, 0, sizeof(match));
> - memset(mask, 0, sizeof(mask));
> -
> if (wake_sc == INVALID_SCANCODE || !device_may_wakeup(dev)) {
> do_wake = false;
> goto finish;
> }
> 
> + rc6_csl = 0;
> + memset(match, 0, sizeof(match));
> + memset(mask, 0, sizeof(mask));
> switch (protocol) {
> case IR_PROTOCOL_RC5:
> if (wake_sc > 0xFFF) {
> -- 
> 2.10.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html