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

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@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@gmail.com>
> AuthorDate: Wed Apr 26 12:18:00 2017 +0300
> Committer:  Kalle Valo <kvalo@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@gmail.com>
>     [kvalo@qca.qualcomm.com: refactoring, cleanup, commit log]
>     Signed-off-by: Kalle Valo <kvalo@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@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

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

[Erik Stromdahl]

> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
> 

Re: ath10k: ret used but uninitialized

[Kalle Valo]

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

Re: ath10k: ret used but uninitialized

[Arnd Bergmann]

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

Re: ath10k: ret used but uninitialized

[Geert Uytterhoeven]

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