[alsa-devel] Missing return check of of_property_read_*()
Hi,
I hoped that I wouldn't need to write this kind of post, but since nothing happened so far, so here we go:
The following compile warnings are present for quite some time. They are due to the lack of return check from of_property_read*().
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
sound/soc/sti/uniperif_player.c: In function ‘uni_player_init’: sound/soc/sti/uniperif_player.c:1006:6: warning: ‘mode’ may be used uninitialized in this function [-Wmaybe-uninitialized] if (strcasecmp(mode, "hdmi") == 0) ^ sound/soc/sti/uniperif_player.c:985:14: note: ‘mode’ was declared here const char *mode; ^
Both should be trivial to fix, but I won't bother to fix these things by myself, as it results always in a matter of taste how to write. So, please fix them appropriately.
Although not shown as compile warning like the above, lots of codec drivers have no return value checks from of_property_read*(), too. A quick grep reveals:
% git grep -B1 '^[[:space:]]*of_property_read_' sound/soc sound/soc/codecs/adau1701.c- sound/soc/codecs/adau1701.c: of_property_read_u32(dev->of_node, "adi,pll-clkdiv", -- sound/soc/codecs/adau1701.c- sound/soc/codecs/adau1701.c: of_property_read_u8_array(dev->of_node, "adi,pin-config", -- sound/soc/codecs/cs35l32.c- sound/soc/codecs/cs35l32.c: of_property_read_u32(np, "cirrus,boost-manager", &val); -- sound/soc/codecs/cs35l32.c- sound/soc/codecs/cs35l32.c: of_property_read_u32(np, "cirrus,sdout-datacfg", &val); -- sound/soc/codecs/cs35l32.c- sound/soc/codecs/cs35l32.c: of_property_read_u32(np, "cirrus,battery-threshold", &val); -- sound/soc/codecs/cs35l32.c- sound/soc/codecs/cs35l32.c: of_property_read_u32(np, "cirrus,battery-recovery", &val); -- sound/soc/codecs/sta32x.c- sound/soc/codecs/sta32x.c: of_property_read_u8(np, "st,output-conf", sound/soc/codecs/sta32x.c- &pdata->output_conf); sound/soc/codecs/sta32x.c: of_property_read_u8(np, "st,ch1-output-mapping", sound/soc/codecs/sta32x.c- &pdata->ch1_output_mapping); sound/soc/codecs/sta32x.c: of_property_read_u8(np, "st,ch2-output-mapping", sound/soc/codecs/sta32x.c- &pdata->ch2_output_mapping); sound/soc/codecs/sta32x.c: of_property_read_u8(np, "st,ch3-output-mapping", -- sound/soc/codecs/sta32x.c- tmp = 140; sound/soc/codecs/sta32x.c: of_property_read_u16(np, "st,drop-compensation-ns", &tmp); -- sound/soc/codecs/sta350.c- sound/soc/codecs/sta350.c: of_property_read_u8(np, "st,output-conf", sound/soc/codecs/sta350.c- &pdata->output_conf); sound/soc/codecs/sta350.c: of_property_read_u8(np, "st,ch1-output-mapping", sound/soc/codecs/sta350.c- &pdata->ch1_output_mapping); sound/soc/codecs/sta350.c: of_property_read_u8(np, "st,ch2-output-mapping", sound/soc/codecs/sta350.c- &pdata->ch2_output_mapping); sound/soc/codecs/sta350.c: of_property_read_u8(np, "st,ch3-output-mapping", -- sound/soc/codecs/sta350.c- tmp = 140; sound/soc/codecs/sta350.c: of_property_read_u16(np, "st,drop-compensation-ns", &tmp); -- sound/soc/codecs/tas5086.c- sound/soc/codecs/tas5086.c: of_property_read_u32(of_node, "ti,charge-period", -- sound/soc/codecs/tlv320aic31xx.c- sound/soc/codecs/tlv320aic31xx.c: of_property_read_u32(np, "ai31xx-micbias-vg", &value); -- sound/soc/codecs/twl4030.c- sound/soc/codecs/twl4030.c: of_property_read_u32(node, "ti,digimic_delay", sound/soc/codecs/twl4030.c- &pdata->digimic_delay); sound/soc/codecs/twl4030.c: of_property_read_u32(node, "ti,ramp_delay_value", sound/soc/codecs/twl4030.c- &pdata->ramp_delay_value); sound/soc/codecs/twl4030.c: of_property_read_u32(node, "ti,offset_cncl_path", -- sound/soc/fsl/fsl_esai.c- esai_priv->synchronous = sound/soc/fsl/fsl_esai.c: of_property_read_bool(np, "fsl,esai-synchronous"); -- sound/soc/omap/omap-abe-twl6040.c- priv->jack_detection = of_property_read_bool(node, "ti,jack-detection"); sound/soc/omap/omap-abe-twl6040.c: of_property_read_u32(node, "ti,mclk-freq", &priv->mclk_freq); -- sound/soc/samsung/i2s.c- } sound/soc/samsung/i2s.c: of_property_read_string_index(dev->of_node, -- sound/soc/sh/rcar/rsrc-card.c- /* sampling rate convert */ sound/soc/sh/rcar/rsrc-card.c: of_property_read_u32(node, "convert-rate", &priv->convert_rate); -- sound/soc/sti/uniperif_player.c- sound/soc/sti/uniperif_player.c: of_property_read_u32(pnode, "version", &player->ver); -- sound/soc/sti/uniperif_player.c- sound/soc/sti/uniperif_player.c: of_property_read_u32(pnode, "uniperiph-id", &info->id); -- sound/soc/sti/uniperif_player.c- /* Read the device mode property */ sound/soc/sti/uniperif_player.c: of_property_read_string(pnode, "mode", &mode); -- sound/soc/sti/uniperif_reader.c- sound/soc/sti/uniperif_reader.c: of_property_read_u32(node, "version", &reader->ver);
Many of them look really buggy (while some have the default value to fall back). And many codes have even no value checks. These should be fixed as well.
thanks,
Takashi
On Wed, Sep 09, 2015 at 04:40:36PM +0200, Takashi Iwai wrote:
I hoped that I wouldn't need to write this kind of post, but since nothing happened so far, so here we go:
Could you be more specific about nothing happening here...
The following compile warnings are present for quite some time. They are due to the lack of return check from of_property_read*().
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
How are you generating these - what compiler and config? None of the build bots are reporting any of the warnings you have here.
On Wed, 9 Sep 2015, Mark Brown wrote:
On Wed, Sep 09, 2015 at 04:40:36PM +0200, Takashi Iwai wrote:
I hoped that I wouldn't need to write this kind of post, but since nothing happened so far, so here we go:
Could you be more specific about nothing happening here...
The following compile warnings are present for quite some time. They are due to the lack of return check from of_property_read*().
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
How are you generating these - what compiler and config? None of the build bots are reporting any of the warnings you have here.
Shouldn't that build server at Intel catch these? I usually get emails about things like this from them.
On Wed, 09 Sep 2015 16:55:53 +0200, Brian Austin wrote:
On Wed, 9 Sep 2015, Mark Brown wrote:
On Wed, Sep 09, 2015 at 04:40:36PM +0200, Takashi Iwai wrote:
I hoped that I wouldn't need to write this kind of post, but since nothing happened so far, so here we go:
Could you be more specific about nothing happening here...
The following compile warnings are present for quite some time. They are due to the lack of return check from of_property_read*().
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
How are you generating these - what compiler and config? None of the build bots are reporting any of the warnings you have here.
Shouldn't that build server at Intel catch these? I usually get emails about things like this from them.
Maybe depending on gcc version. But it's interesting if no other static analyzer hits this, too.
I'm considering whether adding __must_check to of_property_read*() would make sense. Then it'll be caught no matter which gcc version is used. But this would hit too many spots.
Takashi
On Wed, Sep 09, 2015 at 05:05:18PM +0200, Takashi Iwai wrote:
Brian Austin wrote:
Shouldn't that build server at Intel catch these? I usually get emails about things like this from them.
Maybe depending on gcc version. But it's interesting if no other static analyzer hits this, too.
It'll be because nobody is checking !OF ASoC configurations and paying attention to warnings - with !OF the functions are inline in the header and the compiler can tell nothing happens.
I'm considering whether adding __must_check to of_property_read*() would make sense. Then it'll be caught no matter which gcc version is used. But this would hit too many spots.
Well, if they're all buggy...
On Wed, 09 Sep 2015 17:57:37 +0200, Mark Brown wrote:
On Wed, Sep 09, 2015 at 05:05:18PM +0200, Takashi Iwai wrote:
Brian Austin wrote:
Shouldn't that build server at Intel catch these? I usually get emails about things like this from them.
Maybe depending on gcc version. But it's interesting if no other static analyzer hits this, too.
It'll be because nobody is checking !OF ASoC configurations and paying attention to warnings - with !OF the functions are inline in the header and the compiler can tell nothing happens.
Yes, but non-OF ASoC gets more popular on x86 nowadays, so this should be covered regularly, too.
I'm considering whether adding __must_check to of_property_read*() would make sense. Then it'll be caught no matter which gcc version is used. But this would hit too many spots.
Well, if they're all buggy...
Not all of them are buggy, of course. Some codes set the default fallback value beforehand. But, if thinking of future use, it might be better to make the check mandatory. No strong opinion here, though.
Takashi
On Wed, Sep 09, 2015 at 06:01:22PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
It'll be because nobody is checking !OF ASoC configurations and paying attention to warnings - with !OF the functions are inline in the header and the compiler can tell nothing happens.
Yes, but non-OF ASoC gets more popular on x86 nowadays, so this should be covered regularly, too.
Hopefully Intel will be paying attention here! :P
I'm considering whether adding __must_check to of_property_read*() would make sense. Then it'll be caught no matter which gcc version is used. But this would hit too many spots.
Well, if they're all buggy...
Not all of them are buggy, of course. Some codes set the default fallback value beforehand. But, if thinking of future use, it might be better to make the check mandatory. No strong opinion here, though.
I don't know how clear it is that a failed read won't overwrite existing data in the value variable. The other option would be to have Kconfig able to generate all*config with select options (like OF) disabled and then get the build bots to start covering those as a standard configuration.
On Wed, 09 Sep 2015 18:19:19 +0200, Mark Brown wrote:
On Wed, Sep 09, 2015 at 06:01:22PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
It'll be because nobody is checking !OF ASoC configurations and paying attention to warnings - with !OF the functions are inline in the header and the compiler can tell nothing happens.
Yes, but non-OF ASoC gets more popular on x86 nowadays, so this should be covered regularly, too.
Hopefully Intel will be paying attention here! :P
I'm considering whether adding __must_check to of_property_read*() would make sense. Then it'll be caught no matter which gcc version is used. But this would hit too many spots.
Well, if they're all buggy...
Not all of them are buggy, of course. Some codes set the default fallback value beforehand. But, if thinking of future use, it might be better to make the check mandatory. No strong opinion here, though.
I don't know how clear it is that a failed read won't overwrite existing data in the value variable.
Hm, right, this doesn't seem clearly defined although implicitly assumed by some callers. Maybe the explicit error check is safer and more understandable.
The other option would be to have Kconfig able to generate all*config with select options (like OF) disabled and then get the build bots to start covering those as a standard configuration.
Agreed.
Fengguang, we've been discussing about the compile warnings that weren't caught by 0days. It seems that it's triggered by !CONFIG_OF but with CONFIG_COMPILE_TEST=y. Then I got warnings like:
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
The above was with gcc-5.1.1, but Lars told that he saw such a warning with gcc-4.9, too.
Could you add this kind of kconfig in your test?
thanks,
Takashi
Hi Takashi,
Fengguang, we've been discussing about the compile warnings that weren't caught by 0days. It seems that it's triggered by !CONFIG_OF but with CONFIG_COMPILE_TEST=y. Then I got warnings like:
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
The above was with gcc-5.1.1, but Lars told that he saw such a warning with gcc-4.9, too.
Could you add this kind of kconfig in your test?
It's covered through the lots of randconfig tests. However the problem is, they are pretty old warnings and 0day ignores old warnings because old warnings may well be intensionally ignored by people. On the contrast, build errors can be re-reported if remain unfixed for long time.
Thanks, Fengguang
On Thu, Sep 10, 2015 at 11:10:22AM +0800, Fengguang Wu wrote:
Hi Takashi,
Fengguang, we've been discussing about the compile warnings that weren't caught by 0days. It seems that it's triggered by !CONFIG_OF but with CONFIG_COMPILE_TEST=y. Then I got warnings like:
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
The above was with gcc-5.1.1, but Lars told that he saw such a warning with gcc-4.9, too.
Could you add this kind of kconfig in your test?
It's covered through the lots of randconfig tests. However the problem is, they are pretty old warnings and 0day ignores old warnings because old warnings may well be intensionally ignored by people. On the contrast, build errors can be re-reported if remain unfixed for long time.
That said, if as a maintainer you demand "all warnings should be quieted", I'll happily help you reminding the people who break the rule.
If it's a generally agreed rule by the maintainers, I'll be happy to help guarantee it kernel wide.
Thanks, Fengguang
On Thu, 10 Sep 2015 05:23:44 +0200, Fengguang Wu wrote:
On Thu, Sep 10, 2015 at 11:10:22AM +0800, Fengguang Wu wrote:
Hi Takashi,
Fengguang, we've been discussing about the compile warnings that weren't caught by 0days. It seems that it's triggered by !CONFIG_OF but with CONFIG_COMPILE_TEST=y. Then I got warnings like:
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
The above was with gcc-5.1.1, but Lars told that he saw such a warning with gcc-4.9, too.
Could you add this kind of kconfig in your test?
It's covered through the lots of randconfig tests. However the problem is, they are pretty old warnings and 0day ignores old warnings because old warnings may well be intensionally ignored by people. On the contrast, build errors can be re-reported if remain unfixed for long time.
That said, if as a maintainer you demand "all warnings should be quieted", I'll happily help you reminding the people who break the rule.
If it's a generally agreed rule by the maintainers, I'll be happy to help guarantee it kernel wide.
Well, there can be no black-or-white answer for this, as you know. Indeed, what annoys most is repeatedly mail combs for minor or false positive warnings. But sometimes checking warnings is useful; when I look through build warnings occasionally, it really hits real bugs sometimes.
So this made me wonder whether we have a way to take a glance through gathered compile warnings at once. Are they archived and reachable?
thanks,
Takashi
On Thu, 10 Sep 2015 07:32:04 +0200, Takashi Iwai wrote:
On Thu, 10 Sep 2015 05:23:44 +0200, Fengguang Wu wrote:
On Thu, Sep 10, 2015 at 11:10:22AM +0800, Fengguang Wu wrote:
Hi Takashi,
Fengguang, we've been discussing about the compile warnings that weren't caught by 0days. It seems that it's triggered by !CONFIG_OF but with CONFIG_COMPILE_TEST=y. Then I got warnings like:
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
The above was with gcc-5.1.1, but Lars told that he saw such a warning with gcc-4.9, too.
Could you add this kind of kconfig in your test?
It's covered through the lots of randconfig tests. However the problem is, they are pretty old warnings and 0day ignores old warnings because old warnings may well be intensionally ignored by people. On the contrast, build errors can be re-reported if remain unfixed for long time.
That said, if as a maintainer you demand "all warnings should be quieted", I'll happily help you reminding the people who break the rule.
If it's a generally agreed rule by the maintainers, I'll be happy to help guarantee it kernel wide.
Well, there can be no black-or-white answer for this, as you know. Indeed, what annoys most is repeatedly mail combs for minor or false
Oops, s/comb/bomb/ It's quite opposite...
Takashi
positive warnings. But sometimes checking warnings is useful; when I look through build warnings occasionally, it really hits real bugs sometimes.
So this made me wonder whether we have a way to take a glance through gathered compile warnings at once. Are they archived and reachable?
thanks,
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Thu, Sep 10, 2015 at 07:32:04AM +0200, Takashi Iwai wrote:
On Thu, 10 Sep 2015 05:23:44 +0200, Fengguang Wu wrote:
On Thu, Sep 10, 2015 at 11:10:22AM +0800, Fengguang Wu wrote:
Hi Takashi,
Fengguang, we've been discussing about the compile warnings that weren't caught by 0days. It seems that it's triggered by !CONFIG_OF but with CONFIG_COMPILE_TEST=y. Then I got warnings like:
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
The above was with gcc-5.1.1, but Lars told that he saw such a warning with gcc-4.9, too.
Could you add this kind of kconfig in your test?
It's covered through the lots of randconfig tests. However the problem is, they are pretty old warnings and 0day ignores old warnings because old warnings may well be intensionally ignored by people. On the contrast, build errors can be re-reported if remain unfixed for long time.
That said, if as a maintainer you demand "all warnings should be quieted", I'll happily help you reminding the people who break the rule.
If it's a generally agreed rule by the maintainers, I'll be happy to help guarantee it kernel wide.
Well, there can be no black-or-white answer for this, as you know. Indeed, what annoys most is repeatedly mail combs for minor or false positive warnings. But sometimes checking warnings is useful; when I look through build warnings occasionally, it really hits real bugs sometimes.
That's true.
So this made me wonder whether we have a way to take a glance through gathered compile warnings at once. Are they archived and reachable?
Yes. Here are the active "may be used uninitialized" warnings under sound/:
sound/core/pcm_lib.c:244: warning: 'curr_tstamp.tv_nsec' may be used uninitialized in this function sound/core/pcm_lib.c:244: warning: 'curr_tstamp.tv_sec' may be used uninitialized in this function sound/drivers/serial-u16550.c:966:9: warning: 'uart' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/firewire/bebob/bebob_focusrite.c:175:14: warning: 'value' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/asihpi/hpi6205.c:605:5: warning: 'p_control_cache_virtual' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/asihpi/hpioctl.c:304:17: warning: 'puhr' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/atiixp.c:1683:14: warning: 'chip' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/atiixp_modem.c:1314:2: warning: 'chip' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/hda/hda_generic.c:2872:32: warning: 'mix_val' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/hda/hda_generic.c:2875:305: warning: 'mute_val' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/hda/patch_realtek.c:4826:12: warning: 'pin' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/hda/patch_via.c:2108:24: warning: 'dac' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/lx6464es/lx_core.c:1121:31: warning: 'irqsrc' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/pcxhr/pcxhr.c:259:25: warning: 'pllreg' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/pcxhr/pcxhr.c:337:6: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/pcxhr/pcxhr.c:342:24: warning: 'realfreq' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/pci/via82xx.c:2589:2: warning: 'chip' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/soc/codecs/arizona.c:1367:27: warning: 'aif_tx_state' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/soc/codecs/arizona.c:1370:21: warning: 'aif_rx_state' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/soc/codecs/cs35l32.c:329:2: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/soc/codecs/wm2000.c:543:6: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/soc/codecs/wm8978.c:814:5: warning: 'diff' may be used uninitialized in this function [-Wuninitialized] sound/soc/fsl/fsl_ssi.c:715:7: warning: 'baudrate' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/soc/generic/simple-card.c:476:29: warning: 'flags' may be used uninitialized in this function [-Wuninitialized] sound/soc/soc-core.c:1768:7: warning: 'compress_type' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/soc/soc-dapm.c:3125:6: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/usb/caiaq/device.c:494:16: warning: 'card' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/usb/caiaq/device.h:128:51: warning: 'card' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/usb/format.c:482:5: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/usb/usbaudio.c:2812:5: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/usb/usx2y/us122l.c:583:15: warning: 'card' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/usb/usx2y/us122l.h:20:40: warning: 'card' may be used uninitialized in this function [-Wmaybe-uninitialized] sound/usb/usx2y/usbusx2y.c:390:15: warning: 'card' may be used uninitialized in this function [-Wmaybe-uninitialized]
Thanks, Fengguang
On Thu, Sep 10, 2015 at 02:24:32PM +0800, Fengguang Wu wrote:
On Thu, Sep 10, 2015 at 07:32:04AM +0200, Takashi Iwai wrote:
So this made me wonder whether we have a way to take a glance through gathered compile warnings at once. Are they archived and reachable?
Yes. Here are the active "may be used uninitialized" warnings under sound/:
sound/core/pcm_lib.c:244: warning: 'curr_tstamp.tv_nsec' may be used uninitialized in this function
Is there any way we can view the current status on line?
On Thu, Sep 10, 2015 at 11:54:50AM +0100, Mark Brown wrote:
On Thu, Sep 10, 2015 at 02:24:32PM +0800, Fengguang Wu wrote:
On Thu, Sep 10, 2015 at 07:32:04AM +0200, Takashi Iwai wrote:
So this made me wonder whether we have a way to take a glance through gathered compile warnings at once. Are they archived and reachable?
Yes. Here are the active "may be used uninitialized" warnings under sound/:
sound/core/pcm_lib.c:244: warning: 'curr_tstamp.tv_nsec' may be used uninitialized in this function
Is there any way we can view the current status on line?
We don't have interactive web interfaces to query/browse things. Sorry!
Regards, Fengguang
On Wed, Sep 09, 2015 at 06:01:22PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
It'll be because nobody is checking !OF ASoC configurations and paying attention to warnings - with !OF the functions are inline in the header and the compiler can tell nothing happens.
Yes, but non-OF ASoC gets more popular on x86 nowadays, so this should be covered regularly, too.
We should start getting some coverage for this from kernelci.org soon (and x86 allmodconfig with OF disabled).
On Wed, Sep 09, 2015 at 09:55:53AM -0500, Brian Austin wrote:
On Wed, 9 Sep 2015, Mark Brown wrote:
On Wed, Sep 09, 2015 at 04:40:36PM +0200, Takashi Iwai wrote:
How are you generating these - what compiler and config? None of the build bots are reporting any of the warnings you have here.
Shouldn't that build server at Intel catch these? I usually get emails about things like this from them.
Yeah, I haven't particularly noticed anything from them either (they don't publish reporting like kernelci.org does or I have locally for mine so it's harder to see if they reported something and it got missed).
On Wed, 09 Sep 2015 16:51:58 +0200, Mark Brown wrote:
On Wed, Sep 09, 2015 at 04:40:36PM +0200, Takashi Iwai wrote:
I hoped that I wouldn't need to write this kind of post, but since nothing happened so far, so here we go:
Could you be more specific about nothing happening here...
A wonderful gift such like a devoted maintainer/developer already fixing these bugs magically :)
The following compile warnings are present for quite some time. They are due to the lack of return check from of_property_read*().
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
How are you generating these - what compiler and config? None of the build bots are reporting any of the warnings you have here.
Then maybe this can be seen since gcc 5.x. A good progress of gcc. I've seen this since the beginning of merge for cs35l32.c (and I reported this already at merging the branch).
There is nothing special in config or compiler option, just use the normal x86-64 build with CONFIG_COMPILE_TEST. I'm using gcc-5.1.1 for now.
Takashi
On 09/09/2015 05:01 PM, Takashi Iwai wrote:
On Wed, 09 Sep 2015 16:51:58 +0200, Mark Brown wrote:
On Wed, Sep 09, 2015 at 04:40:36PM +0200, Takashi Iwai wrote:
I hoped that I wouldn't need to write this kind of post, but since nothing happened so far, so here we go:
Could you be more specific about nothing happening here...
A wonderful gift such like a devoted maintainer/developer already fixing these bugs magically :)
The following compile warnings are present for quite some time. They are due to the lack of return check from of_property_read*().
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
How are you generating these - what compiler and config? None of the build bots are reporting any of the warnings you have here.
Then maybe this can be seen since gcc 5.x. A good progress of gcc. I've seen this since the beginning of merge for cs35l32.c (and I reported this already at merging the branch).
There is nothing special in config or compiler option, just use the normal x86-64 build with CONFIG_COMPILE_TEST. I'm using gcc-5.1.1 for now.
I think you'll get them for any config with CONFIG_OF disabled. I've been seeing those warnings for a while now when compile-testing for x86 and I'm still on gcc 4.9.
But even if you don't get the warning the code is still broken, the compiler just does not know.
- Lars
On Wed, 09 Sep 2015 17:05:05 +0200, Lars-Peter Clausen wrote:
On 09/09/2015 05:01 PM, Takashi Iwai wrote:
On Wed, 09 Sep 2015 16:51:58 +0200, Mark Brown wrote:
On Wed, Sep 09, 2015 at 04:40:36PM +0200, Takashi Iwai wrote:
I hoped that I wouldn't need to write this kind of post, but since nothing happened so far, so here we go:
Could you be more specific about nothing happening here...
A wonderful gift such like a devoted maintainer/developer already fixing these bugs magically :)
The following compile warnings are present for quite some time. They are due to the lack of return check from of_property_read*().
sound/soc/codecs/cs35l32.c: In function ‘cs35l32_i2c_probe’: sound/soc/codecs/cs35l32.c:278:2: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized] switch (val) { ^ sound/soc/codecs/cs35l32.c:272:15: note: ‘val’ was declared here unsigned int val; ^
How are you generating these - what compiler and config? None of the build bots are reporting any of the warnings you have here.
Then maybe this can be seen since gcc 5.x. A good progress of gcc. I've seen this since the beginning of merge for cs35l32.c (and I reported this already at merging the branch).
There is nothing special in config or compiler option, just use the normal x86-64 build with CONFIG_COMPILE_TEST. I'm using gcc-5.1.1 for now.
I think you'll get them for any config with CONFIG_OF disabled. I've been seeing those warnings for a while now when compile-testing for x86 and I'm still on gcc 4.9.
Ah, good point. That can trigger the check easily, of course. But then I don't know why 0day check didn't catch these.
But even if you don't get the warning the code is still broken, the compiler just does not know.
Right.
Takashi
On Wed, Sep 09, 2015 at 05:05:05PM +0200, Lars-Peter Clausen wrote:
On 09/09/2015 05:01 PM, Takashi Iwai wrote:
There is nothing special in config or compiler option, just use the normal x86-64 build with CONFIG_COMPILE_TEST. I'm using gcc-5.1.1 for now.
I think you'll get them for any config with CONFIG_OF disabled. I've been seeing those warnings for a while now when compile-testing for x86 and I'm still on gcc 4.9.
Oh, right - that'll be the issue, nobody build tests those configurations as standard, all the builders are all*config or configurations that don't include ASoC. I'd assume the Intel people are seeing these in their testing but otherwise everyone is going to be using DT and not seeing warnings.
But even if you don't get the warning the code is still broken, the compiler just does not know.
Sure, I'm not questioning that there's an issue but rather why nobody else had seen the warnings.
Hi Lars
I have two cards originally but the second one failed because of the patch ASoC: Prevent components from being bound to multiple cards.
My configuration is like this: card0: use platform's DAI component A card1: use the same platform's DAI component B
The two cards use different DAI components, so it should be OK but still gets failed. I looked into this and found it is because snd_soc_register_platform() itself will also add a component, and this component was bound to the first card so the second probe failed.
It seems like I might need to separate the platform into two platform drivers, but it is strange since I have only one audio hardware. I wonder if it is reasonable to prevent a single ASoC platform driver from being used by multiple cards? Do you have any suggestion how to fix this?
On Fri, Oct 02, 2015 at 06:34:24PM +0800, Koro Chen wrote:
I have two cards originally but the second one failed because of the patch ASoC: Prevent components from being bound to multiple cards.
My configuration is like this: card0: use platform's DAI component A card1: use the same platform's DAI component B
What do you mean by "DAI component" here? It's not a term I'm familiar with...
On Fri, 2015-10-02 at 15:48 +0100, Mark Brown wrote:
On Fri, Oct 02, 2015 at 06:34:24PM +0800, Koro Chen wrote:
I have two cards originally but the second one failed because of the patch ASoC: Prevent components from being bound to multiple cards.
My configuration is like this: card0: use platform's DAI component A card1: use the same platform's DAI component B
What do you mean by "DAI component" here? It's not a term I'm familiar with...
Sorry I didn't make myself clear enough. In the sound/soc/mediatek/mtk-afe-pcm.c, I register two components "mtk_afe_pcm_dai_component" and "mtk_afe_hdmi_dai_component", each has its corresponding DAIs. Card0 (which is mt8173-rt5676-rt5650.c) uses DAIs from the first component, and card1 (new) uses DAIs of the second component. These two components are probed successfully and separately by card0 and card1, but card1 failed to probe another component created by snd_soc_register_platform(), which is already bound to card0.
On Sat, Oct 03, 2015 at 11:01:40PM +0800, Koro Chen wrote:
On Fri, 2015-10-02 at 15:48 +0100, Mark Brown wrote:
On Fri, Oct 02, 2015 at 06:34:24PM +0800, Koro Chen wrote:
I have two cards originally but the second one failed because of the patch ASoC: Prevent components from being bound to multiple cards.
My configuration is like this: card0: use platform's DAI component A card1: use the same platform's DAI component B
What do you mean by "DAI component" here? It's not a term I'm familiar with...
Sorry I didn't make myself clear enough. In the sound/soc/mediatek/mtk-afe-pcm.c, I register two components "mtk_afe_pcm_dai_component" and "mtk_afe_hdmi_dai_component", each has its corresponding DAIs. Card0 (which is mt8173-rt5676-rt5650.c) uses DAIs from the first component, and card1 (new) uses DAIs of the second component. These two components are probed successfully and separately by card0 and card1, but card1 failed to probe another component created by snd_soc_register_platform(), which is already bound to card0.
OK, I see. We probably do want to relax that restriction, it makes more sense at the DAI level but at the component level when we have components that have a bunch of independant DAIs hanging off a DMA controler as one component (which does make sense) then it breaks.
On Mon, Oct 05, 2015 at 11:07:17AM +0100, Mark Brown wrote:
On Sat, Oct 03, 2015 at 11:01:40PM +0800, Koro Chen wrote:
Sorry I didn't make myself clear enough. In the sound/soc/mediatek/mtk-afe-pcm.c, I register two components "mtk_afe_pcm_dai_component" and "mtk_afe_hdmi_dai_component", each has its corresponding DAIs. Card0 (which is mt8173-rt5676-rt5650.c) uses DAIs from the first component, and card1 (new) uses DAIs of the second component. These two components are probed successfully and separately by card0 and card1, but card1 failed to probe another component created by snd_soc_register_platform(), which is already bound to card0.
OK, I see. We probably do want to relax that restriction, it makes more sense at the DAI level but at the component level when we have components that have a bunch of independant DAIs hanging off a DMA controler as one component (which does make sense) then it breaks.
Oh, one other thing here - if there is routing within your component which would allow the two cards to be interconnected then that's a bit different and the cards should probably be combined into one.
On Mon, 2015-10-05 at 11:07 +0100, Mark Brown wrote:
On Sat, Oct 03, 2015 at 11:01:40PM +0800, Koro Chen wrote:
On Fri, 2015-10-02 at 15:48 +0100, Mark Brown wrote:
On Fri, Oct 02, 2015 at 06:34:24PM +0800, Koro Chen wrote:
I have two cards originally but the second one failed because of the patch ASoC: Prevent components from being bound to multiple cards.
My configuration is like this: card0: use platform's DAI component A card1: use the same platform's DAI component B
What do you mean by "DAI component" here? It's not a term I'm familiar with...
Sorry I didn't make myself clear enough. In the sound/soc/mediatek/mtk-afe-pcm.c, I register two components "mtk_afe_pcm_dai_component" and "mtk_afe_hdmi_dai_component", each has its corresponding DAIs. Card0 (which is mt8173-rt5676-rt5650.c) uses DAIs from the first component, and card1 (new) uses DAIs of the second component. These two components are probed successfully and separately by card0 and card1, but card1 failed to probe another component created by snd_soc_register_platform(), which is already bound to card0.
OK, I see. We probably do want to relax that restriction, it makes more sense at the DAI level but at the component level when we have components that have a bunch of independant DAIs hanging off a DMA controler as one component (which does make sense) then it breaks.
Thanks for your suggestion. Is it make sense if we use component->registered_as_component, which is only true in case of snd_soc_register_component() and will be false for components created by snd_soc_register_platform() or snd_soc_register_codec(), then modify the check condition like this:
@@ -1102,11 +1102,12 @@ static int soc_probe_component(struct snd_soc_card *card, struct snd_soc_dai *dai; int ret;
if (!strcmp(component->name, "snd-soc-dummy")) return 0;
if (component->card) { - if (component->card != card) { + if (component->card != card && component->registered_as_component) {
participants (6)
-
Brian Austin -
Fengguang Wu -
Koro Chen -
Lars-Peter Clausen -
Mark Brown -
Takashi Iwai