* [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup
@ 2011-02-18 1:11 Andy Walls
2011-02-18 1:12 ` [PATCH 01/13] lirc_zilog: Restore checks for existence of the IR_tx object Andy Walls
` (13 more replies)
0 siblings, 14 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:11 UTC (permalink / raw)
To: linux-media
The following 13 patches are a substantial rework of lirc_zilog
reference counting, object allocation and deallocation, and object
locking.
With these changes, devices can now disappear out from under lircd +
lirc_dev + lirc_zilog with no adverse effects. I tested this with irw +
lircd + lirc_dev + lirc_zilog + cx18 + HVR-1600. I could unload the
cx18 driver without any oops or application crashes. When I reloaded
the cx18 driver, irw started receiving RX button presses again, and
irsend worked without a problem (and I didn't even need to restart
lircd!).
The ref counting fixes aren't finished as lirc_zilog itself can still be
unloaded by the user when it shouldn't be, but a hot unplug of an
HD-PVR, PVR-USB2, or HVR-1950 isn't going to trigger that.
These changes are base off of Jarod Wilson's git repo
http://git.linuxtv.org/jarod/linux-2.6-ir.git for-2.6.38 (IIRC)
Regards,
Andy
The following changes since commit c369acfb63914f9f502baef032bacfd5a53a871f:
mceusb: really fix remaining keybounce issues (2011-01-26 10:56:29 -0500)
are available in the git repository at:
ssh://linuxtv.org/git/awalls/media_tree.git z8-wilson-38
Andy Walls (13):
lirc_zilog: Restore checks for existence of the IR_tx object
lirc_zilog: Remove broken, ineffective reference counting
lirc_zilog: Convert ir_device instance array to a linked list
lirc_zilog: Convert the instance open count to an atomic_t
lirc_zilog: Use kernel standard methods for marking device non-seekable
lirc_zilog: Don't acquire the rx->buf_lock in the poll() function
lirc_zilog: Remove unneeded rx->buf_lock
lirc_zilog: Always allocate a Rx lirc_buffer object
lirc_zilog: Move constants from ir_probe() into the lirc_driver template
lirc_zilog: Add ref counting of struct IR, IR_tx, and IR_rx
lirc_zilog: Add locking of the i2c_clients when in use
lirc_zilog: Fix somewhat confusing information messages in ir_probe()
lirc_zilog: Update TODO list based on work completed and revised plans
drivers/staging/lirc/TODO.lirc_zilog | 51 +--
drivers/staging/lirc/lirc_zilog.c | 802 +++++++++++++++++++++-------------
2 files changed, 523 insertions(+), 330 deletions(-)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 01/13] lirc_zilog: Restore checks for existence of the IR_tx object
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
@ 2011-02-18 1:12 ` Andy Walls
2011-02-18 1:13 ` [PATCH 02/13] lirc_zilog: Remove broken, ineffective reference counting Andy Walls
` (12 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:12 UTC (permalink / raw)
To: linux-media
This reverts commit 8090232a237ab62e22307fc060097da1a283dd66 and
adds an additional check for ir->tx == NULL.
The user may need us to handle an RX only unit. Apparently
there are TV capture units in existence with Rx only wiring
and/or RX only firmware for the on-board Zilog Z8 IR unit.
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 27 ++++++++++++++++++++-------
1 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 0aad0d7..7389b77 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -209,7 +209,8 @@ static int add_to_buf(struct IR *ir)
return -ENODATA;
}
schedule_timeout((100 * HZ + 999) / 1000);
- ir->tx->need_boot = 1;
+ if (ir->tx != NULL)
+ ir->tx->need_boot = 1;
++failures;
mutex_unlock(&ir->ir_lock);
@@ -1032,9 +1033,10 @@ static long ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
int result;
unsigned long mode, features = 0;
- features |= LIRC_CAN_SEND_PULSE;
if (ir->rx != NULL)
features |= LIRC_CAN_REC_LIRCCODE;
+ if (ir->tx != NULL)
+ features |= LIRC_CAN_SEND_PULSE;
switch (cmd) {
case LIRC_GET_LENGTH:
@@ -1061,9 +1063,15 @@ static long ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
result = -EINVAL;
break;
case LIRC_GET_SEND_MODE:
+ if (!(features&LIRC_CAN_SEND_MASK))
+ return -ENOSYS;
+
result = put_user(LIRC_MODE_PULSE, (unsigned long *) arg);
break;
case LIRC_SET_SEND_MODE:
+ if (!(features&LIRC_CAN_SEND_MASK))
+ return -ENOSYS;
+
result = get_user(mode, (unsigned long *) arg);
if (!result && mode != LIRC_MODE_PULSE)
return -EINVAL;
@@ -1242,8 +1250,10 @@ static int ir_remove(struct i2c_client *client)
}
/* Good-bye Tx */
- i2c_set_clientdata(ir->tx->c, NULL);
- kfree(ir->tx);
+ if (ir->tx != NULL) {
+ i2c_set_clientdata(ir->tx->c, NULL);
+ kfree(ir->tx);
+ }
/* Good-bye IR */
del_ir_device(ir);
@@ -1393,9 +1403,12 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
* after registering with lirc as otherwise hotplug seems to take
* 10s to create the lirc device.
*/
- ret = tx_init(ir->tx);
- if (ret != 0)
- goto out_unregister;
+ if (ir->tx != NULL) {
+ /* Special TX init */
+ ret = tx_init(ir->tx);
+ if (ret != 0)
+ goto out_unregister;
+ }
zilog_info("probe of IR %s on %s (i2c-%d) done. IR unit ready.\n",
tx_probe ? "Tx" : "Rx", adap->name, adap->nr);
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 02/13] lirc_zilog: Remove broken, ineffective reference counting
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
2011-02-18 1:12 ` [PATCH 01/13] lirc_zilog: Restore checks for existence of the IR_tx object Andy Walls
@ 2011-02-18 1:13 ` Andy Walls
2011-02-18 1:14 ` [PATCH 03/13] lirc_zilog: Convert ir_device instance array to a linked list Andy Walls
` (11 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:13 UTC (permalink / raw)
To: linux-media
The set_use_inc() and set_use_dec() functions tried to lock
the underlying bridge driver device instance in memory by
changing the use count on the device's i2c_clients. This
worked for PCI devices (ivtv, cx18, bttv). It doesn't
work for hot-pluggable usb devices (pvrusb2 and hdpvr).
With usb device instances, the driver may get locked into
memory, but the unplugged hardware is gone.
The set_use_inc() set_use_dec() functions also tried to have
lirc_zilog change its own module refernce count, which is
racy and not guaranteed to work. The lirc_dev module does
actually perform proper module ref count manipulation on the
lirc_zilog module, so there is need for lirc_zilog to
attempt a buggy module get on itself anyway.
lirc_zilog also errantly called these functions on itself
in open() and close(), but lirc_dev did that already too.
So let's just gut the bodies of the set_use_*() functions,
and remove the extra calls to them from within lirc_zilog.
Proper reference counting of the struct IR, IR_rx, and IR_tx
objects -- to handle the case when the underlying
bttv, ivtv, cx18, hdpvr, or pvrusb2 bridge driver module or
device instance goes away -- will be added in subsequent
patches.
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 32 +-------------------------------
1 files changed, 1 insertions(+), 31 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 7389b77..3a91257 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -305,34 +305,12 @@ static int lirc_thread(void *arg)
static int set_use_inc(void *data)
{
- struct IR *ir = data;
-
- if (ir->l.owner == NULL || try_module_get(ir->l.owner) == 0)
- return -ENODEV;
-
- /* lock bttv in memory while /dev/lirc is in use */
- /*
- * this is completely broken code. lirc_unregister_driver()
- * must be possible even when the device is open
- */
- if (ir->rx != NULL)
- i2c_use_client(ir->rx->c);
- if (ir->tx != NULL)
- i2c_use_client(ir->tx->c);
-
return 0;
}
static void set_use_dec(void *data)
{
- struct IR *ir = data;
-
- if (ir->rx)
- i2c_release_client(ir->rx->c);
- if (ir->tx)
- i2c_release_client(ir->tx->c);
- if (ir->l.owner != NULL)
- module_put(ir->l.owner);
+ return;
}
/* safe read of a uint32 (always network byte order) */
@@ -1098,7 +1076,6 @@ static struct IR *find_ir_device_by_minor(unsigned int minor)
static int open(struct inode *node, struct file *filep)
{
struct IR *ir;
- int ret;
unsigned int minor = MINOR(node->i_rdev);
/* find our IR struct */
@@ -1112,12 +1089,6 @@ static int open(struct inode *node, struct file *filep)
/* increment in use count */
mutex_lock(&ir->ir_lock);
++ir->open;
- ret = set_use_inc(ir);
- if (ret != 0) {
- --ir->open;
- mutex_unlock(&ir->ir_lock);
- return ret;
- }
mutex_unlock(&ir->ir_lock);
/* stash our IR struct */
@@ -1139,7 +1110,6 @@ static int close(struct inode *node, struct file *filep)
/* decrement in use count */
mutex_lock(&ir->ir_lock);
--ir->open;
- set_use_dec(ir);
mutex_unlock(&ir->ir_lock);
return 0;
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 03/13] lirc_zilog: Convert ir_device instance array to a linked list
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
2011-02-18 1:12 ` [PATCH 01/13] lirc_zilog: Restore checks for existence of the IR_tx object Andy Walls
2011-02-18 1:13 ` [PATCH 02/13] lirc_zilog: Remove broken, ineffective reference counting Andy Walls
@ 2011-02-18 1:14 ` Andy Walls
2011-02-18 1:15 ` [PATCH 04/13] lirc_zilog: Convert the instance open count to an atomic_t Andy Walls
` (10 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:14 UTC (permalink / raw)
To: linux-media
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 59 +++++++++++++++++++-----------------
1 files changed, 31 insertions(+), 28 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 3a91257..39f7b53 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -89,6 +89,8 @@ struct IR_tx {
};
struct IR {
+ struct list_head list;
+
struct lirc_driver l;
struct mutex ir_lock;
@@ -99,9 +101,9 @@ struct IR {
struct IR_tx *tx;
};
-/* Minor -> data mapping */
-static struct mutex ir_devices_lock;
-static struct IR *ir_devices[MAX_IRCTL_DEVICES];
+/* IR transceiver instance object list */
+static DEFINE_MUTEX(ir_devices_lock);
+static LIST_HEAD(ir_devices_list);
/* Block size for IR transmitter */
#define TX_BLOCK_SIZE 99
@@ -1063,10 +1065,16 @@ static long ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
/* ir_devices_lock must be held */
static struct IR *find_ir_device_by_minor(unsigned int minor)
{
- if (minor >= MAX_IRCTL_DEVICES)
+ struct IR *ir;
+
+ if (list_empty(&ir_devices_list))
return NULL;
- return ir_devices[minor];
+ list_for_each_entry(ir, &ir_devices_list, list)
+ if (ir->l.minor == minor)
+ return ir;
+
+ return NULL;
}
/*
@@ -1172,25 +1180,21 @@ static void destroy_rx_kthread(struct IR_rx *rx)
/* ir_devices_lock must be held */
static int add_ir_device(struct IR *ir)
{
- int i;
-
- for (i = 0; i < MAX_IRCTL_DEVICES; i++)
- if (ir_devices[i] == NULL) {
- ir_devices[i] = ir;
- break;
- }
-
- return i == MAX_IRCTL_DEVICES ? -ENOMEM : i;
+ list_add_tail(&ir->list, &ir_devices_list);
+ return 0;
}
/* ir_devices_lock must be held */
static void del_ir_device(struct IR *ir)
{
- int i;
+ struct IR *p;
+
+ if (list_empty(&ir_devices_list))
+ return;
- for (i = 0; i < MAX_IRCTL_DEVICES; i++)
- if (ir_devices[i] == ir) {
- ir_devices[i] = NULL;
+ list_for_each_entry(p, &ir_devices_list, list)
+ if (p == ir) {
+ list_del(&p->list);
break;
}
}
@@ -1237,17 +1241,16 @@ static int ir_remove(struct i2c_client *client)
/* ir_devices_lock must be held */
static struct IR *find_ir_device_by_adapter(struct i2c_adapter *adapter)
{
- int i;
- struct IR *ir = NULL;
+ struct IR *ir;
- for (i = 0; i < MAX_IRCTL_DEVICES; i++)
- if (ir_devices[i] != NULL &&
- ir_devices[i]->adapter == adapter) {
- ir = ir_devices[i];
- break;
- }
+ if (list_empty(&ir_devices_list))
+ return NULL;
+
+ list_for_each_entry(ir, &ir_devices_list, list)
+ if (ir->adapter == adapter)
+ return ir;
- return ir;
+ return NULL;
}
static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
@@ -1284,6 +1287,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
goto out_no_ir;
}
/* store for use in ir_probe() again, and open() later on */
+ INIT_LIST_HEAD(&ir->list);
ret = add_ir_device(ir);
if (ret)
goto out_free_ir;
@@ -1421,7 +1425,6 @@ static int __init zilog_init(void)
zilog_notify("Zilog/Hauppauge IR driver initializing\n");
mutex_init(&tx_data_lock);
- mutex_init(&ir_devices_lock);
request_module("firmware_class");
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 04/13] lirc_zilog: Convert the instance open count to an atomic_t
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
` (2 preceding siblings ...)
2011-02-18 1:14 ` [PATCH 03/13] lirc_zilog: Convert ir_device instance array to a linked list Andy Walls
@ 2011-02-18 1:15 ` Andy Walls
2011-02-18 1:15 ` [PATCH 05/13] lirc_zilog: Use kernel standard methods for marking device non-seekable Andy Walls
` (9 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:15 UTC (permalink / raw)
To: linux-media
The open count is simply used for deciding if the Rx polling thread
needs to poll the IR chip for userspace. Simplify the manipulation
of the open count by using an atomic_t and not requiring a lock
The polling thread errantly didn't try to take the lock anyway.
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 15 +++++----------
1 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 39f7b53..c857b99 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -94,7 +94,7 @@ struct IR {
struct lirc_driver l;
struct mutex ir_lock;
- int open;
+ atomic_t open_count;
struct i2c_adapter *adapter;
struct IR_rx *rx;
@@ -279,7 +279,7 @@ static int lirc_thread(void *arg)
set_current_state(TASK_INTERRUPTIBLE);
/* if device not opened, we can sleep half a second */
- if (!ir->open) {
+ if (atomic_read(&ir->open_count) == 0) {
schedule_timeout(HZ/2);
continue;
}
@@ -1094,10 +1094,7 @@ static int open(struct inode *node, struct file *filep)
if (ir == NULL)
return -ENODEV;
- /* increment in use count */
- mutex_lock(&ir->ir_lock);
- ++ir->open;
- mutex_unlock(&ir->ir_lock);
+ atomic_inc(&ir->open_count);
/* stash our IR struct */
filep->private_data = ir;
@@ -1115,10 +1112,7 @@ static int close(struct inode *node, struct file *filep)
return -ENODEV;
}
- /* decrement in use count */
- mutex_lock(&ir->ir_lock);
- --ir->open;
- mutex_unlock(&ir->ir_lock);
+ atomic_dec(&ir->open_count);
return 0;
}
@@ -1294,6 +1288,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
ir->adapter = adap;
mutex_init(&ir->ir_lock);
+ atomic_set(&ir->open_count, 0);
/* set lirc_dev stuff */
memcpy(&ir->l, &lirc_template, sizeof(struct lirc_driver));
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 05/13] lirc_zilog: Use kernel standard methods for marking device non-seekable
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
` (3 preceding siblings ...)
2011-02-18 1:15 ` [PATCH 04/13] lirc_zilog: Convert the instance open count to an atomic_t Andy Walls
@ 2011-02-18 1:15 ` Andy Walls
2011-02-18 1:16 ` [PATCH 06/13] lirc_zilog: Don't acquire the rx->buf_lock in the poll() function Andy Walls
` (8 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:15 UTC (permalink / raw)
To: linux-media
lirc_zilog had its own llseek stub that returned -ESPIPE. Get rid of
it and use the kernel's no_llseek() and nonseekable_open() functions
instead.
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 9 ++-------
1 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index c857b99..720ef67 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -712,12 +712,6 @@ static int tx_init(struct IR_tx *tx)
return 0;
}
-/* do nothing stub to make LIRC happy */
-static loff_t lseek(struct file *filep, loff_t offset, int orig)
-{
- return -ESPIPE;
-}
-
/* copied from lirc_dev */
static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos)
{
@@ -1099,6 +1093,7 @@ static int open(struct inode *node, struct file *filep)
/* stash our IR struct */
filep->private_data = ir;
+ nonseekable_open(node, filep);
return 0;
}
@@ -1150,7 +1145,7 @@ static struct i2c_driver driver = {
static const struct file_operations lirc_fops = {
.owner = THIS_MODULE,
- .llseek = lseek,
+ .llseek = no_llseek,
.read = read,
.write = write,
.poll = poll,
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 06/13] lirc_zilog: Don't acquire the rx->buf_lock in the poll() function
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
` (4 preceding siblings ...)
2011-02-18 1:15 ` [PATCH 05/13] lirc_zilog: Use kernel standard methods for marking device non-seekable Andy Walls
@ 2011-02-18 1:16 ` Andy Walls
2011-02-18 1:17 ` [PATCH 07/13] lirc_zilog: Remove unneeded rx->buf_lock Andy Walls
` (7 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:16 UTC (permalink / raw)
To: linux-media
There is no need to take the rx->buf_lock in the the poll() function
as all the underling calls made on objects in the rx->buf lirc_buffer object
are protected by spinlocks.
Corrected a bad error return value in poll(): return POLLERR instead
of -ENODEV.
Added some comments to poll() for when, in the future, I forget what
poll() and poll_wait() are supposed to do.
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 21 ++++++++++++++-------
1 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 720ef67..dfa6a42 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -985,19 +985,26 @@ static unsigned int poll(struct file *filep, poll_table *wait)
unsigned int ret;
dprintk("poll called\n");
- if (rx == NULL)
- return -ENODEV;
- mutex_lock(&rx->buf_lock);
+ if (rx == NULL) {
+ /*
+ * Revisit this, if our poll function ever reports writeable
+ * status for Tx
+ */
+ dprintk("poll result = POLLERR\n");
+ return POLLERR;
+ }
+ /*
+ * Add our lirc_buffer's wait_queue to the poll_table. A wake up on
+ * that buffer's wait queue indicates we may have a new poll status.
+ */
poll_wait(filep, &rx->buf.wait_poll, wait);
- dprintk("poll result = %s\n",
- lirc_buffer_empty(&rx->buf) ? "0" : "POLLIN|POLLRDNORM");
-
+ /* Indicate what ops could happen immediately without blocking */
ret = lirc_buffer_empty(&rx->buf) ? 0 : (POLLIN|POLLRDNORM);
- mutex_unlock(&rx->buf_lock);
+ dprintk("poll result = %s\n", ret ? "POLLIN|POLLRDNORM" : 0);
return ret;
}
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 07/13] lirc_zilog: Remove unneeded rx->buf_lock
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
` (5 preceding siblings ...)
2011-02-18 1:16 ` [PATCH 06/13] lirc_zilog: Don't acquire the rx->buf_lock in the poll() function Andy Walls
@ 2011-02-18 1:17 ` Andy Walls
2011-02-18 1:18 ` [PATCH 08/13] lirc_zilog: Always allocate a Rx lirc_buffer object Andy Walls
` (6 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:17 UTC (permalink / raw)
To: linux-media
Remove the rx->buf_lock that protected the rx->buf lirc_buffer. The
underlying operations on the objects within the lirc_buffer are already
protected by spinlocks, or the objects are constant (e.g. chunk_size).
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 23 +++++++++--------------
1 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index dfa6a42..0f2fa58 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -67,9 +67,8 @@ struct IR_rx {
/* RX device */
struct i2c_client *c;
- /* RX device buffer & lock */
+ /* RX device buffer */
struct lirc_buffer buf;
- struct mutex buf_lock;
/* RX polling thread data */
struct task_struct *task;
@@ -718,18 +717,15 @@ static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos)
struct IR *ir = filep->private_data;
struct IR_rx *rx = ir->rx;
int ret = 0, written = 0;
+ unsigned int m;
DECLARE_WAITQUEUE(wait, current);
dprintk("read called\n");
if (rx == NULL)
return -ENODEV;
- if (mutex_lock_interruptible(&rx->buf_lock))
- return -ERESTARTSYS;
-
if (n % rx->buf.chunk_size) {
dprintk("read result = -EINVAL\n");
- mutex_unlock(&rx->buf_lock);
return -EINVAL;
}
@@ -767,19 +763,19 @@ static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos)
set_current_state(TASK_INTERRUPTIBLE);
} else {
unsigned char buf[rx->buf.chunk_size];
- lirc_buffer_read(&rx->buf, buf);
- ret = copy_to_user((void *)outbuf+written, buf,
- rx->buf.chunk_size);
- written += rx->buf.chunk_size;
+ m = lirc_buffer_read(&rx->buf, buf);
+ if (m == rx->buf.chunk_size) {
+ ret = copy_to_user((void *)outbuf+written, buf,
+ rx->buf.chunk_size);
+ written += rx->buf.chunk_size;
+ }
}
}
remove_wait_queue(&rx->buf.wait_poll, &wait);
set_current_state(TASK_RUNNING);
- mutex_unlock(&rx->buf_lock);
- dprintk("read result = %s (%d)\n",
- ret ? "-EFAULT" : "OK", ret);
+ dprintk("read result = %d (%s)\n", ret, ret ? "Error" : "OK");
return ret ? ret : written;
}
@@ -1327,7 +1323,6 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (ret)
goto out_free_xx;
- mutex_init(&ir->rx->buf_lock);
ir->rx->c = client;
ir->rx->hdpvr_data_fmt =
(id->driver_data & ID_FLAG_HDPVR) ? true : false;
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 08/13] lirc_zilog: Always allocate a Rx lirc_buffer object
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
` (6 preceding siblings ...)
2011-02-18 1:17 ` [PATCH 07/13] lirc_zilog: Remove unneeded rx->buf_lock Andy Walls
@ 2011-02-18 1:18 ` Andy Walls
2011-02-18 1:19 ` [PATCH 09/13] lirc_zilog: Move constants from ir_probe() into the lirc_driver template Andy Walls
` (5 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:18 UTC (permalink / raw)
To: linux-media
Always allocate a lirc_buffer object, instead of just upon setup of
the Rx i2c_client. If we do not allocate a lirc_buffer object, because
we are not handling the Rx i2c_client, lirc_dev will allocate its own
lirc_buffer anyway and not tell us about its location.
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 62 ++++++++++++++++++------------------
1 files changed, 31 insertions(+), 31 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 0f2fa58..a94b10a 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -67,9 +67,6 @@ struct IR_rx {
/* RX device */
struct i2c_client *c;
- /* RX device buffer */
- struct lirc_buffer buf;
-
/* RX polling thread data */
struct task_struct *task;
@@ -91,6 +88,7 @@ struct IR {
struct list_head list;
struct lirc_driver l;
+ struct lirc_buffer rbuf;
struct mutex ir_lock;
atomic_t open_count;
@@ -157,12 +155,13 @@ 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 IR_rx *rx = ir->rx;
if (rx == NULL)
return -ENXIO;
- if (lirc_buffer_full(&rx->buf)) {
+ if (lirc_buffer_full(rbuf)) {
dprintk("buffer overflow\n");
return -EOVERFLOW;
}
@@ -250,9 +249,9 @@ static int add_to_buf(struct IR *ir)
codes[1] = code & 0xff;
/* return it */
- lirc_buffer_write(&rx->buf, codes);
+ lirc_buffer_write(rbuf, codes);
++got_data;
- } while (!lirc_buffer_full(&rx->buf));
+ } while (!lirc_buffer_full(rbuf));
return 0;
}
@@ -270,7 +269,7 @@ static int add_to_buf(struct IR *ir)
static int lirc_thread(void *arg)
{
struct IR *ir = arg;
- struct IR_rx *rx = ir->rx;
+ struct lirc_buffer *rbuf = ir->l.rbuf;
dprintk("poll thread started\n");
@@ -297,7 +296,7 @@ static int lirc_thread(void *arg)
if (kthread_should_stop())
break;
if (!add_to_buf(ir))
- wake_up_interruptible(&rx->buf.wait_poll);
+ wake_up_interruptible(&rbuf->wait_poll);
}
dprintk("poll thread ended\n");
@@ -716,6 +715,7 @@ static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos)
{
struct IR *ir = filep->private_data;
struct IR_rx *rx = ir->rx;
+ struct lirc_buffer *rbuf = ir->l.rbuf;
int ret = 0, written = 0;
unsigned int m;
DECLARE_WAITQUEUE(wait, current);
@@ -724,7 +724,7 @@ static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos)
if (rx == NULL)
return -ENODEV;
- if (n % rx->buf.chunk_size) {
+ if (n % rbuf->chunk_size) {
dprintk("read result = -EINVAL\n");
return -EINVAL;
}
@@ -734,7 +734,7 @@ static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos)
* to avoid losing scan code (in case when queue is awaken somewhere
* between while condition checking and scheduling)
*/
- add_wait_queue(&rx->buf.wait_poll, &wait);
+ add_wait_queue(&rbuf->wait_poll, &wait);
set_current_state(TASK_INTERRUPTIBLE);
/*
@@ -742,7 +742,7 @@ static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos)
* mode and 'copy_to_user' is happy, wait for data.
*/
while (written < n && ret == 0) {
- if (lirc_buffer_empty(&rx->buf)) {
+ if (lirc_buffer_empty(rbuf)) {
/*
* According to the read(2) man page, 'written' can be
* returned as less than 'n', instead of blocking
@@ -762,17 +762,17 @@ static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos)
schedule();
set_current_state(TASK_INTERRUPTIBLE);
} else {
- unsigned char buf[rx->buf.chunk_size];
- m = lirc_buffer_read(&rx->buf, buf);
- if (m == rx->buf.chunk_size) {
+ unsigned char buf[rbuf->chunk_size];
+ m = lirc_buffer_read(rbuf, buf);
+ if (m == rbuf->chunk_size) {
ret = copy_to_user((void *)outbuf+written, buf,
- rx->buf.chunk_size);
- written += rx->buf.chunk_size;
+ rbuf->chunk_size);
+ written += rbuf->chunk_size;
}
}
}
- remove_wait_queue(&rx->buf.wait_poll, &wait);
+ remove_wait_queue(&rbuf->wait_poll, &wait);
set_current_state(TASK_RUNNING);
dprintk("read result = %d (%s)\n", ret, ret ? "Error" : "OK");
@@ -978,6 +978,7 @@ static unsigned int poll(struct file *filep, poll_table *wait)
{
struct IR *ir = filep->private_data;
struct IR_rx *rx = ir->rx;
+ struct lirc_buffer *rbuf = ir->l.rbuf;
unsigned int ret;
dprintk("poll called\n");
@@ -995,10 +996,10 @@ static unsigned int poll(struct file *filep, poll_table *wait)
* Add our lirc_buffer's wait_queue to the poll_table. A wake up on
* that buffer's wait queue indicates we may have a new poll status.
*/
- poll_wait(filep, &rx->buf.wait_poll, wait);
+ poll_wait(filep, &rbuf->wait_poll, wait);
/* Indicate what ops could happen immediately without blocking */
- ret = lirc_buffer_empty(&rx->buf) ? 0 : (POLLIN|POLLRDNORM);
+ ret = lirc_buffer_empty(rbuf) ? 0 : (POLLIN|POLLRDNORM);
dprintk("poll result = %s\n", ret ? "POLLIN|POLLRDNORM" : 0);
return ret;
@@ -1209,8 +1210,6 @@ static int ir_remove(struct i2c_client *client)
/* Good-bye Rx */
destroy_rx_kthread(ir->rx);
if (ir->rx != NULL) {
- if (ir->rx->buf.fifo_initialized)
- lirc_buffer_free(&ir->rx->buf);
i2c_set_clientdata(ir->rx->c, NULL);
kfree(ir->rx);
}
@@ -1222,6 +1221,8 @@ static int ir_remove(struct i2c_client *client)
}
/* Good-bye IR */
+ if (ir->rbuf.fifo_initialized)
+ lirc_buffer_free(&ir->rbuf);
del_ir_device(ir);
kfree(ir);
@@ -1292,11 +1293,17 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
memcpy(&ir->l, &lirc_template, sizeof(struct lirc_driver));
ir->l.minor = minor; /* module option */
ir->l.code_length = 13;
- ir->l.rbuf = NULL;
+ ir->l.chunk_size = 2;
+ ir->l.buffer_size = BUFLEN / 2;
+ ir->l.rbuf = &ir->rbuf;
ir->l.fops = &lirc_fops;
ir->l.data = ir;
ir->l.dev = &adap->dev;
ir->l.sample_rate = 0;
+ ret = lirc_buffer_init(ir->l.rbuf,
+ ir->l.chunk_size, ir->l.buffer_size);
+ if (ret)
+ goto out_free_ir;
}
if (tx_probe) {
@@ -1319,16 +1326,9 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
goto out_free_xx;
}
- ret = lirc_buffer_init(&ir->rx->buf, 2, BUFLEN / 2);
- if (ret)
- goto out_free_xx;
-
ir->rx->c = client;
ir->rx->hdpvr_data_fmt =
(id->driver_data & ID_FLAG_HDPVR) ? true : false;
-
- /* set lirc_dev stuff */
- ir->l.rbuf = &ir->rx->buf;
}
i2c_set_clientdata(client, ir);
@@ -1388,8 +1388,6 @@ out_free_thread:
destroy_rx_kthread(ir->rx);
out_free_xx:
if (ir->rx != NULL) {
- if (ir->rx->buf.fifo_initialized)
- lirc_buffer_free(&ir->rx->buf);
if (ir->rx->c != NULL)
i2c_set_clientdata(ir->rx->c, NULL);
kfree(ir->rx);
@@ -1399,6 +1397,8 @@ out_free_xx:
i2c_set_clientdata(ir->tx->c, NULL);
kfree(ir->tx);
}
+ if (ir->rbuf.fifo_initialized)
+ lirc_buffer_free(&ir->rbuf);
out_free_ir:
del_ir_device(ir);
kfree(ir);
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 09/13] lirc_zilog: Move constants from ir_probe() into the lirc_driver template
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
` (7 preceding siblings ...)
2011-02-18 1:18 ` [PATCH 08/13] lirc_zilog: Always allocate a Rx lirc_buffer object Andy Walls
@ 2011-02-18 1:19 ` Andy Walls
2011-02-18 1:20 ` [PATCH 10/13] lirc_zilog: Add ref counting of struct IR, IR_tx, and IR_rx Andy Walls
` (4 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:19 UTC (permalink / raw)
To: linux-media
ir_probe() makes a number of constant assignments into the lirc_driver
object after copying in a template. Make better use of the template.
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 27 +++++++++++++++------------
1 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index a94b10a..8ab60e9 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -1116,13 +1116,6 @@ static int close(struct inode *node, struct file *filep)
return 0;
}
-static struct lirc_driver lirc_template = {
- .name = "lirc_zilog",
- .set_use_inc = set_use_inc,
- .set_use_dec = set_use_dec,
- .owner = THIS_MODULE
-};
-
static int ir_remove(struct i2c_client *client);
static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id);
@@ -1161,6 +1154,19 @@ static const struct file_operations lirc_fops = {
.release = close
};
+static struct lirc_driver lirc_template = {
+ .name = "lirc_zilog",
+ .minor = -1,
+ .code_length = 13,
+ .buffer_size = BUFLEN / 2,
+ .sample_rate = 0, /* tell lirc_dev to not start its own kthread */
+ .chunk_size = 2,
+ .set_use_inc = set_use_inc,
+ .set_use_dec = set_use_dec,
+ .fops = &lirc_fops,
+ .owner = THIS_MODULE,
+};
+
static void destroy_rx_kthread(struct IR_rx *rx)
{
/* end up polling thread */
@@ -1292,14 +1298,9 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
/* set lirc_dev stuff */
memcpy(&ir->l, &lirc_template, sizeof(struct lirc_driver));
ir->l.minor = minor; /* module option */
- ir->l.code_length = 13;
- ir->l.chunk_size = 2;
- ir->l.buffer_size = BUFLEN / 2;
ir->l.rbuf = &ir->rbuf;
- ir->l.fops = &lirc_fops;
ir->l.data = ir;
ir->l.dev = &adap->dev;
- ir->l.sample_rate = 0;
ret = lirc_buffer_init(ir->l.rbuf,
ir->l.chunk_size, ir->l.buffer_size);
if (ret)
@@ -1314,6 +1315,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
goto out_free_xx;
}
+ ir->l.features |= LIRC_CAN_SEND_PULSE;
ir->tx->c = client;
ir->tx->need_boot = 1;
ir->tx->post_tx_ready_poll =
@@ -1326,6 +1328,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
goto out_free_xx;
}
+ ir->l.features |= LIRC_CAN_REC_LIRCCODE;
ir->rx->c = client;
ir->rx->hdpvr_data_fmt =
(id->driver_data & ID_FLAG_HDPVR) ? true : false;
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 10/13] lirc_zilog: Add ref counting of struct IR, IR_tx, and IR_rx
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
` (8 preceding siblings ...)
2011-02-18 1:19 ` [PATCH 09/13] lirc_zilog: Move constants from ir_probe() into the lirc_driver template Andy Walls
@ 2011-02-18 1:20 ` Andy Walls
2011-02-18 1:20 ` [PATCH 11/13] lirc_zilog: Add locking of the i2c_clients when in use Andy Walls
` (3 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:20 UTC (permalink / raw)
To: linux-media
This is a major change to add pointer reference counting for
struct IR, struct IR_tx, and struct IR_rx object instances.
This ref counting gets lirc_zilog closer to gracefully handling
bridge drivers and hot-unplugged USB devices disappearing out from
under lirc_zilog when the /dev/lircN node is still open. (mutexes
to protect the i2c_client pointers in struct IR_tx and struct IR_rx
still need to be added.)
This reference counting also helps lirc_zilog clean up properly
when the i2c_clients disappear.
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 582 ++++++++++++++++++++++++-------------
1 files changed, 380 insertions(+), 202 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 8ab60e9..755cb39 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -63,8 +63,14 @@
#include <media/lirc_dev.h>
#include <media/lirc.h>
+struct IR;
+
struct IR_rx {
+ struct kref ref;
+ struct IR *ir;
+
/* RX device */
+ /* FIXME mutex lock access to this pointer */
struct i2c_client *c;
/* RX polling thread data */
@@ -76,7 +82,11 @@ struct IR_rx {
};
struct IR_tx {
+ struct kref ref;
+ struct IR *ir;
+
/* TX device */
+ /* FIXME mutex lock access to this pointer */
struct i2c_client *c;
/* TX additional actions needed */
@@ -85,8 +95,10 @@ struct IR_tx {
};
struct IR {
+ struct kref ref;
struct list_head list;
+ /* FIXME spinlock access to l.features */
struct lirc_driver l;
struct lirc_buffer rbuf;
@@ -94,11 +106,21 @@ struct IR {
atomic_t open_count;
struct i2c_adapter *adapter;
+
+ spinlock_t rx_ref_lock; /* struct IR_rx kref get()/put() */
struct IR_rx *rx;
+
+ spinlock_t tx_ref_lock; /* struct IR_tx kref get()/put() */
struct IR_tx *tx;
};
/* IR transceiver instance object list */
+/*
+ * This lock is used for the following:
+ * a. ir_devices_list access, insertions, deletions
+ * b. struct IR kref get()s and put()s
+ * c. serialization of ir_probe() for the two i2c_clients for a Z8
+ */
static DEFINE_MUTEX(ir_devices_lock);
static LIST_HEAD(ir_devices_list);
@@ -146,6 +168,157 @@ static int minor = -1; /* minor number */
## args); \
} while (0)
+
+/* struct IR reference counting */
+static struct IR *get_ir_device(struct IR *ir, bool ir_devices_lock_held)
+{
+ if (ir_devices_lock_held) {
+ kref_get(&ir->ref);
+ } else {
+ mutex_lock(&ir_devices_lock);
+ kref_get(&ir->ref);
+ mutex_unlock(&ir_devices_lock);
+ }
+ return ir;
+}
+
+static void release_ir_device(struct kref *ref)
+{
+ struct IR *ir = container_of(ref, struct IR, ref);
+
+ /*
+ * Things should be in this state by now:
+ * ir->rx set to NULL and deallocated - happens before ir->rx->ir put()
+ * ir->rx->task kthread stopped - happens before ir->rx->ir put()
+ * ir->tx set to NULL and deallocated - happens before ir->tx->ir put()
+ * ir->open_count == 0 - happens on final close()
+ * ir_lock, tx_ref_lock, rx_ref_lock, all released
+ */
+ if (ir->l.minor >= 0 && ir->l.minor < MAX_IRCTL_DEVICES) {
+ lirc_unregister_driver(ir->l.minor);
+ ir->l.minor = MAX_IRCTL_DEVICES;
+ }
+ if (ir->rbuf.fifo_initialized)
+ lirc_buffer_free(&ir->rbuf);
+ list_del(&ir->list);
+ kfree(ir);
+}
+
+static int put_ir_device(struct IR *ir, bool ir_devices_lock_held)
+{
+ int released;
+
+ if (ir_devices_lock_held)
+ return kref_put(&ir->ref, release_ir_device);
+
+ mutex_lock(&ir_devices_lock);
+ released = kref_put(&ir->ref, release_ir_device);
+ mutex_unlock(&ir_devices_lock);
+
+ return released;
+}
+
+/* struct IR_rx reference counting */
+static struct IR_rx *get_ir_rx(struct IR *ir)
+{
+ struct IR_rx *rx;
+
+ spin_lock(&ir->rx_ref_lock);
+ rx = ir->rx;
+ if (rx != NULL)
+ kref_get(&rx->ref);
+ spin_unlock(&ir->rx_ref_lock);
+ return rx;
+}
+
+static void destroy_rx_kthread(struct IR_rx *rx, bool ir_devices_lock_held)
+{
+ /* end up polling thread */
+ if (!IS_ERR_OR_NULL(rx->task)) {
+ kthread_stop(rx->task);
+ rx->task = NULL;
+ /* Put the ir ptr that ir_probe() gave to the rx poll thread */
+ put_ir_device(rx->ir, ir_devices_lock_held);
+ }
+}
+
+static void release_ir_rx(struct kref *ref)
+{
+ struct IR_rx *rx = container_of(ref, struct IR_rx, ref);
+ struct IR *ir = rx->ir;
+
+ /*
+ * This release function can't do all the work, as we want
+ * to keep the rx_ref_lock a spinlock, and killing the poll thread
+ * and releasing the ir reference can cause a sleep. That work is
+ * performed by put_ir_rx()
+ */
+ 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 */
+ return;
+}
+
+static int put_ir_rx(struct IR_rx *rx, bool ir_devices_lock_held)
+{
+ int released;
+ struct IR *ir = rx->ir;
+
+ spin_lock(&ir->rx_ref_lock);
+ released = kref_put(&rx->ref, release_ir_rx);
+ spin_unlock(&ir->rx_ref_lock);
+ /* Destroy the rx kthread while not holding the spinlock */
+ if (released) {
+ destroy_rx_kthread(rx, ir_devices_lock_held);
+ kfree(rx);
+ /* Make sure we're not still in a poll_table somewhere */
+ wake_up_interruptible(&ir->rbuf.wait_poll);
+ }
+ /* Do a reference put() for the rx->ir reference, if we released rx */
+ if (released)
+ put_ir_device(ir, ir_devices_lock_held);
+ return released;
+}
+
+/* struct IR_tx reference counting */
+static struct IR_tx *get_ir_tx(struct IR *ir)
+{
+ struct IR_tx *tx;
+
+ spin_lock(&ir->tx_ref_lock);
+ tx = ir->tx;
+ if (tx != NULL)
+ kref_get(&tx->ref);
+ spin_unlock(&ir->tx_ref_lock);
+ return tx;
+}
+
+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;
+ /* Don't put_ir_device(tx->ir) here, so our lock doesn't get freed */
+ ir->tx = NULL;
+ kfree(tx);
+}
+
+static int put_ir_tx(struct IR_tx *tx, bool ir_devices_lock_held)
+{
+ int released;
+ struct IR *ir = tx->ir;
+
+ spin_lock(&ir->tx_ref_lock);
+ released = kref_put(&tx->ref, release_ir_tx);
+ spin_unlock(&ir->tx_ref_lock);
+ /* Do a reference put() for the tx->ir reference, if we released tx */
+ if (released)
+ put_ir_device(ir, ir_devices_lock_held);
+ return released;
+}
+
static int add_to_buf(struct IR *ir)
{
__u16 code;
@@ -156,23 +329,29 @@ static int add_to_buf(struct IR *ir)
int failures = 0;
unsigned char sendbuf[1] = { 0 };
struct lirc_buffer *rbuf = ir->l.rbuf;
- struct IR_rx *rx = ir->rx;
-
- if (rx == NULL)
- return -ENXIO;
+ struct IR_rx *rx;
+ struct IR_tx *tx;
if (lirc_buffer_full(rbuf)) {
dprintk("buffer overflow\n");
return -EOVERFLOW;
}
+ rx = get_ir_rx(ir);
+ if (rx == NULL)
+ return -ENXIO;
+
+ tx = get_ir_tx(ir);
+
/*
* service the device as long as it is returning
* data and we have space
*/
do {
- if (kthread_should_stop())
- return -ENODATA;
+ if (kthread_should_stop()) {
+ ret = -ENODATA;
+ break;
+ }
/*
* Lock i2c bus for the duration. RX/TX chips interfere so
@@ -182,7 +361,8 @@ static int add_to_buf(struct IR *ir)
if (kthread_should_stop()) {
mutex_unlock(&ir->ir_lock);
- return -ENODATA;
+ ret = -ENODATA;
+ break;
}
/*
@@ -196,7 +376,7 @@ static int add_to_buf(struct IR *ir)
mutex_unlock(&ir->ir_lock);
zilog_error("unable to read from the IR chip "
"after 3 resets, giving up\n");
- return ret;
+ break;
}
/* Looks like the chip crashed, reset it */
@@ -206,20 +386,23 @@ static int add_to_buf(struct IR *ir)
set_current_state(TASK_UNINTERRUPTIBLE);
if (kthread_should_stop()) {
mutex_unlock(&ir->ir_lock);
- return -ENODATA;
+ ret = -ENODATA;
+ break;
}
schedule_timeout((100 * HZ + 999) / 1000);
- if (ir->tx != NULL)
- ir->tx->need_boot = 1;
+ if (tx != NULL)
+ tx->need_boot = 1;
++failures;
mutex_unlock(&ir->ir_lock);
+ ret = 0;
continue;
}
if (kthread_should_stop()) {
mutex_unlock(&ir->ir_lock);
- return -ENODATA;
+ ret = -ENODATA;
+ break;
}
ret = i2c_master_recv(rx->c, keybuf, sizeof(keybuf));
mutex_unlock(&ir->ir_lock);
@@ -235,12 +418,17 @@ static int add_to_buf(struct IR *ir)
/* key pressed ? */
if (rx->hdpvr_data_fmt) {
- if (got_data && (keybuf[0] == 0x80))
- return 0;
- else if (got_data && (keybuf[0] == 0x00))
- return -ENODATA;
- } else if ((rx->b[0] & 0x80) == 0)
- return got_data ? 0 : -ENODATA;
+ if (got_data && (keybuf[0] == 0x80)) {
+ ret = 0;
+ break;
+ } else if (got_data && (keybuf[0] == 0x00)) {
+ ret = -ENODATA;
+ break;
+ }
+ } else if ((rx->b[0] & 0x80) == 0) {
+ ret = got_data ? 0 : -ENODATA;
+ break;
+ }
/* look what we have */
code = (((__u16)rx->b[0] & 0x7f) << 6) | (rx->b[1] >> 2);
@@ -251,9 +439,13 @@ static int add_to_buf(struct IR *ir)
/* return it */
lirc_buffer_write(rbuf, codes);
++got_data;
+ ret = 0;
} while (!lirc_buffer_full(rbuf));
- return 0;
+ if (tx != NULL)
+ put_ir_tx(tx, false);
+ put_ir_rx(rx, false);
+ return ret;
}
/*
@@ -564,7 +756,7 @@ static int fw_load(struct IR_tx *tx)
}
/* Request codeset data file */
- ret = request_firmware(&fw_entry, "haup-ir-blaster.bin", &tx->c->dev);
+ ret = request_firmware(&fw_entry, "haup-ir-blaster.bin", tx->ir->l.dev);
if (ret != 0) {
zilog_error("firmware haup-ir-blaster.bin not available "
"(%d)\n", ret);
@@ -690,45 +882,26 @@ out:
return ret;
}
-/* initialise the IR TX device */
-static int tx_init(struct IR_tx *tx)
-{
- int ret;
-
- /* Load 'firmware' */
- ret = fw_load(tx);
- if (ret != 0)
- return ret;
-
- /* Send boot block */
- ret = send_boot_data(tx);
- if (ret != 0)
- return ret;
- tx->need_boot = 0;
-
- /* Looks good */
- return 0;
-}
-
/* copied from lirc_dev */
static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos)
{
struct IR *ir = filep->private_data;
- struct IR_rx *rx = ir->rx;
+ struct IR_rx *rx;
struct lirc_buffer *rbuf = ir->l.rbuf;
int ret = 0, written = 0;
unsigned int m;
DECLARE_WAITQUEUE(wait, current);
dprintk("read called\n");
- if (rx == NULL)
- return -ENODEV;
-
if (n % rbuf->chunk_size) {
dprintk("read result = -EINVAL\n");
return -EINVAL;
}
+ rx = get_ir_rx(ir);
+ if (rx == NULL)
+ return -ENXIO;
+
/*
* we add ourselves to the task queue before buffer check
* to avoid losing scan code (in case when queue is awaken somewhere
@@ -773,6 +946,7 @@ static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos)
}
remove_wait_queue(&rbuf->wait_poll, &wait);
+ put_ir_rx(rx, false);
set_current_state(TASK_RUNNING);
dprintk("read result = %d (%s)\n", ret, ret ? "Error" : "OK");
@@ -902,17 +1076,19 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
loff_t *ppos)
{
struct IR *ir = filep->private_data;
- struct IR_tx *tx = ir->tx;
+ struct IR_tx *tx;
size_t i;
int failures = 0;
- if (tx == NULL)
- return -ENODEV;
-
/* Validate user parameters */
if (n % sizeof(int))
return -EINVAL;
+ /* Get a struct IR_tx reference */
+ tx = get_ir_tx(ir);
+ if (tx == NULL)
+ return -ENXIO;
+
/* Lock i2c bus for the duration */
mutex_lock(&ir->ir_lock);
@@ -923,11 +1099,22 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
if (copy_from_user(&command, buf + i, sizeof(command))) {
mutex_unlock(&ir->ir_lock);
+ put_ir_tx(tx, false);
return -EFAULT;
}
/* Send boot data first if required */
if (tx->need_boot == 1) {
+ /* Make sure we have the 'firmware' loaded, first */
+ ret = fw_load(tx);
+ if (ret != 0) {
+ mutex_unlock(&ir->ir_lock);
+ put_ir_tx(tx, false);
+ if (ret != -ENOMEM)
+ ret = -EIO;
+ return ret;
+ }
+ /* Prep the chip for transmitting codes */
ret = send_boot_data(tx);
if (ret == 0)
tx->need_boot = 0;
@@ -939,6 +1126,7 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
(unsigned)command & 0xFFFF);
if (ret == -EPROTO) {
mutex_unlock(&ir->ir_lock);
+ put_ir_tx(tx, false);
return ret;
}
}
@@ -956,6 +1144,7 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
zilog_error("unable to send to the IR chip "
"after 3 resets, giving up\n");
mutex_unlock(&ir->ir_lock);
+ put_ir_tx(tx, false);
return ret;
}
set_current_state(TASK_UNINTERRUPTIBLE);
@@ -969,6 +1158,9 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
/* Release i2c bus */
mutex_unlock(&ir->ir_lock);
+ /* Give back our struct IR_tx reference */
+ put_ir_tx(tx, false);
+
/* All looks good */
return n;
}
@@ -977,12 +1169,13 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
static unsigned int poll(struct file *filep, poll_table *wait)
{
struct IR *ir = filep->private_data;
- struct IR_rx *rx = ir->rx;
+ struct IR_rx *rx;
struct lirc_buffer *rbuf = ir->l.rbuf;
unsigned int ret;
dprintk("poll called\n");
+ rx = get_ir_rx(ir);
if (rx == NULL) {
/*
* Revisit this, if our poll function ever reports writeable
@@ -1001,6 +1194,8 @@ 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);
+ put_ir_rx(rx, false);
+
dprintk("poll result = %s\n", ret ? "POLLIN|POLLRDNORM" : 0);
return ret;
}
@@ -1009,12 +1204,9 @@ static long ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
{
struct IR *ir = filep->private_data;
int result;
- unsigned long mode, features = 0;
+ unsigned long mode, features;
- if (ir->rx != NULL)
- features |= LIRC_CAN_REC_LIRCCODE;
- if (ir->tx != NULL)
- features |= LIRC_CAN_SEND_PULSE;
+ features = ir->l.features;
switch (cmd) {
case LIRC_GET_LENGTH:
@@ -1060,19 +1252,24 @@ static long ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
return result;
}
-/* ir_devices_lock must be held */
-static struct IR *find_ir_device_by_minor(unsigned int minor)
+static struct IR *get_ir_device_by_minor(unsigned int minor)
{
struct IR *ir;
+ struct IR *ret = NULL;
- if (list_empty(&ir_devices_list))
- return NULL;
+ mutex_lock(&ir_devices_lock);
- list_for_each_entry(ir, &ir_devices_list, list)
- if (ir->l.minor == minor)
- return ir;
+ 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;
+ }
+ }
+ }
- return NULL;
+ mutex_unlock(&ir_devices_lock);
+ return ret;
}
/*
@@ -1085,9 +1282,7 @@ static int open(struct inode *node, struct file *filep)
unsigned int minor = MINOR(node->i_rdev);
/* find our IR struct */
- mutex_lock(&ir_devices_lock);
- ir = find_ir_device_by_minor(minor);
- mutex_unlock(&ir_devices_lock);
+ ir = get_ir_device_by_minor(minor);
if (ir == NULL)
return -ENODEV;
@@ -1113,6 +1308,7 @@ static int close(struct inode *node, struct file *filep)
atomic_dec(&ir->open_count);
+ put_ir_device(ir, false);
return 0;
}
@@ -1167,78 +1363,23 @@ static struct lirc_driver lirc_template = {
.owner = THIS_MODULE,
};
-static void destroy_rx_kthread(struct IR_rx *rx)
-{
- /* end up polling thread */
- if (rx != NULL && !IS_ERR_OR_NULL(rx->task)) {
- kthread_stop(rx->task);
- rx->task = NULL;
- }
-}
-
-/* ir_devices_lock must be held */
-static int add_ir_device(struct IR *ir)
-{
- list_add_tail(&ir->list, &ir_devices_list);
- return 0;
-}
-
-/* ir_devices_lock must be held */
-static void del_ir_device(struct IR *ir)
-{
- struct IR *p;
-
- if (list_empty(&ir_devices_list))
- return;
-
- list_for_each_entry(p, &ir_devices_list, list)
- if (p == ir) {
- list_del(&p->list);
- break;
- }
-}
-
static int ir_remove(struct i2c_client *client)
{
- struct IR *ir = i2c_get_clientdata(client);
-
- mutex_lock(&ir_devices_lock);
-
- if (ir == NULL) {
- /* We destroyed everything when the first client came through */
- mutex_unlock(&ir_devices_lock);
- return 0;
- }
-
- /* Good-bye LIRC */
- lirc_unregister_driver(ir->l.minor);
-
- /* Good-bye Rx */
- destroy_rx_kthread(ir->rx);
- if (ir->rx != NULL) {
- i2c_set_clientdata(ir->rx->c, NULL);
- kfree(ir->rx);
- }
-
- /* Good-bye Tx */
- if (ir->tx != NULL) {
- i2c_set_clientdata(ir->tx->c, NULL);
- kfree(ir->tx);
+ if (strncmp("ir_tx_z8", client->name, 8) == 0) {
+ struct IR_tx *tx = i2c_get_clientdata(client);
+ if (tx != NULL)
+ put_ir_tx(tx, false);
+ } else if (strncmp("ir_rx_z8", client->name, 8) == 0) {
+ struct IR_rx *rx = i2c_get_clientdata(client);
+ if (rx != NULL)
+ put_ir_rx(rx, false);
}
-
- /* Good-bye IR */
- if (ir->rbuf.fifo_initialized)
- lirc_buffer_free(&ir->rbuf);
- del_ir_device(ir);
- kfree(ir);
-
- mutex_unlock(&ir_devices_lock);
return 0;
}
/* ir_devices_lock must be held */
-static struct IR *find_ir_device_by_adapter(struct i2c_adapter *adapter)
+static struct IR *get_ir_device_by_adapter(struct i2c_adapter *adapter)
{
struct IR *ir;
@@ -1246,8 +1387,10 @@ static struct IR *find_ir_device_by_adapter(struct i2c_adapter *adapter)
return NULL;
list_for_each_entry(ir, &ir_devices_list, list)
- if (ir->adapter == adapter)
+ if (ir->adapter == adapter) {
+ get_ir_device(ir, true);
return ir;
+ }
return NULL;
}
@@ -1255,6 +1398,8 @@ static struct IR *find_ir_device_by_adapter(struct i2c_adapter *adapter)
static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
struct IR *ir;
+ struct IR_tx *tx;
+ struct IR_rx *rx;
struct i2c_adapter *adap = client->adapter;
int ret;
bool tx_probe = false;
@@ -1278,133 +1423,166 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
mutex_lock(&ir_devices_lock);
/* Use a single struct IR instance for both the Rx and Tx functions */
- ir = find_ir_device_by_adapter(adap);
+ ir = get_ir_device_by_adapter(adap);
if (ir == NULL) {
ir = kzalloc(sizeof(struct IR), GFP_KERNEL);
if (ir == NULL) {
ret = -ENOMEM;
goto out_no_ir;
}
+ kref_init(&ir->ref);
+
/* store for use in ir_probe() again, and open() later on */
INIT_LIST_HEAD(&ir->list);
- ret = add_ir_device(ir);
- if (ret)
- goto out_free_ir;
+ list_add_tail(&ir->list, &ir_devices_list);
ir->adapter = adap;
mutex_init(&ir->ir_lock);
atomic_set(&ir->open_count, 0);
+ spin_lock_init(&ir->tx_ref_lock);
+ spin_lock_init(&ir->rx_ref_lock);
/* set lirc_dev stuff */
memcpy(&ir->l, &lirc_template, sizeof(struct lirc_driver));
- ir->l.minor = minor; /* module option */
- ir->l.rbuf = &ir->rbuf;
- ir->l.data = ir;
- ir->l.dev = &adap->dev;
+ /*
+ * 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
+ */
+ ir->l.rbuf = &ir->rbuf;
+ ir->l.dev = &adap->dev;
ret = lirc_buffer_init(ir->l.rbuf,
ir->l.chunk_size, ir->l.buffer_size);
if (ret)
- goto out_free_ir;
+ goto out_put_ir;
}
if (tx_probe) {
+ /* Get the IR_rx instance for later, if already allocated */
+ rx = get_ir_rx(ir);
+
/* Set up a struct IR_tx instance */
- ir->tx = kzalloc(sizeof(struct IR_tx), GFP_KERNEL);
- if (ir->tx == NULL) {
+ tx = kzalloc(sizeof(struct IR_tx), GFP_KERNEL);
+ if (tx == NULL) {
ret = -ENOMEM;
- goto out_free_xx;
+ goto out_put_xx;
}
+ kref_init(&tx->ref);
+ ir->tx = tx;
ir->l.features |= LIRC_CAN_SEND_PULSE;
- ir->tx->c = client;
- ir->tx->need_boot = 1;
- ir->tx->post_tx_ready_poll =
+ tx->c = client;
+ tx->need_boot = 1;
+ tx->post_tx_ready_poll =
(id->driver_data & ID_FLAG_HDPVR) ? false : true;
+
+ /* An ir ref goes to the struct IR_tx instance */
+ tx->ir = get_ir_device(ir, true);
+
+ /* A tx ref goes to the i2c_client */
+ i2c_set_clientdata(client, get_ir_tx(ir));
+
+ /*
+ * Load the 'firmware'. We do this before registering with
+ * lirc_dev, so the first firmware load attempt does not happen
+ * after a open() or write() call on the device.
+ *
+ * Failure here is not deemed catastrophic, so the receiver will
+ * still be usable. Firmware load will be retried in write(),
+ * if it is needed.
+ */
+ fw_load(tx);
+
+ /* Proceed only if the Rx client is also ready or not needed */
+ if (rx == NULL && !tx_only) {
+ zilog_info("probe of IR Tx on %s (i2c-%d) done. Waiting"
+ " on IR Rx.\n", adap->name, adap->nr);
+ goto out_ok;
+ }
} else {
+ /* Get the IR_tx instance for later, if already allocated */
+ tx = get_ir_tx(ir);
+
/* Set up a struct IR_rx instance */
- ir->rx = kzalloc(sizeof(struct IR_rx), GFP_KERNEL);
- if (ir->rx == NULL) {
+ rx = kzalloc(sizeof(struct IR_rx), GFP_KERNEL);
+ if (rx == NULL) {
ret = -ENOMEM;
- goto out_free_xx;
+ goto out_put_xx;
}
+ kref_init(&rx->ref);
+ ir->rx = rx;
ir->l.features |= LIRC_CAN_REC_LIRCCODE;
- ir->rx->c = client;
- ir->rx->hdpvr_data_fmt =
+ rx->c = client;
+ rx->hdpvr_data_fmt =
(id->driver_data & ID_FLAG_HDPVR) ? true : false;
- }
- i2c_set_clientdata(client, ir);
+ /* An ir ref goes to the struct IR_rx instance */
+ rx->ir = get_ir_device(ir, true);
- /* Proceed only if we have the required Tx and Rx clients ready to go */
- if (ir->tx == NULL ||
- (ir->rx == NULL && !tx_only)) {
- zilog_info("probe of IR %s on %s (i2c-%d) done. Waiting on "
- "IR %s.\n", tx_probe ? "Tx" : "Rx", adap->name,
- adap->nr, tx_probe ? "Rx" : "Tx");
- goto out_ok;
- }
+ /* An rx ref goes to the i2c_client */
+ i2c_set_clientdata(client, get_ir_rx(ir));
- /* initialise RX device */
- if (ir->rx != NULL) {
- /* try to fire up polling thread */
- ir->rx->task = kthread_run(lirc_thread, ir,
- "zilog-rx-i2c-%d", adap->nr);
- if (IS_ERR(ir->rx->task)) {
- ret = PTR_ERR(ir->rx->task);
+ /*
+ * Start the polling thread.
+ * It will only perform an empty loop around schedule_timeout()
+ * until we register with lirc_dev and the first user open()
+ */
+ /* An ir ref goes to the new rx polling kthread */
+ rx->task = kthread_run(lirc_thread, get_ir_device(ir, true),
+ "zilog-rx-i2c-%d", adap->nr);
+ if (IS_ERR(rx->task)) {
+ ret = PTR_ERR(rx->task);
zilog_error("%s: could not start IR Rx polling thread"
"\n", __func__);
- goto out_free_xx;
+ /* Failed kthread, so put back the ir ref */
+ put_ir_device(ir, true);
+ /* 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;
+ goto out_put_xx;
+ }
+
+ /* Proceed only if the Tx client is also ready */
+ if (tx == NULL) {
+ zilog_info("probe of IR Rx on %s (i2c-%d) done. Waiting"
+ " on IR Tx.\n", adap->name, adap->nr);
+ goto out_ok;
}
}
/* register with lirc */
+ ir->l.minor = minor; /* module option: user requested minor number */
ir->l.minor = lirc_register_driver(&ir->l);
if (ir->l.minor < 0 || ir->l.minor >= MAX_IRCTL_DEVICES) {
zilog_error("%s: \"minor\" must be between 0 and %d (%d)!\n",
__func__, MAX_IRCTL_DEVICES-1, ir->l.minor);
ret = -EBADRQC;
- goto out_free_thread;
- }
-
- /*
- * if we have the tx device, load the 'firmware'. We do this
- * after registering with lirc as otherwise hotplug seems to take
- * 10s to create the lirc device.
- */
- if (ir->tx != NULL) {
- /* Special TX init */
- ret = tx_init(ir->tx);
- if (ret != 0)
- goto out_unregister;
+ goto out_put_xx;
}
+out_ok:
+ if (rx != NULL)
+ put_ir_rx(rx, true);
+ if (tx != NULL)
+ put_ir_tx(tx, true);
+ put_ir_device(ir, true);
zilog_info("probe of IR %s on %s (i2c-%d) done. IR unit ready.\n",
tx_probe ? "Tx" : "Rx", adap->name, adap->nr);
-out_ok:
mutex_unlock(&ir_devices_lock);
return 0;
-out_unregister:
- lirc_unregister_driver(ir->l.minor);
-out_free_thread:
- destroy_rx_kthread(ir->rx);
-out_free_xx:
- if (ir->rx != NULL) {
- if (ir->rx->c != NULL)
- i2c_set_clientdata(ir->rx->c, NULL);
- kfree(ir->rx);
- }
- if (ir->tx != NULL) {
- if (ir->tx->c != NULL)
- i2c_set_clientdata(ir->tx->c, NULL);
- kfree(ir->tx);
- }
- if (ir->rbuf.fifo_initialized)
- lirc_buffer_free(&ir->rbuf);
-out_free_ir:
- del_ir_device(ir);
- kfree(ir);
+out_put_xx:
+ if (rx != NULL)
+ put_ir_rx(rx, true);
+ if (tx != NULL)
+ put_ir_tx(tx, true);
+out_put_ir:
+ put_ir_device(ir, true);
out_no_ir:
zilog_error("%s: probing IR %s on %s (i2c-%d) failed with %d\n",
__func__, tx_probe ? "Tx" : "Rx", adap->name, adap->nr,
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 11/13] lirc_zilog: Add locking of the i2c_clients when in use
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
` (9 preceding siblings ...)
2011-02-18 1:20 ` [PATCH 10/13] lirc_zilog: Add ref counting of struct IR, IR_tx, and IR_rx Andy Walls
@ 2011-02-18 1:20 ` Andy Walls
2011-02-18 1:21 ` [PATCH 12/13] lirc_zilog: Fix somewhat confusing information messages in ir_probe() Andy Walls
` (2 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:20 UTC (permalink / raw)
To: linux-media
Lock the i2c_client pointers and prevent i2c_client removal when
lirc_zilog is perfoming a series of operations that require valid
i2c_client pointers.
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 41 +++++++++++++++++++++++++++++++++---
1 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 755cb39..a59d32d 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -70,7 +70,7 @@ struct IR_rx {
struct IR *ir;
/* RX device */
- /* FIXME mutex lock access to this pointer */
+ struct mutex client_lock;
struct i2c_client *c;
/* RX polling thread data */
@@ -86,7 +86,7 @@ struct IR_tx {
struct IR *ir;
/* TX device */
- /* FIXME mutex lock access to this pointer */
+ struct mutex client_lock;
struct i2c_client *c;
/* TX additional actions needed */
@@ -341,6 +341,14 @@ static int add_to_buf(struct IR *ir)
if (rx == NULL)
return -ENXIO;
+ /* Ensure our rx->c i2c_client remains valid for the duration */
+ mutex_lock(&rx->client_lock);
+ if (rx->c == NULL) {
+ mutex_unlock(&rx->client_lock);
+ put_ir_rx(rx, false);
+ return -ENXIO;
+ }
+
tx = get_ir_tx(ir);
/*
@@ -442,6 +450,7 @@ static int add_to_buf(struct IR *ir)
ret = 0;
} while (!lirc_buffer_full(rbuf));
+ mutex_unlock(&rx->client_lock);
if (tx != NULL)
put_ir_tx(tx, false);
put_ir_rx(rx, false);
@@ -1089,6 +1098,14 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
if (tx == NULL)
return -ENXIO;
+ /* Ensure our tx->c i2c_client remains valid for the duration */
+ mutex_lock(&tx->client_lock);
+ if (tx->c == NULL) {
+ mutex_unlock(&tx->client_lock);
+ put_ir_tx(tx, false);
+ return -ENXIO;
+ }
+
/* Lock i2c bus for the duration */
mutex_lock(&ir->ir_lock);
@@ -1099,6 +1116,7 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
if (copy_from_user(&command, buf + i, sizeof(command))) {
mutex_unlock(&ir->ir_lock);
+ mutex_unlock(&tx->client_lock);
put_ir_tx(tx, false);
return -EFAULT;
}
@@ -1109,6 +1127,7 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
ret = fw_load(tx);
if (ret != 0) {
mutex_unlock(&ir->ir_lock);
+ mutex_unlock(&tx->client_lock);
put_ir_tx(tx, false);
if (ret != -ENOMEM)
ret = -EIO;
@@ -1126,6 +1145,7 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
(unsigned)command & 0xFFFF);
if (ret == -EPROTO) {
mutex_unlock(&ir->ir_lock);
+ mutex_unlock(&tx->client_lock);
put_ir_tx(tx, false);
return ret;
}
@@ -1144,6 +1164,7 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
zilog_error("unable to send to the IR chip "
"after 3 resets, giving up\n");
mutex_unlock(&ir->ir_lock);
+ mutex_unlock(&tx->client_lock);
put_ir_tx(tx, false);
return ret;
}
@@ -1158,6 +1179,8 @@ static ssize_t write(struct file *filep, const char *buf, size_t n,
/* Release i2c bus */
mutex_unlock(&ir->ir_lock);
+ mutex_unlock(&tx->client_lock);
+
/* Give back our struct IR_tx reference */
put_ir_tx(tx, false);
@@ -1367,12 +1390,20 @@ static int ir_remove(struct i2c_client *client)
{
if (strncmp("ir_tx_z8", client->name, 8) == 0) {
struct IR_tx *tx = i2c_get_clientdata(client);
- if (tx != NULL)
+ if (tx != NULL) {
+ mutex_lock(&tx->client_lock);
+ tx->c = NULL;
+ mutex_unlock(&tx->client_lock);
put_ir_tx(tx, false);
+ }
} else if (strncmp("ir_rx_z8", client->name, 8) == 0) {
struct IR_rx *rx = i2c_get_clientdata(client);
- if (rx != NULL)
+ if (rx != NULL) {
+ mutex_lock(&rx->client_lock);
+ rx->c = NULL;
+ mutex_unlock(&rx->client_lock);
put_ir_rx(rx, false);
+ }
}
return 0;
}
@@ -1474,6 +1505,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
ir->tx = tx;
ir->l.features |= LIRC_CAN_SEND_PULSE;
+ mutex_init(&tx->client_lock);
tx->c = client;
tx->need_boot = 1;
tx->post_tx_ready_poll =
@@ -1516,6 +1548,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
ir->rx = rx;
ir->l.features |= LIRC_CAN_REC_LIRCCODE;
+ mutex_init(&rx->client_lock);
rx->c = client;
rx->hdpvr_data_fmt =
(id->driver_data & ID_FLAG_HDPVR) ? true : false;
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 12/13] lirc_zilog: Fix somewhat confusing information messages in ir_probe()
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
` (10 preceding siblings ...)
2011-02-18 1:20 ` [PATCH 11/13] lirc_zilog: Add locking of the i2c_clients when in use Andy Walls
@ 2011-02-18 1:21 ` Andy Walls
2011-02-18 1:22 ` [PATCH 13/13] lirc_zilog: Update TODO list based on work completed and revised plans Andy Walls
2011-03-06 4:00 ` [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Jarod Wilson
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:21 UTC (permalink / raw)
To: linux-media
The total sequence of messages emitted by the ir_porbe() calls
for a transceiver's two i2c_clients was confusing. Clean it up a bit.
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/lirc_zilog.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index a59d32d..40195ef 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -1597,6 +1597,8 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
ret = -EBADRQC;
goto out_put_xx;
}
+ zilog_info("IR unit on %s (i2c-%d) registered as lirc%d and ready\n",
+ adap->name, adap->nr, ir->l.minor);
out_ok:
if (rx != NULL)
@@ -1604,7 +1606,7 @@ out_ok:
if (tx != NULL)
put_ir_tx(tx, true);
put_ir_device(ir, true);
- zilog_info("probe of IR %s on %s (i2c-%d) done. IR unit ready.\n",
+ zilog_info("probe of IR %s on %s (i2c-%d) done\n",
tx_probe ? "Tx" : "Rx", adap->name, adap->nr);
mutex_unlock(&ir_devices_lock);
return 0;
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 13/13] lirc_zilog: Update TODO list based on work completed and revised plans
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
` (11 preceding siblings ...)
2011-02-18 1:21 ` [PATCH 12/13] lirc_zilog: Fix somewhat confusing information messages in ir_probe() Andy Walls
@ 2011-02-18 1:22 ` Andy Walls
2011-03-06 4:00 ` [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Jarod Wilson
13 siblings, 0 replies; 15+ messages in thread
From: Andy Walls @ 2011-02-18 1:22 UTC (permalink / raw)
To: linux-media
Update the TODO.lirc_zilog based on what has been completed. Also revised
the development plan for lirc_zilog to not try and split Tx/Rx for one IR
transceiver unit between lirc_zilog and ir-kbd-i2c, since that would be a
ref-counting nightmare.
Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
drivers/staging/lirc/TODO.lirc_zilog | 51 ++++++++++++++++-----------------
1 files changed, 25 insertions(+), 26 deletions(-)
diff --git a/drivers/staging/lirc/TODO.lirc_zilog b/drivers/staging/lirc/TODO.lirc_zilog
index 2d0263f..a97800a 100644
--- a/drivers/staging/lirc/TODO.lirc_zilog
+++ b/drivers/staging/lirc/TODO.lirc_zilog
@@ -1,34 +1,33 @@
-1. Both ir-kbd-i2c and lirc_zilog provide support for RX events.
-The 'tx_only' lirc_zilog module parameter will allow ir-kbd-i2c
-and lirc_zilog to coexist in the kernel, if the user requires such a set-up.
-However the IR unit will not work well without coordination between the
-two modules. A shared mutex, for transceiver access locking, needs to be
-supplied by bridge drivers, in struct IR_i2_init_data, to both ir-kbd-i2c
-and lirc_zilog, before they will coexist usefully. This should be fixed
-before moving out of staging.
-
-2. References and locking need careful examination. For cx18 and ivtv PCI
-cards, which are not easily "hot unplugged", the imperfect state of reference
-counting and locking is acceptable if not correct. For USB connected units
-like HD PVR, PVR USB2, HVR-1900, and HVR1950, the likelyhood of an Ooops on
-unplug is probably great. Proper reference counting and locking needs to be
-implemented before this module is moved out of staging.
-
-3. The binding between hdpvr and lirc_zilog is currently disabled,
-due to an OOPS reported a few years ago when both the hdpvr and cx18
-drivers were loaded in his system. More details can be seen at:
- http://www.mail-archive.com/linux-media@vger.kernel.org/msg09163.html
-More tests need to be done, in order to fix the reported issue.
-
-4. In addition to providing a shared mutex for transceiver access
-locking, bridge drivers, if able, should provide a chip reset() callback
+1. Both ir-kbd-i2c and lirc_zilog provide support for RX events for
+the chips supported by lirc_zilog. Before moving lirc_zilog out of staging:
+
+a. ir-kbd-i2c needs a module parameter added to allow the user to tell
+ ir-kbd-i2c to ignore Z8 IR units.
+
+b. lirc_zilog should provide Rx key presses to the rc core like ir-kbd-i2c
+ does.
+
+
+2. lirc_zilog module ref-counting need examination. It has not been
+verified that cdev and lirc_dev will take the proper module references on
+lirc_zilog to prevent removal of lirc_zilog when the /dev/lircN device node
+is open.
+
+(The good news is ref-counting of lirc_zilog internal structures appears to be
+complete. Testing has shown the cx18 module can be unloaded out from under
+irw + lircd + lirc_dev, with the /dev/lirc0 device node open, with no adverse
+effects. The cx18 module could then be reloaded and irw properly began
+receiving button presses again and ir_send worked without error.)
+
+
+3. Bridge drivers, if able, should provide a chip reset() callback
to lirc_zilog via struct IR_i2c_init_data. cx18 and ivtv already have routines
-to perform Z8 chip resets via GPIO manipulations. This will allow lirc_zilog
+to perform Z8 chip resets via GPIO manipulations. This would allow lirc_zilog
to bring the chip back to normal when it hangs, in the same places the
original lirc_pvr150 driver code does. This is not strictly needed, so it
is not required to move lirc_zilog out of staging.
-5. Both lirc_zilog and ir-kbd-i2c support the Zilog Z8 for IR, as programmed
+Note: Both lirc_zilog and ir-kbd-i2c support the Zilog Z8 for IR, as programmed
and installed on Hauppauge products. When working on either module, developers
must consider at least the following bridge drivers which mention an IR Rx unit
at address 0x71 (indicative of a Z8):
--
1.7.2.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
` (12 preceding siblings ...)
2011-02-18 1:22 ` [PATCH 13/13] lirc_zilog: Update TODO list based on work completed and revised plans Andy Walls
@ 2011-03-06 4:00 ` Jarod Wilson
13 siblings, 0 replies; 15+ messages in thread
From: Jarod Wilson @ 2011-03-06 4:00 UTC (permalink / raw)
To: Andy Walls; +Cc: linux-media
On Feb 17, 2011, at 8:11 PM, Andy Walls wrote:
> The following 13 patches are a substantial rework of lirc_zilog
> reference counting, object allocation and deallocation, and object
> locking.
>
> With these changes, devices can now disappear out from under lircd +
> lirc_dev + lirc_zilog with no adverse effects. I tested this with irw +
> lircd + lirc_dev + lirc_zilog + cx18 + HVR-1600. I could unload the
> cx18 driver without any oops or application crashes. When I reloaded
> the cx18 driver, irw started receiving RX button presses again, and
> irsend worked without a problem (and I didn't even need to restart
> lircd!).
>
> The ref counting fixes aren't finished as lirc_zilog itself can still be
> unloaded by the user when it shouldn't be, but a hot unplug of an
> HD-PVR, PVR-USB2, or HVR-1950 isn't going to trigger that.
>
> These changes are base off of Jarod Wilson's git repo
>
> http://git.linuxtv.org/jarod/linux-2.6-ir.git for-2.6.38 (IIRC)
As discussed on irc Friday, after figuring out an issue with trying to
test these using media_build against a 2.6.32 kernel, both the hdpvr
and hvr-1950 still function with these patches, and now you can even
hot-unplug them with lirc_zilog loaded and the system doesn't blow up.
Also gave a cursory read over all the code changes, didn't see anything
jump out that looked out of sorts, aside from the few minor fixlets we
also discussed. I'll just fix those locally in what I merge into the
tree I'm prepping with a variety of IR-related fixes for 2.6.39 to have
Mauro pull.
So for the set:
Acked-by: Jarod Wilson <jarod@redhat.com>
--
Jarod Wilson
jarod@wilsonet.com
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2011-03-06 4:00 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-02-18 1:11 [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Andy Walls
2011-02-18 1:12 ` [PATCH 01/13] lirc_zilog: Restore checks for existence of the IR_tx object Andy Walls
2011-02-18 1:13 ` [PATCH 02/13] lirc_zilog: Remove broken, ineffective reference counting Andy Walls
2011-02-18 1:14 ` [PATCH 03/13] lirc_zilog: Convert ir_device instance array to a linked list Andy Walls
2011-02-18 1:15 ` [PATCH 04/13] lirc_zilog: Convert the instance open count to an atomic_t Andy Walls
2011-02-18 1:15 ` [PATCH 05/13] lirc_zilog: Use kernel standard methods for marking device non-seekable Andy Walls
2011-02-18 1:16 ` [PATCH 06/13] lirc_zilog: Don't acquire the rx->buf_lock in the poll() function Andy Walls
2011-02-18 1:17 ` [PATCH 07/13] lirc_zilog: Remove unneeded rx->buf_lock Andy Walls
2011-02-18 1:18 ` [PATCH 08/13] lirc_zilog: Always allocate a Rx lirc_buffer object Andy Walls
2011-02-18 1:19 ` [PATCH 09/13] lirc_zilog: Move constants from ir_probe() into the lirc_driver template Andy Walls
2011-02-18 1:20 ` [PATCH 10/13] lirc_zilog: Add ref counting of struct IR, IR_tx, and IR_rx Andy Walls
2011-02-18 1:20 ` [PATCH 11/13] lirc_zilog: Add locking of the i2c_clients when in use Andy Walls
2011-02-18 1:21 ` [PATCH 12/13] lirc_zilog: Fix somewhat confusing information messages in ir_probe() Andy Walls
2011-02-18 1:22 ` [PATCH 13/13] lirc_zilog: Update TODO list based on work completed and revised plans Andy Walls
2011-03-06 4:00 ` [PATCH 0/13] lirc_zilog: Ref-counting and locking cleanup Jarod Wilson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox