Sergei, This will be my last post on for this thread. I do not want to waste community bandwidth further in discussing this issue.
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. Your basic premise that once cannot "reliably" stall an inflight IO is proven wrong based on the posts and proof of the same and yet you continue to argue. Strange !!!! > > > 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. Did I ever refer to 2.6.10 driver in my above comment? All the efforts and associated verification is in the context of GIT community driver only. > > 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. > > 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. In this case we do the right thing. > > >> 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. > > >> 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); break; } Strange that you did not notice it. > > 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. > >> 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
