[PATCH] ath10k: avoid -Wmaybe-uninitialized warning

[PATCH] ath10k: avoid -Wmaybe-uninitialized warning

[Arnd Bergmann]

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;
+
 	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;
-- 
2.18.0

Re: [PATCH] ath10k: avoid -Wmaybe-uninitialized warning

[Brian Norris]

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
>

Re: [PATCH] ath10k: avoid -Wmaybe-uninitialized warning

[Kalle Valo]

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.

-- 
Kalle Valo