At Sun, 19 Oct 2008 12:09:32 +0200,
Sven Schnelle wrote:
>
> Hi List,
>
> my kernel dies while probing parport with the following last words:
>
> [ 3.672199] parport_pc 00:0b: reported by Plug and Play ACPI
> [ 3.677969] parport0: PC-style at 0x378 (0x778), irq 7, dma 3 [PCSPP,TRISTATE,COMPAT,EPP,ECP,DMA]
> [ 3.687691] hwdev DMA mask = 0x0000000000ffffff, dev_addr = 0x0000000020000000
> [ 3.694916] Kernel panic - not syncing: swiotlb_alloc_coherent: allocated memory is out of range for device
>
> I haven't started a bisection yet, but this seems to be introduced
> somewhere between 2.6.26 and 2.6.27, at least 2.6.26 was working without
> problems. The dmesg log + config was obtained from a kernel compiled
> from git on 10/16/2008.
This bug hits me, too. Looks like swiotlb assumes that the alloc caller
must set GFP_DMA appropriately by itself since GFP_DMA hack was
removed. The patch below should fix this particular case.
HOWEVER: the fundamental problem appears to be in swiotlb itself.
It assumes that iotlb pages are in DMA area. But, in this case, the
driver sets 24bit DMA (as of PnP) while iotlb pages are allocated
under 32bit DMA via alloc_bootmem_low_pages(). This doesn't work, of
course.
So, even adding GFP_DMA works mostly, it has still potentially
breakage when you can't get the page and fall back to iotlb pages,
just like the panic above.
Also, the removal of GFP_DMA hack is a bad idea. For example, if a
device requires 28bit DMA mask, it doesn't set always GFP_DMA for
allocation because pages in ZONE_NORMAL may be inside that DMA mask.
Normal allocators allow this behavior but swiotlb allocator doesn't.
(Correct me if I'm wrong here -- I haven't followed much the recent
changes.)
Last but not least, I think panic() in allocation error path is too
strict. Usually returning NULL isn't always fatal error, so give some
more chance to debug, e.g. by calling WARN() (or whatever) instead of
panic().
thanks,
Takashi
diff --git a/drivers/parport/parport_pc.c b/drivers/parport/parport_pc.c
index 8a846ad..4fbec16 100644
--- a/drivers/parport/parport_pc.c
+++ b/drivers/parport/parport_pc.c
@@ -2347,7 +2347,7 @@ struct parport *parport_pc_probe_port (unsigned long int base,
dma_alloc_coherent(dev,
PAGE_SIZE,
&priv->dma_handle,
- GFP_KERNEL);
+ GFP_KERNEL | GFP_DMA);
if (! priv->dma_buf) {
printk (KERN_WARNING "%s: "
"cannot get buffer for DMA, "
At Wed, 22 Oct 2008 12:53:58 +0200,
I wrote:
>
> At Sun, 19 Oct 2008 12:09:32 +0200,
> Sven Schnelle wrote:
> >
> > Hi List,
> >
> > my kernel dies while probing parport with the following last words:
> >
> > [ 3.672199] parport_pc 00:0b: reported by Plug and Play ACPI
> > [ 3.677969] parport0: PC-style at 0x378 (0x778), irq 7, dma 3 [PCSPP,TRISTATE,COMPAT,EPP,ECP,DMA]
> > [ 3.687691] hwdev DMA mask = 0x0000000000ffffff, dev_addr = 0x0000000020000000
> > [ 3.694916] Kernel panic - not syncing: swiotlb_alloc_coherent: allocated memory is out of range for device
> >
> > I haven't started a bisection yet, but this seems to be introduced
> > somewhere between 2.6.26 and 2.6.27, at least 2.6.26 was working without
> > problems. The dmesg log + config was obtained from a kernel compiled
> > from git on 10/16/2008.
>
> This bug hits me, too. Looks like swiotlb assumes that the alloc caller
> must set GFP_DMA appropriately by itself since GFP_DMA hack was
> removed. The patch below should fix this particular case.
>
> HOWEVER: the fundamental problem appears to be in swiotlb itself.
> It assumes that iotlb pages are in DMA area. But, in this case, the
> driver sets 24bit DMA (as of PnP) while iotlb pages are allocated
> under 32bit DMA via alloc_bootmem_low_pages(). This doesn't work, of
> course.
>
> So, even adding GFP_DMA works mostly, it has still potentially
> breakage when you can't get the page and fall back to iotlb pages,
> just like the panic above.
>
> Also, the removal of GFP_DMA hack is a bad idea. For example, if a
> device requires 28bit DMA mask, it doesn't set always GFP_DMA for
> allocation because pages in ZONE_NORMAL may be inside that DMA mask.
> Normal allocators allow this behavior but swiotlb allocator doesn't.
> (Correct me if I'm wrong here -- I haven't followed much the recent
> changes.)
The patch below is to a workaround for that.
Takashi
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index f8eebd4..cb20149 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -468,6 +468,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
void *ret;
int order = get_order(size);
+ again:
ret = (void *)__get_free_pages(flags, order);
if (ret && address_needs_mapping(hwdev, virt_to_bus(ret), size)) {
/*
@@ -478,6 +479,10 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
ret = NULL;
}
if (!ret) {
+ if (!(flags & __GFP_DMA)) {
+ flags |= __GFP_DMA;
+ goto again;
+ }
/*
* We are either out of memory or the device can't DMA
* to GFP_DMA memory; fall back on
On Wed, 22 Oct 2008 12:53:58 +0200
Takashi Iwai <[email protected]> wrote:
> At Sun, 19 Oct 2008 12:09:32 +0200,
> Sven Schnelle wrote:
> >
> > Hi List,
> >
> > my kernel dies while probing parport with the following last words:
> >
> > [ 3.672199] parport_pc 00:0b: reported by Plug and Play ACPI
> > [ 3.677969] parport0: PC-style at 0x378 (0x778), irq 7, dma 3 [PCSPP,TRISTATE,COMPAT,EPP,ECP,DMA]
> > [ 3.687691] hwdev DMA mask = 0x0000000000ffffff, dev_addr = 0x0000000020000000
> > [ 3.694916] Kernel panic - not syncing: swiotlb_alloc_coherent: allocated memory is out of range for device
> >
> > I haven't started a bisection yet, but this seems to be introduced
> > somewhere between 2.6.26 and 2.6.27, at least 2.6.26 was working without
> > problems. The dmesg log + config was obtained from a kernel compiled
> > from git on 10/16/2008.
>
> This bug hits me, too. Looks like swiotlb assumes that the alloc caller
> must set GFP_DMA appropriately by itself since GFP_DMA hack was
> removed. The patch below should fix this particular case.
This happens with 2.6.27, right? GFP_DMA hack was removed post
2.6.27. What kernel version do you hit this problem?
Post 2.6.27, x86's alloc_coherent works a bit differently, but neither
require the caller set to GFP flag. arch/x86/kernel/pci-dma.c does
with 2.6.27 and asm-x86/dma-mapping.h does with post 2.6.27.
> HOWEVER: the fundamental problem appears to be in swiotlb itself.
> It assumes that iotlb pages are in DMA area. But, in this case, the
> driver sets 24bit DMA (as of PnP) while iotlb pages are allocated
> under 32bit DMA via alloc_bootmem_low_pages(). This doesn't work, of
> course.
If a device has 24bit dma mask, alloc_coherent is supposed to use
GFP_DMA.
> So, even adding GFP_DMA works mostly, it has still potentially
> breakage when you can't get the page and fall back to iotlb pages,
> just like the panic above.
>
> Also, the removal of GFP_DMA hack is a bad idea. For example, if a
> device requires 28bit DMA mask, it doesn't set always GFP_DMA for
> allocation because pages in ZONE_NORMAL may be inside that DMA mask.
> Normal allocators allow this behavior but swiotlb allocator doesn't.
> (Correct me if I'm wrong here -- I haven't followed much the recent
> changes.)
28bit DMA mask is supposed to be handled properly. Firstly, we try
with DMA_32BIT_MASK and if an allocated address is not fit for 28bit
mask, we try GFP_DMA again.
> Last but not least, I think panic() in allocation error path is too
> strict. Usually returning NULL isn't always fatal error, so give some
> more chance to debug, e.g. by calling WARN() (or whatever) instead of
> panic().
Yeah, this was discussed several times. The problem is that many
drivers assume that dma mapping operations, map_single, map_sg, and
map_coherent, always succeed and doesn't even check the errors. So we
have some panic() in IOMMU drivers to prevent really bad events like
data corruption.
At Wed, 22 Oct 2008 20:29:24 +0900,
FUJITA Tomonori wrote:
>
> On Wed, 22 Oct 2008 12:53:58 +0200
> Takashi Iwai <[email protected]> wrote:
>
> > At Sun, 19 Oct 2008 12:09:32 +0200,
> > Sven Schnelle wrote:
> > >
> > > Hi List,
> > >
> > > my kernel dies while probing parport with the following last words:
> > >
> > > [ 3.672199] parport_pc 00:0b: reported by Plug and Play ACPI
> > > [ 3.677969] parport0: PC-style at 0x378 (0x778), irq 7, dma 3 [PCSPP,TRISTATE,COMPAT,EPP,ECP,DMA]
> > > [ 3.687691] hwdev DMA mask = 0x0000000000ffffff, dev_addr = 0x0000000020000000
> > > [ 3.694916] Kernel panic - not syncing: swiotlb_alloc_coherent: allocated memory is out of range for device
> > >
> > > I haven't started a bisection yet, but this seems to be introduced
> > > somewhere between 2.6.26 and 2.6.27, at least 2.6.26 was working without
> > > problems. The dmesg log + config was obtained from a kernel compiled
> > > from git on 10/16/2008.
> >
> > This bug hits me, too. Looks like swiotlb assumes that the alloc caller
> > must set GFP_DMA appropriately by itself since GFP_DMA hack was
> > removed. The patch below should fix this particular case.
>
> This happens with 2.6.27, right? GFP_DMA hack was removed post
> 2.6.27. What kernel version do you hit this problem?
2.6.27 works fine, at least on my machine.
Likely a post-2.6.27 regression.
> Post 2.6.27, x86's alloc_coherent works a bit differently, but neither
> require the caller set to GFP flag. arch/x86/kernel/pci-dma.c does
> with 2.6.27 and asm-x86/dma-mapping.h does with post 2.6.27.
>
>
> > HOWEVER: the fundamental problem appears to be in swiotlb itself.
> > It assumes that iotlb pages are in DMA area. But, in this case, the
> > driver sets 24bit DMA (as of PnP) while iotlb pages are allocated
> > under 32bit DMA via alloc_bootmem_low_pages(). This doesn't work, of
> > course.
>
> If a device has 24bit dma mask, alloc_coherent is supposed to use
> GFP_DMA.
Yes. But what happens if __get_free_pages() fails? Then you get the
same problem.
> > So, even adding GFP_DMA works mostly, it has still potentially
> > breakage when you can't get the page and fall back to iotlb pages,
> > just like the panic above.
> >
> > Also, the removal of GFP_DMA hack is a bad idea. For example, if a
> > device requires 28bit DMA mask, it doesn't set always GFP_DMA for
> > allocation because pages in ZONE_NORMAL may be inside that DMA mask.
> > Normal allocators allow this behavior but swiotlb allocator doesn't.
> > (Correct me if I'm wrong here -- I haven't followed much the recent
> > changes.)
>
> 28bit DMA mask is supposed to be handled properly. Firstly, we try
> with DMA_32BIT_MASK and if an allocated address is not fit for 28bit
> mask, we try GFP_DMA again.
Yep, dma_generic_alloc_coherent() works like that for ages.
My point is about swiotlb_alloc_coherent(), and I don't see the
relevant code there...
> > Last but not least, I think panic() in allocation error path is too
> > strict. Usually returning NULL isn't always fatal error, so give some
> > more chance to debug, e.g. by calling WARN() (or whatever) instead of
> > panic().
>
> Yeah, this was discussed several times. The problem is that many
> drivers assume that dma mapping operations, map_single, map_sg, and
> map_coherent, always succeed and doesn't even check the errors. So we
> have some panic() in IOMMU drivers to prevent really bad events like
> data corruption.
Well, but also for alloc_coherent()? Returning NULL from
dma_alloc_coherent() is really no fatal error. If the caller doesn't
check the return value, then it's a more serious bug, I'd say.
thanks,
Takashi
On Wed, 22 Oct 2008 14:06:48 +0200
Takashi Iwai <[email protected]> wrote:
> At Wed, 22 Oct 2008 20:29:24 +0900,
> FUJITA Tomonori wrote:
> >
> > On Wed, 22 Oct 2008 12:53:58 +0200
> > Takashi Iwai <[email protected]> wrote:
> >
> > > At Sun, 19 Oct 2008 12:09:32 +0200,
> > > Sven Schnelle wrote:
> > > >
> > > > Hi List,
> > > >
> > > > my kernel dies while probing parport with the following last words:
> > > >
> > > > [ 3.672199] parport_pc 00:0b: reported by Plug and Play ACPI
> > > > [ 3.677969] parport0: PC-style at 0x378 (0x778), irq 7, dma 3 [PCSPP,TRISTATE,COMPAT,EPP,ECP,DMA]
> > > > [ 3.687691] hwdev DMA mask = 0x0000000000ffffff, dev_addr = 0x0000000020000000
> > > > [ 3.694916] Kernel panic - not syncing: swiotlb_alloc_coherent: allocated memory is out of range for device
> > > >
> > > > I haven't started a bisection yet, but this seems to be introduced
> > > > somewhere between 2.6.26 and 2.6.27, at least 2.6.26 was working without
> > > > problems. The dmesg log + config was obtained from a kernel compiled
> > > > from git on 10/16/2008.
> > >
> > > This bug hits me, too. Looks like swiotlb assumes that the alloc caller
> > > must set GFP_DMA appropriately by itself since GFP_DMA hack was
> > > removed. The patch below should fix this particular case.
> >
> > This happens with 2.6.27, right? GFP_DMA hack was removed post
> > 2.6.27. What kernel version do you hit this problem?
>
> 2.6.27 works fine, at least on my machine.
> Likely a post-2.6.27 regression.
Ok, it makes sense because I don't see any major changes to swiotlb
between 2.6.26 and 2.6.27.
> > Post 2.6.27, x86's alloc_coherent works a bit differently, but neither
> > require the caller set to GFP flag. arch/x86/kernel/pci-dma.c does
> > with 2.6.27 and asm-x86/dma-mapping.h does with post 2.6.27.
> >
> >
> > > HOWEVER: the fundamental problem appears to be in swiotlb itself.
> > > It assumes that iotlb pages are in DMA area. But, in this case, the
> > > driver sets 24bit DMA (as of PnP) while iotlb pages are allocated
> > > under 32bit DMA via alloc_bootmem_low_pages(). This doesn't work, of
> > > course.
> >
> > If a device has 24bit dma mask, alloc_coherent is supposed to use
> > GFP_DMA.
>
> Yes. But what happens if __get_free_pages() fails? Then you get the
> same problem.
Yeah, but __get_free_pages() with GFP_DMA fails, what can we do in
such case? You think that it's a good idea to change swiotlb to
allocates < 16MB iotlb pages? I'm not sure it's worth to do
that. 24bit dma mask devices are disappearing.
About the bug that you hit, I suspect that dma_map_coherent() in
asm-x86/dma-mapping.h doesn't set gfp flags correctly.
dma_map_coherent() calls swiotlb_alloc_coherent with the flags GFP_DMA
set? parport driver set dev->coherent_dma_mask properly?
> > > So, even adding GFP_DMA works mostly, it has still potentially
> > > breakage when you can't get the page and fall back to iotlb pages,
> > > just like the panic above.
> > >
> > > Also, the removal of GFP_DMA hack is a bad idea. For example, if a
> > > device requires 28bit DMA mask, it doesn't set always GFP_DMA for
> > > allocation because pages in ZONE_NORMAL may be inside that DMA mask.
> > > Normal allocators allow this behavior but swiotlb allocator doesn't.
> > > (Correct me if I'm wrong here -- I haven't followed much the recent
> > > changes.)
> >
> > 28bit DMA mask is supposed to be handled properly. Firstly, we try
> > with DMA_32BIT_MASK and if an allocated address is not fit for 28bit
> > mask, we try GFP_DMA again.
>
> Yep, dma_generic_alloc_coherent() works like that for ages.
> My point is about swiotlb_alloc_coherent(), and I don't see the
> relevant code there...
Oops, you are right. swiotlb doesn't try again with GFP_DMA now. Joerg
changed the GFP_DMA retry mechanism work only for pci-nommu.c It broke
GART IOMMU and x86's swiotlb. I modified dma_generic_alloc_coherent to
work with pci-nommu and GART. I promised Ingo to fix swiotlb too but I
forgot about it.
Sorry, I'll fix this soon but your case (28bit mask) is supposed to
work without the GFP_DMA retry mechanism. As I wrote above, I suspect
that dma flag is not set correctly.
> > > Last but not least, I think panic() in allocation error path is too
> > > strict. Usually returning NULL isn't always fatal error, so give some
> > > more chance to debug, e.g. by calling WARN() (or whatever) instead of
> > > panic().
> >
> > Yeah, this was discussed several times. The problem is that many
> > drivers assume that dma mapping operations, map_single, map_sg, and
> > map_coherent, always succeed and doesn't even check the errors. So we
> > have some panic() in IOMMU drivers to prevent really bad events like
> > data corruption.
>
> Well, but also for alloc_coherent()? Returning NULL from
> dma_alloc_coherent() is really no fatal error. If the caller doesn't
> check the return value, then it's a more serious bug, I'd say.
Probably, the majority check dma_alloc_coherent failure. I'll check
this later to remove panic() in alloc_coherent in IOMMUs.
On Wed, 22 Oct 2008 22:13:39 +0900
FUJITA Tomonori <[email protected]> wrote:
> > > > So, even adding GFP_DMA works mostly, it has still potentially
> > > > breakage when you can't get the page and fall back to iotlb pages,
> > > > just like the panic above.
> > > >
> > > > Also, the removal of GFP_DMA hack is a bad idea. For example, if a
> > > > device requires 28bit DMA mask, it doesn't set always GFP_DMA for
> > > > allocation because pages in ZONE_NORMAL may be inside that DMA mask.
> > > > Normal allocators allow this behavior but swiotlb allocator doesn't.
> > > > (Correct me if I'm wrong here -- I haven't followed much the recent
> > > > changes.)
> > >
> > > 28bit DMA mask is supposed to be handled properly. Firstly, we try
> > > with DMA_32BIT_MASK and if an allocated address is not fit for 28bit
> > > mask, we try GFP_DMA again.
> >
> > Yep, dma_generic_alloc_coherent() works like that for ages.
> > My point is about swiotlb_alloc_coherent(), and I don't see the
> > relevant code there...
>
> Oops, you are right. swiotlb doesn't try again with GFP_DMA now. Joerg
> changed the GFP_DMA retry mechanism work only for pci-nommu.c It broke
> GART IOMMU and x86's swiotlb. I modified dma_generic_alloc_coherent to
> work with pci-nommu and GART. I promised Ingo to fix swiotlb too but I
> forgot about it.
>
> Sorry, I'll fix this soon but your case (28bit mask) is supposed to
Oops, I meant, 24bit mask, as you know.
> work without the GFP_DMA retry mechanism. As I wrote above, I suspect
> that dma flag is not set correctly.
At Wed, 22 Oct 2008 22:13:39 +0900,
FUJITA Tomonori wrote:
>
> On Wed, 22 Oct 2008 14:06:48 +0200
> Takashi Iwai <[email protected]> wrote:
>
> > At Wed, 22 Oct 2008 20:29:24 +0900,
> > FUJITA Tomonori wrote:
> > >
> > > On Wed, 22 Oct 2008 12:53:58 +0200
> > > Takashi Iwai <[email protected]> wrote:
> > >
> > > > At Sun, 19 Oct 2008 12:09:32 +0200,
> > > > Sven Schnelle wrote:
> > > > >
> > > > > Hi List,
> > > > >
> > > > > my kernel dies while probing parport with the following last words:
> > > > >
> > > > > [ 3.672199] parport_pc 00:0b: reported by Plug and Play ACPI
> > > > > [ 3.677969] parport0: PC-style at 0x378 (0x778), irq 7, dma 3 [PCSPP,TRISTATE,COMPAT,EPP,ECP,DMA]
> > > > > [ 3.687691] hwdev DMA mask = 0x0000000000ffffff, dev_addr = 0x0000000020000000
> > > > > [ 3.694916] Kernel panic - not syncing: swiotlb_alloc_coherent: allocated memory is out of range for device
> > > > >
> > > > > I haven't started a bisection yet, but this seems to be introduced
> > > > > somewhere between 2.6.26 and 2.6.27, at least 2.6.26 was working without
> > > > > problems. The dmesg log + config was obtained from a kernel compiled
> > > > > from git on 10/16/2008.
> > > >
> > > > This bug hits me, too. Looks like swiotlb assumes that the alloc caller
> > > > must set GFP_DMA appropriately by itself since GFP_DMA hack was
> > > > removed. The patch below should fix this particular case.
> > >
> > > This happens with 2.6.27, right? GFP_DMA hack was removed post
> > > 2.6.27. What kernel version do you hit this problem?
> >
> > 2.6.27 works fine, at least on my machine.
> > Likely a post-2.6.27 regression.
>
> Ok, it makes sense because I don't see any major changes to swiotlb
> between 2.6.26 and 2.6.27.
>
>
> > > Post 2.6.27, x86's alloc_coherent works a bit differently, but neither
> > > require the caller set to GFP flag. arch/x86/kernel/pci-dma.c does
> > > with 2.6.27 and asm-x86/dma-mapping.h does with post 2.6.27.
> > >
> > >
> > > > HOWEVER: the fundamental problem appears to be in swiotlb itself.
> > > > It assumes that iotlb pages are in DMA area. But, in this case, the
> > > > driver sets 24bit DMA (as of PnP) while iotlb pages are allocated
> > > > under 32bit DMA via alloc_bootmem_low_pages(). This doesn't work, of
> > > > course.
> > >
> > > If a device has 24bit dma mask, alloc_coherent is supposed to use
> > > GFP_DMA.
> >
> > Yes. But what happens if __get_free_pages() fails? Then you get the
> > same problem.
>
> Yeah, but __get_free_pages() with GFP_DMA fails, what can we do in
> such case?
Just return NULL instead of a kernel panic.
> You think that it's a good idea to change swiotlb to
> allocates < 16MB iotlb pages? I'm not sure it's worth to do
> that. 24bit dma mask devices are disappearing.
Seconded, I don't think it's worth to allocate iotlb pages in <16MB,
too. The case with GFP_DMA is somewhat special and mostly for
obsolete devices.
> About the bug that you hit, I suspect that dma_map_coherent() in
> asm-x86/dma-mapping.h doesn't set gfp flags correctly.
>
> dma_map_coherent() calls swiotlb_alloc_coherent with the flags GFP_DMA
> set? parport driver set dev->coherent_dma_mask properly?
The parport driver itself passes always GFP_KERNEL. So I added
GFP_DMA in my initial patch as a workaround.
The device of the parport driver is passed by another, and the driver
doesn't care about DMA mask by itself. In the panic case, it seems
from PnP, and PnP bus has set it to 24bit beforehand, I guess.
> > > > So, even adding GFP_DMA works mostly, it has still potentially
> > > > breakage when you can't get the page and fall back to iotlb pages,
> > > > just like the panic above.
> > > >
> > > > Also, the removal of GFP_DMA hack is a bad idea. For example, if a
> > > > device requires 28bit DMA mask, it doesn't set always GFP_DMA for
> > > > allocation because pages in ZONE_NORMAL may be inside that DMA mask.
> > > > Normal allocators allow this behavior but swiotlb allocator doesn't.
> > > > (Correct me if I'm wrong here -- I haven't followed much the recent
> > > > changes.)
> > >
> > > 28bit DMA mask is supposed to be handled properly. Firstly, we try
> > > with DMA_32BIT_MASK and if an allocated address is not fit for 28bit
> > > mask, we try GFP_DMA again.
> >
> > Yep, dma_generic_alloc_coherent() works like that for ages.
> > My point is about swiotlb_alloc_coherent(), and I don't see the
> > relevant code there...
>
> Oops, you are right. swiotlb doesn't try again with GFP_DMA now. Joerg
> changed the GFP_DMA retry mechanism work only for pci-nommu.c It broke
> GART IOMMU and x86's swiotlb. I modified dma_generic_alloc_coherent to
> work with pci-nommu and GART. I promised Ingo to fix swiotlb too but I
> forgot about it.
>
> Sorry, I'll fix this soon but your case (28bit mask) is supposed to
> work without the GFP_DMA retry mechanism. As I wrote above, I suspect
> that dma flag is not set correctly.
Yes, the parport case should be fixed by passing GFP_DMA.
But, the unconditional panic should be still fixed.
> > > > Last but not least, I think panic() in allocation error path is too
> > > > strict. Usually returning NULL isn't always fatal error, so give some
> > > > more chance to debug, e.g. by calling WARN() (or whatever) instead of
> > > > panic().
> > >
> > > Yeah, this was discussed several times. The problem is that many
> > > drivers assume that dma mapping operations, map_single, map_sg, and
> > > map_coherent, always succeed and doesn't even check the errors. So we
> > > have some panic() in IOMMU drivers to prevent really bad events like
> > > data corruption.
> >
> > Well, but also for alloc_coherent()? Returning NULL from
> > dma_alloc_coherent() is really no fatal error. If the caller doesn't
> > check the return value, then it's a more serious bug, I'd say.
>
> Probably, the majority check dma_alloc_coherent failure. I'll check
> this later to remove panic() in alloc_coherent in IOMMUs.
Thanks!
Takashi
On Wed, 22 Oct 2008 15:32:52 +0200
Takashi Iwai <[email protected]> wrote:
> > About the bug that you hit, I suspect that dma_map_coherent() in
> > asm-x86/dma-mapping.h doesn't set gfp flags correctly.
> >
> > dma_map_coherent() calls swiotlb_alloc_coherent with the flags GFP_DMA
> > set? parport driver set dev->coherent_dma_mask properly?
>
> The parport driver itself passes always GFP_KERNEL. So I added
> GFP_DMA in my initial patch as a workaround.
Hmm, have you actually tried your patch?
dma_alloc_coherent clears the gfp zone flags that the callers pass. So
even if a driver passes GFP_DMA, swiotlb_alloc_coherent doesn't get
GFP_DMA. So I'm not sure how your patch fixes the parport problem.
The current dma_alloc_coherent (asm-x86/dma-mapping.h) handles the gfp
flags in the exact same way as the old dma_alloc_coherent
(pci-dma.c). Neither sets GFP_DMA even if coherent_dma_mask is
24bits. The old code is fine because of the GFP_DMA retry
mechanism. But if coherent_dma_mask is 24bits, there is no point to go
into the GFP_DMA retry mechanism. We should use GFP_DMA in the first
place.
How about the following patch?
diff --git a/include/asm-x86/dma-mapping.h b/include/asm-x86/dma-mapping.h
index 219c33d..05fcec5 100644
--- a/include/asm-x86/dma-mapping.h
+++ b/include/asm-x86/dma-mapping.h
@@ -255,9 +255,11 @@ static inline unsigned long dma_alloc_coherent_mask(struct device *dev,
static inline gfp_t dma_alloc_coherent_gfp_flags(struct device *dev, gfp_t gfp)
{
-#ifdef CONFIG_X86_64
unsigned long dma_mask = dma_alloc_coherent_mask(dev, gfp);
+ if (dma_mask <= DMA_24BIT_MASK)
+ gfp |= GFP_DMA;
+#ifdef CONFIG_X86_64
if (dma_mask <= DMA_32BIT_MASK && !(gfp & GFP_DMA))
gfp |= GFP_DMA32;
#endif
At Thu, 23 Oct 2008 11:36:20 +0900,
FUJITA Tomonori wrote:
>
> On Wed, 22 Oct 2008 15:32:52 +0200
> Takashi Iwai <[email protected]> wrote:
>
> > > About the bug that you hit, I suspect that dma_map_coherent() in
> > > asm-x86/dma-mapping.h doesn't set gfp flags correctly.
> > >
> > > dma_map_coherent() calls swiotlb_alloc_coherent with the flags GFP_DMA
> > > set? parport driver set dev->coherent_dma_mask properly?
> >
> > The parport driver itself passes always GFP_KERNEL. So I added
> > GFP_DMA in my initial patch as a workaround.
>
> Hmm, have you actually tried your patch?
Err, sorry, no I checked only the combination of two patches.
Apparently the first one doesn't do anything good.
> dma_alloc_coherent clears the gfp zone flags that the callers pass. So
> even if a driver passes GFP_DMA, swiotlb_alloc_coherent doesn't get
> GFP_DMA. So I'm not sure how your patch fixes the parport problem.
>
>
> The current dma_alloc_coherent (asm-x86/dma-mapping.h) handles the gfp
> flags in the exact same way as the old dma_alloc_coherent
> (pci-dma.c). Neither sets GFP_DMA even if coherent_dma_mask is
> 24bits. The old code is fine because of the GFP_DMA retry
> mechanism. But if coherent_dma_mask is 24bits, there is no point to go
> into the GFP_DMA retry mechanism. We should use GFP_DMA in the first
> place.
>
> How about the following patch?
I'll give it a try later (the machine is in my office).
thanks,
Takashi
>
>
> diff --git a/include/asm-x86/dma-mapping.h b/include/asm-x86/dma-mapping.h
> index 219c33d..05fcec5 100644
> --- a/include/asm-x86/dma-mapping.h
> +++ b/include/asm-x86/dma-mapping.h
> @@ -255,9 +255,11 @@ static inline unsigned long dma_alloc_coherent_mask(struct device *dev,
>
> static inline gfp_t dma_alloc_coherent_gfp_flags(struct device *dev, gfp_t gfp)
> {
> -#ifdef CONFIG_X86_64
> unsigned long dma_mask = dma_alloc_coherent_mask(dev, gfp);
>
> + if (dma_mask <= DMA_24BIT_MASK)
> + gfp |= GFP_DMA;
> +#ifdef CONFIG_X86_64
> if (dma_mask <= DMA_32BIT_MASK && !(gfp & GFP_DMA))
> gfp |= GFP_DMA32;
> #endif
>
At Thu, 23 Oct 2008 07:22:09 +0200,
I wrote:
>
> > The current dma_alloc_coherent (asm-x86/dma-mapping.h) handles the gfp
> > flags in the exact same way as the old dma_alloc_coherent
> > (pci-dma.c). Neither sets GFP_DMA even if coherent_dma_mask is
> > 24bits. The old code is fine because of the GFP_DMA retry
> > mechanism. But if coherent_dma_mask is 24bits, there is no point to go
> > into the GFP_DMA retry mechanism. We should use GFP_DMA in the first
> > place.
> >
> > How about the following patch?
>
> I'll give it a try later (the machine is in my office).
The patch seems working fine. Thanks!
Tested-by: Takashi Iwai <[email protected]>
Takashi
* Takashi Iwai <[email protected]> wrote:
> At Thu, 23 Oct 2008 07:22:09 +0200,
> I wrote:
> >
> > > The current dma_alloc_coherent (asm-x86/dma-mapping.h) handles the gfp
> > > flags in the exact same way as the old dma_alloc_coherent
> > > (pci-dma.c). Neither sets GFP_DMA even if coherent_dma_mask is
> > > 24bits. The old code is fine because of the GFP_DMA retry
> > > mechanism. But if coherent_dma_mask is 24bits, there is no point to go
> > > into the GFP_DMA retry mechanism. We should use GFP_DMA in the first
> > > place.
> > >
> > > How about the following patch?
> >
> > I'll give it a try later (the machine is in my office).
>
> The patch seems working fine. Thanks!
>
> Tested-by: Takashi Iwai <[email protected]>
thanks guys - i've queued it up in tip/core/urgent and will send a pull
request to Linus later today.
Ingo
On Thu, 23 Oct 2008 12:15:57 +0200
Ingo Molnar <[email protected]> wrote:
>
> * Takashi Iwai <[email protected]> wrote:
>
> > At Thu, 23 Oct 2008 07:22:09 +0200,
> > I wrote:
> > >
> > > > The current dma_alloc_coherent (asm-x86/dma-mapping.h) handles the gfp
> > > > flags in the exact same way as the old dma_alloc_coherent
> > > > (pci-dma.c). Neither sets GFP_DMA even if coherent_dma_mask is
> > > > 24bits. The old code is fine because of the GFP_DMA retry
> > > > mechanism. But if coherent_dma_mask is 24bits, there is no point to go
> > > > into the GFP_DMA retry mechanism. We should use GFP_DMA in the first
> > > > place.
> > > >
> > > > How about the following patch?
> > >
> > > I'll give it a try later (the machine is in my office).
> >
> > The patch seems working fine. Thanks!
> >
> > Tested-by: Takashi Iwai <[email protected]>
>
> thanks guys - i've queued it up in tip/core/urgent and will send a pull
> request to Linus later today.
Thanks, I can't find the commit right now so I'm not sure if you need
the patch description but anyway, here's a sane format of the patch.
=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] x86: use GFP_DMA for 24bit coherent_dma_mask
dma_alloc_coherent (include/asm-x86/dma-mapping.h) avoids GFP_DMA
allocation first and if the allocated address is not fit for the
device's coherent_dma_mask, then dma_alloc_coherent does GFP_DMA
allocation. This is because dma_alloc_coherent avoids precious GFP_DMA
zone if possible. This is also how the old dma_alloc_coherent
(arch/x86/kernel/pci-dma.c) works.
However, if the coherent_dma_mask of a device is 24bit, there is no
point to go into the above GFP_DMA retry mechanism. We had better use
GFP_DMA in the first place.
Signed-off-by: FUJITA Tomonori <[email protected]>
Tested-by: Takashi Iwai <[email protected]>
---
include/asm-x86/dma-mapping.h | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/include/asm-x86/dma-mapping.h b/include/asm-x86/dma-mapping.h
index 219c33d..05fcec5 100644
--- a/include/asm-x86/dma-mapping.h
+++ b/include/asm-x86/dma-mapping.h
@@ -255,9 +255,11 @@ static inline unsigned long dma_alloc_coherent_mask(struct device *dev,
static inline gfp_t dma_alloc_coherent_gfp_flags(struct device *dev, gfp_t gfp)
{
-#ifdef CONFIG_X86_64
unsigned long dma_mask = dma_alloc_coherent_mask(dev, gfp);
+ if (dma_mask <= DMA_24BIT_MASK)
+ gfp |= GFP_DMA;
+#ifdef CONFIG_X86_64
if (dma_mask <= DMA_32BIT_MASK && !(gfp & GFP_DMA))
gfp |= GFP_DMA32;
#endif
--
1.5.5.GIT
At Thu, 23 Oct 2008 12:15:57 +0200,
Ingo Molnar wrote:
>
>
> * Takashi Iwai <[email protected]> wrote:
>
> > At Thu, 23 Oct 2008 07:22:09 +0200,
> > I wrote:
> > >
> > > > The current dma_alloc_coherent (asm-x86/dma-mapping.h) handles the gfp
> > > > flags in the exact same way as the old dma_alloc_coherent
> > > > (pci-dma.c). Neither sets GFP_DMA even if coherent_dma_mask is
> > > > 24bits. The old code is fine because of the GFP_DMA retry
> > > > mechanism. But if coherent_dma_mask is 24bits, there is no point to go
> > > > into the GFP_DMA retry mechanism. We should use GFP_DMA in the first
> > > > place.
> > > >
> > > > How about the following patch?
> > >
> > > I'll give it a try later (the machine is in my office).
> >
> > The patch seems working fine. Thanks!
> >
> > Tested-by: Takashi Iwai <[email protected]>
>
> thanks guys - i've queued it up in tip/core/urgent and will send a pull
> request to Linus later today.
Ingo, what is the status about these patches?
They don't appear in rc2.
thanks,
Takashi
* Takashi Iwai <[email protected]> wrote:
> At Thu, 23 Oct 2008 12:15:57 +0200,
> Ingo Molnar wrote:
> >
> >
> > * Takashi Iwai <[email protected]> wrote:
> >
> > > At Thu, 23 Oct 2008 07:22:09 +0200,
> > > I wrote:
> > > >
> > > > > The current dma_alloc_coherent (asm-x86/dma-mapping.h) handles the gfp
> > > > > flags in the exact same way as the old dma_alloc_coherent
> > > > > (pci-dma.c). Neither sets GFP_DMA even if coherent_dma_mask is
> > > > > 24bits. The old code is fine because of the GFP_DMA retry
> > > > > mechanism. But if coherent_dma_mask is 24bits, there is no point to go
> > > > > into the GFP_DMA retry mechanism. We should use GFP_DMA in the first
> > > > > place.
> > > > >
> > > > > How about the following patch?
> > > >
> > > > I'll give it a try later (the machine is in my office).
> > >
> > > The patch seems working fine. Thanks!
> > >
> > > Tested-by: Takashi Iwai <[email protected]>
> >
> > thanks guys - i've queued it up in tip/core/urgent and will send a pull
> > request to Linus later today.
>
> Ingo, what is the status about these patches?
> They don't appear in rc2.
Will send it today-ish. -rc2 was strictly for brown paperbag bugs
crashing more than half of all testsystems.
Ingo
At Tue, 28 Oct 2008 11:39:02 +0100,
Ingo Molnar wrote:
>
>
> * Takashi Iwai <[email protected]> wrote:
>
> > At Thu, 23 Oct 2008 12:15:57 +0200,
> > Ingo Molnar wrote:
> > >
> > >
> > > * Takashi Iwai <[email protected]> wrote:
> > >
> > > > At Thu, 23 Oct 2008 07:22:09 +0200,
> > > > I wrote:
> > > > >
> > > > > > The current dma_alloc_coherent (asm-x86/dma-mapping.h) handles the gfp
> > > > > > flags in the exact same way as the old dma_alloc_coherent
> > > > > > (pci-dma.c). Neither sets GFP_DMA even if coherent_dma_mask is
> > > > > > 24bits. The old code is fine because of the GFP_DMA retry
> > > > > > mechanism. But if coherent_dma_mask is 24bits, there is no point to go
> > > > > > into the GFP_DMA retry mechanism. We should use GFP_DMA in the first
> > > > > > place.
> > > > > >
> > > > > > How about the following patch?
> > > > >
> > > > > I'll give it a try later (the machine is in my office).
> > > >
> > > > The patch seems working fine. Thanks!
> > > >
> > > > Tested-by: Takashi Iwai <[email protected]>
> > >
> > > thanks guys - i've queued it up in tip/core/urgent and will send a pull
> > > request to Linus later today.
> >
> > Ingo, what is the status about these patches?
> > They don't appear in rc2.
>
> Will send it today-ish. -rc2 was strictly for brown paperbag bugs
> crashing more than half of all testsystems.
OK, thanks!
Takashi