* [PATCH 0/2] cec: improve locking
@ 2016-08-02 11:23 Hans Verkuil
2016-08-02 11:23 ` [PATCH 1/2] cec: rename cec_devnode fhs_lock to just lock Hans Verkuil
2016-08-02 11:23 ` [PATCH 2/2] cec: improve locking Hans Verkuil
0 siblings, 2 replies; 3+ messages in thread
From: Hans Verkuil @ 2016-08-02 11:23 UTC (permalink / raw)
To: linux-media
From: Hans Verkuil <hans.verkuil@cisco.com>
While reviewing the CEC core code I noticed a few potential problems
with locking under normal (and even not so normal) circumstances this
wouldn't cause problems, but in theory there could be corner cases where
you could get a deadlock or perhaps a race condition.
The first patch just renames a lock, the second actually improved the
locking scheme.
Regards,
Hans
Hans Verkuil (2):
cec: rename cec_devnode fhs_lock to just lock
cec: improve locking
drivers/staging/media/cec/cec-adap.c | 12 ++++++------
drivers/staging/media/cec/cec-api.c | 8 ++++----
drivers/staging/media/cec/cec-core.c | 27 +++++++++++++++------------
include/media/cec.h | 2 +-
4 files changed, 26 insertions(+), 23 deletions(-)
--
2.8.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 1/2] cec: rename cec_devnode fhs_lock to just lock
2016-08-02 11:23 [PATCH 0/2] cec: improve locking Hans Verkuil
@ 2016-08-02 11:23 ` Hans Verkuil
2016-08-02 11:23 ` [PATCH 2/2] cec: improve locking Hans Verkuil
1 sibling, 0 replies; 3+ messages in thread
From: Hans Verkuil @ 2016-08-02 11:23 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
This lock will be used to protect more than just the fhs list.
So rename it to just 'lock'.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/staging/media/cec/cec-adap.c | 12 ++++++------
drivers/staging/media/cec/cec-api.c | 8 ++++----
drivers/staging/media/cec/cec-core.c | 6 +++---
include/media/cec.h | 2 +-
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/media/cec/cec-adap.c b/drivers/staging/media/cec/cec-adap.c
index b2393bb..9dcb784 100644
--- a/drivers/staging/media/cec/cec-adap.c
+++ b/drivers/staging/media/cec/cec-adap.c
@@ -124,10 +124,10 @@ static void cec_queue_event(struct cec_adapter *adap,
u64 ts = ktime_get_ns();
struct cec_fh *fh;
- mutex_lock(&adap->devnode.fhs_lock);
+ mutex_lock(&adap->devnode.lock);
list_for_each_entry(fh, &adap->devnode.fhs, list)
cec_queue_event_fh(fh, ev, ts);
- mutex_unlock(&adap->devnode.fhs_lock);
+ mutex_unlock(&adap->devnode.lock);
}
/*
@@ -191,12 +191,12 @@ static void cec_queue_msg_monitor(struct cec_adapter *adap,
u32 monitor_mode = valid_la ? CEC_MODE_MONITOR :
CEC_MODE_MONITOR_ALL;
- mutex_lock(&adap->devnode.fhs_lock);
+ mutex_lock(&adap->devnode.lock);
list_for_each_entry(fh, &adap->devnode.fhs, list) {
if (fh->mode_follower >= monitor_mode)
cec_queue_msg_fh(fh, msg);
}
- mutex_unlock(&adap->devnode.fhs_lock);
+ mutex_unlock(&adap->devnode.lock);
}
/*
@@ -207,12 +207,12 @@ static void cec_queue_msg_followers(struct cec_adapter *adap,
{
struct cec_fh *fh;
- mutex_lock(&adap->devnode.fhs_lock);
+ mutex_lock(&adap->devnode.lock);
list_for_each_entry(fh, &adap->devnode.fhs, list) {
if (fh->mode_follower == CEC_MODE_FOLLOWER)
cec_queue_msg_fh(fh, msg);
}
- mutex_unlock(&adap->devnode.fhs_lock);
+ mutex_unlock(&adap->devnode.lock);
}
/* Notify userspace of an adapter state change. */
diff --git a/drivers/staging/media/cec/cec-api.c b/drivers/staging/media/cec/cec-api.c
index 7be7615..4e2696a 100644
--- a/drivers/staging/media/cec/cec-api.c
+++ b/drivers/staging/media/cec/cec-api.c
@@ -508,14 +508,14 @@ static int cec_open(struct inode *inode, struct file *filp)
filp->private_data = fh;
- mutex_lock(&devnode->fhs_lock);
+ mutex_lock(&devnode->lock);
/* Queue up initial state events */
ev_state.state_change.phys_addr = adap->phys_addr;
ev_state.state_change.log_addr_mask = adap->log_addrs.log_addr_mask;
cec_queue_event_fh(fh, &ev_state, 0);
list_add(&fh->list, &devnode->fhs);
- mutex_unlock(&devnode->fhs_lock);
+ mutex_unlock(&devnode->lock);
return 0;
}
@@ -540,9 +540,9 @@ static int cec_release(struct inode *inode, struct file *filp)
cec_monitor_all_cnt_dec(adap);
mutex_unlock(&adap->lock);
- mutex_lock(&devnode->fhs_lock);
+ mutex_lock(&devnode->lock);
list_del(&fh->list);
- mutex_unlock(&devnode->fhs_lock);
+ mutex_unlock(&devnode->lock);
/* Unhook pending transmits from this filehandle. */
mutex_lock(&adap->lock);
diff --git a/drivers/staging/media/cec/cec-core.c b/drivers/staging/media/cec/cec-core.c
index 112a5fa..73792d0 100644
--- a/drivers/staging/media/cec/cec-core.c
+++ b/drivers/staging/media/cec/cec-core.c
@@ -117,7 +117,7 @@ static int __must_check cec_devnode_register(struct cec_devnode *devnode,
/* Initialization */
INIT_LIST_HEAD(&devnode->fhs);
- mutex_init(&devnode->fhs_lock);
+ mutex_init(&devnode->lock);
/* Part 1: Find a free minor number */
mutex_lock(&cec_devnode_lock);
@@ -181,10 +181,10 @@ static void cec_devnode_unregister(struct cec_devnode *devnode)
if (!devnode->registered || devnode->unregistered)
return;
- mutex_lock(&devnode->fhs_lock);
+ mutex_lock(&devnode->lock);
list_for_each_entry(fh, &devnode->fhs, list)
wake_up_interruptible(&fh->wait);
- mutex_unlock(&devnode->fhs_lock);
+ mutex_unlock(&devnode->lock);
devnode->registered = false;
devnode->unregistered = true;
diff --git a/include/media/cec.h b/include/media/cec.h
index dc7854b..fdb5d60 100644
--- a/include/media/cec.h
+++ b/include/media/cec.h
@@ -57,8 +57,8 @@ struct cec_devnode {
int minor;
bool registered;
bool unregistered;
- struct mutex fhs_lock;
struct list_head fhs;
+ struct mutex lock;
};
struct cec_adapter;
--
2.8.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 2/2] cec: improve locking
2016-08-02 11:23 [PATCH 0/2] cec: improve locking Hans Verkuil
2016-08-02 11:23 ` [PATCH 1/2] cec: rename cec_devnode fhs_lock to just lock Hans Verkuil
@ 2016-08-02 11:23 ` Hans Verkuil
1 sibling, 0 replies; 3+ messages in thread
From: Hans Verkuil @ 2016-08-02 11:23 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
- The global lock was used in cec_get_device when it should have
used the devnode lock.
- cec_put_device also took the global lock, but since the release
function takes that lock as well this could lead to a deadlock.
Just don't take the lock here since there is no reason for it.
- cec_devnode_register() should take the global lock when clearing
the bit in the global bitmap.
- In cec_devnode_unregister() place the devnode->(un)register tests
and assignments under the devnode lock as well: this has to be
in a critical block.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/staging/media/cec/cec-core.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/media/cec/cec-core.c b/drivers/staging/media/cec/cec-core.c
index 73792d0..3b1e4d2 100644
--- a/drivers/staging/media/cec/cec-core.c
+++ b/drivers/staging/media/cec/cec-core.c
@@ -51,31 +51,29 @@ int cec_get_device(struct cec_devnode *devnode)
{
/*
* Check if the cec device is available. This needs to be done with
- * the cec_devnode_lock held to prevent an open/unregister race:
+ * the devnode->lock held to prevent an open/unregister race:
* without the lock, the device could be unregistered and freed between
* the devnode->registered check and get_device() calls, leading to
* a crash.
*/
- mutex_lock(&cec_devnode_lock);
+ mutex_lock(&devnode->lock);
/*
* return ENXIO if the cec device has been removed
* already or if it is not registered anymore.
*/
if (!devnode->registered) {
- mutex_unlock(&cec_devnode_lock);
+ mutex_unlock(&devnode->lock);
return -ENXIO;
}
/* and increase the device refcount */
get_device(&devnode->dev);
- mutex_unlock(&cec_devnode_lock);
+ mutex_unlock(&devnode->lock);
return 0;
}
void cec_put_device(struct cec_devnode *devnode)
{
- mutex_lock(&cec_devnode_lock);
put_device(&devnode->dev);
- mutex_unlock(&cec_devnode_lock);
}
/* Called when the last user of the cec device exits. */
@@ -84,11 +82,10 @@ static void cec_devnode_release(struct device *cd)
struct cec_devnode *devnode = to_cec_devnode(cd);
mutex_lock(&cec_devnode_lock);
-
/* Mark device node number as free */
clear_bit(devnode->minor, cec_devnode_nums);
-
mutex_unlock(&cec_devnode_lock);
+
cec_delete_adapter(to_cec_adapter(devnode));
}
@@ -160,7 +157,9 @@ static int __must_check cec_devnode_register(struct cec_devnode *devnode,
cdev_del:
cdev_del(&devnode->cdev);
clr_bit:
+ mutex_lock(&cec_devnode_lock);
clear_bit(devnode->minor, cec_devnode_nums);
+ mutex_unlock(&cec_devnode_lock);
return ret;
}
@@ -177,17 +176,21 @@ static void cec_devnode_unregister(struct cec_devnode *devnode)
{
struct cec_fh *fh;
+ mutex_lock(&devnode->lock);
+
/* Check if devnode was never registered or already unregistered */
- if (!devnode->registered || devnode->unregistered)
+ if (!devnode->registered || devnode->unregistered) {
+ mutex_unlock(&devnode->lock);
return;
+ }
- mutex_lock(&devnode->lock);
list_for_each_entry(fh, &devnode->fhs, list)
wake_up_interruptible(&fh->wait);
- mutex_unlock(&devnode->lock);
devnode->registered = false;
devnode->unregistered = true;
+ mutex_unlock(&devnode->lock);
+
device_del(&devnode->dev);
cdev_del(&devnode->cdev);
put_device(&devnode->dev);
--
2.8.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-08-02 11:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-02 11:23 [PATCH 0/2] cec: improve locking Hans Verkuil
2016-08-02 11:23 ` [PATCH 1/2] cec: rename cec_devnode fhs_lock to just lock Hans Verkuil
2016-08-02 11:23 ` [PATCH 2/2] cec: improve locking Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).