[PATCH] x86: adjust GFP mask handling for coherent allocations

[PATCH] x86: adjust GFP mask handling for coherent allocations

[Jan Beulich]

Rather than forcing GFP flags and DMA mask to be inconsistent, GFP
flags should be determined even for the fallback device through
dma_alloc_coherent_mask()/dma_alloc_coherent_gfp_flags(). This
restores 64-bit behavior as it was prior to commits
8965eb19386fdf5ccd0ef8b02593eb8560aa3416 and
4a367f3a9dbf2e7ffcee4702203479809236ee6e (not sure why there are two of
them), where GFP_DMA was forced on for 32-bit, but not for 64-bit, with
the slight adjustment that afaict even 32-bit doesn't need this without
CONFIG_ISA.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>

---
 arch/x86/include/asm/dma-mapping.h |    4 +---
 arch/x86/kernel/pci-dma.c          |    8 +++++---
 2 files changed, 6 insertions(+), 6 deletions(-)

--- linux-2.6.32-rc5/arch/x86/include/asm/dma-mapping.h	2009-10-19 13:09:44.000000000 +0200
+++ 2.6.32-rc5-x86-dma-mask-gfp/arch/x86/include/asm/dma-mapping.h	2009-10-22 10:54:08.000000000 +0200
@@ -124,10 +124,8 @@ dma_alloc_coherent(struct device *dev, s
 	if (dma_alloc_from_coherent(dev, size, dma_handle, &memory))
 		return memory;
 
-	if (!dev) {
+	if (!dev)
 		dev = &x86_dma_fallback_dev;
-		gfp |= GFP_DMA;
-	}
 
 	if (!is_device_dma_capable(dev))
 		return NULL;
--- linux-2.6.32-rc5/arch/x86/kernel/pci-dma.c	2009-10-19 13:09:45.000000000 +0200
+++ 2.6.32-rc5-x86-dma-mask-gfp/arch/x86/kernel/pci-dma.c	2009-10-22 10:56:46.000000000 +0200
@@ -45,12 +45,14 @@ int iommu_pass_through __read_mostly;
 dma_addr_t bad_dma_address __read_mostly = 0;
 EXPORT_SYMBOL(bad_dma_address);
 
-/* Dummy device used for NULL arguments (normally ISA). Better would
-   be probably a smaller DMA mask, but this is bug-to-bug compatible
-   to older i386. */
+/* Dummy device used for NULL arguments (normally ISA). */
 struct device x86_dma_fallback_dev = {
 	.init_name = "fallback device",
+#ifdef CONFIG_ISA
+	.coherent_dma_mask = DMA_BIT_MASK(24),
+#else
 	.coherent_dma_mask = DMA_BIT_MASK(32),
+#endif
 	.dma_mask = &x86_dma_fallback_dev.coherent_dma_mask,
 };
 EXPORT_SYMBOL(x86_dma_fallback_dev);

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[Ingo Molnar]

* Jan Beulich <JBeulich@novell.com> wrote:

> Rather than forcing GFP flags and DMA mask to be inconsistent, GFP
> flags should be determined even for the fallback device through
> dma_alloc_coherent_mask()/dma_alloc_coherent_gfp_flags(). This
> restores 64-bit behavior as it was prior to commits
> 8965eb19386fdf5ccd0ef8b02593eb8560aa3416 and
> 4a367f3a9dbf2e7ffcee4702203479809236ee6e (not sure why there are two of
> them), where GFP_DMA was forced on for 32-bit, but not for 64-bit, with
> the slight adjustment that afaict even 32-bit doesn't need this without
> CONFIG_ISA.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> ---
>  arch/x86/include/asm/dma-mapping.h |    4 +---
>  arch/x86/kernel/pci-dma.c          |    8 +++++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> --- linux-2.6.32-rc5/arch/x86/include/asm/dma-mapping.h	2009-10-19 13:09:44.000000000 +0200
> +++ 2.6.32-rc5-x86-dma-mask-gfp/arch/x86/include/asm/dma-mapping.h	2009-10-22 10:54:08.000000000 +0200
> @@ -124,10 +124,8 @@ dma_alloc_coherent(struct device *dev, s
>  	if (dma_alloc_from_coherent(dev, size, dma_handle, &memory))
>  		return memory;
>  
> -	if (!dev) {
> +	if (!dev)
>  		dev = &x86_dma_fallback_dev;
> -		gfp |= GFP_DMA;
> -	}
>  
>  	if (!is_device_dma_capable(dev))
>  		return NULL;
> --- linux-2.6.32-rc5/arch/x86/kernel/pci-dma.c	2009-10-19 13:09:45.000000000 +0200
> +++ 2.6.32-rc5-x86-dma-mask-gfp/arch/x86/kernel/pci-dma.c	2009-10-22 10:56:46.000000000 +0200
> @@ -45,12 +45,14 @@ int iommu_pass_through __read_mostly;
>  dma_addr_t bad_dma_address __read_mostly = 0;
>  EXPORT_SYMBOL(bad_dma_address);
>  
> -/* Dummy device used for NULL arguments (normally ISA). Better would
> -   be probably a smaller DMA mask, but this is bug-to-bug compatible
> -   to older i386. */
> +/* Dummy device used for NULL arguments (normally ISA). */
>  struct device x86_dma_fallback_dev = {
>  	.init_name = "fallback device",
> +#ifdef CONFIG_ISA
> +	.coherent_dma_mask = DMA_BIT_MASK(24),
> +#else
>  	.coherent_dma_mask = DMA_BIT_MASK(32),
> +#endif
>  	.dma_mask = &x86_dma_fallback_dev.coherent_dma_mask,
>  };
>  EXPORT_SYMBOL(x86_dma_fallback_dev);

makes sense (although there were a few odd ends in various ISA sound 
driver details so this should go on the .33 not .32), but please 
introduce a cleaner construct, like a new DMA_ISA_BIT_MASK() or so.

Thanks,

	Ingo

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[Jan Beulich]

>>> Ingo Molnar <mingo@elte.hu> 23.10.09 13:48 >>>
>makes sense (although there were a few odd ends in various ISA sound 
>driver details so this should go on the .33 not .32), but please 
>introduce a cleaner construct, like a new DMA_ISA_BIT_MASK() or so.

Hmm, I could see DMA_ISA_BIT_MASK only replace DMA_BIT_MASK(24)
(but specifically not being conditional upon CONFIG_ISA) without becoming
confusing. Consequently this would eliminate the conditional in the .c
file. And any attempt to eliminate the conditional another way would just
introduce a very similar conditional elsewhere; with this having a single
user (and foreseeably not ever a second one) I would think this would
just make the code less readable.

Or did you have something else in mind that I just don't realize?

Jan

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[Ingo Molnar]

* Jan Beulich <JBeulich@novell.com> wrote:

> >>> Ingo Molnar <mingo@elte.hu> 23.10.09 13:48 >>>
> >makes sense (although there were a few odd ends in various ISA sound 
> >driver details so this should go on the .33 not .32), but please 
> >introduce a cleaner construct, like a new DMA_ISA_BIT_MASK() or so.
> 
> Hmm, I could see DMA_ISA_BIT_MASK only replace DMA_BIT_MASK(24) (but 
> specifically not being conditional upon CONFIG_ISA) without becoming 
> confusing. Consequently this would eliminate the conditional in the .c 
> file. [...]
>
> Or did you have something else in mind that I just don't realize?

DMA_ISA_BIT_MASK was what i had in mind.

> And any attempt to eliminate the conditional another way would just 
> introduce a very similar conditional elsewhere; with this having a 
> single user (and foreseeably not ever a second one) I would think this 
> would just make the code less readable.

There's 3 other current uses of DMA_BIT_MASK(24) in arch/x86 - couldnt 
those use ISA_DMA_BIT_MASK too?

	Ingo

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[Jan Beulich]

>>> Ingo Molnar <mingo@elte.hu> 26.10.09 16:22 >>>
>* Jan Beulich <JBeulich@novell.com> wrote:
>> And any attempt to eliminate the conditional another way would just 
>> introduce a very similar conditional elsewhere; with this having a 
>> single user (and foreseeably not ever a second one) I would think this 
>> would just make the code less readable.
>
>There's 3 other current uses of DMA_BIT_MASK(24) in arch/x86 - couldnt 
>those use ISA_DMA_BIT_MASK too?

Oh, so you didn't mean me to eliminate the conditional in pci-dma.c, but
just to replace the DMA_BIT_MASK(24) here an elsewhere. Sure, I'm fine
with adding this to the patch.

Jan

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[Ingo Molnar]

* Jan Beulich <JBeulich@novell.com> wrote:

> >>> Ingo Molnar <mingo@elte.hu> 26.10.09 16:22 >>>
> >* Jan Beulich <JBeulich@novell.com> wrote:
> >> And any attempt to eliminate the conditional another way would just 
> >> introduce a very similar conditional elsewhere; with this having a 
> >> single user (and foreseeably not ever a second one) I would think this 
> >> would just make the code less readable.
> >
> >There's 3 other current uses of DMA_BIT_MASK(24) in arch/x86 - couldnt 
> >those use ISA_DMA_BIT_MASK too?
> 
> Oh, so you didn't mean me to eliminate the conditional in pci-dma.c, 
> but just to replace the DMA_BIT_MASK(24) here an elsewhere. Sure, I'm 
> fine with adding this to the patch.

Well, can ISA_BIT_MASK fall back to DMA_BIT_MASK(32) on !CONFIG_ISA? If 
we have ISA support disabled we might as well pretend the whole world is 
PCI, right?

That way we'd get rid of that #ifdef in the .c code too.

	Ingo

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[Jesse Barnes]

On Mon, 26 Oct 2009 21:19:17 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Jan Beulich <JBeulich@novell.com> wrote:
> 
> > >>> Ingo Molnar <mingo@elte.hu> 26.10.09 16:22 >>>
> > >* Jan Beulich <JBeulich@novell.com> wrote:
> > >> And any attempt to eliminate the conditional another way would
> > >> just introduce a very similar conditional elsewhere; with this
> > >> having a single user (and foreseeably not ever a second one) I
> > >> would think this would just make the code less readable.
> > >
> > >There's 3 other current uses of DMA_BIT_MASK(24) in arch/x86 -
> > >couldnt those use ISA_DMA_BIT_MASK too?
> > 
> > Oh, so you didn't mean me to eliminate the conditional in
> > pci-dma.c, but just to replace the DMA_BIT_MASK(24) here an
> > elsewhere. Sure, I'm fine with adding this to the patch.
> 
> Well, can ISA_BIT_MASK fall back to DMA_BIT_MASK(32) on !CONFIG_ISA?
> If we have ISA support disabled we might as well pretend the whole
> world is PCI, right?
> 
> That way we'd get rid of that #ifdef in the .c code too.

Sounds good to me, feel free to add my acked-by and push it via the
-tip tree.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[FUJITA Tomonori]

On Mon, 26 Oct 2009 21:19:17 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Jan Beulich <JBeulich@novell.com> wrote:
> 
> > >>> Ingo Molnar <mingo@elte.hu> 26.10.09 16:22 >>>
> > >* Jan Beulich <JBeulich@novell.com> wrote:
> > >> And any attempt to eliminate the conditional another way would just 
> > >> introduce a very similar conditional elsewhere; with this having a 
> > >> single user (and foreseeably not ever a second one) I would think this 
> > >> would just make the code less readable.
> > >
> > >There's 3 other current uses of DMA_BIT_MASK(24) in arch/x86 - couldnt 
> > >those use ISA_DMA_BIT_MASK too?
> > 
> > Oh, so you didn't mean me to eliminate the conditional in pci-dma.c, 
> > but just to replace the DMA_BIT_MASK(24) here an elsewhere. Sure, I'm 
> > fine with adding this to the patch.
> 
> Well, can ISA_BIT_MASK fall back to DMA_BIT_MASK(32) on !CONFIG_ISA? If 
> we have ISA support disabled we might as well pretend the whole world is 
> PCI, right?

I don't think that it works. At least, you can't do that with
the DMA_BIT_MASK(24) in arch/x86/kernl/pci-dma.c; it must be
DMA_BIT_MASK(24) even with !CONFIG_ISA.


> That way we'd get rid of that #ifdef in the .c code too.

Well, in the first place, we don't need the #ifdef in Jan's patch. We
can always use DMA_BIT_MASK(24) for the fallback device.

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[Jan Beulich]

>>> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> 27.10.09 02:38 >>>
>On Mon, 26 Oct 2009 21:19:17 +0100
>Ingo Molnar <mingo@elte.hu> wrote:
>> Well, can ISA_BIT_MASK fall back to DMA_BIT_MASK(32) on !CONFIG_ISA? If 
>> we have ISA support disabled we might as well pretend the whole world is 
>> PCI, right?
>
>I don't think that it works. At least, you can't do that with
>the DMA_BIT_MASK(24) in arch/x86/kernl/pci-dma.c; it must be
>DMA_BIT_MASK(24) even with !CONFIG_ISA.

This one I too was about to point out, which is why I think
DMA_ISA_BIT_MASK should only be an alias of DMA_BIT_MASK(24), with
no fallback to DMA_BIT_MASK(32).

>> That way we'd get rid of that #ifdef in the .c code too.
>
>Well, in the first place, we don't need the #ifdef in Jan's patch. We
>can always use DMA_BIT_MASK(24) for the fallback device.

But this one I don't agree with - the purpose of the patch is to not have
a 24-bit (or 32-bit) mask here unconditionally: It would result in GFP_DMA
to be forced on for the allocation (which the patch specifically eliminates),
and for x86-64 this wasn't the case up to .25, which is the behavior I'm
trying to restore (and extend to the !CONFIG_ISA case for ix86).

Jan

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[Jan Beulich]

>>> Ingo Molnar <mingo@elte.hu> 26.10.09 21:19 >>>
>
>* Jan Beulich <JBeulich@novell.com> wrote:
>
>> >>> Ingo Molnar <mingo@elte.hu> 26.10.09 16:22 >>>
>> >* Jan Beulich <JBeulich@novell.com> wrote:
>> >> And any attempt to eliminate the conditional another way would just 
>> >> introduce a very similar conditional elsewhere; with this having a 
>> >> single user (and foreseeably not ever a second one) I would think this 
>> >> would just make the code less readable.
>> >
>> >There's 3 other current uses of DMA_BIT_MASK(24) in arch/x86 - couldnt 
>> >those use ISA_DMA_BIT_MASK too?
>> 
>> Oh, so you didn't mean me to eliminate the conditional in pci-dma.c, 
>> but just to replace the DMA_BIT_MASK(24) here an elsewhere. Sure, I'm 
>> fine with adding this to the patch.
>
>Well, can ISA_BIT_MASK fall back to DMA_BIT_MASK(32) on !CONFIG_ISA? If 
>we have ISA support disabled we might as well pretend the whole world is 
>PCI, right?
>
>That way we'd get rid of that #ifdef in the .c code too.

I can certainly code it that way, but then we can't use it to replace any
instance of DMA_BIT_MASK(24) used to derive the need for GFP_DMA.
So I'm not sure elimination of which of the instances of DMA_BIT_MASK(24)
is more desirable...

Jan

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[FUJITA Tomonori]

On Tue, 27 Oct 2009 08:58:14 +0000
"Jan Beulich" <JBeulich@novell.com> wrote:

> >Well, in the first place, we don't need the #ifdef in Jan's patch. We
> >can always use DMA_BIT_MASK(24) for the fallback device.
> 
> But this one I don't agree with - the purpose of the patch is to not have
> a 24-bit (or 32-bit) mask here unconditionally: It would result in GFP_DMA
> to be forced on for the allocation (which the patch specifically eliminates),

If a driver doesn't want to GFP_DMA, it should set up the
dma_coherent_mask of the device and pass it. In fact, it should do. A
driver that uses the fallback device is broken.

Why can't you fix drivers that use the fallback instead of adding
another hack to the common place?

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[Jan Beulich]

>>> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> 27.10.09 10:11 >>>
>If a driver doesn't want to GFP_DMA, it should set up the
>dma_coherent_mask of the device and pass it. In fact, it should do. A
>driver that uses the fallback device is broken.

Agreed.

>Why can't you fix drivers that use the fallback instead of adding
>another hack to the common place?

Because there's quite a few of them - just search for callers of
dma_alloc_coherent() passing NULL as the first argument. Of course
I'd rather see the fallback gone, but I think this requires cooperation
from/action taken by the driver maintainers first. A first step might be
to add a WRN_ON() for that case, but I don't see it being reasonable
to eliminate the fallback right away. Hence, with it having got broken
in .26 I think it is reasonable to fix it for the time being.

Jan

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[FUJITA Tomonori]

On Tue, 27 Oct 2009 09:23:38 +0000
"Jan Beulich" <JBeulich@novell.com> wrote:

> Because there's quite a few of them - just search for callers of
> dma_alloc_coherent() passing NULL as the first argument. Of course
> I'd rather see the fallback gone, but I think this requires cooperation
> from/action taken by the driver maintainers first. A first step might be
> to add a WRN_ON() for that case, but I don't see it being reasonable
> to eliminate the fallback right away. Hence, with it having got broken
> in .26 I think it is reasonable to fix it for the time being.

Well, the most of them are drivers that nobody cares about (uses).

What drivers did you hit this problem with?

We've lived with the current code for some years so this problem isn't
urgent at all. Please don't add another workaround for broken code.
Let's put WARN_ON for callers that use NULL and see who complains.

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[Jan Beulich]

>>> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> 27.10.09 10:37 >>>
>On Tue, 27 Oct 2009 09:23:38 +0000
>"Jan Beulich" <JBeulich@novell.com> wrote:
>
>> Because there's quite a few of them - just search for callers of
>> dma_alloc_coherent() passing NULL as the first argument. Of course
>> I'd rather see the fallback gone, but I think this requires cooperation
>> from/action taken by the driver maintainers first. A first step might be
>> to add a WRN_ON() for that case, but I don't see it being reasonable
>> to eliminate the fallback right away. Hence, with it having got broken
>> in .26 I think it is reasonable to fix it for the time being.
>
>Well, the most of them are drivers that nobody cares about (uses).
>
>What drivers did you hit this problem with?

drivers/firmware/dell_rbu.c (on .27.x, and in particular with the derivation
of __GFP_NORETRY from __GFP_DMA, which fortunately doesn't exist in
newer kernels anymore).

>We've lived with the current code for some years so this problem isn't
>urgent at all. Please don't add another workaround for broken code.
>Let's put WARN_ON for callers that use NULL and see who complains.

Jan

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[FUJITA Tomonori]

On Tue, 27 Oct 2009 09:47:32 +0000
"Jan Beulich" <JBeulich@novell.com> wrote:

> >>> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> 27.10.09 10:37 >>>
> >On Tue, 27 Oct 2009 09:23:38 +0000
> >"Jan Beulich" <JBeulich@novell.com> wrote:
> >
> >> Because there's quite a few of them - just search for callers of
> >> dma_alloc_coherent() passing NULL as the first argument. Of course
> >> I'd rather see the fallback gone, but I think this requires cooperation
> >> from/action taken by the driver maintainers first. A first step might be
> >> to add a WRN_ON() for that case, but I don't see it being reasonable
> >> to eliminate the fallback right away. Hence, with it having got broken
> >> in .26 I think it is reasonable to fix it for the time being.
> >
> >Well, the most of them are drivers that nobody cares about (uses).
> >
> >What drivers did you hit this problem with?
> 
> drivers/firmware/dell_rbu.c (on .27.x, and in particular with the derivation
> of __GFP_NORETRY from __GFP_DMA, which fortunately doesn't exist in
> newer kernels anymore).

I don't know anything about this driver (and even I'm not sure why
this driver uses dma_alloc_coherent) but why can't this driver use
rbu_device with dma API?


diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index b4704e1..481b009 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -372,8 +372,8 @@ static void img_update_free(void)
 	memset(rbu_data.image_update_buffer, 0,
 		rbu_data.image_update_buffer_size);
 	if (rbu_data.dma_alloc == 1)
-		dma_free_coherent(NULL, rbu_data.bios_image_size,
-			rbu_data.image_update_buffer, dell_rbu_dmaaddr);
+		dma_free_coherent(&rbu_device->dev, rbu_data.bios_image_size,
+				  rbu_data.image_update_buffer, dell_rbu_dmaaddr);
 	else
 		free_pages((unsigned long) rbu_data.image_update_buffer,
 			rbu_data.image_update_ordernum);
@@ -443,7 +443,7 @@ static int img_update_realloc(unsigned long size)
 	if (img_buf_phys_addr > BIOS_SCAN_LIMIT) {
 		free_pages((unsigned long) image_update_buffer, ordernum);
 		ordernum = -1;
-		image_update_buffer = dma_alloc_coherent(NULL, size,
+		image_update_buffer = dma_alloc_coherent(&rbu_device->dev, size,
 			&dell_rbu_dmaaddr, GFP_KERNEL);
 		dma_alloc = 1;
 	}
@@ -699,6 +699,8 @@ static int __init dcdrbu_init(void)
 			"failed\n", __func__);
 		return PTR_ERR(rbu_device);
 	}
+	rbu_device->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	rbu_device->dev.dma_mask = &rbu_device->dev.coherent_dma_mask;
 
 	rc = sysfs_create_bin_file(&rbu_device->dev.kobj, &rbu_data_attr);
 	if (rc)

Re: [PATCH] x86: adjust GFP mask handling for coherent allocations

[Alan Cox]

> to eliminate the fallback right away. Hence, with it having got broken
> in .26 I think it is reasonable to fix it for the time being.

And .26 was released by distributions when ?

Perhaps that says something about how many WARN_ON()s are likely to show
up ;)

Alan