On 04/22/15 09:05, Tian, Feng wrote: > Hi, Baranee > > Yes, XHCI spec has the same statement for short packet. > > Quote from Xhci spec: > A Short Packet will trigger the generation of a Transfer Event TRB on the > Event Ring if the Interrupt-on-Short > (ISP) or Interrupt On Completion(IOC) flags are set in the TRB that the Short > Packet was detected on. The > Completion Code field of the Transfer Event shall be set to Short Packet. The > Length field of the Transfer > Event shall be set to the residual number of bytes not written to the > Transfer TRBs' data buffer. > > I attach the final git patch for your review. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Feng Tian <feng.t...@intel.com>
For the MdeModulePkg patch: Reviewed-by: Laszlo Ersek <ler...@redhat.com> (I checked the following in the spec: - 6.4.1.1 Normal TRB - 6.4.1.2.2 Data Stage TRB - 6.4.1.3 Isoch TRB and indeed the TRB Transfer Length field in all of them occupies the same bits.) For the SourceLevelDebugPkg patch: There is no explicit check for TRBPtr->Type; we don't verify the type of the original TRB that the Event TRB points to. Is it safe to assume that the original TRB was one of the above types, and therefore TRANSFER_TRB_NORMAL.Length is safe to get its length? Thanks Laszlo > > Thanks > Feng > > -----Original Message----- > From: Anbazhagan, Baraneedharan [mailto:anbazha...@hp.com] > Sent: Wednesday, April 22, 2015 06:46 > To: Laszlo Ersek; Tian, Feng > Cc: edk2-devel@lists.sourceforge.net > Subject: RE: [edk2] XHCI data transfer size > > Hi Feng, > Verified that incase of short packet, EvtTrb->Lenth reports remaining size > and change should be okay. Assuming code change will be applied to below 3 > modules. > > MdeModulePkg\Bus\Pci\XhciDxe\XhciSched.c > MdeModulePkg\Bus\Pci\XhciPei\XhciSched.c > SourceLevelDebugPkg\Library\DebugCommunicationLibUsb3\DebugCommunicationLibUsb3Transfer.c > > Also, could you please rename Lenth to Length in XhciDxe module. Thanks. > > -Baranee > > > CONFIDENTIALITY NOTICE: The information contained in this e-mail and any > accompanying documents may contain information which is HP confidential or > otherwise protected from disclosure. This transmission may also be protected > by the attorney-client privilege, the attorney work-product privilege, or > both. If you are not the intended recipient of this message, or if this > message has been addressed to you in error, please immediately alert the > sender by reply e-mail and then delete this message, including any > attachments. Any dissemination, distribution or other use of the contents of > this message by anyone other than the intended recipient is strictly > prohibited. > > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Tuesday, April 21, 2015 6:53 AM > To: Tian, Feng > Cc: edk2-devel@lists.sourceforge.net; Anbazhagan, Baraneedharan > Subject: Re: [edk2] XHCI data transfer size > > On 04/21/15 09:29, Tian, Feng wrote: >> Laszlo, >> >> Please note the code is checking the field of Event TRBs, so you need to >> refer to Xhci 1.0 spec Table 85 for the meaning of TRB Transfer Length >> field. > > Right; what I have downloaded is 1.1, but table 91 ("Offset 08h - Transfer > Event TRB Field Definitions") indeed says about "TRB Transfer Length" that it > carries the residue. (Even if the receive transfer is terminated with a short > packet.) > > There are cases when the field seems to be set to the EDTLA instead > (dependent on the Event Data (ED) bit, and the condition code), but those > don't seem to be handled here at all. > > So I guess this change should be okay (I'm definitely new to XHCI, beyond the > one QEMU patch that I mentioned). > > Thanks > Laszlo > >> For your concern on Isoch Endpoint Error Handling, the Xhci spec states it's >> for Error case rather than success case. >> >> Thanks >> Feng >> >> -----Original Message----- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Tuesday, April 21, 2015 15:09 >> To: Tian, Feng >> Cc: edk2-devel@lists.sourceforge.net; Anbazhagan, Baraneedharan >> Subject: Re: [edk2] XHCI data transfer size >> >> On 04/21/15 05:04, Tian, Feng wrote: >>> Baranee & Laszlo, >>> >>> I agree this is a bug and I made a unit test on this logic and found why >>> this issue didn't get raised before is because Fat&DiskIo driver will split >>> big data transfer request (>64K) to multiple of 64K. This way will cause a >>> request, that is a URB, only needs a transfer TRB, so it will not return >>> wrong completed data length as the Urb->DataLen is only accumulated once >>> for every URB request. >>> >>> But if a URB request needs multiple TRBs, then the Urb->Completed is >>> calculated wrongly. >>> >>> Here is my proposed fix, could you help review it? >>> >>> Index: XhciSched.c >>> =================================================================== >>> --- XhciSched.c (revision 17189) >>> +++ XhciSched.c (working copy) >>> @@ -1137,7 +1137,7 @@ >>> if ((TRBType == TRB_TYPE_DATA_STAGE) || >>> (TRBType == TRB_TYPE_NORMAL) || >>> (TRBType == TRB_TYPE_ISOCH)) { >>> - CheckedUrb->Completed += (CheckedUrb->DataLen - EvtTrb->Lenth); >>> + CheckedUrb->Completed += >>> + (((TRANSFER_TRB_NORMAL*)TRBPtr)->Length - EvtTrb->Lenth); >>> } >>> >>> The TRBPtr->Length field means expected transfer length, and the >>> EvtTrb->Lenth means the residual number of bytes not transferred. >> >> Can you please explain why "EvtTrb->Lenth" is the residue (ie. the number of >> bytes *not* transferred) instead of the number of bytes that have been >> transferred? >> >> My understanding would be helped most if you could provide: >> - an explanation of the code that implies this (it's quite complicated >> code...) >> - and a reference from the XHCI spec that the code implements. >> >> I'm asking because (as I wrote earlier) the XHCI spec defines >> EvtTrb->Lenth with different meanings in different contexts. In some >> contexts it is indeed a residue, but in other contexts EvtTrb->Lenth is the >> number of bytes actually transferred. I'm unsure which case we have at hand. >> >> Thanks! >> Laszlo >> > > ------------------------------------------------------------------------------ BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT Develop your own process in accordance with the BPMN 2 standard Learn Process modeling best practices with Bonita BPM through live exercises http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_ source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
