login
Search
Good day. commit 8779f2fc3b84ebb6c5181fb13d702e9944c16069 "x86: don't try to allocate from DMA zone at first" breaks all of ISA DMA. Or all of ALSA ISA DMA at least. All ISA soundcards are silent following that commit -- no error messages, everything appears fine, just silence. It won't just revert due to 32/64 merge. Rene. --
At Fri, 09 May 2008 03:37:54 +0200,
Thanks for catching it. Yeah, the patch looks buggy. We had an
implicit assumption that dev = NULL for ISA devices that require 24bit
DMA.
How about the patch below? It's against the latest Linus git tree.
thanks,
Takashi
[PATCH] x86: Fix dma_alloc_coherent() for ISA devices
The recent work on x86 dma_alloc_coherent() breaks the ISA DMA buffer
allocation, which is represented by "dev = NULL" and requires 24bit
DMA implicitly.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index 0c37f16..c5ef1af 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -385,11 +385,13 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
if (dma_alloc_from_coherent_mem(dev, size, dma_handle, &memory))
return memory;
- if (!dev)
+ if (!dev) {
dev = &fallback_dev;
+ gfp |= GFP_DMA;
+ }
dma_mask = dev->coherent_dma_mask;
if (dma_mask == 0)
- dma_mask = DMA_32BIT_MASK;
+ dma_mask = (gfp & GFP_DMA) ? DMA_24BIT_MASK : DMA_32BIT_MASK;
/* Device not DMA able */
if (dev->dma_mask == NULL)
@@ -403,7 +405,7 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
larger than 16MB and in this case we have a chance of
finding fitting memory in the next higher zone first. If
not retry with true GFP_DMA. -AK */
- if (dma_mask <= DMA_32BIT_MASK)
+ if (dma_mask <= DMA_32BIT_MASK && !(gfp & GFP_DMA))
gfp |= GFP_DMA32;
#endif
--
good catch! Queued it up for testing. Jesse, do you concur? Ingo --
here's the patch below, tidied up.
Ingo
------------------------------->
Subject: x86/pci: fix broken ISA DMA
From: Takashi Iwai <tiwai@suse.de>
Date: Fri, 09 May 2008 08:06:55 +0200
That patch is buggy. We had an implicit assumption that
dev = NULL for ISA devices that require 24bit DMA.
The recent work on x86 dma_alloc_coherent() breaks the ISA DMA buffer
allocation, which is represented by "dev = NULL" and requires 24bit
DMA implicitly.
Bisected-by: Rene Herman <rene.herman@keyaccess.nl>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
arch/x86/kernel/pci-dma.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
Index: linux-x86.q/arch/x86/kernel/pci-dma.c
===================================================================
--- linux-x86.q.orig/arch/x86/kernel/pci-dma.c
+++ linux-x86.q/arch/x86/kernel/pci-dma.c
@@ -386,11 +386,13 @@ dma_alloc_coherent(struct device *dev, s
if (dma_alloc_from_coherent_mem(dev, size, dma_handle, &memory))
return memory;
- if (!dev)
+ if (!dev) {
dev = &fallback_dev;
+ gfp |= GFP_DMA;
+ }
dma_mask = dev->coherent_dma_mask;
if (dma_mask == 0)
- dma_mask = DMA_32BIT_MASK;
+ dma_mask = (gfp & GFP_DMA) ? DMA_24BIT_MASK : DMA_32BIT_MASK;
/* Device not DMA able */
if (dev->dma_mask == NULL)
@@ -404,7 +406,7 @@ dma_alloc_coherent(struct device *dev, s
larger than 16MB and in this case we have a chance of
finding fitting memory in the next higher zone first. If
not retry with true GFP_DMA. -AK */
- if (dma_mask <= DMA_32BIT_MASK)
+ if (dma_mask <= DMA_32BIT_MASK && !(gfp & GFP_DMA))
gfp |= GFP_DMA32;
#endif
--
Patch looks good, applied to my 'for-linus' tree. Thanks, Jesse --
great, thanks Rene for catching this! Added this line to the patch as well: Tested-by: Rene Herman <rene.herman@keyaccess.nl> Ingo --
Oh... it was rather significantly in the AM when I posted so I forgot to mention in the haste to get this out of the way but it was Pete Clements who reported the regression: http://www.nabble.com/Lost-Sound-from-2.6.24-to-2.6.25----CS4236-ISA-tc17062547.html He also tested it, so as long as we're name-mention-flattering around... And this a good excuse to ask how you edit changelog after the fact. Just reset/reapply and stuff or is there a "better" way? I fairly frequently find myself wanting to do just that but it's a bit of a mess when there's already commits on top. Rene. --
generally we prefer append-only repositories for public trees. But as long as you've not pushed it out to others yet, i.e. it's a purely local development tree, you can use two methods: If it's just one commit in some devel branch that you want to put into a 'fixes' branch one you can use git-cherry-pick --edit to shuffle it over. For more complex scenarios you can use git-rebase --interactive to rebase your commits and to edit them. Replace the command "pick" with "edit" to change/fix the commit message. "squash" can be used to fold fixes. Ingo --
Many thanks. Had in fact failed to notice --edit and will definitely check out the interactive rebase. Sounds very useful. Rene. --
Quoting Rene Herman > On 09-05-08 08:06, Takashi Iwai wrote: > > > Thanks for catching it. Yeah, the patch looks buggy. We had an > > implicit assumption that dev = NULL for ISA devices that require 24bit > > DMA. > > > > How about the patch below? It's against the latest Linus git tree. > > Yes, works well. Thank you. > > Rene. > Confirmed here also with git7. Thanks -- Pete Clements --
Woke up and saw it, got worried, but there is already a fix. ;-) --
Naive question #1: Why don't we have a struct device for these ISA devices? PNP builds a struct device with DMA_24BIT_MASK for ISAPNP devices. Naive question #2: Do other architectures need similar fixes in --
On Tue, 13 May 2008 10:59:32 -0600 Because nobody has done the needed work to get all the old ISA drivers converted. I guess isa_device would actually be a platform_device wrapper ? --
No, isa_device is its own thing, on its own isa_bus (*). It has a struct device * readily available though... Rene (*) drivers/base/isa.c, and explanatory changelog at: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=a5117b... --
Thanks for the nice changelog. isa_register_driver() currently doesn't set a DMA mask. Should it? I only see about 35 dma_alloc_coherent() calls that pass NULL. I guess even those would be a fair amount of work to change, and I suppose there would be more that I missed. Bjorn --
At Tue, 13 May 2008 17:18:39 -0600, There are 5 pci_alloc_consistent() with NULL, too. Takashi --
If it's going to be useful, definitely. The attached does not just set dev->dma_mask = &dev->coherent_dma_mask as in the fallback_dev when dma_alloc_coherent() is passed a NULL device only due to the mask juggling in snd_dma_hack_alloc_coherent() (which wouldn't break, but...) but introduces its own copy in struct isa_dev same as struct pnp_dev. As far as I'm aware, there's no actual reason for keeping it other than that and if the hack could go I'd rather lose the private mask copy again also. (the device model still uses a plain u64 by the way but I guess the clean type would be a dma64_addr_t) At least the ALSA one isn't passing a literal NULL it seems. But yes, current NULL-hack reinstatement (it's been merged by Linus already) is definitely the correct fix for now. Would like a comment on the snd_dma_hack_alloc_coherent thing first (no signoff...) but other than that I'll submit this in preparation for it being useful, I guess? Rene.
At Wed, 14 May 2008 14:46:44 +0200, The snd_dma_hack_alloc_coherent() is gone in the latest ALSA tree. Yes. We need to fix the caller of snd_pcm_lib_preallocate_pages*() under sound/isa. Currently it's called with snd_dma_isa_data(), which is expanded to NULL. Replace it with a proper device pointer should suffice. Takashi --
Like this? With this (on top of the previous patch setting the dma_mask ofcourse) legacy ISA actually appears to be fine but it's then ISAPnP which goes bonkers again. Sigh. Getting an allocation failure. Don't understand why yet since pnp_alloc_dev() definitely sets the mask already. Will stare... Rene.
At Wed, 14 May 2008 17:40:55 +0200, Check the master branch of the following tree for the latest ALSA: I prefer passing the device pointer directly. Note that snd_dma_isa_data() is just for making the code compatible more easily with older kernels in alsa-driver tree. Otherwise we'd need a patch file for each source file. But, the patch-file approach thanks, Takashi --
On 14-05-08 17:40, Rene Herman wrote:
You're in a maze of struct device *s, all alike... I was passing the
pnp_card->dev instead of the initialized pnp_dev->dev.
And, not doing so brings out a difference between ISAPnP and legacy ISA
again insofar that legacy ISA does not consist of cards with multiple
devices. We just have the single struct device * for the ISA device.
This therefore would be the easiest solution (and works fine) but seems
a bit of a hack. Bjorn, do you have an opinion? If I abstract things out
a bit more I might be able to do this nicer. One might on the other hand
argue that the dma_mask is going to be constant for all card devices so
might as well just use the card dev.
sound/{isa,oss} together with drivers/isdn/hisax/ are the only pnp_card
users.
diff --git a/drivers/pnp/card.c b/drivers/pnp/card.c
index a762a41..a2842a7 100644
--- a/drivers/pnp/card.c
+++ b/drivers/pnp/card.c
@@ -7,6 +7,7 @@
#include <linux/module.h>
#include <linux/ctype.h>
#include <linux/slab.h>
+#include <linux/dma-mapping.h>
#include <linux/pnp.h>
#include "base.h"
@@ -167,6 +168,9 @@ struct pnp_card *pnp_alloc_card(struct pnp_protocol *protocol, int id, char *pnp
sprintf(card->dev.bus_id, "%02x:%02x", card->protocol->number,
card->number);
+ card->dev.coherent_dma_mask = DMA_24BIT_MASK;
+ card->dev.dma_mask = &card->dev.coherent_dma_mask;
+
dev_id = pnp_add_card_id(card, pnpid);
if (!dev_id) {
kfree(card);
--
I agree, it seems a bit of a hack to use a DMA mask from the card instead of from the device, since the driver should be programming the device to do the DMA. But I know very little about pnp_card in general, so don't attach too --
Okay, I'll sit on this for a bit. Right now we're using a global device even but this is exactly about cleaning that up so couldn't convince myself. Will see what happens when I try to make it nice... Rene. --
It gets uglier. ALSA ISA drivers (for cards that exist both as legacy and as ISAPnP at least) keep a merged legacy/isapnp model; PnP is used mostly for initializing global variables that the same old legacy probe routines then reference. This means that beyond that global resource init step the specific struct device is no longer available. Without restructuring too many things really only fixable through other hacks again such as a global dma_dev[] array or some such. From the viewpoint of PnP itself setting the dma_mask for a pnp_card (a pnp_dev collection) makes isolated sense so if no objections, I'll submit the attached after all. From the ALSA side we'd then pass the card dev (which we'd also do for isa_dev) and keep in mind that we might want to get more specific if over time structure permits it. struct snd_pcm already has its own struct device * which would be the right one here but it's setting that which gets ugly... Rene.
On 30-05-08 23:15, Rene Herman wrote: ... this note by the way I believe pnp_dev might as well get rid of its dma_mask as well. As far I've googled up the history of that the reason why dev->dma_mask is a pointer is only that it's been moved into struct device from struct pci_dev where the latter location was kept as the main one so as to not upset then current code. Everyone else seems to have then faithfully cloned pci_dev and stuck it in their private structs as well but for no good reason it would appear. And in the case of the PnP ISA masks, we're talking about constant masks, dictated by the shared global DMA controller and not the card itself (there are a few ISA cards that do their own busmastering but they're special) so the mask might as well just point to the coherent mask. Unless I'm missing something ofcourse... Rene. --
Looks good to me. It does sound like a lot of work and possibly more risk than it's worth to fix up some of this stuff. I do still wonder whether any non-x86 architectures need similar fixes in dma_alloc_coherent(), i.e., check for dev==NULL and fall back to a 24-bit DMA mask. Acked-by: Bjorn Helgaas <bjorn.helgaas@hp.com> --
Fairly invasive at least. The good thing though is that with the recent pnp_manual_config_dev() removal the PnP drivers have no actual need/use for this global variable model anymore and now I have a great excuse for Hrmmpf. Good question. In sound/isa, we've had a but of alpha spottyness Thanks. I'll assume Andrew picks it up from the CC... Rene. --
On 31-05-08 00:11, Rene Herman wrote: And here's the ISA one... Rene.
On Sat, 31 May 2008 00:37:05 +0200 Argh. Could we please be better with the changelogs? This one tells us briefly what the patch does, but it deosn't tell us why it does it. It doesn't tell us whether it fixes some bug (and if so what that bug is) and it gives me no means of determining whether the patch is needed in 2.6.26 or in 2.6.25.x. Ditto pnp-set-the-pnp_card-dma_mask-for-use-by-isapnp-cards.patch --
If you already knew what it was about these were nicely concise and to the point... Yaya. I'll resend. Rene. --
At Sat, 31 May 2008 01:54:10 +0200, Acked-by: Takashi Iwai <tiwai@suse.de> thanks, --
At Sat, 31 May 2008 01:55:52 +0200, Acked-by: Takashi Iwai <tiwai@suse.de> --