| Message ID | 20181102161833.2956376-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath10k: avoid -Wmaybe-uninitialized warning | expand |
Hi, On Fri, Nov 2, 2018 at 9:19 AM Arnd Bergmann <arnd@arndb.de> wrote: > > In some configurations the inlining in gcc is suboptimal, causing > a false-positive warning: > > drivers/net/wireless/ath/ath10k/mac.c: In function 'ath10k_mac_init_rd': > drivers/net/wireless/ath/ath10k/mac.c:8374:39: error: 'rd' may be used uninitialized in this function [-Werror=maybe-uninitialized] > ar->ath_common.regulatory.current_rd = rd; > > If we initialize the output of ath10k_mac_get_wrdd_regulatory() > before returning, this problem goes away. > > Fixes: 209b2a68de76 ("ath10k: add platform regulatory domain support") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/net/wireless/ath/ath10k/mac.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c > index a1c2801ded10..0d5fde28ee44 100644 > --- a/drivers/net/wireless/ath/ath10k/mac.c > +++ b/drivers/net/wireless/ath/ath10k/mac.c > @@ -8321,6 +8321,8 @@ static int ath10k_mac_get_wrdd_regulatory(struct ath10k *ar, u16 *rd) > u32 alpha2_code; > char alpha2[3]; > > + *rd = ar->hw_eeprom_rd; > + Maybe it's just me, but it seems kinda weird for this function to assign a (valid) value to its "output" and still potentially return an error. If you really need to work around this compiler bug, maybe just put the eeprom assignment back in ath10k_mac_init_rd()? I'll leave it up to Kalle as to whether he wants to work around the compiler at all :) Oh wait, one more thing: this is actually an invalid refactoring. See how this function assigns '*rd' later in error cases. Today, we still treat those as errors and clobber those with the eeprom value, but now, you're making the fallback case continue to use the erroneous value (0xffff). You need to make that use a local variable and avoid clobbering *rd, if you want this to be correct. > root_handle = ACPI_HANDLE(&pdev->dev); Side note: your patch made me notice -- this code is *not* only used on PCI devices, yet it utilizes the 'pci_dev' structure. Fortunately, it only does this to needless convert it right back to a bare 'device', and then the ACPI code safely handles non-ACPI devices (should result in -EOPNOTSUPP below), but this is awfully strange code. Anyway, I'm just thinking out loud. I'll probably patch the pci_dev out myself. Brian > if (!root_handle) > return -EOPNOTSUPP; > @@ -8365,11 +8367,9 @@ static int ath10k_mac_init_rd(struct ath10k *ar) > u16 rd; > > ret = ath10k_mac_get_wrdd_regulatory(ar, &rd); > - if (ret) { > + if (ret) > ath10k_dbg(ar, ATH10K_DBG_BOOT, > "fallback to eeprom programmed regulatory settings\n"); > - rd = ar->hw_eeprom_rd; > - } > > ar->ath_common.regulatory.current_rd = rd; > return 0; > -- > 2.18.0 >
Brian Norris <briannorris@chromium.org> writes: > Hi, > > On Fri, Nov 2, 2018 at 9:19 AM Arnd Bergmann <arnd@arndb.de> wrote: >> >> In some configurations the inlining in gcc is suboptimal, causing >> a false-positive warning: >> >> drivers/net/wireless/ath/ath10k/mac.c: In function 'ath10k_mac_init_rd': >> drivers/net/wireless/ath/ath10k/mac.c:8374:39: error: 'rd' may be used uninitialized in this function [-Werror=maybe-uninitialized] >> ar->ath_common.regulatory.current_rd = rd; >> >> If we initialize the output of ath10k_mac_get_wrdd_regulatory() >> before returning, this problem goes away. >> >> Fixes: 209b2a68de76 ("ath10k: add platform regulatory domain support") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> drivers/net/wireless/ath/ath10k/mac.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c >> index a1c2801ded10..0d5fde28ee44 100644 >> --- a/drivers/net/wireless/ath/ath10k/mac.c >> +++ b/drivers/net/wireless/ath/ath10k/mac.c >> @@ -8321,6 +8321,8 @@ static int ath10k_mac_get_wrdd_regulatory(struct ath10k *ar, u16 *rd) >> u32 alpha2_code; >> char alpha2[3]; >> >> + *rd = ar->hw_eeprom_rd; >> + > > Maybe it's just me, but it seems kinda weird for this function to > assign a (valid) value to its "output" and still potentially return an > error. > > If you really need to work around this compiler bug, maybe just put > the eeprom assignment back in ath10k_mac_init_rd()? I'll leave it up > to Kalle as to whether he wants to work around the compiler at all :) In general I'm happy take workaround to compiler problems, I prefer to keep ath10k warning free much as possible. > Oh wait, one more thing: this is actually an invalid refactoring. See > how this function assigns '*rd' later in error cases. Today, we still > treat those as errors and clobber those with the eeprom value, but > now, you're making the fallback case continue to use the erroneous > value (0xffff). You need to make that use a local variable and avoid > clobbering *rd, if you want this to be correct. But I agree with Brian here, I don't think this patch is correct.
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index a1c2801ded10..0d5fde28ee44 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -8321,6 +8321,8 @@ static int ath10k_mac_get_wrdd_regulatory(struct ath10k *ar, u16 *rd) u32 alpha2_code; char alpha2[3]; + *rd = ar->hw_eeprom_rd; + root_handle = ACPI_HANDLE(&pdev->dev); if (!root_handle) return -EOPNOTSUPP; @@ -8365,11 +8367,9 @@ static int ath10k_mac_init_rd(struct ath10k *ar) u16 rd; ret = ath10k_mac_get_wrdd_regulatory(ar, &rd); - if (ret) { + if (ret) ath10k_dbg(ar, ATH10K_DBG_BOOT, "fallback to eeprom programmed regulatory settings\n"); - rd = ar->hw_eeprom_rd; - } ar->ath_common.regulatory.current_rd = rd; return 0;
In some configurations the inlining in gcc is suboptimal, causing a false-positive warning: drivers/net/wireless/ath/ath10k/mac.c: In function 'ath10k_mac_init_rd': drivers/net/wireless/ath/ath10k/mac.c:8374:39: error: 'rd' may be used uninitialized in this function [-Werror=maybe-uninitialized] ar->ath_common.regulatory.current_rd = rd; If we initialize the output of ath10k_mac_get_wrdd_regulatory() before returning, this problem goes away. Fixes: 209b2a68de76 ("ath10k: add platform regulatory domain support") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/net/wireless/ath/ath10k/mac.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)