[PATCH] ASoC: Intel: Skylake: Check the kcontrol against NULL

[PATCH] ASoC: Intel: Skylake: Check the kcontrol against NULL

[Lukasz Majczak]

There is no check for the kcontrol against NULL and in some cases
it causes kernel to crash.

Fixes: 2d744ecf2b984 ("ASoC: Intel: Skylake: Automatic DMIC format configuration according to information from NHLT")
Cc: <stable@vger.kernel.org> # 5.4+
Signed-off-by: Lukasz Majczak <lma@semihalf.com>
---
 sound/soc/intel/skylake/skl-topology.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index ae466cd592922..c9abbe4ff0ba3 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -3618,12 +3618,18 @@ static void skl_tplg_complete(struct snd_soc_component *component)
 	int i;
 
 	list_for_each_entry(dobj, &component->dobj_list, list) {
-		struct snd_kcontrol *kcontrol = dobj->control.kcontrol;
-		struct soc_enum *se =
-			(struct soc_enum *)kcontrol->private_value;
-		char **texts = dobj->control.dtexts;
+		struct snd_kcontrol *kcontrol;
+		struct soc_enum *se;
+		char **texts;
 		char chan_text[4];
 
+		kcontrol = dobj->control.kcontrol;
+		if(!kcontrol)
+			continue;
+
+		se = (struct soc_enum *)kcontrol->private_value;
+		texts = dobj->control.dtexts;
+
 		if (dobj->type != SND_SOC_DOBJ_ENUM ||
 		    dobj->control.kcontrol->put !=
 		    skl_tplg_multi_config_set_dmic)

base-commit: 69fe63aa100220c8fd1f451dd54dd0895df1441d
-- 
2.25.1

Re: [PATCH] ASoC: Intel: Skylake: Check the kcontrol against NULL

[Gorski, Mateusz]

> There is no check for the kcontrol against NULL and in some cases
> it causes kernel to crash.
>
> Fixes: 2d744ecf2b984 ("ASoC: Intel: Skylake: Automatic DMIC format configuration according to information from NHLT")
> Cc: <stable@vger.kernel.org> # 5.4+
> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> ---
>   sound/soc/intel/skylake/skl-topology.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
> index ae466cd592922..c9abbe4ff0ba3 100644
> --- a/sound/soc/intel/skylake/skl-topology.c
> +++ b/sound/soc/intel/skylake/skl-topology.c
> @@ -3618,12 +3618,18 @@ static void skl_tplg_complete(struct snd_soc_component *component)
>   	int i;
>   
>   	list_for_each_entry(dobj, &component->dobj_list, list) {
> -		struct snd_kcontrol *kcontrol = dobj->control.kcontrol;
> -		struct soc_enum *se =
> -			(struct soc_enum *)kcontrol->private_value;
> -		char **texts = dobj->control.dtexts;
> +		struct snd_kcontrol *kcontrol;
> +		struct soc_enum *se;
> +		char **texts;
>   		char chan_text[4];
>   
> +		kcontrol = dobj->control.kcontrol;
> +		if(!kcontrol)
> +			continue;
> +
> +		se = (struct soc_enum *)kcontrol->private_value;
> +		texts = dobj->control.dtexts;
> +
>   		if (dobj->type != SND_SOC_DOBJ_ENUM ||
>   		    dobj->control.kcontrol->put !=
>   		    skl_tplg_multi_config_set_dmic)
>
> base-commit: 69fe63aa100220c8fd1f451dd54dd0895df1441d


Thanks for pointing out and fixing this. This check was obviously 
missing there. I did a quick verification on few of our platforms, 
encountered no issues, so:

     Reviewed-by: Mateusz Gorski <mateusz.gorski@linux.intel.com>


Also, could you please:

- describe the affected configuration (used machine driver or audio card 
name, device name),
- share full dmesg logs from one of said crashes,
- copy the output of "amixer -c0 controls" command executed on affected 
device.

These would be useful information for us to further improve our 
validation and help us with debugging.


Thanks,

Mateusz

Re: [PATCH] ASoC: Intel: Skylake: Check the kcontrol against NULL

[Guenter Roeck]

On Thu, Dec 10, 2020 at 7:55 AM Gorski, Mateusz <
mateusz.gorski@linux.intel.com> wrote:

>
> > There is no check for the kcontrol against NULL and in some cases
> > it causes kernel to crash.
> >
> > Fixes: 2d744ecf2b984 ("ASoC: Intel: Skylake: Automatic DMIC format
> configuration according to information from NHLT")
> > Cc: <stable@vger.kernel.org> # 5.4+
> > Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> > ---
> >   sound/soc/intel/skylake/skl-topology.c | 14 ++++++++++----
> >   1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/intel/skylake/skl-topology.c
> b/sound/soc/intel/skylake/skl-topology.c
> > index ae466cd592922..c9abbe4ff0ba3 100644
> > --- a/sound/soc/intel/skylake/skl-topology.c
> > +++ b/sound/soc/intel/skylake/skl-topology.c
> > @@ -3618,12 +3618,18 @@ static void skl_tplg_complete(struct
> snd_soc_component *component)
> >       int i;
> >
> >       list_for_each_entry(dobj, &component->dobj_list, list) {
> > -             struct snd_kcontrol *kcontrol = dobj->control.kcontrol;
> > -             struct soc_enum *se =
> > -                     (struct soc_enum *)kcontrol->private_value;
> > -             char **texts = dobj->control.dtexts;
> > +             struct snd_kcontrol *kcontrol;
> > +             struct soc_enum *se;
> > +             char **texts;
> >               char chan_text[4];
> >
> > +             kcontrol = dobj->control.kcontrol;
> > +             if(!kcontrol)
> > +                     continue;
> > +
> > +             se = (struct soc_enum *)kcontrol->private_value;
> > +             texts = dobj->control.dtexts;
> > +
> >               if (dobj->type != SND_SOC_DOBJ_ENUM ||
> >                   dobj->control.kcontrol->put !=
> >                   skl_tplg_multi_config_set_dmic)
> >
> > base-commit: 69fe63aa100220c8fd1f451dd54dd0895df1441d
>
>
> Thanks for pointing out and fixing this. This check was obviously
> missing there. I did a quick verification on few of our platforms,
> encountered no issues, so:
>
>      Reviewed-by: Mateusz Gorski <mateusz.gorski@linux.intel.com>
>
>
> Also, could you please:
>
> - describe the affected configuration (used machine driver or audio card
> name, device name),
>

Primarily Google Pixelbook. However, this can happen whenever
dobj->type != SND_SOC_DOBJ_ENUM. For many of those other types,
kcontrol is not set. It is pure luck that the problem is not seen
everywhere,
and it seems to be compiler dependent. [Some compilers or compiler
versions only assign se when needed, ie after the if() statement].

Guenter


> - share full dmesg logs from one of said crashes,
> - copy the output of "amixer -c0 controls" command executed on affected
> device.
>
> These would be useful information for us to further improve our
> validation and help us with debugging.
>
>
> Thanks,
>
> Mateusz
>
>
>

Re: [PATCH] ASoC: Intel: Skylake: Check the kcontrol against NULL

[Amadeusz Sławiński]

On 12/10/2020 1:14 PM, Lukasz Majczak wrote:
> +		kcontrol = dobj->control.kcontrol;
> +		if(!kcontrol)
> +			continue;

Small nitpick, there should be space between if and opening parenthesis 
as recommended by coding style.

Thanks,
Amadeusz

[PATCH v2] ASoC: Intel: Skylake: Check the kcontrol against NULL

[Lukasz Majczak]

There is no check for the kcontrol against NULL and in some cases
it causes kernel to crash.

Fixes: 2d744ecf2b984 ("ASoC: Intel: Skylake: Automatic DMIC format configuration according to information from NHLT")
Cc: <stable@vger.kernel.org> # 5.4+
Signed-off-by: Lukasz Majczak <lma@semihalf.com>
Reviewed-by: Mateusz Gorski <mateusz.gorski@linux.intel.com>
---
 sound/soc/intel/skylake/skl-topology.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
 v1 -> v2: fixed coding style

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index ae466cd592922..8f0bfda7096a9 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -3618,12 +3618,18 @@ static void skl_tplg_complete(struct snd_soc_component *component)
 	int i;
 
 	list_for_each_entry(dobj, &component->dobj_list, list) {
-		struct snd_kcontrol *kcontrol = dobj->control.kcontrol;
-		struct soc_enum *se =
-			(struct soc_enum *)kcontrol->private_value;
-		char **texts = dobj->control.dtexts;
+		struct snd_kcontrol *kcontrol;
+		struct soc_enum *se;
+		char **texts;
 		char chan_text[4];
 
+		kcontrol = dobj->control.kcontrol;
+		if (!kcontrol)
+			continue;
+
+		se = (struct soc_enum *)kcontrol->private_value;
+		texts = dobj->control.dtexts;
+
 		if (dobj->type != SND_SOC_DOBJ_ENUM ||
 		    dobj->control.kcontrol->put !=
 		    skl_tplg_multi_config_set_dmic)
-- 
2.25.1

Re: [PATCH v2] ASoC: Intel: Skylake: Check the kcontrol against NULL

[Łukasz Majczak]

Hi,

This is just a kind reminder. Is there anything more required to
upstream this patch?

Best regards,
Lukasz


czw., 17 gru 2020 o 14:06 Lukasz Majczak <lma@semihalf.com> napisał(a):
>
> There is no check for the kcontrol against NULL and in some cases
> it causes kernel to crash.
>
> Fixes: 2d744ecf2b984 ("ASoC: Intel: Skylake: Automatic DMIC format configuration according to information from NHLT")
> Cc: <stable@vger.kernel.org> # 5.4+
> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> Reviewed-by: Mateusz Gorski <mateusz.gorski@linux.intel.com>
> ---
>  sound/soc/intel/skylake/skl-topology.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>  v1 -> v2: fixed coding style
>
> diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
> index ae466cd592922..8f0bfda7096a9 100644
> --- a/sound/soc/intel/skylake/skl-topology.c
> +++ b/sound/soc/intel/skylake/skl-topology.c
> @@ -3618,12 +3618,18 @@ static void skl_tplg_complete(struct snd_soc_component *component)
>         int i;
>
>         list_for_each_entry(dobj, &component->dobj_list, list) {
> -               struct snd_kcontrol *kcontrol = dobj->control.kcontrol;
> -               struct soc_enum *se =
> -                       (struct soc_enum *)kcontrol->private_value;
> -               char **texts = dobj->control.dtexts;
> +               struct snd_kcontrol *kcontrol;
> +               struct soc_enum *se;
> +               char **texts;
>                 char chan_text[4];
>
> +               kcontrol = dobj->control.kcontrol;
> +               if (!kcontrol)
> +                       continue;
> +
> +               se = (struct soc_enum *)kcontrol->private_value;
> +               texts = dobj->control.dtexts;
> +
>                 if (dobj->type != SND_SOC_DOBJ_ENUM ||
>                     dobj->control.kcontrol->put !=
>                     skl_tplg_multi_config_set_dmic)
> --
> 2.25.1
>

Re: [PATCH v2] ASoC: Intel: Skylake: Check the kcontrol against NULL

[Łukasz Majczak]

Hi Pierre,

Is there anything more to do to get the ACK for this patch?

Best regards,
Lukasz

wt., 12 sty 2021 o 12:34 Łukasz Majczak <lma@semihalf.com> napisał(a):
>
> Hi,
>
> This is just a kind reminder. Is there anything more required to
> upstream this patch?
>
> Best regards,
> Lukasz
>
>
> czw., 17 gru 2020 o 14:06 Lukasz Majczak <lma@semihalf.com> napisał(a):
> >
> > There is no check for the kcontrol against NULL and in some cases
> > it causes kernel to crash.
> >
> > Fixes: 2d744ecf2b984 ("ASoC: Intel: Skylake: Automatic DMIC format configuration according to information from NHLT")
> > Cc: <stable@vger.kernel.org> # 5.4+
> > Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> > Reviewed-by: Mateusz Gorski <mateusz.gorski@linux.intel.com>
> > ---
> >  sound/soc/intel/skylake/skl-topology.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >  v1 -> v2: fixed coding style
> >
> > diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
> > index ae466cd592922..8f0bfda7096a9 100644
> > --- a/sound/soc/intel/skylake/skl-topology.c
> > +++ b/sound/soc/intel/skylake/skl-topology.c
> > @@ -3618,12 +3618,18 @@ static void skl_tplg_complete(struct snd_soc_component *component)
> >         int i;
> >
> >         list_for_each_entry(dobj, &component->dobj_list, list) {
> > -               struct snd_kcontrol *kcontrol = dobj->control.kcontrol;
> > -               struct soc_enum *se =
> > -                       (struct soc_enum *)kcontrol->private_value;
> > -               char **texts = dobj->control.dtexts;
> > +               struct snd_kcontrol *kcontrol;
> > +               struct soc_enum *se;
> > +               char **texts;
> >                 char chan_text[4];
> >
> > +               kcontrol = dobj->control.kcontrol;
> > +               if (!kcontrol)
> > +                       continue;
> > +
> > +               se = (struct soc_enum *)kcontrol->private_value;
> > +               texts = dobj->control.dtexts;
> > +
> >                 if (dobj->type != SND_SOC_DOBJ_ENUM ||
> >                     dobj->control.kcontrol->put !=
> >                     skl_tplg_multi_config_set_dmic)
> > --
> > 2.25.1
> >

Re: [PATCH v2] ASoC: Intel: Skylake: Check the kcontrol against NULL

[Pierre-Louis Bossart]

On 1/20/21 9:49 AM, Łukasz Majczak wrote:
> Hi Pierre,
> 
> Is there anything more to do to get the ACK for this patch?

Adding Cezary and Amadeusz for feedback, I can't pretend having any sort 
of knowledge on the Skylake driver internals and topology usage.

> Best regards,
> Lukasz
> 
> wt., 12 sty 2021 o 12:34 Łukasz Majczak <lma@semihalf.com> napisał(a):
>>
>> Hi,
>>
>> This is just a kind reminder. Is there anything more required to
>> upstream this patch?
>>
>> Best regards,
>> Lukasz
>>
>>
>> czw., 17 gru 2020 o 14:06 Lukasz Majczak <lma@semihalf.com> napisał(a):
>>>
>>> There is no check for the kcontrol against NULL and in some cases
>>> it causes kernel to crash.
>>>
>>> Fixes: 2d744ecf2b984 ("ASoC: Intel: Skylake: Automatic DMIC format configuration according to information from NHLT")
>>> Cc: <stable@vger.kernel.org> # 5.4+
>>> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
>>> Reviewed-by: Mateusz Gorski <mateusz.gorski@linux.intel.com>
>>> ---
>>>   sound/soc/intel/skylake/skl-topology.c | 14 ++++++++++----
>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>>   v1 -> v2: fixed coding style
>>>
>>> diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
>>> index ae466cd592922..8f0bfda7096a9 100644
>>> --- a/sound/soc/intel/skylake/skl-topology.c
>>> +++ b/sound/soc/intel/skylake/skl-topology.c
>>> @@ -3618,12 +3618,18 @@ static void skl_tplg_complete(struct snd_soc_component *component)
>>>          int i;
>>>
>>>          list_for_each_entry(dobj, &component->dobj_list, list) {
>>> -               struct snd_kcontrol *kcontrol = dobj->control.kcontrol;
>>> -               struct soc_enum *se =
>>> -                       (struct soc_enum *)kcontrol->private_value;
>>> -               char **texts = dobj->control.dtexts;
>>> +               struct snd_kcontrol *kcontrol;
>>> +               struct soc_enum *se;
>>> +               char **texts;
>>>                  char chan_text[4];
>>>
>>> +               kcontrol = dobj->control.kcontrol;
>>> +               if (!kcontrol)
>>> +                       continue;
>>> +
>>> +               se = (struct soc_enum *)kcontrol->private_value;
>>> +               texts = dobj->control.dtexts;
>>> +
>>>                  if (dobj->type != SND_SOC_DOBJ_ENUM ||
>>>                      dobj->control.kcontrol->put !=
>>>                      skl_tplg_multi_config_set_dmic)
>>> --
>>> 2.25.1
>>>

RE: [PATCH v2] ASoC: Intel: Skylake: Check the kcontrol against NULL

[Rojewski, Cezary]

On 2021-01-20 5:33 PM, Pierre-Louis Bossart wrote:
> On 1/20/21 9:49 AM, Łukasz Majczak wrote:
>> Hi Pierre,
>>
>> Is there anything more to do to get the ACK for this patch?
> 
> Adding Cezary and Amadeusz for feedback, I can't pretend having any sort 
> of knowledge on the Skylake driver internals and topology usage.
> 

Thanks for the CC, Pierre.

...

>>>>
>>>> diff --git a/sound/soc/intel/skylake/skl-topology.c 
>>>> b/sound/soc/intel/skylake/skl-topology.c
>>>> index ae466cd592922..8f0bfda7096a9 100644
>>>> --- a/sound/soc/intel/skylake/skl-topology.c
>>>> +++ b/sound/soc/intel/skylake/skl-topology.c
>>>> @@ -3618,12 +3618,18 @@ static void skl_tplg_complete(struct 
>>>> snd_soc_component *component)
>>>>          int i;
>>>>
>>>>          list_for_each_entry(dobj, &component->dobj_list, list) {
>>>> -               struct snd_kcontrol *kcontrol = dobj->control.kcontrol;
>>>> -               struct soc_enum *se =
>>>> -                       (struct soc_enum *)kcontrol->private_value;
>>>> -               char **texts = dobj->control.dtexts;
>>>> +               struct snd_kcontrol *kcontrol;
>>>> +               struct soc_enum *se;
>>>> +               char **texts;
>>>>                  char chan_text[4];
>>>>
>>>> +               kcontrol = dobj->control.kcontrol;
>>>> +               if (!kcontrol)
>>>> +                       continue;
>>>> +
>>>> +               se = (struct soc_enum *)kcontrol->private_value;
>>>> +               texts = dobj->control.dtexts;
>>>> +
>>>>                  if (dobj->type != SND_SOC_DOBJ_ENUM ||
>>>>                      dobj->control.kcontrol->put !=
>>>>                      skl_tplg_multi_config_set_dmic)

Just checked the history behind this. And must say, I liked Ricardo's
version better. Except for the "= {};" bit which Mark already pointed
out - it should be a separate fix - it's simply more optional

e.g.: 'kcontrol' gets assigned yet 'if' above is not updated accordingly:
s/dobj->control.kcontrol->put/kcontrol->put

Czarek

RE: [PATCH v2] ASoC: Intel: Skylake: Check the kcontrol against NULL

[Rojewski, Cezary]

On 2021-01-20 5:41 PM, Rojewski, Cezary wrote:
> 
> Just checked the history behind this. And must say, I liked Ricardo's
> version better. Except for the "= {};" bit which Mark already pointed
> out - it should be a separate fix - it's simply more optional

Meant to say: optimal.