>> + do {
>> + if (cycle_found) {
>> + if (new_seg->trbs > cur_td->last_trb ||
>> + &new_seg->trbs[TRBS_PER_SEGMENT - 1] < cur_td->last_trb)
>> + new_deq = &new_seg->trbs[TRBS_PER_SEGMENT - 1];
>> + } else if (xhci_trb_virt_to_dma(new_seg, new_deq)
>> + == (dma_addr_t)(hw_dequeue & ~0xf))
>> + cycle_found = true;
>> + tmp_trb = new_deq;
>> + next_trb(xhci, ep_ring, &new_seg, &new_deq);
>> +
>> + if (cycle_found &&
>> + TRB_TYPE_LINK_LE32(tmp_trb->generic.field[3]) &&
>> + tmp_trb->generic.field[3] & cpu_to_le32(LINK_TOGGLE))
>> + state->new_cycle_state ^= 0x1;
>> +
>> + } while (tmp_trb != cur_td->last_trb && new_deq != ep->ring->dequeue);
>
> What happens when new_deq == ep->ring->dequeue? I'm not sure if
> there's anything good you could do (save for reinitializing the whole
> ring), and it shouldn't happen anyway, but you should probably at
> least print some kind of warning.
Printing a warning sounds reasonable to begin with.
>
>> +
>> + state->new_deq_seg = new_seg;
>> + state->new_deq_ptr = new_deq;
>>
>> /* Don't update the ring cycle state for the producer (us). */
>> xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
>> --
>> 1.8.3.2
>>
>
> Looks pretty good! Merging both searches together like that really
> does make it look much simpler.
>
> But it makes the slightly dangerous assumption that hw_dequeue is at
> most one TRB after last_trb (because once you found last_trb you
> always exit even if cycle_found hasn't been set yet). I'm worried
> about what Maciej's controller will do if it hits a Stall Error on the
> last Normal TRB before a Link TRB with cycle change (a hard case to
> test, unfortunately). Will it just put hw_dequeue on the Link TRB, or
> will it keep advancing further to the first TRB of the next segment?
> In the latter case, your cycle state would be wrong with this code
> (because you set the dequeue pointer to the Link TRB but give it the
> cycle state from the next segment).
> --
> 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
>
I guess we can't rule out the hw_dequeue advancing to next segment for some controllers
I'll create a new version where the old cycle_state from the endpoint is used until hw_dequeue is found.
Toggle checking needs to be done even if hw_dequeue isn't found yet as well
-Mathias
--
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
