[bug report] ASoC: Intel: cht_bsw_rt5645: Set card.components string

[bug report] ASoC: Intel: cht_bsw_rt5645: Set card.components string

[Dan Carpenter]

Hello Hans de Goede,

Commit f87b4402163b ("ASoC: Intel: cht_bsw_rt5645: Set
card.components string") from Nov 26, 2023 (linux-next), leads to the
following Smatch static checker warning:

	sound/soc/intel/boards/cht_bsw_rt5645.c:587 snd_cht_mc_probe()
	error: we previously assumed 'adev' could be null (see line 581)

sound/soc/intel/boards/cht_bsw_rt5645.c
    570         /* set correct codec name */
    571         for (i = 0; i < ARRAY_SIZE(cht_dailink); i++)
    572                 if (cht_dailink[i].codecs->name &&
    573                     !strcmp(cht_dailink[i].codecs->name,
    574                             "i2c-10EC5645:00")) {
    575                         dai_index = i;
    576                         break;
    577                 }
    578 
    579         /* fixup codec name based on HID */
    580         adev = acpi_dev_get_first_match_dev(mach->id, NULL, -1);
    581         if (adev) {
                    ^^^^
The old code assumes adev can be NULL

    582                 snprintf(cht_rt5645_codec_name, sizeof(cht_rt5645_codec_name),
    583                          "i2c-%s", acpi_dev_name(adev));
    584                 cht_dailink[dai_index].codecs->name = cht_rt5645_codec_name;
    585         }
    586         /* acpi_get_first_physical_node() returns a borrowed ref, no need to deref */
--> 587         codec_dev = acpi_get_first_physical_node(adev);
                                                         ^^^^
Unchecked dereference

    588         acpi_dev_put(adev);
    589         if (!codec_dev)
    590                 return -EPROBE_DEFER;
    591 
    592         snd_soc_card_chtrt5645.components = rt5645_components(codec_dev);
    593         snd_soc_card_chtrt5650.components = rt5645_components(codec_dev);
    594 

regards,
dan carpenter

Re: [bug report] ASoC: Intel: cht_bsw_rt5645: Set card.components string

[Pierre-Louis Bossart]

Thanks Dan for the report.


> Commit f87b4402163b ("ASoC: Intel: cht_bsw_rt5645: Set
> card.components string") from Nov 26, 2023 (linux-next), leads to the
> following Smatch static checker warning:
> 
> 	sound/soc/intel/boards/cht_bsw_rt5645.c:587 snd_cht_mc_probe()
> 	error: we previously assumed 'adev' could be null (see line 581)
> 
> sound/soc/intel/boards/cht_bsw_rt5645.c
>     570         /* set correct codec name */
>     571         for (i = 0; i < ARRAY_SIZE(cht_dailink); i++)
>     572                 if (cht_dailink[i].codecs->name &&
>     573                     !strcmp(cht_dailink[i].codecs->name,
>     574                             "i2c-10EC5645:00")) {
>     575                         dai_index = i;
>     576                         break;
>     577                 }
>     578 
>     579         /* fixup codec name based on HID */
>     580         adev = acpi_dev_get_first_match_dev(mach->id, NULL, -1);
>     581         if (adev) {
>                     ^^^^
> The old code assumes adev can be NULL
> 
>     582                 snprintf(cht_rt5645_codec_name, sizeof(cht_rt5645_codec_name),
>     583                          "i2c-%s", acpi_dev_name(adev));
>     584                 cht_dailink[dai_index].codecs->name = cht_rt5645_codec_name;
>     585         }
>     586         /* acpi_get_first_physical_node() returns a borrowed ref, no need to deref */
> --> 587         codec_dev = acpi_get_first_physical_node(adev);
>                                                          ^^^^
> Unchecked dereference

This looks like a problem in multiple machine drivers sharing similar
code, if we want to consistently check the output we probably need
something like https://github.com/thesofproject/linux/pull/5117 ?

Re: [bug report] ASoC: Intel: cht_bsw_rt5645: Set card.components string

[Dan Carpenter]

On Mon, Jul 22, 2024 at 04:06:58PM +0200, Pierre-Louis Bossart wrote:
> Thanks Dan for the report.
> 
> 
> > Commit f87b4402163b ("ASoC: Intel: cht_bsw_rt5645: Set
> > card.components string") from Nov 26, 2023 (linux-next), leads to the
> > following Smatch static checker warning:
> > 
> > 	sound/soc/intel/boards/cht_bsw_rt5645.c:587 snd_cht_mc_probe()
> > 	error: we previously assumed 'adev' could be null (see line 581)
> > 
> > sound/soc/intel/boards/cht_bsw_rt5645.c
> >     570         /* set correct codec name */
> >     571         for (i = 0; i < ARRAY_SIZE(cht_dailink); i++)
> >     572                 if (cht_dailink[i].codecs->name &&
> >     573                     !strcmp(cht_dailink[i].codecs->name,
> >     574                             "i2c-10EC5645:00")) {
> >     575                         dai_index = i;
> >     576                         break;
> >     577                 }
> >     578 
> >     579         /* fixup codec name based on HID */
> >     580         adev = acpi_dev_get_first_match_dev(mach->id, NULL, -1);
> >     581         if (adev) {
> >                     ^^^^
> > The old code assumes adev can be NULL
> > 
> >     582                 snprintf(cht_rt5645_codec_name, sizeof(cht_rt5645_codec_name),
> >     583                          "i2c-%s", acpi_dev_name(adev));
> >     584                 cht_dailink[dai_index].codecs->name = cht_rt5645_codec_name;
> >     585         }
> >     586         /* acpi_get_first_physical_node() returns a borrowed ref, no need to deref */
> > --> 587         codec_dev = acpi_get_first_physical_node(adev);
> >                                                          ^^^^
> > Unchecked dereference
> 
> This looks like a problem in multiple machine drivers sharing similar
> code, if we want to consistently check the output we probably need
> something like https://github.com/thesofproject/linux/pull/5117 ?
> 

These are actually Smatch warnings, not Sparse warnings btw.  (Smatch
uses Sparse as a parser.  Everyone gets them mixed up).

regards,
dan carpenter