* [PATCH 01/16] lirc_dev: remove pointless functions
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
@ 2017-05-01 16:03 ` David Härdeman
2017-05-01 16:03 ` [PATCH 02/16] lirc_dev: remove unused set_use_inc/set_use_dec David Härdeman
` (14 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:03 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
drv->set_use_inc and drv->set_use_dec are already optional so we can remove all
dummy functions.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ir-lirc-codec.c | 14 ++------------
drivers/staging/media/lirc/lirc_zilog.c | 11 -----------
2 files changed, 2 insertions(+), 23 deletions(-)
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 8f0669c9894c..fc58745b26b8 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -327,16 +327,6 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
return ret;
}
-static int ir_lirc_open(void *data)
-{
- return 0;
-}
-
-static void ir_lirc_close(void *data)
-{
- return;
-}
-
static const struct file_operations lirc_fops = {
.owner = THIS_MODULE,
.write = ir_lirc_transmit_ir,
@@ -396,8 +386,8 @@ static int ir_lirc_register(struct rc_dev *dev)
drv->features = features;
drv->data = &dev->raw->lirc;
drv->rbuf = NULL;
- drv->set_use_inc = &ir_lirc_open;
- drv->set_use_dec = &ir_lirc_close;
+ drv->set_use_inc = NULL;
+ drv->set_use_dec = NULL;
drv->code_length = sizeof(struct ir_raw_event) * 8;
drv->chunk_size = sizeof(int);
drv->buffer_size = LIRCBUF_SIZE;
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index e4a533b6beb3..436cf1b6a70a 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -497,15 +497,6 @@ static int lirc_thread(void *arg)
return 0;
}
-static int set_use_inc(void *data)
-{
- return 0;
-}
-
-static void set_use_dec(void *data)
-{
-}
-
/* safe read of a uint32 (always network byte order) */
static int read_uint32(unsigned char **data,
unsigned char *endp, unsigned int *val)
@@ -1396,8 +1387,6 @@ static struct lirc_driver lirc_template = {
.buffer_size = BUFLEN / 2,
.sample_rate = 0, /* tell lirc_dev to not start its own kthread */
.chunk_size = 2,
- .set_use_inc = set_use_inc,
- .set_use_dec = set_use_dec,
.fops = &lirc_fops,
.owner = THIS_MODULE,
};
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 02/16] lirc_dev: remove unused set_use_inc/set_use_dec
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
2017-05-01 16:03 ` [PATCH 01/16] lirc_dev: remove pointless functions David Härdeman
@ 2017-05-01 16:03 ` David Härdeman
2017-05-01 16:03 ` [PATCH 03/16] lirc_dev: correct error handling David Härdeman
` (13 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:03 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Since there are no users of this functionality, it can be removed altogether.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ir-lirc-codec.c | 2 --
drivers/media/rc/lirc_dev.c | 24 ++++++------------------
include/media/lirc_dev.h | 6 ------
3 files changed, 6 insertions(+), 26 deletions(-)
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index fc58745b26b8..a30af91710fe 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -386,8 +386,6 @@ static int ir_lirc_register(struct rc_dev *dev)
drv->features = features;
drv->data = &dev->raw->lirc;
drv->rbuf = NULL;
- drv->set_use_inc = NULL;
- drv->set_use_dec = NULL;
drv->code_length = sizeof(struct ir_raw_event) * 8;
drv->chunk_size = sizeof(int);
drv->buffer_size = LIRCBUF_SIZE;
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 42704552b005..05f600bd6c67 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -418,12 +418,6 @@ int lirc_unregister_driver(int minor)
wake_up_interruptible(&ir->buf->wait_poll);
}
- mutex_lock(&ir->irctl_lock);
-
- if (ir->d.set_use_dec)
- ir->d.set_use_dec(ir->d.data);
-
- mutex_unlock(&ir->irctl_lock);
mutex_unlock(&lirc_dev_lock);
device_del(&ir->dev);
@@ -473,17 +467,13 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
goto error;
}
+ if (ir->buf)
+ lirc_buffer_clear(ir->buf);
+
+ if (ir->task)
+ wake_up_process(ir->task);
+
ir->open++;
- if (ir->d.set_use_inc)
- retval = ir->d.set_use_inc(ir->d.data);
- if (retval) {
- ir->open--;
- } else {
- if (ir->buf)
- lirc_buffer_clear(ir->buf);
- if (ir->task)
- wake_up_process(ir->task);
- }
error:
nonseekable_open(inode, file);
@@ -508,8 +498,6 @@ int lirc_dev_fop_close(struct inode *inode, struct file *file)
rc_close(ir->d.rdev);
ir->open--;
- if (ir->d.set_use_dec)
- ir->d.set_use_dec(ir->d.data);
if (!ret)
mutex_unlock(&lirc_dev_lock);
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index cec7d35602d1..71c1c11950fe 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -165,10 +165,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
* have to write to the buffer by other means, like irq's
* (see also lirc_serial.c).
*
- * @set_use_inc: set_use_inc will be called after device is opened
- *
- * @set_use_dec: set_use_dec will be called after device is closed
- *
* @rdev: Pointed to struct rc_dev associated with the LIRC
* device.
*
@@ -198,8 +194,6 @@ struct lirc_driver {
int max_timeout;
int (*add_to_buf)(void *data, struct lirc_buffer *buf);
struct lirc_buffer *rbuf;
- int (*set_use_inc)(void *data);
- void (*set_use_dec)(void *data);
struct rc_dev *rdev;
const struct file_operations *fops;
struct device *dev;
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 03/16] lirc_dev: correct error handling
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
2017-05-01 16:03 ` [PATCH 01/16] lirc_dev: remove pointless functions David Härdeman
2017-05-01 16:03 ` [PATCH 02/16] lirc_dev: remove unused set_use_inc/set_use_dec David Härdeman
@ 2017-05-01 16:03 ` David Härdeman
2017-05-21 8:57 ` Sean Young
2017-05-01 16:03 ` [PATCH 04/16] lirc_dev: remove sampling kthread David Härdeman
` (12 subsequent siblings)
15 siblings, 1 reply; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:03 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
If an error is generated, nonseekable_open() shouldn't be called.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 05f600bd6c67..7f13ed479e1c 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -431,7 +431,7 @@ EXPORT_SYMBOL(lirc_unregister_driver);
int lirc_dev_fop_open(struct inode *inode, struct file *file)
{
struct irctl *ir;
- int retval = 0;
+ int retval;
if (iminor(inode) >= MAX_IRCTL_DEVICES) {
pr_err("open result for %d is -ENODEV\n", iminor(inode));
@@ -475,9 +475,11 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
ir->open++;
-error:
nonseekable_open(inode, file);
+ return 0;
+
+error:
return retval;
}
EXPORT_SYMBOL(lirc_dev_fop_open);
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 03/16] lirc_dev: correct error handling
2017-05-01 16:03 ` [PATCH 03/16] lirc_dev: correct error handling David Härdeman
@ 2017-05-21 8:57 ` Sean Young
2017-05-28 8:23 ` David Härdeman
0 siblings, 1 reply; 28+ messages in thread
From: Sean Young @ 2017-05-21 8:57 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, mchehab
On Mon, May 01, 2017 at 06:03:51PM +0200, David Härdeman wrote:
> If an error is generated, nonseekable_open() shouldn't be called.
There is no harm in calling nonseekable_open(), so this commit is
misleading.
Sean
>
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
> drivers/media/rc/lirc_dev.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 05f600bd6c67..7f13ed479e1c 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -431,7 +431,7 @@ EXPORT_SYMBOL(lirc_unregister_driver);
> int lirc_dev_fop_open(struct inode *inode, struct file *file)
> {
> struct irctl *ir;
> - int retval = 0;
> + int retval;
>
> if (iminor(inode) >= MAX_IRCTL_DEVICES) {
> pr_err("open result for %d is -ENODEV\n", iminor(inode));
> @@ -475,9 +475,11 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
>
> ir->open++;
>
> -error:
> nonseekable_open(inode, file);
>
> + return 0;
> +
> +error:
> return retval;
> }
> EXPORT_SYMBOL(lirc_dev_fop_open);
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 03/16] lirc_dev: correct error handling
2017-05-21 8:57 ` Sean Young
@ 2017-05-28 8:23 ` David Härdeman
2017-05-28 15:04 ` Sean Young
0 siblings, 1 reply; 28+ messages in thread
From: David Härdeman @ 2017-05-28 8:23 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, mchehab
On Sun, May 21, 2017 at 09:57:13AM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:03:51PM +0200, David Härdeman wrote:
>> If an error is generated, nonseekable_open() shouldn't be called.
>
>There is no harm in calling nonseekable_open(), so this commit is
>misleading.
I'm not sure why you consider it misleading? If there's an error, the
logic thing to do is to error out immediately and not do any further
work?
>> Signed-off-by: David Härdeman <david@hardeman.nu>
>> ---
>> drivers/media/rc/lirc_dev.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
>> index 05f600bd6c67..7f13ed479e1c 100644
>> --- a/drivers/media/rc/lirc_dev.c
>> +++ b/drivers/media/rc/lirc_dev.c
>> @@ -431,7 +431,7 @@ EXPORT_SYMBOL(lirc_unregister_driver);
>> int lirc_dev_fop_open(struct inode *inode, struct file *file)
>> {
>> struct irctl *ir;
>> - int retval = 0;
>> + int retval;
>>
>> if (iminor(inode) >= MAX_IRCTL_DEVICES) {
>> pr_err("open result for %d is -ENODEV\n", iminor(inode));
>> @@ -475,9 +475,11 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
>>
>> ir->open++;
>>
>> -error:
>> nonseekable_open(inode, file);
>>
>> + return 0;
>> +
>> +error:
>> return retval;
>> }
>> EXPORT_SYMBOL(lirc_dev_fop_open);
--
David Härdeman
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 03/16] lirc_dev: correct error handling
2017-05-28 8:23 ` David Härdeman
@ 2017-05-28 15:04 ` Sean Young
2017-06-17 11:14 ` David Härdeman
0 siblings, 1 reply; 28+ messages in thread
From: Sean Young @ 2017-05-28 15:04 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, mchehab
On Sun, May 28, 2017 at 10:23:37AM +0200, David Härdeman wrote:
> On Sun, May 21, 2017 at 09:57:13AM +0100, Sean Young wrote:
> >On Mon, May 01, 2017 at 06:03:51PM +0200, David Härdeman wrote:
> >> If an error is generated, nonseekable_open() shouldn't be called.
> >
> >There is no harm in calling nonseekable_open(), so this commit is
> >misleading.
>
> I'm not sure why you consider it misleading? If there's an error, the
> logic thing to do is to error out immediately and not do any further
> work?
The commit message says that nonseekable_open() should not be called,
suggesting there is a bug which is not the case.
Sean
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 03/16] lirc_dev: correct error handling
2017-05-28 15:04 ` Sean Young
@ 2017-06-17 11:14 ` David Härdeman
0 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-06-17 11:14 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, mchehab
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 04/16] lirc_dev: remove sampling kthread
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (2 preceding siblings ...)
2017-05-01 16:03 ` [PATCH 03/16] lirc_dev: correct error handling David Härdeman
@ 2017-05-01 16:03 ` David Härdeman
2017-05-01 16:04 ` [PATCH 05/16] lirc_dev: clarify error handling David Härdeman
` (11 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:03 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
There are no drivers which use this functionality.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 94 +------------------------------
drivers/staging/media/lirc/lirc_zilog.c | 1
include/media/lirc_dev.h | 16 -----
3 files changed, 2 insertions(+), 109 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 7f13ed479e1c..5c2b009b6d50 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -28,7 +28,6 @@
#include <linux/mutex.h>
#include <linux/wait.h>
#include <linux/unistd.h>
-#include <linux/kthread.h>
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/cdev.h>
@@ -57,9 +56,6 @@ struct irctl {
struct device dev;
struct cdev cdev;
-
- struct task_struct *task;
- long jiffies_to_wait;
};
static DEFINE_MUTEX(lirc_dev_lock);
@@ -95,59 +91,6 @@ static void lirc_release(struct device *ld)
kfree(ir);
}
-/* helper function
- * reads key codes from driver and puts them into buffer
- * returns 0 on success
- */
-static int lirc_add_to_buf(struct irctl *ir)
-{
- int res;
- int got_data = -1;
-
- if (!ir->d.add_to_buf)
- return 0;
-
- /*
- * service the device as long as it is returning
- * data and we have space
- */
- do {
- got_data++;
- res = ir->d.add_to_buf(ir->d.data, ir->buf);
- } while (!res);
-
- if (res == -ENODEV)
- kthread_stop(ir->task);
-
- return got_data ? 0 : res;
-}
-
-/* main function of the polling thread
- */
-static int lirc_thread(void *irctl)
-{
- struct irctl *ir = irctl;
-
- do {
- if (ir->open) {
- if (ir->jiffies_to_wait) {
- set_current_state(TASK_INTERRUPTIBLE);
- schedule_timeout(ir->jiffies_to_wait);
- }
- if (kthread_should_stop())
- break;
- if (!lirc_add_to_buf(ir))
- wake_up_interruptible(&ir->buf->wait_poll);
- } else {
- set_current_state(TASK_INTERRUPTIBLE);
- schedule();
- }
- } while (!kthread_should_stop());
-
- return 0;
-}
-
-
static const struct file_operations lirc_dev_fops = {
.owner = THIS_MODULE,
.read = lirc_dev_fop_read,
@@ -252,18 +195,8 @@ static int lirc_allocate_driver(struct lirc_driver *d)
return -EBADRQC;
}
- if (d->sample_rate) {
- if (2 > d->sample_rate || HZ < d->sample_rate) {
- dev_err(d->dev, "invalid %d sample rate\n",
- d->sample_rate);
- return -EBADRQC;
- }
- if (!d->add_to_buf) {
- dev_err(d->dev, "add_to_buf not set\n");
- return -EBADRQC;
- }
- } else if (!d->rbuf && !(d->fops && d->fops->read &&
- d->fops->poll && d->fops->unlocked_ioctl)) {
+ if (!d->rbuf && !(d->fops && d->fops->read &&
+ d->fops->poll && d->fops->unlocked_ioctl)) {
dev_err(d->dev, "undefined read, poll, ioctl\n");
return -EBADRQC;
}
@@ -312,22 +245,6 @@ static int lirc_allocate_driver(struct lirc_driver *d)
dev_set_name(&ir->dev, "lirc%d", ir->d.minor);
device_initialize(&ir->dev);
- if (d->sample_rate) {
- ir->jiffies_to_wait = HZ / d->sample_rate;
-
- /* try to fire up polling thread */
- ir->task = kthread_run(lirc_thread, (void *)ir, "lirc_dev");
- if (IS_ERR(ir->task)) {
- dev_err(d->dev, "cannot run thread for minor = %d\n",
- d->minor);
- err = -ECHILD;
- goto out_sysfs;
- }
- } else {
- /* it means - wait for external event in task queue */
- ir->jiffies_to_wait = 0;
- }
-
err = lirc_cdev_add(ir);
if (err)
goto out_sysfs;
@@ -404,10 +321,6 @@ int lirc_unregister_driver(int minor)
return -ENOENT;
}
- /* end up polling thread */
- if (ir->task)
- kthread_stop(ir->task);
-
dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
ir->d.name, ir->d.minor);
@@ -470,9 +383,6 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
if (ir->buf)
lirc_buffer_clear(ir->buf);
- if (ir->task)
- wake_up_process(ir->task);
-
ir->open++;
nonseekable_open(inode, file);
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index 436cf1b6a70a..8d8fd8b164e2 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1385,7 +1385,6 @@ static struct lirc_driver lirc_template = {
.minor = -1,
.code_length = 13,
.buffer_size = BUFLEN / 2,
- .sample_rate = 0, /* tell lirc_dev to not start its own kthread */
.chunk_size = 2,
.fops = &lirc_fops,
.owner = THIS_MODULE,
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 71c1c11950fe..01649b009922 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -133,12 +133,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
* @buffer_size: Number of FIFO buffers with @chunk_size size. If zero,
* creates a buffer with BUFLEN size (16 bytes).
*
- * @sample_rate: if zero, the device will wait for an event with a new
- * code to be parsed. Otherwise, specifies the sample
- * rate for polling. Value should be between 0
- * and HZ. If equal to HZ, it would mean one polling per
- * second.
- *
* @features: lirc compatible hardware features, like LIRC_MODE_RAW,
* LIRC_CAN\_\*, as defined at include/media/lirc.h.
*
@@ -153,14 +147,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
* @max_timeout: Maximum timeout for record. Valid only if
* LIRC_CAN_SET_REC_TIMEOUT is defined.
*
- * @add_to_buf: add_to_buf will be called after specified period of the
- * time or triggered by the external event, this behavior
- * depends on value of the sample_rate this function will
- * be called in user context. This routine should return
- * 0 if data was added to the buffer and -ENODATA if none
- * was available. This should add some number of bits
- * evenly divisible by code_length to the buffer.
- *
* @rbuf: if not NULL, it will be used as a read buffer, you will
* have to write to the buffer by other means, like irq's
* (see also lirc_serial.c).
@@ -184,7 +170,6 @@ struct lirc_driver {
int minor;
__u32 code_length;
unsigned int buffer_size; /* in chunks holding one code each */
- int sample_rate;
__u32 features;
unsigned int chunk_size;
@@ -192,7 +177,6 @@ struct lirc_driver {
void *data;
int min_timeout;
int max_timeout;
- int (*add_to_buf)(void *data, struct lirc_buffer *buf);
struct lirc_buffer *rbuf;
struct rc_dev *rdev;
const struct file_operations *fops;
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 05/16] lirc_dev: clarify error handling
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (3 preceding siblings ...)
2017-05-01 16:03 ` [PATCH 04/16] lirc_dev: remove sampling kthread David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
2017-05-01 16:04 ` [PATCH 06/16] lirc_dev: make fops mandatory David Härdeman
` (10 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
out_sysfs is misleading, sysfs only comes into play after device_add(). Also,
calling device_init() before the rest of struct dev is filled out is clearer.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 5c2b009b6d50..fb487c39b834 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -238,16 +238,16 @@ static int lirc_allocate_driver(struct lirc_driver *d)
ir->d = *d;
+ device_initialize(&ir->dev);
ir->dev.devt = MKDEV(MAJOR(lirc_base_dev), ir->d.minor);
ir->dev.class = lirc_class;
ir->dev.parent = d->dev;
ir->dev.release = lirc_release;
dev_set_name(&ir->dev, "lirc%d", ir->d.minor);
- device_initialize(&ir->dev);
err = lirc_cdev_add(ir);
if (err)
- goto out_sysfs;
+ goto out_free_dev;
ir->attached = 1;
@@ -264,7 +264,7 @@ static int lirc_allocate_driver(struct lirc_driver *d)
return minor;
out_cdev:
cdev_del(&ir->cdev);
-out_sysfs:
+out_free_dev:
put_device(&ir->dev);
out_lock:
mutex_unlock(&lirc_dev_lock);
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 06/16] lirc_dev: make fops mandatory
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (4 preceding siblings ...)
2017-05-01 16:04 ` [PATCH 05/16] lirc_dev: clarify error handling David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
2017-05-01 16:04 ` [PATCH 07/16] lirc_dev: merge lirc_register_driver() and lirc_allocate_driver() David Härdeman
` (9 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Every caller of lirc_register_driver() passes their own fops and there are no
users of lirc_dev_fop_write() in the kernel tree. Thus we can make fops
mandatory and remove lirc_dev_fop_write().
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 41 +++++------------------------------------
include/media/lirc_dev.h | 3 ---
2 files changed, 5 insertions(+), 39 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index fb487c39b834..29eecddbd371 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -91,17 +91,6 @@ static void lirc_release(struct device *ld)
kfree(ir);
}
-static const struct file_operations lirc_dev_fops = {
- .owner = THIS_MODULE,
- .read = lirc_dev_fop_read,
- .write = lirc_dev_fop_write,
- .poll = lirc_dev_fop_poll,
- .unlocked_ioctl = lirc_dev_fop_ioctl,
- .open = lirc_dev_fop_open,
- .release = lirc_dev_fop_close,
- .llseek = noop_llseek,
-};
-
static int lirc_cdev_add(struct irctl *ir)
{
struct lirc_driver *d = &ir->d;
@@ -110,13 +99,11 @@ static int lirc_cdev_add(struct irctl *ir)
cdev = &ir->cdev;
- if (d->fops) {
- cdev_init(cdev, d->fops);
- cdev->owner = d->owner;
- } else {
- cdev_init(cdev, &lirc_dev_fops);
- cdev->owner = THIS_MODULE;
- }
+ if (!d->fops)
+ return -EINVAL;
+
+ cdev_init(cdev, d->fops);
+ cdev->owner = d->owner;
retval = kobject_set_name(&cdev->kobj, "lirc%d", d->minor);
if (retval)
return retval;
@@ -640,24 +627,6 @@ void *lirc_get_pdata(struct file *file)
EXPORT_SYMBOL(lirc_get_pdata);
-ssize_t lirc_dev_fop_write(struct file *file, const char __user *buffer,
- size_t length, loff_t *ppos)
-{
- struct irctl *ir = irctls[iminor(file_inode(file))];
-
- if (!ir) {
- pr_err("called with invalid irctl\n");
- return -ENODEV;
- }
-
- if (!ir->attached)
- return -ENODEV;
-
- return -EINVAL;
-}
-EXPORT_SYMBOL(lirc_dev_fop_write);
-
-
static int __init lirc_dev_init(void)
{
int retval;
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 01649b009922..1f327e25a9be 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -210,7 +210,4 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait);
long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
ssize_t lirc_dev_fop_read(struct file *file, char __user *buffer, size_t length,
loff_t *ppos);
-ssize_t lirc_dev_fop_write(struct file *file, const char __user *buffer,
- size_t length, loff_t *ppos);
-
#endif
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 07/16] lirc_dev: merge lirc_register_driver() and lirc_allocate_driver()
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (5 preceding siblings ...)
2017-05-01 16:04 ` [PATCH 06/16] lirc_dev: make fops mandatory David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
2017-05-01 16:04 ` [PATCH 08/16] lirc_zilog: remove module parameter minor David Härdeman
` (8 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Merging the two means that lirc_allocate_buffer() is called before device_add()
and cdev_add() which makes more sense. This also simplifies the locking
slightly because lirc_allocate_buffer() will always be called with lirc_dev_lock
held.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 41 +++++++++++++----------------------------
1 file changed, 13 insertions(+), 28 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 29eecddbd371..fcc88a09b108 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -120,8 +120,6 @@ static int lirc_allocate_buffer(struct irctl *ir)
unsigned int buffer_size;
struct lirc_driver *d = &ir->d;
- mutex_lock(&lirc_dev_lock);
-
bytes_in_key = BITS_TO_LONGS(d->code_length) +
(d->code_length % 8 ? 1 : 0);
buffer_size = d->buffer_size ? d->buffer_size : BUFLEN / bytes_in_key;
@@ -145,16 +143,15 @@ static int lirc_allocate_buffer(struct irctl *ir)
}
ir->buf_internal = true;
+ d->rbuf = ir->buf;
}
ir->chunk_size = ir->buf->chunk_size;
out:
- mutex_unlock(&lirc_dev_lock);
-
return err;
}
-static int lirc_allocate_driver(struct lirc_driver *d)
+int lirc_register_driver(struct lirc_driver *d)
{
struct irctl *ir;
int minor;
@@ -225,6 +222,15 @@ static int lirc_allocate_driver(struct lirc_driver *d)
ir->d = *d;
+ if (LIRC_CAN_REC(d->features)) {
+ err = lirc_allocate_buffer(irctls[minor]);
+ if (err) {
+ kfree(ir);
+ goto out_lock;
+ }
+ d->rbuf = ir->buf;
+ }
+
device_initialize(&ir->dev);
ir->dev.devt = MKDEV(MAJOR(lirc_base_dev), ir->d.minor);
ir->dev.class = lirc_class;
@@ -248,7 +254,9 @@ static int lirc_allocate_driver(struct lirc_driver *d)
dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
ir->d.name, ir->d.minor);
+
return minor;
+
out_cdev:
cdev_del(&ir->cdev);
out_free_dev:
@@ -258,29 +266,6 @@ static int lirc_allocate_driver(struct lirc_driver *d)
return err;
}
-
-int lirc_register_driver(struct lirc_driver *d)
-{
- int minor, err = 0;
-
- minor = lirc_allocate_driver(d);
- if (minor < 0)
- return minor;
-
- if (LIRC_CAN_REC(d->features)) {
- err = lirc_allocate_buffer(irctls[minor]);
- if (err)
- lirc_unregister_driver(minor);
- else
- /*
- * This is kind of a hack but ir-lirc-codec needs
- * access to the buffer that lirc_dev allocated.
- */
- d->rbuf = irctls[minor]->buf;
- }
-
- return err ? err : minor;
-}
EXPORT_SYMBOL(lirc_register_driver);
int lirc_unregister_driver(int minor)
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 08/16] lirc_zilog: remove module parameter minor
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (6 preceding siblings ...)
2017-05-01 16:04 ` [PATCH 07/16] lirc_dev: merge lirc_register_driver() and lirc_allocate_driver() David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
2017-05-01 16:04 ` [PATCH 09/16] lirc_dev: remove lirc_irctl_init() and lirc_cdev_add() David Härdeman
` (7 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Remove the "minor" module parameter in preparation for the next patch.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/staging/media/lirc/lirc_zilog.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index 8d8fd8b164e2..59e05aa1bc55 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -156,7 +156,6 @@ static struct mutex tx_data_lock;
/* module parameters */
static bool debug; /* debug output */
static bool tx_only; /* only handle the IR Tx function */
-static int minor = -1; /* minor number */
/* struct IR reference counting */
@@ -184,10 +183,11 @@ static void release_ir_device(struct kref *ref)
* ir->open_count == 0 - happens on final close()
* ir_lock, tx_ref_lock, rx_ref_lock, all released
*/
- if (ir->l.minor >= 0 && ir->l.minor < MAX_IRCTL_DEVICES) {
+ if (ir->l.minor >= 0) {
lirc_unregister_driver(ir->l.minor);
- ir->l.minor = MAX_IRCTL_DEVICES;
+ ir->l.minor = -1;
}
+
if (kfifo_initialized(&ir->rbuf.fifo))
lirc_buffer_free(&ir->rbuf);
list_del(&ir->list);
@@ -1597,12 +1597,11 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
}
/* register with lirc */
- ir->l.minor = minor; /* module option: user requested minor number */
ir->l.minor = lirc_register_driver(&ir->l);
- if (ir->l.minor < 0 || ir->l.minor >= MAX_IRCTL_DEVICES) {
+ if (ir->l.minor < 0) {
dev_err(tx->ir->l.dev,
- "%s: \"minor\" must be between 0 and %d (%d)!\n",
- __func__, MAX_IRCTL_DEVICES-1, ir->l.minor);
+ "%s: lirc_register_driver() failed: %i\n",
+ __func__, ir->l.minor);
ret = -EBADRQC;
goto out_put_xx;
}
@@ -1673,9 +1672,6 @@ MODULE_LICENSE("GPL");
/* for compat with old name, which isn't all that accurate anymore */
MODULE_ALIAS("lirc_pvr150");
-module_param(minor, int, 0444);
-MODULE_PARM_DESC(minor, "Preferred minor device number");
-
module_param(debug, bool, 0644);
MODULE_PARM_DESC(debug, "Enable debugging messages");
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 09/16] lirc_dev: remove lirc_irctl_init() and lirc_cdev_add()
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (7 preceding siblings ...)
2017-05-01 16:04 ` [PATCH 08/16] lirc_zilog: remove module parameter minor David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
2017-05-01 16:04 ` [PATCH 10/16] lirc_dev: remove superfluous get/put_device() calls David Härdeman
` (6 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
These two functions only make the logic in lirc_register_driver() harder to
follow.
(Note that almost no other driver calls kobject_set_name() on their cdev so I
simply removed that part).
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 44 ++++++++++++-------------------------------
1 file changed, 12 insertions(+), 32 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index fcc88a09b108..574f4dd416b8 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -65,15 +65,6 @@ static struct irctl *irctls[MAX_IRCTL_DEVICES];
/* Only used for sysfs but defined to void otherwise */
static struct class *lirc_class;
-/* helper function
- * initializes the irctl structure
- */
-static void lirc_irctl_init(struct irctl *ir)
-{
- mutex_init(&ir->irctl_lock);
- ir->d.minor = NOPLUG;
-}
-
static void lirc_release(struct device *ld)
{
struct irctl *ir = container_of(ld, struct irctl, dev);
@@ -91,27 +82,6 @@ static void lirc_release(struct device *ld)
kfree(ir);
}
-static int lirc_cdev_add(struct irctl *ir)
-{
- struct lirc_driver *d = &ir->d;
- struct cdev *cdev;
- int retval;
-
- cdev = &ir->cdev;
-
- if (!d->fops)
- return -EINVAL;
-
- cdev_init(cdev, d->fops);
- cdev->owner = d->owner;
- retval = kobject_set_name(&cdev->kobj, "lirc%d", d->minor);
- if (retval)
- return retval;
-
- cdev->kobj.parent = &ir->dev.kobj;
- return cdev_add(cdev, ir->dev.devt, 1);
-}
-
static int lirc_allocate_buffer(struct irctl *ir)
{
int err = 0;
@@ -167,6 +137,11 @@ int lirc_register_driver(struct lirc_driver *d)
return -EINVAL;
}
+ if (!d->fops) {
+ pr_err("fops pointer not filled in!\n");
+ return -EINVAL;
+ }
+
if (d->minor >= MAX_IRCTL_DEVICES) {
dev_err(d->dev, "minor must be between 0 and %d!\n",
MAX_IRCTL_DEVICES - 1);
@@ -210,7 +185,8 @@ int lirc_register_driver(struct lirc_driver *d)
err = -ENOMEM;
goto out_lock;
}
- lirc_irctl_init(ir);
+
+ mutex_init(&ir->irctl_lock);
irctls[minor] = ir;
d->minor = minor;
@@ -238,7 +214,11 @@ int lirc_register_driver(struct lirc_driver *d)
ir->dev.release = lirc_release;
dev_set_name(&ir->dev, "lirc%d", ir->d.minor);
- err = lirc_cdev_add(ir);
+ cdev_init(&ir->cdev, d->fops);
+ ir->cdev.owner = ir->d.owner;
+ ir->cdev.kobj.parent = &ir->dev.kobj;
+
+ err = cdev_add(&ir->cdev, ir->dev.devt, 1);
if (err)
goto out_free_dev;
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 10/16] lirc_dev: remove superfluous get/put_device() calls
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (8 preceding siblings ...)
2017-05-01 16:04 ` [PATCH 09/16] lirc_dev: remove lirc_irctl_init() and lirc_cdev_add() David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
2017-05-01 16:04 ` [PATCH 11/16] lirc_dev: remove unused module parameter David Härdeman
` (5 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
device_add() and friends alredy manage the references to the parent device so
these calls aren't necessary.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 574f4dd416b8..34bd3f8bf30d 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -69,8 +69,6 @@ static void lirc_release(struct device *ld)
{
struct irctl *ir = container_of(ld, struct irctl, dev);
- put_device(ir->dev.parent);
-
if (ir->buf_internal) {
lirc_buffer_free(ir->buf);
kfree(ir->buf);
@@ -230,8 +228,6 @@ int lirc_register_driver(struct lirc_driver *d)
mutex_unlock(&lirc_dev_lock);
- get_device(ir->dev.parent);
-
dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
ir->d.name, ir->d.minor);
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 11/16] lirc_dev: remove unused module parameter
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (9 preceding siblings ...)
2017-05-01 16:04 ` [PATCH 10/16] lirc_dev: remove superfluous get/put_device() calls David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
2017-05-01 16:04 ` [PATCH 12/16] lirc_dev: return POLLHUP and POLLERR when device is gone David Härdeman
` (4 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
The "debug" parameter isn't actually used anywhere.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 34bd3f8bf30d..57d21201ff93 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -36,8 +36,6 @@
#include <media/lirc.h>
#include <media/lirc_dev.h>
-static bool debug;
-
#define IRCTL_DEV_NAME "BaseRemoteCtl"
#define NOPLUG -1
#define LOGHEAD "lirc_dev (%s[%d]): "
@@ -625,6 +623,3 @@ module_exit(lirc_dev_exit);
MODULE_DESCRIPTION("LIRC base driver module");
MODULE_AUTHOR("Artur Lipowski");
MODULE_LICENSE("GPL");
-
-module_param(debug, bool, S_IRUGO | S_IWUSR);
-MODULE_PARM_DESC(debug, "Enable debugging messages");
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 12/16] lirc_dev: return POLLHUP and POLLERR when device is gone
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (10 preceding siblings ...)
2017-05-01 16:04 ` [PATCH 11/16] lirc_dev: remove unused module parameter David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
2017-05-01 16:04 ` [PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors David Härdeman
` (3 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Most drivers return both values when the device is gone.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 57d21201ff93..e44e0b23b883 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -374,7 +374,7 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
}
if (!ir->attached)
- return POLLERR;
+ return POLLHUP | POLLERR;
if (ir->buf) {
poll_wait(file, &ir->buf->wait_poll, wait);
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (11 preceding siblings ...)
2017-05-01 16:04 ` [PATCH 12/16] lirc_dev: return POLLHUP and POLLERR when device is gone David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
2017-05-22 20:09 ` Sean Young
2017-05-01 16:04 ` [PATCH 14/16] lirc_dev: cleanup includes David Härdeman
` (2 subsequent siblings)
15 siblings, 1 reply; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Using the kernel ida facilities, we can avoid a lot of unnecessary code and at the same
time get rid of lirc_dev_lock in favour of per-device locks (the irctls array was used
throughout lirc_dev so this patch necessarily touches a lot of code).
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ir-lirc-codec.c | 9 -
drivers/media/rc/lirc_dev.c | 258 ++++++++++++-------------------
drivers/staging/media/lirc/lirc_zilog.c | 16 +-
include/media/lirc_dev.h | 14 --
4 files changed, 115 insertions(+), 182 deletions(-)
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index a30af91710fe..2c1221a61ea1 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -382,7 +382,6 @@ static int ir_lirc_register(struct rc_dev *dev)
snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
dev->driver_name);
- drv->minor = -1;
drv->features = features;
drv->data = &dev->raw->lirc;
drv->rbuf = NULL;
@@ -394,11 +393,9 @@ static int ir_lirc_register(struct rc_dev *dev)
drv->rdev = dev;
drv->owner = THIS_MODULE;
- drv->minor = lirc_register_driver(drv);
- if (drv->minor < 0) {
- rc = -ENODEV;
+ rc = lirc_register_driver(drv);
+ if (rc < 0)
goto out;
- }
dev->raw->lirc.drv = drv;
dev->raw->lirc.dev = dev;
@@ -413,7 +410,7 @@ static int ir_lirc_unregister(struct rc_dev *dev)
{
struct lirc_codec *lirc = &dev->raw->lirc;
- lirc_unregister_driver(lirc->drv->minor);
+ lirc_unregister_driver(lirc->drv);
kfree(lirc->drv);
lirc->drv = NULL;
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index e44e0b23b883..7db7d4c57991 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -31,23 +31,35 @@
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/cdev.h>
+#include <linux/idr.h>
#include <media/rc-core.h>
#include <media/lirc.h>
#include <media/lirc_dev.h>
#define IRCTL_DEV_NAME "BaseRemoteCtl"
-#define NOPLUG -1
#define LOGHEAD "lirc_dev (%s[%d]): "
static dev_t lirc_base_dev;
+/**
+ * struct irctl - lirc per-device structure
+ * @d: internal copy of the &struct lirc_driver for the device
+ * @dead: if the device has gone away
+ * @users: the number of users of the lirc chardev (currently limited to 1)
+ * @mutex: synchronizes open()/close()/ioctl()/etc calls
+ * @buf: used to store lirc RX data
+ * @buf_internal: whether @buf was allocated internally or not
+ * @chunk_size: @buf stores and read() returns data chunks of this size
+ * @dev: the &struct device for the lirc device
+ * @cdev: the &struct device for the lirc chardev
+ */
struct irctl {
struct lirc_driver d;
- int attached;
- int open;
+ bool dead;
+ unsigned users;
- struct mutex irctl_lock;
+ struct mutex mutex;
struct lirc_buffer *buf;
bool buf_internal;
unsigned int chunk_size;
@@ -56,9 +68,9 @@ struct irctl {
struct cdev cdev;
};
-static DEFINE_MUTEX(lirc_dev_lock);
-
-static struct irctl *irctls[MAX_IRCTL_DEVICES];
+/* Used to keep track of allocated lirc devices */
+#define LIRC_MAX_DEVICES 256
+static DEFINE_IDA(lirc_ida);
/* Only used for sysfs but defined to void otherwise */
static struct class *lirc_class;
@@ -72,9 +84,6 @@ static void lirc_release(struct device *ld)
kfree(ir->buf);
}
- mutex_lock(&lirc_dev_lock);
- irctls[ir->d.minor] = NULL;
- mutex_unlock(&lirc_dev_lock);
kfree(ir);
}
@@ -117,7 +126,18 @@ static int lirc_allocate_buffer(struct irctl *ir)
return err;
}
-int lirc_register_driver(struct lirc_driver *d)
+/**
+ * lirc_register_driver() - create a new lirc device by registering a driver
+ * @d: the &struct lirc_driver to register
+ *
+ * This function allocates and registers a lirc device for a given
+ * &lirc_driver. The &lirc_driver structure is updated to contain
+ * information about the allocated device (minor, buffer, etc).
+ *
+ * Return: zero on success or a negative error value.
+ */
+int
+lirc_register_driver(struct lirc_driver *d)
{
struct irctl *ir;
int minor;
@@ -138,12 +158,6 @@ int lirc_register_driver(struct lirc_driver *d)
return -EINVAL;
}
- if (d->minor >= MAX_IRCTL_DEVICES) {
- dev_err(d->dev, "minor must be between 0 and %d!\n",
- MAX_IRCTL_DEVICES - 1);
- return -EBADRQC;
- }
-
if (d->code_length < 1 || d->code_length > (BUFLEN * 8)) {
dev_err(d->dev, "code length must be less than %d bits\n",
BUFLEN * 8);
@@ -156,49 +170,30 @@ int lirc_register_driver(struct lirc_driver *d)
return -EBADRQC;
}
- mutex_lock(&lirc_dev_lock);
-
- minor = d->minor;
+ d->name[sizeof(d->name)-1] = '\0';
+ if (d->features == 0)
+ d->features = LIRC_CAN_REC_LIRCCODE;
- if (minor < 0) {
- /* find first free slot for driver */
- for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++)
- if (!irctls[minor])
- break;
- if (minor == MAX_IRCTL_DEVICES) {
- dev_err(d->dev, "no free slots for drivers!\n");
- err = -ENOMEM;
- goto out_lock;
- }
- } else if (irctls[minor]) {
- dev_err(d->dev, "minor (%d) just registered!\n", minor);
- err = -EBUSY;
- goto out_lock;
- }
+ minor = ida_simple_get(&lirc_ida, 0, LIRC_MAX_DEVICES, GFP_KERNEL);
+ if (minor < 0)
+ return minor;
+ d->minor = minor;
ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
if (!ir) {
err = -ENOMEM;
- goto out_lock;
+ goto out_minor;
}
- mutex_init(&ir->irctl_lock);
- irctls[minor] = ir;
- d->minor = minor;
-
- /* some safety check 8-) */
- d->name[sizeof(d->name)-1] = '\0';
-
- if (d->features == 0)
- d->features = LIRC_CAN_REC_LIRCCODE;
+ mutex_init(&ir->mutex);
ir->d = *d;
if (LIRC_CAN_REC(d->features)) {
- err = lirc_allocate_buffer(irctls[minor]);
+ err = lirc_allocate_buffer(ir);
if (err) {
kfree(ir);
- goto out_lock;
+ goto out_minor;
}
d->rbuf = ir->buf;
}
@@ -218,107 +213,82 @@ int lirc_register_driver(struct lirc_driver *d)
if (err)
goto out_free_dev;
- ir->attached = 1;
-
err = device_add(&ir->dev);
if (err)
goto out_cdev;
- mutex_unlock(&lirc_dev_lock);
-
dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
ir->d.name, ir->d.minor);
- return minor;
+ d->lirc_internal = ir;
+ return 0;
out_cdev:
cdev_del(&ir->cdev);
out_free_dev:
put_device(&ir->dev);
-out_lock:
- mutex_unlock(&lirc_dev_lock);
+out_minor:
+ ida_simple_remove(&lirc_ida, minor);
return err;
}
EXPORT_SYMBOL(lirc_register_driver);
-int lirc_unregister_driver(int minor)
+/**
+ * lirc_unregister_driver() - remove the lirc device for a given driver
+ * @d: the &struct lirc_driver to unregister
+ *
+ * This function unregisters the lirc device for a given &lirc_driver.
+ */
+void
+lirc_unregister_driver(struct lirc_driver *d)
{
struct irctl *ir;
- if (minor < 0 || minor >= MAX_IRCTL_DEVICES) {
- pr_err("minor (%d) must be between 0 and %d!\n",
- minor, MAX_IRCTL_DEVICES - 1);
- return -EBADRQC;
- }
+ if (!d->lirc_internal)
+ return;
- ir = irctls[minor];
- if (!ir) {
- pr_err("failed to get irctl\n");
- return -ENOENT;
- }
+ ir = d->lirc_internal;
- mutex_lock(&lirc_dev_lock);
-
- if (ir->d.minor != minor) {
- dev_err(ir->d.dev, "lirc_dev: minor %d device not registered\n",
- minor);
- mutex_unlock(&lirc_dev_lock);
- return -ENOENT;
- }
+ mutex_lock(&ir->mutex);
dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
ir->d.name, ir->d.minor);
- ir->attached = 0;
- if (ir->open) {
+ ir->dead = true;
+ if (ir->users) {
dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
ir->d.name, ir->d.minor);
wake_up_interruptible(&ir->buf->wait_poll);
}
- mutex_unlock(&lirc_dev_lock);
+ mutex_unlock(&ir->mutex);
device_del(&ir->dev);
cdev_del(&ir->cdev);
- put_device(&ir->dev);
-
- return 0;
+ ida_simple_remove(&lirc_ida, d->minor);
+ d->minor = -1;
+ d->lirc_internal = NULL;
}
EXPORT_SYMBOL(lirc_unregister_driver);
int lirc_dev_fop_open(struct inode *inode, struct file *file)
{
- struct irctl *ir;
+ struct irctl *ir = container_of(inode->i_cdev, struct irctl, cdev);
int retval;
- if (iminor(inode) >= MAX_IRCTL_DEVICES) {
- pr_err("open result for %d is -ENODEV\n", iminor(inode));
- return -ENODEV;
- }
-
- if (mutex_lock_interruptible(&lirc_dev_lock))
- return -ERESTARTSYS;
+ mutex_lock(&ir->mutex);
- ir = irctls[iminor(inode)];
- mutex_unlock(&lirc_dev_lock);
-
- if (!ir) {
- retval = -ENODEV;
+ if (ir->users) {
+ retval = -EBUSY;
+ mutex_unlock(&ir->mutex);
goto error;
}
+ ir->users++;
- dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
-
- if (ir->d.minor == NOPLUG) {
- retval = -ENODEV;
- goto error;
- }
+ mutex_unlock(&ir->mutex);
- if (ir->open) {
- retval = -EBUSY;
- goto error;
- }
+ dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
if (ir->d.rdev) {
retval = rc_open(ir->d.rdev);
@@ -329,8 +299,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
if (ir->buf)
lirc_buffer_clear(ir->buf);
- ir->open++;
-
+ file->private_data = ir;
nonseekable_open(inode, file);
return 0;
@@ -342,22 +311,14 @@ EXPORT_SYMBOL(lirc_dev_fop_open);
int lirc_dev_fop_close(struct inode *inode, struct file *file)
{
- struct irctl *ir = irctls[iminor(inode)];
- int ret;
-
- if (!ir) {
- pr_err("called with invalid irctl\n");
- return -EINVAL;
- }
+ struct irctl *ir = file->private_data;
- ret = mutex_lock_killable(&lirc_dev_lock);
- WARN_ON(ret);
+ mutex_lock(&ir->mutex);
rc_close(ir->d.rdev);
+ ir->users--;
- ir->open--;
- if (!ret)
- mutex_unlock(&lirc_dev_lock);
+ mutex_unlock(&ir->mutex);
return 0;
}
@@ -365,26 +326,21 @@ EXPORT_SYMBOL(lirc_dev_fop_close);
unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
{
- struct irctl *ir = irctls[iminor(file_inode(file))];
+ struct irctl *ir = file->private_data;
unsigned int ret;
- if (!ir) {
- pr_err("called with invalid irctl\n");
- return POLLERR;
- }
-
- if (!ir->attached)
+ if (ir->dead)
return POLLHUP | POLLERR;
- if (ir->buf) {
- poll_wait(file, &ir->buf->wait_poll, wait);
+ if (!ir->buf)
+ return POLLERR;
+
+ poll_wait(file, &ir->buf->wait_poll, wait);
- if (lirc_buffer_empty(ir->buf))
- ret = 0;
- else
- ret = POLLIN | POLLRDNORM;
- } else
- ret = POLLERR;
+ if (lirc_buffer_empty(ir->buf))
+ ret = 0;
+ else
+ ret = POLLIN | POLLRDNORM;
dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n",
ir->d.name, ir->d.minor, ret);
@@ -395,25 +351,20 @@ EXPORT_SYMBOL(lirc_dev_fop_poll);
long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
+ struct irctl *ir = file->private_data;
__u32 mode;
int result = 0;
- struct irctl *ir = irctls[iminor(file_inode(file))];
-
- if (!ir) {
- pr_err("no irctl found!\n");
- return -ENODEV;
- }
dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
ir->d.name, ir->d.minor, cmd);
- if (ir->d.minor == NOPLUG || !ir->attached) {
+ if (ir->dead) {
dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
ir->d.name, ir->d.minor);
return -ENODEV;
}
- mutex_lock(&ir->irctl_lock);
+ mutex_lock(&ir->mutex);
switch (cmd) {
case LIRC_GET_FEATURES:
@@ -468,7 +419,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
result = -ENOTTY;
}
- mutex_unlock(&ir->irctl_lock);
+ mutex_unlock(&ir->mutex);
return result;
}
@@ -479,16 +430,11 @@ ssize_t lirc_dev_fop_read(struct file *file,
size_t length,
loff_t *ppos)
{
- struct irctl *ir = irctls[iminor(file_inode(file))];
+ struct irctl *ir = file->private_data;
unsigned char *buf;
int ret = 0, written = 0;
DECLARE_WAITQUEUE(wait, current);
- if (!ir) {
- pr_err("called with invalid irctl\n");
- return -ENODEV;
- }
-
if (!LIRC_CAN_REC(ir->d.features))
return -EINVAL;
@@ -498,11 +444,12 @@ ssize_t lirc_dev_fop_read(struct file *file,
if (!buf)
return -ENOMEM;
- if (mutex_lock_interruptible(&ir->irctl_lock)) {
+ if (mutex_lock_interruptible(&ir->mutex)) {
ret = -ERESTARTSYS;
goto out_unlocked;
}
- if (!ir->attached) {
+
+ if (ir->dead) {
ret = -ENODEV;
goto out_locked;
}
@@ -541,18 +488,18 @@ ssize_t lirc_dev_fop_read(struct file *file,
break;
}
- mutex_unlock(&ir->irctl_lock);
+ mutex_unlock(&ir->mutex);
set_current_state(TASK_INTERRUPTIBLE);
schedule();
set_current_state(TASK_RUNNING);
- if (mutex_lock_interruptible(&ir->irctl_lock)) {
+ if (mutex_lock_interruptible(&ir->mutex)) {
ret = -ERESTARTSYS;
remove_wait_queue(&ir->buf->wait_poll, &wait);
goto out_unlocked;
}
- if (!ir->attached) {
+ if (ir->dead) {
ret = -ENODEV;
goto out_locked;
}
@@ -570,7 +517,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
remove_wait_queue(&ir->buf->wait_poll, &wait);
out_locked:
- mutex_unlock(&ir->irctl_lock);
+ mutex_unlock(&ir->mutex);
out_unlocked:
kfree(buf);
@@ -581,7 +528,8 @@ EXPORT_SYMBOL(lirc_dev_fop_read);
void *lirc_get_pdata(struct file *file)
{
- return irctls[iminor(file_inode(file))]->d.data;
+ struct irctl *ir = file->private_data;
+ return ir->d.data;
}
EXPORT_SYMBOL(lirc_get_pdata);
@@ -596,7 +544,7 @@ static int __init lirc_dev_init(void)
return PTR_ERR(lirc_class);
}
- retval = alloc_chrdev_region(&lirc_base_dev, 0, MAX_IRCTL_DEVICES,
+ retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
IRCTL_DEV_NAME);
if (retval) {
class_destroy(lirc_class);
@@ -613,7 +561,7 @@ static int __init lirc_dev_init(void)
static void __exit lirc_dev_exit(void)
{
class_destroy(lirc_class);
- unregister_chrdev_region(lirc_base_dev, MAX_IRCTL_DEVICES);
+ unregister_chrdev_region(lirc_base_dev, LIRC_MAX_DEVICES);
pr_info("module unloaded\n");
}
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index 59e05aa1bc55..ffb70dee4547 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -183,11 +183,7 @@ static void release_ir_device(struct kref *ref)
* ir->open_count == 0 - happens on final close()
* ir_lock, tx_ref_lock, rx_ref_lock, all released
*/
- if (ir->l.minor >= 0) {
- lirc_unregister_driver(ir->l.minor);
- ir->l.minor = -1;
- }
-
+ lirc_unregister_driver(&ir->l);
if (kfifo_initialized(&ir->rbuf.fifo))
lirc_buffer_free(&ir->rbuf);
list_del(&ir->list);
@@ -1382,7 +1378,6 @@ static const struct file_operations lirc_fops = {
static struct lirc_driver lirc_template = {
.name = "lirc_zilog",
- .minor = -1,
.code_length = 13,
.buffer_size = BUFLEN / 2,
.chunk_size = 2,
@@ -1597,14 +1592,13 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
}
/* register with lirc */
- ir->l.minor = lirc_register_driver(&ir->l);
- if (ir->l.minor < 0) {
+ ret = lirc_register_driver(&ir->l);
+ if (ret < 0) {
dev_err(tx->ir->l.dev,
- "%s: lirc_register_driver() failed: %i\n",
- __func__, ir->l.minor);
- ret = -EBADRQC;
+ "%s: lirc_register_driver failed: %i\n", __func__, ret);
goto out_put_xx;
}
+
dev_info(ir->l.dev,
"IR unit on %s (i2c-%d) registered as lirc%d and ready\n",
adap->name, adap->nr, ir->l.minor);
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 1f327e25a9be..f7629ff116a9 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -9,7 +9,6 @@
#ifndef _LINUX_LIRC_DEV_H
#define _LINUX_LIRC_DEV_H
-#define MAX_IRCTL_DEVICES 8
#define BUFLEN 16
#define mod(n, div) ((n) % (div))
@@ -164,6 +163,8 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
* device.
*
* @owner: the module owning this struct
+ *
+ * @lirc_internal: lirc_dev bookkeeping data, don't touch.
*/
struct lirc_driver {
char name[40];
@@ -182,19 +183,12 @@ struct lirc_driver {
const struct file_operations *fops;
struct device *dev;
struct module *owner;
+ void *lirc_internal;
};
-/* following functions can be called ONLY from user context
- *
- * returns negative value on error or minor number
- * of the registered device if success
- * contents of the structure pointed by p is copied
- */
extern int lirc_register_driver(struct lirc_driver *d);
-/* returns negative value on error or 0 if success
-*/
-extern int lirc_unregister_driver(int minor);
+extern void lirc_unregister_driver(struct lirc_driver *d);
/* Returns the private data stored in the lirc_driver
* associated with the given device file pointer.
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors
2017-05-01 16:04 ` [PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors David Härdeman
@ 2017-05-22 20:09 ` Sean Young
2017-05-28 8:26 ` David Härdeman
0 siblings, 1 reply; 28+ messages in thread
From: Sean Young @ 2017-05-22 20:09 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, mchehab
On Mon, May 01, 2017 at 06:04:42PM +0200, David Härdeman wrote:
> Using the kernel ida facilities, we can avoid a lot of unnecessary code and at the same
> time get rid of lirc_dev_lock in favour of per-device locks (the irctls array was used
> throughout lirc_dev so this patch necessarily touches a lot of code).
>
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
> drivers/media/rc/ir-lirc-codec.c | 9 -
> drivers/media/rc/lirc_dev.c | 258 ++++++++++++-------------------
> drivers/staging/media/lirc/lirc_zilog.c | 16 +-
> include/media/lirc_dev.h | 14 --
> 4 files changed, 115 insertions(+), 182 deletions(-)
>
> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> index a30af91710fe..2c1221a61ea1 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -382,7 +382,6 @@ static int ir_lirc_register(struct rc_dev *dev)
>
> snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
> dev->driver_name);
> - drv->minor = -1;
> drv->features = features;
> drv->data = &dev->raw->lirc;
> drv->rbuf = NULL;
> @@ -394,11 +393,9 @@ static int ir_lirc_register(struct rc_dev *dev)
> drv->rdev = dev;
> drv->owner = THIS_MODULE;
>
> - drv->minor = lirc_register_driver(drv);
> - if (drv->minor < 0) {
> - rc = -ENODEV;
> + rc = lirc_register_driver(drv);
> + if (rc < 0)
> goto out;
> - }
>
> dev->raw->lirc.drv = drv;
> dev->raw->lirc.dev = dev;
> @@ -413,7 +410,7 @@ static int ir_lirc_unregister(struct rc_dev *dev)
> {
> struct lirc_codec *lirc = &dev->raw->lirc;
>
> - lirc_unregister_driver(lirc->drv->minor);
> + lirc_unregister_driver(lirc->drv);
> kfree(lirc->drv);
> lirc->drv = NULL;
>
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index e44e0b23b883..7db7d4c57991 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -31,23 +31,35 @@
> #include <linux/bitops.h>
> #include <linux/device.h>
> #include <linux/cdev.h>
> +#include <linux/idr.h>
>
> #include <media/rc-core.h>
> #include <media/lirc.h>
> #include <media/lirc_dev.h>
>
> #define IRCTL_DEV_NAME "BaseRemoteCtl"
> -#define NOPLUG -1
> #define LOGHEAD "lirc_dev (%s[%d]): "
>
> static dev_t lirc_base_dev;
>
> +/**
> + * struct irctl - lirc per-device structure
> + * @d: internal copy of the &struct lirc_driver for the device
> + * @dead: if the device has gone away
> + * @users: the number of users of the lirc chardev (currently limited to 1)
> + * @mutex: synchronizes open()/close()/ioctl()/etc calls
> + * @buf: used to store lirc RX data
> + * @buf_internal: whether @buf was allocated internally or not
> + * @chunk_size: @buf stores and read() returns data chunks of this size
> + * @dev: the &struct device for the lirc device
> + * @cdev: the &struct device for the lirc chardev
> + */
> struct irctl {
> struct lirc_driver d;
> - int attached;
> - int open;
> + bool dead;
> + unsigned users;
>
> - struct mutex irctl_lock;
> + struct mutex mutex;
> struct lirc_buffer *buf;
> bool buf_internal;
> unsigned int chunk_size;
> @@ -56,9 +68,9 @@ struct irctl {
> struct cdev cdev;
> };
>
> -static DEFINE_MUTEX(lirc_dev_lock);
> -
> -static struct irctl *irctls[MAX_IRCTL_DEVICES];
> +/* Used to keep track of allocated lirc devices */
> +#define LIRC_MAX_DEVICES 256
> +static DEFINE_IDA(lirc_ida);
>
> /* Only used for sysfs but defined to void otherwise */
> static struct class *lirc_class;
> @@ -72,9 +84,6 @@ static void lirc_release(struct device *ld)
> kfree(ir->buf);
> }
>
> - mutex_lock(&lirc_dev_lock);
> - irctls[ir->d.minor] = NULL;
> - mutex_unlock(&lirc_dev_lock);
> kfree(ir);
> }
>
> @@ -117,7 +126,18 @@ static int lirc_allocate_buffer(struct irctl *ir)
> return err;
> }
>
> -int lirc_register_driver(struct lirc_driver *d)
> +/**
> + * lirc_register_driver() - create a new lirc device by registering a driver
> + * @d: the &struct lirc_driver to register
> + *
> + * This function allocates and registers a lirc device for a given
> + * &lirc_driver. The &lirc_driver structure is updated to contain
> + * information about the allocated device (minor, buffer, etc).
> + *
> + * Return: zero on success or a negative error value.
> + */
> +int
> +lirc_register_driver(struct lirc_driver *d)
> {
> struct irctl *ir;
> int minor;
> @@ -138,12 +158,6 @@ int lirc_register_driver(struct lirc_driver *d)
> return -EINVAL;
> }
>
> - if (d->minor >= MAX_IRCTL_DEVICES) {
> - dev_err(d->dev, "minor must be between 0 and %d!\n",
> - MAX_IRCTL_DEVICES - 1);
> - return -EBADRQC;
> - }
> -
> if (d->code_length < 1 || d->code_length > (BUFLEN * 8)) {
> dev_err(d->dev, "code length must be less than %d bits\n",
> BUFLEN * 8);
> @@ -156,49 +170,30 @@ int lirc_register_driver(struct lirc_driver *d)
> return -EBADRQC;
> }
>
> - mutex_lock(&lirc_dev_lock);
> -
> - minor = d->minor;
> + d->name[sizeof(d->name)-1] = '\0';
> + if (d->features == 0)
> + d->features = LIRC_CAN_REC_LIRCCODE;
>
> - if (minor < 0) {
> - /* find first free slot for driver */
> - for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++)
> - if (!irctls[minor])
> - break;
> - if (minor == MAX_IRCTL_DEVICES) {
> - dev_err(d->dev, "no free slots for drivers!\n");
> - err = -ENOMEM;
> - goto out_lock;
> - }
> - } else if (irctls[minor]) {
> - dev_err(d->dev, "minor (%d) just registered!\n", minor);
> - err = -EBUSY;
> - goto out_lock;
> - }
> + minor = ida_simple_get(&lirc_ida, 0, LIRC_MAX_DEVICES, GFP_KERNEL);
> + if (minor < 0)
> + return minor;
> + d->minor = minor;
>
> ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
> if (!ir) {
> err = -ENOMEM;
> - goto out_lock;
> + goto out_minor;
> }
>
> - mutex_init(&ir->irctl_lock);
> - irctls[minor] = ir;
> - d->minor = minor;
> -
> - /* some safety check 8-) */
> - d->name[sizeof(d->name)-1] = '\0';
> -
> - if (d->features == 0)
> - d->features = LIRC_CAN_REC_LIRCCODE;
> + mutex_init(&ir->mutex);
>
> ir->d = *d;
Here we copy lirc_driver.
>
> if (LIRC_CAN_REC(d->features)) {
> - err = lirc_allocate_buffer(irctls[minor]);
> + err = lirc_allocate_buffer(ir);
> if (err) {
> kfree(ir);
> - goto out_lock;
> + goto out_minor;
> }
> d->rbuf = ir->buf;
> }
> @@ -218,107 +213,82 @@ int lirc_register_driver(struct lirc_driver *d)
> if (err)
> goto out_free_dev;
>
> - ir->attached = 1;
> -
> err = device_add(&ir->dev);
> if (err)
> goto out_cdev;
>
> - mutex_unlock(&lirc_dev_lock);
> -
> dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
> ir->d.name, ir->d.minor);
>
> - return minor;
> + d->lirc_internal = ir;
And now a store a pointer to the copy in the original lirc_driver.
It would be much better to not copy lirc_driver and the lirc_internal
member would be unnecessary.
> + return 0;
>
> out_cdev:
> cdev_del(&ir->cdev);
> out_free_dev:
> put_device(&ir->dev);
> -out_lock:
> - mutex_unlock(&lirc_dev_lock);
> +out_minor:
> + ida_simple_remove(&lirc_ida, minor);
>
> return err;
> }
> EXPORT_SYMBOL(lirc_register_driver);
>
> -int lirc_unregister_driver(int minor)
> +/**
> + * lirc_unregister_driver() - remove the lirc device for a given driver
> + * @d: the &struct lirc_driver to unregister
> + *
> + * This function unregisters the lirc device for a given &lirc_driver.
> + */
> +void
> +lirc_unregister_driver(struct lirc_driver *d)
> {
> struct irctl *ir;
>
> - if (minor < 0 || minor >= MAX_IRCTL_DEVICES) {
> - pr_err("minor (%d) must be between 0 and %d!\n",
> - minor, MAX_IRCTL_DEVICES - 1);
> - return -EBADRQC;
> - }
> + if (!d->lirc_internal)
> + return;
>
> - ir = irctls[minor];
> - if (!ir) {
> - pr_err("failed to get irctl\n");
> - return -ENOENT;
> - }
> + ir = d->lirc_internal;
This is done to find a our copy again.
> - mutex_lock(&lirc_dev_lock);
> -
> - if (ir->d.minor != minor) {
> - dev_err(ir->d.dev, "lirc_dev: minor %d device not registered\n",
> - minor);
> - mutex_unlock(&lirc_dev_lock);
> - return -ENOENT;
> - }
> + mutex_lock(&ir->mutex);
>
> dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
> ir->d.name, ir->d.minor);
>
> - ir->attached = 0;
> - if (ir->open) {
> + ir->dead = true;
> + if (ir->users) {
> dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
> ir->d.name, ir->d.minor);
> wake_up_interruptible(&ir->buf->wait_poll);
> }
>
> - mutex_unlock(&lirc_dev_lock);
> + mutex_unlock(&ir->mutex);
>
> device_del(&ir->dev);
> cdev_del(&ir->cdev);
> - put_device(&ir->dev);
> -
> - return 0;
> + ida_simple_remove(&lirc_ida, d->minor);
> + d->minor = -1;
> + d->lirc_internal = NULL;
> }
> EXPORT_SYMBOL(lirc_unregister_driver);
>
> int lirc_dev_fop_open(struct inode *inode, struct file *file)
> {
> - struct irctl *ir;
> + struct irctl *ir = container_of(inode->i_cdev, struct irctl, cdev);
> int retval;
>
> - if (iminor(inode) >= MAX_IRCTL_DEVICES) {
> - pr_err("open result for %d is -ENODEV\n", iminor(inode));
> - return -ENODEV;
> - }
> -
> - if (mutex_lock_interruptible(&lirc_dev_lock))
> - return -ERESTARTSYS;
> + mutex_lock(&ir->mutex);
>
> - ir = irctls[iminor(inode)];
> - mutex_unlock(&lirc_dev_lock);
> -
> - if (!ir) {
> - retval = -ENODEV;
> + if (ir->users) {
> + retval = -EBUSY;
> + mutex_unlock(&ir->mutex);
> goto error;
> }
> + ir->users++;
>
> - dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
> -
> - if (ir->d.minor == NOPLUG) {
> - retval = -ENODEV;
> - goto error;
> - }
> + mutex_unlock(&ir->mutex);
>
> - if (ir->open) {
> - retval = -EBUSY;
> - goto error;
> - }
> + dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
>
> if (ir->d.rdev) {
> retval = rc_open(ir->d.rdev);
> @@ -329,8 +299,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
> if (ir->buf)
> lirc_buffer_clear(ir->buf);
>
> - ir->open++;
> -
> + file->private_data = ir;
> nonseekable_open(inode, file);
>
> return 0;
> @@ -342,22 +311,14 @@ EXPORT_SYMBOL(lirc_dev_fop_open);
>
> int lirc_dev_fop_close(struct inode *inode, struct file *file)
> {
> - struct irctl *ir = irctls[iminor(inode)];
> - int ret;
> -
> - if (!ir) {
> - pr_err("called with invalid irctl\n");
> - return -EINVAL;
> - }
> + struct irctl *ir = file->private_data;
>
> - ret = mutex_lock_killable(&lirc_dev_lock);
> - WARN_ON(ret);
> + mutex_lock(&ir->mutex);
>
> rc_close(ir->d.rdev);
> + ir->users--;
>
> - ir->open--;
> - if (!ret)
> - mutex_unlock(&lirc_dev_lock);
> + mutex_unlock(&ir->mutex);
>
> return 0;
> }
> @@ -365,26 +326,21 @@ EXPORT_SYMBOL(lirc_dev_fop_close);
>
> unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
> {
> - struct irctl *ir = irctls[iminor(file_inode(file))];
> + struct irctl *ir = file->private_data;
> unsigned int ret;
>
> - if (!ir) {
> - pr_err("called with invalid irctl\n");
> - return POLLERR;
> - }
> -
> - if (!ir->attached)
> + if (ir->dead)
> return POLLHUP | POLLERR;
>
> - if (ir->buf) {
> - poll_wait(file, &ir->buf->wait_poll, wait);
> + if (!ir->buf)
> + return POLLERR;
> +
> + poll_wait(file, &ir->buf->wait_poll, wait);
>
> - if (lirc_buffer_empty(ir->buf))
> - ret = 0;
> - else
> - ret = POLLIN | POLLRDNORM;
> - } else
> - ret = POLLERR;
> + if (lirc_buffer_empty(ir->buf))
> + ret = 0;
> + else
> + ret = POLLIN | POLLRDNORM;
>
> dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n",
> ir->d.name, ir->d.minor, ret);
> @@ -395,25 +351,20 @@ EXPORT_SYMBOL(lirc_dev_fop_poll);
>
> long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> + struct irctl *ir = file->private_data;
> __u32 mode;
> int result = 0;
> - struct irctl *ir = irctls[iminor(file_inode(file))];
> -
> - if (!ir) {
> - pr_err("no irctl found!\n");
> - return -ENODEV;
> - }
>
> dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
> ir->d.name, ir->d.minor, cmd);
>
> - if (ir->d.minor == NOPLUG || !ir->attached) {
> + if (ir->dead) {
> dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
> ir->d.name, ir->d.minor);
> return -ENODEV;
> }
>
> - mutex_lock(&ir->irctl_lock);
> + mutex_lock(&ir->mutex);
>
> switch (cmd) {
> case LIRC_GET_FEATURES:
> @@ -468,7 +419,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> result = -ENOTTY;
> }
>
> - mutex_unlock(&ir->irctl_lock);
> + mutex_unlock(&ir->mutex);
>
> return result;
> }
> @@ -479,16 +430,11 @@ ssize_t lirc_dev_fop_read(struct file *file,
> size_t length,
> loff_t *ppos)
> {
> - struct irctl *ir = irctls[iminor(file_inode(file))];
> + struct irctl *ir = file->private_data;
> unsigned char *buf;
> int ret = 0, written = 0;
> DECLARE_WAITQUEUE(wait, current);
>
> - if (!ir) {
> - pr_err("called with invalid irctl\n");
> - return -ENODEV;
> - }
> -
> if (!LIRC_CAN_REC(ir->d.features))
> return -EINVAL;
>
> @@ -498,11 +444,12 @@ ssize_t lirc_dev_fop_read(struct file *file,
> if (!buf)
> return -ENOMEM;
>
> - if (mutex_lock_interruptible(&ir->irctl_lock)) {
> + if (mutex_lock_interruptible(&ir->mutex)) {
> ret = -ERESTARTSYS;
> goto out_unlocked;
> }
> - if (!ir->attached) {
> +
> + if (ir->dead) {
> ret = -ENODEV;
> goto out_locked;
> }
> @@ -541,18 +488,18 @@ ssize_t lirc_dev_fop_read(struct file *file,
> break;
> }
>
> - mutex_unlock(&ir->irctl_lock);
> + mutex_unlock(&ir->mutex);
> set_current_state(TASK_INTERRUPTIBLE);
> schedule();
> set_current_state(TASK_RUNNING);
>
> - if (mutex_lock_interruptible(&ir->irctl_lock)) {
> + if (mutex_lock_interruptible(&ir->mutex)) {
> ret = -ERESTARTSYS;
> remove_wait_queue(&ir->buf->wait_poll, &wait);
> goto out_unlocked;
> }
>
> - if (!ir->attached) {
> + if (ir->dead) {
> ret = -ENODEV;
> goto out_locked;
> }
> @@ -570,7 +517,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
> remove_wait_queue(&ir->buf->wait_poll, &wait);
>
> out_locked:
> - mutex_unlock(&ir->irctl_lock);
> + mutex_unlock(&ir->mutex);
>
> out_unlocked:
> kfree(buf);
> @@ -581,7 +528,8 @@ EXPORT_SYMBOL(lirc_dev_fop_read);
>
> void *lirc_get_pdata(struct file *file)
> {
> - return irctls[iminor(file_inode(file))]->d.data;
> + struct irctl *ir = file->private_data;
> + return ir->d.data;
> }
> EXPORT_SYMBOL(lirc_get_pdata);
>
> @@ -596,7 +544,7 @@ static int __init lirc_dev_init(void)
> return PTR_ERR(lirc_class);
> }
>
> - retval = alloc_chrdev_region(&lirc_base_dev, 0, MAX_IRCTL_DEVICES,
> + retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
> IRCTL_DEV_NAME);
> if (retval) {
> class_destroy(lirc_class);
> @@ -613,7 +561,7 @@ static int __init lirc_dev_init(void)
> static void __exit lirc_dev_exit(void)
> {
> class_destroy(lirc_class);
> - unregister_chrdev_region(lirc_base_dev, MAX_IRCTL_DEVICES);
> + unregister_chrdev_region(lirc_base_dev, LIRC_MAX_DEVICES);
> pr_info("module unloaded\n");
> }
>
> diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
> index 59e05aa1bc55..ffb70dee4547 100644
> --- a/drivers/staging/media/lirc/lirc_zilog.c
> +++ b/drivers/staging/media/lirc/lirc_zilog.c
> @@ -183,11 +183,7 @@ static void release_ir_device(struct kref *ref)
> * ir->open_count == 0 - happens on final close()
> * ir_lock, tx_ref_lock, rx_ref_lock, all released
> */
> - if (ir->l.minor >= 0) {
> - lirc_unregister_driver(ir->l.minor);
> - ir->l.minor = -1;
> - }
> -
> + lirc_unregister_driver(&ir->l);
> if (kfifo_initialized(&ir->rbuf.fifo))
> lirc_buffer_free(&ir->rbuf);
> list_del(&ir->list);
> @@ -1382,7 +1378,6 @@ static const struct file_operations lirc_fops = {
>
> static struct lirc_driver lirc_template = {
> .name = "lirc_zilog",
> - .minor = -1,
> .code_length = 13,
> .buffer_size = BUFLEN / 2,
> .chunk_size = 2,
> @@ -1597,14 +1592,13 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
> }
>
> /* register with lirc */
> - ir->l.minor = lirc_register_driver(&ir->l);
> - if (ir->l.minor < 0) {
> + ret = lirc_register_driver(&ir->l);
> + if (ret < 0) {
> dev_err(tx->ir->l.dev,
> - "%s: lirc_register_driver() failed: %i\n",
> - __func__, ir->l.minor);
> - ret = -EBADRQC;
> + "%s: lirc_register_driver failed: %i\n", __func__, ret);
> goto out_put_xx;
> }
> +
> dev_info(ir->l.dev,
> "IR unit on %s (i2c-%d) registered as lirc%d and ready\n",
> adap->name, adap->nr, ir->l.minor);
> diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
> index 1f327e25a9be..f7629ff116a9 100644
> --- a/include/media/lirc_dev.h
> +++ b/include/media/lirc_dev.h
> @@ -9,7 +9,6 @@
> #ifndef _LINUX_LIRC_DEV_H
> #define _LINUX_LIRC_DEV_H
>
> -#define MAX_IRCTL_DEVICES 8
> #define BUFLEN 16
>
> #define mod(n, div) ((n) % (div))
> @@ -164,6 +163,8 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
> * device.
> *
> * @owner: the module owning this struct
> + *
> + * @lirc_internal: lirc_dev bookkeeping data, don't touch.
It's not a great name or comment :)
Otherwise, it's a good idea.
Sean
> */
> struct lirc_driver {
> char name[40];
> @@ -182,19 +183,12 @@ struct lirc_driver {
> const struct file_operations *fops;
> struct device *dev;
> struct module *owner;
> + void *lirc_internal;
> };
>
> -/* following functions can be called ONLY from user context
> - *
> - * returns negative value on error or minor number
> - * of the registered device if success
> - * contents of the structure pointed by p is copied
> - */
> extern int lirc_register_driver(struct lirc_driver *d);
>
> -/* returns negative value on error or 0 if success
> -*/
> -extern int lirc_unregister_driver(int minor);
> +extern void lirc_unregister_driver(struct lirc_driver *d);
>
> /* Returns the private data stored in the lirc_driver
> * associated with the given device file pointer.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors
2017-05-22 20:09 ` Sean Young
@ 2017-05-28 8:26 ` David Härdeman
2017-05-28 15:08 ` Sean Young
0 siblings, 1 reply; 28+ messages in thread
From: David Härdeman @ 2017-05-28 8:26 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, mchehab
On Mon, May 22, 2017 at 09:09:42PM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:04:42PM +0200, David Härdeman wrote:
>> Using the kernel ida facilities, we can avoid a lot of unnecessary code and at the same
>> time get rid of lirc_dev_lock in favour of per-device locks (the irctls array was used
>> throughout lirc_dev so this patch necessarily touches a lot of code).
>>
>> Signed-off-by: David Härdeman <david@hardeman.nu>
>> ---
>> drivers/media/rc/ir-lirc-codec.c | 9 -
>> drivers/media/rc/lirc_dev.c | 258 ++++++++++++-------------------
>> drivers/staging/media/lirc/lirc_zilog.c | 16 +-
>> include/media/lirc_dev.h | 14 --
>> 4 files changed, 115 insertions(+), 182 deletions(-)
>>
>> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
>> index a30af91710fe..2c1221a61ea1 100644
>> --- a/drivers/media/rc/ir-lirc-codec.c
>> +++ b/drivers/media/rc/ir-lirc-codec.c
>> @@ -382,7 +382,6 @@ static int ir_lirc_register(struct rc_dev *dev)
>>
>> snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
>> dev->driver_name);
>> - drv->minor = -1;
>> drv->features = features;
>> drv->data = &dev->raw->lirc;
>> drv->rbuf = NULL;
>> @@ -394,11 +393,9 @@ static int ir_lirc_register(struct rc_dev *dev)
>> drv->rdev = dev;
>> drv->owner = THIS_MODULE;
>>
>> - drv->minor = lirc_register_driver(drv);
>> - if (drv->minor < 0) {
>> - rc = -ENODEV;
>> + rc = lirc_register_driver(drv);
>> + if (rc < 0)
>> goto out;
>> - }
>>
>> dev->raw->lirc.drv = drv;
>> dev->raw->lirc.dev = dev;
>> @@ -413,7 +410,7 @@ static int ir_lirc_unregister(struct rc_dev *dev)
>> {
>> struct lirc_codec *lirc = &dev->raw->lirc;
>>
>> - lirc_unregister_driver(lirc->drv->minor);
>> + lirc_unregister_driver(lirc->drv);
>> kfree(lirc->drv);
>> lirc->drv = NULL;
>>
>> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
>> index e44e0b23b883..7db7d4c57991 100644
>> --- a/drivers/media/rc/lirc_dev.c
>> +++ b/drivers/media/rc/lirc_dev.c
>> @@ -31,23 +31,35 @@
>> #include <linux/bitops.h>
>> #include <linux/device.h>
>> #include <linux/cdev.h>
>> +#include <linux/idr.h>
>>
>> #include <media/rc-core.h>
>> #include <media/lirc.h>
>> #include <media/lirc_dev.h>
>>
>> #define IRCTL_DEV_NAME "BaseRemoteCtl"
>> -#define NOPLUG -1
>> #define LOGHEAD "lirc_dev (%s[%d]): "
>>
>> static dev_t lirc_base_dev;
>>
>> +/**
>> + * struct irctl - lirc per-device structure
>> + * @d: internal copy of the &struct lirc_driver for the device
>> + * @dead: if the device has gone away
>> + * @users: the number of users of the lirc chardev (currently limited to 1)
>> + * @mutex: synchronizes open()/close()/ioctl()/etc calls
>> + * @buf: used to store lirc RX data
>> + * @buf_internal: whether @buf was allocated internally or not
>> + * @chunk_size: @buf stores and read() returns data chunks of this size
>> + * @dev: the &struct device for the lirc device
>> + * @cdev: the &struct device for the lirc chardev
>> + */
>> struct irctl {
>> struct lirc_driver d;
>> - int attached;
>> - int open;
>> + bool dead;
>> + unsigned users;
>>
>> - struct mutex irctl_lock;
>> + struct mutex mutex;
>> struct lirc_buffer *buf;
>> bool buf_internal;
>> unsigned int chunk_size;
>> @@ -56,9 +68,9 @@ struct irctl {
>> struct cdev cdev;
>> };
>>
>> -static DEFINE_MUTEX(lirc_dev_lock);
>> -
>> -static struct irctl *irctls[MAX_IRCTL_DEVICES];
>> +/* Used to keep track of allocated lirc devices */
>> +#define LIRC_MAX_DEVICES 256
>> +static DEFINE_IDA(lirc_ida);
>>
>> /* Only used for sysfs but defined to void otherwise */
>> static struct class *lirc_class;
>> @@ -72,9 +84,6 @@ static void lirc_release(struct device *ld)
>> kfree(ir->buf);
>> }
>>
>> - mutex_lock(&lirc_dev_lock);
>> - irctls[ir->d.minor] = NULL;
>> - mutex_unlock(&lirc_dev_lock);
>> kfree(ir);
>> }
>>
>> @@ -117,7 +126,18 @@ static int lirc_allocate_buffer(struct irctl *ir)
>> return err;
>> }
>>
>> -int lirc_register_driver(struct lirc_driver *d)
>> +/**
>> + * lirc_register_driver() - create a new lirc device by registering a driver
>> + * @d: the &struct lirc_driver to register
>> + *
>> + * This function allocates and registers a lirc device for a given
>> + * &lirc_driver. The &lirc_driver structure is updated to contain
>> + * information about the allocated device (minor, buffer, etc).
>> + *
>> + * Return: zero on success or a negative error value.
>> + */
>> +int
>> +lirc_register_driver(struct lirc_driver *d)
>> {
>> struct irctl *ir;
>> int minor;
>> @@ -138,12 +158,6 @@ int lirc_register_driver(struct lirc_driver *d)
>> return -EINVAL;
>> }
>>
>> - if (d->minor >= MAX_IRCTL_DEVICES) {
>> - dev_err(d->dev, "minor must be between 0 and %d!\n",
>> - MAX_IRCTL_DEVICES - 1);
>> - return -EBADRQC;
>> - }
>> -
>> if (d->code_length < 1 || d->code_length > (BUFLEN * 8)) {
>> dev_err(d->dev, "code length must be less than %d bits\n",
>> BUFLEN * 8);
>> @@ -156,49 +170,30 @@ int lirc_register_driver(struct lirc_driver *d)
>> return -EBADRQC;
>> }
>>
>> - mutex_lock(&lirc_dev_lock);
>> -
>> - minor = d->minor;
>> + d->name[sizeof(d->name)-1] = '\0';
>> + if (d->features == 0)
>> + d->features = LIRC_CAN_REC_LIRCCODE;
>>
>> - if (minor < 0) {
>> - /* find first free slot for driver */
>> - for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++)
>> - if (!irctls[minor])
>> - break;
>> - if (minor == MAX_IRCTL_DEVICES) {
>> - dev_err(d->dev, "no free slots for drivers!\n");
>> - err = -ENOMEM;
>> - goto out_lock;
>> - }
>> - } else if (irctls[minor]) {
>> - dev_err(d->dev, "minor (%d) just registered!\n", minor);
>> - err = -EBUSY;
>> - goto out_lock;
>> - }
>> + minor = ida_simple_get(&lirc_ida, 0, LIRC_MAX_DEVICES, GFP_KERNEL);
>> + if (minor < 0)
>> + return minor;
>> + d->minor = minor;
>>
>> ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
>> if (!ir) {
>> err = -ENOMEM;
>> - goto out_lock;
>> + goto out_minor;
>> }
>>
>> - mutex_init(&ir->irctl_lock);
>> - irctls[minor] = ir;
>> - d->minor = minor;
>> -
>> - /* some safety check 8-) */
>> - d->name[sizeof(d->name)-1] = '\0';
>> -
>> - if (d->features == 0)
>> - d->features = LIRC_CAN_REC_LIRCCODE;
>> + mutex_init(&ir->mutex);
>>
>> ir->d = *d;
>
>Here we copy lirc_driver.
>
>>
>> if (LIRC_CAN_REC(d->features)) {
>> - err = lirc_allocate_buffer(irctls[minor]);
>> + err = lirc_allocate_buffer(ir);
>> if (err) {
>> kfree(ir);
>> - goto out_lock;
>> + goto out_minor;
>> }
>> d->rbuf = ir->buf;
>> }
>> @@ -218,107 +213,82 @@ int lirc_register_driver(struct lirc_driver *d)
>> if (err)
>> goto out_free_dev;
>>
>> - ir->attached = 1;
>> -
>> err = device_add(&ir->dev);
>> if (err)
>> goto out_cdev;
>>
>> - mutex_unlock(&lirc_dev_lock);
>> -
>> dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
>> ir->d.name, ir->d.minor);
>>
>> - return minor;
>> + d->lirc_internal = ir;
>
>And now a store a pointer to the copy in the original lirc_driver.
>
>It would be much better to not copy lirc_driver and the lirc_internal
>member would be unnecessary.
I know, but this is a more minimal fix. The struct copy is already in
the current code and I'd prefer removing it in a separate patch once the
use of lirc_driver has been vetted.
>> + return 0;
>>
>> out_cdev:
>> cdev_del(&ir->cdev);
>> out_free_dev:
>> put_device(&ir->dev);
>> -out_lock:
>> - mutex_unlock(&lirc_dev_lock);
>> +out_minor:
>> + ida_simple_remove(&lirc_ida, minor);
>>
>> return err;
>> }
>> EXPORT_SYMBOL(lirc_register_driver);
>>
>> -int lirc_unregister_driver(int minor)
>> +/**
>> + * lirc_unregister_driver() - remove the lirc device for a given driver
>> + * @d: the &struct lirc_driver to unregister
>> + *
>> + * This function unregisters the lirc device for a given &lirc_driver.
>> + */
>> +void
>> +lirc_unregister_driver(struct lirc_driver *d)
>> {
>> struct irctl *ir;
>>
>> - if (minor < 0 || minor >= MAX_IRCTL_DEVICES) {
>> - pr_err("minor (%d) must be between 0 and %d!\n",
>> - minor, MAX_IRCTL_DEVICES - 1);
>> - return -EBADRQC;
>> - }
>> + if (!d->lirc_internal)
>> + return;
>>
>> - ir = irctls[minor];
>> - if (!ir) {
>> - pr_err("failed to get irctl\n");
>> - return -ENOENT;
>> - }
>> + ir = d->lirc_internal;
>
>This is done to find a our copy again.
>
>> - mutex_lock(&lirc_dev_lock);
>> -
>> - if (ir->d.minor != minor) {
>> - dev_err(ir->d.dev, "lirc_dev: minor %d device not registered\n",
>> - minor);
>> - mutex_unlock(&lirc_dev_lock);
>> - return -ENOENT;
>> - }
>> + mutex_lock(&ir->mutex);
>>
>> dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
>> ir->d.name, ir->d.minor);
>>
>> - ir->attached = 0;
>> - if (ir->open) {
>> + ir->dead = true;
>> + if (ir->users) {
>> dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
>> ir->d.name, ir->d.minor);
>> wake_up_interruptible(&ir->buf->wait_poll);
>> }
>>
>> - mutex_unlock(&lirc_dev_lock);
>> + mutex_unlock(&ir->mutex);
>>
>> device_del(&ir->dev);
>> cdev_del(&ir->cdev);
>> - put_device(&ir->dev);
>> -
>> - return 0;
>> + ida_simple_remove(&lirc_ida, d->minor);
>> + d->minor = -1;
>> + d->lirc_internal = NULL;
>> }
>> EXPORT_SYMBOL(lirc_unregister_driver);
>>
>> int lirc_dev_fop_open(struct inode *inode, struct file *file)
>> {
>> - struct irctl *ir;
>> + struct irctl *ir = container_of(inode->i_cdev, struct irctl, cdev);
>> int retval;
>>
>> - if (iminor(inode) >= MAX_IRCTL_DEVICES) {
>> - pr_err("open result for %d is -ENODEV\n", iminor(inode));
>> - return -ENODEV;
>> - }
>> -
>> - if (mutex_lock_interruptible(&lirc_dev_lock))
>> - return -ERESTARTSYS;
>> + mutex_lock(&ir->mutex);
>>
>> - ir = irctls[iminor(inode)];
>> - mutex_unlock(&lirc_dev_lock);
>> -
>> - if (!ir) {
>> - retval = -ENODEV;
>> + if (ir->users) {
>> + retval = -EBUSY;
>> + mutex_unlock(&ir->mutex);
>> goto error;
>> }
>> + ir->users++;
>>
>> - dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
>> -
>> - if (ir->d.minor == NOPLUG) {
>> - retval = -ENODEV;
>> - goto error;
>> - }
>> + mutex_unlock(&ir->mutex);
>>
>> - if (ir->open) {
>> - retval = -EBUSY;
>> - goto error;
>> - }
>> + dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
>>
>> if (ir->d.rdev) {
>> retval = rc_open(ir->d.rdev);
>> @@ -329,8 +299,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
>> if (ir->buf)
>> lirc_buffer_clear(ir->buf);
>>
>> - ir->open++;
>> -
>> + file->private_data = ir;
>> nonseekable_open(inode, file);
>>
>> return 0;
>> @@ -342,22 +311,14 @@ EXPORT_SYMBOL(lirc_dev_fop_open);
>>
>> int lirc_dev_fop_close(struct inode *inode, struct file *file)
>> {
>> - struct irctl *ir = irctls[iminor(inode)];
>> - int ret;
>> -
>> - if (!ir) {
>> - pr_err("called with invalid irctl\n");
>> - return -EINVAL;
>> - }
>> + struct irctl *ir = file->private_data;
>>
>> - ret = mutex_lock_killable(&lirc_dev_lock);
>> - WARN_ON(ret);
>> + mutex_lock(&ir->mutex);
>>
>> rc_close(ir->d.rdev);
>> + ir->users--;
>>
>> - ir->open--;
>> - if (!ret)
>> - mutex_unlock(&lirc_dev_lock);
>> + mutex_unlock(&ir->mutex);
>>
>> return 0;
>> }
>> @@ -365,26 +326,21 @@ EXPORT_SYMBOL(lirc_dev_fop_close);
>>
>> unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
>> {
>> - struct irctl *ir = irctls[iminor(file_inode(file))];
>> + struct irctl *ir = file->private_data;
>> unsigned int ret;
>>
>> - if (!ir) {
>> - pr_err("called with invalid irctl\n");
>> - return POLLERR;
>> - }
>> -
>> - if (!ir->attached)
>> + if (ir->dead)
>> return POLLHUP | POLLERR;
>>
>> - if (ir->buf) {
>> - poll_wait(file, &ir->buf->wait_poll, wait);
>> + if (!ir->buf)
>> + return POLLERR;
>> +
>> + poll_wait(file, &ir->buf->wait_poll, wait);
>>
>> - if (lirc_buffer_empty(ir->buf))
>> - ret = 0;
>> - else
>> - ret = POLLIN | POLLRDNORM;
>> - } else
>> - ret = POLLERR;
>> + if (lirc_buffer_empty(ir->buf))
>> + ret = 0;
>> + else
>> + ret = POLLIN | POLLRDNORM;
>>
>> dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n",
>> ir->d.name, ir->d.minor, ret);
>> @@ -395,25 +351,20 @@ EXPORT_SYMBOL(lirc_dev_fop_poll);
>>
>> long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> {
>> + struct irctl *ir = file->private_data;
>> __u32 mode;
>> int result = 0;
>> - struct irctl *ir = irctls[iminor(file_inode(file))];
>> -
>> - if (!ir) {
>> - pr_err("no irctl found!\n");
>> - return -ENODEV;
>> - }
>>
>> dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
>> ir->d.name, ir->d.minor, cmd);
>>
>> - if (ir->d.minor == NOPLUG || !ir->attached) {
>> + if (ir->dead) {
>> dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
>> ir->d.name, ir->d.minor);
>> return -ENODEV;
>> }
>>
>> - mutex_lock(&ir->irctl_lock);
>> + mutex_lock(&ir->mutex);
>>
>> switch (cmd) {
>> case LIRC_GET_FEATURES:
>> @@ -468,7 +419,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> result = -ENOTTY;
>> }
>>
>> - mutex_unlock(&ir->irctl_lock);
>> + mutex_unlock(&ir->mutex);
>>
>> return result;
>> }
>> @@ -479,16 +430,11 @@ ssize_t lirc_dev_fop_read(struct file *file,
>> size_t length,
>> loff_t *ppos)
>> {
>> - struct irctl *ir = irctls[iminor(file_inode(file))];
>> + struct irctl *ir = file->private_data;
>> unsigned char *buf;
>> int ret = 0, written = 0;
>> DECLARE_WAITQUEUE(wait, current);
>>
>> - if (!ir) {
>> - pr_err("called with invalid irctl\n");
>> - return -ENODEV;
>> - }
>> -
>> if (!LIRC_CAN_REC(ir->d.features))
>> return -EINVAL;
>>
>> @@ -498,11 +444,12 @@ ssize_t lirc_dev_fop_read(struct file *file,
>> if (!buf)
>> return -ENOMEM;
>>
>> - if (mutex_lock_interruptible(&ir->irctl_lock)) {
>> + if (mutex_lock_interruptible(&ir->mutex)) {
>> ret = -ERESTARTSYS;
>> goto out_unlocked;
>> }
>> - if (!ir->attached) {
>> +
>> + if (ir->dead) {
>> ret = -ENODEV;
>> goto out_locked;
>> }
>> @@ -541,18 +488,18 @@ ssize_t lirc_dev_fop_read(struct file *file,
>> break;
>> }
>>
>> - mutex_unlock(&ir->irctl_lock);
>> + mutex_unlock(&ir->mutex);
>> set_current_state(TASK_INTERRUPTIBLE);
>> schedule();
>> set_current_state(TASK_RUNNING);
>>
>> - if (mutex_lock_interruptible(&ir->irctl_lock)) {
>> + if (mutex_lock_interruptible(&ir->mutex)) {
>> ret = -ERESTARTSYS;
>> remove_wait_queue(&ir->buf->wait_poll, &wait);
>> goto out_unlocked;
>> }
>>
>> - if (!ir->attached) {
>> + if (ir->dead) {
>> ret = -ENODEV;
>> goto out_locked;
>> }
>> @@ -570,7 +517,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
>> remove_wait_queue(&ir->buf->wait_poll, &wait);
>>
>> out_locked:
>> - mutex_unlock(&ir->irctl_lock);
>> + mutex_unlock(&ir->mutex);
>>
>> out_unlocked:
>> kfree(buf);
>> @@ -581,7 +528,8 @@ EXPORT_SYMBOL(lirc_dev_fop_read);
>>
>> void *lirc_get_pdata(struct file *file)
>> {
>> - return irctls[iminor(file_inode(file))]->d.data;
>> + struct irctl *ir = file->private_data;
>> + return ir->d.data;
>> }
>> EXPORT_SYMBOL(lirc_get_pdata);
>>
>> @@ -596,7 +544,7 @@ static int __init lirc_dev_init(void)
>> return PTR_ERR(lirc_class);
>> }
>>
>> - retval = alloc_chrdev_region(&lirc_base_dev, 0, MAX_IRCTL_DEVICES,
>> + retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
>> IRCTL_DEV_NAME);
>> if (retval) {
>> class_destroy(lirc_class);
>> @@ -613,7 +561,7 @@ static int __init lirc_dev_init(void)
>> static void __exit lirc_dev_exit(void)
>> {
>> class_destroy(lirc_class);
>> - unregister_chrdev_region(lirc_base_dev, MAX_IRCTL_DEVICES);
>> + unregister_chrdev_region(lirc_base_dev, LIRC_MAX_DEVICES);
>> pr_info("module unloaded\n");
>> }
>>
>> diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
>> index 59e05aa1bc55..ffb70dee4547 100644
>> --- a/drivers/staging/media/lirc/lirc_zilog.c
>> +++ b/drivers/staging/media/lirc/lirc_zilog.c
>> @@ -183,11 +183,7 @@ static void release_ir_device(struct kref *ref)
>> * ir->open_count == 0 - happens on final close()
>> * ir_lock, tx_ref_lock, rx_ref_lock, all released
>> */
>> - if (ir->l.minor >= 0) {
>> - lirc_unregister_driver(ir->l.minor);
>> - ir->l.minor = -1;
>> - }
>> -
>> + lirc_unregister_driver(&ir->l);
>> if (kfifo_initialized(&ir->rbuf.fifo))
>> lirc_buffer_free(&ir->rbuf);
>> list_del(&ir->list);
>> @@ -1382,7 +1378,6 @@ static const struct file_operations lirc_fops = {
>>
>> static struct lirc_driver lirc_template = {
>> .name = "lirc_zilog",
>> - .minor = -1,
>> .code_length = 13,
>> .buffer_size = BUFLEN / 2,
>> .chunk_size = 2,
>> @@ -1597,14 +1592,13 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
>> }
>>
>> /* register with lirc */
>> - ir->l.minor = lirc_register_driver(&ir->l);
>> - if (ir->l.minor < 0) {
>> + ret = lirc_register_driver(&ir->l);
>> + if (ret < 0) {
>> dev_err(tx->ir->l.dev,
>> - "%s: lirc_register_driver() failed: %i\n",
>> - __func__, ir->l.minor);
>> - ret = -EBADRQC;
>> + "%s: lirc_register_driver failed: %i\n", __func__, ret);
>> goto out_put_xx;
>> }
>> +
>> dev_info(ir->l.dev,
>> "IR unit on %s (i2c-%d) registered as lirc%d and ready\n",
>> adap->name, adap->nr, ir->l.minor);
>> diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
>> index 1f327e25a9be..f7629ff116a9 100644
>> --- a/include/media/lirc_dev.h
>> +++ b/include/media/lirc_dev.h
>> @@ -9,7 +9,6 @@
>> #ifndef _LINUX_LIRC_DEV_H
>> #define _LINUX_LIRC_DEV_H
>>
>> -#define MAX_IRCTL_DEVICES 8
>> #define BUFLEN 16
>>
>> #define mod(n, div) ((n) % (div))
>> @@ -164,6 +163,8 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
>> * device.
>> *
>> * @owner: the module owning this struct
>> + *
>> + * @lirc_internal: lirc_dev bookkeeping data, don't touch.
>
>It's not a great name or comment :)
>
>Otherwise, it's a good idea.
>
>
>Sean
>
>> */
>> struct lirc_driver {
>> char name[40];
>> @@ -182,19 +183,12 @@ struct lirc_driver {
>> const struct file_operations *fops;
>> struct device *dev;
>> struct module *owner;
>> + void *lirc_internal;
>> };
>>
>> -/* following functions can be called ONLY from user context
>> - *
>> - * returns negative value on error or minor number
>> - * of the registered device if success
>> - * contents of the structure pointed by p is copied
>> - */
>> extern int lirc_register_driver(struct lirc_driver *d);
>>
>> -/* returns negative value on error or 0 if success
>> -*/
>> -extern int lirc_unregister_driver(int minor);
>> +extern void lirc_unregister_driver(struct lirc_driver *d);
>>
>> /* Returns the private data stored in the lirc_driver
>> * associated with the given device file pointer.
--
David Härdeman
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors
2017-05-28 8:26 ` David Härdeman
@ 2017-05-28 15:08 ` Sean Young
0 siblings, 0 replies; 28+ messages in thread
From: Sean Young @ 2017-05-28 15:08 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, mchehab
On Sun, May 28, 2017 at 10:26:59AM +0200, David Härdeman wrote:
> On Mon, May 22, 2017 at 09:09:42PM +0100, Sean Young wrote:
> >On Mon, May 01, 2017 at 06:04:42PM +0200, David Härdeman wrote:
> >> Using the kernel ida facilities, we can avoid a lot of unnecessary code and at the same
> >> time get rid of lirc_dev_lock in favour of per-device locks (the irctls array was used
> >> throughout lirc_dev so this patch necessarily touches a lot of code).
> >>
> >> Signed-off-by: David Härdeman <david@hardeman.nu>
> >> ---
> >> drivers/media/rc/ir-lirc-codec.c | 9 -
> >> drivers/media/rc/lirc_dev.c | 258 ++++++++++++-------------------
> >> drivers/staging/media/lirc/lirc_zilog.c | 16 +-
> >> include/media/lirc_dev.h | 14 --
> >> 4 files changed, 115 insertions(+), 182 deletions(-)
> >>
> >> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> >> index a30af91710fe..2c1221a61ea1 100644
> >> --- a/drivers/media/rc/ir-lirc-codec.c
> >> +++ b/drivers/media/rc/ir-lirc-codec.c
> >> @@ -382,7 +382,6 @@ static int ir_lirc_register(struct rc_dev *dev)
> >>
> >> snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
> >> dev->driver_name);
> >> - drv->minor = -1;
> >> drv->features = features;
> >> drv->data = &dev->raw->lirc;
> >> drv->rbuf = NULL;
> >> @@ -394,11 +393,9 @@ static int ir_lirc_register(struct rc_dev *dev)
> >> drv->rdev = dev;
> >> drv->owner = THIS_MODULE;
> >>
> >> - drv->minor = lirc_register_driver(drv);
> >> - if (drv->minor < 0) {
> >> - rc = -ENODEV;
> >> + rc = lirc_register_driver(drv);
> >> + if (rc < 0)
> >> goto out;
> >> - }
> >>
> >> dev->raw->lirc.drv = drv;
> >> dev->raw->lirc.dev = dev;
> >> @@ -413,7 +410,7 @@ static int ir_lirc_unregister(struct rc_dev *dev)
> >> {
> >> struct lirc_codec *lirc = &dev->raw->lirc;
> >>
> >> - lirc_unregister_driver(lirc->drv->minor);
> >> + lirc_unregister_driver(lirc->drv);
> >> kfree(lirc->drv);
> >> lirc->drv = NULL;
> >>
> >> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> >> index e44e0b23b883..7db7d4c57991 100644
> >> --- a/drivers/media/rc/lirc_dev.c
> >> +++ b/drivers/media/rc/lirc_dev.c
> >> @@ -31,23 +31,35 @@
> >> #include <linux/bitops.h>
> >> #include <linux/device.h>
> >> #include <linux/cdev.h>
> >> +#include <linux/idr.h>
> >>
> >> #include <media/rc-core.h>
> >> #include <media/lirc.h>
> >> #include <media/lirc_dev.h>
> >>
> >> #define IRCTL_DEV_NAME "BaseRemoteCtl"
> >> -#define NOPLUG -1
> >> #define LOGHEAD "lirc_dev (%s[%d]): "
> >>
> >> static dev_t lirc_base_dev;
> >>
> >> +/**
> >> + * struct irctl - lirc per-device structure
> >> + * @d: internal copy of the &struct lirc_driver for the device
> >> + * @dead: if the device has gone away
> >> + * @users: the number of users of the lirc chardev (currently limited to 1)
> >> + * @mutex: synchronizes open()/close()/ioctl()/etc calls
> >> + * @buf: used to store lirc RX data
> >> + * @buf_internal: whether @buf was allocated internally or not
> >> + * @chunk_size: @buf stores and read() returns data chunks of this size
> >> + * @dev: the &struct device for the lirc device
> >> + * @cdev: the &struct device for the lirc chardev
> >> + */
> >> struct irctl {
> >> struct lirc_driver d;
> >> - int attached;
> >> - int open;
> >> + bool dead;
> >> + unsigned users;
> >>
> >> - struct mutex irctl_lock;
> >> + struct mutex mutex;
> >> struct lirc_buffer *buf;
> >> bool buf_internal;
> >> unsigned int chunk_size;
> >> @@ -56,9 +68,9 @@ struct irctl {
> >> struct cdev cdev;
> >> };
> >>
> >> -static DEFINE_MUTEX(lirc_dev_lock);
> >> -
> >> -static struct irctl *irctls[MAX_IRCTL_DEVICES];
> >> +/* Used to keep track of allocated lirc devices */
> >> +#define LIRC_MAX_DEVICES 256
> >> +static DEFINE_IDA(lirc_ida);
> >>
> >> /* Only used for sysfs but defined to void otherwise */
> >> static struct class *lirc_class;
> >> @@ -72,9 +84,6 @@ static void lirc_release(struct device *ld)
> >> kfree(ir->buf);
> >> }
> >>
> >> - mutex_lock(&lirc_dev_lock);
> >> - irctls[ir->d.minor] = NULL;
> >> - mutex_unlock(&lirc_dev_lock);
> >> kfree(ir);
> >> }
> >>
> >> @@ -117,7 +126,18 @@ static int lirc_allocate_buffer(struct irctl *ir)
> >> return err;
> >> }
> >>
> >> -int lirc_register_driver(struct lirc_driver *d)
> >> +/**
> >> + * lirc_register_driver() - create a new lirc device by registering a driver
> >> + * @d: the &struct lirc_driver to register
> >> + *
> >> + * This function allocates and registers a lirc device for a given
> >> + * &lirc_driver. The &lirc_driver structure is updated to contain
> >> + * information about the allocated device (minor, buffer, etc).
> >> + *
> >> + * Return: zero on success or a negative error value.
> >> + */
> >> +int
> >> +lirc_register_driver(struct lirc_driver *d)
> >> {
> >> struct irctl *ir;
> >> int minor;
> >> @@ -138,12 +158,6 @@ int lirc_register_driver(struct lirc_driver *d)
> >> return -EINVAL;
> >> }
> >>
> >> - if (d->minor >= MAX_IRCTL_DEVICES) {
> >> - dev_err(d->dev, "minor must be between 0 and %d!\n",
> >> - MAX_IRCTL_DEVICES - 1);
> >> - return -EBADRQC;
> >> - }
> >> -
> >> if (d->code_length < 1 || d->code_length > (BUFLEN * 8)) {
> >> dev_err(d->dev, "code length must be less than %d bits\n",
> >> BUFLEN * 8);
> >> @@ -156,49 +170,30 @@ int lirc_register_driver(struct lirc_driver *d)
> >> return -EBADRQC;
> >> }
> >>
> >> - mutex_lock(&lirc_dev_lock);
> >> -
> >> - minor = d->minor;
> >> + d->name[sizeof(d->name)-1] = '\0';
> >> + if (d->features == 0)
> >> + d->features = LIRC_CAN_REC_LIRCCODE;
> >>
> >> - if (minor < 0) {
> >> - /* find first free slot for driver */
> >> - for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++)
> >> - if (!irctls[minor])
> >> - break;
> >> - if (minor == MAX_IRCTL_DEVICES) {
> >> - dev_err(d->dev, "no free slots for drivers!\n");
> >> - err = -ENOMEM;
> >> - goto out_lock;
> >> - }
> >> - } else if (irctls[minor]) {
> >> - dev_err(d->dev, "minor (%d) just registered!\n", minor);
> >> - err = -EBUSY;
> >> - goto out_lock;
> >> - }
> >> + minor = ida_simple_get(&lirc_ida, 0, LIRC_MAX_DEVICES, GFP_KERNEL);
> >> + if (minor < 0)
> >> + return minor;
> >> + d->minor = minor;
> >>
> >> ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
> >> if (!ir) {
> >> err = -ENOMEM;
> >> - goto out_lock;
> >> + goto out_minor;
> >> }
> >>
> >> - mutex_init(&ir->irctl_lock);
> >> - irctls[minor] = ir;
> >> - d->minor = minor;
> >> -
> >> - /* some safety check 8-) */
> >> - d->name[sizeof(d->name)-1] = '\0';
> >> -
> >> - if (d->features == 0)
> >> - d->features = LIRC_CAN_REC_LIRCCODE;
> >> + mutex_init(&ir->mutex);
> >>
> >> ir->d = *d;
> >
> >Here we copy lirc_driver.
> >
> >>
> >> if (LIRC_CAN_REC(d->features)) {
> >> - err = lirc_allocate_buffer(irctls[minor]);
> >> + err = lirc_allocate_buffer(ir);
> >> if (err) {
> >> kfree(ir);
> >> - goto out_lock;
> >> + goto out_minor;
> >> }
> >> d->rbuf = ir->buf;
> >> }
> >> @@ -218,107 +213,82 @@ int lirc_register_driver(struct lirc_driver *d)
> >> if (err)
> >> goto out_free_dev;
> >>
> >> - ir->attached = 1;
> >> -
> >> err = device_add(&ir->dev);
> >> if (err)
> >> goto out_cdev;
> >>
> >> - mutex_unlock(&lirc_dev_lock);
> >> -
> >> dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
> >> ir->d.name, ir->d.minor);
> >>
> >> - return minor;
> >> + d->lirc_internal = ir;
> >
> >And now a store a pointer to the copy in the original lirc_driver.
> >
> >It would be much better to not copy lirc_driver and the lirc_internal
> >member would be unnecessary.
>
> I know, but this is a more minimal fix. The struct copy is already in
> the current code and I'd prefer removing it in a separate patch once the
> use of lirc_driver has been vetted.
Your patch adds the lirc_internal pointer to work around this, which makes
the code even worse than it is.
Sean
>
> >> + return 0;
> >>
> >> out_cdev:
> >> cdev_del(&ir->cdev);
> >> out_free_dev:
> >> put_device(&ir->dev);
> >> -out_lock:
> >> - mutex_unlock(&lirc_dev_lock);
> >> +out_minor:
> >> + ida_simple_remove(&lirc_ida, minor);
> >>
> >> return err;
> >> }
> >> EXPORT_SYMBOL(lirc_register_driver);
> >>
> >> -int lirc_unregister_driver(int minor)
> >> +/**
> >> + * lirc_unregister_driver() - remove the lirc device for a given driver
> >> + * @d: the &struct lirc_driver to unregister
> >> + *
> >> + * This function unregisters the lirc device for a given &lirc_driver.
> >> + */
> >> +void
> >> +lirc_unregister_driver(struct lirc_driver *d)
> >> {
> >> struct irctl *ir;
> >>
> >> - if (minor < 0 || minor >= MAX_IRCTL_DEVICES) {
> >> - pr_err("minor (%d) must be between 0 and %d!\n",
> >> - minor, MAX_IRCTL_DEVICES - 1);
> >> - return -EBADRQC;
> >> - }
> >> + if (!d->lirc_internal)
> >> + return;
> >>
> >> - ir = irctls[minor];
> >> - if (!ir) {
> >> - pr_err("failed to get irctl\n");
> >> - return -ENOENT;
> >> - }
> >> + ir = d->lirc_internal;
> >
> >This is done to find a our copy again.
> >
> >> - mutex_lock(&lirc_dev_lock);
> >> -
> >> - if (ir->d.minor != minor) {
> >> - dev_err(ir->d.dev, "lirc_dev: minor %d device not registered\n",
> >> - minor);
> >> - mutex_unlock(&lirc_dev_lock);
> >> - return -ENOENT;
> >> - }
> >> + mutex_lock(&ir->mutex);
> >>
> >> dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
> >> ir->d.name, ir->d.minor);
> >>
> >> - ir->attached = 0;
> >> - if (ir->open) {
> >> + ir->dead = true;
> >> + if (ir->users) {
> >> dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
> >> ir->d.name, ir->d.minor);
> >> wake_up_interruptible(&ir->buf->wait_poll);
> >> }
> >>
> >> - mutex_unlock(&lirc_dev_lock);
> >> + mutex_unlock(&ir->mutex);
> >>
> >> device_del(&ir->dev);
> >> cdev_del(&ir->cdev);
> >> - put_device(&ir->dev);
> >> -
> >> - return 0;
> >> + ida_simple_remove(&lirc_ida, d->minor);
> >> + d->minor = -1;
> >> + d->lirc_internal = NULL;
> >> }
> >> EXPORT_SYMBOL(lirc_unregister_driver);
> >>
> >> int lirc_dev_fop_open(struct inode *inode, struct file *file)
> >> {
> >> - struct irctl *ir;
> >> + struct irctl *ir = container_of(inode->i_cdev, struct irctl, cdev);
> >> int retval;
> >>
> >> - if (iminor(inode) >= MAX_IRCTL_DEVICES) {
> >> - pr_err("open result for %d is -ENODEV\n", iminor(inode));
> >> - return -ENODEV;
> >> - }
> >> -
> >> - if (mutex_lock_interruptible(&lirc_dev_lock))
> >> - return -ERESTARTSYS;
> >> + mutex_lock(&ir->mutex);
> >>
> >> - ir = irctls[iminor(inode)];
> >> - mutex_unlock(&lirc_dev_lock);
> >> -
> >> - if (!ir) {
> >> - retval = -ENODEV;
> >> + if (ir->users) {
> >> + retval = -EBUSY;
> >> + mutex_unlock(&ir->mutex);
> >> goto error;
> >> }
> >> + ir->users++;
> >>
> >> - dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
> >> -
> >> - if (ir->d.minor == NOPLUG) {
> >> - retval = -ENODEV;
> >> - goto error;
> >> - }
> >> + mutex_unlock(&ir->mutex);
> >>
> >> - if (ir->open) {
> >> - retval = -EBUSY;
> >> - goto error;
> >> - }
> >> + dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
> >>
> >> if (ir->d.rdev) {
> >> retval = rc_open(ir->d.rdev);
> >> @@ -329,8 +299,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
> >> if (ir->buf)
> >> lirc_buffer_clear(ir->buf);
> >>
> >> - ir->open++;
> >> -
> >> + file->private_data = ir;
> >> nonseekable_open(inode, file);
> >>
> >> return 0;
> >> @@ -342,22 +311,14 @@ EXPORT_SYMBOL(lirc_dev_fop_open);
> >>
> >> int lirc_dev_fop_close(struct inode *inode, struct file *file)
> >> {
> >> - struct irctl *ir = irctls[iminor(inode)];
> >> - int ret;
> >> -
> >> - if (!ir) {
> >> - pr_err("called with invalid irctl\n");
> >> - return -EINVAL;
> >> - }
> >> + struct irctl *ir = file->private_data;
> >>
> >> - ret = mutex_lock_killable(&lirc_dev_lock);
> >> - WARN_ON(ret);
> >> + mutex_lock(&ir->mutex);
> >>
> >> rc_close(ir->d.rdev);
> >> + ir->users--;
> >>
> >> - ir->open--;
> >> - if (!ret)
> >> - mutex_unlock(&lirc_dev_lock);
> >> + mutex_unlock(&ir->mutex);
> >>
> >> return 0;
> >> }
> >> @@ -365,26 +326,21 @@ EXPORT_SYMBOL(lirc_dev_fop_close);
> >>
> >> unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
> >> {
> >> - struct irctl *ir = irctls[iminor(file_inode(file))];
> >> + struct irctl *ir = file->private_data;
> >> unsigned int ret;
> >>
> >> - if (!ir) {
> >> - pr_err("called with invalid irctl\n");
> >> - return POLLERR;
> >> - }
> >> -
> >> - if (!ir->attached)
> >> + if (ir->dead)
> >> return POLLHUP | POLLERR;
> >>
> >> - if (ir->buf) {
> >> - poll_wait(file, &ir->buf->wait_poll, wait);
> >> + if (!ir->buf)
> >> + return POLLERR;
> >> +
> >> + poll_wait(file, &ir->buf->wait_poll, wait);
> >>
> >> - if (lirc_buffer_empty(ir->buf))
> >> - ret = 0;
> >> - else
> >> - ret = POLLIN | POLLRDNORM;
> >> - } else
> >> - ret = POLLERR;
> >> + if (lirc_buffer_empty(ir->buf))
> >> + ret = 0;
> >> + else
> >> + ret = POLLIN | POLLRDNORM;
> >>
> >> dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n",
> >> ir->d.name, ir->d.minor, ret);
> >> @@ -395,25 +351,20 @@ EXPORT_SYMBOL(lirc_dev_fop_poll);
> >>
> >> long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >> {
> >> + struct irctl *ir = file->private_data;
> >> __u32 mode;
> >> int result = 0;
> >> - struct irctl *ir = irctls[iminor(file_inode(file))];
> >> -
> >> - if (!ir) {
> >> - pr_err("no irctl found!\n");
> >> - return -ENODEV;
> >> - }
> >>
> >> dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
> >> ir->d.name, ir->d.minor, cmd);
> >>
> >> - if (ir->d.minor == NOPLUG || !ir->attached) {
> >> + if (ir->dead) {
> >> dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
> >> ir->d.name, ir->d.minor);
> >> return -ENODEV;
> >> }
> >>
> >> - mutex_lock(&ir->irctl_lock);
> >> + mutex_lock(&ir->mutex);
> >>
> >> switch (cmd) {
> >> case LIRC_GET_FEATURES:
> >> @@ -468,7 +419,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >> result = -ENOTTY;
> >> }
> >>
> >> - mutex_unlock(&ir->irctl_lock);
> >> + mutex_unlock(&ir->mutex);
> >>
> >> return result;
> >> }
> >> @@ -479,16 +430,11 @@ ssize_t lirc_dev_fop_read(struct file *file,
> >> size_t length,
> >> loff_t *ppos)
> >> {
> >> - struct irctl *ir = irctls[iminor(file_inode(file))];
> >> + struct irctl *ir = file->private_data;
> >> unsigned char *buf;
> >> int ret = 0, written = 0;
> >> DECLARE_WAITQUEUE(wait, current);
> >>
> >> - if (!ir) {
> >> - pr_err("called with invalid irctl\n");
> >> - return -ENODEV;
> >> - }
> >> -
> >> if (!LIRC_CAN_REC(ir->d.features))
> >> return -EINVAL;
> >>
> >> @@ -498,11 +444,12 @@ ssize_t lirc_dev_fop_read(struct file *file,
> >> if (!buf)
> >> return -ENOMEM;
> >>
> >> - if (mutex_lock_interruptible(&ir->irctl_lock)) {
> >> + if (mutex_lock_interruptible(&ir->mutex)) {
> >> ret = -ERESTARTSYS;
> >> goto out_unlocked;
> >> }
> >> - if (!ir->attached) {
> >> +
> >> + if (ir->dead) {
> >> ret = -ENODEV;
> >> goto out_locked;
> >> }
> >> @@ -541,18 +488,18 @@ ssize_t lirc_dev_fop_read(struct file *file,
> >> break;
> >> }
> >>
> >> - mutex_unlock(&ir->irctl_lock);
> >> + mutex_unlock(&ir->mutex);
> >> set_current_state(TASK_INTERRUPTIBLE);
> >> schedule();
> >> set_current_state(TASK_RUNNING);
> >>
> >> - if (mutex_lock_interruptible(&ir->irctl_lock)) {
> >> + if (mutex_lock_interruptible(&ir->mutex)) {
> >> ret = -ERESTARTSYS;
> >> remove_wait_queue(&ir->buf->wait_poll, &wait);
> >> goto out_unlocked;
> >> }
> >>
> >> - if (!ir->attached) {
> >> + if (ir->dead) {
> >> ret = -ENODEV;
> >> goto out_locked;
> >> }
> >> @@ -570,7 +517,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
> >> remove_wait_queue(&ir->buf->wait_poll, &wait);
> >>
> >> out_locked:
> >> - mutex_unlock(&ir->irctl_lock);
> >> + mutex_unlock(&ir->mutex);
> >>
> >> out_unlocked:
> >> kfree(buf);
> >> @@ -581,7 +528,8 @@ EXPORT_SYMBOL(lirc_dev_fop_read);
> >>
> >> void *lirc_get_pdata(struct file *file)
> >> {
> >> - return irctls[iminor(file_inode(file))]->d.data;
> >> + struct irctl *ir = file->private_data;
> >> + return ir->d.data;
> >> }
> >> EXPORT_SYMBOL(lirc_get_pdata);
> >>
> >> @@ -596,7 +544,7 @@ static int __init lirc_dev_init(void)
> >> return PTR_ERR(lirc_class);
> >> }
> >>
> >> - retval = alloc_chrdev_region(&lirc_base_dev, 0, MAX_IRCTL_DEVICES,
> >> + retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
> >> IRCTL_DEV_NAME);
> >> if (retval) {
> >> class_destroy(lirc_class);
> >> @@ -613,7 +561,7 @@ static int __init lirc_dev_init(void)
> >> static void __exit lirc_dev_exit(void)
> >> {
> >> class_destroy(lirc_class);
> >> - unregister_chrdev_region(lirc_base_dev, MAX_IRCTL_DEVICES);
> >> + unregister_chrdev_region(lirc_base_dev, LIRC_MAX_DEVICES);
> >> pr_info("module unloaded\n");
> >> }
> >>
> >> diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
> >> index 59e05aa1bc55..ffb70dee4547 100644
> >> --- a/drivers/staging/media/lirc/lirc_zilog.c
> >> +++ b/drivers/staging/media/lirc/lirc_zilog.c
> >> @@ -183,11 +183,7 @@ static void release_ir_device(struct kref *ref)
> >> * ir->open_count == 0 - happens on final close()
> >> * ir_lock, tx_ref_lock, rx_ref_lock, all released
> >> */
> >> - if (ir->l.minor >= 0) {
> >> - lirc_unregister_driver(ir->l.minor);
> >> - ir->l.minor = -1;
> >> - }
> >> -
> >> + lirc_unregister_driver(&ir->l);
> >> if (kfifo_initialized(&ir->rbuf.fifo))
> >> lirc_buffer_free(&ir->rbuf);
> >> list_del(&ir->list);
> >> @@ -1382,7 +1378,6 @@ static const struct file_operations lirc_fops = {
> >>
> >> static struct lirc_driver lirc_template = {
> >> .name = "lirc_zilog",
> >> - .minor = -1,
> >> .code_length = 13,
> >> .buffer_size = BUFLEN / 2,
> >> .chunk_size = 2,
> >> @@ -1597,14 +1592,13 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >> }
> >>
> >> /* register with lirc */
> >> - ir->l.minor = lirc_register_driver(&ir->l);
> >> - if (ir->l.minor < 0) {
> >> + ret = lirc_register_driver(&ir->l);
> >> + if (ret < 0) {
> >> dev_err(tx->ir->l.dev,
> >> - "%s: lirc_register_driver() failed: %i\n",
> >> - __func__, ir->l.minor);
> >> - ret = -EBADRQC;
> >> + "%s: lirc_register_driver failed: %i\n", __func__, ret);
> >> goto out_put_xx;
> >> }
> >> +
> >> dev_info(ir->l.dev,
> >> "IR unit on %s (i2c-%d) registered as lirc%d and ready\n",
> >> adap->name, adap->nr, ir->l.minor);
> >> diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
> >> index 1f327e25a9be..f7629ff116a9 100644
> >> --- a/include/media/lirc_dev.h
> >> +++ b/include/media/lirc_dev.h
> >> @@ -9,7 +9,6 @@
> >> #ifndef _LINUX_LIRC_DEV_H
> >> #define _LINUX_LIRC_DEV_H
> >>
> >> -#define MAX_IRCTL_DEVICES 8
> >> #define BUFLEN 16
> >>
> >> #define mod(n, div) ((n) % (div))
> >> @@ -164,6 +163,8 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
> >> * device.
> >> *
> >> * @owner: the module owning this struct
> >> + *
> >> + * @lirc_internal: lirc_dev bookkeeping data, don't touch.
> >
> >It's not a great name or comment :)
> >
> >Otherwise, it's a good idea.
> >
> >
> >Sean
> >
> >> */
> >> struct lirc_driver {
> >> char name[40];
> >> @@ -182,19 +183,12 @@ struct lirc_driver {
> >> const struct file_operations *fops;
> >> struct device *dev;
> >> struct module *owner;
> >> + void *lirc_internal;
> >> };
> >>
> >> -/* following functions can be called ONLY from user context
> >> - *
> >> - * returns negative value on error or minor number
> >> - * of the registered device if success
> >> - * contents of the structure pointed by p is copied
> >> - */
> >> extern int lirc_register_driver(struct lirc_driver *d);
> >>
> >> -/* returns negative value on error or 0 if success
> >> -*/
> >> -extern int lirc_unregister_driver(int minor);
> >> +extern void lirc_unregister_driver(struct lirc_driver *d);
> >>
> >> /* Returns the private data stored in the lirc_driver
> >> * associated with the given device file pointer.
>
> --
> David Härdeman
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 14/16] lirc_dev: cleanup includes
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (12 preceding siblings ...)
2017-05-01 16:04 ` [PATCH 13/16] lirc_dev: use an ida instead of a hand-rolled array to keep track of minors David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
2017-05-19 18:21 ` Sean Young
2017-05-01 16:04 ` [PATCH 15/16] lirc_dev: remove name from struct lirc_driver David Härdeman
2017-05-01 16:04 ` [PATCH 16/16] lirc_dev: cleanup header David Härdeman
15 siblings, 1 reply; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Remove superfluous includes and defines.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 7db7d4c57991..4ba6c7e2d41b 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -15,20 +15,11 @@
*
*/
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
#include <linux/module.h>
-#include <linux/kernel.h>
#include <linux/sched/signal.h>
-#include <linux/errno.h>
#include <linux/ioctl.h>
-#include <linux/fs.h>
#include <linux/poll.h>
-#include <linux/completion.h>
#include <linux/mutex.h>
-#include <linux/wait.h>
-#include <linux/unistd.h>
-#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/cdev.h>
#include <linux/idr.h>
@@ -37,7 +28,6 @@
#include <media/lirc.h>
#include <media/lirc_dev.h>
-#define IRCTL_DEV_NAME "BaseRemoteCtl"
#define LOGHEAD "lirc_dev (%s[%d]): "
static dev_t lirc_base_dev;
@@ -545,7 +535,7 @@ static int __init lirc_dev_init(void)
}
retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
- IRCTL_DEV_NAME);
+ "BaseRemoteCtl");
if (retval) {
class_destroy(lirc_class);
pr_err("alloc_chrdev_region failed\n");
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 14/16] lirc_dev: cleanup includes
2017-05-01 16:04 ` [PATCH 14/16] lirc_dev: cleanup includes David Härdeman
@ 2017-05-19 18:21 ` Sean Young
2017-05-21 6:51 ` David Härdeman
0 siblings, 1 reply; 28+ messages in thread
From: Sean Young @ 2017-05-19 18:21 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, mchehab
On Mon, May 01, 2017 at 06:04:47PM +0200, David Härdeman wrote:
> Remove superfluous includes and defines.
>
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
> drivers/media/rc/lirc_dev.c | 12 +-----------
> 1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 7db7d4c57991..4ba6c7e2d41b 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -15,20 +15,11 @@
> *
> */
>
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
> #include <linux/module.h>
> -#include <linux/kernel.h>
> #include <linux/sched/signal.h>
> -#include <linux/errno.h>
> #include <linux/ioctl.h>
> -#include <linux/fs.h>
> #include <linux/poll.h>
> -#include <linux/completion.h>
> #include <linux/mutex.h>
> -#include <linux/wait.h>
> -#include <linux/unistd.h>
> -#include <linux/bitops.h>
> #include <linux/device.h>
> #include <linux/cdev.h>
> #include <linux/idr.h>
> @@ -37,7 +28,6 @@
> #include <media/lirc.h>
> #include <media/lirc_dev.h>
>
> -#define IRCTL_DEV_NAME "BaseRemoteCtl"
> #define LOGHEAD "lirc_dev (%s[%d]): "
>
> static dev_t lirc_base_dev;
> @@ -545,7 +535,7 @@ static int __init lirc_dev_init(void)
> }
>
> retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
> - IRCTL_DEV_NAME);
> + "BaseRemoteCtl");
This has always surprised/annoyed me. Why is this called BaseRemoteCtl? As
far as I know, this is only used for /proc/devices, where it says:
$ grep 239 /proc/devices
239 BaseRemoteCtl
And not lirc, as you would expect.
Sean
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 14/16] lirc_dev: cleanup includes
2017-05-19 18:21 ` Sean Young
@ 2017-05-21 6:51 ` David Härdeman
0 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-21 6:51 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, mchehab
On Fri, May 19, 2017 at 07:21:23PM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:04:47PM +0200, David Härdeman wrote:
>> Remove superfluous includes and defines.
>>
>> Signed-off-by: David Härdeman <david@hardeman.nu>
>> ---
>> drivers/media/rc/lirc_dev.c | 12 +-----------
>> 1 file changed, 1 insertion(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
>> index 7db7d4c57991..4ba6c7e2d41b 100644
>> --- a/drivers/media/rc/lirc_dev.c
>> +++ b/drivers/media/rc/lirc_dev.c
>> @@ -15,20 +15,11 @@
>> *
>> */
>>
>> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> -
>> #include <linux/module.h>
>> -#include <linux/kernel.h>
>> #include <linux/sched/signal.h>
>> -#include <linux/errno.h>
>> #include <linux/ioctl.h>
>> -#include <linux/fs.h>
>> #include <linux/poll.h>
>> -#include <linux/completion.h>
>> #include <linux/mutex.h>
>> -#include <linux/wait.h>
>> -#include <linux/unistd.h>
>> -#include <linux/bitops.h>
>> #include <linux/device.h>
>> #include <linux/cdev.h>
>> #include <linux/idr.h>
>> @@ -37,7 +28,6 @@
>> #include <media/lirc.h>
>> #include <media/lirc_dev.h>
>>
>> -#define IRCTL_DEV_NAME "BaseRemoteCtl"
>> #define LOGHEAD "lirc_dev (%s[%d]): "
>>
>> static dev_t lirc_base_dev;
>> @@ -545,7 +535,7 @@ static int __init lirc_dev_init(void)
>> }
>>
>> retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
>> - IRCTL_DEV_NAME);
>> + "BaseRemoteCtl");
>
>This has always surprised/annoyed me. Why is this called BaseRemoteCtl? As
>far as I know, this is only used for /proc/devices, where it says:
>
>$ grep 239 /proc/devices
>239 BaseRemoteCtl
>
>And not lirc, as you would expect.
Yeah, I also find it a bit of an ugly wart. I didn't dare to change it
though since userspace might rely on "BaseRemoteCtl". For example:
https://build.opensuse.org/package/view_file/openSUSE:12.2/lirc/rc.lirc?expand=1)
--
David Härdeman
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 15/16] lirc_dev: remove name from struct lirc_driver
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (13 preceding siblings ...)
2017-05-01 16:04 ` [PATCH 14/16] lirc_dev: cleanup includes David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
2017-05-02 17:04 ` Sean Young
2017-05-01 16:04 ` [PATCH 16/16] lirc_dev: cleanup header David Härdeman
15 siblings, 1 reply; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
The name is only used for a few debug messages and the name of the parent
device as well as the name of the lirc device (e.g. "lirc0") are sufficient
anyway.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ir-lirc-codec.c | 2 --
drivers/media/rc/lirc_dev.c | 25 ++++++++-----------------
drivers/staging/media/lirc/lirc_zilog.c | 1 -
include/media/lirc_dev.h | 3 ---
4 files changed, 8 insertions(+), 23 deletions(-)
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 2c1221a61ea1..74ce27f92901 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -380,8 +380,6 @@ static int ir_lirc_register(struct rc_dev *dev)
if (dev->max_timeout)
features |= LIRC_CAN_SET_REC_TIMEOUT;
- snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
- dev->driver_name);
drv->features = features;
drv->data = &dev->raw->lirc;
drv->rbuf = NULL;
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 4ba6c7e2d41b..10783ef75a25 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -28,8 +28,6 @@
#include <media/lirc.h>
#include <media/lirc_dev.h>
-#define LOGHEAD "lirc_dev (%s[%d]): "
-
static dev_t lirc_base_dev;
/**
@@ -160,7 +158,6 @@ lirc_register_driver(struct lirc_driver *d)
return -EBADRQC;
}
- d->name[sizeof(d->name)-1] = '\0';
if (d->features == 0)
d->features = LIRC_CAN_REC_LIRCCODE;
@@ -207,8 +204,7 @@ lirc_register_driver(struct lirc_driver *d)
if (err)
goto out_cdev;
- dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
- ir->d.name, ir->d.minor);
+ dev_info(ir->d.dev, "lirc device registered as lirc%d\n", minor);
d->lirc_internal = ir;
return 0;
@@ -242,13 +238,11 @@ lirc_unregister_driver(struct lirc_driver *d)
mutex_lock(&ir->mutex);
- dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
- ir->d.name, ir->d.minor);
+ dev_dbg(&ir->dev, "unregistered\n");
ir->dead = true;
if (ir->users) {
- dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
- ir->d.name, ir->d.minor);
+ dev_dbg(&ir->dev, "releasing opened driver\n");
wake_up_interruptible(&ir->buf->wait_poll);
}
@@ -278,7 +272,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
mutex_unlock(&ir->mutex);
- dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
+ dev_dbg(&ir->dev, "open called\n");
if (ir->d.rdev) {
retval = rc_open(ir->d.rdev);
@@ -332,8 +326,7 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
else
ret = POLLIN | POLLRDNORM;
- dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n",
- ir->d.name, ir->d.minor, ret);
+ dev_dbg(&ir->dev, "poll result = %d\n", ret);
return ret;
}
@@ -345,12 +338,10 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
__u32 mode;
int result = 0;
- dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
- ir->d.name, ir->d.minor, cmd);
+ dev_dbg(&ir->dev, "ioctl called (0x%x)\n", cmd);
if (ir->dead) {
- dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
- ir->d.name, ir->d.minor);
+ dev_err(&ir->dev, "ioctl result = -ENODEV\n");
return -ENODEV;
}
@@ -428,7 +419,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
if (!LIRC_CAN_REC(ir->d.features))
return -EINVAL;
- dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
+ dev_dbg(&ir->dev, "read called\n");
buf = kzalloc(ir->chunk_size, GFP_KERNEL);
if (!buf)
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index ffb70dee4547..131d87a04aab 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1377,7 +1377,6 @@ static const struct file_operations lirc_fops = {
};
static struct lirc_driver lirc_template = {
- .name = "lirc_zilog",
.code_length = 13,
.buffer_size = BUFLEN / 2,
.chunk_size = 2,
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index f7629ff116a9..11f455a34090 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -120,8 +120,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
/**
* struct lirc_driver - Defines the parameters on a LIRC driver
*
- * @name: this string will be used for logs
- *
* @minor: indicates minor device (/dev/lirc) number for
* registered driver if caller fills it with negative
* value, then the first free minor number will be used
@@ -167,7 +165,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
* @lirc_internal: lirc_dev bookkeeping data, don't touch.
*/
struct lirc_driver {
- char name[40];
int minor;
__u32 code_length;
unsigned int buffer_size; /* in chunks holding one code each */
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 15/16] lirc_dev: remove name from struct lirc_driver
2017-05-01 16:04 ` [PATCH 15/16] lirc_dev: remove name from struct lirc_driver David Härdeman
@ 2017-05-02 17:04 ` Sean Young
2017-05-02 18:41 ` David Härdeman
0 siblings, 1 reply; 28+ messages in thread
From: Sean Young @ 2017-05-02 17:04 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, mchehab
On Mon, May 01, 2017 at 06:04:52PM +0200, David Härdeman wrote:
> The name is only used for a few debug messages and the name of the parent
> device as well as the name of the lirc device (e.g. "lirc0") are sufficient
> anyway.
>
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
> drivers/media/rc/ir-lirc-codec.c | 2 --
> drivers/media/rc/lirc_dev.c | 25 ++++++++-----------------
> drivers/staging/media/lirc/lirc_zilog.c | 1 -
> include/media/lirc_dev.h | 3 ---
> 4 files changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> index 2c1221a61ea1..74ce27f92901 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -380,8 +380,6 @@ static int ir_lirc_register(struct rc_dev *dev)
> if (dev->max_timeout)
> features |= LIRC_CAN_SET_REC_TIMEOUT;
>
> - snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
> - dev->driver_name);
> drv->features = features;
> drv->data = &dev->raw->lirc;
> drv->rbuf = NULL;
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 4ba6c7e2d41b..10783ef75a25 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -28,8 +28,6 @@
> #include <media/lirc.h>
> #include <media/lirc_dev.h>
>
> -#define LOGHEAD "lirc_dev (%s[%d]): "
> -
> static dev_t lirc_base_dev;
>
> /**
> @@ -160,7 +158,6 @@ lirc_register_driver(struct lirc_driver *d)
> return -EBADRQC;
> }
>
> - d->name[sizeof(d->name)-1] = '\0';
> if (d->features == 0)
> d->features = LIRC_CAN_REC_LIRCCODE;
>
> @@ -207,8 +204,7 @@ lirc_register_driver(struct lirc_driver *d)
> if (err)
> goto out_cdev;
>
> - dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
> - ir->d.name, ir->d.minor);
> + dev_info(ir->d.dev, "lirc device registered as lirc%d\n", minor);
I'm not so sure this is a good idea. First of all, the documentation says
you can use "dmesg |grep lirc_dev" to find your lirc devices and you've
just replaced lirc_dev with lirc.
https://linuxtv.org/downloads/v4l-dvb-apis/uapi/rc/lirc-dev-intro.html
It's useful having the driver name in the message. For example, I have
two rc devices connected usually:
[sean@bigcore ~]$ dmesg | grep lirc_dev
[ 5.938284] lirc_dev: IR Remote Control driver registered, major 239
[ 5.945324] rc rc0: lirc_dev: driver ir-lirc-codec (winbond-cir) registered at minor = 0
[ 5111.830118] rc rc1: lirc_dev: driver ir-lirc-codec (mceusb) registered at minor = 1
With the driver name I know which one is which.
Maybe lirc_driver.name should be a "const char*" so no strcpy is needed
(the ir-lirc-codec does not seem necessary).
Thanks
Sean
>
> d->lirc_internal = ir;
> return 0;
> @@ -242,13 +238,11 @@ lirc_unregister_driver(struct lirc_driver *d)
>
> mutex_lock(&ir->mutex);
>
> - dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
> - ir->d.name, ir->d.minor);
> + dev_dbg(&ir->dev, "unregistered\n");
>
> ir->dead = true;
> if (ir->users) {
> - dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
> - ir->d.name, ir->d.minor);
> + dev_dbg(&ir->dev, "releasing opened driver\n");
> wake_up_interruptible(&ir->buf->wait_poll);
> }
>
> @@ -278,7 +272,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
>
> mutex_unlock(&ir->mutex);
>
> - dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
> + dev_dbg(&ir->dev, "open called\n");
>
> if (ir->d.rdev) {
> retval = rc_open(ir->d.rdev);
> @@ -332,8 +326,7 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
> else
> ret = POLLIN | POLLRDNORM;
>
> - dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n",
> - ir->d.name, ir->d.minor, ret);
> + dev_dbg(&ir->dev, "poll result = %d\n", ret);
>
> return ret;
> }
> @@ -345,12 +338,10 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> __u32 mode;
> int result = 0;
>
> - dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
> - ir->d.name, ir->d.minor, cmd);
> + dev_dbg(&ir->dev, "ioctl called (0x%x)\n", cmd);
>
> if (ir->dead) {
> - dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
> - ir->d.name, ir->d.minor);
> + dev_err(&ir->dev, "ioctl result = -ENODEV\n");
> return -ENODEV;
> }
>
> @@ -428,7 +419,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
> if (!LIRC_CAN_REC(ir->d.features))
> return -EINVAL;
>
> - dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
> + dev_dbg(&ir->dev, "read called\n");
>
> buf = kzalloc(ir->chunk_size, GFP_KERNEL);
> if (!buf)
> diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
> index ffb70dee4547..131d87a04aab 100644
> --- a/drivers/staging/media/lirc/lirc_zilog.c
> +++ b/drivers/staging/media/lirc/lirc_zilog.c
> @@ -1377,7 +1377,6 @@ static const struct file_operations lirc_fops = {
> };
>
> static struct lirc_driver lirc_template = {
> - .name = "lirc_zilog",
> .code_length = 13,
> .buffer_size = BUFLEN / 2,
> .chunk_size = 2,
> diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
> index f7629ff116a9..11f455a34090 100644
> --- a/include/media/lirc_dev.h
> +++ b/include/media/lirc_dev.h
> @@ -120,8 +120,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
> /**
> * struct lirc_driver - Defines the parameters on a LIRC driver
> *
> - * @name: this string will be used for logs
> - *
> * @minor: indicates minor device (/dev/lirc) number for
> * registered driver if caller fills it with negative
> * value, then the first free minor number will be used
> @@ -167,7 +165,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
> * @lirc_internal: lirc_dev bookkeeping data, don't touch.
> */
> struct lirc_driver {
> - char name[40];
> int minor;
> __u32 code_length;
> unsigned int buffer_size; /* in chunks holding one code each */
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 15/16] lirc_dev: remove name from struct lirc_driver
2017-05-02 17:04 ` Sean Young
@ 2017-05-02 18:41 ` David Härdeman
0 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-02 18:41 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, mchehab
On Tue, May 02, 2017 at 06:04:18PM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:04:52PM +0200, David Härdeman wrote:
>> The name is only used for a few debug messages and the name of the parent
>> device as well as the name of the lirc device (e.g. "lirc0") are sufficient
>> anyway.
>>...
>> @@ -207,8 +204,7 @@ lirc_register_driver(struct lirc_driver *d)
>> if (err)
>> goto out_cdev;
>>
>> - dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
>> - ir->d.name, ir->d.minor);
>> + dev_info(ir->d.dev, "lirc device registered as lirc%d\n", minor);
>
>I'm not so sure this is a good idea. First of all, the documentation says
>you can use "dmesg |grep lirc_dev" to find your lirc devices and you've
>just replaced lirc_dev with lirc.
Sure, no strong preferences here, you could change the line to say
"lirc_dev device...", or drop the patch.
>https://linuxtv.org/downloads/v4l-dvb-apis/uapi/rc/lirc-dev-intro.html
>
>It's useful having the driver name in the message. For example, I have
>two rc devices connected usually:
>
>[sean@bigcore ~]$ dmesg | grep lirc_dev
>[ 5.938284] lirc_dev: IR Remote Control driver registered, major 239
>[ 5.945324] rc rc0: lirc_dev: driver ir-lirc-codec (winbond-cir) registered at minor = 0
>[ 5111.830118] rc rc1: lirc_dev: driver ir-lirc-codec (mceusb) registered at minor = 1
winbond-cir....good man :)
How about "dmesg | grep lirc -A2 -B2"?
I don't think the situation is that different from how you'd know which
input dev is allocated to any given rc_dev? With this patch applied the
relevant output will be:
[ 0.393494] rc rc0: rc-core loopback device as /devices/virtual/rc/rc0
[ 0.394458] input: rc-core loopback device as /devices/virtual/rc/rc0/input2
[ 0.395717] rc rc0: lirc device registered as lirc0
[ 12.612313] rc rc1: mceusb device as /devices/virtual/rc/rc1
[ 12.612768] input: mceusb device as /devices/virtual/rc/rc1/input4
[ 12.613112] rc rc1: lirc device registered as lirc1
(and we might want to change the lirc line to include the sysfs path?)
But realistically, how much dmesg grepping are we expecting normal
end-users to be doing?
Anyway, as I said, this patch isn't crucial, and we can revisit printk's
later (I'm looking at the ioctl locking right now and I think an
ir-lirc-codec and lirc_dev merger might be a good idea once the fate of
lirc_zilog has been decided).
>With the driver name I know which one is which.
>
>Maybe lirc_driver.name should be a "const char*" so no strcpy is needed
>(the ir-lirc-codec does not seem necessary).
Not that it really pertains to whether d->name should be kept or not,
but I think that lirc_dev shouldn't copy the lirc_driver struct into an
irctl struct internal copy at all, but just keep a normal pointer. I
haven't gotten around to vetting the (ab)use of the lirc_driver struct
yet though.
Regards,
David
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 16/16] lirc_dev: cleanup header
2017-05-01 16:03 [PATCH 00/16] lirc_dev spring cleaning David Härdeman
` (14 preceding siblings ...)
2017-05-01 16:04 ` [PATCH 15/16] lirc_dev: remove name from struct lirc_driver David Härdeman
@ 2017-05-01 16:04 ` David Härdeman
15 siblings, 0 replies; 28+ messages in thread
From: David Härdeman @ 2017-05-01 16:04 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Remove some stuff from lirc_dev.h which is not used anywhere.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
include/media/lirc_dev.h | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 11f455a34090..af738d522dec 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -9,10 +9,6 @@
#ifndef _LINUX_LIRC_DEV_H
#define _LINUX_LIRC_DEV_H
-#define BUFLEN 16
-
-#define mod(n, div) ((n) % (div))
-
#include <linux/slab.h>
#include <linux/fs.h>
#include <linux/ioctl.h>
@@ -20,6 +16,8 @@
#include <linux/kfifo.h>
#include <media/lirc.h>
+#define BUFLEN 16
+
struct lirc_buffer {
wait_queue_head_t wait_poll;
spinlock_t fifo_lock;
@@ -89,11 +87,6 @@ static inline int lirc_buffer_empty(struct lirc_buffer *buf)
return !lirc_buffer_len(buf);
}
-static inline int lirc_buffer_available(struct lirc_buffer *buf)
-{
- return buf->size - (lirc_buffer_len(buf) / buf->chunk_size);
-}
-
static inline unsigned int lirc_buffer_read(struct lirc_buffer *buf,
unsigned char *dest)
{
^ permalink raw reply related [flat|nested] 28+ messages in thread