Message-ID: <0bce0b2f-1b74-4ffb-b34d-601ff5bd5490@linux.intel.com>
Date: Thu, 27 Feb 2025 14:10:04 +0200
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: Michał Pecio <michal.pecio@...il.com>
Cc: Mathias Nyman <mathias.nyman@...el.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Niklas Neronin <niklas.neronin@...ux.intel.com>, linux-usb@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 0/5] xHCI: Isochronous error handling fixes and
 improvements

On 27.2.2025 0.05, Michał Pecio wrote:
> On Wed, 26 Feb 2025 14:41:44 +0200, Mathias Nyman wrote:
>> Updated my for-usb-next branch to this v3 version
> 
> 
> A few remarks regarding "Add helper to find trb from its dma address":
> 
> xhci_dma_to_trb():
> This function could use xhci_for_each_ring_seg.

Good point

> The use of in_range() perhaps deserves a comment, because its
> correctness is not as obvious as it might seem.
> 
> Long story short, my own version:
> 
> /*
>   * Look up a TRB on the ring by its DMA address, return NULL if not found.
>   * Start from deq_seg to optimize for event handling use.
>   *
>   * Note: false positive is possible if dma < TRB_SEGMENT_SIZE *and*
>   * seg->dma > (dma_addr_t) 0 - TRB_SEGMENT_SIZE, but that shouldn't
>   * happen if seg->dma is an allocation of size TRB_SEGMENT_SIZE.
>   */

True, but as you said this shouldn't happen as we allocate TRB_SEGMENT_SIZE bytes
starting at seg->dma, so seg->dma should be at least TRB_SEGMENT_SIZE bytes from
max u64 (or u32)

We can also use "if (dma >= seg->dma && (dma - seg->dma) < TRB_SEGMENT_SIZE)"
instead of in_range(). It's a bit uglier, but we can skip additional notes.
    
> static union xhci_trb *xhci_dma_to_trb(struct xhci_ring *ring, dma_addr_t dma)
> {
>         struct xhci_segment *seg;
> 
>         xhci_for_each_ring_seg(ring->deq_seg, seg)
>                 if (in_range(dma, seg->dma, TRB_SEGMENT_SIZE))
>                         return seg->trbs + (dma - seg->dma) / sizeof(seg->trbs[0]);
> 
>         return NULL;
> }
> 
>> +       struct xhci_td *matched_td;
> 
> This variable is only used as bool so it could be declared as such.
> Other places still use 'td' and assume that it equals 'matched_td'.
> And that's OK because there is no need for iterating further after
> the matching TD is found.

True, it could be just a bool

> 
>> +       /* find the transfer trb this events points to */
>> +       if (ep_trb_dma)
>> +               ep_trb = xhci_dma_to_trb(ep_ring, ep_trb_dma);
> 
> This may deserve a dedicated warning. It's a pathology. Either the
> event is bogus due to internal corruption in the HC, or it's executing
> TRBs from a wrong ring due to damaged/ignored Link TRB or bad Set Deq.
> Or we completely screwed up and are looking at a wrong ep_ring here.
> 
>> -       if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !ep_trb_dma)
>> +       if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !ep_trb)
>>                 return 0;
> 
> Good idea. I think I would go further and refuse to handle anything
> when (ep_trb_dma && !ep_trb). Nothing is going to match, nothing good
> will come from trying as far as I see.
> 
> But that's a behavior change, so maybe material for a separate patch.

Idea of this patch is to slowly migrate handle_tx_event() towards the
vision in my feature_transfer_event_rework branch

https://web.git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_transfer_event_rework

Niklas is working towards a similar goal, and he just informed me that this
patch conflicts a bit with his plan to get there, so I might drop this.

Thanks for looking at it.

-Mathias

Powered by blists - more mailing lists