Subbrathnam, Swaminathan wrote:
Sergei,
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
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.
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.
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?
Regards
Swami
-----Original Message-----
From: Sergei Shtylyov [mailto:[EMAIL PROTECTED]
Sent: Wednesday, December 03, 2008 10:43 PM
To: Subbrathnam, Swaminathan
Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED];
[email protected]
Subject: Re: [PATCH 3/3] MUSB : Fix for STALL handling in musb gadget code
Swaminathan S wrote:
Fixes the SENTSTALL handling code to look for EP busy status before
returning the request. If EP is not busy do not giveback the request and
restart the request in the context of CLEAR STALL feature from the
host.
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.
Signed-off-by: Swaminathan S <[EMAIL PROTECTED]>
---
drivers/usb/musb/musb_gadget.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index d6a802c..491a3e7 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -335,10 +335,10 @@ static void txstate(struct musb *musb, struct
musb_request *req)
#elif defined(CONFIG_USB_TI_CPPI_DMA)
/* program endpoint CSR first, then setup DMA */
csr &= ~(MUSB_TXCSR_AUTOSET
- | MUSB_TXCSR_DMAMODE
| MUSB_TXCSR_P_UNDERRUN
| MUSB_TXCSR_TXPKTRDY);
- csr |= MUSB_TXCSR_MODE | MUSB_TXCSR_DMAENAB;
+ csr |= MUSB_TXCSR_MODE | MUSB_TXCSR_DMAENAB |
+ MUSB_TXCSR_DMAMODE;
What this has to do with STALL handling?
So?
WBR, Sergei
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
