On Fri, Oct 11, 2013 at 11:40:07AM +0300, Xenia Ragiadakou wrote: > Hi Sarah, > > On 10/10/2013 09:25 PM, Sarah Sharp wrote: > >Hi Xenia, > > > >On Mon, Oct 07, 2013 at 06:52:39PM +0300, Xenia Ragiadakou wrote: > >>The previous patch on the endpoint reset uses the already implemented function > >>xhci_drop_endpoint() to reduce code duplication. However, the way that xhci > >>updates the last valid endpoint in the Input Slot Context, when an endpoint > >>is dropped, can lead to incosistent value for the last valid endpoint. > >>That can happen when the endpoint to be reset has index smaller than the > >>current last valid endpoint. In that case the last valid endpoint will end up > >>being updated with a smaller value and if there are valid endpoints with index > >>higher than the index of the reset endpoint, the xHC will consider them as > >>invalid and the transactions to these endpoints will break. > >> > >>This patch updates the last valid endpoint with the index of the first not > >>disabled endpoint, starting from the current last valid endpoint and skipping > >>the dropped endpoint. > >I notice that in section 4.6.6 of the 08/2012 errata to the xHCI 1.0 > >spec, that when the host executes the Configure Endpoint command, it > >must: > > > >"Set the Context Entries field in the Output Slot Context to the index > >of the last valid Endpoint Context in its Output Device Context > >structure, which shall be greater to or equal to the value of the > >Context Entries field in the Input Slot Context." > > > >There was a discussion about a similar patch to this one in June: > > > >http://marc.info/?l=linux-usb&m=137158978503741&w=2 > > > >The end result was that I didn't take the patch, and VMWare fixed their > >virtual xHCI host. > > Hmmm, I was not aware of that patch. > > > > >Is this patch necessary for your first patch to work? > > No, it is not necessary that's why before i send this patchset i > decided to send it as second and not first. Thanks for doing making the patchset that way, it was a good choice. :) > The reason i send it is to cover a possible failure case because I > have not found a satisfying way to test the reset endpoint patch > exhaustively under all possible contexts. > > >What happens if you don't have this patch applied? Did you actually experience the > >transactions to those endpoints breaking, or do you just theorize that > >it could happen? > > > >Sarah Sharp > > While i was working at this fix i met some problems that I thought > at the moment that they were due to the fact that Context Entries > were updated with a value smaller than the actual last valid > endpoint and at that point i changed the implementation of last > valid endpoint update. But since then I had changed the code so > maybe they were due to some other bug. When later I tested my last > version (without the last valid endpoint fix) I have not met > problems but I still considered this patch necessary because I was > theorizing that without it something will break. > > The main reasons that made me come to this conclusion was the fact > that Context Entries by being updated with a value other than the > last valid endpoint will be inconsistent with the definition in xhci > spec (revision 1.0 5/21/10) "Context Entries. This field identifies > the index of the last valid Endpoint Context within this Device > Context structure." and that in 6.4.3.8 Stop Endpoint Command TRB > field definitions for Endpoint ID field it states that "Valid values > are ‘1’ to Slot Context Context Entries", from which i hypothesize > that when Context Entries are set with a smaller value a subsequent > Stop Command to an endpoint with higher index will fail. Yeah, that's a confusing sentence, which is why it was clarified in the later spec revision. I think when they said "This field identifies the index of the last valid Endpoint Context within this Device Context structure", by "this" they meant the Input Device Context structure. Meaning the last valid context should be the last added or changed context index. And then the host should update the last valid Endpoint Context field in the Output Device Context once it drops, adds, or changes all contexts. And I agree that the field initially seems redundant, since the hardware could just look at the add flags and figure out which bit is the last set bit. But the xHCI hardware doesn't have access to CPU instructions to find the last set bit, so when you look at it from a hardware perspective, it sort of makes sense. > I compile now to see if that actually can happen. However, the best > person to answer this question is an xHC architect. Will the xHC > take under consideration the value in Context Entries field of the > Output Slot Context only when a Configure Endpoint command is issued > or also for other commands in order to check if the endpoint > affected by the command is a valid endpoint? I'm pretty sure I asked the xHCI spec architect about this, and he tried to clarify the spec to say that the last valid Endpoint Context only referred to contexts in the input context, not output context. > The fact is that if somebody attempts to reset an endpoint with > index smaller than the actual last valid endpoint in Device Context, > without that fix the Context Entries field of the Output Slot > Context will be set to value lower than the actual last valid > endpoint. > > I don't know if I made clear the reasons of my confusion that made > me consider this fix necessary. You did make it clear where your confusion came from. :) Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
