[PATCH] usb: core: do not allocate buffers from a DMA zone unnecessarily

[PATCH] usb: core: do not allocate buffers from a DMA zone unnecessarily

[Petr Tesarik]

There is no reason to keep GFP zone modifiers in hcd_buffer_alloc():
- PIO mode can use any kernel-addressable memory
- dma_alloc_coherent() ignores memory zone bits

The only place where GFP zone bits are used is when dma_pool_alloc() needs
to allocate a new struct dma_page, but that is an omission rather than
intention.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 drivers/usb/core/buffer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index 87230869e1fa..2be4c2a5b214 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -108,10 +108,6 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
 }
 
 
-/* sometimes alloc/free could use kmalloc with GFP_DMA, for
- * better sharing and to leverage mm/slab.c intelligence.
- */
-
 void *hcd_buffer_alloc(
 	struct usb_bus		*bus,
 	size_t			size,
@@ -128,6 +124,9 @@ void *hcd_buffer_alloc(
 	if (hcd->localmem_pool)
 		return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
 
+	/* DMA API ignores zone modifiers, and PIO should go with default */
+	mem_flags &= ~GFP_ZONEMASK;
+
 	/* some USB hosts just use PIO */
 	if (!hcd_uses_dma(hcd)) {
 		*dma = ~(dma_addr_t) 0;
-- 
2.48.1

[PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Petr Tesarik]

Remove a misleading comment and issue a warning if a zone modifier is
specified when allocating a hcd buffer.

There is no valid use case for a GFP zone modifier in hcd_buffer_alloc():
- PIO mode can use any kernel-addressable memory
- dma_alloc_coherent() ignores memory zone bits

This function is called by usb_alloc_coherent() and indirectly by
usb_submit_urb(). Despite the comment, no in-tree users currently pass
GFP_DMA.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 drivers/usb/core/buffer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index 87230869e1fa..10844cd42e66 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -108,10 +108,6 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
 }
 
 
-/* sometimes alloc/free could use kmalloc with GFP_DMA, for
- * better sharing and to leverage mm/slab.c intelligence.
- */
-
 void *hcd_buffer_alloc(
 	struct usb_bus		*bus,
 	size_t			size,
@@ -128,6 +124,12 @@ void *hcd_buffer_alloc(
 	if (hcd->localmem_pool)
 		return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
 
+	/*
+	 * Zone modifiers are ignored by DMA API, and PIO should always use
+	 * GFP_KERNEL.
+	 */
+	WARN_ON_ONCE(mem_flags & GFP_ZONEMASK);
+
 	/* some USB hosts just use PIO */
 	if (!hcd_uses_dma(hcd)) {
 		*dma = ~(dma_addr_t) 0;
-- 
2.48.1

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Petr Tesarik]

On Tue, 25 Mar 2025 14:40:00 +0100
Petr Tesarik <ptesarik@suse.com> wrote:

> Remove a misleading comment and issue a warning if a zone modifier is
> specified when allocating a hcd buffer.
> 
> There is no valid use case for a GFP zone modifier in hcd_buffer_alloc():
> - PIO mode can use any kernel-addressable memory
> - dma_alloc_coherent() ignores memory zone bits
> 
> This function is called by usb_alloc_coherent() and indirectly by
> usb_submit_urb(). Despite the comment, no in-tree users currently pass
> GFP_DMA.

Let me provide a bit of background on this patch. My actual goal is to
remove the DMA zone; I'm now going through core code and removing
GFP_DMA references that somehow look incorrect to me.

I hope this preparation makes it easier to review the removal of
GFP_DMA later.

Petr T

> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
>  drivers/usb/core/buffer.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> index 87230869e1fa..10844cd42e66 100644
> --- a/drivers/usb/core/buffer.c
> +++ b/drivers/usb/core/buffer.c
> @@ -108,10 +108,6 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
>  }
>  
>  
> -/* sometimes alloc/free could use kmalloc with GFP_DMA, for
> - * better sharing and to leverage mm/slab.c intelligence.
> - */
> -
>  void *hcd_buffer_alloc(
>  	struct usb_bus		*bus,
>  	size_t			size,
> @@ -128,6 +124,12 @@ void *hcd_buffer_alloc(
>  	if (hcd->localmem_pool)
>  		return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
>  
> +	/*
> +	 * Zone modifiers are ignored by DMA API, and PIO should always use
> +	 * GFP_KERNEL.
> +	 */
> +	WARN_ON_ONCE(mem_flags & GFP_ZONEMASK);
> +
>  	/* some USB hosts just use PIO */
>  	if (!hcd_uses_dma(hcd)) {
>  		*dma = ~(dma_addr_t) 0;

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Petr Tesařík]

On Tue, 25 Mar 2025 14:40:00 +0100
Petr Tesarik <ptesarik@suse.com> wrote:

> Remove a misleading comment and issue a warning if a zone modifier is
> specified when allocating a hcd buffer.
> 
> There is no valid use case for a GFP zone modifier in hcd_buffer_alloc():
> - PIO mode can use any kernel-addressable memory
> - dma_alloc_coherent() ignores memory zone bits
> 
> This function is called by usb_alloc_coherent() and indirectly by
> usb_submit_urb(). Despite the comment, no in-tree users currently pass
> GFP_DMA.
> 
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>

I know this was posted during the merge window, but that's now over.
Any comment on this patch?

Petr T

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Greg Kroah-Hartman]

On Wed, Apr 09, 2025 at 05:40:36PM +0200, Petr Tesařík wrote:
> On Tue, 25 Mar 2025 14:40:00 +0100
> Petr Tesarik <ptesarik@suse.com> wrote:
> 
> > Remove a misleading comment and issue a warning if a zone modifier is
> > specified when allocating a hcd buffer.
> > 
> > There is no valid use case for a GFP zone modifier in hcd_buffer_alloc():
> > - PIO mode can use any kernel-addressable memory
> > - dma_alloc_coherent() ignores memory zone bits
> > 
> > This function is called by usb_alloc_coherent() and indirectly by
> > usb_submit_urb(). Despite the comment, no in-tree users currently pass
> > GFP_DMA.
> > 
> > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> 
> I know this was posted during the merge window, but that's now over.
> Any comment on this patch?

Please give me a chance to catch up, it's been non-stop conference
travel during the merge window, and through this week :(

thanks,

greg k-h

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Greg Kroah-Hartman]

On Tue, Mar 25, 2025 at 02:40:00PM +0100, Petr Tesarik wrote:
> Remove a misleading comment and issue a warning if a zone modifier is
> specified when allocating a hcd buffer.
> 
> There is no valid use case for a GFP zone modifier in hcd_buffer_alloc():
> - PIO mode can use any kernel-addressable memory
> - dma_alloc_coherent() ignores memory zone bits
> 
> This function is called by usb_alloc_coherent() and indirectly by
> usb_submit_urb(). Despite the comment, no in-tree users currently pass
> GFP_DMA.
> 
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
>  drivers/usb/core/buffer.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> index 87230869e1fa..10844cd42e66 100644
> --- a/drivers/usb/core/buffer.c
> +++ b/drivers/usb/core/buffer.c
> @@ -108,10 +108,6 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
>  }
>  
>  
> -/* sometimes alloc/free could use kmalloc with GFP_DMA, for
> - * better sharing and to leverage mm/slab.c intelligence.
> - */
> -
>  void *hcd_buffer_alloc(
>  	struct usb_bus		*bus,
>  	size_t			size,
> @@ -128,6 +124,12 @@ void *hcd_buffer_alloc(
>  	if (hcd->localmem_pool)
>  		return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
>  
> +	/*
> +	 * Zone modifiers are ignored by DMA API, and PIO should always use
> +	 * GFP_KERNEL.
> +	 */
> +	WARN_ON_ONCE(mem_flags & GFP_ZONEMASK);

You just rebooted the box if this happens, do you REALLY want to do
that?  People generally do not like their data lost :(

Why not just fix the callers, OR if this really isn't going to be
allowed, return an error and just fail the whole submission?  And stick
around to fix up all of the drivers that end up triggering this...

thanks,

greg k-h

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Petr Tesarik]

On Fri, 11 Apr 2025 15:57:19 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Tue, Mar 25, 2025 at 02:40:00PM +0100, Petr Tesarik wrote:
> > Remove a misleading comment and issue a warning if a zone modifier is
> > specified when allocating a hcd buffer.
> > 
> > There is no valid use case for a GFP zone modifier in hcd_buffer_alloc():
> > - PIO mode can use any kernel-addressable memory
> > - dma_alloc_coherent() ignores memory zone bits
> > 
> > This function is called by usb_alloc_coherent() and indirectly by
> > usb_submit_urb(). Despite the comment, no in-tree users currently pass
> > GFP_DMA.
> > 
> > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > ---
> >  drivers/usb/core/buffer.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> > index 87230869e1fa..10844cd42e66 100644
> > --- a/drivers/usb/core/buffer.c
> > +++ b/drivers/usb/core/buffer.c
> > @@ -108,10 +108,6 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
> >  }
> >  
> >  
> > -/* sometimes alloc/free could use kmalloc with GFP_DMA, for
> > - * better sharing and to leverage mm/slab.c intelligence.
> > - */
> > -
> >  void *hcd_buffer_alloc(
> >  	struct usb_bus		*bus,
> >  	size_t			size,
> > @@ -128,6 +124,12 @@ void *hcd_buffer_alloc(
> >  	if (hcd->localmem_pool)
> >  		return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
> >  
> > +	/*
> > +	 * Zone modifiers are ignored by DMA API, and PIO should always use
> > +	 * GFP_KERNEL.
> > +	 */
> > +	WARN_ON_ONCE(mem_flags & GFP_ZONEMASK);  
> 
> You just rebooted the box if this happens, do you REALLY want to do
> that?  People generally do not like their data lost :(

FWIW my box does not reboot on a warning. But I admit there are people
who want to run their systems with panic_on_warn (although I suspect
they already experience some sudden reboots, so they had better be
prepared).

> Why not just fix the callers, OR if this really isn't going to be
> allowed, return an error and just fail the whole submission?  And stick
> around to fix up all of the drivers that end up triggering this...

That's the point. AFAICS there are _no_ in-tree callers that would pass
GFP_DMA or GFP_DMA32 to hcd_buffer_alloc(), directly or indirectly. But
nobody should be tempted to add the flag, because I cannot imagine how
that would ever be the right thing to do.

I can change it back to mem_flags &= ~GFP_ZONEMASK to fix it silently;
I simply thought that driver authors may appreciate a warning that
they're trying to do something silly.

Whatever works for you, but please keep in mind that there seems to be
agreement among mm people that DMA and DMA32 zones should be removed
from the kernel eventually.

Petr T

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Greg Kroah-Hartman]

On Mon, Apr 14, 2025 at 09:02:16AM +0200, Petr Tesarik wrote:
> On Fri, 11 Apr 2025 15:57:19 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Mar 25, 2025 at 02:40:00PM +0100, Petr Tesarik wrote:
> > > Remove a misleading comment and issue a warning if a zone modifier is
> > > specified when allocating a hcd buffer.
> > > 
> > > There is no valid use case for a GFP zone modifier in hcd_buffer_alloc():
> > > - PIO mode can use any kernel-addressable memory
> > > - dma_alloc_coherent() ignores memory zone bits
> > > 
> > > This function is called by usb_alloc_coherent() and indirectly by
> > > usb_submit_urb(). Despite the comment, no in-tree users currently pass
> > > GFP_DMA.
> > > 
> > > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > > ---
> > >  drivers/usb/core/buffer.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> > > index 87230869e1fa..10844cd42e66 100644
> > > --- a/drivers/usb/core/buffer.c
> > > +++ b/drivers/usb/core/buffer.c
> > > @@ -108,10 +108,6 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
> > >  }
> > >  
> > >  
> > > -/* sometimes alloc/free could use kmalloc with GFP_DMA, for
> > > - * better sharing and to leverage mm/slab.c intelligence.
> > > - */
> > > -
> > >  void *hcd_buffer_alloc(
> > >  	struct usb_bus		*bus,
> > >  	size_t			size,
> > > @@ -128,6 +124,12 @@ void *hcd_buffer_alloc(
> > >  	if (hcd->localmem_pool)
> > >  		return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
> > >  
> > > +	/*
> > > +	 * Zone modifiers are ignored by DMA API, and PIO should always use
> > > +	 * GFP_KERNEL.
> > > +	 */
> > > +	WARN_ON_ONCE(mem_flags & GFP_ZONEMASK);  
> > 
> > You just rebooted the box if this happens, do you REALLY want to do
> > that?  People generally do not like their data lost :(
> 
> FWIW my box does not reboot on a warning. But I admit there are people
> who want to run their systems with panic_on_warn (although I suspect
> they already experience some sudden reboots, so they had better be
> prepared).

There are billions of Linux systems out there with panic-on-warn enabled :(

> > Why not just fix the callers, OR if this really isn't going to be
> > allowed, return an error and just fail the whole submission?  And stick
> > around to fix up all of the drivers that end up triggering this...
> 
> That's the point. AFAICS there are _no_ in-tree callers that would pass
> GFP_DMA or GFP_DMA32 to hcd_buffer_alloc(), directly or indirectly. But
> nobody should be tempted to add the flag, because I cannot imagine how
> that would ever be the right thing to do.
> 
> I can change it back to mem_flags &= ~GFP_ZONEMASK to fix it silently;
> I simply thought that driver authors may appreciate a warning that
> they're trying to do something silly.

A warning is fine, but don't reboot a box please.  dev_warn() with a
ratelimit and then return an error perhaps?

> Whatever works for you, but please keep in mind that there seems to be
> agreement among mm people that DMA and DMA32 zones should be removed
> from the kernel eventually.

I agree, they should be removed as they don't do what people think they
do.  So why not just remove them entirely, otherwise are you going to go
and add this type of checking to all bus subsystems?

thanks,

greg k-h

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Petr Tesarik]

On Mon, 14 Apr 2025 09:12:09 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Mon, Apr 14, 2025 at 09:02:16AM +0200, Petr Tesarik wrote:
> > On Fri, 11 Apr 2025 15:57:19 +0200
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >   
> > > On Tue, Mar 25, 2025 at 02:40:00PM +0100, Petr Tesarik wrote:  
> > > > Remove a misleading comment and issue a warning if a zone modifier is
> > > > specified when allocating a hcd buffer.
> > > > 
> > > > There is no valid use case for a GFP zone modifier in hcd_buffer_alloc():
> > > > - PIO mode can use any kernel-addressable memory
> > > > - dma_alloc_coherent() ignores memory zone bits
> > > > 
> > > > This function is called by usb_alloc_coherent() and indirectly by
> > > > usb_submit_urb(). Despite the comment, no in-tree users currently pass
> > > > GFP_DMA.
> > > > 
> > > > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > > > ---
> > > >  drivers/usb/core/buffer.c | 10 ++++++----
> > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> > > > index 87230869e1fa..10844cd42e66 100644
> > > > --- a/drivers/usb/core/buffer.c
> > > > +++ b/drivers/usb/core/buffer.c
> > > > @@ -108,10 +108,6 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
> > > >  }
> > > >  
> > > >  
> > > > -/* sometimes alloc/free could use kmalloc with GFP_DMA, for
> > > > - * better sharing and to leverage mm/slab.c intelligence.
> > > > - */
> > > > -
> > > >  void *hcd_buffer_alloc(
> > > >  	struct usb_bus		*bus,
> > > >  	size_t			size,
> > > > @@ -128,6 +124,12 @@ void *hcd_buffer_alloc(
> > > >  	if (hcd->localmem_pool)
> > > >  		return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
> > > >  
> > > > +	/*
> > > > +	 * Zone modifiers are ignored by DMA API, and PIO should always use
> > > > +	 * GFP_KERNEL.
> > > > +	 */
> > > > +	WARN_ON_ONCE(mem_flags & GFP_ZONEMASK);    
> > > 
> > > You just rebooted the box if this happens, do you REALLY want to do
> > > that?  People generally do not like their data lost :(  
> > 
> > FWIW my box does not reboot on a warning. But I admit there are people
> > who want to run their systems with panic_on_warn (although I suspect
> > they already experience some sudden reboots, so they had better be
> > prepared).  
> 
> There are billions of Linux systems out there with panic-on-warn enabled :(
> 
> > > Why not just fix the callers, OR if this really isn't going to be
> > > allowed, return an error and just fail the whole submission?  And stick
> > > around to fix up all of the drivers that end up triggering this...  
> > 
> > That's the point. AFAICS there are _no_ in-tree callers that would pass
> > GFP_DMA or GFP_DMA32 to hcd_buffer_alloc(), directly or indirectly. But
> > nobody should be tempted to add the flag, because I cannot imagine how
> > that would ever be the right thing to do.
> > 
> > I can change it back to mem_flags &= ~GFP_ZONEMASK to fix it silently;
> > I simply thought that driver authors may appreciate a warning that
> > they're trying to do something silly.  
> 
> A warning is fine, but don't reboot a box please.  dev_warn() with a
> ratelimit and then return an error perhaps?

If we're concerned about breaking existing systems in the wild, then we
should merely issue a warning that the flag is ignored. So, probably a
ratelimited dev_warn() and continue operation.

> > Whatever works for you, but please keep in mind that there seems to be
> > agreement among mm people that DMA and DMA32 zones should be removed
> > from the kernel eventually.  
> 
> I agree, they should be removed as they don't do what people think they
> do.  So why not just remove them entirely, otherwise are you going to go
> and add this type of checking to all bus subsystems?

I'm kind of testing grounds here. But yes, I'm browsing all in-tree
occurrences of GFP_DMA and GFP_DMA32, looking for corner cases that
may break. So far, I have found exactly one user of the DMA zone who
appears to be quite right: s390 I/O channels to cope with the legacy
31-bit addressing of the CCW instruction. JFYI.

Petr T

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Oliver Neukum]

On 14.04.25 09:02, Petr Tesarik wrote:

Hi,
  
> That's the point. AFAICS there are _no_ in-tree callers that would pass
> GFP_DMA or GFP_DMA32 to hcd_buffer_alloc(), directly or indirectly. But
> nobody should be tempted to add the flag, because I cannot imagine how
> that would ever be the right thing to do.

You do not dream about putting USB onto PCMCIA over Thunderbolt?

> I can change it back to mem_flags &= ~GFP_ZONEMASK to fix it silently;
> I simply thought that driver authors may appreciate a warning that
> they're trying to do something silly.

People rarely appreciate warnings. I think we should limit them
to cases where something goes wrong or something unexpected happens.

> Whatever works for you, but please keep in mind that there seems to be
> agreement among mm people that DMA and DMA32 zones should be removed
> from the kernel eventually.

Well, if somebody finds a legitimate use case for these flags, the mm
people should deal with it. They are likelier to find a good solution than
all driver writers being forced into finding individual solutions.

	Regards
		Oliver

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Petr Tesarik]

On Tue, 15 Apr 2025 09:53:24 +0200
Oliver Neukum <oneukum@suse.com> wrote:

> On 14.04.25 09:02, Petr Tesarik wrote:
> 
> Hi,
>   
> > That's the point. AFAICS there are _no_ in-tree callers that would pass
> > GFP_DMA or GFP_DMA32 to hcd_buffer_alloc(), directly or indirectly. But
> > nobody should be tempted to add the flag, because I cannot imagine how
> > that would ever be the right thing to do.  
> 
> You do not dream about putting USB onto PCMCIA over Thunderbolt?

Oh, I do, and that's precisely why these GFP flags are no good. The
address (and other) constraints imposed by different buses may not
(and often do not) match any existing memory zone.

However, zone address ranges are determined statically at compile time,
or latest at boot time (e.g. arm64). It's too late to adjust the limits
when you hotplug a more constrained bus at run-time. And I haven't even
mentioned bus bridges which add a non-zero offset to the address...

> > I can change it back to mem_flags &= ~GFP_ZONEMASK to fix it silently;
> > I simply thought that driver authors may appreciate a warning that
> > they're trying to do something silly.  
> 
> People rarely appreciate warnings. I think we should limit them
> to cases where something goes wrong or something unexpected happens.

I'm certainly no expert on what is expected to happen if you include
GFP_DMA in your HCD buffer allocation flags, but the current code will
*ignore* it, unless the HCD uses PIO.

I thought this was rather unexpected.

> > Whatever works for you, but please keep in mind that there seems to be
> > agreement among mm people that DMA and DMA32 zones should be removed
> > from the kernel eventually.  
> 
> Well, if somebody finds a legitimate use case for these flags, the mm
> people should deal with it. They are likelier to find a good solution than
> all driver writers being forced into finding individual solutions.

My goal is to provide an allocator that is a better match for the
constraints defined by dma_mask, coherent_dma_mask and bus_dma_limit.
For now, I'm trying to clean up some users of GFP_DMA and GFP_DMA32
flags which look obviously incorrect.

Petr T

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Oliver Neukum]

On 16.04.25 09:48, Petr Tesarik wrote:

> Oh, I do, and that's precisely why these GFP flags are no good. The
> address (and other) constraints imposed by different buses may not
> (and often do not) match any existing memory zone.

True. So we currently have a non-portable series of flags.
It would we better if we passed a hypothetical 'struct mem_constraint*'.
But we don't for now.

> However, zone address ranges are determined statically at compile time,
> or latest at boot time (e.g. arm64). It's too late to adjust the limits
> when you hotplug a more constrained bus at run-time. And I haven't even
> mentioned bus bridges which add a non-zero offset to the address...

Yes. Hence the only time somebody would pass a flag like that would be
on very arch specific code. That means that such a developer would be on
his or her own. Hence I'd say the simplest solution is just to do nothing.

	Regards
		Oliver

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Petr Tesarik]

On Wed, 16 Apr 2025 10:45:19 +0200
Oliver Neukum <oneukum@suse.com> wrote:

> On 16.04.25 09:48, Petr Tesarik wrote:
> 
> > Oh, I do, and that's precisely why these GFP flags are no good. The
> > address (and other) constraints imposed by different buses may not
> > (and often do not) match any existing memory zone.  
> 
> True. So we currently have a non-portable series of flags.
> It would we better if we passed a hypothetical 'struct mem_constraint*'.
> But we don't for now.

We don't have this struct as such, but all required values are stored
directly or indirectly (dma_range_map) in struct device, which is
already passed around.

There's merely no mm API that could allocate based on these raw values,
so there are some ugly kludges to cope with most scenarios.

> > However, zone address ranges are determined statically at compile time,
> > or latest at boot time (e.g. arm64). It's too late to adjust the limits
> > when you hotplug a more constrained bus at run-time. And I haven't even
> > mentioned bus bridges which add a non-zero offset to the address...  
> 
> Yes. Hence the only time somebody would pass a flag like that would be
> on very arch specific code. That means that such a developer would be on
> his or her own. Hence I'd say the simplest solution is just to do nothing.

I have found no such thing in tree (with the exception of s390-specific
drivers, mentioned elsewhere in this thread). But whatever is possible
with a GFP flag is also possible by setting a bus limit.

If I stay with the USB buffer allocations, AFAICS the mem_flags
parameter should be used only for non-zone flags. If you specify,
GFP_DMA here, it will have no impact whatsoever on allocating DMA
buffers. It may unnecessarily allocate from the DMA zone for doing PIO.

Now I think I should really write an article for LWN to debunk some
myths about GFP_DMA.

HTH
Petr T

Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Oliver Neukum]

On 16.04.25 12:47, Petr Tesarik wrote:
  
> If I stay with the USB buffer allocations, AFAICS the mem_flags
> parameter should be used only for non-zone flags. If you specify,
> GFP_DMA here, it will have no impact whatsoever on allocating DMA
> buffers. It may unnecessarily allocate from the DMA zone for doing PIO.

Yes. But we should not limit enforcement of such a _new_ policy
to one method in order to fix a hypothetical issue.
There is just no need for action.

> Now I think I should really write an article for LWN to debunk some
> myths about GFP_DMA.

Well, if you go to that trouble an explanation of why memflags
are passed in USB at all and how DMA works in general would be
productive.

	Regards
		Oliver