Question about ASOC codec drivers

Question about ASOC codec drivers

[Neil Jones]

Hi,

Im trying to write an ASOC Codec driver for a new codec and im having
problems setting up the widgets and interconnects.

The input structure is similar to the wm8731 so i have based my codec
on this (my widget code is included at the bottom)

problem is I get this error on startup:

Failed to add route Line Input->Input Mux

Add then when I run alsa-mixer I get a memfault in the strcpy in
snd_soc_info_enum_double()

it appears to be indexing a string 1 past the end, ie
uinfo->value.enumerated.item = 3 but e->texts is pointing to the
tansen_input_select[] array of strings which only has 3 entries, so it
blows up (e->max = 7??).

also shouldn't the strcpy by a strncpy as we are copying to a fixed
64byte buffer ?

thanks for anyhelp.

Neil



my widget code:

static const struct snd_kcontrol_new tansen_snd_controls[] = {
SOC_DOUBLE_R("Master Playback Volume",
	     TANSEN_TM_OP_TOP7, TANSEN_TM_OP_TOP8,
	     0, 0xFF, 0
	     ),
SOC_DOUBLE("Line/IPOD-In Gain", TANSEN_TM_IP_TOP5, 5, 2, 0x7, 0),
SOC_DOUBLE("Mic-In Gain", TANSEN_TM_IP_TOP4, 0, 4, 0xF, 0),
};

static const char *tansen_input_select[] = { "Mic In",
					    "Line In",
					    "IPOD In",
					  };

static const struct soc_enum tansen_ip_enum =
SOC_ENUM_SINGLE(TANSEN_TM_IP_TOP2, 4, 0x7, tansen_input_select);

/* Input mux */
static const struct snd_kcontrol_new tansen_input_mux_controls =
SOC_DAPM_ENUM("Input Select", tansen_ip_enum);

static const struct snd_soc_dapm_widget tansen_dapm_widgets[] = {
SND_SOC_DAPM_DAC("DAC", "Playback", SND_SOC_NOPM, 0, 0),
SND_SOC_DAPM_OUTPUT("LHPOUT"),
SND_SOC_DAPM_OUTPUT("RHPOUT"),
SND_SOC_DAPM_ADC("ADC", "Capture", SND_SOC_NOPM, 0, 0),
SND_SOC_DAPM_MUX("Input Mux", SND_SOC_NOPM, 0, 0, &tansen_input_mux_controls),
SND_SOC_DAPM_PGA("PGA", SND_SOC_NOPM, 0, 0, NULL, 0),
SND_SOC_DAPM_MICBIAS("Mic Bias", TANSEN_TM_MICBIAS_1, 4, 1),
SND_SOC_DAPM_INPUT("MICIN"),
SND_SOC_DAPM_INPUT("RLINEIN"),
SND_SOC_DAPM_INPUT("LLINEIN"),
SND_SOC_DAPM_INPUT("RIPODIN"),
SND_SOC_DAPM_INPUT("LIPODIN")
};

static const struct snd_soc_dapm_route intercon[] = {

	/* outputs */
	{"RHPOUT", NULL, "DAC"},
	{"LHPOUT", NULL, "DAC"},

	/* input mux */
	{"Input Mux", "Line In", "Line Input"},
	{"Input Mux", "Mic In",  "Mic bias"},
	{"Input Mux", "Ipod In", "Ipod Input"},
	{"ADC", NULL, "PGA"},
	{"PGA", NULL, "Input Mux"},

	/* inputs */
	{"Line Input", NULL, "LLINEIN"},
	{"Line Input", NULL, "RLINEIN"},
	{"Ipod Input", NULL, "LIPODIN"},
	{"Ipod Input", NULL, "RIPODIN"},
	{"Mic bias", NULL, "MICIN"},
};

Re: Question about ASOC codec drivers

[Mark Brown]

On Tue, Jan 18, 2011 at 05:35:22PM +0000, Neil Jones wrote:

> tansen_input_select[] array of strings which only has 3 entries, so it
> blows up (e->max = 7??).

This...

> also shouldn't the strcpy by a strncpy as we are copying to a fixed
> 64byte buffer ?

There's rather a lot of strcpy() calls in the kernel; perhaps you could
be more specific?  Ideally if you think there's a change that should be
made please post a patch.

> static const struct soc_enum tansen_ip_enum =
> SOC_ENUM_SINGLE(TANSEN_TM_IP_TOP2, 4, 0x7, tansen_input_select);

...is exactly what you told the driver to do.  You've told the core
there are seven items in the enumeration so it's expecting an array of
seven strings.

Re: Question about ASOC codec drivers

[Neil Jones]

> ...is exactly what you told the driver to do.  You've told the core
> there are seven items in the enumeration so it's expecting an array of
> seven strings.

DUH! sorry, in my head i read .xmax as .xmask cheers for the spot.

I still get 'Failed to add route Line Input->Input Mux' though ?

> Ideally if you think there's a change that should be
> made please post a patch.

From: Neil Jones <neiljay@gmail.com>
Date: Thu, 20 Jan 2011 15:59:38 +0000
Subject: [PATCH] sound/soc: change potentially risky strcpy to strncpy.

Signed-off-by: Neil Jones <neiljay@gmail.com>
---
 include/sound/asound.h |   11 +++++++----
 sound/soc/soc-core.c   |    5 +++--
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/sound/asound.h b/include/sound/asound.h
index a1803ec..f694604 100644
--- a/include/sound/asound.h
+++ b/include/sound/asound.h
@@ -761,6 +761,8 @@ typedef int __bitwise snd_ctl_elem_iface_t;
 #define SNDRV_CTL_POWER_D3hot          (SNDRV_CTL_POWER_D3|0x0000)
 /* Off, with power */
 #define SNDRV_CTL_POWER_D3cold         (SNDRV_CTL_POWER_D3|0x0001)
 /* Off, without power */

+#define SNDRV_CTL_ELEMENT_INFO_NAME_LENGTH 64
+
 struct snd_ctl_elem_id {
        unsigned int numid;             /* numeric identifier, zero = invalid */
        snd_ctl_elem_iface_t iface;     /* interface identifier */
@@ -799,7 +801,8 @@ struct snd_ctl_elem_info {
                struct {
                        unsigned int items;     /* R: number of items */
                        unsigned int item;      /* W: item number */
-                       char name[64];          /* R: value name */
+                       /* R: value name */
+                       char name[SNDRV_CTL_ELEMENT_INFO_NAME_LENGTH];
                } enumerated;
                unsigned char reserved[128];
        } value;
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 85b7d54..3ad3361 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2056,8 +2056,9 @@ int snd_soc_info_enum_double(struct snd_kcontrol
*kcontrol,

        if (uinfo->value.enumerated.item > e->max - 1)
                uinfo->value.enumerated.item = e->max - 1;
-       strcpy(uinfo->value.enumerated.name,
-               e->texts[uinfo->value.enumerated.item]);
+       strncpy(uinfo->value.enumerated.name,
+               e->texts[uinfo->value.enumerated.item],
+               SNDRV_CTL_ELEMENT_INFO_NAME_LENGTH);
        return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_info_enum_double);
--
1.5.5.2



On Tue, Jan 18, 2011 at 7:19 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Jan 18, 2011 at 05:35:22PM +0000, Neil Jones wrote:
>
>> tansen_input_select[] array of strings which only has 3 entries, so it
>> blows up (e->max = 7??).
>
> This...
>
>> also shouldn't the strcpy by a strncpy as we are copying to a fixed
>> 64byte buffer ?
>
> There's rather a lot of strcpy() calls in the kernel; perhaps you could
> be more specific?  Ideally if you think there's a change that should be
> made please post a patch.
>
>> static const struct soc_enum tansen_ip_enum =
>> SOC_ENUM_SINGLE(TANSEN_TM_IP_TOP2, 4, 0x7, tansen_input_select);
>
> ...is exactly what you told the driver to do.  You've told the core
> there are seven items in the enumeration so it's expecting an array of
> seven strings.
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Question about ASOC codec drivers

[Mark Brown]

On Thu, Jan 20, 2011 at 04:12:12PM +0000, Neil Jones wrote:
> > ...is exactly what you told the driver to do.  You've told the core
> > there are seven items in the enumeration so it's expecting an array of
> > seven strings.

> DUH! sorry, in my head i read .xmax as .xmask cheers for the spot.

> I still get 'Failed to add route Line Input->Input Mux' though ?

You've probably typoed one of the strings.  Look at the code to see why
it's detecting this error.

> @@ -761,6 +761,8 @@ typedef int __bitwise snd_ctl_elem_iface_t;
>  #define SNDRV_CTL_POWER_D3hot          (SNDRV_CTL_POWER_D3|0x0000)
>  /* Off, with power */
>  #define SNDRV_CTL_POWER_D3cold         (SNDRV_CTL_POWER_D3|0x0001)
>  /* Off, without power */
> 
> +#define SNDRV_CTL_ELEMENT_INFO_NAME_LENGTH 64
> +
>  struct snd_ctl_elem_id {
>         unsigned int numid;             /* numeric identifier, zero = invalid */
>         snd_ctl_elem_iface_t iface;     /* interface identifier */
> @@ -799,7 +801,8 @@ struct snd_ctl_elem_info {
>                 struct {
>                         unsigned int items;     /* R: number of items */
>                         unsigned int item;      /* W: item number */
> -                       char name[64];          /* R: value name */
> +                       /* R: value name */
> +                       char name[SNDRV_CTL_ELEMENT_INFO_NAME_LENGTH];
>                 } enumerated;
>                 unsigned char reserved[128];
>         } value;

This I'd submit separately to Takashi.

> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -2056,8 +2056,9 @@ int snd_soc_info_enum_double(struct snd_kcontrol
> *kcontrol,
> 
>         if (uinfo->value.enumerated.item > e->max - 1)
>                 uinfo->value.enumerated.item = e->max - 1;
> -       strcpy(uinfo->value.enumerated.name,
> -               e->texts[uinfo->value.enumerated.item]);
> +       strncpy(uinfo->value.enumerated.name,
> +               e->texts[uinfo->value.enumerated.item],
> +               SNDRV_CTL_ELEMENT_INFO_NAME_LENGTH);
>         return 0;

ARRAY_SIZE() would do just as well here but this does look reasonable.