Re: cleanup of hiddev

[Jiri Slaby <jirislaby@gmail.com>]

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