lkml.org 
[lkml]   [2017]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: ath10k: ret used but uninitialized (was: Re: ath10k: add initial SDIO support)
From
Date
> 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.
>
I think it would be best to use 0 as initial value of ret in this case.
This will make all other interrupts be processed in a normal way.

Kalle: Should I create a new patch (initializing ret with zero)?

>> + 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?
>
ret = 0 or ret = -ECANCELED, will result in no warning message.
-ETIMEDOUT could be used perhaps.

Note that the function is a void function so the error will not get
propagated.

I am fine with ret = 0 in this case as well.
>> + 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@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
>

\
 
 \ /
  Last update: 2017-07-06 22:02    [W:0.064 / U:1.052 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site