Messages in this thread | | | Subject | Re: ath10k: ret used but uninitialized (was: Re: ath10k: add initial SDIO support) | From | Erik Stromdahl <> | Date | Thu, 6 Jul 2017 22:01:57 +0200 |
| |
> 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 >
| |