Hello.
Subbrathnam, Swaminathan wrote:
Sergei,
My comments inlined.
Regards
Swami
-----Original Message-----
From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Sergei Shtylyov
Sent: Thursday, December 04, 2008 10:36 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
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.
<SWAMI> Pasting the MUSB spec w.r.t stall handling here for reference.
| D4 | SendStall | The CPU writes a 1 to this bit to issue a STALL
handshake to an IN token. The CPU clears this
bit to terminate the stall condition. Note:
This bit has no effect where the endpoint is being
used for
Isochronous transfers.
I've read the spec, thank you.
MUSB controller spec does not specify that one cannot stall in presence of
in-flight IO.
Why it should? A programmer should have his own a reasoning. :-)
Pl. refer to the MUSB controller spec for further details. I am attaching snapshot of
MUSB driver debug traces and USB analyzer traces for reference, were we are stalling in
presence of in-flight IO and the MUSB driver with the patch is able to recover from it
"reliably". I hacked the MUSB driver for demonstrating this capability.
I keep telling you that your code doesn't make sense in the context
of the community driver, you keep telling me that it worked in context
of 2.6.10 driver heavily modified by you. Have you examined the patch
behavior with the community driver?
Interestingly as you can see even after setting STALL with TX FIFO being full the
mentor controller flushes the FIFO when it acknowledges (txcsr -> 0x2030)
through SENT stall interrupt.
Sigh, that's not a proof for anything. Whether you can do somethuing
with hardware doesn't necessarily mean that it's a correct thing to do...
The fact that other drivers do not do so does not mean MUSB cannot do. There might be bugs in other USB controllers that prevent them from doing it. Mentor controller does not have that limitation.
Or it might be that the programmers just had better reasoning to not
do this than just "if it's works, then it's right thing to do and we
should do it".
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.
<SWAMI> Pl. refer to my above explanation and the attached file to convince yourself
that it can be done "reliably".
If it did convinced me, I wouldn't continue this discussion.
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.
<SWAMI> I think you need to review the patch correctly before coming to wrong
conclusions. There could be other requests that are queued which would be re-started
once the current stall condition is cleared and in-flight IO if any is aborted. That
is what this patch enables.
Yes, it does enable this BUT at the expense of not being able to
abort the in-glight I/O -- which as you claimed is your goal.
Instead of freeing the wrong request (original implementation was doing it,
freeing the CSW in MSC case resulting in device reset) we free the correct
request by verifying if the STALL was actually set on that request through the
busy field status.
No need to explain me what the old code did, I've seen that myself
and explained it myself in the prior msg.
That ep->busy flag you're referring to does *not* get set on halting the
endpoint. Please take the time to study the community code already.
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?
<SWAMI> Pl. refer to my preceding explanation for the same.
It didn't really explain anything to me WRT the community code.
Please look at that code again.
WBR, Sergei
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
