cleanup of hiddev

cleanup of hiddev

[Oliver Neukum]

Hi,

this, I think, fixes everything.

	Regards
		Oliver

Signed-off-by: Oliver Neukum <oneukum@suse.de>

---

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 3ac3207..a158bd1 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -63,6 +63,7 @@ struct hiddev_list {
 	struct fasync_struct *fasync;
 	struct hiddev *hiddev;
 	struct list_head node;
+	struct mutex thread_lock;
 };
 
 static struct hiddev *hiddev_table[HIDDEV_MINORS];
@@ -266,29 +267,44 @@ static int hiddev_release(struct inode * inode, struct file * file)
 static int hiddev_open(struct inode *inode, struct file *file)
 {
 	struct hiddev_list *list;
-	unsigned long flags;
+	int res;
 
 	int i = iminor(inode) - HIDDEV_MINOR_BASE;
 
-	if (i >= HIDDEV_MINORS || !hiddev_table[i])
+	if (i >= HIDDEV_MINORS || i < 0 || !hiddev_table[i])
 		return -ENODEV;
 
 	if (!(list = kzalloc(sizeof(struct hiddev_list), GFP_KERNEL)))
 		return -ENOMEM;
+	mutex_init(&list->thread_lock);
 
 	list->hiddev = hiddev_table[i];
-
-	spin_lock_irqsave(&list->hiddev->list_lock, flags);
-	list_add_tail(&list->node, &hiddev_table[i]->list);
-	spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
+	
 
 	file->private_data = list;
 
-	if (!list->hiddev->open++)
-		if (list->hiddev->exist)
-			usbhid_open(hiddev_table[i]->hid);
+	if (list->hiddev->exist) {
+		if (!list->hiddev->open++) {
+			res = usbhid_open(hiddev_table[i]->hid);
+			if (res < 0) {
+				res = -EIO;
+				goto bail;
+			}
+		}
+	} else {
+		res = -ENODEV;
+		goto bail;
+	}
+
+	spin_lock_irq(&list->hiddev->list_lock);
+	list_add_tail(&list->node, &hiddev_table[i]->list);
+	spin_unlock_irq(&list->hiddev->list_lock);
 
 	return 0;
+bail:
+	file->private_data = NULL;
+	kfree(list->hiddev);
+	return res;
 }
 
 /*
@@ -307,7 +323,7 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 	DECLARE_WAITQUEUE(wait, current);
 	struct hiddev_list *list = file->private_data;
 	int event_size;
-	int retval = 0;
+	int retval;
 
 	event_size = ((list->flags & HIDDEV_FLAG_UREF) != 0) ?
 		sizeof(struct hiddev_usage_ref) : sizeof(struct hiddev_event);
@@ -315,10 +331,14 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 	if (count < event_size)
 		return 0;
 
+	/* lock against other threads */
+	retval = mutex_lock_interruptible(&list->thread_lock);
+	if (retval)
+		return -ERESTARTSYS;
+
 	while (retval == 0) {
 		if (list->head == list->tail) {
-			add_wait_queue(&list->hiddev->wait, &wait);
-			set_current_state(TASK_INTERRUPTIBLE);
+			prepare_to_wait(&list->hiddev->wait, &wait, TASK_INTERRUPTIBLE);
 
 			while (list->head == list->tail) {
 				if (file->f_flags & O_NONBLOCK) {
@@ -334,35 +354,45 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 					break;
 				}
 
+				/* let O_NONBLOCK tasks run */
+				mutex_unlock(&list->thread_lock);
 				schedule();
+				if (mutex_lock_interruptible(&list->thread_lock))
+					return -ERESTARTSYS;
 				set_current_state(TASK_INTERRUPTIBLE);
 			}
+			finish_wait(&list->hiddev->wait, &wait);
 
-			set_current_state(TASK_RUNNING);
-			remove_wait_queue(&list->hiddev->wait, &wait);
 		}
 
-		if (retval)
+		if (retval) {
+			mutex_unlock(&list->thread_lock);
 			return retval;
+		}
 
 
 		while (list->head != list->tail &&
 		       retval + event_size <= count) {
 			if ((list->flags & HIDDEV_FLAG_UREF) == 0) {
-				if (list->buffer[list->tail].field_index !=
-				    HID_FIELD_INDEX_NONE) {
+				if (list->buffer[list->tail].field_index != HID_FIELD_INDEX_NONE) {
 					struct hiddev_event event;
+
 					event.hid = list->buffer[list->tail].usage_code;
 					event.value = list->buffer[list->tail].value;
-					if (copy_to_user(buffer + retval, &event, sizeof(struct hiddev_event)))
+					if (copy_to_user(buffer + retval, &event, sizeof(struct hiddev_event))) {
+						mutex_unlock(&list->thread_lock);
 						return -EFAULT;
+					}
 					retval += sizeof(struct hiddev_event);
 				}
 			} else {
 				if (list->buffer[list->tail].field_index != HID_FIELD_INDEX_NONE ||
 				    (list->flags & HIDDEV_FLAG_REPORT) != 0) {
-					if (copy_to_user(buffer + retval, list->buffer + list->tail, sizeof(struct hiddev_usage_ref)))
+
+					if (copy_to_user(buffer + retval, list->buffer + list->tail, sizeof(struct hiddev_usage_ref))) {
+						mutex_unlock(&list->thread_lock);
 						return -EFAULT;
+					}
 					retval += sizeof(struct hiddev_usage_ref);
 				}
 			}
@@ -370,6 +400,7 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 		}
 
 	}
+	mutex_unlock(&list->thread_lock);
 
 	return retval;
 }
@@ -557,7 +588,7 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct hid_field *field;
 	struct usbhid_device *usbhid = hid->driver_data;
 	void __user *user_arg = (void __user *)arg;
-	int i;
+	int i, r;
 	
 	/* Called without BKL by compat methods so no BKL taken */
 
@@ -621,10 +652,22 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		}
 
 	case HIDIOCGSTRING:
-		return hiddev_ioctl_string(hiddev, cmd, user_arg);
+		lock_kernel();
+		if (!hiddev->exist)
+			r = hiddev_ioctl_string(hiddev, cmd, user_arg);
+		else
+			r = -ENODEV;
+		unlock_kernel();
+		return r;
 
 	case HIDIOCINITREPORT:
+		lock_kernel();
+		if (!hiddev->exist) {
+			unlock_kernel();
+			return -ENODEV;
+		}
 		usbhid_init_reports(hid);
+		unlock_kernel();
 
 		return 0;
 
@@ -638,8 +681,12 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if ((report = hiddev_lookup_report(hid, &rinfo)) == NULL)
 			return -EINVAL;
 
-		usbhid_submit_report(hid, report, USB_DIR_IN);
-		usbhid_wait_io(hid);
+		lock_kernel();
+		if (hiddev->exist) {
+			usbhid_submit_report(hid, report, USB_DIR_IN);
+			usbhid_wait_io(hid);
+		}
+		unlock_kernel();
 
 		return 0;
 
@@ -653,8 +700,11 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if ((report = hiddev_lookup_report(hid, &rinfo)) == NULL)
 			return -EINVAL;
 
-		usbhid_submit_report(hid, report, USB_DIR_OUT);
-		usbhid_wait_io(hid);
+		lock_kernel();
+		if (hiddev->exist) {
+			usbhid_submit_report(hid, report, USB_DIR_OUT);
+			usbhid_wait_io(hid);
+		}
 
 		return 0;
 
@@ -712,7 +762,13 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case HIDIOCGUSAGES:
 	case HIDIOCSUSAGES:
 	case HIDIOCGCOLLECTIONINDEX:
-		return hiddev_ioctl_usage(hiddev, cmd, user_arg);
+		lock_kernel();
+		if (hiddev->exist)
+			r = hiddev_ioctl_usage(hiddev, cmd, user_arg);
+		else
+			r = -ENODEV;
+		unlock_kernel();
+		return r;
 
 	case HIDIOCGCOLLECTIONINFO:
 		if (copy_from_user(&cinfo, user_arg, sizeof(cinfo)))
@@ -810,23 +866,21 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
 		return -1;
 
-	retval = usb_register_dev(usbhid->intf, &hiddev_class);
-	if (retval) {
-		err_hid("Not able to get a minor for this device.");
-		kfree(hiddev);
-		return -1;
-	}
-
 	init_waitqueue_head(&hiddev->wait);
 	INIT_LIST_HEAD(&hiddev->list);
 	spin_lock_init(&hiddev->list_lock);
 	hiddev->hid = hid;
 	hiddev->exist = 1;
 
-	hid->minor = usbhid->intf->minor;
-	hid->hiddev = hiddev;
-
-	hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
+	retval = usb_register_dev(usbhid->intf, &hiddev_class);
+	if (retval) {
+		err_hid("Not able to get a minor for this device.");
+		kfree(hiddev);
+		return -1;
+	} else {
+		hid->minor = usbhid->intf->minor;
+		hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
+	}
 
 	return 0;
 }
@@ -841,7 +895,9 @@ void hiddev_disconnect(struct hid_device *hid)
 	struct hiddev *hiddev = hid->hiddev;
 	struct usbhid_device *usbhid = hid->driver_data;
 
+	lock_kernel();
 	hiddev->exist = 0;
+	unlock_kernel();
 
 	hiddev_table[hiddev->hid->minor - HIDDEV_MINOR_BASE] = NULL;
 	usb_deregister_dev(usbhid->intf, &hiddev_class);

Re: cleanup of hiddev

[Jiri Slaby]

On 11/05/2008 12:52 PM, Oliver Neukum wrote:
> @@ -334,35 +354,45 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
>  					break;
>  				}
>  
> +				/* let O_NONBLOCK tasks run */
> +				mutex_unlock(&list->thread_lock);
>  				schedule();
> +				if (mutex_lock_interruptible(&list->thread_lock))
> +					return -ERESTARTSYS;

This should be EINTR, since the code potentially moved with tail and no rollback
is performed.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cleanup of hiddev

[Jiri Kosina]

On Wed, 5 Nov 2008, Oliver Neukum wrote:

> this, I think, fixes everything.

Thanks a lot for working on this, Oliver. Could you please add a proper
changelog, describing all the changes that had been done and why they were
needed, so that this can be queued for 2.6.29?

>  				schedule();
> +				if (mutex_lock_interruptible(&list->thread_lock))
> +					return -ERESTARTSYS;

Is ERESTARTSYS really appropriate here? Tail could have already moved from
within other thread, right?

>  	case HIDIOCGSTRING:
> -		return hiddev_ioctl_string(hiddev, cmd, user_arg);
> +		lock_kernel();
> +		if (!hiddev->exist)
> +			r = hiddev_ioctl_string(hiddev, cmd, user_arg);
> +		else
> +			r = -ENODEV;
> +		unlock_kernel();
> +		return r;

Hmm, I know that this is a legacy interface, but I am really hesitant to
add a new synchronization using lock_kernel(), instead of removing it
gradually altogether.

Thanks,

-- 
Jiri Kosina
SUSE Labs

cleanup of hiddev

[Oliver Neukum]

Hi Jiřis,

this is the cleanup of hiddev against current vanilla. What do you think?

	Regards
		Oliver

---

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 3ac3207..78f5a3b 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -266,11 +266,11 @@ static int hiddev_release(struct inode * inode, struct file * file)
 static int hiddev_open(struct inode *inode, struct file *file)
 {
 	struct hiddev_list *list;
-	unsigned long flags;
+	int res;
 
 	int i = iminor(inode) - HIDDEV_MINOR_BASE;
 
-	if (i >= HIDDEV_MINORS || !hiddev_table[i])
+	if (i >= HIDDEV_MINORS || i < 0 || !hiddev_table[i])
 		return -ENODEV;
 
 	if (!(list = kzalloc(sizeof(struct hiddev_list), GFP_KERNEL)))
@@ -278,17 +278,25 @@ static int hiddev_open(struct inode *inode, struct file *file)
 
 	list->hiddev = hiddev_table[i];
 
-	spin_lock_irqsave(&list->hiddev->list_lock, flags);
-	list_add_tail(&list->node, &hiddev_table[i]->list);
-	spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
-
 	file->private_data = list;
 
-	if (!list->hiddev->open++)
-		if (list->hiddev->exist)
-			usbhid_open(hiddev_table[i]->hid);
+	if (!list->hiddev->open++) {
+		if (list->hiddev->exist) {
+			res = usbhid_open(hiddev_table[i]->hid);
+			if (res < 0)
+				goto bail;
+		}
+	}
+
+	spin_lock_irq(&list->hiddev->list_lock);
+	list_add_tail(&list->node, &hiddev_table[i]->list);
+	spin_unlock_irq(&list->hiddev->list_lock);
 
 	return 0;
+bail:
+	file->private_data = NULL;
+	kfree(list->hiddev);
+	return -EIO;
 }
 
 /*
@@ -317,8 +325,8 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 
 	while (retval == 0) {
 		if (list->head == list->tail) {
-			add_wait_queue(&list->hiddev->wait, &wait);
 			set_current_state(TASK_INTERRUPTIBLE);
+			add_wait_queue(&list->hiddev->wait, &wait);
 
 			while (list->head == list->tail) {
 				if (file->f_flags & O_NONBLOCK) {
@@ -335,7 +343,6 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 				}
 
 				schedule();
-				set_current_state(TASK_INTERRUPTIBLE);
 			}
 
 			set_current_state(TASK_RUNNING);
@@ -810,13 +817,6 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
 		return -1;
 
-	retval = usb_register_dev(usbhid->intf, &hiddev_class);
-	if (retval) {
-		err_hid("Not able to get a minor for this device.");
-		kfree(hiddev);
-		return -1;
-	}

Re: cleanup of hiddev

[Jiri Slaby]

Oliver Neukum napsal(a):
> Hi Jiřis,

:)

Hi.

> this is the cleanup of hiddev against current vanilla. What do you think?

See my comments below.

> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 3ac3207..78f5a3b 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -317,8 +325,8 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
>  
>  	while (retval == 0) {
>  		if (list->head == list->tail) {
> -			add_wait_queue(&list->hiddev->wait, &wait);
>  			set_current_state(TASK_INTERRUPTIBLE);
> +			add_wait_queue(&list->hiddev->wait, &wait);

maybe better to convert to prepare_to_wait()?

>  
>  			while (list->head == list->tail) {

But moved here (or the set_current_state).

>  				if (file->f_flags & O_NONBLOCK) {
> @@ -335,7 +343,6 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
>  				}
>  
>  				schedule();
> -				set_current_state(TASK_INTERRUPTIBLE);

This is wrong. Schedule ensures state to be TASK_RUNNING. Not setting it
again for the next loop would be a bug (but better to be done right after
the while statement).

>  			}
>  
>  			set_current_state(TASK_RUNNING);

and finish_wait()

> @@ -810,13 +817,6 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
>  	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
>  		return -1;
>  
> -	retval = usb_register_dev(usbhid->intf, &hiddev_class);
> -	if (retval) {
> -		err_hid("Not able to get a minor for this device.");
> -		kfree(hiddev);
> -		return -1;
> -	}
> -
>  	init_waitqueue_head(&hiddev->wait);
>  	INIT_LIST_HEAD(&hiddev->list);
>  	spin_lock_init(&hiddev->list_lock);
> @@ -828,6 +828,14 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
>  
>  	hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;

usbhid->intf->minor is set even after this call:

> +	retval = usb_register_dev(usbhid->intf, &hiddev_class);
> +	if (retval) {
> +		err_hid("Not able to get a minor for this device.");
> +		hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = NULL;
> +		kfree(hiddev);
> +		return -1;
> +	}
> +
>  	return 0;
>  }
>  
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cleanup of hiddev

[Oliver Neukum]

Am Montag, 3. November 2008 20:58:11 schrieb Jiri Slaby:
> Oliver Neukum napsal(a):
> > Hi Jiřis,
> 
> :)
> 
> Hi.

Hi,

> 
> > this is the cleanup of hiddev against current vanilla. What do you think?
> 
> See my comments below.

OK, all clear, except for:

> > @@ -828,6 +828,14 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
> >  
> >  	hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
> 
> usbhid->intf->minor is set even after this call:

What does this mean?

	Regards
		Oliver



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cleanup of hiddev

[Jiri Slaby]

Oliver Neukum napsal(a):
> Am Montag, 3. November 2008 20:58:11 schrieb Jiri Slaby:
>>> @@ -828,6 +828,14 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
>>>  
>>>  	hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
>> usbhid->intf->minor is set even after this call:
> 
> What does this mean?

You removed the usb_register_dev() call when replying. It sets the
intf->minor, but you use it above it now.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cleanup of hiddev

[Oliver Neukum]

Am Dienstag, 4. November 2008 16:19:57 schrieb Jiri Slaby:
> Oliver Neukum napsal(a):
> > Am Montag, 3. November 2008 20:58:11 schrieb Jiri Slaby:
> >>> @@ -828,6 +828,14 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
> >>>  
> >>>  	hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
> >> usbhid->intf->minor is set even after this call:
> > 
> > What does this mean?
> 
> You removed the usb_register_dev() call when replying. It sets the
> intf->minor, but you use it above it now.
> 

Yes, I see. However, the device must not be registered before the table
is correctly filled and the minor must be filled out before. A mutex then?

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cleanup of hiddev

[Jiri Slaby]

Oliver Neukum napsal(a):
> Yes, I see. However, the device must not be registered before the table
> is correctly filled and the minor must be filled out before. A mutex then?

It's enough to assign the hiddev_table after the register, isn't it?
Actually, it's how it's done now... BTW what was the reason to move with that?

Re: cleanup of hiddev

[Oliver Neukum]

Am Dienstag, 4. November 2008 16:33:49 schrieb Jiri Slaby:
> Oliver Neukum napsal(a):
> > Yes, I see. However, the device must not be registered before the table
> > is correctly filled and the minor must be filled out before. A mutex then?
> 
> It's enough to assign the hiddev_table after the register, isn't it?
> Actually, it's how it's done now... BTW what was the reason to move with that?

No, you have a race condition where open() for an announced device
will fail.

	Regards
		Oliver

Re: cleanup of hiddev

[Jiri Slaby]

Oliver Neukum napsal(a):
> Am Dienstag, 4. November 2008 16:33:49 schrieb Jiri Slaby:
>> Oliver Neukum napsal(a):
>>> Yes, I see. However, the device must not be registered before the table
>>> is correctly filled and the minor must be filled out before. A mutex then?
>> It's enough to assign the hiddev_table after the register, isn't it?
>> Actually, it's how it's done now... BTW what was the reason to move with that?
> 
> No, you have a race condition where open() for an announced device
> will fail.

Could you expand this? I don't understand.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cleanup of hiddev

[Oliver Neukum]

Am Dienstag, 4. November 2008 21:34:43 schrieb Jiri Slaby:
> Oliver Neukum napsal(a):
> > Am Dienstag, 4. November 2008 16:33:49 schrieb Jiri Slaby:
> >> Oliver Neukum napsal(a):
> >>> Yes, I see. However, the device must not be registered before the table
> >>> is correctly filled and the minor must be filled out before. A mutex then?
> >> It's enough to assign the hiddev_table after the register, isn't it?
> >> Actually, it's how it's done now... BTW what was the reason to move with that?
> > 
> > No, you have a race condition where open() for an announced device
> > will fail.
> 
> Could you expand this? I don't understand.
> 

If you call usb_register_dev() before you set
hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE]
this check in hiddev_open() may fail:
	if (i >= HIDDEV_MINORS || i < 0 || !hiddev_table[i])
		return -ENODEV;

	Regards
		Oliver

Re: cleanup of hiddev

[Jiri Slaby]

On 11/04/2008 11:51 PM, Oliver Neukum wrote:
> If you call usb_register_dev() before you set
> hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE]
> this check in hiddev_open() may fail:
> 	if (i >= HIDDEV_MINORS || i < 0 || !hiddev_table[i])
> 		return -ENODEV;

I think, that's the point. You should also get ENODEV if you don't call
usb_register_dev(), create a node for non-existent minor manually and try to
open it. It behaves the same as if there was a lock you suggested...

Ex.:
# grep 137 /proc/devices
# mknod node c 137 0
# cat node
cat: node: No such device or address

Re: cleanup of hiddev

[Oliver Neukum]

Am Dienstag, 4. November 2008 23:57:02 schrieb Jiri Slaby:
> On 11/04/2008 11:51 PM, Oliver Neukum wrote:
> > If you call usb_register_dev() before you set
> > hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE]
> > this check in hiddev_open() may fail:
> >       if (i >= HIDDEV_MINORS || i < 0 || !hiddev_table[i])
> >               return -ENODEV;
> 
> I think, that's the point. You should also get ENODEV if you don't call
> usb_register_dev(), create a node for non-existent minor manually and try to
> open it. It behaves the same as if there was a lock you suggested...

But by calling usb_register_dev() we also cause the kernel to emit a
hotplug event that tells udev that the device exists.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cleanup of hiddev

[Oliver Neukum]

Am Montag, 3. November 2008 20:58:11 schrieb Jiri Slaby:
> Hi.
> 
> > this is the cleanup of hiddev against current vanilla. What do you think?
> 
> See my comments below.

Hi,

there still is a race of disconnect() with ioctl(). It seems like that
code still operates under the assumption that both methods take
BKL. Do you prefer a conservative fix with selective application of BKL
or introduction of a new mutex?

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cleanup of hiddev

[Alan Stern]

On Tue, 4 Nov 2008, Oliver Neukum wrote:

> Am Montag, 3. November 2008 20:58:11 schrieb Jiri Slaby:
> > Hi.
> > 
> > > this is the cleanup of hiddev against current vanilla. What do you think?
> > 
> > See my comments below.
> 
> Hi,
> 
> there still is a race of disconnect() with ioctl(). It seems like that
> code still operates under the assumption that both methods take
> BKL. Do you prefer a conservative fix with selective application of BKL
> or introduction of a new mutex?

It's none of my business... but IMO we should try to move away from the 
BKL as much as possible.

Alan Stern

Re: cleanup of hiddev

[Oliver Neukum]

Am Dienstag, 4. November 2008 16:00:06 schrieb Alan Stern:
> On Tue, 4 Nov 2008, Oliver Neukum wrote:
> 
> > Am Montag, 3. November 2008 20:58:11 schrieb Jiri Slaby:
> > > Hi.
> > > 
> > > > this is the cleanup of hiddev against current vanilla. What do you think?
> > > 
> > > See my comments below.
> > 
> > Hi,
> > 
> > there still is a race of disconnect() with ioctl(). It seems like that
> > code still operates under the assumption that both methods take
> > BKL. Do you prefer a conservative fix with selective application of BKL
> > or introduction of a new mutex?
> 
> It's none of my business... but IMO we should try to move away from the 
> BKL as much as possible.

This is true but hiddev is in a sense legacy code which should be replaced
with hidraw, so I tend to fix it conservatively.

	Regards
		Oliver

Re: cleanup of hiddev

[Jiri Slaby]

Oliver Neukum napsal(a):
> Am Dienstag, 4. November 2008 16:00:06 schrieb Alan Stern:
>> On Tue, 4 Nov 2008, Oliver Neukum wrote:
>>
>>> Am Montag, 3. November 2008 20:58:11 schrieb Jiri Slaby:
>>>> Hi.
>>>>
>>>>> this is the cleanup of hiddev against current vanilla. What do you think?
>>>> See my comments below.
>>> Hi,
>>>
>>> there still is a race of disconnect() with ioctl(). It seems like that
>>> code still operates under the assumption that both methods take
>>> BKL. Do you prefer a conservative fix with selective application of BKL
>>> or introduction of a new mutex?
>> It's none of my business... but IMO we should try to move away from the 
>> BKL as much as possible.
> 
> This is true but hiddev is in a sense legacy code which should be replaced
> with hidraw, so I tend to fix it conservatively.

Jiri K. is off for few days, I'm not responsible for the HID layer, he is, I
would let him to decide ;).

Re: cleanup of hiddev

[Oliver Neukum]

Am Montag, 3. November 2008 20:58:11 schrieb Jiri Slaby:
> Hi.
> 
> > this is the cleanup of hiddev against current vanilla. What do you think?
> 
> See my comments below.

Hi,

I've addressed all comments I understood. In addition it seems to me that
the read method is not thread-safe.

	Regards
		Oliver

Signed-off-by: Oliver Neukum <oneukum@suse.de>

---

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 3ac3207..a35576b 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -63,6 +63,7 @@ struct hiddev_list {
 	struct fasync_struct *fasync;
 	struct hiddev *hiddev;
 	struct list_head node;
+	struct mutex thread_lock;
 };
 
 static struct hiddev *hiddev_table[HIDDEV_MINORS];
@@ -266,29 +267,38 @@ static int hiddev_release(struct inode * inode, struct file * file)
 static int hiddev_open(struct inode *inode, struct file *file)
 {
 	struct hiddev_list *list;
-	unsigned long flags;
+	int res;
 
 	int i = iminor(inode) - HIDDEV_MINOR_BASE;
 
-	if (i >= HIDDEV_MINORS || !hiddev_table[i])
+	if (i >= HIDDEV_MINORS || i < 0 || !hiddev_table[i])
 		return -ENODEV;
 
 	if (!(list = kzalloc(sizeof(struct hiddev_list), GFP_KERNEL)))
 		return -ENOMEM;
+	mutex_init(&list->thread_lock);
 
 	list->hiddev = hiddev_table[i];
 
-	spin_lock_irqsave(&list->hiddev->list_lock, flags);
-	list_add_tail(&list->node, &hiddev_table[i]->list);
-	spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
-
 	file->private_data = list;
 
-	if (!list->hiddev->open++)
-		if (list->hiddev->exist)
-			usbhid_open(hiddev_table[i]->hid);
+	if (!list->hiddev->open++) {
+		if (list->hiddev->exist) {
+			res = usbhid_open(hiddev_table[i]->hid);
+			if (res < 0)
+				goto bail;
+		}
+	}
+
+	spin_lock_irq(&list->hiddev->list_lock);
+	list_add_tail(&list->node, &hiddev_table[i]->list);
+	spin_unlock_irq(&list->hiddev->list_lock);
 
 	return 0;
+bail:
+	file->private_data = NULL;
+	kfree(list->hiddev);
+	return -EIO;
 }
 
 /*
@@ -307,7 +317,7 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 	DECLARE_WAITQUEUE(wait, current);
 	struct hiddev_list *list = file->private_data;
 	int event_size;
-	int retval = 0;
+	int retval;
 
 	event_size = ((list->flags & HIDDEV_FLAG_UREF) != 0) ?
 		sizeof(struct hiddev_usage_ref) : sizeof(struct hiddev_event);
@@ -315,10 +325,14 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 	if (count < event_size)
 		return 0;
 
+	/* lock against other threads */
+	retval = mutex_lock_interruptible(&list->thread_lock);
+	if (retval)
+		return -ERESTARTSYS;
+
 	while (retval == 0) {
 		if (list->head == list->tail) {
-			add_wait_queue(&list->hiddev->wait, &wait);
-			set_current_state(TASK_INTERRUPTIBLE);
+			prepare_to_wait(&list->hiddev->wait, &wait, TASK_INTERRUPTIBLE);
 
 			while (list->head == list->tail) {
 				if (file->f_flags & O_NONBLOCK) {
@@ -337,32 +351,38 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 				schedule();
 				set_current_state(TASK_INTERRUPTIBLE);
 			}
+			finish_wait(&list->hiddev->wait, &wait);
 
-			set_current_state(TASK_RUNNING);
-			remove_wait_queue(&list->hiddev->wait, &wait);
 		}
 
-		if (retval)
+		if (retval) {
+			mutex_unlock(&list->thread_lock);
 			return retval;
+		}
 
 
 		while (list->head != list->tail &&
 		       retval + event_size <= count) {
 			if ((list->flags & HIDDEV_FLAG_UREF) == 0) {
-				if (list->buffer[list->tail].field_index !=
-				    HID_FIELD_INDEX_NONE) {
+				if (list->buffer[list->tail].field_index != HID_FIELD_INDEX_NONE) {
 					struct hiddev_event event;
+
 					event.hid = list->buffer[list->tail].usage_code;
 					event.value = list->buffer[list->tail].value;
-					if (copy_to_user(buffer + retval, &event, sizeof(struct hiddev_event)))
+					if (copy_to_user(buffer + retval, &event, sizeof(struct hiddev_event))) {
+						mutex_unlock(&list->thread_lock);
 						return -EFAULT;
+					}
 					retval += sizeof(struct hiddev_event);
 				}
 			} else {
 				if (list->buffer[list->tail].field_index != HID_FIELD_INDEX_NONE ||
 				    (list->flags & HIDDEV_FLAG_REPORT) != 0) {
-					if (copy_to_user(buffer + retval, list->buffer + list->tail, sizeof(struct hiddev_usage_ref)))
+
+					if (copy_to_user(buffer + retval, list->buffer + list->tail, sizeof(struct hiddev_usage_ref))) {
+						mutex_unlock(&list->thread_lock);
 						return -EFAULT;
+					}
 					retval += sizeof(struct hiddev_usage_ref);
 				}
 			}
@@ -370,6 +390,7 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 		}
 
 	}
+	mutex_unlock(&list->thread_lock);
 
 	return retval;
 }
@@ -810,13 +831,6 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
 		return -1;
 
-	retval = usb_register_dev(usbhid->intf, &hiddev_class);
-	if (retval) {
-		err_hid("Not able to get a minor for this device.");
-		kfree(hiddev);
-		return -1;
-	}
-
 	init_waitqueue_head(&hiddev->wait);
 	INIT_LIST_HEAD(&hiddev->list);
 	spin_lock_init(&hiddev->list_lock);
@@ -828,6 +842,14 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 
 	hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
 
+	retval = usb_register_dev(usbhid->intf, &hiddev_class);
+	if (retval) {
+		err_hid("Not able to get a minor for this device.");
+		hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = NULL;
+		kfree(hiddev);
+		return -1;
+	}
+
 	return 0;
 }