Re: [PATCH] usb: xhci-mtk: set the dma max_seg_size

[Chunfeng Yun (云春峰)]

On Fri, 2023-06-30 at 14:25 +0300, Mathias Nyman wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 29.6.2023 22.19, Robin Murphy wrote:
> > On 2023-06-29 19:29, Ricardo Ribalda wrote:
> >> Hi Robin
> >>
> >> On Thu, 29 Jun 2023 at 20:11, Robin Murphy <robin.murphy@xxxxxxx>
> wrote:
> >>>
> >>> On 2023-06-28 22:00, Ricardo Ribalda wrote:
> >>>> Allow devices to have dma operations beyond 64K, and avoid
> warnings such
> >>>> as:
> >>>
> >>> Hang on, is this actually correct? I just had a vague memory of
> XHCI
> >>> having some restrictions, and sure enough according to the spec
> it
> >>> *does* require buffers to be split at 64KB boundaries, since
> that's the
> >>> maximum length a single TRB can encode - that's exactly the kind
> of
> >>> constraint that the max_seg_size abstraction is intended to
> represent,
> >>> so it seems a bit odd to be explicitly claiming a very different
> value.
> >>>
> >>> Thanks,
> >>> Robin.
> >>
> >> I think we had a similar discussion for  93915a4170e9 ("xhci-pci:
> set
> >> the dma max_seg_size")
> >> on
> >> 
> https://lore.kernel.org/all/1fe8f8a7-c88f-0c91-e74f-4d3f2f885c89@xxxxxxxxxxxxxxx/
> >>
> >> ```
> >> Preferred max segment size of sg list would be 64k as xHC hardware
> has
> >> 64k TRB payload size
> >> limit, but xhci driver will take care of larger segments,
> splitting
> >> them into 64k chunks.
> >> ```
> > 
> > OK, but it still seems off to me to claim to support something that
> the hardware doesn't support, and the driver has to fake, especially
> when it's only to paper over a warning which isn't even the driver's
> fault in the first place.
> 
> xHC Hardware has odd alignments and size restrictions that the driver
> anyway need to sort out.
> The 64K is already fake, it's the most common max supported size for
> TRBs, but not always true.
> Varies depending on TRB location in TRB ring.
> 
> xhci driver can handle any size.
> 
> > 
> > The aim of the DMA_API_DEBUG_SG warnings wasn't to go round blindly
> adding dma_set_max_seg_size(UINT_MAX) all over the place, it was
> always to consider whether the dma_map_sg() call and/or the
> scatterlist itself are right, just as much as whether the driver may
> have forgotten to set an appropriate parameter. As I've already
> raised, in this particular case I think it's actually the debug check
> that's misplaced, since it's not dma_map_sg() anyway, but as it
> stands, the implementations of dma_alloc_noncontiguous() are
> definitely doing the wrong thing with respect to what it is then
> asking to validate.
> 
> Agree that this seems to be an issue in the DMA debugging side.
> Would it need to take into account cases where device driver can
> support different sizes than the host controller?

static inline unsigned int dma_get_max_seg_size(struct device *dev)
{
    if (dev->dma_parms && dev->dma_parms->max_segment_size)
        return dev->dma_parms->max_segment_size;
    return SZ_64K;
}

By the way, why it returns SZ_64K, but not UINT_MAX?

I find many drivers set max_segment_size as UINT_MAX, seem it's better
to return UNIT_MAX by default, if there is no limitation for a driver.


> 
> > 
> > Unless there is some known reason to make this change to any USB
> host controller *other* than that someone sees UVC allocate a >64KB
> buffer via this path on a system which happens to have that
> particular HCD, it is not the right change to make.
> 
> This would be all USB 3.x hosts, from all vendors.
> 
> keeping the 64K max seg size, and fixing the dma debug side would be
> optimal, but until that gets done I think
> we can take this oneliner as it resolves a real world issue where USB
> isn't working.
> 
> Thanks
> -Mathias
>