2.6.26-rc1 regression: ISA DMA broken (bisected)

2.6.26-rc1 regression: ISA DMA broken (bisected)

[Rene Herman]

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.

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Takashi Iwai]

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
 

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Ingo Molnar]

* 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

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Ingo Molnar]

* 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
 

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Jesse Barnes]

On Friday, May 09, 2008 1:58 am Ingo Molnar wrote:
> * Ingo Molnar <mingo@elte.hu> wrote:
> > good catch! Queued it up for testing. Jesse, do you concur?
>
> here's the patch below, tidied up.

Yeah, that looks like a clear problem. :)

> ------------------------------->
> 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.

Patch looks good, applied to my 'for-linus' tree.

Thanks,
Jesse

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[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.

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Ingo Molnar]

* 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

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Rene Herman]

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-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.

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Ingo Molnar]

* 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-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.

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

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Rene Herman]

On 13-05-08 16:36, Ingo Molnar wrote:

> 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.

Many thanks. Had in fact failed to notice --edit and will definitely 
check out the interactive rebase. Sounds very useful.

Rene.

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Pete Clements]

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 

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Glauber Costa]

Takashi Iwai wrote:
> 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>

Woke up and saw it, got worried, but there is already a fix. ;-)

Nice work! And thank you

> ---
> 
> 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
>  

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Bjorn Helgaas]

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
>  

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Alan Cox]

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 ?

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Rene Herman]

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=commitdiff;h=a5117ba7da37deb09df5eb802dace229b3fb1e9f

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Bjorn Helgaas]

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=commitdiff;h=a5117ba7da37deb09df5eb802dace229b3fb1e9f

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

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Takashi Iwai]

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=commitdiff;h=a5117ba7da37deb09df5eb802dace229b3fb1e9f
> 
> 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

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

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=commitdiff;h=a5117ba7da37deb09df5eb802dace229b3fb1e9f
> 
> 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.

[-- Attachment #2: isa_dev_dma_mask.diff --]
[-- Type: text/plain, Size: 1461 bytes --]

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) {
-- 
1.5.2.2

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Takashi Iwai]

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=commitdiff;h=a5117ba7da37deb09df5eb802dace229b3fb1e9f
> > 
> > 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

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1530 bytes --]

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.

[-- Attachment #2: sound_isa_use_dma_dev.diff --]
[-- Type: text/plain, Size: 17066 bytes --]

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;

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Takashi Iwai]

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

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Rene Herman]

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);

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Bjorn Helgaas]

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);
> 
> 

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Rene Herman]

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.

[PATCH] Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

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.

[-- Attachment #2: 0001-PNP-set-the-pnp_card-dma_mask-for-use-by-ISAPnP-car.patch --]
[-- Type: text/plain, Size: 1126 bytes --]

>From b4e8e6b833d2e8ed3a2b78912c4afff563d8ae21 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.

This makes the pnp_card->dev available as a DMAable device same as
the pnp_dev->dev.

Signed-off-by: Rene Herman <rene.herman@gmail.com>
---
 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

[DEVICE MODEL] dev->dma_mask

[Rene Herman]

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.

Re: [PATCH] Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Bjorn Helgaas]

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>

Re: [PATCH] Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Rene Herman]

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.

[PATCH] ISA: set 24-bit dma_mask for ISA devices.

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 190 bytes --]

On 31-05-08 00:11, Rene Herman wrote:

[ PNP: set the pnp_card dma_mask for use by ISAPnP cards. ]

> Thanks. I'll assume Andrew picks it up from the CC...

And here's the ISA one...

Rene.

[-- Attachment #2: 0001-ISA-set-24-bit-dma_mask-for-ISA-devices.patch --]
[-- Type: text/plain, Size: 1138 bytes --]

>From 35f30388ad8536860719b3b44ea1f232b95ba42b 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.

Set the ISA device dma_mask in preparation for using the actual device
with the DMA API.

Signed-off-by: Rene Herman <rene.herman@gmail.com>
---
 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

Re: [PATCH] ISA: set 24-bit dma_mask for ISA devices.

[Andrew Morton]

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

Re: [PATCH] ISA: set 24-bit dma_mask for ISA devices.

[Rene Herman]

On 31-05-08 00:55, Andrew Morton wrote:

> Argh.  Could we please be better with the changelogs?

If you already knew what it was about these were nicely concise and to 
the point...

Yaya. I'll resend.

Rene.

[PATCH] PNP: set the pnp_card dma_mask for use by ISAPnP cards

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 2 bytes --]




[-- Attachment #2: 0001-PNP-set-the-pnp_card-dma_mask-for-use-by-ISAPnP-c.patch --]
[-- Type: text/plain, Size: 1864 bytes --]

>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>
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

Re: [PATCH] PNP: set the pnp_card dma_mask for use by ISAPnP cards

[Takashi Iwai]

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
> 

[PATCH] ISA: set 24-bit dma_mask for ISA devices.

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: 0002-ISA-set-24-bit-dma_mask-for-ISA-devices.patch --]
[-- Type: text/plain, Size: 1818 bytes --]

>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>
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

Re: [PATCH] ISA: set 24-bit dma_mask for ISA devices.

[Takashi Iwai]

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
> 

Re: 2.6.26-rc1 regression: ISA DMA broken (bisected)

[Bjorn Helgaas]

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=commitdiff;h=a5117ba7da37deb09df5eb802dace229b3fb1e9f
> > 
> > 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.