Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683096

Re: ath10k: ret used but uninitialized

Show all headers | View raw

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.

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)


-- 
Kalle Valo

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

ath10k: ret used but uninitialized (was: Re: ath10k: add initial SDIO support) Geert Uytterhoeven <geert@linux-m68k.org> - 2017-07-06 10:30 +0200
  Re: ath10k: ret used but uninitialized (was: Re: ath10k: add initial  SDIO support) Erik Stromdahl <erik.stromdahl@gmail.com> - 2017-07-06 22:10 +0200
    Re: ath10k: ret used but uninitialized Kalle Valo <kvalo@qca.qualcomm.com> - 2017-07-07 12:10 +0200
      Re: ath10k: ret used but uninitialized Geert Uytterhoeven <geert@linux-m68k.org> - 2017-07-07 16:20 +0200
      Re: ath10k: ret used but uninitialized Arnd Bergmann <arnd@arndb.de> - 2017-07-07 16:20 +0200

csiph-web