On Friday 15 March 2013 00:19:00 Du Xing wrote:
> Ming Lei
> Below is the Patch v2 according to your solutions.is there any misunderstanding?
The problem is that I needed to work around the counting nature of completions.
If you go to a waitqueue the need is removed. Your original patch together with
the change to use a wait queue would be correct.
> ---
> diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> index ce31017..3ad1840 100644
> --- a/drivers/usb/usb-skeleton.c
> +++ b/drivers/usb/usb-skeleton.c
> @@ -61,7 +61,6 @@ struct usb_skel {
> __u8 bulk_out_endpointAddr; /* the address of the bulk out endpoint */
> int errors; /* the last request tanked */
> bool ongoing_read; /* a read is going on */
> - bool processed_urb; /* indicates we haven't processed the urb */
> spinlock_t err_lock; /* lock for errors */
> struct kref kref;
> struct mutex io_mutex; /* synchronize I/O with disconnect */
> @@ -181,11 +180,12 @@ static void skel_read_bulk_callback(struct urb *urb)
> dev->errors = urb->status;
> } else {
> dev->bulk_in_filled = urb->actual_length;
> + dev->bulk_in_copied = 0;
> }
> dev->ongoing_read = 0;
> + complete(&dev->bulk_in_completion);
> spin_unlock(&dev->err_lock);
>
> - complete(&dev->bulk_in_completion);
> }
>
> static int skel_do_read_io(struct usb_skel *dev, size_t count)
> @@ -227,7 +227,6 @@ static ssize_t skel_read(struct file *file, char *buffer, size_t count,
> {
> struct usb_skel *dev;
> int rv;
> - bool ongoing_io;
>
> dev = file->private_data;
>
> @@ -248,10 +247,8 @@ static ssize_t skel_read(struct file *file, char *buffer, size_t count,
> /* if IO is under way, we must not touch things */
> retry:
> spin_lock_irq(&dev->err_lock);
> - ongoing_io = dev->ongoing_read;
> - spin_unlock_irq(&dev->err_lock);
>
> - if (ongoing_io) {
> + if (dev->ongoing_io) {
> /* nonblocking IO shall not wait */
> if (file->f_flags & O_NONBLOCK) {
> rv = -EAGAIN;
> @@ -264,23 +261,11 @@ retry:
> rv = wait_for_completion_interruptible(&dev->bulk_in_completion);
Not good. You are sleeping with a spinlock held. This is absolutely bad.
If you wish to retain the completion you need to change locking a bit more.
Regards
Oliver
--
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
