* [PATCH 01/19] lirc_dev: clarify error handling
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
@ 2017-06-25 12:31 ` David Härdeman
2017-06-25 12:31 ` [PATCH 02/19] lirc_dev: remove support for manually specifying minor number David Härdeman
` (17 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:31 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
If an error is generated, it is more logical to error out ASAP.
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 db1e7b70c998..9c1d55e41e34 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -282,7 +282,7 @@ EXPORT_SYMBOL(lirc_unregister_driver);
int lirc_dev_fop_open(struct inode *inode, struct file *file)
{
struct irctl *ir;
- int retval = 0;
+ int retval;
if (iminor(inode) >= MAX_IRCTL_DEVICES) {
pr_err("open result for %d is -ENODEV\n", iminor(inode));
@@ -323,9 +323,11 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
ir->open++;
-error:
nonseekable_open(inode, file);
+ return 0;
+
+error:
return retval;
}
EXPORT_SYMBOL(lirc_dev_fop_open);
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 02/19] lirc_dev: remove support for manually specifying minor number
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
2017-06-25 12:31 ` [PATCH 01/19] lirc_dev: clarify error handling David Härdeman
@ 2017-06-25 12:31 ` David Härdeman
2017-06-25 12:31 ` [PATCH 03/19] lirc_dev: remove min_timeout and max_timeout David Härdeman
` (16 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:31 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
All users of lirc_register_driver() uses dynamic minor allocation, therefore
we can remove the ability to explicitly request a given number.
This changes the function prototype of lirc_unregister_driver() to also
take a struct lirc_driver pointer as the sole argument.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ir-lirc-codec.c | 9 +---
drivers/media/rc/lirc_dev.c | 68 ++++++++-----------------------
drivers/staging/media/lirc/lirc_zilog.c | 14 ++----
include/media/lirc_dev.h | 18 ++++----
4 files changed, 33 insertions(+), 76 deletions(-)
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index a30af91710fe..2c1221a61ea1 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -382,7 +382,6 @@ static int ir_lirc_register(struct rc_dev *dev)
snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
dev->driver_name);
- drv->minor = -1;
drv->features = features;
drv->data = &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 9c1d55e41e34..c9afaf5e64a9 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -29,7 +29,6 @@
#include <media/lirc.h>
#include <media/lirc_dev.h>
-#define NOPLUG -1
#define LOGHEAD "lirc_dev (%s[%d]): "
static dev_t lirc_base_dev;
@@ -112,7 +111,7 @@ static int lirc_allocate_buffer(struct irctl *ir)
int lirc_register_driver(struct lirc_driver *d)
{
struct irctl *ir;
- int minor;
+ unsigned minor;
int err;
if (!d) {
@@ -130,12 +129,6 @@ int lirc_register_driver(struct lirc_driver *d)
return -EINVAL;
}
- if (d->minor >= MAX_IRCTL_DEVICES) {
- dev_err(d->dev, "minor must be between 0 and %d!\n",
- MAX_IRCTL_DEVICES - 1);
- return -EBADRQC;
- }
-
if (d->code_length < 1 || d->code_length > (BUFLEN * 8)) {
dev_err(d->dev, "code length must be less than %d bits\n",
BUFLEN * 8);
@@ -150,21 +143,14 @@ int lirc_register_driver(struct lirc_driver *d)
mutex_lock(&lirc_dev_lock);
- minor = d->minor;
+ /* find first free slot for driver */
+ for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++)
+ if (!irctls[minor])
+ break;
- if (minor < 0) {
- /* find first free slot for driver */
- for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++)
- if (!irctls[minor])
- break;
- if (minor == MAX_IRCTL_DEVICES) {
- dev_err(d->dev, "no free slots for drivers!\n");
- err = -ENOMEM;
- goto out_lock;
- }
- } else if (irctls[minor]) {
- dev_err(d->dev, "minor (%d) just registered!\n", minor);
- err = -EBUSY;
+ if (minor == MAX_IRCTL_DEVICES) {
+ dev_err(d->dev, "no free slots for drivers!\n");
+ err = -ENOMEM;
goto out_lock;
}
@@ -176,6 +162,7 @@ int lirc_register_driver(struct lirc_driver *d)
mutex_init(&ir->irctl_lock);
irctls[minor] = ir;
+ d->irctl = ir;
d->minor = minor;
/* some safety check 8-) */
@@ -221,7 +208,7 @@ int lirc_register_driver(struct lirc_driver *d)
dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
ir->d.name, ir->d.minor);
- return minor;
+ return 0;
out_cdev:
cdev_del(&ir->cdev);
@@ -234,38 +221,24 @@ int lirc_register_driver(struct lirc_driver *d)
}
EXPORT_SYMBOL(lirc_register_driver);
-int lirc_unregister_driver(int minor)
+void lirc_unregister_driver(struct lirc_driver *d)
{
struct irctl *ir;
- if (minor < 0 || minor >= MAX_IRCTL_DEVICES) {
- pr_err("minor (%d) must be between 0 and %d!\n",
- minor, MAX_IRCTL_DEVICES - 1);
- return -EBADRQC;
- }
+ if (!d || !d->irctl)
+ return;
- ir = irctls[minor];
- if (!ir) {
- pr_err("failed to get irctl\n");
- return -ENOENT;
- }
+ ir = d->irctl;
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;
- }
-
dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
- ir->d.name, ir->d.minor);
+ d->name, d->minor);
ir->attached = 0;
if (ir->open) {
dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
- ir->d.name, ir->d.minor);
+ d->name, d->minor);
wake_up_interruptible(&ir->buf->wait_poll);
}
@@ -274,8 +247,6 @@ int lirc_unregister_driver(int minor)
device_del(&ir->dev);
cdev_del(&ir->cdev);
put_device(&ir->dev);
-
- return 0;
}
EXPORT_SYMBOL(lirc_unregister_driver);
@@ -302,11 +273,6 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
- if (ir->d.minor == NOPLUG) {
- retval = -ENODEV;
- goto error;
- }
-
if (ir->open) {
retval = -EBUSY;
goto error;
@@ -399,7 +365,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
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->attached) {
dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
ir->d.name, ir->d.minor);
return -ENODEV;
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index 015e41bd036e..10594aea2a5c 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -183,10 +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);
@@ -1385,7 +1382,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,
@@ -1599,14 +1595,14 @@ 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;
+ __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 86d15a9b6c01..1419d64e2e59 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -116,10 +116,8 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
*
* @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
- * (if available).
+ * @minor: the minor device (/dev/lircX) number for a registered
+ * driver.
*
* @code_length: length of the remote control key code expressed in bits.
*
@@ -157,10 +155,12 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
* device.
*
* @owner: the module owning this struct
+ *
+ * @irctl: pointer to the struct irctl assigned to this LIRC device.
*/
struct lirc_driver {
char name[40];
- int minor;
+ unsigned minor;
__u32 code_length;
unsigned int buffer_size; /* in chunks holding one code each */
__u32 features;
@@ -175,19 +175,17 @@ struct lirc_driver {
const struct file_operations *fops;
struct device *dev;
struct module *owner;
+ struct irctl *irctl;
};
/* following functions can be called ONLY from user context
*
- * returns negative value on error or minor number
- * of the registered device if success
+ * returns negative value on error or zero
* 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] 23+ messages in thread* [PATCH 03/19] lirc_dev: remove min_timeout and max_timeout
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
2017-06-25 12:31 ` [PATCH 01/19] lirc_dev: clarify error handling David Härdeman
2017-06-25 12:31 ` [PATCH 02/19] lirc_dev: remove support for manually specifying minor number David Härdeman
@ 2017-06-25 12:31 ` David Härdeman
2017-06-25 12:31 ` [PATCH 04/19] lirc_dev: use cdev_device_add() helper function David Härdeman
` (15 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:31 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
There are no users of this functionality (ir-lirc-codec.c has its own
implementation and lirc_zilog.c doesn't use it) so remove it.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 18 ------------------
include/media/lirc_dev.h | 8 --------
2 files changed, 26 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index c9afaf5e64a9..591dee9f6ba2 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -404,24 +404,6 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
case LIRC_GET_LENGTH:
result = put_user(ir->d.code_length, (__u32 __user *)arg);
break;
- case LIRC_GET_MIN_TIMEOUT:
- if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||
- ir->d.min_timeout == 0) {
- result = -ENOTTY;
- break;
- }
-
- result = put_user(ir->d.min_timeout, (__u32 __user *)arg);
- break;
- case LIRC_GET_MAX_TIMEOUT:
- if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||
- ir->d.max_timeout == 0) {
- result = -ENOTTY;
- break;
- }
-
- result = put_user(ir->d.max_timeout, (__u32 __user *)arg);
- break;
default:
result = -ENOTTY;
}
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 1419d64e2e59..53eef86e07a0 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -132,12 +132,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
* @data: it may point to any driver data and this pointer will
* be passed to all callback functions.
*
- * @min_timeout: Minimum timeout for record. Valid only if
- * LIRC_CAN_SET_REC_TIMEOUT is defined.
- *
- * @max_timeout: Maximum timeout for record. Valid only if
- * LIRC_CAN_SET_REC_TIMEOUT is defined.
- *
* @rbuf: if not NULL, it will be used as a read buffer, you will
* have to write to the buffer by other means, like irq's
* (see also lirc_serial.c).
@@ -168,8 +162,6 @@ struct lirc_driver {
unsigned int chunk_size;
void *data;
- int min_timeout;
- int max_timeout;
struct lirc_buffer *rbuf;
struct rc_dev *rdev;
const struct file_operations *fops;
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 04/19] lirc_dev: use cdev_device_add() helper function
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (2 preceding siblings ...)
2017-06-25 12:31 ` [PATCH 03/19] lirc_dev: remove min_timeout and max_timeout David Härdeman
@ 2017-06-25 12:31 ` David Härdeman
2017-06-25 12:31 ` [PATCH 05/19] lirc_dev: make better use of file->private_data David Härdeman
` (14 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:31 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Replace calls to cdev_add() and device_add() with the cdev_device_add()
helper function.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 591dee9f6ba2..61ed90a975ad 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -191,17 +191,11 @@ int lirc_register_driver(struct lirc_driver *d)
cdev_init(&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;
-
ir->attached = 1;
- err = device_add(&ir->dev);
+ err = cdev_device_add(&ir->cdev, &ir->dev);
if (err)
- goto out_cdev;
+ goto out_dev;
mutex_unlock(&lirc_dev_lock);
@@ -210,9 +204,7 @@ int lirc_register_driver(struct lirc_driver *d)
return 0;
-out_cdev:
- cdev_del(&ir->cdev);
-out_free_dev:
+out_dev:
put_device(&ir->dev);
out_lock:
mutex_unlock(&lirc_dev_lock);
@@ -244,8 +236,7 @@ void lirc_unregister_driver(struct lirc_driver *d)
mutex_unlock(&lirc_dev_lock);
- device_del(&ir->dev);
- cdev_del(&ir->cdev);
+ cdev_device_del(&ir->cdev, &ir->dev);
put_device(&ir->dev);
}
EXPORT_SYMBOL(lirc_unregister_driver);
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 05/19] lirc_dev: make better use of file->private_data
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (3 preceding siblings ...)
2017-06-25 12:31 ` [PATCH 04/19] lirc_dev: use cdev_device_add() helper function David Härdeman
@ 2017-06-25 12:31 ` David Härdeman
2017-06-25 12:31 ` [PATCH 06/19] lirc_dev: make chunk_size and buffer_size mandatory David Härdeman
` (13 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:31 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
By making better use of file->private_data in lirc_dev we can avoid
digging around in the irctls[] array, thereby simplifying the code.
External drivers need to use lirc_get_pdata() instead of mucking around
in file->private_data.
The newly introduced lirc_init_pdata() function isn't very elegant, but
it's a stopgap measure which can be removed once lirc_zilog is converted
to rc-core.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 70 +++++++++----------------------
drivers/staging/media/lirc/lirc_zilog.c | 53 ++++-------------------
include/media/lirc_dev.h | 3 +
3 files changed, 33 insertions(+), 93 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 61ed90a975ad..2de840dd829d 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -243,36 +243,18 @@ EXPORT_SYMBOL(lirc_unregister_driver);
int lirc_dev_fop_open(struct inode *inode, struct file *file)
{
- struct irctl *ir;
+ struct irctl *ir = container_of(inode->i_cdev, struct irctl, cdev);
int retval;
- if (iminor(inode) >= MAX_IRCTL_DEVICES) {
- pr_err("open result for %d is -ENODEV\n", iminor(inode));
- return -ENODEV;
- }
-
- if (mutex_lock_interruptible(&lirc_dev_lock))
- return -ERESTARTSYS;
-
- ir = irctls[iminor(inode)];
- mutex_unlock(&lirc_dev_lock);
-
- if (!ir) {
- retval = -ENODEV;
- goto error;
- }
-
dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
- if (ir->open) {
- retval = -EBUSY;
- goto error;
- }
+ if (ir->open)
+ return -EBUSY;
if (ir->d.rdev) {
retval = rc_open(ir->d.rdev);
if (retval)
- goto error;
+ return retval;
}
if (ir->buf)
@@ -280,25 +262,18 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
ir->open++;
+ lirc_init_pdata(inode, file);
nonseekable_open(inode, file);
return 0;
-
-error:
- return retval;
}
EXPORT_SYMBOL(lirc_dev_fop_open);
int lirc_dev_fop_close(struct inode *inode, struct file *file)
{
- struct irctl *ir = irctls[iminor(inode)];
+ struct irctl *ir = file->private_data;
int ret;
- if (!ir) {
- pr_err("called with invalid irctl\n");
- return -EINVAL;
- }
-
ret = mutex_lock_killable(&lirc_dev_lock);
WARN_ON(ret);
@@ -314,14 +289,9 @@ EXPORT_SYMBOL(lirc_dev_fop_close);
unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
{
- struct irctl *ir = irctls[iminor(file_inode(file))];
+ struct irctl *ir = file->private_data;
unsigned int ret;
- if (!ir) {
- pr_err("called with invalid irctl\n");
- return POLLERR;
- }
-
if (!ir->attached)
return POLLHUP | POLLERR;
@@ -344,14 +314,9 @@ EXPORT_SYMBOL(lirc_dev_fop_poll);
long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
+ struct irctl *ir = file->private_data;
__u32 mode;
int result = 0;
- struct irctl *ir = irctls[iminor(file_inode(file))];
-
- if (!ir) {
- pr_err("no irctl found!\n");
- return -ENODEV;
- }
dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
ir->d.name, ir->d.minor, cmd);
@@ -410,16 +375,11 @@ ssize_t lirc_dev_fop_read(struct file *file,
size_t length,
loff_t *ppos)
{
- struct irctl *ir = irctls[iminor(file_inode(file))];
+ struct irctl *ir = file->private_data;
unsigned char *buf;
int ret = 0, written = 0;
DECLARE_WAITQUEUE(wait, current);
- if (!ir) {
- pr_err("called with invalid irctl\n");
- return -ENODEV;
- }
-
if (!LIRC_CAN_REC(ir->d.features))
return -EINVAL;
@@ -510,9 +470,19 @@ ssize_t lirc_dev_fop_read(struct file *file,
}
EXPORT_SYMBOL(lirc_dev_fop_read);
+void lirc_init_pdata(struct inode *inode, struct file *file)
+{
+ struct irctl *ir = container_of(inode->i_cdev, struct irctl, cdev);
+
+ file->private_data = ir;
+}
+EXPORT_SYMBOL(lirc_init_pdata);
+
void *lirc_get_pdata(struct file *file)
{
- return irctls[iminor(file_inode(file))]->d.data;
+ struct irctl *ir = file->private_data;
+
+ return ir->d.data;
}
EXPORT_SYMBOL(lirc_get_pdata);
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index 10594aea2a5c..6d4c5a957ab4 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -879,7 +879,7 @@ static int fw_load(struct IR_tx *tx)
static ssize_t read(struct file *filep, char __user *outbuf, size_t n,
loff_t *ppos)
{
- struct IR *ir = filep->private_data;
+ struct IR *ir = lirc_get_pdata(filep);
struct IR_rx *rx;
struct lirc_buffer *rbuf = ir->l.rbuf;
int ret = 0, written = 0, retries = 0;
@@ -1089,7 +1089,7 @@ static int send_code(struct IR_tx *tx, unsigned int code, unsigned int key)
static ssize_t write(struct file *filep, const char __user *buf, size_t n,
loff_t *ppos)
{
- struct IR *ir = filep->private_data;
+ struct IR *ir = lirc_get_pdata(filep);
struct IR_tx *tx;
size_t i;
int failures = 0;
@@ -1197,7 +1197,7 @@ static ssize_t write(struct file *filep, const char __user *buf, size_t n,
/* copied from lirc_dev */
static unsigned int poll(struct file *filep, poll_table *wait)
{
- struct IR *ir = filep->private_data;
+ struct IR *ir = lirc_get_pdata(filep);
struct IR_rx *rx;
struct lirc_buffer *rbuf = ir->l.rbuf;
unsigned int ret;
@@ -1230,7 +1230,7 @@ static unsigned int poll(struct file *filep, poll_table *wait)
static long ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
{
- struct IR *ir = filep->private_data;
+ struct IR *ir = lirc_get_pdata(filep);
unsigned long __user *uptr = (unsigned long __user *)arg;
int result;
unsigned long mode, features;
@@ -1280,46 +1280,18 @@ static long ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
return result;
}
-static struct IR *get_ir_device_by_minor(unsigned int minor)
-{
- struct IR *ir;
- struct IR *ret = NULL;
-
- mutex_lock(&ir_devices_lock);
-
- if (!list_empty(&ir_devices_list)) {
- list_for_each_entry(ir, &ir_devices_list, list) {
- if (ir->l.minor == minor) {
- ret = get_ir_device(ir, true);
- break;
- }
- }
- }
-
- mutex_unlock(&ir_devices_lock);
- return ret;
-}
-
/*
- * Open the IR device. Get hold of our IR structure and
- * stash it in private_data for the file
+ * Open the IR device.
*/
static int open(struct inode *node, struct file *filep)
{
struct IR *ir;
- unsigned int minor = MINOR(node->i_rdev);
-
- /* find our IR struct */
- ir = get_ir_device_by_minor(minor);
- if (!ir)
- return -ENODEV;
+ lirc_init_pdata(node, filep);
+ ir = lirc_get_pdata(filep);
atomic_inc(&ir->open_count);
- /* stash our IR struct */
- filep->private_data = ir;
-
nonseekable_open(node, filep);
return 0;
}
@@ -1327,14 +1299,7 @@ static int open(struct inode *node, struct file *filep)
/* Close the IR device */
static int close(struct inode *node, struct file *filep)
{
- /* find our IR struct */
- struct IR *ir = filep->private_data;
-
- if (!ir) {
- pr_err("ir: %s: no private_data attached to the file!\n",
- __func__);
- return -ENODEV;
- }
+ struct IR *ir = lirc_get_pdata(filep);
atomic_dec(&ir->open_count);
@@ -1489,6 +1454,8 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
*/
ir->l.rbuf = &ir->rbuf;
ir->l.dev = &adap->dev;
+ /* This will be returned by lirc_get_pdata() */
+ ir->l.data = ir;
ret = lirc_buffer_init(ir->l.rbuf,
ir->l.chunk_size, ir->l.buffer_size);
if (ret)
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 53eef86e07a0..20c5c5d6f101 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -179,6 +179,9 @@ extern int lirc_register_driver(struct lirc_driver *d);
extern void lirc_unregister_driver(struct lirc_driver *d);
+/* Must be called in the open fop before lirc_get_pdata() can be used */
+void lirc_init_pdata(struct inode *inode, struct file *file);
+
/* Returns the private data stored in the lirc_driver
* associated with the given device file pointer.
*/
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 06/19] lirc_dev: make chunk_size and buffer_size mandatory
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (4 preceding siblings ...)
2017-06-25 12:31 ` [PATCH 05/19] lirc_dev: make better use of file->private_data David Härdeman
@ 2017-06-25 12:31 ` David Härdeman
2017-06-25 12:31 ` [PATCH 07/19] lirc_dev: remove kmalloc in lirc_dev_fop_read() David Härdeman
` (12 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:31 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Make setting chunk_size and buffer_size mandatory for drivers which
expect lirc_dev to allocate the lirc_buffer (i.e. ir-lirc-codec) and
don't set them in lirc-zilog (which creates its own buffer).
Also remove an unnecessary copy of chunk_size in struct irctl (the
same information is already available from struct lirc_buffer).
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 26 +++++++++++++-------------
drivers/staging/media/lirc/lirc_zilog.c | 5 +----
include/media/lirc_dev.h | 9 +++++----
3 files changed, 19 insertions(+), 21 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 2de840dd829d..1773a2934484 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -41,7 +41,6 @@ struct irctl {
struct mutex irctl_lock;
struct lirc_buffer *buf;
bool buf_internal;
- unsigned int chunk_size;
struct device dev;
struct cdev cdev;
@@ -72,16 +71,8 @@ static void lirc_release(struct device *ld)
static int lirc_allocate_buffer(struct irctl *ir)
{
int err = 0;
- int bytes_in_key;
- unsigned int chunk_size;
- unsigned int buffer_size;
struct lirc_driver *d = &ir->d;
- bytes_in_key = BITS_TO_LONGS(d->code_length) +
- (d->code_length % 8 ? 1 : 0);
- buffer_size = d->buffer_size ? d->buffer_size : BUFLEN / bytes_in_key;
- chunk_size = d->chunk_size ? d->chunk_size : bytes_in_key;
-
if (d->rbuf) {
ir->buf = d->rbuf;
ir->buf_internal = false;
@@ -92,7 +83,7 @@ static int lirc_allocate_buffer(struct irctl *ir)
goto out;
}
- err = lirc_buffer_init(ir->buf, chunk_size, buffer_size);
+ err = lirc_buffer_init(ir->buf, d->chunk_size, d->buffer_size);
if (err) {
kfree(ir->buf);
ir->buf = NULL;
@@ -102,7 +93,6 @@ static int lirc_allocate_buffer(struct irctl *ir)
ir->buf_internal = true;
d->rbuf = ir->buf;
}
- ir->chunk_size = ir->buf->chunk_size;
out:
return err;
@@ -129,6 +119,16 @@ int lirc_register_driver(struct lirc_driver *d)
return -EINVAL;
}
+ if (!d->rbuf && d->chunk_size < 1) {
+ pr_err("chunk_size must be set!\n");
+ return -EINVAL;
+ }
+
+ if (!d->rbuf && d->buffer_size < 1) {
+ pr_err("buffer_size must be set!\n");
+ return -EINVAL;
+ }
+
if (d->code_length < 1 || d->code_length > (BUFLEN * 8)) {
dev_err(d->dev, "code length must be less than %d bits\n",
BUFLEN * 8);
@@ -385,7 +385,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
- buf = kzalloc(ir->chunk_size, GFP_KERNEL);
+ buf = kzalloc(ir->buf->chunk_size, GFP_KERNEL);
if (!buf)
return -ENOMEM;
@@ -398,7 +398,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
goto out_locked;
}
- if (length % ir->chunk_size) {
+ if (length % ir->buf->chunk_size) {
ret = -EINVAL;
goto out_locked;
}
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index 6d4c5a957ab4..c6a2fe2ad210 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1348,8 +1348,6 @@ static const struct file_operations lirc_fops = {
static struct lirc_driver lirc_template = {
.name = "lirc_zilog",
.code_length = 13,
- .buffer_size = BUFLEN / 2,
- .chunk_size = 2,
.fops = &lirc_fops,
.owner = THIS_MODULE,
};
@@ -1456,8 +1454,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
ir->l.dev = &adap->dev;
/* This will be returned by lirc_get_pdata() */
ir->l.data = ir;
- ret = lirc_buffer_init(ir->l.rbuf,
- ir->l.chunk_size, ir->l.buffer_size);
+ ret = lirc_buffer_init(ir->l.rbuf, 2, BUFLEN / 2);
if (ret)
goto out_put_ir;
}
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 20c5c5d6f101..a01fe5433bb7 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -121,13 +121,14 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
*
* @code_length: length of the remote control key code expressed in bits.
*
- * @buffer_size: Number of FIFO buffers with @chunk_size size. If zero,
- * creates a buffer with BUFLEN size (16 bytes).
- *
* @features: lirc compatible hardware features, like LIRC_MODE_RAW,
* LIRC_CAN\_\*, as defined at include/media/lirc.h.
*
+ * @buffer_size: Number of FIFO buffers with @chunk_size size.
+ * Only used if @rbuf is NULL.
+ *
* @chunk_size: Size of each FIFO buffer.
+ * Only used if @rbuf is NULL.
*
* @data: it may point to any driver data and this pointer will
* be passed to all callback functions.
@@ -156,9 +157,9 @@ struct lirc_driver {
char name[40];
unsigned minor;
__u32 code_length;
- unsigned int buffer_size; /* in chunks holding one code each */
__u32 features;
+ unsigned int buffer_size; /* in chunks holding one code each */
unsigned int chunk_size;
void *data;
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 07/19] lirc_dev: remove kmalloc in lirc_dev_fop_read()
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (5 preceding siblings ...)
2017-06-25 12:31 ` [PATCH 06/19] lirc_dev: make chunk_size and buffer_size mandatory David Härdeman
@ 2017-06-25 12:31 ` David Härdeman
2017-10-04 16:56 ` Mauro Carvalho Chehab
2017-06-25 12:31 ` [PATCH 08/19] lirc_dev: change irctl->attached to be a boolean David Härdeman
` (11 subsequent siblings)
18 siblings, 1 reply; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:31 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
lirc_zilog uses a chunk_size of 2 and ir-lirc-codec uses sizeof(int).
Therefore, using stack memory should be perfectly fine.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 1773a2934484..92048d945ba7 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -376,7 +376,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
loff_t *ppos)
{
struct irctl *ir = file->private_data;
- unsigned char *buf;
+ unsigned char buf[ir->buf->chunk_size];
int ret = 0, written = 0;
DECLARE_WAITQUEUE(wait, current);
@@ -385,10 +385,6 @@ ssize_t lirc_dev_fop_read(struct file *file,
dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
- buf = kzalloc(ir->buf->chunk_size, GFP_KERNEL);
- if (!buf)
- return -ENOMEM;
-
if (mutex_lock_interruptible(&ir->irctl_lock)) {
ret = -ERESTARTSYS;
goto out_unlocked;
@@ -464,8 +460,6 @@ ssize_t lirc_dev_fop_read(struct file *file,
mutex_unlock(&ir->irctl_lock);
out_unlocked:
- kfree(buf);
-
return ret ? ret : written;
}
EXPORT_SYMBOL(lirc_dev_fop_read);
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 07/19] lirc_dev: remove kmalloc in lirc_dev_fop_read()
2017-06-25 12:31 ` [PATCH 07/19] lirc_dev: remove kmalloc in lirc_dev_fop_read() David Härdeman
@ 2017-10-04 16:56 ` Mauro Carvalho Chehab
2017-10-09 9:45 ` David Härdeman
0 siblings, 1 reply; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2017-10-04 16:56 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, sean
Em Sun, 25 Jun 2017 14:31:50 +0200
David Härdeman <david@hardeman.nu> escreveu:
> lirc_zilog uses a chunk_size of 2 and ir-lirc-codec uses sizeof(int).
>
> Therefore, using stack memory should be perfectly fine.
>
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
> drivers/media/rc/lirc_dev.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 1773a2934484..92048d945ba7 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -376,7 +376,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
> loff_t *ppos)
> {
> struct irctl *ir = file->private_data;
> - unsigned char *buf;
> + unsigned char buf[ir->buf->chunk_size];
No. We don't do dynamic buffer allocation on stak at the Kernel,
as this could cause the Linux stack to overflow without notice.
This should also generate alerts on static code analyzers like
sparse.
I'll drop this patch from the series.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 07/19] lirc_dev: remove kmalloc in lirc_dev_fop_read()
2017-10-04 16:56 ` Mauro Carvalho Chehab
@ 2017-10-09 9:45 ` David Härdeman
0 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-10-09 9:45 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, sean
October 4, 2017 6:57 PM, "Mauro Carvalho Chehab" <mchehab@s-opensource.com> wrote:
> Em Sun, 25 Jun 2017 14:31:50 +0200
> David Härdeman <david@hardeman.nu> escreveu:
>
>> lirc_zilog uses a chunk_size of 2 and ir-lirc-codec uses sizeof(int).
>>
>> Therefore, using stack memory should be perfectly fine.
>>
>> Signed-off-by: David Härdeman <david@hardeman.nu>
>> ---
>> drivers/media/rc/lirc_dev.c | 8 +-------
>> 1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
>> index 1773a2934484..92048d945ba7 100644
>> --- a/drivers/media/rc/lirc_dev.c
>> +++ b/drivers/media/rc/lirc_dev.c
>> @@ -376,7 +376,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
>> loff_t *ppos)
>> {
>> struct irctl *ir = file->private_data;
>> - unsigned char *buf;
>> + unsigned char buf[ir->buf->chunk_size];
>
> No. We don't do dynamic buffer allocation on stak at the Kernel,
> as this could cause the Linux stack to overflow without notice.
While the general policy is to avoid large stack allocations (in order to not overflow the 4K stack), I'm not aware of a general ban on *any* stack allocations - that sounds like an overly dogmatic approach. If such a generic ban exists, could you please point me to some kind of discussion/message to that effect?
The commit message clearly explained what kind of stack allocations we're talking about here (i.e. sizeof(int) is the maximum), so the stack allocation is clearly not able to cause stack overflows. And once the last lirc driver is gone, this can be changed to a simple int (which would also be allocated on stack).
Regards,
David
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 08/19] lirc_dev: change irctl->attached to be a boolean
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (6 preceding siblings ...)
2017-06-25 12:31 ` [PATCH 07/19] lirc_dev: remove kmalloc in lirc_dev_fop_read() David Härdeman
@ 2017-06-25 12:31 ` David Härdeman
2017-06-25 12:32 ` [PATCH 09/19] lirc_dev: sanitize locking David Härdeman
` (10 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:31 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
The "attached" member of struct irctl is a boolean value, so let the code
reflect that.
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 92048d945ba7..aece6b619796 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -35,7 +35,7 @@ static dev_t lirc_base_dev;
struct irctl {
struct lirc_driver d;
- int attached;
+ bool attached;
int open;
struct mutex irctl_lock;
@@ -191,7 +191,7 @@ int lirc_register_driver(struct lirc_driver *d)
cdev_init(&ir->cdev, d->fops);
ir->cdev.owner = ir->d.owner;
- ir->attached = 1;
+ ir->attached = true;
err = cdev_device_add(&ir->cdev, &ir->dev);
if (err)
@@ -227,7 +227,7 @@ void lirc_unregister_driver(struct lirc_driver *d)
dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
d->name, d->minor);
- ir->attached = 0;
+ ir->attached = false;
if (ir->open) {
dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
d->name, d->minor);
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 09/19] lirc_dev: sanitize locking
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (7 preceding siblings ...)
2017-06-25 12:31 ` [PATCH 08/19] lirc_dev: change irctl->attached to be a boolean David Härdeman
@ 2017-06-25 12:32 ` David Härdeman
2017-06-25 12:32 ` [PATCH 10/19] lirc_dev: use an IDA instead of an array to keep track of registered devices David Härdeman
` (9 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:32 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Use the irctl mutex for all device operations and only use lirc_dev_lock to
protect the irctls array. Also, make sure that the device is alive early in
each fops function before doing anything else.
Since this patch touches nearly every line where the irctl mutex is
taken/released, it also renames the mutex at the same time (the name
irctl_lock will be misleading once struct irctl goes away in later
patches).
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 164 ++++++++++++++++++++++++-------------------
1 file changed, 91 insertions(+), 73 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index aece6b619796..6bd21609a719 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -38,7 +38,7 @@ struct irctl {
bool attached;
int open;
- struct mutex irctl_lock;
+ struct mutex mutex;
struct lirc_buffer *buf;
bool buf_internal;
@@ -46,6 +46,7 @@ struct irctl {
struct cdev cdev;
};
+/* This mutex protects the irctls array */
static DEFINE_MUTEX(lirc_dev_lock);
static struct irctl *irctls[MAX_IRCTL_DEVICES];
@@ -53,18 +54,23 @@ static struct irctl *irctls[MAX_IRCTL_DEVICES];
/* Only used for sysfs but defined to void otherwise */
static struct class *lirc_class;
-static void lirc_release(struct device *ld)
+static void lirc_free_buffer(struct irctl *ir)
{
- struct irctl *ir = container_of(ld, struct irctl, dev);
-
if (ir->buf_internal) {
lirc_buffer_free(ir->buf);
kfree(ir->buf);
+ ir->buf = NULL;
}
+}
+
+static void lirc_release(struct device *ld)
+{
+ struct irctl *ir = container_of(ld, struct irctl, dev);
mutex_lock(&lirc_dev_lock);
irctls[ir->d.minor] = NULL;
mutex_unlock(&lirc_dev_lock);
+ lirc_free_buffer(ir);
kfree(ir);
}
@@ -141,6 +147,28 @@ int lirc_register_driver(struct lirc_driver *d)
return -EBADRQC;
}
+ /* some safety check 8-) */
+ d->name[sizeof(d->name)-1] = '\0';
+
+ if (d->features == 0)
+ d->features = LIRC_CAN_REC_LIRCCODE;
+
+ ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
+ if (!ir)
+ return -ENOMEM;
+
+ mutex_init(&ir->mutex);
+ ir->d = *d;
+
+ if (LIRC_CAN_REC(d->features)) {
+ err = lirc_allocate_buffer(ir);
+ if (err) {
+ kfree(ir);
+ return err;
+ }
+ d->rbuf = ir->buf;
+ }
+
mutex_lock(&lirc_dev_lock);
/* find first free slot for driver */
@@ -150,37 +178,17 @@ int lirc_register_driver(struct lirc_driver *d)
if (minor == MAX_IRCTL_DEVICES) {
dev_err(d->dev, "no free slots for drivers!\n");
- err = -ENOMEM;
- goto out_lock;
- }
-
- ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
- if (!ir) {
- err = -ENOMEM;
- goto out_lock;
+ mutex_unlock(&lirc_dev_lock);
+ lirc_free_buffer(ir);
+ kfree(ir);
+ return -ENOMEM;
}
- mutex_init(&ir->irctl_lock);
irctls[minor] = ir;
d->irctl = ir;
d->minor = minor;
- /* some safety check 8-) */
- d->name[sizeof(d->name)-1] = '\0';
-
- if (d->features == 0)
- d->features = LIRC_CAN_REC_LIRCCODE;
-
- ir->d = *d;
-
- if (LIRC_CAN_REC(d->features)) {
- err = lirc_allocate_buffer(irctls[minor]);
- if (err) {
- kfree(ir);
- goto out_lock;
- }
- d->rbuf = ir->buf;
- }
+ mutex_unlock(&lirc_dev_lock);
device_initialize(&ir->dev);
ir->dev.devt = MKDEV(MAJOR(lirc_base_dev), ir->d.minor);
@@ -194,22 +202,15 @@ int lirc_register_driver(struct lirc_driver *d)
ir->attached = true;
err = cdev_device_add(&ir->cdev, &ir->dev);
- if (err)
- goto out_dev;
-
- mutex_unlock(&lirc_dev_lock);
+ if (err) {
+ put_device(&ir->dev);
+ return err;
+ }
dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
ir->d.name, ir->d.minor);
return 0;
-
-out_dev:
- put_device(&ir->dev);
-out_lock:
- mutex_unlock(&lirc_dev_lock);
-
- return err;
}
EXPORT_SYMBOL(lirc_register_driver);
@@ -222,11 +223,13 @@ void lirc_unregister_driver(struct lirc_driver *d)
ir = d->irctl;
- mutex_lock(&lirc_dev_lock);
-
dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
d->name, d->minor);
+ cdev_device_del(&ir->cdev, &ir->dev);
+
+ mutex_lock(&ir->mutex);
+
ir->attached = false;
if (ir->open) {
dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
@@ -234,9 +237,8 @@ void lirc_unregister_driver(struct lirc_driver *d)
wake_up_interruptible(&ir->buf->wait_poll);
}
- mutex_unlock(&lirc_dev_lock);
+ mutex_unlock(&ir->mutex);
- cdev_device_del(&ir->cdev, &ir->dev);
put_device(&ir->dev);
}
EXPORT_SYMBOL(lirc_unregister_driver);
@@ -248,13 +250,24 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
- if (ir->open)
- return -EBUSY;
+ retval = mutex_lock_interruptible(&ir->mutex);
+ if (retval)
+ return retval;
+
+ if (!ir->attached) {
+ retval = -ENODEV;
+ goto out;
+ }
+
+ if (ir->open) {
+ retval = -EBUSY;
+ goto out;
+ }
if (ir->d.rdev) {
retval = rc_open(ir->d.rdev);
if (retval)
- return retval;
+ goto out;
}
if (ir->buf)
@@ -264,24 +277,26 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
lirc_init_pdata(inode, file);
nonseekable_open(inode, file);
+ mutex_unlock(&ir->mutex);
return 0;
+
+out:
+ mutex_unlock(&ir->mutex);
+ return retval;
}
EXPORT_SYMBOL(lirc_dev_fop_open);
int lirc_dev_fop_close(struct inode *inode, struct file *file)
{
struct irctl *ir = file->private_data;
- int ret;
- ret = mutex_lock_killable(&lirc_dev_lock);
- WARN_ON(ret);
+ mutex_lock(&ir->mutex);
rc_close(ir->d.rdev);
-
ir->open--;
- if (!ret)
- mutex_unlock(&lirc_dev_lock);
+
+ mutex_unlock(&ir->mutex);
return 0;
}
@@ -316,19 +331,20 @@ 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;
+ int result;
dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
ir->d.name, ir->d.minor, cmd);
+ result = mutex_lock_interruptible(&ir->mutex);
+ if (result)
+ return result;
+
if (!ir->attached) {
- dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
- ir->d.name, ir->d.minor);
- return -ENODEV;
+ result = -ENODEV;
+ goto out;
}
- mutex_lock(&ir->irctl_lock);
-
switch (cmd) {
case LIRC_GET_FEATURES:
result = put_user(ir->d.features, (__u32 __user *)arg);
@@ -364,8 +380,8 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
result = -ENOTTY;
}
- mutex_unlock(&ir->irctl_lock);
-
+out:
+ mutex_unlock(&ir->mutex);
return result;
}
EXPORT_SYMBOL(lirc_dev_fop_ioctl);
@@ -377,23 +393,25 @@ ssize_t lirc_dev_fop_read(struct file *file,
{
struct irctl *ir = file->private_data;
unsigned char buf[ir->buf->chunk_size];
- int ret = 0, written = 0;
+ int ret, written = 0;
DECLARE_WAITQUEUE(wait, current);
- if (!LIRC_CAN_REC(ir->d.features))
- return -EINVAL;
-
dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
- if (mutex_lock_interruptible(&ir->irctl_lock)) {
- ret = -ERESTARTSYS;
- goto out_unlocked;
- }
+ ret = mutex_lock_interruptible(&ir->mutex);
+ if (ret)
+ return ret;
+
if (!ir->attached) {
ret = -ENODEV;
goto out_locked;
}
+ if (!LIRC_CAN_REC(ir->d.features)) {
+ ret = -EINVAL;
+ goto out_locked;
+ }
+
if (length % ir->buf->chunk_size) {
ret = -EINVAL;
goto out_locked;
@@ -428,13 +446,13 @@ 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)) {
- ret = -ERESTARTSYS;
+ ret = mutex_lock_interruptible(&ir->mutex);
+ if (ret) {
remove_wait_queue(&ir->buf->wait_poll, &wait);
goto out_unlocked;
}
@@ -457,7 +475,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:
return ret ? ret : written;
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 10/19] lirc_dev: use an IDA instead of an array to keep track of registered devices
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (8 preceding siblings ...)
2017-06-25 12:32 ` [PATCH 09/19] lirc_dev: sanitize locking David Härdeman
@ 2017-06-25 12:32 ` David Härdeman
2017-06-25 12:32 ` [PATCH 11/19] lirc_dev: rename struct lirc_driver to struct lirc_dev David Härdeman
` (8 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:32 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Using the kernel-provided IDA simplifies the code and makes it possible
to remove the lirc_dev_lock mutex.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 36 ++++++++++++------------------------
include/media/lirc_dev.h | 1 -
2 files changed, 12 insertions(+), 25 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 6bd21609a719..996cb5e778dc 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -24,6 +24,7 @@
#include <linux/mutex.h>
#include <linux/device.h>
#include <linux/cdev.h>
+#include <linux/idr.h>
#include <media/rc-core.h>
#include <media/lirc.h>
@@ -46,10 +47,9 @@ struct irctl {
struct cdev cdev;
};
-/* This mutex protects the irctls array */
-static DEFINE_MUTEX(lirc_dev_lock);
-
-static struct irctl *irctls[MAX_IRCTL_DEVICES];
+/* Used to keep track of allocated lirc devices */
+#define LIRC_MAX_DEVICES 256
+static DEFINE_IDA(lirc_ida);
/* Only used for sysfs but defined to void otherwise */
static struct class *lirc_class;
@@ -67,9 +67,6 @@ static void lirc_release(struct device *ld)
{
struct irctl *ir = container_of(ld, struct irctl, dev);
- mutex_lock(&lirc_dev_lock);
- irctls[ir->d.minor] = NULL;
- mutex_unlock(&lirc_dev_lock);
lirc_free_buffer(ir);
kfree(ir);
}
@@ -107,7 +104,7 @@ static int lirc_allocate_buffer(struct irctl *ir)
int lirc_register_driver(struct lirc_driver *d)
{
struct irctl *ir;
- unsigned minor;
+ int minor;
int err;
if (!d) {
@@ -169,27 +166,16 @@ int lirc_register_driver(struct lirc_driver *d)
d->rbuf = ir->buf;
}
- mutex_lock(&lirc_dev_lock);
-
- /* find first free slot for driver */
- for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++)
- if (!irctls[minor])
- break;
-
- if (minor == MAX_IRCTL_DEVICES) {
- dev_err(d->dev, "no free slots for drivers!\n");
- mutex_unlock(&lirc_dev_lock);
+ minor = ida_simple_get(&lirc_ida, 0, LIRC_MAX_DEVICES, GFP_KERNEL);
+ if (minor < 0) {
lirc_free_buffer(ir);
kfree(ir);
- return -ENOMEM;
+ return minor;
}
- irctls[minor] = ir;
d->irctl = ir;
d->minor = minor;
- mutex_unlock(&lirc_dev_lock);
-
device_initialize(&ir->dev);
ir->dev.devt = MKDEV(MAJOR(lirc_base_dev), ir->d.minor);
ir->dev.class = lirc_class;
@@ -203,6 +189,7 @@ int lirc_register_driver(struct lirc_driver *d)
err = cdev_device_add(&ir->cdev, &ir->dev);
if (err) {
+ ida_simple_remove(&lirc_ida, minor);
put_device(&ir->dev);
return err;
}
@@ -239,6 +226,7 @@ void lirc_unregister_driver(struct lirc_driver *d)
mutex_unlock(&ir->mutex);
+ ida_simple_remove(&lirc_ida, d->minor);
put_device(&ir->dev);
}
EXPORT_SYMBOL(lirc_unregister_driver);
@@ -509,7 +497,7 @@ static int __init lirc_dev_init(void)
return PTR_ERR(lirc_class);
}
- retval = alloc_chrdev_region(&lirc_base_dev, 0, MAX_IRCTL_DEVICES,
+ retval = alloc_chrdev_region(&lirc_base_dev, 0, LIRC_MAX_DEVICES,
"BaseRemoteCtl");
if (retval) {
class_destroy(lirc_class);
@@ -526,7 +514,7 @@ static int __init lirc_dev_init(void)
static void __exit lirc_dev_exit(void)
{
class_destroy(lirc_class);
- unregister_chrdev_region(lirc_base_dev, MAX_IRCTL_DEVICES);
+ unregister_chrdev_region(lirc_base_dev, LIRC_MAX_DEVICES);
pr_info("module unloaded\n");
}
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index a01fe5433bb7..2629c40e8a39 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -9,7 +9,6 @@
#ifndef _LINUX_LIRC_DEV_H
#define _LINUX_LIRC_DEV_H
-#define MAX_IRCTL_DEVICES 8
#define BUFLEN 16
#include <linux/slab.h>
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 11/19] lirc_dev: rename struct lirc_driver to struct lirc_dev
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (9 preceding siblings ...)
2017-06-25 12:32 ` [PATCH 10/19] lirc_dev: use an IDA instead of an array to keep track of registered devices David Härdeman
@ 2017-06-25 12:32 ` David Härdeman
2017-06-25 12:32 ` [PATCH 12/19] lirc_dev: introduce lirc_allocate_device and lirc_free_device David Härdeman
` (7 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:32 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
This is in preparation for the later patches which do away with
struct irctl entirely.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ir-lirc-codec.c | 50 ++++++++++++++++---------------
drivers/media/rc/lirc_dev.c | 12 ++++---
drivers/media/rc/rc-core-priv.h | 2 +
drivers/staging/media/lirc/lirc_zilog.c | 12 ++++---
include/media/lirc_dev.h | 46 ++++++++---------------------
5 files changed, 51 insertions(+), 71 deletions(-)
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 2c1221a61ea1..2349630ed383 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -35,7 +35,7 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
struct lirc_codec *lirc = &dev->raw->lirc;
int sample;
- if (!dev->raw->lirc.drv || !dev->raw->lirc.drv->rbuf)
+ if (!dev->raw->lirc.ldev || !dev->raw->lirc.ldev->rbuf)
return -EINVAL;
/* Packet start */
@@ -84,7 +84,7 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
(u64)LIRC_VALUE_MASK);
gap_sample = LIRC_SPACE(lirc->gap_duration);
- lirc_buffer_write(dev->raw->lirc.drv->rbuf,
+ lirc_buffer_write(dev->raw->lirc.ldev->rbuf,
(unsigned char *) &gap_sample);
lirc->gap = false;
}
@@ -95,9 +95,9 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
TO_US(ev.duration), TO_STR(ev.pulse));
}
- lirc_buffer_write(dev->raw->lirc.drv->rbuf,
+ lirc_buffer_write(dev->raw->lirc.ldev->rbuf,
(unsigned char *) &sample);
- wake_up(&dev->raw->lirc.drv->rbuf->wait_poll);
+ wake_up(&dev->raw->lirc.ldev->rbuf->wait_poll);
return 0;
}
@@ -343,12 +343,12 @@ static const struct file_operations lirc_fops = {
static int ir_lirc_register(struct rc_dev *dev)
{
- struct lirc_driver *drv;
+ struct lirc_dev *ldev;
int rc = -ENOMEM;
unsigned long features = 0;
- drv = kzalloc(sizeof(struct lirc_driver), GFP_KERNEL);
- if (!drv)
+ ldev = kzalloc(sizeof(struct lirc_dev), GFP_KERNEL);
+ if (!ldev)
return rc;
if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
@@ -380,29 +380,29 @@ static int ir_lirc_register(struct rc_dev *dev)
if (dev->max_timeout)
features |= LIRC_CAN_SET_REC_TIMEOUT;
- snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)",
+ snprintf(ldev->name, sizeof(ldev->name), "ir-lirc-codec (%s)",
dev->driver_name);
- drv->features = features;
- drv->data = &dev->raw->lirc;
- drv->rbuf = NULL;
- drv->code_length = sizeof(struct ir_raw_event) * 8;
- drv->chunk_size = sizeof(int);
- drv->buffer_size = LIRCBUF_SIZE;
- drv->fops = &lirc_fops;
- drv->dev = &dev->dev;
- drv->rdev = dev;
- drv->owner = THIS_MODULE;
-
- rc = lirc_register_driver(drv);
+ ldev->features = features;
+ ldev->data = &dev->raw->lirc;
+ ldev->rbuf = NULL;
+ ldev->code_length = sizeof(struct ir_raw_event) * 8;
+ ldev->chunk_size = sizeof(int);
+ ldev->buffer_size = LIRCBUF_SIZE;
+ ldev->fops = &lirc_fops;
+ ldev->dev = &dev->dev;
+ ldev->rdev = dev;
+ ldev->owner = THIS_MODULE;
+
+ rc = lirc_register_device(ldev);
if (rc < 0)
goto out;
- dev->raw->lirc.drv = drv;
+ dev->raw->lirc.ldev = ldev;
dev->raw->lirc.dev = dev;
return 0;
out:
- kfree(drv);
+ kfree(ldev);
return rc;
}
@@ -410,9 +410,9 @@ static int ir_lirc_unregister(struct rc_dev *dev)
{
struct lirc_codec *lirc = &dev->raw->lirc;
- lirc_unregister_driver(lirc->drv);
- kfree(lirc->drv);
- lirc->drv = NULL;
+ lirc_unregister_device(lirc->ldev);
+ kfree(lirc->ldev);
+ lirc->ldev = NULL;
return 0;
}
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 996cb5e778dc..1c2f1a07bdaa 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -35,7 +35,7 @@
static dev_t lirc_base_dev;
struct irctl {
- struct lirc_driver d;
+ struct lirc_dev d;
bool attached;
int open;
@@ -74,7 +74,7 @@ static void lirc_release(struct device *ld)
static int lirc_allocate_buffer(struct irctl *ir)
{
int err = 0;
- struct lirc_driver *d = &ir->d;
+ struct lirc_dev *d = &ir->d;
if (d->rbuf) {
ir->buf = d->rbuf;
@@ -101,7 +101,7 @@ static int lirc_allocate_buffer(struct irctl *ir)
return err;
}
-int lirc_register_driver(struct lirc_driver *d)
+int lirc_register_device(struct lirc_dev *d)
{
struct irctl *ir;
int minor;
@@ -199,9 +199,9 @@ int lirc_register_driver(struct lirc_driver *d)
return 0;
}
-EXPORT_SYMBOL(lirc_register_driver);
+EXPORT_SYMBOL(lirc_register_device);
-void lirc_unregister_driver(struct lirc_driver *d)
+void lirc_unregister_device(struct lirc_dev *d)
{
struct irctl *ir;
@@ -229,7 +229,7 @@ void lirc_unregister_driver(struct lirc_driver *d)
ida_simple_remove(&lirc_ida, d->minor);
put_device(&ir->dev);
}
-EXPORT_SYMBOL(lirc_unregister_driver);
+EXPORT_SYMBOL(lirc_unregister_device);
int lirc_dev_fop_open(struct inode *inode, struct file *file)
{
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index b3e7cac2c3ee..f5ececd06e94 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -105,7 +105,7 @@ struct ir_raw_event_ctrl {
} mce_kbd;
struct lirc_codec {
struct rc_dev *dev;
- struct lirc_driver *drv;
+ struct lirc_dev *ldev;
int carrier_low;
ktime_t gap_start;
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index c6a2fe2ad210..a8aefd033ad9 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -100,7 +100,7 @@ struct IR {
struct list_head list;
/* FIXME spinlock access to l.features */
- struct lirc_driver l;
+ struct lirc_dev l;
struct lirc_buffer rbuf;
struct mutex ir_lock;
@@ -183,7 +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
*/
- lirc_unregister_driver(&ir->l);
+ lirc_unregister_device(&ir->l);
if (kfifo_initialized(&ir->rbuf.fifo))
lirc_buffer_free(&ir->rbuf);
@@ -1345,7 +1345,7 @@ static const struct file_operations lirc_fops = {
.release = close
};
-static struct lirc_driver lirc_template = {
+static struct lirc_dev lirc_template = {
.name = "lirc_zilog",
.code_length = 13,
.fops = &lirc_fops,
@@ -1441,7 +1441,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
spin_lock_init(&ir->rx_ref_lock);
/* set lirc_dev stuff */
- memcpy(&ir->l, &lirc_template, sizeof(struct lirc_driver));
+ memcpy(&ir->l, &lirc_template, sizeof(struct lirc_dev));
/*
* FIXME this is a pointer reference to us, but no refcount.
*
@@ -1559,10 +1559,10 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
}
/* register with lirc */
- ret = lirc_register_driver(&ir->l);
+ ret = lirc_register_device(&ir->l);
if (ret < 0) {
dev_err(tx->ir->l.dev,
- "%s: lirc_register_driver() failed: %i\n",
+ "%s: lirc_register_device() failed: %i\n",
__func__, ret);
goto out_put_xx;
}
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 2629c40e8a39..567959e9524f 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -111,48 +111,28 @@ 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: the minor device (/dev/lircX) number for a registered
- * driver.
- *
- * @code_length: length of the remote control key code expressed in bits.
+ * struct lirc_dev - represents a LIRC device
*
+ * @name: used for logging
+ * @minor: the minor device (/dev/lircX) number for the device
+ * @code_length: length of a remote control key code expressed in bits
* @features: lirc compatible hardware features, like LIRC_MODE_RAW,
* LIRC_CAN\_\*, as defined at include/media/lirc.h.
- *
* @buffer_size: Number of FIFO buffers with @chunk_size size.
* Only used if @rbuf is NULL.
- *
* @chunk_size: Size of each FIFO buffer.
* Only used if @rbuf is NULL.
- *
- * @data: it may point to any driver data and this pointer will
- * be passed to all callback functions.
- *
+ * @data: private per-driver data
* @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).
- *
- * @rdev: Pointed to struct rc_dev associated with the LIRC
- * device.
- *
- * @fops: file_operations for drivers which don't fit the current
- * driver model.
- * Some ioctl's can be directly handled by lirc_dev if the
- * driver's ioctl function is NULL or if it returns
- * -ENOIOCTLCMD (see also lirc_serial.c).
- *
- * @dev: pointer to the struct device associated with the LIRC
- * device.
- *
+ * @rdev: &struct rc_dev associated with the device
+ * @fops: &struct file_operations for the device
+ * @dev: &struct device assigned to the device
* @owner: the module owning this struct
- *
- * @irctl: pointer to the struct irctl assigned to this LIRC device.
+ * @irctl: &struct irctl assigned to the device
*/
-struct lirc_driver {
+struct lirc_dev {
char name[40];
unsigned minor;
__u32 code_length;
@@ -175,14 +155,14 @@ struct lirc_driver {
* returns negative value on error or zero
* contents of the structure pointed by p is copied
*/
-extern int lirc_register_driver(struct lirc_driver *d);
+extern int lirc_register_device(struct lirc_dev *d);
-extern void lirc_unregister_driver(struct lirc_driver *d);
+extern void lirc_unregister_device(struct lirc_dev *d);
/* Must be called in the open fop before lirc_get_pdata() can be used */
void lirc_init_pdata(struct inode *inode, struct file *file);
-/* Returns the private data stored in the lirc_driver
+/* Returns the private data stored in the lirc_dev
* associated with the given device file pointer.
*/
void *lirc_get_pdata(struct file *file);
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 12/19] lirc_dev: introduce lirc_allocate_device and lirc_free_device
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (10 preceding siblings ...)
2017-06-25 12:32 ` [PATCH 11/19] lirc_dev: rename struct lirc_driver to struct lirc_dev David Härdeman
@ 2017-06-25 12:32 ` David Härdeman
2017-06-25 12:32 ` [PATCH 13/19] lirc_dev: remove the BUFLEN define David Härdeman
` (6 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:32 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Introduce two new functions so that the API for lirc_dev matches that
of the rc-core and input subsystems.
This means that lirc_dev structs are managed using the usual four functions:
lirc_allocate_device
lirc_free_device
lirc_register_device
lirc_unregister_device
The functions are pretty simplistic at this point, later patches will put
more flesh on the bones of both.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ir-lirc-codec.c | 2 +-
drivers/media/rc/lirc_dev.c | 13 +++++++++++++
include/media/lirc_dev.h | 9 ++++-----
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 2349630ed383..f276c4f56fb5 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -347,7 +347,7 @@ static int ir_lirc_register(struct rc_dev *dev)
int rc = -ENOMEM;
unsigned long features = 0;
- ldev = kzalloc(sizeof(struct lirc_dev), GFP_KERNEL);
+ ldev = lirc_allocate_device();
if (!ldev)
return rc;
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 1c2f1a07bdaa..d107ed6b634b 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -101,6 +101,19 @@ static int lirc_allocate_buffer(struct irctl *ir)
return err;
}
+struct lirc_dev *
+lirc_allocate_device(void)
+{
+ return kzalloc(sizeof(struct lirc_dev), GFP_KERNEL);
+}
+EXPORT_SYMBOL(lirc_allocate_device);
+
+void lirc_free_device(struct lirc_dev *d)
+{
+ kfree(d);
+}
+EXPORT_SYMBOL(lirc_free_device);
+
int lirc_register_device(struct lirc_dev *d)
{
struct irctl *ir;
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 567959e9524f..3f8edabfef88 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -150,11 +150,10 @@ struct lirc_dev {
struct irctl *irctl;
};
-/* following functions can be called ONLY from user context
- *
- * returns negative value on error or zero
- * contents of the structure pointed by p is copied
- */
+extern struct lirc_dev *lirc_allocate_device(void);
+
+extern void lirc_free_device(struct lirc_dev *d);
+
extern int lirc_register_device(struct lirc_dev *d);
extern void lirc_unregister_device(struct lirc_dev *d);
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 13/19] lirc_dev: remove the BUFLEN define
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (11 preceding siblings ...)
2017-06-25 12:32 ` [PATCH 12/19] lirc_dev: introduce lirc_allocate_device and lirc_free_device David Härdeman
@ 2017-06-25 12:32 ` David Härdeman
2017-06-25 12:32 ` [PATCH 14/19] lirc_zilog: add a pointer to the parent device to struct IR David Härdeman
` (5 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:32 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
The define is only used in the lirc_zilog driver and once in lirc_dev.
In lirc_dev it rather serves to make the limits on d->code_length less clear,
so move the define to lirc_zilog.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/lirc_dev.c | 5 ++---
drivers/staging/media/lirc/lirc_zilog.c | 3 +++
include/media/lirc_dev.h | 2 --
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index d107ed6b634b..80944c2f7e91 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -145,9 +145,8 @@ int lirc_register_device(struct lirc_dev *d)
return -EINVAL;
}
- if (d->code_length < 1 || d->code_length > (BUFLEN * 8)) {
- dev_err(d->dev, "code length must be less than %d bits\n",
- BUFLEN * 8);
+ if (d->code_length < 1 || d->code_length > 128) {
+ dev_err(d->dev, "invalid code_length!\n");
return -EBADRQC;
}
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index a8aefd033ad9..f54b66de4a27 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -64,6 +64,9 @@
/* Max transfer size done by I2C transfer functions */
#define MAX_XFER_SIZE 64
+/* LIRC buffer size */
+#define BUFLEN 16
+
struct IR;
struct IR_rx {
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 3f8edabfef88..21aac9494678 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -9,8 +9,6 @@
#ifndef _LINUX_LIRC_DEV_H
#define _LINUX_LIRC_DEV_H
-#define BUFLEN 16
-
#include <linux/slab.h>
#include <linux/fs.h>
#include <linux/ioctl.h>
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 14/19] lirc_zilog: add a pointer to the parent device to struct IR
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (12 preceding siblings ...)
2017-06-25 12:32 ` [PATCH 13/19] lirc_dev: remove the BUFLEN define David Härdeman
@ 2017-06-25 12:32 ` David Härdeman
2017-06-25 12:32 ` [PATCH 15/19] lirc_zilog: use a dynamically allocated lirc_dev David Härdeman
` (4 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:32 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
lirc_zilog stashes a pointer to the parent device in struct lirc_dev and uses
it for logging. It makes more sense to let lirc_zilog keep track of that
pointer in its own struct (this is in preparation for subsequent patches
which will remodel struct lirc_dev).
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/staging/media/lirc/lirc_zilog.c | 98 ++++++++++++++++---------------
1 file changed, 50 insertions(+), 48 deletions(-)
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index f54b66de4a27..51512ba7f5b8 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -109,6 +109,7 @@ struct IR {
struct mutex ir_lock;
atomic_t open_count;
+ struct device *dev;
struct i2c_adapter *adapter;
spinlock_t rx_ref_lock; /* struct IR_rx kref get()/put() */
@@ -322,7 +323,7 @@ static int add_to_buf(struct IR *ir)
struct IR_tx *tx;
if (lirc_buffer_full(rbuf)) {
- dev_dbg(ir->l.dev, "buffer overflow\n");
+ dev_dbg(ir->dev, "buffer overflow\n");
return -EOVERFLOW;
}
@@ -368,17 +369,17 @@ static int add_to_buf(struct IR *ir)
*/
ret = i2c_master_send(rx->c, sendbuf, 1);
if (ret != 1) {
- dev_err(ir->l.dev, "i2c_master_send failed with %d\n",
+ dev_err(ir->dev, "i2c_master_send failed with %d\n",
ret);
if (failures >= 3) {
mutex_unlock(&ir->ir_lock);
- dev_err(ir->l.dev,
+ dev_err(ir->dev,
"unable to read from the IR chip after 3 resets, giving up\n");
break;
}
/* Looks like the chip crashed, reset it */
- dev_err(ir->l.dev,
+ dev_err(ir->dev,
"polling the IR receiver chip failed, trying reset\n");
set_current_state(TASK_UNINTERRUPTIBLE);
@@ -405,14 +406,14 @@ static int add_to_buf(struct IR *ir)
ret = i2c_master_recv(rx->c, keybuf, sizeof(keybuf));
mutex_unlock(&ir->ir_lock);
if (ret != sizeof(keybuf)) {
- dev_err(ir->l.dev,
+ dev_err(ir->dev,
"i2c_master_recv failed with %d -- keeping last read buffer\n",
ret);
} else {
rx->b[0] = keybuf[3];
rx->b[1] = keybuf[4];
rx->b[2] = keybuf[5];
- dev_dbg(ir->l.dev,
+ dev_dbg(ir->dev,
"key (0x%02x/0x%02x)\n",
rx->b[0], rx->b[1]);
}
@@ -465,7 +466,7 @@ static int lirc_thread(void *arg)
struct IR *ir = arg;
struct lirc_buffer *rbuf = ir->l.rbuf;
- dev_dbg(ir->l.dev, "poll thread started\n");
+ dev_dbg(ir->dev, "poll thread started\n");
while (!kthread_should_stop()) {
set_current_state(TASK_INTERRUPTIBLE);
@@ -493,7 +494,7 @@ static int lirc_thread(void *arg)
wake_up_interruptible(&rbuf->wait_poll);
}
- dev_dbg(ir->l.dev, "poll thread ended\n");
+ dev_dbg(ir->dev, "poll thread ended\n");
return 0;
}
@@ -646,10 +647,10 @@ static int send_data_block(struct IR_tx *tx, unsigned char *data_block)
buf[0] = (unsigned char)(i + 1);
for (j = 0; j < tosend; ++j)
buf[1 + j] = data_block[i + j];
- dev_dbg(tx->ir->l.dev, "%*ph", 5, buf);
+ dev_dbg(tx->ir->dev, "%*ph", 5, buf);
ret = i2c_master_send(tx->c, buf, tosend + 1);
if (ret != tosend + 1) {
- dev_err(tx->ir->l.dev,
+ dev_err(tx->ir->dev,
"i2c_master_send failed with %d\n", ret);
return ret < 0 ? ret : -EFAULT;
}
@@ -674,7 +675,7 @@ static int send_boot_data(struct IR_tx *tx)
buf[1] = 0x20;
ret = i2c_master_send(tx->c, buf, 2);
if (ret != 2) {
- dev_err(tx->ir->l.dev, "i2c_master_send failed with %d\n", ret);
+ dev_err(tx->ir->dev, "i2c_master_send failed with %d\n", ret);
return ret < 0 ? ret : -EFAULT;
}
@@ -691,22 +692,22 @@ static int send_boot_data(struct IR_tx *tx)
}
if (ret != 1) {
- dev_err(tx->ir->l.dev, "i2c_master_send failed with %d\n", ret);
+ dev_err(tx->ir->dev, "i2c_master_send failed with %d\n", ret);
return ret < 0 ? ret : -EFAULT;
}
/* Here comes the firmware version... (hopefully) */
ret = i2c_master_recv(tx->c, buf, 4);
if (ret != 4) {
- dev_err(tx->ir->l.dev, "i2c_master_recv failed with %d\n", ret);
+ dev_err(tx->ir->dev, "i2c_master_recv failed with %d\n", ret);
return 0;
}
if ((buf[0] != 0x80) && (buf[0] != 0xa0)) {
- dev_err(tx->ir->l.dev, "unexpected IR TX init response: %02x\n",
+ dev_err(tx->ir->dev, "unexpected IR TX init response: %02x\n",
buf[0]);
return 0;
}
- dev_notice(tx->ir->l.dev,
+ dev_notice(tx->ir->dev,
"Zilog/Hauppauge IR blaster firmware version %d.%d.%d loaded\n",
buf[1], buf[2], buf[3]);
@@ -751,15 +752,15 @@ static int fw_load(struct IR_tx *tx)
}
/* Request codeset data file */
- ret = request_firmware(&fw_entry, "haup-ir-blaster.bin", tx->ir->l.dev);
+ ret = request_firmware(&fw_entry, "haup-ir-blaster.bin", tx->ir->dev);
if (ret != 0) {
- dev_err(tx->ir->l.dev,
+ dev_err(tx->ir->dev,
"firmware haup-ir-blaster.bin not available (%d)\n",
ret);
ret = ret < 0 ? ret : -EFAULT;
goto out;
}
- dev_dbg(tx->ir->l.dev, "firmware of size %zu loaded\n", fw_entry->size);
+ dev_dbg(tx->ir->dev, "firmware of size %zu loaded\n", fw_entry->size);
/* Parse the file */
tx_data = vmalloc(sizeof(*tx_data));
@@ -787,7 +788,7 @@ static int fw_load(struct IR_tx *tx)
if (!read_uint8(&data, tx_data->endp, &version))
goto corrupt;
if (version != 1) {
- dev_err(tx->ir->l.dev,
+ dev_err(tx->ir->dev,
"unsupported code set file version (%u, expected 1) -- please upgrade to a newer driver\n",
version);
fw_unload_locked();
@@ -804,7 +805,7 @@ static int fw_load(struct IR_tx *tx)
&tx_data->num_code_sets))
goto corrupt;
- dev_dbg(tx->ir->l.dev, "%u IR blaster codesets loaded\n",
+ dev_dbg(tx->ir->dev, "%u IR blaster codesets loaded\n",
tx_data->num_code_sets);
tx_data->code_sets = vmalloc(
@@ -869,7 +870,7 @@ static int fw_load(struct IR_tx *tx)
goto out;
corrupt:
- dev_err(tx->ir->l.dev, "firmware is corrupt\n");
+ dev_err(tx->ir->dev, "firmware is corrupt\n");
fw_unload_locked();
ret = -EFAULT;
@@ -889,9 +890,9 @@ static ssize_t read(struct file *filep, char __user *outbuf, size_t n,
unsigned int m;
DECLARE_WAITQUEUE(wait, current);
- dev_dbg(ir->l.dev, "read called\n");
+ dev_dbg(ir->dev, "read called\n");
if (n % rbuf->chunk_size) {
- dev_dbg(ir->l.dev, "read result = -EINVAL\n");
+ dev_dbg(ir->dev, "read result = -EINVAL\n");
return -EINVAL;
}
@@ -935,7 +936,7 @@ static ssize_t read(struct file *filep, char __user *outbuf, size_t n,
unsigned char buf[MAX_XFER_SIZE];
if (rbuf->chunk_size > sizeof(buf)) {
- dev_err(ir->l.dev,
+ dev_err(ir->dev,
"chunk_size is too big (%d)!\n",
rbuf->chunk_size);
ret = -EINVAL;
@@ -950,7 +951,7 @@ static ssize_t read(struct file *filep, char __user *outbuf, size_t n,
retries++;
}
if (retries >= 5) {
- dev_err(ir->l.dev, "Buffer read failed!\n");
+ dev_err(ir->dev, "Buffer read failed!\n");
ret = -EIO;
}
}
@@ -960,7 +961,7 @@ static ssize_t read(struct file *filep, char __user *outbuf, size_t n,
put_ir_rx(rx, false);
set_current_state(TASK_RUNNING);
- dev_dbg(ir->l.dev, "read result = %d (%s)\n", ret,
+ dev_dbg(ir->dev, "read result = %d (%s)\n", ret,
ret ? "Error" : "OK");
return ret ? ret : written;
@@ -977,7 +978,7 @@ static int send_code(struct IR_tx *tx, unsigned int code, unsigned int key)
ret = get_key_data(data_block, code, key);
if (ret == -EPROTO) {
- dev_err(tx->ir->l.dev,
+ dev_err(tx->ir->dev,
"failed to get data for code %u, key %u -- check lircd.conf entries\n",
code, key);
return ret;
@@ -995,7 +996,7 @@ static int send_code(struct IR_tx *tx, unsigned int code, unsigned int key)
buf[1] = 0x40;
ret = i2c_master_send(tx->c, buf, 2);
if (ret != 2) {
- dev_err(tx->ir->l.dev, "i2c_master_send failed with %d\n", ret);
+ dev_err(tx->ir->dev, "i2c_master_send failed with %d\n", ret);
return ret < 0 ? ret : -EFAULT;
}
@@ -1008,18 +1009,18 @@ static int send_code(struct IR_tx *tx, unsigned int code, unsigned int key)
}
if (ret != 1) {
- dev_err(tx->ir->l.dev, "i2c_master_send failed with %d\n", ret);
+ dev_err(tx->ir->dev, "i2c_master_send failed with %d\n", ret);
return ret < 0 ? ret : -EFAULT;
}
/* Send finished download? */
ret = i2c_master_recv(tx->c, buf, 1);
if (ret != 1) {
- dev_err(tx->ir->l.dev, "i2c_master_recv failed with %d\n", ret);
+ dev_err(tx->ir->dev, "i2c_master_recv failed with %d\n", ret);
return ret < 0 ? ret : -EFAULT;
}
if (buf[0] != 0xA0) {
- dev_err(tx->ir->l.dev, "unexpected IR TX response #1: %02x\n",
+ dev_err(tx->ir->dev, "unexpected IR TX response #1: %02x\n",
buf[0]);
return -EFAULT;
}
@@ -1029,7 +1030,7 @@ static int send_code(struct IR_tx *tx, unsigned int code, unsigned int key)
buf[1] = 0x80;
ret = i2c_master_send(tx->c, buf, 2);
if (ret != 2) {
- dev_err(tx->ir->l.dev, "i2c_master_send failed with %d\n", ret);
+ dev_err(tx->ir->dev, "i2c_master_send failed with %d\n", ret);
return ret < 0 ? ret : -EFAULT;
}
@@ -1039,7 +1040,7 @@ static int send_code(struct IR_tx *tx, unsigned int code, unsigned int key)
* going to skip this whole mess and say we're done on the HD PVR
*/
if (!tx->post_tx_ready_poll) {
- dev_dbg(tx->ir->l.dev, "sent code %u, key %u\n", code, key);
+ dev_dbg(tx->ir->dev, "sent code %u, key %u\n", code, key);
return 0;
}
@@ -1055,12 +1056,12 @@ static int send_code(struct IR_tx *tx, unsigned int code, unsigned int key)
ret = i2c_master_send(tx->c, buf, 1);
if (ret == 1)
break;
- dev_dbg(tx->ir->l.dev,
+ dev_dbg(tx->ir->dev,
"NAK expected: i2c_master_send failed with %d (try %d)\n",
ret, i + 1);
}
if (ret != 1) {
- dev_err(tx->ir->l.dev,
+ dev_err(tx->ir->dev,
"IR TX chip never got ready: last i2c_master_send failed with %d\n",
ret);
return ret < 0 ? ret : -EFAULT;
@@ -1069,17 +1070,17 @@ static int send_code(struct IR_tx *tx, unsigned int code, unsigned int key)
/* Seems to be an 'ok' response */
i = i2c_master_recv(tx->c, buf, 1);
if (i != 1) {
- dev_err(tx->ir->l.dev, "i2c_master_recv failed with %d\n", ret);
+ dev_err(tx->ir->dev, "i2c_master_recv failed with %d\n", ret);
return -EFAULT;
}
if (buf[0] != 0x80) {
- dev_err(tx->ir->l.dev, "unexpected IR TX response #2: %02x\n",
+ dev_err(tx->ir->dev, "unexpected IR TX response #2: %02x\n",
buf[0]);
return -EFAULT;
}
/* Oh good, it worked */
- dev_dbg(tx->ir->l.dev, "sent code %u, key %u\n", code, key);
+ dev_dbg(tx->ir->dev, "sent code %u, key %u\n", code, key);
return 0;
}
@@ -1165,11 +1166,11 @@ static ssize_t write(struct file *filep, const char __user *buf, size_t n,
*/
if (ret != 0) {
/* Looks like the chip crashed, reset it */
- dev_err(tx->ir->l.dev,
+ dev_err(tx->ir->dev,
"sending to the IR transmitter chip failed, trying reset\n");
if (failures >= 3) {
- dev_err(tx->ir->l.dev,
+ dev_err(tx->ir->dev,
"unable to send to the IR chip after 3 resets, giving up\n");
mutex_unlock(&ir->ir_lock);
mutex_unlock(&tx->client_lock);
@@ -1205,7 +1206,7 @@ static unsigned int poll(struct file *filep, poll_table *wait)
struct lirc_buffer *rbuf = ir->l.rbuf;
unsigned int ret;
- dev_dbg(ir->l.dev, "%s called\n", __func__);
+ dev_dbg(ir->dev, "%s called\n", __func__);
rx = get_ir_rx(ir);
if (!rx) {
@@ -1213,7 +1214,7 @@ static unsigned int poll(struct file *filep, poll_table *wait)
* Revisit this, if our poll function ever reports writeable
* status for Tx
*/
- dev_dbg(ir->l.dev, "%s result = POLLERR\n", __func__);
+ dev_dbg(ir->dev, "%s result = POLLERR\n", __func__);
return POLLERR;
}
@@ -1226,7 +1227,7 @@ static unsigned int poll(struct file *filep, poll_table *wait)
/* Indicate what ops could happen immediately without blocking */
ret = lirc_buffer_empty(rbuf) ? 0 : (POLLIN | POLLRDNORM);
- dev_dbg(ir->l.dev, "%s result = %s\n", __func__,
+ dev_dbg(ir->dev, "%s result = %s\n", __func__,
ret ? "POLLIN|POLLRDNORM" : "none");
return ret;
}
@@ -1438,6 +1439,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
list_add_tail(&ir->list, &ir_devices_list);
ir->adapter = adap;
+ ir->dev = &adap->dev;
mutex_init(&ir->ir_lock);
atomic_set(&ir->open_count, 0);
spin_lock_init(&ir->tx_ref_lock);
@@ -1501,7 +1503,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
/* Proceed only if the Rx client is also ready or not needed */
if (!rx && !tx_only) {
- dev_info(tx->ir->l.dev,
+ dev_info(tx->ir->dev,
"probe of IR Tx on %s (i2c-%d) done. Waiting on IR Rx.\n",
adap->name, adap->nr);
goto out_ok;
@@ -1541,7 +1543,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
"zilog-rx-i2c-%d", adap->nr);
if (IS_ERR(rx->task)) {
ret = PTR_ERR(rx->task);
- dev_err(tx->ir->l.dev,
+ dev_err(tx->ir->dev,
"%s: could not start IR Rx polling thread\n",
__func__);
/* Failed kthread, so put back the ir ref */
@@ -1564,13 +1566,13 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
/* register with lirc */
ret = lirc_register_device(&ir->l);
if (ret < 0) {
- dev_err(tx->ir->l.dev,
+ dev_err(tx->ir->dev,
"%s: lirc_register_device() failed: %i\n",
__func__, ret);
goto out_put_xx;
}
- dev_info(ir->l.dev,
+ dev_info(ir->dev,
"IR unit on %s (i2c-%d) registered as lirc%d and ready\n",
adap->name, adap->nr, ir->l.minor);
@@ -1580,7 +1582,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (tx)
put_ir_tx(tx, true);
put_ir_device(ir, true);
- dev_info(ir->l.dev,
+ dev_info(ir->dev,
"probe of IR %s on %s (i2c-%d) done\n",
tx_probe ? "Tx" : "Rx", adap->name, adap->nr);
mutex_unlock(&ir_devices_lock);
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 15/19] lirc_zilog: use a dynamically allocated lirc_dev
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (13 preceding siblings ...)
2017-06-25 12:32 ` [PATCH 14/19] lirc_zilog: add a pointer to the parent device to struct IR David Härdeman
@ 2017-06-25 12:32 ` David Härdeman
2017-06-25 12:32 ` [PATCH 16/19] lirc_dev: merge struct irctl into struct lirc_dev David Härdeman
` (3 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:32 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
lirc_zilog currently embeds a struct lirc_dev in its own struct IR, but
subsequent patches will make the lifetime of struct lirc_dev dynamic (i.e.
it will be free():d once lirc_dev is sure there are no users of the struct).
Therefore, change lirc_zilog to use a pointer to a dynamically allocated
struct lirc_dev.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/staging/media/lirc/lirc_zilog.c | 69 ++++++++++++++++++-------------
1 file changed, 40 insertions(+), 29 deletions(-)
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index 51512ba7f5b8..bbbba25ae574 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -102,8 +102,8 @@ struct IR {
struct kref ref;
struct list_head list;
- /* FIXME spinlock access to l.features */
- struct lirc_dev l;
+ /* FIXME spinlock access to l->features */
+ struct lirc_dev *l;
struct lirc_buffer rbuf;
struct mutex ir_lock;
@@ -187,7 +187,10 @@ static void release_ir_device(struct kref *ref)
* ir->open_count == 0 - happens on final close()
* ir_lock, tx_ref_lock, rx_ref_lock, all released
*/
- lirc_unregister_device(&ir->l);
+ if (ir->l) {
+ lirc_unregister_device(ir->l);
+ lirc_free_device(ir->l);
+ }
if (kfifo_initialized(&ir->rbuf.fifo))
lirc_buffer_free(&ir->rbuf);
@@ -244,7 +247,7 @@ static void release_ir_rx(struct kref *ref)
* and releasing the ir reference can cause a sleep. That work is
* performed by put_ir_rx()
*/
- ir->l.features &= ~LIRC_CAN_REC_LIRCCODE;
+ ir->l->features &= ~LIRC_CAN_REC_LIRCCODE;
/* Don't put_ir_device(rx->ir) here; lock can't be freed yet */
ir->rx = NULL;
/* Don't do the kfree(rx) here; we still need to kill the poll thread */
@@ -289,7 +292,7 @@ static void release_ir_tx(struct kref *ref)
struct IR_tx *tx = container_of(ref, struct IR_tx, ref);
struct IR *ir = tx->ir;
- ir->l.features &= ~LIRC_CAN_SEND_PULSE;
+ ir->l->features &= ~LIRC_CAN_SEND_PULSE;
/* Don't put_ir_device(tx->ir) here, so our lock doesn't get freed */
ir->tx = NULL;
kfree(tx);
@@ -318,7 +321,7 @@ static int add_to_buf(struct IR *ir)
int ret;
int failures = 0;
unsigned char sendbuf[1] = { 0 };
- struct lirc_buffer *rbuf = ir->l.rbuf;
+ struct lirc_buffer *rbuf = ir->l->rbuf;
struct IR_rx *rx;
struct IR_tx *tx;
@@ -464,7 +467,7 @@ static int add_to_buf(struct IR *ir)
static int lirc_thread(void *arg)
{
struct IR *ir = arg;
- struct lirc_buffer *rbuf = ir->l.rbuf;
+ struct lirc_buffer *rbuf = ir->l->rbuf;
dev_dbg(ir->dev, "poll thread started\n");
@@ -885,7 +888,7 @@ static ssize_t read(struct file *filep, char __user *outbuf, size_t n,
{
struct IR *ir = lirc_get_pdata(filep);
struct IR_rx *rx;
- struct lirc_buffer *rbuf = ir->l.rbuf;
+ struct lirc_buffer *rbuf = ir->l->rbuf;
int ret = 0, written = 0, retries = 0;
unsigned int m;
DECLARE_WAITQUEUE(wait, current);
@@ -1203,7 +1206,7 @@ static unsigned int poll(struct file *filep, poll_table *wait)
{
struct IR *ir = lirc_get_pdata(filep);
struct IR_rx *rx;
- struct lirc_buffer *rbuf = ir->l.rbuf;
+ struct lirc_buffer *rbuf = ir->l->rbuf;
unsigned int ret;
dev_dbg(ir->dev, "%s called\n", __func__);
@@ -1239,7 +1242,7 @@ static long ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
int result;
unsigned long mode, features;
- features = ir->l.features;
+ features = ir->l->features;
switch (cmd) {
case LIRC_GET_LENGTH:
@@ -1349,13 +1352,6 @@ static const struct file_operations lirc_fops = {
.release = close
};
-static struct lirc_dev lirc_template = {
- .name = "lirc_zilog",
- .code_length = 13,
- .fops = &lirc_fops,
- .owner = THIS_MODULE,
-};
-
static int ir_remove(struct i2c_client *client)
{
if (strncmp("ir_tx_z8", client->name, 8) == 0) {
@@ -1446,22 +1442,35 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
spin_lock_init(&ir->rx_ref_lock);
/* set lirc_dev stuff */
- memcpy(&ir->l, &lirc_template, sizeof(struct lirc_dev));
+ ir->l = lirc_allocate_device();
+ if (!ir->l) {
+ ret = -ENOMEM;
+ goto out_put_ir;
+ }
+
+ snprintf(ir->l->name, sizeof(ir->l->name), "lirc_zilog");
+ ir->l->code_length = 13;
+ ir->l->fops = &lirc_fops;
+ ir->l->owner = THIS_MODULE;
+
/*
* FIXME this is a pointer reference to us, but no refcount.
*
* This OK for now, since lirc_dev currently won't touch this
* buffer as we provide our own lirc_fops.
*
- * Currently our own lirc_fops rely on this ir->l.rbuf pointer
+ * Currently our own lirc_fops rely on this ir->l->rbuf pointer
*/
- ir->l.rbuf = &ir->rbuf;
- ir->l.dev = &adap->dev;
+ ir->l->rbuf = &ir->rbuf;
+ ir->l->dev = &adap->dev;
/* This will be returned by lirc_get_pdata() */
- ir->l.data = ir;
- ret = lirc_buffer_init(ir->l.rbuf, 2, BUFLEN / 2);
- if (ret)
+ ir->l->data = ir;
+ ret = lirc_buffer_init(ir->l->rbuf, 2, BUFLEN / 2);
+ if (ret) {
+ lirc_free_device(ir->l);
+ ir->l = NULL;
goto out_put_ir;
+ }
}
if (tx_probe) {
@@ -1477,7 +1486,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
kref_init(&tx->ref);
ir->tx = tx;
- ir->l.features |= LIRC_CAN_SEND_PULSE;
+ ir->l->features |= LIRC_CAN_SEND_PULSE;
mutex_init(&tx->client_lock);
tx->c = client;
tx->need_boot = 1;
@@ -1521,7 +1530,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
kref_init(&rx->ref);
ir->rx = rx;
- ir->l.features |= LIRC_CAN_REC_LIRCCODE;
+ ir->l->features |= LIRC_CAN_REC_LIRCCODE;
mutex_init(&rx->client_lock);
rx->c = client;
rx->hdpvr_data_fmt =
@@ -1551,7 +1560,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
/* Failure exit, so put back rx ref from i2c_client */
i2c_set_clientdata(client, NULL);
put_ir_rx(rx, true);
- ir->l.features &= ~LIRC_CAN_REC_LIRCCODE;
+ ir->l->features &= ~LIRC_CAN_REC_LIRCCODE;
goto out_put_tx;
}
@@ -1564,17 +1573,19 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
}
/* register with lirc */
- ret = lirc_register_device(&ir->l);
+ ret = lirc_register_device(ir->l);
if (ret < 0) {
dev_err(tx->ir->dev,
"%s: lirc_register_device() failed: %i\n",
__func__, ret);
+ lirc_free_device(ir->l);
+ ir->l = NULL;
goto out_put_xx;
}
dev_info(ir->dev,
"IR unit on %s (i2c-%d) registered as lirc%d and ready\n",
- adap->name, adap->nr, ir->l.minor);
+ adap->name, adap->nr, ir->l->minor);
out_ok:
if (rx)
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 16/19] lirc_dev: merge struct irctl into struct lirc_dev
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (14 preceding siblings ...)
2017-06-25 12:32 ` [PATCH 15/19] lirc_zilog: use a dynamically allocated lirc_dev David Härdeman
@ 2017-06-25 12:32 ` David Härdeman
2017-06-25 12:32 ` [PATCH 17/19] ir-lirc-codec: merge lirc_dev_fop_ioctl into ir_lirc_ioctl David Härdeman
` (2 subsequent siblings)
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:32 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
The use of two separate structs (lirc_dev aka lirc_driver and irctl) makes
it much harder to follow the proper lifetime of the various structs and
necessitates hacks such as keeping a copy of struct lirc_dev inside
struct irctl.
Merging the two structs means that lirc_dev can properly manage the lifetime
of the resulting struct and simplifies the code at the same time.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ir-lirc-codec.c | 15 +-
drivers/media/rc/lirc_dev.c | 288 +++++++++++++------------------
drivers/staging/media/lirc/lirc_zilog.c | 20 +-
include/media/lirc_dev.h | 26 ++-
4 files changed, 161 insertions(+), 188 deletions(-)
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index f276c4f56fb5..ba20a4ce9cbc 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -35,7 +35,7 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
struct lirc_codec *lirc = &dev->raw->lirc;
int sample;
- if (!dev->raw->lirc.ldev || !dev->raw->lirc.ldev->rbuf)
+ if (!dev->raw->lirc.ldev || !dev->raw->lirc.ldev->buf)
return -EINVAL;
/* Packet start */
@@ -84,7 +84,7 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
(u64)LIRC_VALUE_MASK);
gap_sample = LIRC_SPACE(lirc->gap_duration);
- lirc_buffer_write(dev->raw->lirc.ldev->rbuf,
+ lirc_buffer_write(dev->raw->lirc.ldev->buf,
(unsigned char *) &gap_sample);
lirc->gap = false;
}
@@ -95,9 +95,9 @@ static int ir_lirc_decode(struct rc_dev *dev, struct ir_raw_event ev)
TO_US(ev.duration), TO_STR(ev.pulse));
}
- lirc_buffer_write(dev->raw->lirc.ldev->rbuf,
+ lirc_buffer_write(dev->raw->lirc.ldev->buf,
(unsigned char *) &sample);
- wake_up(&dev->raw->lirc.ldev->rbuf->wait_poll);
+ wake_up(&dev->raw->lirc.ldev->buf->wait_poll);
return 0;
}
@@ -384,12 +384,12 @@ static int ir_lirc_register(struct rc_dev *dev)
dev->driver_name);
ldev->features = features;
ldev->data = &dev->raw->lirc;
- ldev->rbuf = NULL;
+ ldev->buf = NULL;
ldev->code_length = sizeof(struct ir_raw_event) * 8;
ldev->chunk_size = sizeof(int);
ldev->buffer_size = LIRCBUF_SIZE;
ldev->fops = &lirc_fops;
- ldev->dev = &dev->dev;
+ ldev->dev.parent = &dev->dev;
ldev->rdev = dev;
ldev->owner = THIS_MODULE;
@@ -402,7 +402,7 @@ static int ir_lirc_register(struct rc_dev *dev)
return 0;
out:
- kfree(ldev);
+ lirc_free_device(ldev);
return rc;
}
@@ -411,7 +411,6 @@ static int ir_lirc_unregister(struct rc_dev *dev)
struct lirc_codec *lirc = &dev->raw->lirc;
lirc_unregister_device(lirc->ldev);
- kfree(lirc->ldev);
lirc->ldev = NULL;
return 0;
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 80944c2f7e91..4ad08d3d4e2f 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -34,19 +34,6 @@
static dev_t lirc_base_dev;
-struct irctl {
- struct lirc_dev d;
- bool attached;
- int open;
-
- struct mutex mutex;
- struct lirc_buffer *buf;
- bool buf_internal;
-
- struct device dev;
- struct cdev cdev;
-};
-
/* Used to keep track of allocated lirc devices */
#define LIRC_MAX_DEVICES 256
static DEFINE_IDA(lirc_ida);
@@ -54,69 +41,72 @@ static DEFINE_IDA(lirc_ida);
/* Only used for sysfs but defined to void otherwise */
static struct class *lirc_class;
-static void lirc_free_buffer(struct irctl *ir)
+static void lirc_release_device(struct device *ld)
{
- if (ir->buf_internal) {
- lirc_buffer_free(ir->buf);
- kfree(ir->buf);
- ir->buf = NULL;
+ struct lirc_dev *d = container_of(ld, struct lirc_dev, dev);
+
+ if (d->buf_internal) {
+ lirc_buffer_free(d->buf);
+ kfree(d->buf);
+ d->buf = NULL;
}
+ kfree(d);
+ module_put(THIS_MODULE);
}
-static void lirc_release(struct device *ld)
+static int lirc_allocate_buffer(struct lirc_dev *d)
{
- struct irctl *ir = container_of(ld, struct irctl, dev);
-
- lirc_free_buffer(ir);
- kfree(ir);
-}
+ int err;
-static int lirc_allocate_buffer(struct irctl *ir)
-{
- int err = 0;
- struct lirc_dev *d = &ir->d;
-
- if (d->rbuf) {
- ir->buf = d->rbuf;
- ir->buf_internal = false;
- } else {
- ir->buf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
- if (!ir->buf) {
- err = -ENOMEM;
- goto out;
- }
+ if (d->buf) {
+ d->buf_internal = false;
+ return 0;
+ }
- err = lirc_buffer_init(ir->buf, d->chunk_size, d->buffer_size);
- if (err) {
- kfree(ir->buf);
- ir->buf = NULL;
- goto out;
- }
+ d->buf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
+ if (!d->buf)
+ return -ENOMEM;
- ir->buf_internal = true;
- d->rbuf = ir->buf;
+ err = lirc_buffer_init(d->buf, d->chunk_size, d->buffer_size);
+ if (err) {
+ kfree(d->buf);
+ d->buf = NULL;
+ return err;
}
-out:
- return err;
+ d->buf_internal = true;
+ return 0;
}
struct lirc_dev *
lirc_allocate_device(void)
{
- return kzalloc(sizeof(struct lirc_dev), GFP_KERNEL);
+ struct lirc_dev *d;
+
+ d = kzalloc(sizeof(struct lirc_dev), GFP_KERNEL);
+ if (d) {
+ mutex_init(&d->mutex);
+ device_initialize(&d->dev);
+ d->dev.class = lirc_class;
+ d->dev.release = lirc_release_device;
+ __module_get(THIS_MODULE);
+ }
+
+ return d;
}
EXPORT_SYMBOL(lirc_allocate_device);
void lirc_free_device(struct lirc_dev *d)
{
- kfree(d);
+ if (!d)
+ return;
+
+ put_device(&d->dev);
}
EXPORT_SYMBOL(lirc_free_device);
int lirc_register_device(struct lirc_dev *d)
{
- struct irctl *ir;
int minor;
int err;
@@ -125,8 +115,8 @@ int lirc_register_device(struct lirc_dev *d)
return -EBADRQC;
}
- if (!d->dev) {
- pr_err("dev pointer not filled in!\n");
+ if (!d->dev.parent) {
+ pr_err("dev parent pointer not filled in!\n");
return -EINVAL;
}
@@ -135,79 +125,58 @@ int lirc_register_device(struct lirc_dev *d)
return -EINVAL;
}
- if (!d->rbuf && d->chunk_size < 1) {
+ if (!d->buf && d->chunk_size < 1) {
pr_err("chunk_size must be set!\n");
return -EINVAL;
}
- if (!d->rbuf && d->buffer_size < 1) {
+ if (!d->buf && d->buffer_size < 1) {
pr_err("buffer_size must be set!\n");
return -EINVAL;
}
if (d->code_length < 1 || d->code_length > 128) {
- dev_err(d->dev, "invalid code_length!\n");
+ dev_err(&d->dev, "invalid code_length!\n");
return -EBADRQC;
}
- if (!d->rbuf && !(d->fops && d->fops->read &&
+ if (!d->buf && !(d->fops && d->fops->read &&
d->fops->poll && d->fops->unlocked_ioctl)) {
- dev_err(d->dev, "undefined read, poll, ioctl\n");
+ dev_err(&d->dev, "undefined read, poll, ioctl\n");
return -EBADRQC;
}
- /* some safety check 8-) */
d->name[sizeof(d->name)-1] = '\0';
if (d->features == 0)
d->features = LIRC_CAN_REC_LIRCCODE;
- ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
- if (!ir)
- return -ENOMEM;
-
- mutex_init(&ir->mutex);
- ir->d = *d;
-
if (LIRC_CAN_REC(d->features)) {
- err = lirc_allocate_buffer(ir);
- if (err) {
- kfree(ir);
+ err = lirc_allocate_buffer(d);
+ if (err)
return err;
- }
- d->rbuf = ir->buf;
}
minor = ida_simple_get(&lirc_ida, 0, LIRC_MAX_DEVICES, GFP_KERNEL);
- if (minor < 0) {
- lirc_free_buffer(ir);
- kfree(ir);
+ if (minor < 0)
return minor;
- }
- d->irctl = ir;
d->minor = minor;
+ d->dev.devt = MKDEV(MAJOR(lirc_base_dev), d->minor);
+ dev_set_name(&d->dev, "lirc%d", d->minor);
- 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);
+ cdev_init(&d->cdev, d->fops);
+ d->cdev.owner = d->owner;
+ d->attached = true;
- cdev_init(&ir->cdev, d->fops);
- ir->cdev.owner = ir->d.owner;
- ir->attached = true;
-
- err = cdev_device_add(&ir->cdev, &ir->dev);
+ err = cdev_device_add(&d->cdev, &d->dev);
if (err) {
ida_simple_remove(&lirc_ida, minor);
- put_device(&ir->dev);
return err;
}
- dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
- ir->d.name, ir->d.minor);
+ dev_info(&d->dev, "lirc_dev: driver %s registered at minor = %d\n",
+ d->name, d->minor);
return 0;
}
@@ -215,88 +184,83 @@ EXPORT_SYMBOL(lirc_register_device);
void lirc_unregister_device(struct lirc_dev *d)
{
- struct irctl *ir;
-
- if (!d || !d->irctl)
+ if (!d)
return;
- ir = d->irctl;
-
- dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
+ dev_dbg(&d->dev, "lirc_dev: driver %s unregistered from minor = %d\n",
d->name, d->minor);
- cdev_device_del(&ir->cdev, &ir->dev);
-
- mutex_lock(&ir->mutex);
+ mutex_lock(&d->mutex);
- ir->attached = false;
- if (ir->open) {
- dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
+ d->attached = false;
+ if (d->open) {
+ dev_dbg(&d->dev, LOGHEAD "releasing opened driver\n",
d->name, d->minor);
- wake_up_interruptible(&ir->buf->wait_poll);
+ wake_up_interruptible(&d->buf->wait_poll);
}
- mutex_unlock(&ir->mutex);
+ mutex_unlock(&d->mutex);
+ cdev_device_del(&d->cdev, &d->dev);
ida_simple_remove(&lirc_ida, d->minor);
- put_device(&ir->dev);
+ put_device(&d->dev);
}
EXPORT_SYMBOL(lirc_unregister_device);
int lirc_dev_fop_open(struct inode *inode, struct file *file)
{
- struct irctl *ir = container_of(inode->i_cdev, struct irctl, cdev);
+ struct lirc_dev *d = container_of(inode->i_cdev, struct lirc_dev, cdev);
int retval;
- dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
+ dev_dbg(&d->dev, LOGHEAD "open called\n", d->name, d->minor);
- retval = mutex_lock_interruptible(&ir->mutex);
+ retval = mutex_lock_interruptible(&d->mutex);
if (retval)
return retval;
- if (!ir->attached) {
+ if (!d->attached) {
retval = -ENODEV;
goto out;
}
- if (ir->open) {
+ if (d->open) {
retval = -EBUSY;
goto out;
}
- if (ir->d.rdev) {
- retval = rc_open(ir->d.rdev);
+ if (d->rdev) {
+ retval = rc_open(d->rdev);
if (retval)
goto out;
}
- if (ir->buf)
- lirc_buffer_clear(ir->buf);
+ if (d->buf)
+ lirc_buffer_clear(d->buf);
- ir->open++;
+ d->open++;
lirc_init_pdata(inode, file);
nonseekable_open(inode, file);
- mutex_unlock(&ir->mutex);
+ mutex_unlock(&d->mutex);
return 0;
out:
- mutex_unlock(&ir->mutex);
+ mutex_unlock(&d->mutex);
return retval;
}
EXPORT_SYMBOL(lirc_dev_fop_open);
int lirc_dev_fop_close(struct inode *inode, struct file *file)
{
- struct irctl *ir = file->private_data;
+ struct lirc_dev *d = file->private_data;
- mutex_lock(&ir->mutex);
+ mutex_lock(&d->mutex);
- rc_close(ir->d.rdev);
- ir->open--;
+ rc_close(d->rdev);
+ d->open--;
- mutex_unlock(&ir->mutex);
+ mutex_unlock(&d->mutex);
return 0;
}
@@ -304,24 +268,23 @@ EXPORT_SYMBOL(lirc_dev_fop_close);
unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
{
- struct irctl *ir = file->private_data;
+ struct lirc_dev *d = file->private_data;
unsigned int ret;
- if (!ir->attached)
+ if (!d->attached)
return POLLHUP | POLLERR;
- if (ir->buf) {
- poll_wait(file, &ir->buf->wait_poll, wait);
+ if (d->buf) {
+ poll_wait(file, &d->buf->wait_poll, wait);
- if (lirc_buffer_empty(ir->buf))
+ if (lirc_buffer_empty(d->buf))
ret = 0;
else
ret = POLLIN | POLLRDNORM;
} else
ret = POLLERR;
- dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n",
- ir->d.name, ir->d.minor, ret);
+ dev_dbg(&d->dev, LOGHEAD "poll result = %d\n", d->name, d->minor, ret);
return ret;
}
@@ -329,44 +292,43 @@ 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;
+ struct lirc_dev *d = file->private_data;
__u32 mode;
int result;
- dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
- ir->d.name, ir->d.minor, cmd);
+ dev_dbg(&d->dev, LOGHEAD "ioctl called (0x%x)\n", d->name, d->minor, cmd);
- result = mutex_lock_interruptible(&ir->mutex);
+ result = mutex_lock_interruptible(&d->mutex);
if (result)
return result;
- if (!ir->attached) {
+ if (!d->attached) {
result = -ENODEV;
goto out;
}
switch (cmd) {
case LIRC_GET_FEATURES:
- result = put_user(ir->d.features, (__u32 __user *)arg);
+ result = put_user(d->features, (__u32 __user *)arg);
break;
case LIRC_GET_REC_MODE:
- if (!LIRC_CAN_REC(ir->d.features)) {
+ if (!LIRC_CAN_REC(d->features)) {
result = -ENOTTY;
break;
}
result = put_user(LIRC_REC2MODE
- (ir->d.features & LIRC_CAN_REC_MASK),
+ (d->features & LIRC_CAN_REC_MASK),
(__u32 __user *)arg);
break;
case LIRC_SET_REC_MODE:
- if (!LIRC_CAN_REC(ir->d.features)) {
+ if (!LIRC_CAN_REC(d->features)) {
result = -ENOTTY;
break;
}
result = get_user(mode, (__u32 __user *)arg);
- if (!result && !(LIRC_MODE2REC(mode) & ir->d.features))
+ if (!result && !(LIRC_MODE2REC(mode) & d->features))
result = -EINVAL;
/*
* FIXME: We should actually set the mode somehow but
@@ -374,14 +336,14 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
*/
break;
case LIRC_GET_LENGTH:
- result = put_user(ir->d.code_length, (__u32 __user *)arg);
+ result = put_user(d->code_length, (__u32 __user *)arg);
break;
default:
result = -ENOTTY;
}
out:
- mutex_unlock(&ir->mutex);
+ mutex_unlock(&d->mutex);
return result;
}
EXPORT_SYMBOL(lirc_dev_fop_ioctl);
@@ -391,28 +353,28 @@ ssize_t lirc_dev_fop_read(struct file *file,
size_t length,
loff_t *ppos)
{
- struct irctl *ir = file->private_data;
- unsigned char buf[ir->buf->chunk_size];
+ struct lirc_dev *d = file->private_data;
+ unsigned char buf[d->buf->chunk_size];
int ret, written = 0;
DECLARE_WAITQUEUE(wait, current);
- dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
+ dev_dbg(&d->dev, LOGHEAD "read called\n", d->name, d->minor);
- ret = mutex_lock_interruptible(&ir->mutex);
+ ret = mutex_lock_interruptible(&d->mutex);
if (ret)
return ret;
- if (!ir->attached) {
+ if (!d->attached) {
ret = -ENODEV;
goto out_locked;
}
- if (!LIRC_CAN_REC(ir->d.features)) {
+ if (!LIRC_CAN_REC(d->features)) {
ret = -EINVAL;
goto out_locked;
}
- if (length % ir->buf->chunk_size) {
+ if (length % d->buf->chunk_size) {
ret = -EINVAL;
goto out_locked;
}
@@ -422,14 +384,14 @@ ssize_t lirc_dev_fop_read(struct file *file,
* to avoid losing scan code (in case when queue is awaken somewhere
* between while condition checking and scheduling)
*/
- add_wait_queue(&ir->buf->wait_poll, &wait);
+ add_wait_queue(&d->buf->wait_poll, &wait);
/*
* while we didn't provide 'length' bytes, device is opened in blocking
* mode and 'copy_to_user' is happy, wait for data.
*/
while (written < length && ret == 0) {
- if (lirc_buffer_empty(ir->buf)) {
+ if (lirc_buffer_empty(d->buf)) {
/* According to the read(2) man page, 'written' can be
* returned as less than 'length', instead of blocking
* again, returning -EWOULDBLOCK, or returning
@@ -446,36 +408,36 @@ ssize_t lirc_dev_fop_read(struct file *file,
break;
}
- mutex_unlock(&ir->mutex);
+ mutex_unlock(&d->mutex);
set_current_state(TASK_INTERRUPTIBLE);
schedule();
set_current_state(TASK_RUNNING);
- ret = mutex_lock_interruptible(&ir->mutex);
+ ret = mutex_lock_interruptible(&d->mutex);
if (ret) {
- remove_wait_queue(&ir->buf->wait_poll, &wait);
+ remove_wait_queue(&d->buf->wait_poll, &wait);
goto out_unlocked;
}
- if (!ir->attached) {
+ if (!d->attached) {
ret = -ENODEV;
goto out_locked;
}
} else {
- lirc_buffer_read(ir->buf, buf);
+ lirc_buffer_read(d->buf, buf);
ret = copy_to_user((void __user *)buffer+written, buf,
- ir->buf->chunk_size);
+ d->buf->chunk_size);
if (!ret)
- written += ir->buf->chunk_size;
+ written += d->buf->chunk_size;
else
ret = -EFAULT;
}
}
- remove_wait_queue(&ir->buf->wait_poll, &wait);
+ remove_wait_queue(&d->buf->wait_poll, &wait);
out_locked:
- mutex_unlock(&ir->mutex);
+ mutex_unlock(&d->mutex);
out_unlocked:
return ret ? ret : written;
@@ -484,17 +446,17 @@ EXPORT_SYMBOL(lirc_dev_fop_read);
void lirc_init_pdata(struct inode *inode, struct file *file)
{
- struct irctl *ir = container_of(inode->i_cdev, struct irctl, cdev);
+ struct lirc_dev *d = container_of(inode->i_cdev, struct lirc_dev, cdev);
- file->private_data = ir;
+ file->private_data = d;
}
EXPORT_SYMBOL(lirc_init_pdata);
void *lirc_get_pdata(struct file *file)
{
- struct irctl *ir = file->private_data;
+ struct lirc_dev *d = file->private_data;
- return ir->d.data;
+ return d->data;
}
EXPORT_SYMBOL(lirc_get_pdata);
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c
index bbbba25ae574..406833d3a28e 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -187,10 +187,8 @@ 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) {
+ if (ir->l)
lirc_unregister_device(ir->l);
- lirc_free_device(ir->l);
- }
if (kfifo_initialized(&ir->rbuf.fifo))
lirc_buffer_free(&ir->rbuf);
@@ -321,7 +319,7 @@ static int add_to_buf(struct IR *ir)
int ret;
int failures = 0;
unsigned char sendbuf[1] = { 0 };
- struct lirc_buffer *rbuf = ir->l->rbuf;
+ struct lirc_buffer *rbuf = ir->l->buf;
struct IR_rx *rx;
struct IR_tx *tx;
@@ -467,7 +465,7 @@ static int add_to_buf(struct IR *ir)
static int lirc_thread(void *arg)
{
struct IR *ir = arg;
- struct lirc_buffer *rbuf = ir->l->rbuf;
+ struct lirc_buffer *rbuf = ir->l->buf;
dev_dbg(ir->dev, "poll thread started\n");
@@ -888,7 +886,7 @@ static ssize_t read(struct file *filep, char __user *outbuf, size_t n,
{
struct IR *ir = lirc_get_pdata(filep);
struct IR_rx *rx;
- struct lirc_buffer *rbuf = ir->l->rbuf;
+ struct lirc_buffer *rbuf = ir->l->buf;
int ret = 0, written = 0, retries = 0;
unsigned int m;
DECLARE_WAITQUEUE(wait, current);
@@ -1206,7 +1204,7 @@ static unsigned int poll(struct file *filep, poll_table *wait)
{
struct IR *ir = lirc_get_pdata(filep);
struct IR_rx *rx;
- struct lirc_buffer *rbuf = ir->l->rbuf;
+ struct lirc_buffer *rbuf = ir->l->buf;
unsigned int ret;
dev_dbg(ir->dev, "%s called\n", __func__);
@@ -1452,6 +1450,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
ir->l->code_length = 13;
ir->l->fops = &lirc_fops;
ir->l->owner = THIS_MODULE;
+ ir->l->dev.parent = &adap->dev;
/*
* FIXME this is a pointer reference to us, but no refcount.
@@ -1459,13 +1458,12 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
* This OK for now, since lirc_dev currently won't touch this
* buffer as we provide our own lirc_fops.
*
- * Currently our own lirc_fops rely on this ir->l->rbuf pointer
+ * Currently our own lirc_fops rely on this ir->l->buf pointer
*/
- ir->l->rbuf = &ir->rbuf;
- ir->l->dev = &adap->dev;
+ ir->l->buf = &ir->rbuf;
/* This will be returned by lirc_get_pdata() */
ir->l->data = ir;
- ret = lirc_buffer_init(ir->l->rbuf, 2, BUFLEN / 2);
+ ret = lirc_buffer_init(ir->l->buf, 2, BUFLEN / 2);
if (ret) {
lirc_free_device(ir->l);
ir->l = NULL;
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 21aac9494678..63dd88b02479 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -15,6 +15,8 @@
#include <linux/poll.h>
#include <linux/kfifo.h>
#include <media/lirc.h>
+#include <linux/device.h>
+#include <linux/cdev.h>
struct lirc_buffer {
wait_queue_head_t wait_poll;
@@ -121,14 +123,19 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
* @chunk_size: Size of each FIFO buffer.
* Only used if @rbuf is NULL.
* @data: private per-driver data
- * @rbuf: if not NULL, it will be used as a read buffer, you will
+ * @buf: if %NULL, lirc_dev will allocate and manage the buffer,
+ * otherwise allocated by the caller which will
* have to write to the buffer by other means, like irq's
* (see also lirc_serial.c).
+ * @buf_internal: whether lirc_dev has allocated the read buffer or not
* @rdev: &struct rc_dev associated with the device
* @fops: &struct file_operations for the device
- * @dev: &struct device assigned to the device
* @owner: the module owning this struct
- * @irctl: &struct irctl assigned to the device
+ * @attached: if the device is still live
+ * @open: open count for the device's chardev
+ * @mutex: serialises file_operations calls
+ * @dev: &struct device assigned to the device
+ * @cdev: &struct cdev assigned to the device
*/
struct lirc_dev {
char name[40];
@@ -138,14 +145,21 @@ struct lirc_dev {
unsigned int buffer_size; /* in chunks holding one code each */
unsigned int chunk_size;
+ struct lirc_buffer *buf;
+ bool buf_internal;
void *data;
- struct lirc_buffer *rbuf;
struct rc_dev *rdev;
const struct file_operations *fops;
- struct device *dev;
struct module *owner;
- struct irctl *irctl;
+
+ bool attached;
+ int open;
+
+ struct mutex mutex;
+
+ struct device dev;
+ struct cdev cdev;
};
extern struct lirc_dev *lirc_allocate_device(void);
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 17/19] ir-lirc-codec: merge lirc_dev_fop_ioctl into ir_lirc_ioctl
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (15 preceding siblings ...)
2017-06-25 12:32 ` [PATCH 16/19] lirc_dev: merge struct irctl into struct lirc_dev David Härdeman
@ 2017-06-25 12:32 ` David Härdeman
2017-06-25 12:32 ` [PATCH 18/19] ir-lirc-codec: move the remaining fops over from lirc_dev David Härdeman
2017-06-25 12:32 ` [PATCH 19/19] lirc_dev: consistent device registration printk David Härdeman
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:32 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
ir_lirc_ioctl() is the only caller of lirc_dev_fop_ioctl() so merging the
latter into the former makes the code more readable. At the same time, this
allows the locking situation in ir_lirc_ioctl() to be fixed by holding
the lirc_dev mutex during the whole ioctl call.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ir-lirc-codec.c | 168 ++++++++++++++++++++++++--------------
drivers/media/rc/lirc_dev.c | 59 -------------
include/media/lirc_dev.h | 1
3 files changed, 105 insertions(+), 123 deletions(-)
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index ba20a4ce9cbc..f914a3d5a468 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -178,12 +178,13 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
}
static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
- unsigned long arg)
+ unsigned long arg)
{
struct lirc_codec *lirc;
struct rc_dev *dev;
+ struct lirc_dev *d;
u32 __user *argp = (u32 __user *)(arg);
- int ret = 0;
+ int ret;
__u32 val = 0, tmp;
lirc = lirc_get_pdata(filep);
@@ -194,10 +195,23 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
if (!dev)
return -EFAULT;
+ d = lirc->ldev;
+ if (!d)
+ return -EFAULT;
+
+ ret = mutex_lock_interruptible(&d->mutex);
+ if (ret)
+ return ret;
+
+ if (!d->attached) {
+ ret = -ENODEV;
+ goto out;
+ }
+
if (_IOC_DIR(cmd) & _IOC_WRITE) {
ret = get_user(val, argp);
if (ret)
- return ret;
+ goto out;
}
switch (cmd) {
@@ -205,125 +219,153 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
/* legacy support */
case LIRC_GET_SEND_MODE:
if (!dev->tx_ir)
- return -ENOTTY;
-
- val = LIRC_MODE_PULSE;
+ ret = -ENOTTY;
+ else
+ val = LIRC_MODE_PULSE;
break;
case LIRC_SET_SEND_MODE:
if (!dev->tx_ir)
- return -ENOTTY;
-
- if (val != LIRC_MODE_PULSE)
- return -EINVAL;
- return 0;
+ ret = -ENOTTY;
+ else if (val != LIRC_MODE_PULSE)
+ ret = -EINVAL;
+ break;
/* TX settings */
case LIRC_SET_TRANSMITTER_MASK:
if (!dev->s_tx_mask)
- return -ENOTTY;
-
- return dev->s_tx_mask(dev, val);
+ ret = -ENOTTY;
+ else
+ ret = dev->s_tx_mask(dev, val);
+ break;
case LIRC_SET_SEND_CARRIER:
if (!dev->s_tx_carrier)
- return -ENOTTY;
-
- return dev->s_tx_carrier(dev, val);
+ ret = -ENOTTY;
+ else
+ ret = dev->s_tx_carrier(dev, val);
+ break;
case LIRC_SET_SEND_DUTY_CYCLE:
if (!dev->s_tx_duty_cycle)
- return -ENOTTY;
-
- if (val <= 0 || val >= 100)
- return -EINVAL;
-
- return dev->s_tx_duty_cycle(dev, val);
+ ret = -ENOTTY;
+ else if (val <= 0 || val >= 100)
+ ret = -EINVAL;
+ else
+ ret = dev->s_tx_duty_cycle(dev, val);
+ break;
/* RX settings */
case LIRC_SET_REC_CARRIER:
if (!dev->s_rx_carrier_range)
- return -ENOTTY;
-
- if (val <= 0)
- return -EINVAL;
-
- return dev->s_rx_carrier_range(dev,
- dev->raw->lirc.carrier_low,
- val);
+ ret = -ENOTTY;
+ else if (val <= 0)
+ ret = -EINVAL;
+ else
+ ret = dev->s_rx_carrier_range(dev,
+ dev->raw->lirc.carrier_low,
+ val);
+ break;
case LIRC_SET_REC_CARRIER_RANGE:
if (!dev->s_rx_carrier_range)
- return -ENOTTY;
-
- if (val <= 0)
- return -EINVAL;
-
- dev->raw->lirc.carrier_low = val;
- return 0;
+ ret = -ENOTTY;
+ else if (val <= 0)
+ ret = -EINVAL;
+ else
+ dev->raw->lirc.carrier_low = val;
+ break;
case LIRC_GET_REC_RESOLUTION:
if (!dev->rx_resolution)
- return -ENOTTY;
-
- val = dev->rx_resolution;
+ ret = -ENOTTY;
+ else
+ val = dev->rx_resolution;
break;
case LIRC_SET_WIDEBAND_RECEIVER:
if (!dev->s_learning_mode)
- return -ENOTTY;
-
- return dev->s_learning_mode(dev, !!val);
+ ret = -ENOTTY;
+ else
+ ret = dev->s_learning_mode(dev, !!val);
+ break;
case LIRC_SET_MEASURE_CARRIER_MODE:
if (!dev->s_carrier_report)
- return -ENOTTY;
-
- return dev->s_carrier_report(dev, !!val);
+ ret = -ENOTTY;
+ else
+ ret = dev->s_carrier_report(dev, !!val);
+ break;
/* Generic timeout support */
case LIRC_GET_MIN_TIMEOUT:
if (!dev->max_timeout)
- return -ENOTTY;
- val = DIV_ROUND_UP(dev->min_timeout, 1000);
+ ret = -ENOTTY;
+ else
+ val = DIV_ROUND_UP(dev->min_timeout, 1000);
break;
case LIRC_GET_MAX_TIMEOUT:
if (!dev->max_timeout)
- return -ENOTTY;
- val = dev->max_timeout / 1000;
+ ret = -ENOTTY;
+ else
+ val = dev->max_timeout / 1000;
break;
case LIRC_SET_REC_TIMEOUT:
- if (!dev->max_timeout)
- return -ENOTTY;
-
tmp = val * 1000;
- if (tmp < dev->min_timeout ||
- tmp > dev->max_timeout)
- return -EINVAL;
-
- if (dev->s_timeout)
+ if (!dev->max_timeout)
+ ret = -ENOTTY;
+ else if (tmp < dev->min_timeout)
+ ret = -EINVAL;
+ else if (tmp > dev->max_timeout)
+ ret = -EINVAL;
+ else if (dev->s_timeout)
ret = dev->s_timeout(dev, tmp);
+
if (!ret)
dev->timeout = tmp;
break;
case LIRC_SET_REC_TIMEOUT_REPORTS:
if (!dev->timeout)
- return -ENOTTY;
+ ret = -ENOTTY;
+ else
+ lirc->send_timeout_reports = !!val;
+ break;
+
+ case LIRC_GET_FEATURES:
+ val = d->features;
+ break;
- lirc->send_timeout_reports = !!val;
+ case LIRC_GET_REC_MODE:
+ if (!LIRC_CAN_REC(d->features))
+ ret = -ENOTTY;
+ else
+ val = LIRC_REC2MODE(d->features & LIRC_CAN_REC_MASK);
+ break;
+
+ case LIRC_SET_REC_MODE:
+ if (!LIRC_CAN_REC(d->features))
+ ret = -ENOTTY;
+ else if (!(LIRC_MODE2REC(val) & d->features))
+ ret = -EINVAL;
+ break;
+
+ case LIRC_GET_LENGTH:
+ val = d->code_length;
break;
default:
- return lirc_dev_fop_ioctl(filep, cmd, arg);
+ ret = -ENOTTY;
}
- if (_IOC_DIR(cmd) & _IOC_READ)
+ if (!ret && (_IOC_DIR(cmd) & _IOC_READ))
ret = put_user(val, argp);
+out:
+ mutex_unlock(&d->mutex);
return ret;
}
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 4ad08d3d4e2f..278d9b34d382 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -19,7 +19,6 @@
#include <linux/module.h>
#include <linux/sched/signal.h>
-#include <linux/ioctl.h>
#include <linux/poll.h>
#include <linux/mutex.h>
#include <linux/device.h>
@@ -290,64 +289,6 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
}
EXPORT_SYMBOL(lirc_dev_fop_poll);
-long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
- struct lirc_dev *d = file->private_data;
- __u32 mode;
- int result;
-
- dev_dbg(&d->dev, LOGHEAD "ioctl called (0x%x)\n", d->name, d->minor, cmd);
-
- result = mutex_lock_interruptible(&d->mutex);
- if (result)
- return result;
-
- if (!d->attached) {
- result = -ENODEV;
- goto out;
- }
-
- switch (cmd) {
- case LIRC_GET_FEATURES:
- result = put_user(d->features, (__u32 __user *)arg);
- break;
- case LIRC_GET_REC_MODE:
- if (!LIRC_CAN_REC(d->features)) {
- result = -ENOTTY;
- break;
- }
-
- result = put_user(LIRC_REC2MODE
- (d->features & LIRC_CAN_REC_MASK),
- (__u32 __user *)arg);
- break;
- case LIRC_SET_REC_MODE:
- if (!LIRC_CAN_REC(d->features)) {
- result = -ENOTTY;
- break;
- }
-
- result = get_user(mode, (__u32 __user *)arg);
- if (!result && !(LIRC_MODE2REC(mode) & d->features))
- result = -EINVAL;
- /*
- * FIXME: We should actually set the mode somehow but
- * for now, lirc_serial doesn't support mode changing either
- */
- break;
- case LIRC_GET_LENGTH:
- result = put_user(d->code_length, (__u32 __user *)arg);
- break;
- default:
- result = -ENOTTY;
- }
-
-out:
- mutex_unlock(&d->mutex);
- return result;
-}
-EXPORT_SYMBOL(lirc_dev_fop_ioctl);
-
ssize_t lirc_dev_fop_read(struct file *file,
char __user *buffer,
size_t length,
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 63dd88b02479..e1bf7ef20fdc 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -184,7 +184,6 @@ void *lirc_get_pdata(struct file *file);
int lirc_dev_fop_open(struct inode *inode, struct file *file);
int lirc_dev_fop_close(struct inode *inode, struct file *file);
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);
#endif
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 18/19] ir-lirc-codec: move the remaining fops over from lirc_dev
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (16 preceding siblings ...)
2017-06-25 12:32 ` [PATCH 17/19] ir-lirc-codec: merge lirc_dev_fop_ioctl into ir_lirc_ioctl David Härdeman
@ 2017-06-25 12:32 ` David Härdeman
2017-06-25 12:32 ` [PATCH 19/19] lirc_dev: consistent device registration printk David Härdeman
18 siblings, 0 replies; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:32 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
ir-lirc-codec is the only user of these functions, so instead of exporting them
from lirc_dev, move all of them over to ir-lirc-codec.
This means that all ir-lirc-codec fops can be found next to each other in
ir-lirc-codec.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ir-lirc-codec.c | 181 ++++++++++++++++++++++++++++++++++++-
drivers/media/rc/lirc_dev.c | 187 --------------------------------------
include/media/lirc_dev.h | 8 --
3 files changed, 178 insertions(+), 198 deletions(-)
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index f914a3d5a468..05f88401f694 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -13,6 +13,7 @@
*/
#include <linux/sched.h>
+#include <linux/sched/signal.h>
#include <linux/wait.h>
#include <linux/module.h>
#include <media/lirc.h>
@@ -21,6 +22,7 @@
#include "rc-core-priv.h"
#define LIRCBUF_SIZE 256
+#define LOGHEAD "lirc_dev (%s[%d]): "
/**
* ir_lirc_decode() - Send raw IR data to lirc_dev to be relayed to the
@@ -369,6 +371,177 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
return ret;
}
+static ssize_t ir_lirc_read(struct file *file, char __user *buffer,
+ size_t length, loff_t *ppos)
+{
+ struct lirc_dev *d = file->private_data;
+ unsigned char buf[d->buf->chunk_size];
+ int ret, written = 0;
+ DECLARE_WAITQUEUE(wait, current);
+
+ dev_dbg(&d->dev, LOGHEAD "read called\n", d->name, d->minor);
+
+ ret = mutex_lock_interruptible(&d->mutex);
+ if (ret)
+ return ret;
+
+ if (!d->attached) {
+ ret = -ENODEV;
+ goto out_locked;
+ }
+
+ if (!LIRC_CAN_REC(d->features)) {
+ ret = -EINVAL;
+ goto out_locked;
+ }
+
+ if (length % d->buf->chunk_size) {
+ ret = -EINVAL;
+ goto out_locked;
+ }
+
+ /*
+ * we add ourselves to the task queue before buffer check
+ * to avoid losing scan code (in case when queue is awaken somewhere
+ * between while condition checking and scheduling)
+ */
+ add_wait_queue(&d->buf->wait_poll, &wait);
+
+ /*
+ * while we didn't provide 'length' bytes, device is opened in blocking
+ * mode and 'copy_to_user' is happy, wait for data.
+ */
+ while (written < length && ret == 0) {
+ if (lirc_buffer_empty(d->buf)) {
+ /* According to the read(2) man page, 'written' can be
+ * returned as less than 'length', instead of blocking
+ * again, returning -EWOULDBLOCK, or returning
+ * -ERESTARTSYS
+ */
+ if (written)
+ break;
+ if (file->f_flags & O_NONBLOCK) {
+ ret = -EWOULDBLOCK;
+ break;
+ }
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ break;
+ }
+
+ mutex_unlock(&d->mutex);
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule();
+ set_current_state(TASK_RUNNING);
+
+ ret = mutex_lock_interruptible(&d->mutex);
+ if (ret) {
+ remove_wait_queue(&d->buf->wait_poll, &wait);
+ goto out_unlocked;
+ }
+
+ if (!d->attached) {
+ ret = -ENODEV;
+ goto out_locked;
+ }
+ } else {
+ lirc_buffer_read(d->buf, buf);
+ ret = copy_to_user((void __user *)buffer+written, buf,
+ d->buf->chunk_size);
+ if (!ret)
+ written += d->buf->chunk_size;
+ else
+ ret = -EFAULT;
+ }
+ }
+
+ remove_wait_queue(&d->buf->wait_poll, &wait);
+
+out_locked:
+ mutex_unlock(&d->mutex);
+
+out_unlocked:
+ return ret ? ret : written;
+}
+
+static unsigned int ir_lirc_poll(struct file *file, poll_table *wait)
+{
+ struct lirc_dev *d = file->private_data;
+ unsigned int ret;
+
+ if (!d->attached)
+ return POLLHUP | POLLERR;
+
+ if (d->buf) {
+ poll_wait(file, &d->buf->wait_poll, wait);
+
+ if (lirc_buffer_empty(d->buf))
+ ret = 0;
+ else
+ ret = POLLIN | POLLRDNORM;
+ } else
+ ret = POLLERR;
+
+ dev_dbg(&d->dev, LOGHEAD "poll result = %d\n", d->name, d->minor, ret);
+
+ return ret;
+}
+
+static int ir_lirc_open(struct inode *inode, struct file *file)
+{
+ struct lirc_dev *d = container_of(inode->i_cdev, struct lirc_dev, cdev);
+ int retval;
+
+ dev_dbg(&d->dev, LOGHEAD "open called\n", d->name, d->minor);
+
+ retval = mutex_lock_interruptible(&d->mutex);
+ if (retval)
+ return retval;
+
+ if (!d->attached) {
+ retval = -ENODEV;
+ goto out;
+ }
+
+ if (d->open) {
+ retval = -EBUSY;
+ goto out;
+ }
+
+ retval = rc_open(d->rdev);
+ if (retval)
+ goto out;
+
+ if (d->buf)
+ lirc_buffer_clear(d->buf);
+
+ d->open++;
+
+ lirc_init_pdata(inode, file);
+ nonseekable_open(inode, file);
+ mutex_unlock(&d->mutex);
+
+ return 0;
+
+out:
+ mutex_unlock(&d->mutex);
+ return retval;
+}
+
+static int ir_lirc_close(struct inode *inode, struct file *file)
+{
+ struct lirc_dev *d = file->private_data;
+
+ mutex_lock(&d->mutex);
+
+ rc_close(d->rdev);
+ d->open--;
+
+ mutex_unlock(&d->mutex);
+
+ return 0;
+}
+
static const struct file_operations lirc_fops = {
.owner = THIS_MODULE,
.write = ir_lirc_transmit_ir,
@@ -376,10 +549,10 @@ static const struct file_operations lirc_fops = {
#ifdef CONFIG_COMPAT
.compat_ioctl = ir_lirc_ioctl,
#endif
- .read = lirc_dev_fop_read,
- .poll = lirc_dev_fop_poll,
- .open = lirc_dev_fop_open,
- .release = lirc_dev_fop_close,
+ .read = ir_lirc_read,
+ .poll = ir_lirc_poll,
+ .open = ir_lirc_open,
+ .release = ir_lirc_close,
.llseek = no_llseek,
};
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 278d9b34d382..c1c917932f7e 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -18,7 +18,6 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/module.h>
-#include <linux/sched/signal.h>
#include <linux/poll.h>
#include <linux/mutex.h>
#include <linux/device.h>
@@ -29,8 +28,6 @@
#include <media/lirc.h>
#include <media/lirc_dev.h>
-#define LOGHEAD "lirc_dev (%s[%d]): "
-
static dev_t lirc_base_dev;
/* Used to keep track of allocated lirc devices */
@@ -192,11 +189,8 @@ void lirc_unregister_device(struct lirc_dev *d)
mutex_lock(&d->mutex);
d->attached = false;
- if (d->open) {
- dev_dbg(&d->dev, LOGHEAD "releasing opened driver\n",
- d->name, d->minor);
+ if (d->open)
wake_up_interruptible(&d->buf->wait_poll);
- }
mutex_unlock(&d->mutex);
@@ -206,185 +200,6 @@ void lirc_unregister_device(struct lirc_dev *d)
}
EXPORT_SYMBOL(lirc_unregister_device);
-int lirc_dev_fop_open(struct inode *inode, struct file *file)
-{
- struct lirc_dev *d = container_of(inode->i_cdev, struct lirc_dev, cdev);
- int retval;
-
- dev_dbg(&d->dev, LOGHEAD "open called\n", d->name, d->minor);
-
- retval = mutex_lock_interruptible(&d->mutex);
- if (retval)
- return retval;
-
- if (!d->attached) {
- retval = -ENODEV;
- goto out;
- }
-
- if (d->open) {
- retval = -EBUSY;
- goto out;
- }
-
- if (d->rdev) {
- retval = rc_open(d->rdev);
- if (retval)
- goto out;
- }
-
- if (d->buf)
- lirc_buffer_clear(d->buf);
-
- d->open++;
-
- lirc_init_pdata(inode, file);
- nonseekable_open(inode, file);
- mutex_unlock(&d->mutex);
-
- return 0;
-
-out:
- mutex_unlock(&d->mutex);
- return retval;
-}
-EXPORT_SYMBOL(lirc_dev_fop_open);
-
-int lirc_dev_fop_close(struct inode *inode, struct file *file)
-{
- struct lirc_dev *d = file->private_data;
-
- mutex_lock(&d->mutex);
-
- rc_close(d->rdev);
- d->open--;
-
- mutex_unlock(&d->mutex);
-
- return 0;
-}
-EXPORT_SYMBOL(lirc_dev_fop_close);
-
-unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
-{
- struct lirc_dev *d = file->private_data;
- unsigned int ret;
-
- if (!d->attached)
- return POLLHUP | POLLERR;
-
- if (d->buf) {
- poll_wait(file, &d->buf->wait_poll, wait);
-
- if (lirc_buffer_empty(d->buf))
- ret = 0;
- else
- ret = POLLIN | POLLRDNORM;
- } else
- ret = POLLERR;
-
- dev_dbg(&d->dev, LOGHEAD "poll result = %d\n", d->name, d->minor, ret);
-
- return ret;
-}
-EXPORT_SYMBOL(lirc_dev_fop_poll);
-
-ssize_t lirc_dev_fop_read(struct file *file,
- char __user *buffer,
- size_t length,
- loff_t *ppos)
-{
- struct lirc_dev *d = file->private_data;
- unsigned char buf[d->buf->chunk_size];
- int ret, written = 0;
- DECLARE_WAITQUEUE(wait, current);
-
- dev_dbg(&d->dev, LOGHEAD "read called\n", d->name, d->minor);
-
- ret = mutex_lock_interruptible(&d->mutex);
- if (ret)
- return ret;
-
- if (!d->attached) {
- ret = -ENODEV;
- goto out_locked;
- }
-
- if (!LIRC_CAN_REC(d->features)) {
- ret = -EINVAL;
- goto out_locked;
- }
-
- if (length % d->buf->chunk_size) {
- ret = -EINVAL;
- goto out_locked;
- }
-
- /*
- * we add ourselves to the task queue before buffer check
- * to avoid losing scan code (in case when queue is awaken somewhere
- * between while condition checking and scheduling)
- */
- add_wait_queue(&d->buf->wait_poll, &wait);
-
- /*
- * while we didn't provide 'length' bytes, device is opened in blocking
- * mode and 'copy_to_user' is happy, wait for data.
- */
- while (written < length && ret == 0) {
- if (lirc_buffer_empty(d->buf)) {
- /* According to the read(2) man page, 'written' can be
- * returned as less than 'length', instead of blocking
- * again, returning -EWOULDBLOCK, or returning
- * -ERESTARTSYS
- */
- if (written)
- break;
- if (file->f_flags & O_NONBLOCK) {
- ret = -EWOULDBLOCK;
- break;
- }
- if (signal_pending(current)) {
- ret = -ERESTARTSYS;
- break;
- }
-
- mutex_unlock(&d->mutex);
- set_current_state(TASK_INTERRUPTIBLE);
- schedule();
- set_current_state(TASK_RUNNING);
-
- ret = mutex_lock_interruptible(&d->mutex);
- if (ret) {
- remove_wait_queue(&d->buf->wait_poll, &wait);
- goto out_unlocked;
- }
-
- if (!d->attached) {
- ret = -ENODEV;
- goto out_locked;
- }
- } else {
- lirc_buffer_read(d->buf, buf);
- ret = copy_to_user((void __user *)buffer+written, buf,
- d->buf->chunk_size);
- if (!ret)
- written += d->buf->chunk_size;
- else
- ret = -EFAULT;
- }
- }
-
- remove_wait_queue(&d->buf->wait_poll, &wait);
-
-out_locked:
- mutex_unlock(&d->mutex);
-
-out_unlocked:
- return ret ? ret : written;
-}
-EXPORT_SYMBOL(lirc_dev_fop_read);
-
void lirc_init_pdata(struct inode *inode, struct file *file)
{
struct lirc_dev *d = container_of(inode->i_cdev, struct lirc_dev, cdev);
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index e1bf7ef20fdc..710d8abc9962 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -178,12 +178,4 @@ void lirc_init_pdata(struct inode *inode, struct file *file);
*/
void *lirc_get_pdata(struct file *file);
-/* default file operations
- * used by drivers if they override only some operations
- */
-int lirc_dev_fop_open(struct inode *inode, struct file *file);
-int lirc_dev_fop_close(struct inode *inode, struct file *file);
-unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait);
-ssize_t lirc_dev_fop_read(struct file *file, char __user *buffer, size_t length,
- loff_t *ppos);
#endif
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 19/19] lirc_dev: consistent device registration printk
2017-06-25 12:31 [PATCH 00/19] lirc_dev modernisation David Härdeman
` (17 preceding siblings ...)
2017-06-25 12:32 ` [PATCH 18/19] ir-lirc-codec: move the remaining fops over from lirc_dev David Härdeman
@ 2017-06-25 12:32 ` David Härdeman
2017-08-01 21:20 ` Sean Young
18 siblings, 1 reply; 23+ messages in thread
From: David Härdeman @ 2017-06-25 12:32 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
This patch changes the message that is printed on lirc device registration to
make it more consistent with the input and rc subsystems.
Before:
rc rc0: rc-core loopback device as /devices/virtual/rc/rc0
input: rc-core loopback device as /devices/virtual/rc/rc0/input43
lirc lirc0: lirc_dev: driver ir-lirc-codec (rc-loopback) registered at minor = 0
After:
rc rc0: rc-core loopback device as /devices/virtual/rc/rc0
input: rc-core loopback device as /devices/virtual/rc/rc0/input23
lirc lirc0: rc-core loopback device as /devices/virtual/rc/rc0/lirc0
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ir-lirc-codec.c | 3 +--
drivers/media/rc/lirc_dev.c | 6 ++++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 05f88401f694..4f33516a95a3 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -595,8 +595,7 @@ static int ir_lirc_register(struct rc_dev *dev)
if (dev->max_timeout)
features |= LIRC_CAN_SET_REC_TIMEOUT;
- snprintf(ldev->name, sizeof(ldev->name), "ir-lirc-codec (%s)",
- dev->driver_name);
+ snprintf(ldev->name, sizeof(ldev->name), "%s", dev->input_name);
ldev->features = features;
ldev->data = &dev->raw->lirc;
ldev->buf = NULL;
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index c1c917932f7e..03430a1fb192 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -105,6 +105,7 @@ int lirc_register_device(struct lirc_dev *d)
{
int minor;
int err;
+ const char *path;
if (!d) {
pr_err("driver pointer must be not NULL!\n");
@@ -171,8 +172,9 @@ int lirc_register_device(struct lirc_dev *d)
return err;
}
- dev_info(&d->dev, "lirc_dev: driver %s registered at minor = %d\n",
- d->name, d->minor);
+ path = kobject_get_path(&d->dev.kobj, GFP_KERNEL);
+ dev_info(&d->dev, "%s as %s\n", d->name, path ?: "N/A");
+ kfree(path);
return 0;
}
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 19/19] lirc_dev: consistent device registration printk
2017-06-25 12:32 ` [PATCH 19/19] lirc_dev: consistent device registration printk David Härdeman
@ 2017-08-01 21:20 ` Sean Young
0 siblings, 0 replies; 23+ messages in thread
From: Sean Young @ 2017-08-01 21:20 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, mchehab
On Sun, Jun 25, 2017 at 02:32:51PM +0200, David Härdeman wrote:
> This patch changes the message that is printed on lirc device registration to
> make it more consistent with the input and rc subsystems.
>
> Before:
> rc rc0: rc-core loopback device as /devices/virtual/rc/rc0
> input: rc-core loopback device as /devices/virtual/rc/rc0/input43
> lirc lirc0: lirc_dev: driver ir-lirc-codec (rc-loopback) registered at minor = 0
>
> After:
> rc rc0: rc-core loopback device as /devices/virtual/rc/rc0
> input: rc-core loopback device as /devices/virtual/rc/rc0/input23
> lirc lirc0: rc-core loopback device as /devices/virtual/rc/rc0/lirc0
There is a couple of problems with this.
1. lirc_dev name is 40 bytes, but rc_dev input_name has no limit, so it ends
up truncating which you don't want. For example:
[ 106.303589] rc rc2: IguanaWorks USB IR Transceiver version 0x0308 as /devices/pci0000:00/0000:00:1a.1/usb4/4-2/4-2.3/4-2.3:1.0/rc/rc2
[ 106.303664] input: IguanaWorks USB IR Transceiver version 0x0308 as /devices/pci0000:00/0000:00:1a.1/usb4/4-2/4-2.3/4-2.3:1.0/rc/rc2/input24
[ 106.307272] lirc lirc2: IguanaWorks USB IR Transceiver version as /devices/pci0000:00/0000:00:1a.1/usb4/4-2/4-2.3/4-2.3:1.0/rc/rc2/lirc2
[ 233.005834] rc rc2: Media Center Ed. eHome Infrared Remote Transceiver (0609:031d) as /devices/pci0000:00/0000:00:1a.1/usb4/4-2/4-2.3/4-2.3:1.0/rc/rc2
[ 233.005907] input: Media Center Ed. eHome Infrared Remote Transceiver (0609:031d) as /devices/pci0000:00/0000:00:1a.1/usb4/4-2/4-2.3/4-2.3:1.0/rc/rc2/input25
[ 233.006307] lirc lirc2: Media Center Ed. eHome Infrared Remote as /devices/pci0000:00/0000:00:1a.1/usb4/4-2/4-2.3/4-2.3:1.0/rc/rc2/lirc2
2. Documentation/media/uapi/rc/lirc-dev-intro.rst explicitly mentions this
message, so that will need updating too. On a related note, should we be
changing messages, esp. documented ones? I agree the original message is
a bit ugly.
3. I like have the driver name in the message. As you can see above, you
have no idea you're plugging in an mceusb device (iguanair is a bit
more obvious). Then again none of the other messages mention this, maybe
it be added there.
As it is the patch can't be applied.
Maybe lirc_dev name can be changed to const char* so we avoid the strcpy
and truncation.
Sean
>
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
> drivers/media/rc/ir-lirc-codec.c | 3 +--
> drivers/media/rc/lirc_dev.c | 6 ++++--
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> index 05f88401f694..4f33516a95a3 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -595,8 +595,7 @@ static int ir_lirc_register(struct rc_dev *dev)
> if (dev->max_timeout)
> features |= LIRC_CAN_SET_REC_TIMEOUT;
>
> - snprintf(ldev->name, sizeof(ldev->name), "ir-lirc-codec (%s)",
> - dev->driver_name);
> + snprintf(ldev->name, sizeof(ldev->name), "%s", dev->input_name);
> ldev->features = features;
> ldev->data = &dev->raw->lirc;
> ldev->buf = NULL;
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index c1c917932f7e..03430a1fb192 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -105,6 +105,7 @@ int lirc_register_device(struct lirc_dev *d)
> {
> int minor;
> int err;
> + const char *path;
>
> if (!d) {
> pr_err("driver pointer must be not NULL!\n");
> @@ -171,8 +172,9 @@ int lirc_register_device(struct lirc_dev *d)
> return err;
> }
>
> - dev_info(&d->dev, "lirc_dev: driver %s registered at minor = %d\n",
> - d->name, d->minor);
> + path = kobject_get_path(&d->dev.kobj, GFP_KERNEL);
> + dev_info(&d->dev, "%s as %s\n", d->name, path ?: "N/A");
> + kfree(path);
>
> return 0;
> }
^ permalink raw reply [flat|nested] 23+ messages in thread