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

[Brian Norris <briannorris@chromium.org>]

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
>