[PATCH] ASoC: soc-core: Add support for NULL default register caches

[PATCH] ASoC: soc-core: Add support for NULL default register caches

[Dimitris Papastamos]

The infrastructure for handling NULL default register maps is already
included in the soc-cache code, just ensure that we don't try to dereference
a NULL pointer while accessing a NULL register cache.

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 sound/soc/soc-core.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index bac7291..a9f19c1 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3503,11 +3503,13 @@ int snd_soc_register_codec(struct device *dev,
 		 * kernel might have freed the array by the time we initialize
 		 * the cache.
 		 */
-		codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default,
-					      reg_size, GFP_KERNEL);
-		if (!codec->reg_def_copy) {
-			ret = -ENOMEM;
-			goto fail;
+		if (codec_drv->reg_cache_default) {
+			codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default,
+						      reg_size, GFP_KERNEL);
+			if (!codec->reg_def_copy) {
+				ret = -ENOMEM;
+				goto fail;
+			}
 		}
 	}
 
-- 
1.7.3.5

Re: [PATCH] ASoC: soc-core: Add support for NULL default register caches

[Liam Girdwood]

On Mon, 2011-01-10 at 10:10 +0000, Dimitris Papastamos wrote:
> The infrastructure for handling NULL default register maps is already
> included in the soc-cache code, just ensure that we don't try to dereference
> a NULL pointer while accessing a NULL register cache.
> 
> Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
> ---
>  sound/soc/soc-core.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index bac7291..a9f19c1 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -3503,11 +3503,13 @@ int snd_soc_register_codec(struct device *dev,
>  		 * kernel might have freed the array by the time we initialize
>  		 * the cache.
>  		 */
> -		codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default,
> -					      reg_size, GFP_KERNEL);
> -		if (!codec->reg_def_copy) {
> -			ret = -ENOMEM;
> -			goto fail;
> +		if (codec_drv->reg_cache_default) {
> +			codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default,
> +						      reg_size, GFP_KERNEL);
> +			if (!codec->reg_def_copy) {
> +				ret = -ENOMEM;
> +				goto fail;
> +			}
>  		}
>  	}
>  

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH] ASoC: soc-core: Add support for NULL default register caches

[Timur Tabi]

Dimitris Papastamos wrote:
> The infrastructure for handling NULL default register maps is already
> included in the soc-cache code, just ensure that we don't try to dereference
> a NULL pointer while accessing a NULL register cache.
> 
> Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>

Acked-by: Timur Tabi <timur@freescale.com>

This fixes the kernel panic I was seeing earlier with my CS4270 driver, but it's
not a replacement for my patch, "[v3] ASoC: cs4270: use the built-in register
cache support".

With Dimitris' patch alone, the CS4270 still has other bugs:

# cat ./devices/platform/soc-audio/playback/codec_reg
cs4270-codec.0-004f registers
 0: <no data: -5>
 1:   c3
 2:    0
 3:   30
 4:    0
 5:    0
 6:    0
 7:    0


So both patches need to be applied, however, I can't say whether Dimitris' patch
is sufficient for handling NULL default register maps, since I removed that from
my driver.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: soc-core: Add support for NULL default register caches

[Mark Brown]

On Mon, Jan 10, 2011 at 10:10:56AM +0000, Dimitris Papastamos wrote:
> The infrastructure for handling NULL default register maps is already
> included in the soc-cache code, just ensure that we don't try to dereference
> a NULL pointer while accessing a NULL register cache.
> 
> Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>

Applied, thanks.

Re: [PATCH] ASoC: soc-core: Add support for NULL default register caches

[Mark Brown]

On Mon, Jan 10, 2011 at 04:10:52PM -0600, Timur Tabi wrote:

> # cat ./devices/platform/soc-audio/playback/codec_reg
> cs4270-codec.0-004f registers
>  0: <no data: -5>
>  1:   c3
>  2:    0
>  3:   30
>  4:    0
>  5:    0
>  6:    0
>  7:    0

> So both patches need to be applied, however, I can't say whether Dimitris' patch
> is sufficient for handling NULL default register maps, since I removed that from
> my driver.

You've not identified what the issue is with the above?  Is it just that
nothing else ensured the cache was set up (eg, by doing reads from the
hardware)?

Re: [PATCH] ASoC: soc-core: Add support for NULL default register caches

[Timur Tabi]

Mark Brown wrote:
> You've not identified what the issue is with the above?  Is it just that
> nothing else ensured the cache was set up (eg, by doing reads from the
> hardware)?

The CS4270 registers are numbered from 1 through 8, so codec_reg should look
like this:

 1:   c3
 2:    0
 3:   30
 4:    0
 5:    0
 6:    0
 7:    0
 8:    0

What I don't know is why the bad output, but it's probably because the register
cache code broke during the various changes applied to it since multi-component
was introduced.  Originally, the driver handled the register cache completely
internally.  Then, as register caching was added to ASoC itself, the driver was
modified to use it.  I believe that those modifications were not really tested.

And since I removed all that code for my patch, I'm not really inclined to go
back and see what specifically was wrong with it.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: soc-core: Add support for NULL default register caches

[Mark Brown]

On Mon, Jan 10, 2011 at 04:40:00PM -0600, Timur Tabi wrote:

> The CS4270 registers are numbered from 1 through 8, so codec_reg should look
> like this:

>  1:   c3
>  2:    0
>  3:   30
>  4:    0
>  5:    0
>  6:    0
>  7:    0
>  8:    0

> What I don't know is why the bad output, but it's probably because the register
> cache code broke during the various changes applied to it since multi-component
> was introduced.  Originally, the driver handled the register cache completely
> internally.  Then, as register caching was added to ASoC itself, the driver was
> modified to use it.  I believe that those modifications were not really tested.

The issue is that you were using 1 based array indexing and the core
uses zero based indexing - nothing dramatically wrong and it should've
fallen through to using the hardware I/O when it went beyond the cache
so I'd expect things to work fine.  The read failure for register zero
was handled gracefully, so it looks like everything did what I'd expect
here.