[alsa-devel] 2.6.26-rc1 regression: ISA DMA broken (bisected)
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, Rene Herman wrote:
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.
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
* Takashi Iwai tiwai@suse.de 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.
good catch! Queued it up for testing. Jesse, do you concur?
Ingo
* Ingo Molnar mingo@elte.hu wrote:
good catch! Queued it up for testing. Jesse, do you concur?
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
Rene Herman reported:
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.
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
* Rene Herman rene.herman@keyaccess.nl wrote:
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.
great, thanks Rene for catching this! Added this line to the patch as well:
Tested-by: Rene Herman rene.herman@keyaccess.nl
Ingo
On 09-05-08 14:28, Ingo Molnar wrote:
- Rene Herman rene.herman@keyaccess.nl wrote:
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.
great, thanks Rene for catching this! Added this line to the patch as well:
Tested-by: Rene Herman rene.herman@keyaccess.nl
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-tc17062...
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.
* Rene Herman rene.herman@keyaccess.nl wrote:
great, thanks Rene for catching this! Added this line to the patch as well:
Tested-by: Rene Herman rene.herman@keyaccess.nl
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-tc17062...
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.
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
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
On Friday 09 May 2008 12:06:55 am 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.
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 dma_alloc_coherent()?
[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
On Tue, 13 May 2008 10:59:32 -0600 Bjorn Helgaas bjorn.helgaas@hp.com wrote:
On Friday 09 May 2008 12:06:55 am 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.
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.
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 ?
On 13-05-08 19:01, Alan Cox wrote:
On Tue, 13 May 2008 10:59:32 -0600 Bjorn Helgaas bjorn.helgaas@hp.com wrote:
On Friday 09 May 2008 12:06:55 am 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.
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.
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=commitdif...
On Tuesday 13 May 2008 11:33:25 am Rene Herman wrote:
On 13-05-08 19:01, Alan Cox wrote:
On Tue, 13 May 2008 10:59:32 -0600, Bjorn Helgaas bjorn.helgaas@hp.com wrote:
On Friday 09 May 2008 12:06:55 am 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.
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.
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...
(*) drivers/base/isa.c, and explanatory changelog at:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdif...
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, Bjorn Helgaas wrote:
On Tuesday 13 May 2008 11:33:25 am Rene Herman wrote:
On 13-05-08 19:01, Alan Cox wrote:
On Tue, 13 May 2008 10:59:32 -0600, Bjorn Helgaas bjorn.helgaas@hp.com wrote:
On Friday 09 May 2008 12:06:55 am 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.
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.
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...
(*) drivers/base/isa.c, and explanatory changelog at:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdif...
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.
There are 5 pci_alloc_consistent() with NULL, too.
Takashi
On 14-05-08 01:18, Bjorn Helgaas wrote:
On Tuesday 13 May 2008 11:33:25 am Rene Herman wrote:
No, isa_device is its own thing, on its own isa_bus (*). It has a struct device * readily available though...
(*) drivers/base/isa.c, and explanatory changelog at:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdif...
Thanks for the nice changelog.
isa_register_driver() currently doesn't set a DMA mask. Should it?
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)
Inlining is whitespace-failing here. Patch itself is trivial...
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.
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.
From: Rene Herman rene.herman@gmail.com
ISA: set 24-bit dma_mask for ISA devices
Set the ISA device dma_mask in preparation for using the actual device with the DMA API. --- drivers/base/isa.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/drivers/base/isa.c b/drivers/base/isa.c index d222239..842ca08 100644 --- a/drivers/base/isa.c +++ b/drivers/base/isa.c @@ -7,6 +7,8 @@ #include <linux/slab.h> #include <linux/module.h> #include <linux/init.h> +#include <linux/types.h> +#include <linux/dma-mapping.h> #include <linux/isa.h>
static struct device isa_bus = { @@ -17,6 +19,7 @@ struct isa_dev { struct device dev; struct device *next; unsigned int id; + u64 dma_mask; };
#define to_isa_dev(x) container_of((x), struct isa_dev, dev) @@ -131,6 +134,9 @@ int isa_register_driver(struct isa_driver *isa_driver, unsigned int ndev) break; }
+ isa_dev->id = id; + isa_dev->dma_mask = DMA_24BIT_MASK; + isa_dev->dev.parent = &isa_bus; isa_dev->dev.bus = &isa_bus_type;
@@ -138,8 +144,9 @@ int isa_register_driver(struct isa_driver *isa_driver, unsigned int ndev) isa_driver->driver.name, id);
isa_dev->dev.platform_data = isa_driver; + isa_dev->dev.dma_mask = &isa_dev->dma_mask; + isa_dev->dev.coherent_dma_mask = isa_dev->dma_mask; isa_dev->dev.release = isa_dev_release; - isa_dev->id = id;
error = device_register(&isa_dev->dev); if (error) {
At Wed, 14 May 2008 14:46:44 +0200, Rene Herman wrote:
On 14-05-08 01:18, Bjorn Helgaas wrote:
On Tuesday 13 May 2008 11:33:25 am Rene Herman wrote:
No, isa_device is its own thing, on its own isa_bus (*). It has a struct device * readily available though...
(*) drivers/base/isa.c, and explanatory changelog at:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdif...
Thanks for the nice changelog.
isa_register_driver() currently doesn't set a DMA mask. Should it?
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 snd_dma_hack_alloc_coherent() is gone in the latest ALSA tree. It wasn't merged to 2.6.26, though.
(the device model still uses a plain u64 by the way but I guess the clean type would be a dma64_addr_t)
Inlining is whitespace-failing here. Patch itself is trivial...
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.
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.
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
On 14-05-08 15:01, Takashi Iwai wrote:
At Wed, 14 May 2008 14:46:44 +0200, Rene Herman wrote:
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 snd_dma_hack_alloc_coherent() is gone in the latest ALSA tree. It wasn't merged to 2.6.26, though.
Ah, good, thanks, I'll forward port to current ALSA.
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.
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.
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.
diff --git a/drivers/base/isa.c b/drivers/base/isa.c diff --git a/include/sound/memalloc.h b/include/sound/memalloc.h index ae2921d..81dd009 100644 --- a/include/sound/memalloc.h +++ b/include/sound/memalloc.h @@ -36,7 +36,7 @@ struct snd_dma_device {
#ifndef snd_dma_pci_data #define snd_dma_pci_data(pci) (&(pci)->dev) -#define snd_dma_isa_data() NULL +#define snd_dma_isa_data(card) ((card)->dev) #define snd_dma_sbus_data(sbus) ((struct device *)(sbus)) #define snd_dma_continuous_data(x) ((struct device *)(unsigned long)(x)) #endif diff --git a/sound/isa/ad1816a/ad1816a_lib.c b/sound/isa/ad1816a/ad1816a_lib.c index 4b8dfe2..b1a1b79 100644 --- a/sound/isa/ad1816a/ad1816a_lib.c +++ b/sound/isa/ad1816a/ad1816a_lib.c @@ -677,7 +677,7 @@ int __devinit snd_ad1816a_pcm(struct snd_ad1816a *chip, int device, struct snd_p snd_ad1816a_init(chip);
snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, - snd_dma_isa_data(), + snd_dma_isa_data(chip->card), 64*1024, chip->dma1 > 3 || chip->dma2 > 3 ? 128*1024 : 64*1024);
chip->pcm = pcm; diff --git a/sound/isa/ad1848/ad1848.c b/sound/isa/ad1848/ad1848.c index 5f5271e..e992d69 100644 --- a/sound/isa/ad1848/ad1848.c +++ b/sound/isa/ad1848/ad1848.c @@ -95,6 +95,8 @@ static int __devinit snd_ad1848_probe(struct device *dev, unsigned int n) if (!card) return -EINVAL;
+ snd_card_set_dev(card, dev); + error = snd_ad1848_create(card, port[n], irq[n], dma1[n], thinkpad[n] ? AD1848_HW_THINKPAD : AD1848_HW_DETECT, &chip); if (error < 0) @@ -118,8 +120,6 @@ static int __devinit snd_ad1848_probe(struct device *dev, unsigned int n) if (thinkpad[n]) strcat(card->longname, " [Thinkpad]");
- snd_card_set_dev(card, dev); - error = snd_card_register(card); if (error < 0) goto out; diff --git a/sound/isa/ad1848/ad1848_lib.c b/sound/isa/ad1848/ad1848_lib.c index 630c90f..2bdcb50 100644 --- a/sound/isa/ad1848/ad1848_lib.c +++ b/sound/isa/ad1848/ad1848_lib.c @@ -963,7 +963,7 @@ int snd_ad1848_pcm(struct snd_ad1848 *chip, int device, struct snd_pcm **rpcm) strcpy(pcm->name, snd_ad1848_chip_id(chip));
snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, - snd_dma_isa_data(), + snd_dma_isa_data(chip->card), 64*1024, chip->dma > 3 ? 128*1024 : 64*1024);
chip->pcm = pcm; diff --git a/sound/isa/adlib.c b/sound/isa/adlib.c index efa8c80..809f514 100644 --- a/sound/isa/adlib.c +++ b/sound/isa/adlib.c @@ -59,6 +59,8 @@ static int __devinit snd_adlib_probe(struct device *dev, unsigned int n) return -EINVAL; }
+ snd_card_set_dev(card, dev); + card->private_data = request_region(port[n], 4, CRD_NAME); if (!card->private_data) { snd_printk(KERN_ERR "%s: could not grab ports\n", dev->bus_id); @@ -83,8 +85,6 @@ static int __devinit snd_adlib_probe(struct device *dev, unsigned int n) goto out; }
- snd_card_set_dev(card, dev); - error = snd_card_register(card); if (error < 0) { snd_printk(KERN_ERR "%s: could not register card\n", dev->bus_id); diff --git a/sound/isa/cmi8330.c b/sound/isa/cmi8330.c index 4d198ec..75b5db6 100644 --- a/sound/isa/cmi8330.c +++ b/sound/isa/cmi8330.c @@ -395,7 +395,7 @@ static int __devinit snd_cmi8330_pcm(struct snd_card *card, struct snd_cmi8330 * snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &chip->streams[SNDRV_PCM_STREAM_CAPTURE].ops);
snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, - snd_dma_isa_data(), + snd_dma_isa_data(card), 64*1024, 128*1024); chip->pcm = pcm;
@@ -603,12 +603,12 @@ static int __devinit snd_cmi8330_pnp_detect(struct pnp_card_link *pcard, card = snd_cmi8330_card_new(dev); if (! card) return -ENOMEM; + snd_card_set_dev(card, &pcard->card->dev); if ((res = snd_cmi8330_pnp(dev, card->private_data, pcard, pid)) < 0) { snd_printk(KERN_ERR PFX "PnP detection failed\n"); snd_card_free(card); return res; } - snd_card_set_dev(card, &pcard->card->dev); if ((res = snd_cmi8330_probe(card, dev)) < 0) { snd_card_free(card); return res; diff --git a/sound/isa/cs423x/cs4231.c b/sound/isa/cs423x/cs4231.c index e9462b9..fcbe1b2 100644 --- a/sound/isa/cs423x/cs4231.c +++ b/sound/isa/cs423x/cs4231.c @@ -99,6 +99,8 @@ static int __devinit snd_cs4231_probe(struct device *dev, unsigned int n) if (!card) return -EINVAL;
+ snd_card_set_dev(card, dev); + error = snd_cs4231_create(card, port[n], -1, irq[n], dma1[n], dma2[n], CS4231_HW_DETECT, 0, &chip); if (error < 0) @@ -136,8 +138,6 @@ static int __devinit snd_cs4231_probe(struct device *dev, unsigned int n) printk(KERN_WARNING "%s: MPU401 not detected\n", dev->bus_id); }
- snd_card_set_dev(card, dev); - error = snd_card_register(card); if (error < 0) goto out; diff --git a/sound/isa/cs423x/cs4231_lib.c b/sound/isa/cs423x/cs4231_lib.c index 0aa8649..678e9df 100644 --- a/sound/isa/cs423x/cs4231_lib.c +++ b/sound/isa/cs423x/cs4231_lib.c @@ -1541,7 +1541,7 @@ int snd_cs4231_pcm(struct snd_cs4231 *chip, int device, struct snd_pcm **rpcm) strcpy(pcm->name, snd_cs4231_chip_id(chip));
snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, - snd_dma_isa_data(), + snd_dma_isa_data(chip->card), 64*1024, chip->dma1 > 3 || chip->dma2 > 3 ? 128*1024 : 64*1024);
chip->pcm = pcm; diff --git a/sound/isa/es1688/es1688.c b/sound/isa/es1688/es1688.c index f88639e..80951b8 100644 --- a/sound/isa/es1688/es1688.c +++ b/sound/isa/es1688/es1688.c @@ -128,6 +128,8 @@ static int __devinit snd_es1688_probe(struct device *dev, unsigned int n) if (!card) return -EINVAL;
+ snd_card_set_dev(card, dev); + error = snd_es1688_legacy_create(card, dev, n, &chip); if (error < 0) goto out; @@ -164,8 +166,6 @@ static int __devinit snd_es1688_probe(struct device *dev, unsigned int n) goto out; }
- snd_card_set_dev(card, dev); - error = snd_card_register(card); if (error < 0) goto out; diff --git a/sound/isa/es1688/es1688_lib.c b/sound/isa/es1688/es1688_lib.c index 1e1e575..170d78c 100644 --- a/sound/isa/es1688/es1688_lib.c +++ b/sound/isa/es1688/es1688_lib.c @@ -740,7 +740,7 @@ int snd_es1688_pcm(struct snd_es1688 * chip, int device, struct snd_pcm ** rpcm) chip->pcm = pcm;
snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, - snd_dma_isa_data(), + snd_dma_isa_data(chip->card), 64*1024, 64*1024);
if (rpcm) diff --git a/sound/isa/es18xx.c b/sound/isa/es18xx.c index 90498e4..05b3aca 100644 --- a/sound/isa/es18xx.c +++ b/sound/isa/es18xx.c @@ -1721,7 +1721,7 @@ static int __devinit snd_es18xx_pcm(struct snd_es18xx *chip, int device, struct chip->pcm = pcm;
snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, - snd_dma_isa_data(), + snd_dma_isa_data(chip->card), 64*1024, chip->dma1 > 3 || chip->dma2 > 3 ? 128*1024 : 64*1024);
diff --git a/sound/isa/gus/gus_pcm.c b/sound/isa/gus/gus_pcm.c index 99731dc..b82bc55 100644 --- a/sound/isa/gus/gus_pcm.c +++ b/sound/isa/gus/gus_pcm.c @@ -857,7 +857,7 @@ int snd_gf1_pcm_new(struct snd_gus_card * gus, int pcm_dev, int control_index, s
for (substream = pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream; substream; substream = substream->next) snd_pcm_lib_preallocate_pages(substream, SNDRV_DMA_TYPE_DEV, - snd_dma_isa_data(), + snd_dma_isa_data(card), 64*1024, gus->gf1.dma1 > 3 ? 128*1024 : 64*1024); pcm->info_flags = 0; @@ -867,7 +867,7 @@ int snd_gf1_pcm_new(struct snd_gus_card * gus, int pcm_dev, int control_index, s if (gus->gf1.dma2 == gus->gf1.dma1) pcm->info_flags |= SNDRV_PCM_INFO_HALF_DUPLEX; snd_pcm_lib_preallocate_pages(pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream, - SNDRV_DMA_TYPE_DEV, snd_dma_isa_data(), + SNDRV_DMA_TYPE_DEV, snd_dma_isa_data(card), 64*1024, gus->gf1.dma2 > 3 ? 128*1024 : 64*1024); } strcpy(pcm->name, pcm->id); diff --git a/sound/isa/gus/gusclassic.c b/sound/isa/gus/gusclassic.c index 8f914b3..d6e5124 100644 --- a/sound/isa/gus/gusclassic.c +++ b/sound/isa/gus/gusclassic.c @@ -155,6 +155,8 @@ static int __devinit snd_gusclassic_probe(struct device *dev, unsigned int n) if (!card) return -EINVAL;
+ snd_card_set_dev(card, dev); + if (pcm_channels[n] < 2) pcm_channels[n] = 2;
@@ -201,8 +203,6 @@ static int __devinit snd_gusclassic_probe(struct device *dev, unsigned int n) sprintf(card->longname + strlen(card->longname), "&%d", gus->gf1.dma2);
- snd_card_set_dev(card, dev); - error = snd_card_register(card); if (error < 0) goto out; diff --git a/sound/isa/gus/gusextreme.c b/sound/isa/gus/gusextreme.c index da13185..c016193 100644 --- a/sound/isa/gus/gusextreme.c +++ b/sound/isa/gus/gusextreme.c @@ -249,6 +249,8 @@ static int __devinit snd_gusextreme_probe(struct device *dev, unsigned int n) if (!card) return -EINVAL;
+ snd_card_set_dev(card, dev); + if (mpu_port[n] == SNDRV_AUTO_PORT) mpu_port[n] = 0;
@@ -330,8 +332,6 @@ static int __devinit snd_gusextreme_probe(struct device *dev, unsigned int n) "irq %i&%i, dma %i&%i", es1688->port, gus->gf1.irq, es1688->irq, gus->gf1.dma1, es1688->dma8);
- snd_card_set_dev(card, dev); - error = snd_card_register(card); if (error < 0) goto out; diff --git a/sound/isa/gus/gusmax.c b/sound/isa/gus/gusmax.c index f87c623..47c287b 100644 --- a/sound/isa/gus/gusmax.c +++ b/sound/isa/gus/gusmax.c @@ -221,6 +221,9 @@ static int __devinit snd_gusmax_probe(struct device *pdev, unsigned int dev) sizeof(struct snd_gusmax)); if (card == NULL) return -ENOMEM; + + snd_card_set_dev(card, pdev); + card->private_free = snd_gusmax_free; maxcard = (struct snd_gusmax *)card->private_data; maxcard->card = card; @@ -334,8 +337,6 @@ static int __devinit snd_gusmax_probe(struct device *pdev, unsigned int dev) if (xdma2 >= 0) sprintf(card->longname + strlen(card->longname), "&%i", xdma2);
- snd_card_set_dev(card, pdev); - if ((err = snd_card_register(card)) < 0) goto _err; diff --git a/sound/isa/opti9xx/miro.c b/sound/isa/opti9xx/miro.c index 2a1e2f5..2dbf2f8 100644 --- a/sound/isa/opti9xx/miro.c +++ b/sound/isa/opti9xx/miro.c @@ -1231,6 +1231,8 @@ static int __devinit snd_miro_probe(struct device *devptr, unsigned int n) sizeof(struct snd_miro)))) return -ENOMEM;
+ snd_card_set_dev(card, devptr); + card->private_free = snd_card_miro_free; miro = card->private_data; miro->card = card; @@ -1396,8 +1398,6 @@ static int __devinit snd_miro_probe(struct device *devptr, unsigned int n) return error; }
- snd_card_set_dev(card, devptr); - if ((error = snd_card_register(card))) { snd_card_free(card); return error; diff --git a/sound/isa/opti9xx/opti92x-ad1848.c b/sound/isa/opti9xx/opti92x-ad1848.c index fe1afc1..761347f 100644 --- a/sound/isa/opti9xx/opti92x-ad1848.c +++ b/sound/isa/opti9xx/opti92x-ad1848.c @@ -1368,7 +1368,7 @@ static int snd_opti93x_pcm(struct snd_opti93x *codec, int device, struct snd_pcm strcpy(pcm->name, snd_opti93x_chip_id(codec));
snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, - snd_dma_isa_data(), + snd_dma_isa_data(codec->card), 64*1024, codec->dma1 > 3 || codec->dma2 > 3 ? 128*1024 : 64*1024);
codec->pcm = pcm; diff --git a/sound/isa/sb/sb16_main.c b/sound/isa/sb/sb16_main.c index f7e8192..9878041 100644 --- a/sound/isa/sb/sb16_main.c +++ b/sound/isa/sb/sb16_main.c @@ -887,7 +887,7 @@ int snd_sb16dsp_pcm(struct snd_sb * chip, int device, struct snd_pcm ** rpcm) pcm->info_flags = SNDRV_PCM_INFO_HALF_DUPLEX;
snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, - snd_dma_isa_data(), + snd_dma_isa_data(card), 64*1024, 128*1024);
if (rpcm) diff --git a/sound/isa/sb/sb8.c b/sound/isa/sb/sb8.c index 336a342..8cb40ef 100644 --- a/sound/isa/sb/sb8.c +++ b/sound/isa/sb/sb8.c @@ -107,6 +107,9 @@ static int __devinit snd_sb8_probe(struct device *pdev, unsigned int dev) sizeof(struct snd_sb8)); if (card == NULL) return -ENOMEM; + + snd_card_set_dev(card, pdev); + acard = card->private_data; card->private_free = snd_sb8_free;
@@ -191,8 +194,6 @@ static int __devinit snd_sb8_probe(struct device *pdev, unsigned int dev) chip->port, irq[dev], dma8[dev]);
- snd_card_set_dev(card, pdev); - if ((err = snd_card_register(card)) < 0) goto _err;
diff --git a/sound/isa/sb/sb8_main.c b/sound/isa/sb/sb8_main.c index fe03bb8..7ef15a3 100644 --- a/sound/isa/sb/sb8_main.c +++ b/sound/isa/sb/sb8_main.c @@ -524,7 +524,7 @@ int snd_sb8dsp_pcm(struct snd_sb *chip, int device, struct snd_pcm ** rpcm) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &snd_sb8_capture_ops);
snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, - snd_dma_isa_data(), + snd_dma_isa_data(card), 64*1024, 64*1024);
if (rpcm) diff --git a/sound/isa/sc6000.c b/sound/isa/sc6000.c index da3d152..a99c42c 100644 --- a/sound/isa/sc6000.c +++ b/sound/isa/sc6000.c @@ -493,6 +493,8 @@ static int __devinit snd_sc6000_probe(struct device *devptr, unsigned int dev) if (!card) return -ENOMEM;
+ snd_card_set_dev(card, devptr); + if (xirq == SNDRV_AUTO_IRQ) { xirq = snd_legacy_find_free_irq(possible_irqs); if (xirq < 0) { @@ -602,8 +604,6 @@ static int __devinit snd_sc6000_probe(struct device *devptr, unsigned int dev) sprintf(card->longname, "Gallant SC-6000 at 0x%lx, irq %d, dma %d", mss_port[dev], xirq, xdma);
- snd_card_set_dev(card, devptr); - err = snd_card_register(card); if (err < 0) goto err_unmap2; diff --git a/sound/isa/sgalaxy.c b/sound/isa/sgalaxy.c index a07274e..2e04666 100644 --- a/sound/isa/sgalaxy.c +++ b/sound/isa/sgalaxy.c @@ -243,6 +243,8 @@ static int __devinit snd_sgalaxy_probe(struct device *devptr, unsigned int dev) if (card == NULL) return -ENOMEM;
+ snd_card_set_dev(card, devptr); + xirq = irq[dev]; if (xirq == SNDRV_AUTO_IRQ) { if ((xirq = snd_legacy_find_free_irq(possible_irqs)) < 0) { @@ -287,8 +289,6 @@ static int __devinit snd_sgalaxy_probe(struct device *devptr, unsigned int dev) sprintf(card->longname, "Sound Galaxy at 0x%lx, irq %d, dma %d", wssport[dev], xirq, xdma1);
- snd_card_set_dev(card, devptr); - if ((err = snd_card_register(card)) < 0) goto _err;
diff --git a/sound/isa/sscape.c b/sound/isa/sscape.c index 06ad786..002a3a0 100644 --- a/sound/isa/sscape.c +++ b/sound/isa/sscape.c @@ -147,6 +147,7 @@ struct soundscape { enum card_type type; struct resource *io_res; struct resource *wss_res; + struct snd_card *card; struct snd_cs4231 *chip; struct snd_mpu401 *mpu; struct snd_hwdep *hw; @@ -185,16 +186,18 @@ static inline struct soundscape *get_hwdep_soundscape(struct snd_hwdep * hw) * I think this means that the memory has to map to * contiguous pages of physical memory. */ -static struct snd_dma_buffer *get_dmabuf(struct snd_dma_buffer *buf, unsigned long size) +static struct snd_dma_buffer *get_dmabuf(struct snd_card *card, + struct snd_dma_buffer *buf, unsigned long size) { if (buf) { - if (snd_dma_alloc_pages_fallback(SNDRV_DMA_TYPE_DEV, snd_dma_isa_data(), + if (snd_dma_alloc_pages_fallback(SNDRV_DMA_TYPE_DEV, + snd_dma_isa_data(card), size, buf) < 0) { - snd_printk(KERN_ERR "sscape: Failed to allocate %lu bytes for DMA\n", size); + snd_printk(KERN_ERR "sscape: Failed to allocate " + "%lu bytes for DMA\n", size); return NULL; } } - return buf; }
@@ -452,7 +455,7 @@ static int upload_dma_data(struct soundscape *s, struct snd_dma_buffer dma; int ret;
- if (!get_dmabuf(&dma, PAGE_ALIGN(size))) + if (!get_dmabuf(s->card, &dma, PAGE_ALIGN(size))) return -ENOMEM;
spin_lock_irqsave(&s->lock, flags); @@ -1359,7 +1362,10 @@ static int __devinit snd_sscape_probe(struct device *pdev, unsigned int dev) if (!card) return -ENOMEM;
+ snd_card_set_dev(card, pdev); + sscape = get_card_soundscape(card); + sscape->card = card; sscape->type = SSCAPE;
dma[dev] &= 0x03; @@ -1367,7 +1373,6 @@ static int __devinit snd_sscape_probe(struct device *pdev, unsigned int dev) if (ret < 0) goto _release_card;
- snd_card_set_dev(card, pdev); if ((ret = snd_card_register(card)) < 0) { printk(KERN_ERR "sscape: Failed to register sound card\n"); goto _release_card; @@ -1464,7 +1469,10 @@ static int __devinit sscape_pnp_detect(struct pnp_card_link *pcard, if (!card) return -ENOMEM;
+ snd_card_set_dev(card, &pcard->card->dev); + sscape = get_card_soundscape(card); + sscape->card = card;
/* * Identify card model ... @@ -1493,7 +1501,6 @@ static int __devinit sscape_pnp_detect(struct pnp_card_link *pcard, if (ret < 0) goto _release_card;
- snd_card_set_dev(card, &pcard->card->dev); if ((ret = snd_card_register(card)) < 0) { printk(KERN_ERR "sscape: Failed to register sound card\n"); goto _release_card;
At Wed, 14 May 2008 17:40:55 +0200, Rene Herman wrote:
On 14-05-08 15:01, Takashi Iwai wrote:
At Wed, 14 May 2008 14:46:44 +0200, Rene Herman wrote:
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 snd_dma_hack_alloc_coherent() is gone in the latest ALSA tree. It wasn't merged to 2.6.26, though.
Ah, good, thanks, I'll forward port to current ALSA.
Check the master branch of the following tree for the latest ALSA: git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git
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.
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.
Like this?
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 is cleaner (for the upstream) anyway at this stage.
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...
thanks,
Takashi
On 14-05-08 17:40, Rene Herman wrote:
CC list trimmed as now PNP and ALSA specific.
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...
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);
On Wednesday 14 May 2008 12:41:56 pm Rene Herman wrote:
On 14-05-08 17:40, Rene Herman wrote:
CC list trimmed as now PNP and ALSA specific.
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...
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.
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 much weight to my opinion.
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);
On 14-05-08 20:50, Bjorn Helgaas wrote:
On Wednesday 14 May 2008 12:41:56 pm 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.
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 much weight to my opinion.
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.
On 14-05-08 21:09, Rene Herman wrote:
On 14-05-08 20:50, Bjorn Helgaas wrote:
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 much weight to my opinion.
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...
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:
On ...
@@ -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);
... 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.
On Friday 30 May 2008 03:15:57 pm Rene Herman wrote:
On 14-05-08 21:09, Rene Herman wrote:
On 14-05-08 20:50, Bjorn Helgaas wrote:
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 much weight to my opinion.
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...
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...
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
On 30-05-08 23:43, Bjorn Helgaas wrote:
On Friday 30 May 2008 03:15:57 pm Rene Herman wrote:
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...
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.
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 rewriting them. That can happen one at a time though...
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.
Hrmmpf. Good question. In sound/isa, we've had a but of alpha spottyness over time but nothing which would seem to be related.
Acked-by: Bjorn Helgaas bjorn.helgaas@hp.com
Thanks. I'll assume Andrew picks it up from the CC...
Rene.
On Sat, 31 May 2008 00:37:05 +0200 Rene Herman rene.herman@keyaccess.nl wrote:
[ PNP: set the pnp_card dma_mask for use by ISAPnP cards. ]
Thanks. I'll assume Andrew picks it up from the CC...
Actually I would have missed it
And here's the ISA one...
If this one hadn't mentioned it.
From: Rene Herman rene.herman@gmail.com Date: Sat, 31 May 2008 00:31:40 +0200 Subject: [PATCH] ISA: set 24-bit dma_mask for ISA devices.
Set the ISA device dma_mask in preparation for using the actual device with the DMA API.
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
At Sat, 31 May 2008 01:54:10 +0200, Rene Herman wrote:
From 801c13fb3e8564221a1fb21892dbe13add3d7cea Mon Sep 17 00:00:00 2001
From: Rene Herman rene.herman@gmail.com Date: Fri, 30 May 2008 23:10:23 +0200 Subject: [PATCH] PNP: set the pnp_card dma_mask for use by ISAPnP cards.
dma_alloc_coherent() on x86 currently takes a passed in NULL device pointer to mean that it should allocate an ISA compatible (24-bit) buffer which is a bit of a hack.
The ALSA ISA drivers are the main consumers of this but have a struct device in fact readily available.
For the PnP drivers, the specific pnp_dev->dev device pointer is not always available at the right time so for now we want to pass the pnp_card->dev instead which is always available. Set its dma_mask in preparation for doing so.
This does not fix a current bug -- 2.6.26-rc1 stumbled over the NULL hack in dma_alloc_coherent() but this has already been fixed in commit 4a367f3a9dbf2e7ffcee4702203479809236ee6e by Takashi Iwai.
Signed-off-by: Rene Herman rene.herman@gmail.com Acked-by: Bjorn Helgaas bjorn.helgaas@hp.com Cc: Takashi Iwai tiwai@suse.de
Acked-by: Takashi Iwai tiwai@suse.de
thanks,
Takashi
Cc: Alan Cox alan@lxorguk.ukuu.org.uk
drivers/pnp/card.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/pnp/card.c b/drivers/pnp/card.c index a762a41..b00ef10 100644 --- a/drivers/pnp/card.c +++ b/drivers/pnp/card.c @@ -8,6 +8,7 @@ #include <linux/ctype.h> #include <linux/slab.h> #include <linux/pnp.h> +#include <linux/dma-mapping.h> #include "base.h"
LIST_HEAD(pnp_cards); @@ -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);
-- 1.5.2.2
At Sat, 31 May 2008 01:55:52 +0200, Rene Herman wrote:
From 56e4b14aa58612aeb41b51d73a75f85dd72127a1 Mon Sep 17 00:00:00 2001
From: Rene Herman rene.herman@gmail.com Date: Sat, 31 May 2008 00:31:40 +0200 Subject: [PATCH] ISA: set 24-bit dma_mask for ISA devices.
dma_alloc_coherent() on x86 currently takes a passed in NULL device pointer to mean that it should allocate an ISA compatible (24-bit) buffer which is a bit of a hack.
The ALSA ISA drivers are the main consumers of this but have a struct device in fact readily available.
For the legacy drivers, this sets the device dma_mask in preparation for using the actual device with the DMA API so as to eventually not need the NULL hack in dma_alloc_coherent().
This does not fix a current bug -- 2.6.26-rc1 stumbled over the NULL hack in dma_alloc_coherent() but this has already been fixed in commit 4a367f3a9dbf2e7ffcee4702203479809236ee6e by Takashi Iwai.
Signed-off-by: Rene Herman rene.herman@gmail.com Cc: Bjorn Helgaas bjorn.helgaas@hp.com Cc: Takashi Iwai tiwai@suse.de
Acked-by: Takashi Iwai tiwai@suse.de
Takashi
Cc: Alan Cox alan@lxorguk.ukuu.org.uk
drivers/base/isa.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/base/isa.c b/drivers/base/isa.c index d222239..efd5775 100644 --- a/drivers/base/isa.c +++ b/drivers/base/isa.c @@ -7,6 +7,7 @@ #include <linux/slab.h> #include <linux/module.h> #include <linux/init.h> +#include <linux/dma-mapping.h> #include <linux/isa.h>
static struct device isa_bus = { @@ -141,6 +142,9 @@ int isa_register_driver(struct isa_driver *isa_driver, unsigned int ndev) isa_dev->dev.release = isa_dev_release; isa_dev->id = id;
isa_dev->dev.coherent_dma_mask = DMA_24BIT_MASK;isa_dev->dev.dma_mask = &isa_dev->dev.coherent_dma_mask;- error = device_register(&isa_dev->dev); if (error) { put_device(&isa_dev->dev);
-- 1.5.2.2
On Wednesday 14 May 2008 06:46:44 am Rene Herman wrote:
On 14-05-08 01:18, Bjorn Helgaas wrote:
On Tuesday 13 May 2008 11:33:25 am Rene Herman wrote:
No, isa_device is its own thing, on its own isa_bus (*). It has a struct device * readily available though...
(*) drivers/base/isa.c, and explanatory changelog at:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdif...
Thanks for the nice changelog.
isa_register_driver() currently doesn't set a DMA mask. Should it?
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)
Inlining is whitespace-failing here. Patch itself is trivial...
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.
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?
Yes, I like this patch.
participants (7)
-
Alan Cox -
Andrew Morton -
Bjorn Helgaas -
Ingo Molnar -
Pete Clements -
Rene Herman -
Takashi Iwai