Plus, regarding the reset case, urb-status will be non-zero, won't it?
----------------------------------------
> From: unicorn_w...@outlook.com
> To: oneu...@suse.de
> CC: linux-usb@vger.kernel.org
> Subject: RE: question on skel_read func of usb_skeleton.c
> Date: Tue, 9 Jul 2013 09:29:50 +0000
>
> Adding CC
> Sorry, I still have one more quesiton embedded.
>
>> From: oneu...@suse.de
>> To: unicorn_w...@outlook.com
>> Subject: Re: question on skel_read func of usb_skeleton.c
>> Date: Tue, 9 Jul 2013 08:42:19 +0200
>>
>> On Tuesday 09 July 2013 01:25:00 汪辰 wrote:
>>> Thanks.
>>> Regarding my first question. I think the original sample is used to demo
>>> both sync call in skel_read and async call in skel_write.
>>
>> Yes. However, a sync call is just incompatible with asynchronous IO.
>> And drivers should support non-blocking IO.
>>
>>> From practice perspective, I agree your improvements looks better. But only
>>> one quick quesiton is, current skel_read and skel_write both use async
>>> method but they are a bit different. For skel_write, I see there is just
>>> small improvement to limit the # of concurrent out bulks and for the case
>>> urb callback returns status error, we just record it and leave the next
>>> write failed on this, is that your design intent?
>>
>> Writing is different because you can just free the buffer after IO.
>> If you do async IO you have no choice but to report the error at a later
>> time.
>>
>>> Regarding the "retry" question, waht I'm talking is about the code segment
>>> from line 289 to line 315 (use the sample file with 3.10 kernel).
>>> line 289, size_t available = dev->bulk_in_filled - dev->bulk_in_copied;
>>> when and in what case, available equals to zero? What I can think of is for
>>> the case urb returns with 0 but the bulk_in_filled equals to zero too, bcos
>>> buil_in_copied will be always init as zero for every urb request, right?
>>
>> Yes. You also need to consider the reset case.
> Sorry, I still have not understood why we need to get the var - available by
> "dev->bulk_in_filled - dev->bulk_in_copied". dev->bulk_in_copied is always
> reset as zero in skel_do_read_io, right? So when we go to line 289, I think
> it happens after read callback returns, and at that time, bulk_in_copied
> should have been set as zero. Though I see at line 315, bulk_in_copied is
> given new data, but it will be reset to zero in next retry - skel_do_read_io,
> right? Maybe I have not understood the reset case you mentioned makes me can
> not reach your idea.
> For the case there may be data left from the last read, I have the same
> quesiton. I don't know what the bulk_in_copied is used for. Maybe I have to
> do some more practice test to see how these codes work.
>
>>
>>> You mentioned urb may finish at once with an error, but in the code line
>>> 273, I see for all error, read will exit, right?
>>
>> Yes, of course. But that is good. Errors should be reported soon.
>>
>>> Anway, upon is what I can not unerstand plus the comments line 294 to 295.
>>> Also, I'm not sure if we need the bulk_in_copied, I see it is always filled
>>> as zero in skel_do_read_io. Is it a bug or intend to this?
>>
>> There may be data left from the last read.
>>
>>> Regarding my thrid question, as to line 321 to 322, I would suggest just
>>> remove them and leave to userspace app to call more read() if they found
>>> the last return of read() doesn't reach the total length they wanted. This
>>> code in driver looks a bit tricky, what do you think?
>>
>> That would be bad for performance. The skeleton driver does what a driver
>> ought to do, just as plain as possible.
>>
>> Regards
>> Oliver
>>
>> PS: Please don't cut CC. Other people want to learn, too. ;-)
>>
