[PATCH] MIPS: Alchemy: fix dbdma ring destruction memory debugcheck.

[PATCH] MIPS: Alchemy: fix dbdma ring destruction memory debugcheck.

[Manuel Lauss]

DBDMA descriptors need to be located at 32-byte aligned addresses;
however kmalloc rarely delivers such addresses.  The dbdma code
works around that by allocating 63 bytes and re-aligning the
descriptor base afterwards.  Hoewever when freeing memory it does
not account for this adjustment and trips the kfree debugcheck:

Kernel bug detected[#1]:
[...]
Call Trace:
[<80186010>] cache_free_debugcheck+0x284/0x318
[<801869d8>] kfree+0xe8/0x2a0
[<8010b31c>] au1xxx_dbdma_chan_free+0x2c/0x7c
[<80388dc8>] au1x_pcm_dbdma_free+0x34/0x4c
[<80388fa8>] au1xpsc_pcm_close+0x28/0x38
[<80383cb8>] soc_codec_close+0x14c/0x1cc
[<8036dbb4>] snd_pcm_release_substream+0x60/0xac
[<8036dc40>] snd_pcm_release+0x40/0xa0
[<8018c7a8>] __fput+0x11c/0x228
[<80188f60>] filp_close+0x7c/0x98
[<80189018>] sys_close+0x9c/0xe4
[<801022a0>] stack_done+0x20/0x3c

Fix this by recording the address delivered by kmalloc() and using
it as parameter to kfree().

Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
---
 arch/mips/alchemy/common/dbdma.c                 |    7 +++++--
 arch/mips/include/asm/mach-au1x00/au1xxx_dbdma.h |    1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/mips/alchemy/common/dbdma.c b/arch/mips/alchemy/common/dbdma.c
index 40071bd..3b2ccc0 100644
--- a/arch/mips/alchemy/common/dbdma.c
+++ b/arch/mips/alchemy/common/dbdma.c
@@ -411,8 +411,11 @@ u32 au1xxx_dbdma_ring_alloc(u32 chanid, int entries)
 		if (desc_base == 0)
 			return 0;
 
+		ctp->cdb_membase = desc_base;
 		desc_base = ALIGN_ADDR(desc_base, sizeof(au1x_ddma_desc_t));
-	}
+	} else
+		ctp->cdb_membase = desc_base;
+
 	dp = (au1x_ddma_desc_t *)desc_base;
 
 	/* Keep track of the base descriptor. */
@@ -829,7 +832,7 @@ void au1xxx_dbdma_chan_free(u32 chanid)
 
 	au1xxx_dbdma_stop(chanid);
 
-	kfree((void *)ctp->chan_desc_base);
+	kfree((void *)ctp->cdb_membase);
 
 	stp->dev_flags &= ~DEV_FLAGS_INUSE;
 	dtp->dev_flags &= ~DEV_FLAGS_INUSE;
diff --git a/arch/mips/include/asm/mach-au1x00/au1xxx_dbdma.h b/arch/mips/include/asm/mach-au1x00/au1xxx_dbdma.h
index c098b45..8c6b110 100644
--- a/arch/mips/include/asm/mach-au1x00/au1xxx_dbdma.h
+++ b/arch/mips/include/asm/mach-au1x00/au1xxx_dbdma.h
@@ -305,6 +305,7 @@ typedef struct dbdma_chan_config {
 	dbdev_tab_t		*chan_dest;
 	au1x_dma_chan_t		*chan_ptr;
 	au1x_ddma_desc_t	*chan_desc_base;
+	u32			cdb_membase; /* kmalloc base of above */
 	au1x_ddma_desc_t	*get_ptr, *put_ptr, *cur_ptr;
 	void			*chan_callparam;
 	void			(*chan_callback)(int, void *);
-- 
1.6.6

Re: [PATCH] MIPS: Alchemy: fix dbdma ring destruction memory debugcheck.

[David Daney]

Manuel Lauss wrote:
> DBDMA descriptors need to be located at 32-byte aligned addresses;
> however kmalloc rarely delivers such addresses.  The dbdma code
> works around that by allocating 63 bytes and re-aligning the
> descriptor base afterwards.  Hoewever when freeing memory it does
> not account for this adjustment and trips the kfree debugcheck:
> 

Correct me if I am wrong, but don't kmalloc et al. return blocks aligned 
   boundaries of the size rounded up the the next power of two?  So if 
you need 32-byte aligned addresses, just use a size value of 32 or 
greater.  You wouldn't have to add 63 and do masking and remember the 
membase value as you do in the patch.

David Daney



> Kernel bug detected[#1]:
> [...]
> Call Trace:
> [<80186010>] cache_free_debugcheck+0x284/0x318
> [<801869d8>] kfree+0xe8/0x2a0
> [<8010b31c>] au1xxx_dbdma_chan_free+0x2c/0x7c
> [<80388dc8>] au1x_pcm_dbdma_free+0x34/0x4c
> [<80388fa8>] au1xpsc_pcm_close+0x28/0x38
> [<80383cb8>] soc_codec_close+0x14c/0x1cc
> [<8036dbb4>] snd_pcm_release_substream+0x60/0xac
> [<8036dc40>] snd_pcm_release+0x40/0xa0
> [<8018c7a8>] __fput+0x11c/0x228
> [<80188f60>] filp_close+0x7c/0x98
> [<80189018>] sys_close+0x9c/0xe4
> [<801022a0>] stack_done+0x20/0x3c
> 
> Fix this by recording the address delivered by kmalloc() and using
> it as parameter to kfree().
> 
> Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
> ---
>  arch/mips/alchemy/common/dbdma.c                 |    7 +++++--
>  arch/mips/include/asm/mach-au1x00/au1xxx_dbdma.h |    1 +
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/mips/alchemy/common/dbdma.c b/arch/mips/alchemy/common/dbdma.c
> index 40071bd..3b2ccc0 100644
> --- a/arch/mips/alchemy/common/dbdma.c
> +++ b/arch/mips/alchemy/common/dbdma.c
> @@ -411,8 +411,11 @@ u32 au1xxx_dbdma_ring_alloc(u32 chanid, int entries)
>  		if (desc_base == 0)
>  			return 0;
>  
> +		ctp->cdb_membase = desc_base;
>  		desc_base = ALIGN_ADDR(desc_base, sizeof(au1x_ddma_desc_t));
> -	}
> +	} else
> +		ctp->cdb_membase = desc_base;
> +
>  	dp = (au1x_ddma_desc_t *)desc_base;
>  
>  	/* Keep track of the base descriptor. */
> @@ -829,7 +832,7 @@ void au1xxx_dbdma_chan_free(u32 chanid)
>  
>  	au1xxx_dbdma_stop(chanid);
>  
> -	kfree((void *)ctp->chan_desc_base);
> +	kfree((void *)ctp->cdb_membase);
>  
>  	stp->dev_flags &= ~DEV_FLAGS_INUSE;
>  	dtp->dev_flags &= ~DEV_FLAGS_INUSE;
> diff --git a/arch/mips/include/asm/mach-au1x00/au1xxx_dbdma.h b/arch/mips/include/asm/mach-au1x00/au1xxx_dbdma.h
> index c098b45..8c6b110 100644
> --- a/arch/mips/include/asm/mach-au1x00/au1xxx_dbdma.h
> +++ b/arch/mips/include/asm/mach-au1x00/au1xxx_dbdma.h
> @@ -305,6 +305,7 @@ typedef struct dbdma_chan_config {
>  	dbdev_tab_t		*chan_dest;
>  	au1x_dma_chan_t		*chan_ptr;
>  	au1x_ddma_desc_t	*chan_desc_base;
> +	u32			cdb_membase; /* kmalloc base of above */
>  	au1x_ddma_desc_t	*get_ptr, *put_ptr, *cur_ptr;
>  	void			*chan_callparam;
>  	void			(*chan_callback)(int, void *);

Re: [PATCH] MIPS: Alchemy: fix dbdma ring destruction memory debugcheck.

[Manuel Lauss]

On Tue, Jan 26, 2010 at 6:44 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> Manuel Lauss wrote:
>>
>> DBDMA descriptors need to be located at 32-byte aligned addresses;
>> however kmalloc rarely delivers such addresses.  The dbdma code
>> works around that by allocating 63 bytes and re-aligning the
>> descriptor base afterwards.  Hoewever when freeing memory it does
>> not account for this adjustment and trips the kfree debugcheck:
>>
>
> Correct me if I am wrong, but don't kmalloc et al. return blocks aligned
>  boundaries of the size rounded up the the next power of two?  So if you
> need 32-byte aligned addresses, just use a size value of 32 or greater.  You
> wouldn't have to add 63 and do masking and remember the membase value as you
> do in the patch.

The description is not completely correct (I suck a writing those):
It allocates a number
of descriptor entries (64 bytes each) specified by the driver in a single block:

        desc_base = (u32)kmalloc(entries * sizeof(au1x_ddma_desc_t),
                                 GFP_KERNEL|GFP_DMA);
        if (desc_base == 0)
                return 0;

        if (desc_base & 0x1f) {

So far the 3 users I have (mmc, spi, audio) always return true on the above
check (2 descriptors for audio for instance).

Manuel

Re: [PATCH] MIPS: Alchemy: fix dbdma ring destruction memory debugcheck.

[Manuel Lauss]

On Tue, Jan 26, 2010 at 6:44 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> Manuel Lauss wrote:
>>
>> DBDMA descriptors need to be located at 32-byte aligned addresses;
>> however kmalloc rarely delivers such addresses.  The dbdma code
>> works around that by allocating 63 bytes and re-aligning the
>> descriptor base afterwards.  Hoewever when freeing memory it does
>> not account for this adjustment and trips the kfree debugcheck:
>>
>
> Correct me if I am wrong, but don't kmalloc et al. return blocks aligned
>  boundaries of the size rounded up the the next power of two?  So if you
> need 32-byte aligned addresses, just use a size value of 32 or greater.  You
> wouldn't have to add 63 and do masking and remember the membase value as you
> do in the patch.

The description is not completely correct (I suck a writing those):
It allocates a number
of descriptor entries (64 bytes each) specified by the driver in a single block:

        desc_base = (u32)kmalloc(entries * sizeof(au1x_ddma_desc_t),
                                 GFP_KERNEL|GFP_DMA);
        if (desc_base == 0)
                return 0;

        if (desc_base & 0x1f) {

So far the 3 users I have (mmc, spi, audio) always return true on the above
check (2 descriptors for audio for instance).

Manuel

Re: [PATCH] MIPS: Alchemy: fix dbdma ring destruction memory debugcheck.

[Manuel Lauss]

David,

On Tue, Jan 26, 2010 at 6:58 PM, Manuel Lauss
<manuel.lauss@googlemail.com> wrote:
> On Tue, Jan 26, 2010 at 6:44 PM, David Daney <ddaney@caviumnetworks.com> wrote:
>> Manuel Lauss wrote:
>>>
>>> DBDMA descriptors need to be located at 32-byte aligned addresses;
>>> however kmalloc rarely delivers such addresses.  The dbdma code
>>> works around that by allocating 63 bytes and re-aligning the
>>> descriptor base afterwards.  Hoewever when freeing memory it does
>>> not account for this adjustment and trips the kfree debugcheck:
>>>
>>
>> Correct me if I am wrong, but don't kmalloc et al. return blocks aligned
>>  boundaries of the size rounded up the the next power of two?  So if you
>> need 32-byte aligned addresses, just use a size value of 32 or greater.  You
>> wouldn't have to add 63 and do masking and remember the membase value as you
>> do in the patch.
>
> The description is not completely correct (I suck a writing those):
> It allocates a number
> of descriptor entries (64 bytes each) specified by the driver in a single block:
>
>        desc_base = (u32)kmalloc(entries * sizeof(au1x_ddma_desc_t),
>                                 GFP_KERNEL|GFP_DMA);
>        if (desc_base == 0)
>                return 0;
>
>        if (desc_base & 0x1f) {
>
> So far the 3 users I have (mmc, spi, audio) always return true on the above
> check (2 descriptors for audio for instance).

... but only if CONFIG_DEBUG_SLAB is enabled.  You are correct in that
kmalloc() returns aligned blocks with a non-debug slab allocator and this
fix becomes superfluous.   I'll try to confirm this with the other
allocators and
resend with a fixed description.

Thank you for the pointer!
     Manuel Lauss

Re: [PATCH] MIPS: Alchemy: fix dbdma ring destruction memory debugcheck.

[Manuel Lauss]

David,

On Tue, Jan 26, 2010 at 6:58 PM, Manuel Lauss
<manuel.lauss@googlemail.com> wrote:
> On Tue, Jan 26, 2010 at 6:44 PM, David Daney <ddaney@caviumnetworks.com> wrote:
>> Manuel Lauss wrote:
>>>
>>> DBDMA descriptors need to be located at 32-byte aligned addresses;
>>> however kmalloc rarely delivers such addresses.  The dbdma code
>>> works around that by allocating 63 bytes and re-aligning the
>>> descriptor base afterwards.  Hoewever when freeing memory it does
>>> not account for this adjustment and trips the kfree debugcheck:
>>>
>>
>> Correct me if I am wrong, but don't kmalloc et al. return blocks aligned
>>  boundaries of the size rounded up the the next power of two?  So if you
>> need 32-byte aligned addresses, just use a size value of 32 or greater.  You
>> wouldn't have to add 63 and do masking and remember the membase value as you
>> do in the patch.
>
> The description is not completely correct (I suck a writing those):
> It allocates a number
> of descriptor entries (64 bytes each) specified by the driver in a single block:
>
>        desc_base = (u32)kmalloc(entries * sizeof(au1x_ddma_desc_t),
>                                 GFP_KERNEL|GFP_DMA);
>        if (desc_base == 0)
>                return 0;
>
>        if (desc_base & 0x1f) {
>
> So far the 3 users I have (mmc, spi, audio) always return true on the above
> check (2 descriptors for audio for instance).

... but only if CONFIG_DEBUG_SLAB is enabled.  You are correct in that
kmalloc() returns aligned blocks with a non-debug slab allocator and this
fix becomes superfluous.   I'll try to confirm this with the other
allocators and
resend with a fixed description.

Thank you for the pointer!
     Manuel Lauss

Re: [PATCH] MIPS: Alchemy: fix dbdma ring destruction memory debugcheck.

[David Daney]

Manuel Lauss wrote:
> David,
> 
> On Tue, Jan 26, 2010 at 6:58 PM, Manuel Lauss
> <manuel.lauss@googlemail.com> wrote:
>> On Tue, Jan 26, 2010 at 6:44 PM, David Daney <ddaney@caviumnetworks.com> wrote:
>>> Manuel Lauss wrote:
>>>> DBDMA descriptors need to be located at 32-byte aligned addresses;
>>>> however kmalloc rarely delivers such addresses.  The dbdma code
>>>> works around that by allocating 63 bytes and re-aligning the
>>>> descriptor base afterwards.  Hoewever when freeing memory it does
>>>> not account for this adjustment and trips the kfree debugcheck:
>>>>
>>> Correct me if I am wrong, but don't kmalloc et al. return blocks aligned
>>>  boundaries of the size rounded up the the next power of two?  So if you
>>> need 32-byte aligned addresses, just use a size value of 32 or greater.  You
>>> wouldn't have to add 63 and do masking and remember the membase value as you
>>> do in the patch.
>> The description is not completely correct (I suck a writing those):
>> It allocates a number
>> of descriptor entries (64 bytes each) specified by the driver in a single block:
>>
>>        desc_base = (u32)kmalloc(entries * sizeof(au1x_ddma_desc_t),
>>                                 GFP_KERNEL|GFP_DMA);
>>        if (desc_base == 0)
>>                return 0;
>>
>>        if (desc_base & 0x1f) {
>>
>> So far the 3 users I have (mmc, spi, audio) always return true on the above
>> check (2 descriptors for audio for instance).
> 
> ... but only if CONFIG_DEBUG_SLAB is enabled.

Oh no!  I make this assumption in my drivers.  If CONFIG_DEBUG_SLAB 
causes it to not be true... Bad news.

David Daney

> You are correct in that
> kmalloc() returns aligned blocks with a non-debug slab allocator and this
> fix becomes superfluous.   I'll try to confirm this with the other
> allocators and
> resend with a fixed description.
> 
> Thank you for the pointer!
>      Manuel Lauss
>