On Thursday, March 14, 2013 06:35 PM, Ming Lei wrote:
> On Thu, Mar 14, 2013 at 6:19 PM, Oliver Neukum <oneukum@xxxxxxx> wrote:
>> On Thursday 14 March 2013 18:07:29 Ming Lei wrote:
>>>> But then it makes no sense and you'd be better of with a waitqueue
>>>> instead of a completion.
>>>
>>> Maybe we can do it in another patch.
>>
>> Part of the locking changes would need to be reverted.
>> It is less work to convert now.
>
> Fair enough, it is fine.
>
> Thanks,
Ming Lei, Oliver
Thank both of you for pointing out problem in my fix and providing solutions.
Oliver
I haven't got that why we'd be better of with a waitqueue instead of a completion.
Could you explain more detailed?
Ming Lei
Below is the Patch v2 according to your solutions.is there any misunderstanding?
---
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);
if (rv < 0)
goto exit;
- /*
- * by waiting we also semiprocessed the urb
- * we must finish now
- */
- dev->bulk_in_copied = 0;
- dev->processed_urb = 1;
+ } else {
+ INIT_COMPLETION(dev->bulk_in_completion);
}
- if (!dev->processed_urb) {
- /*
- * the URB hasn't been processed
- * do it now
- */
- wait_for_completion(&dev->bulk_in_completion);
- dev->bulk_in_copied = 0;
- dev->processed_urb = 1;
- }
+ spin_unlock_irq(&dev->err_lock);
/* errors must be reported */
rv = dev->errors;
---
Du Xing
--
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
