| Message ID | 87bmowd0mh.fsf@kamboji.qca.qualcomm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 7, 2017 at 12:04 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > Erik Stromdahl <erik.stromdahl@gmail.com> writes: > >>> 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)? > > Yes, please send a new patch fixing this. > > But I don't like that much with the style of initialising ret to zero, > it tends to hide things. Instead my preference is something like below > where the error handling is more explicit and easier to find where it's > exactly failing. But that's just an example how I would try to solve it, > it still lacks the handling of -ECANCEL etc. I think I would simply replace the "while() {}" loop with "do{} while()", as that would guarantee it to be run at least once in a way that the compiler can see. Arnd
Hi Arnd, On Fri, Jul 7, 2017 at 4:14 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Jul 7, 2017 at 12:04 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >> Erik Stromdahl <erik.stromdahl@gmail.com> writes: >>>> 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)? >> >> Yes, please send a new patch fixing this. >> >> But I don't like that much with the style of initialising ret to zero, >> it tends to hide things. Instead my preference is something like below >> where the error handling is more explicit and easier to find where it's >> exactly failing. But that's just an example how I would try to solve it, >> it still lacks the handling of -ECANCEL etc. > > I think I would simply replace the "while() {}" loop with "do{} while()", > as that would guarantee it to be run at least once in a way that the > compiler can see. Right, that's probably the simplest and cleanest solution. 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
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c index 859ed870bd97..19a53e577932 100644 --- a/drivers/net/wireless/ath/ath10k/sdio.c +++ b/drivers/net/wireless/ath/ath10k/sdio.c @@ -689,8 +689,10 @@ static int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k *ar, */ ret = ath10k_sdio_mbox_rx_alloc(ar, lookaheads, n_lookaheads); - if (ret) - break; + if (ret) { + ath10k_warn(ar, "failed to ....: %d", ret); + return ret; + } if (ar_sdio->n_rx_pkts >= 2) /* A recv bundle was detected, force IRQ status @@ -709,8 +711,10 @@ static int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k *ar, lookaheads, &n_lookaheads); - if (!n_lookaheads || ret) - break; + if (!n_lookaheads || ret) { + ath10k_warn(ar, "failed to ...."); + return ret; + } /* For SYNCH processing, if we get here, we are running * through the loop again due to updated lookaheads. Set @@ -721,11 +725,7 @@ static int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k *ar, *done = false; } - if (ret && (ret != -ECANCELED)) - ath10k_warn(ar, "failed to get pending recv messages: %d\n", - ret); - - return ret; + return 0; } static int ath10k_sdio_mbox_proc_dbg_intr(struct ath10k *ar)