Hello.
Subbrathnam, Swaminathan wrote:
-----Original Message-----
From: Sergei Shtylyov [mailto:[EMAIL PROTECTED]
Sent: Thursday, December 04, 2008 10:54 PM
To: Sergei Shtylyov
Cc: Subbrathnam, Swaminathan; [EMAIL PROTECTED];
[EMAIL PROTECTED]; [email protected]
Subject: Re: [PATCH 3/3] MUSB : Fix for STALL handling in musb gadget code
Hello.
Sergei Shtylyov wrote:
If EP is busy, we have even less reason to do a giveback. This is
not a real fix, just pallialtive. Actually, whether a STALL token has
been sent or not shouldn't play any role in giving back URB, so the
fragment being pacthed is just totally wrong. Other drivers just don't
allow EP that is still active to be halted, and this driver should do
the same.
Your comment is plain wrong.
No, you code is.
Even your proof case contradicts your own code.
I think there are scenarios that would require requests to be given
back during stall handling.
I am laying one out below for
1. USB Device is in the process of transferring data to/from Host.
2. USB Device queues another request to send.
3. For some reason say Host/Device needs to STALL the endpoint
with the in-flight IO.
This can't be done reliably with requests in flight. That's why the
other drivers don't allow halting EP with requests in flight.
4. On completion of the STALL (on SENTSTALL interrupt) driver
would need to abort the current transfer and return the USB request
with appropriate status. Here the EP state is busy handling the
in-flight IO and the request that is active has to be
returned/givenback.
This just cannot work reliably. There should be connection between
actually sending STALL tokens and URB completion because this cannot be
handled reliably.
5. Once the CLEAR Stall is completed the Device would need to
process the next request that is queued if one exists and continue
further normally.
The requests *can* be queued while the EP is halted, and that's what
the other drivers do. You just can't stall EP until the request queue
drains.
Or at least until the current request's I/O is complete (this doesn't
mean
that it's given back as the driver employs early giveback).
<SWAMI> Driver does not employ early giveback.
No, it gives back a request before the packet is actually sent.
Please look at musb_g_tx() and txstate().
It is returned with error in the context of reception of STALL interrupt.
There will be no other interrupt raised after this condition. The only other
event is the request from Host to clear the STALL over EP0.
Do tell me how the driver can differ the request it should abort from
the request queued after halting endpoint that it must not abort. Your
code isn't able to do that.
In the above scenario as you can see we need to abort the in-flight
request (musb_ep->busy) on SENTSTALL interrupt and process the queued
request in the context of clear stall. Consider SENTSTALL interrupt
as a completion interrupt for the current in-flight request that needs
to be completed with error.
I'm not seeing why we would need to abort an in-flight request.
Moreover, I'm not seeing a conection between the EP busy state and an
URB giveback. What your patch does is just avoiding a race between the
EP halt and queueing URB afterwards -- with the gadget not intending to
abort that URB but which gets aborted. This for example happens when the
file-backed storage gadget sends short data in response to the MODE
SENSE command and the necessairily halts the endpoint, then queues the
CSW but the MUSB gadget driver would prematurely giveback that URB afetr
having sent STALL. This results in host's failure to get any CSW and USB
reset finally ensues...
And now please tell me how can the driver differ the URB that you
want to abort from one that you must not abort?
And also tell me how you fix helps the driver to abort in-flight
requests
when you specifically disallow it to do so.
<SWAMI> Pl. refer to the SENT Stall handling for further information. This is
the code that has been in the community for a very long time now. The patch just
adds check for busy status before returning the IO.
Please take the time to study the *real* community driver, not only
what you have in 2.6.10 LSP -- ep->busy does *not* correspond to
endpoint having I/O in progress, so your changhe is meanigless.
WBR, Sergei
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
