[PATCH] ASoC: SOF: Intel: Initialize chip in hda_sdw_check_wakeen_irq()

[PATCH] ASoC: SOF: Intel: Initialize chip in hda_sdw_check_wakeen_irq()

[Nathan Chancellor]

Clang warns (or errors with CONFIG_WERROR):

  sound/soc/sof/intel/hda.c:423:6: error: variable 'chip' is uninitialized when used here [-Werror,-Wuninitialized]
    423 |         if (chip && chip->check_sdw_wakeen_irq)
        |             ^~~~
  sound/soc/sof/intel/hda.c:418:39: note: initialize the variable 'chip' to silence this warning
    418 |         const struct sof_intel_dsp_desc *chip;
        |                                              ^
        |                                               = NULL
  1 error generated.

Add the missing initialization, following the pattern of the other irq
functions.

Fixes: 9362ab78f175 ("ASoC: SOF: Intel: add abstraction for SoundWire wake-ups")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 sound/soc/sof/intel/hda.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index 04c748a72b13..15e6779efaa3 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -420,6 +420,7 @@ static bool hda_sdw_check_wakeen_irq(struct snd_sof_dev *sdev)
 	if (!(interface_mask & BIT(SOF_DAI_INTEL_ALH)))
 		return false;
 
+	chip = get_chip_info(sdev->pdata);
 	if (chip && chip->check_sdw_wakeen_irq)
 		return chip->check_sdw_wakeen_irq(sdev);
 

---
base-commit: 59146c3cd326a622e9041614842346aada11ca99
change-id: 20230809-intel-hda-missing-chip-init-dcccbe6365a4

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

Re: [PATCH] ASoC: SOF: Intel: Initialize chip in hda_sdw_check_wakeen_irq()

[Pierre-Louis Bossart]

On 8/9/23 13:12, Nathan Chancellor wrote:
> Clang warns (or errors with CONFIG_WERROR):
> 
>   sound/soc/sof/intel/hda.c:423:6: error: variable 'chip' is uninitialized when used here [-Werror,-Wuninitialized]
>     423 |         if (chip && chip->check_sdw_wakeen_irq)
>         |             ^~~~
>   sound/soc/sof/intel/hda.c:418:39: note: initialize the variable 'chip' to silence this warning
>     418 |         const struct sof_intel_dsp_desc *chip;
>         |                                              ^
>         |                                               = NULL
>   1 error generated.
> 
> Add the missing initialization, following the pattern of the other irq
> functions.
> 
> Fixes: 9362ab78f175 ("ASoC: SOF: Intel: add abstraction for SoundWire wake-ups")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

Indeed, thanks Nathan for flagging this obvious mistake. I must have
done something wrong when extracting the patches.

Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

That said, we DO compile with clang and there was no warning
https://github.com/thesofproject/linux/actions/runs/5542372669/job/15010818307

Is this dependent on a specific version of clang? I'd like to make sure
our tools and tests are updated.

Re: [PATCH] ASoC: SOF: Intel: Initialize chip in hda_sdw_check_wakeen_irq()

[Nathan Chancellor]

On Wed, Aug 09, 2023 at 01:41:18PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 8/9/23 13:12, Nathan Chancellor wrote:
> > Clang warns (or errors with CONFIG_WERROR):
> > 
> >   sound/soc/sof/intel/hda.c:423:6: error: variable 'chip' is uninitialized when used here [-Werror,-Wuninitialized]
> >     423 |         if (chip && chip->check_sdw_wakeen_irq)
> >         |             ^~~~
> >   sound/soc/sof/intel/hda.c:418:39: note: initialize the variable 'chip' to silence this warning
> >     418 |         const struct sof_intel_dsp_desc *chip;
> >         |                                              ^
> >         |                                               = NULL
> >   1 error generated.
> > 
> > Add the missing initialization, following the pattern of the other irq
> > functions.
> > 
> > Fixes: 9362ab78f175 ("ASoC: SOF: Intel: add abstraction for SoundWire wake-ups")
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> 
> Indeed, thanks Nathan for flagging this obvious mistake. I must have
> done something wrong when extracting the patches.
> 
> Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Thanks for the quick response!

> That said, we DO compile with clang and there was no warning
> https://github.com/thesofproject/linux/actions/runs/5542372669/job/15010818307
> 
> Is this dependent on a specific version of clang? I'd like to make sure
> our tools and tests are updated.

It should not be, I can reproduce it with all the versions of clang that
the kernel supports (11.x+).

Looking at your GitHub Actions files, I am not sure exporting CC works
correctly so I don't think you are building with clang. If I do it
locally:

$ export CC=clang

$ make -j$(nproc) defconfig

$ grep -E 'CONFIG_(CC_IS|CLANG|GCC)' .config
CONFIG_CC_IS_GCC=y
CONFIG_GCC_VERSION=130201
CONFIG_CLANG_VERSION=0
CONFIG_GCC11_NO_ARRAY_BOUNDS=y
CONFIG_GCC_PLUGINS=y
# CONFIG_GCC_PLUGIN_LATENT_ENTROPY is not set
# CONFIG_GCC_PLUGIN_STACKLEAK is not set

$ make -j$(nproc) sound/soc/sof/intel/hda.o

$ head -1 sound/soc/sof/intel/.hda.o.cmd
savedcmd_sound/soc/sof/intel/hda.o := gcc ...

This was brought up some time ago and Masahiro made a decent point that
this might not be a desirable behavior change.

https://lore.kernel.org/CAK7LNAT6Yp3oemUxSst+htnmM-St8WmSv+UZ2x2XF23cw-kU-Q@mail.gmail.com/

Switching to passing CC via the actual make command should fix that.

Cheers,
Nathan

Re: [PATCH] ASoC: SOF: Intel: Initialize chip in hda_sdw_check_wakeen_irq()

[Pierre-Louis Bossart]

>> That said, we DO compile with clang and there was no warning
>> https://github.com/thesofproject/linux/actions/runs/5542372669/job/15010818307
>>
>> Is this dependent on a specific version of clang? I'd like to make sure
>> our tools and tests are updated.
> 
> It should not be, I can reproduce it with all the versions of clang that
> the kernel supports (11.x+).
> 
> Looking at your GitHub Actions files, I am not sure exporting CC works
> correctly so I don't think you are building with clang. If I do it

D'oh. I did not see this one coming... nice.

> locally:
> 
> $ export CC=clang
> 
> $ make -j$(nproc) defconfig
> 
> $ grep -E 'CONFIG_(CC_IS|CLANG|GCC)' .config
> CONFIG_CC_IS_GCC=y
> CONFIG_GCC_VERSION=130201
> CONFIG_CLANG_VERSION=0
> CONFIG_GCC11_NO_ARRAY_BOUNDS=y
> CONFIG_GCC_PLUGINS=y
> # CONFIG_GCC_PLUGIN_LATENT_ENTROPY is not set
> # CONFIG_GCC_PLUGIN_STACKLEAK is not set
> 
> $ make -j$(nproc) sound/soc/sof/intel/hda.o
> 
> $ head -1 sound/soc/sof/intel/.hda.o.cmd
> savedcmd_sound/soc/sof/intel/hda.o := gcc ...
> 
> This was brought up some time ago and Masahiro made a decent point that
> this might not be a desirable behavior change.
> 
> https://lore.kernel.org/CAK7LNAT6Yp3oemUxSst+htnmM-St8WmSv+UZ2x2XF23cw-kU-Q@mail.gmail.com/
> 
> Switching to passing CC via the actual make command should fix that.

Not quite. We generate our .config using "make defconfig" as a baseline
and then "merge_config.sh" to add a bunch of fragments we need [1]. And
of course the latter script does not understand CC=clang and switches
back to GCC.

Looks like I painted myself in a corner for the last 5 years...Any
recommendations would be welcome.

[1]
https://github.com/thesofproject/kconfig/blob/master/kconfig-sof-default.sh

Re: [PATCH] ASoC: SOF: Intel: Initialize chip in hda_sdw_check_wakeen_irq()

[Nathan Chancellor]

On Wed, Aug 09, 2023 at 02:57:13PM -0500, Pierre-Louis Bossart wrote:
> > Looking at your GitHub Actions files, I am not sure exporting CC works
> > correctly so I don't think you are building with clang. If I do it
> 
> D'oh. I did not see this one coming... nice.
> 
> > locally:
> > 
> > $ export CC=clang
> > 
> > $ make -j$(nproc) defconfig
> > 
> > $ grep -E 'CONFIG_(CC_IS|CLANG|GCC)' .config
> > CONFIG_CC_IS_GCC=y
> > CONFIG_GCC_VERSION=130201
> > CONFIG_CLANG_VERSION=0
> > CONFIG_GCC11_NO_ARRAY_BOUNDS=y
> > CONFIG_GCC_PLUGINS=y
> > # CONFIG_GCC_PLUGIN_LATENT_ENTROPY is not set
> > # CONFIG_GCC_PLUGIN_STACKLEAK is not set
> > 
> > $ make -j$(nproc) sound/soc/sof/intel/hda.o
> > 
> > $ head -1 sound/soc/sof/intel/.hda.o.cmd
> > savedcmd_sound/soc/sof/intel/hda.o := gcc ...
> > 
> > This was brought up some time ago and Masahiro made a decent point that
> > this might not be a desirable behavior change.
> > 
> > https://lore.kernel.org/CAK7LNAT6Yp3oemUxSst+htnmM-St8WmSv+UZ2x2XF23cw-kU-Q@mail.gmail.com/
> > 
> > Switching to passing CC via the actual make command should fix that.
> 
> Not quite. We generate our .config using "make defconfig" as a baseline
> and then "merge_config.sh" to add a bunch of fragments we need [1]. And
> of course the latter script does not understand CC=clang and switches
> back to GCC.

Ah, I still think you will need to pass CC to make directly, rather than
through the environment but you should be able to prevent
merge_config.sh from getting in the way by passing '-m' to avoid having
it invoke make itself, then you can add a 'make olddefconfig' step after
that, perhaps something like this?

  - name: build start
    run: |
      export ARCH=x86_64 KCFLAGS="-Wall -Werror"
      export MAKEFLAGS=j"$(nproc)"
      bash kconfig/kconfig-sof-default.sh -m
      make CC=clang olddefconfig
      make CC=clang sound/
      make CC=clang drivers/soundwire/
      make CC=clang

Cheers,
Nathan

Re: [PATCH] ASoC: SOF: Intel: Initialize chip in hda_sdw_check_wakeen_irq()

[Pierre-Louis Bossart]

> Ah, I still think you will need to pass CC to make directly, rather than
> through the environment but you should be able to prevent
> merge_config.sh from getting in the way by passing '-m' to avoid having
> it invoke make itself, then you can add a 'make olddefconfig' step after
> that, perhaps something like this?
> 
>   - name: build start
>     run: |
>       export ARCH=x86_64 KCFLAGS="-Wall -Werror"
>       export MAKEFLAGS=j"$(nproc)"
>       bash kconfig/kconfig-sof-default.sh -m

The -m doesn't work since it's added last, but it's not even needed. The
sequence below re-adds clang, that's just fine.

>       make CC=clang olddefconfig
>       make CC=clang sound/
>       make CC=clang drivers/soundwire/
>       make CC=clang
The fun part now is that I get tons of unrelated errors - but at least
that's a sign we're using the clang compiler

https://github.com/thesofproject/linux/actions/runs/5813817494/job/15762178568?pr=4518


sound/pci/hda/hda_bind.c:232:18: error: format string is not a string
literal (potentially insecure) [-Werror,-Wformat-security]
2151
                request_module(mod);
2152
                               ^~~
2153
./include/linux/kmod.h:25:55: note: expanded from macro 'request_module'
2154
#define request_module(mod...) __request_module(true, mod)
2155
                                                      ^~~
2156
sound/pci/hda/hda_bind.c:232:18: note: treat the string as an argument
to avoid this
2157
                request_module(mod);
2158
                               ^
2159
                               "%s",
2160
./include/linux/kmod.h:25:55: note: expanded from macro 'request_module'
2161
#define request_module(mod...) __request_module(true, mod)
2162
                                                      ^
2163
1 error generated.

Re: [PATCH] ASoC: SOF: Intel: Initialize chip in hda_sdw_check_wakeen_irq()

[Mark Brown]

On Wed, 09 Aug 2023 11:12:07 -0700, Nathan Chancellor wrote:
> Clang warns (or errors with CONFIG_WERROR):
> 
>   sound/soc/sof/intel/hda.c:423:6: error: variable 'chip' is uninitialized when used here [-Werror,-Wuninitialized]
>     423 |         if (chip && chip->check_sdw_wakeen_irq)
>         |             ^~~~
>   sound/soc/sof/intel/hda.c:418:39: note: initialize the variable 'chip' to silence this warning
>     418 |         const struct sof_intel_dsp_desc *chip;
>         |                                              ^
>         |                                               = NULL
>   1 error generated.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: SOF: Intel: Initialize chip in hda_sdw_check_wakeen_irq()
      commit: 9c28423d3caae63e665e2b8d704fa41ac823b2a6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH] ASoC: SOF: Intel: Initialize chip in hda_sdw_check_wakeen_irq()

[Nathan Chancellor]

On Wed, Aug 09, 2023 at 03:42:44PM -0500, Pierre-Louis Bossart wrote:
> 
> > Ah, I still think you will need to pass CC to make directly, rather than
> > through the environment but you should be able to prevent
> > merge_config.sh from getting in the way by passing '-m' to avoid having
> > it invoke make itself, then you can add a 'make olddefconfig' step after
> > that, perhaps something like this?
> > 
> >   - name: build start
> >     run: |
> >       export ARCH=x86_64 KCFLAGS="-Wall -Werror"
> >       export MAKEFLAGS=j"$(nproc)"
> >       bash kconfig/kconfig-sof-default.sh -m
> 
> The -m doesn't work since it's added last, but it's not even needed. The
> sequence below re-adds clang, that's just fine.

Ah right!

> >       make CC=clang olddefconfig
> >       make CC=clang sound/
> >       make CC=clang drivers/soundwire/
> >       make CC=clang
> The fun part now is that I get tons of unrelated errors - but at least
> that's a sign we're using the clang compiler

> sound/pci/hda/hda_bind.c:232:18: error: format string is not a string
> literal (potentially insecure) [-Werror,-Wformat-security]

Heh, I suppose that is somewhat self inflicted with the '-Wall -Wextra',
as '-Wformat-security' gets re-enabled after being disabled in the main
Makefile. May be worth sticking a '-Wno-format-security' back on there.
Glad to hear that it is working now and thank you for testing with clang
to help catch issues before they make it to a tree :)

Cheers,
Nathan