On Sat, 6 Mar 2010, Alan Stern wrote:
>
> And while you're at it, please also follow Linus's implied suggestion
> and change that stupid variable name. Generations of code readers to
> come will thank you.
How about this? It
- makes the naming less atrocious
- uses 'f_version' as suggested by Dmitry, which is obviously the
RightThing(tm) to do.
- uses an atomic event counter (with a simple but commented subtle thing
to make sure that 'f_version = 0' always triggers as "new events might
be pending")
- gets rid of the insane locking on poll
Can somebody test it? I in no way tested it, except it compiles for me.
Linus
---
drivers/usb/core/devices.c | 66 ++++++++++++++++----------------------------
1 files changed, 24 insertions(+), 42 deletions(-)
diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
index c83c975..6053a8b 100644
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -117,9 +117,21 @@ static const char *format_endpt =
* However, these will come from functions that return ptrs to each of them.
*/
-static DECLARE_WAIT_QUEUE_HEAD(deviceconndiscwq);
-/* guarded by usbfs_mutex */
-static unsigned int conndiscevcnt;
+/*
+ * Wait for an connect/disconnect event to happen. We initialize
+ * the event counter with an odd number, and each event will increment
+ * the event counter by two, so it will always _stay_ odd. That means
+ * that it will never be zero, so "event 0" will never match a current
+ * event, and thus 'poll' will always trigger as readable for the first
+ * time it gets called.
+ */
+static struct device_connect_event {
+ atomic_t count;
+ wait_queue_head_t wait;
+} device_event = {
+ .count = ATOMIC_INIT(1),
+ .wait = __WAIT_QUEUE_HEAD_INITIALIZER(device_event.wait)
+};
/* this struct stores the poll state for <mountpoint>/devices pollers */
struct usb_device_status {
@@ -157,10 +169,8 @@ static const struct class_info clas_info[] =
void usbfs_conn_disc_event(void)
{
- mutex_lock(&usbfs_mutex);
- conndiscevcnt++;
- mutex_unlock(&usbfs_mutex);
- wake_up(&deviceconndiscwq);
+ atomic_add(2, &device_event.count);
+ wake_up(&device_event.wait);
}
static const char *class_decode(const int class)
@@ -632,42 +642,16 @@ static ssize_t usb_device_read(struct file *file, char __user *buf,
static unsigned int usb_device_poll(struct file *file,
struct poll_table_struct *wait)
{
- struct usb_device_status *st;
- unsigned int mask = 0;
-
- mutex_lock(&usbfs_mutex);
- st = file->private_data;
- if (!st) {
- st = kmalloc(sizeof(struct usb_device_status), GFP_KERNEL);
- if (!st) {
- mutex_unlock(&usbfs_mutex);
- return POLLIN;
- }
-
- st->lastev = conndiscevcnt;
- file->private_data = st;
- mask = POLLIN;
- }
+ unsigned int event_count;
- if (file->f_mode & FMODE_READ)
- poll_wait(file, &deviceconndiscwq, wait);
- if (st->lastev != conndiscevcnt)
- mask |= POLLIN;
- st->lastev = conndiscevcnt;
- mutex_unlock(&usbfs_mutex);
- return mask;
-}
+ poll_wait(file, &device_event.wait, wait);
-static int usb_device_open(struct inode *inode, struct file *file)
-{
- file->private_data = NULL;
- return 0;
-}
+ event_count = atomic_read(&device_event.count);
+ if (file->f_version != event_count) {
+ file->f_version = event_count;
+ return POLLIN | POLLRDNORM;
+ }
-static int usb_device_release(struct inode *inode, struct file *file)
-{
- kfree(file->private_data);
- file->private_data = NULL;
return 0;
}
@@ -699,6 +683,4 @@ const struct file_operations usbfs_devices_fops = {
.llseek = usb_device_lseek,
.read = usb_device_read,
.poll = usb_device_poll,
- .open = usb_device_open,
- .release = usb_device_release,
};
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
