linux-kernel - Re: [PATCH] usb: xhci-mtk: set the dma max_seg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <516070d1504e6ff5ef6a81db75851bf86296e47d.camel@mediatek.com>
Date:   Tue, 4 Jul 2023 05:57:58 +0000
From:   Chunfeng Yun (云春峰) 
        <Chunfeng.Yun@...iatek.com>
To:     "robin.murphy@....com" <robin.murphy@....com>,
        "mathias.nyman@...ux.intel.com" <mathias.nyman@...ux.intel.com>,
        "ribalda@...omium.org" <ribalda@...omium.org>
CC:     "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "mathias.nyman@...el.com" <mathias.nyman@...el.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH] usb: xhci-mtk: set the dma max_seg_size

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@....com>
> 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@linux.intel.com/
> >>
> >> ```
> >> 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
>