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
