Re: [PATCH v3 2/3] usb: xhci: Fix TD invalidation under pending Set TR Dequeue

["Michał Pecio" <michal.pecio@gmail.com>]

On Thu, 31 Oct 2024 15:57:16 +0200, Mathias Nyman wrote:
> On 31.10.2024 11.44, Michal Pecio wrote:
> > +			case TD_CLEARING_CACHE: /* set TR deq command completed */
> > +				td->cancel_status = TD_CLEARED;  
> 
> This should only happen if 'Set TR Deq' fails as in success cases the
> hw_deq is moved past this TD. when it fails the cache may not be
> cleared. i.e. rd is not TD_CLEARED
Thanks, that was a bug, completely unintended. I should have placed
this line elsewhere.
 
> We do have the same problem in current code, but moving it here makes
> that even harder to fix.
Right, I will no longer try to move this code at all.

> xhci_invalidate_cancelled_tds() will now also perform some extra work
> on each already cleared td that is still in the ep->cancelled_td_list
I think this shouldn't be a problem, because every call to invalidate()
is now followed by a call to giveback(), so a future invalidate() should
never seen stale cleared TDs left by a past invalidate().

> I think its clearer to run though the cancelled td list above, and
> only call xhci_invalidate_cancelled_tds() if needed.
> 
> i.e.
> 
> list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list,
> cancelled_td_list) { switch (td->cancel_status) {
> 	case TD_CLEARING_CACHE:
> 		td->cancel_status = TD_CLEARED;
> 		break;
> 	case TD_CLEARING_CACHE_DEFERRED:
> 	case TD_DIRTY:
> 		pending_td_cancels = true;
> 		break;
> 	default:
> 		break;
> 	}
> }
> ...
> if (pending_td_cancels)
> 	xhci_invalidate_cancelled_tds(ep);
I have done something similar, but without an extra bool. Simply test
for unempty cancelled_td_list.


Regards,
Michal