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:

quoted

On 2023-06-29 19:29, Ricardo Ribalda wrote:

quoted

Hi Robin

On Thu, 29 Jun 2023 at 20:11, Robin Murphy [off-list ref]

wrote:

quoted

quoted

quoted

On 2023-06-28 22:00, Ricardo Ribalda wrote:

quoted

Allow devices to have dma operations beyond 64K, and avoid

warnings such

quoted

quoted

quoted

quoted

as:

Hang on, is this actually correct? I just had a vague memory of

XHCI

quoted

quoted

quoted

having some restrictions, and sure enough according to the spec

it

quoted

quoted

quoted

*does* require buffers to be split at 64KB boundaries, since

that's the

quoted

quoted

quoted

maximum length a single TRB can encode - that's exactly the kind

of

quoted

quoted

quoted

constraint that the max_seg_size abstraction is intended to

represent,

quoted

quoted

quoted

so it seems a bit odd to be explicitly claiming a very different

value.

quoted

quoted

quoted

Thanks,
Robin.

I think we had a similar discussion for  93915a4170e9 ("xhci-pci:

set

quoted

quoted

the dma max_seg_size")
on

https://lore.kernel.org/all/1fe8f8a7-c88f-0c91-e74f-4d3f2f885c89@linux.intel.com/ (local)

quoted

quoted

Preferred max segment size of sg list would be 64k as xHC hardware

has

quoted

quoted

64k TRB payload size
limit, but xhci driver will take care of larger segments,

splitting

quoted

quoted

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.

quoted

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.

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel