| Message ID | 20230809-intel-hda-missing-chip-init-v1-1-61557ca6fa8a@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9c28423d3caae63e665e2b8d704fa41ac823b2a6 |
| Headers | show |
| Series | ASoC: SOF: Intel: Initialize chip in hda_sdw_check_wakeen_irq() | expand |
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.
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
>> 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
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
> 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.
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
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
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);
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(+) --- base-commit: 59146c3cd326a622e9041614842346aada11ca99 change-id: 20230809-intel-hda-missing-chip-init-dcccbe6365a4 Best regards,