Adding CC
Sorry, I still have one more quesiton embedded.
> From: oneukum@xxxxxxx
> To: unicorn_wang@xxxxxxxxxxx
> 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. ;-)
> ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
