Hello.
Subbrathnam, Swaminathan wrote:
This will be my last post on for this thread. I do not want to waste community
bandwidth further in discussing this issue.
That's a good decisoion. Stop wasting everybody's time by restating
your incorrect arguments.
My response inlined for your comments in this thread.
Regards
swami
-----Original Message-----
From: Sergei Shtylyov [mailto:[EMAIL PROTECTED]
Sent: Friday, December 05, 2008 4:12 PM
To: Subbrathnam, Swaminathan
Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; davinci-linux-open-
[EMAIL PROTECTED]
Subject: Re: [PATCH 3/3] MUSB : Fix for STALL handling in musb gadget code
Hello.
Subbrathnam, Swaminathan wrote:
Sergei,
My comments inlined.
Regards
Swami
-----Original Message-----
From: [EMAIL PROTECTED] [mailto:linux-usb-
[EMAIL PROTECTED] On Behalf Of Sergei Shtylyov
Sent: Thursday, December 04, 2008 10:36 PM
To: Subbrathnam, Swaminathan
Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; davinci-linux-
[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. :-)
<SWAMI> Many do have it and hence the contributions to the community to get
things to a better shape that was existed until now. You too are welcome to try it
instead of commenting on things that you do not understand.
You've not proven yourself to be up to the task of judging what do I
and do not undestand. I can say that after having seein (and having to
fix) enough of your MUSB code.
Your basic premise that once cannot "reliably" stall an inflight IO
Your patch is unable to reliable abort the in-flight requests and
prevent aborting the requests queued after halting EP, that's all I have
to say.
is proven wrong based on the posts and proof of the same and yet you continue
to argue. Strange !!!!
You have only proven yourself inconsistent.
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?
<SWAMI> Again you are hallucinating here.
Don't know about you, but I don't take hallucinogens.
Did I ever refer to 2.6.10 driver in my above comment?
No, you haven't indeed. But I totally fail to take your claims about
in-flight requests for granted WRT the current community code.
All the efforts and associated verification is in the context of GIT
community driver only.
Then please do study the code and review your argumenatation about
aborting in-flight requests. It doesn't match your description of its
behavior.
You *can* stall them, but they will be left "hanging" in the queue after
that -- due to your check. I do not think this is a correct behavior.
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...
<SWAMI> Sergei, simply put it means that hardware can do it and hence the driver which
relies on harware can handle this situation "reliably". This makes the driver and
the associated hardware more robust to handle any situation thrown at it.
That's *over* simply put. The hardware *cannot ever* guarantee the
software's consistent behavior. This is not a proof (as I've already
written).
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".
<SWAMI> Basing on your own argument, the programmers were limited by their hardware and
hence chose to "work around" it.
The programmers are usually mostly limited by their skills and abilities.
In this case we do the right thing.
No, you don't. At least not as you're describing that it should work.
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.
<SWAMI> One can continue to discuss as one see's fit even if the basic premise
of one's argument has been quashed with hard proof. Nothing stops them from continue
to discuss/argue.
If you're continuing to ignore my appeals to studying the actual code
what can I do?
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.
<SWAMI pasting the relevant piece of code here to show the abort happening in
the g_tx code
if (csr & MUSB_TXCSR_P_SENTSTALL) {
csr |= MUSB_TXCSR_P_WZC_BITS;
csr &= ~MUSB_TXCSR_P_SENTSTALL;
musb_writew(epio, MUSB_TXCSR, csr);
if (dma_channel_status(dma) == MUSB_DMA_STATUS_BUSY) {
dma->status = MUSB_DMA_STATUS_CORE_ABORT;
musb->dma_controller->channel_abort(dma);
}
if (request && musb_ep->busy)
musb_g_giveback(musb_ep, request, -EPIPE);
You're aborting DMA but not giving back the request...
break;
}
Strange that you did not notice it.
Did not notice what? There's not much to notice there. :-)
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.
<SWAMI> Again pl. refer the patch that was submitted yesterday for clarifying
yourself.
I'm seeing it just now -- let me look... (Unfortunately, that patch
we're talking about have been previously committed to our internal tree
without your other change.)
I must note however that it would really have saved everybody's time
if you've made your patches more concentrated on the task (without
sideband changes) and posted them in the *correct order*.
WBR, Sergei
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
