[PATCH] dma-mapping: Remove WARN_ON in dma_free_coherent

[PATCH] dma-mapping: Remove WARN_ON in dma_free_coherent

[Stefan Bader]

BugLink: http://bugs.launchpad.net/ubuntu/bugs/458201

Triggered by the following backtrace I was looking at the sym53c8xx_2
driver and in there whether it would be possible to avoid the WARN_ON.

WARNING: at
/build/buildd/linux-2.6.32/arch/x86/include/asm/dma-mapping.h:154
___free_dma_mem_cluster+0x102/0x110()

 [<ffffffff81064f9b>] warn_slowpath_common+0x7b/0xc0
 [<ffffffff81064ff4>] warn_slowpath_null+0x14/0x20
 [<ffffffff8139a2a2>] ___free_dma_mem_cluster+0x102/0x110
 [<ffffffff8139a072>] __sym_mfree+0xd2/0x100
 [<ffffffff8139a109>] __sym_mfree_dma+0x69/0x100
 [<ffffffff8139245f>] sym_hcb_free+0x8f/0x1f0

But it seems modifying that driver would be a bigger effort and maybe
not really worthwile as it is a rather old driver. On the other side
I was told [1] that this WARN_ON applies only to ARM which could sleep
in the free path and it is just bogus for X86. As the code is in
arch/x86 it would never be used for anything else than X86 and if
its truely bogus on X86, couldn't it get removed?

[1] http://kerneltrap.org/mailarchive/linux-scsi/2010/3/25/6886023

Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 arch/x86/include/asm/dma-mapping.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h
index 6a25d5d..2d6097f 100644
--- a/arch/x86/include/asm/dma-mapping.h
+++ b/arch/x86/include/asm/dma-mapping.h
@@ -151,8 +151,6 @@ static inline void dma_free_coherent(struct device *dev, size_t size,
 {
 	struct dma_map_ops *ops = get_dma_ops(dev);
 
-	WARN_ON(irqs_disabled());       /* for portability */
-
 	if (dma_release_from_coherent(dev, get_order(size), vaddr))
 		return;
 
-- 
1.6.3.3

Re: [PATCH] dma-mapping: Remove WARN_ON in dma_free_coherent

[Matthew Wilcox]

On Mon, Mar 29, 2010 at 08:09:21PM +0200, Stefan Bader wrote:
> But it seems modifying that driver would be a bigger effort and maybe
> not really worthwile as it is a rather old driver. On the other side
> I was told [1] that this WARN_ON applies only to ARM which could sleep
> in the free path and it is just bogus for X86. As the code is in
> arch/x86 it would never be used for anything else than X86 and if
> its truely bogus on X86, couldn't it get removed?

My suggestion was that you remove it from the Ubuntu kernel, not submit
it upstream.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] dma-mapping: Remove WARN_ON in dma_free_coherent

[Stefan Bader]

Matthew Wilcox wrote:
> On Mon, Mar 29, 2010 at 08:09:21PM +0200, Stefan Bader wrote:
>> But it seems modifying that driver would be a bigger effort and maybe
>> not really worthwile as it is a rather old driver. On the other side
>> I was told [1] that this WARN_ON applies only to ARM which could sleep
>> in the free path and it is just bogus for X86. As the code is in
>> arch/x86 it would never be used for anything else than X86 and if
>> its truely bogus on X86, couldn't it get removed?
> 
> My suggestion was that you remove it from the Ubuntu kernel, not submit
> it upstream.
> 
No, I did not want to give that impression. But my rational was, that if this is
 really a warning that has no meaning on X86, then why should it stay in code
that is x86 only. And then why should that not go upstream.
When I tried following the code, it looked like this was carried forward for
quite a while and maybe it just stayed because nobody noticed it.
But with KVM using that scsi driver, other distros might see the same bogus
warning and then it would be beneficial for all to just remove it.
If Ingo or hpa say it is needed, ok. But its at least worth a go.

-Stefan

Re: [PATCH] dma-mapping: Remove WARN_ON in dma_free_coherent

[H. Peter Anvin]

On 03/29/2010 01:50 PM, Stefan Bader wrote:
> Matthew Wilcox wrote:
>> On Mon, Mar 29, 2010 at 08:09:21PM +0200, Stefan Bader wrote:
>>> But it seems modifying that driver would be a bigger effort and maybe
>>> not really worthwile as it is a rather old driver. On the other side
>>> I was told [1] that this WARN_ON applies only to ARM which could sleep
>>> in the free path and it is just bogus for X86. As the code is in
>>> arch/x86 it would never be used for anything else than X86 and if
>>> its truely bogus on X86, couldn't it get removed?
>>
>> My suggestion was that you remove it from the Ubuntu kernel, not submit
>> it upstream.
>>
> No, I did not want to give that impression. But my rational was, that if this is
>  really a warning that has no meaning on X86, then why should it stay in code
> that is x86 only. And then why should that not go upstream.
> When I tried following the code, it looked like this was carried forward for
> quite a while and maybe it just stayed because nobody noticed it.
> But with KVM using that scsi driver, other distros might see the same bogus
> warning and then it would be beneficial for all to just remove it.
> If Ingo or hpa say it is needed, ok. But its at least worth a go.
> 


-	WARN_ON(irqs_disabled());       /* for portability */
                                           ^^^^^^^^^^^^^^^

It's there to keep people from writing their drivers incorrectly,
testing them on x86 only, and then pushing them upstream.

Nacked-by: H. Peter Anvin <hpa@zytor.com>

	-hpa