Re: [PATCH] sym53c8xx_2: Avoid calling __sym_mfree with irqs disabled

Re: [PATCH] sym53c8xx_2: Avoid calling __sym_mfree with irqs disabled

[James Bottomley]

On Thu, 2010-03-25 at 18:05 +0100, Stefan Bader wrote:
> The following patch was tested and seemed to avoid the warning. Howver
> I am not completely sure that none of the later functions need to be
> protected with spinlocks. Though it feels ok. But maybe someone can do
> some sanity checking.

So, I'm afraid you're right, the patch as is won't work ... the problem
is that __sym_mfree_dma does list manipulation and for safety that has
to be under a lock.

The inception of this problem is that ARM needs a sleeping function on
DMA free but nothing else does so, on every platform that sym2 is
supported, this warning is bogus.

One way of getting rid of it might be to undef SYM_MEM_FREE_UNUSED which
will prevent the free routines actually from releasing memory ...
another might be to drop the lock and reacquire it around the free ...
but that's sitting in a list traversal function so it may expose us to
list races again, so really the whole of the freeing routines would have
to be re-written to be list safe under lock ...

James

> Thanks,
> Stefan
> 
> From d8c94332b8c7422c9c3e20d236f7ba6f59170408 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> Date: Wed, 24 Mar 2010 17:06:05 +0000
> Subject: [PATCH] sym53c8xx_2: Avoid calling __sym_mfree with irqs disabled
> 
> BugLink: http://bugs.launchpad.net/bugs/458201
> 
> Testing disk hotswap in kvm produced the following warning when removing
> an inactive drive from the vm. Looking at the code it is __sym_mfree_dma,
> that disables irqs and then calls __sym_mfree which at some point ends up
> at code which checks for irqs disabled.
> 
> WARNING: at /build/buildd/linux-2.6.32/arch/x86/include/asm/dma-mapping.h:154 ___free_dma_mem_cluster+0x102/0x110()
> Hardware name: Bochs
> Pid: 17, comm: kacpi_notify Not tainted 2.6.32-16-server #25-Ubuntu
> Call Trace:
>  [<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
>  [<ffffffff8138fa96>] sym_free_resources+0x46/0x90
>  [<ffffffff8138fb93>] sym_detach+0xb3/0xe0
>  [<ffffffff8138fbfb>] sym2_remove+0x3b/0x60
> 
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> CC: stable@kernel.org
> ---
>  drivers/scsi/sym53c8xx_2/sym_malloc.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/sym53c8xx_2/sym_malloc.c b/drivers/scsi/sym53c8xx_2/sym_malloc.c
> index 883cac1..006a079 100644
> --- a/drivers/scsi/sym53c8xx_2/sym_malloc.c
> +++ b/drivers/scsi/sym53c8xx_2/sym_malloc.c
> @@ -339,15 +339,14 @@ void __sym_mfree_dma(m_pool_ident_t dev_dmat, void *m, int size, char *name)
>  
>  	spin_lock_irqsave(&sym53c8xx_lock, flags);
>  	mp = ___get_dma_pool(dev_dmat);
> +	spin_unlock_irqrestore(&sym53c8xx_lock, flags);
>  	if (!mp)
> -		goto out;
> +		return;
>  	__sym_mfree(mp, m, size, name);
>  #ifdef	SYM_MEM_FREE_UNUSED
>  	if (!mp->nump)
>  		___del_dma_pool(mp);
>  #endif
> - out:
> -	spin_unlock_irqrestore(&sym53c8xx_lock, flags);
>  }
>  
>  /*

Re: [PATCH] sym53c8xx_2: Avoid calling __sym_mfree with irqs disabled

[Matthew Wilcox]

On Thu, Mar 25, 2010 at 11:21:17AM -0600, James Bottomley wrote:
> On Thu, 2010-03-25 at 18:05 +0100, Stefan Bader wrote:
> > The following patch was tested and seemed to avoid the warning. Howver
> > I am not completely sure that none of the later functions need to be
> > protected with spinlocks. Though it feels ok. But maybe someone can do
> > some sanity checking.
> 
> So, I'm afraid you're right, the patch as is won't work ... the problem
> is that __sym_mfree_dma does list manipulation and for safety that has
> to be under a lock.
> 
> The inception of this problem is that ARM needs a sleeping function on
> DMA free but nothing else does so, on every platform that sym2 is
> supported, this warning is bogus.

Oh good, I'm glad you wrote this, it saves me the typing ;-)

> One way of getting rid of it might be to undef SYM_MEM_FREE_UNUSED which
> will prevent the free routines actually from releasing memory ...
> another might be to drop the lock and reacquire it around the free ...
> but that's sitting in a list traversal function so it may expose us to
> list races again, so really the whole of the freeing routines would have
> to be re-written to be list safe under lock ...

The solution I'd suggest is to take out the WARN_ON in the x86 code.
It's never going to cause trouble on x86.

The real solution is to use DMA pools, but my supply of tuits is quite
limited these days, and converting sym2 to modern APIs isn't high on
my priority list.

-- 
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] sym53c8xx_2: Avoid calling __sym_mfree with irqs disabled

[Stefan Bader]

Matthew Wilcox wrote:
> On Thu, Mar 25, 2010 at 11:21:17AM -0600, James Bottomley wrote:
>> On Thu, 2010-03-25 at 18:05 +0100, Stefan Bader wrote:
>>> The following patch was tested and seemed to avoid the warning. Howver
>>> I am not completely sure that none of the later functions need to be
>>> protected with spinlocks. Though it feels ok. But maybe someone can do
>>> some sanity checking.
>> So, I'm afraid you're right, the patch as is won't work ... the problem
>> is that __sym_mfree_dma does list manipulation and for safety that has
>> to be under a lock.
>>
>> The inception of this problem is that ARM needs a sleeping function on
>> DMA free but nothing else does so, on every platform that sym2 is
>> supported, this warning is bogus.
> 
> Oh good, I'm glad you wrote this, it saves me the typing ;-)
> 
>> One way of getting rid of it might be to undef SYM_MEM_FREE_UNUSED which
>> will prevent the free routines actually from releasing memory ...
>> another might be to drop the lock and reacquire it around the free ...
>> but that's sitting in a list traversal function so it may expose us to
>> list races again, so really the whole of the freeing routines would have
>> to be re-written to be list safe under lock ...
> 
> The solution I'd suggest is to take out the WARN_ON in the x86 code.
> It's never going to cause trouble on x86.

Ok, thanks for the infos. Sounds like this is the quickest solution to a problem
which is not a real one for x86. Maybe limit it to just ARM if that's the only
architecture that has problems with it.

Stefan

> The real solution is to use DMA pools, but my supply of tuits is quite
> limited these days, and converting sym2 to modern APIs isn't high on
> my priority list.
>