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

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

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