ath10k: ret used but uninitialized (was: Re: ath10k: add initial SDIO support)

[Geert Uytterhoeven]

On Wed, Jul 5, 2017 at 9:52 PM, Linux Kernel Mailing List
<linux-kernel at vger.kernel.org> wrote:
> Web:        https://git.kernel.org/torvalds/c/d96db25d20256208ce47d71b9f673a1de4c6fd7e
> Commit:     d96db25d20256208ce47d71b9f673a1de4c6fd7e
> Parent:     f008d1537bf88396cf41a7c7a831e3acd1ee92a1
> Refname:    refs/heads/master
> Author:     Erik Stromdahl <erik.stromdahl at gmail.com>
> AuthorDate: Wed Apr 26 12:18:00 2017 +0300
> Committer:  Kalle Valo <kvalo at qca.qualcomm.com>
> CommitDate: Thu May 4 15:55:55 2017 +0300
>
>     ath10k: add initial SDIO support
>
>     Chipsets like QCA6584 have support for SDIO so add initial SDIO bus support to
>     ath10k. With this patch we have the low level HTC protocol working and it's
>     possible to boot the firmware, but it's still not possible to connect or
>     anything like. More changes are needed for full functionality. For that reason
>     we print during initialisation:
>
>     WARNING: ath10k SDIO support is incomplete, don't expect anything to work!
>
>     Signed-off-by: Erik Stromdahl <erik.stromdahl at gmail.com>
>     [kvalo at qca.qualcomm.com: refactoring, cleanup, commit log]
>     Signed-off-by: Kalle Valo <kvalo at qca.qualcomm.com>

> --- /dev/null
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c

> +static int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k *ar,
> +                                                 u32 msg_lookahead, bool *done)
> +{
> +       struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> +       u32 lookaheads[ATH10K_SDIO_MAX_RX_MSGS];
> +       int n_lookaheads = 1;
> +       unsigned long timeout;
> +       int ret;

With gcc 4.1.2:

drivers/net/wireless/ath/ath10k/sdio.c: In function
‘ath10k_sdio_mbox_rxmsg_pending_handler’:
drivers/net/wireless/ath/ath10k/sdio.c:676: warning: ‘ret’ may be used
uninitialized in this function

> +
> +       *done = true;
> +
> +       /* Copy the lookahead obtained from the HTC register table into our
> +        * temp array as a start value.
> +        */
> +       lookaheads[0] = msg_lookahead;
> +
> +       timeout = jiffies + SDIO_MBOX_PROCESSING_TIMEOUT_HZ;

Although very unlikely due to the long timeout, if the code is preempted here,
and the loop below never entered, ret will indeed be uninitialized.

It's unclear to me what the proper initialization would be, though, so
that's why I didn't send a patch.

> +       while (time_before(jiffies, timeout)) {
> +               /* Try to allocate as many HTC RX packets indicated by
> +                * n_lookaheads.
> +                */
> +               ret = ath10k_sdio_mbox_rx_alloc(ar, lookaheads,
> +                                               n_lookaheads);
> +               if (ret)
> +                       break;
> +
> +               if (ar_sdio->n_rx_pkts >= 2)
> +                       /* A recv bundle was detected, force IRQ status
> +                        * re-check again.
> +                        */
> +                       *done = false;
> +
> +               ret = ath10k_sdio_mbox_rx_fetch(ar);
> +
> +               /* Process fetched packets. This will potentially update
> +                * n_lookaheads depending on if the packets contain lookahead
> +                * reports.
> +                */
> +               n_lookaheads = 0;
> +               ret = ath10k_sdio_mbox_rx_process_packets(ar,
> +                                                         lookaheads,
> +                                                         &n_lookaheads);
> +
> +               if (!n_lookaheads || ret)
> +                       break;
> +
> +               /* For SYNCH processing, if we get here, we are running
> +                * through the loop again due to updated lookaheads. Set
> +                * flag that we should re-check IRQ status registers again
> +                * before leaving IRQ processing, this can net better
> +                * performance in high throughput situations.
> +                */
> +               *done = false;
> +       }
> +
> +       if (ret && (ret != -ECANCELED))
> +               ath10k_warn(ar, "failed to get pending recv messages: %d\n",
> +                           ret);
> +
> +       return ret;
> +}

> +static void ath10k_sdio_irq_handler(struct sdio_func *func)
> +{
> +       struct ath10k_sdio *ar_sdio = sdio_get_drvdata(func);
> +       struct ath10k *ar = ar_sdio->ar;
> +       unsigned long timeout;
> +       bool done = false;
> +       int ret;

drivers/net/wireless/ath/ath10k/sdio.c: In function ‘ath10k_sdio_irq_handler’:
drivers/net/wireless/ath/ath10k/sdio.c:1331: warning: ‘ret’ may be
used uninitialized in this function

> +
> +       /* Release the host during interrupts so we can pick it back up when
> +        * we process commands.
> +        */
> +       sdio_release_host(ar_sdio->func);
> +
> +       timeout = jiffies + ATH10K_SDIO_HIF_COMMUNICATION_TIMEOUT_HZ;

Same here.

Should ret be preinitialized to 0, -ECANCELED, or something else?

> +       while (time_before(jiffies, timeout) && !done) {
> +               ret = ath10k_sdio_mbox_proc_pending_irqs(ar, &done);
> +               if (ret)
> +                       break;
> +       }
> +
> +       sdio_claim_host(ar_sdio->func);
> +
> +       wake_up(&ar_sdio->irq_wq);
> +
> +       if (ret && ret != -ECANCELED)
> +               ath10k_warn(ar, "failed to process pending SDIO interrupts: %d\n",
> +                           ret);
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds