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/d96db25d20256208ce47d71b9f6=
73a1de4c6fd7e
> 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 s=
upport to
>     ath10k. With this patch we have the low level HTC protocol working an=
d 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 th=
at 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 =3D ath10k_sdio_priv(ar);
> +       u32 lookaheads[ATH10K_SDIO_MAX_RX_MSGS];
> +       int n_lookaheads =3D 1;
> +       unsigned long timeout;
> +       int ret;

With gcc 4.1.2:

drivers/net/wireless/ath/ath10k/sdio.c: In function
=E2=80=98ath10k_sdio_mbox_rxmsg_pending_handler=E2=80=99:
drivers/net/wireless/ath/ath10k/sdio.c:676: warning: =E2=80=98ret=E2=80=99 =
may be used
uninitialized in this function

> +
> +       *done =3D true;
> +
> +       /* Copy the lookahead obtained from the HTC register table into o=
ur
> +        * temp array as a start value.
> +        */
> +       lookaheads[0] =3D msg_lookahead;
> +
> +       timeout =3D jiffies + SDIO_MBOX_PROCESSING_TIMEOUT_HZ;

Although very unlikely due to the long timeout, if the code is preempted he=
re,
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 =3D ath10k_sdio_mbox_rx_alloc(ar, lookaheads,
> +                                               n_lookaheads);
> +               if (ret)
> +                       break;
> +
> +               if (ar_sdio->n_rx_pkts >=3D 2)
> +                       /* A recv bundle was detected, force IRQ status
> +                        * re-check again.
> +                        */
> +                       *done =3D false;
> +
> +               ret =3D ath10k_sdio_mbox_rx_fetch(ar);
> +
> +               /* Process fetched packets. This will potentially update
> +                * n_lookaheads depending on if the packets contain looka=
head
> +                * reports.
> +                */
> +               n_lookaheads =3D 0;
> +               ret =3D 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 agai=
n
> +                * before leaving IRQ processing, this can net better
> +                * performance in high throughput situations.
> +                */
> +               *done =3D false;
> +       }
> +
> +       if (ret && (ret !=3D -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 =3D sdio_get_drvdata(func);
> +       struct ath10k *ar =3D ar_sdio->ar;
> +       unsigned long timeout;
> +       bool done =3D false;
> +       int ret;

drivers/net/wireless/ath/ath10k/sdio.c: In function =E2=80=98ath10k_sdio_ir=
q_handler=E2=80=99:
drivers/net/wireless/ath/ath10k/sdio.c:1331: warning: =E2=80=98ret=E2=80=99=
 may be
used uninitialized in this function

> +
> +       /* Release the host during interrupts so we can pick it back up w=
hen
> +        * we process commands.
> +        */
> +       sdio_release_host(ar_sdio->func);
> +
> +       timeout =3D 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 =3D 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 !=3D -ECANCELED)
> +               ath10k_warn(ar, "failed to process pending SDIO interrupt=
s: %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. Bu=
t
when I'm talking to journalists I just say "programmer" or something like t=
hat.
                                -- 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@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

[Kalle Valo]

RXJpayBTdHJvbWRhaGwgPGVyaWsuc3Ryb21kYWhsQGdtYWlsLmNvbT4gd3JpdGVzOg0KDQo+PiBX
aXRoIGdjYyA0LjEuMjoNCj4+DQo+PiBkcml2ZXJzL25ldC93aXJlbGVzcy9hdGgvYXRoMTBrL3Nk
aW8uYzogSW4gZnVuY3Rpb24NCj4+IOKAmGF0aDEwa19zZGlvX21ib3hfcnhtc2dfcGVuZGluZ19o
YW5kbGVy4oCZOg0KPj4gZHJpdmVycy9uZXQvd2lyZWxlc3MvYXRoL2F0aDEway9zZGlvLmM6Njc2
OiB3YXJuaW5nOiDigJhyZXTigJkgbWF5IGJlIHVzZWQNCj4+IHVuaW5pdGlhbGl6ZWQgaW4gdGhp
cyBmdW5jdGlvbg0KPj4NCj4+PiArDQo+Pj4gKyAgICAgICAqZG9uZSA9IHRydWU7DQo+Pj4gKw0K
Pj4+ICsgICAgICAgLyogQ29weSB0aGUgbG9va2FoZWFkIG9idGFpbmVkIGZyb20gdGhlIEhUQyBy
ZWdpc3RlciB0YWJsZSBpbnRvIG91cg0KPj4+ICsgICAgICAgICogdGVtcCBhcnJheSBhcyBhIHN0
YXJ0IHZhbHVlLg0KPj4+ICsgICAgICAgICovDQo+Pj4gKyAgICAgICBsb29rYWhlYWRzWzBdID0g
bXNnX2xvb2thaGVhZDsNCj4+PiArDQo+Pj4gKyAgICAgICB0aW1lb3V0ID0gamlmZmllcyArIFNE
SU9fTUJPWF9QUk9DRVNTSU5HX1RJTUVPVVRfSFo7DQo+Pg0KPj4gQWx0aG91Z2ggdmVyeSB1bmxp
a2VseSBkdWUgdG8gdGhlIGxvbmcgdGltZW91dCwgaWYgdGhlIGNvZGUgaXMgcHJlZW1wdGVkIGhl
cmUsDQo+PiBhbmQgdGhlIGxvb3AgYmVsb3cgbmV2ZXIgZW50ZXJlZCwgcmV0IHdpbGwgaW5kZWVk
IGJlIHVuaW5pdGlhbGl6ZWQuDQo+Pg0KPj4gSXQncyB1bmNsZWFyIHRvIG1lIHdoYXQgdGhlIHBy
b3BlciBpbml0aWFsaXphdGlvbiB3b3VsZCBiZSwgdGhvdWdoLCBzbw0KPj4gdGhhdCdzIHdoeSBJ
IGRpZG4ndCBzZW5kIGEgcGF0Y2guDQo+Pg0KPiBJIHRoaW5rIGl0IHdvdWxkIGJlIGJlc3QgdG8g
dXNlIDAgYXMgaW5pdGlhbCB2YWx1ZSBvZiByZXQgaW4gdGhpcyBjYXNlLg0KPiBUaGlzIHdpbGwg
bWFrZSBhbGwgb3RoZXIgaW50ZXJydXB0cyBiZSBwcm9jZXNzZWQgaW4gYSBub3JtYWwgd2F5Lg0K
Pg0KPiBLYWxsZTogU2hvdWxkIEkgY3JlYXRlIGEgbmV3IHBhdGNoIChpbml0aWFsaXppbmcgcmV0
IHdpdGggemVybyk/DQoNClllcywgcGxlYXNlIHNlbmQgYSBuZXcgcGF0Y2ggZml4aW5nIHRoaXMu
DQoNCkJ1dCBJIGRvbid0IGxpa2UgdGhhdCBtdWNoIHdpdGggdGhlIHN0eWxlIG9mIGluaXRpYWxp
c2luZyByZXQgdG8gemVybywNCml0IHRlbmRzIHRvIGhpZGUgdGhpbmdzLiBJbnN0ZWFkIG15IHBy
ZWZlcmVuY2UgaXMgc29tZXRoaW5nIGxpa2UgYmVsb3cNCndoZXJlIHRoZSBlcnJvciBoYW5kbGlu
ZyBpcyBtb3JlIGV4cGxpY2l0IGFuZCBlYXNpZXIgdG8gZmluZCB3aGVyZSBpdCdzDQpleGFjdGx5
IGZhaWxpbmcuIEJ1dCB0aGF0J3MganVzdCBhbiBleGFtcGxlIGhvdyBJIHdvdWxkIHRyeSB0byBz
b2x2ZSBpdCwNCml0IHN0aWxsIGxhY2tzIHRoZSBoYW5kbGluZyBvZiAtRUNBTkNFTCBldGMuDQoN
CmRpZmYgLS1naXQgYS9kcml2ZXJzL25ldC93aXJlbGVzcy9hdGgvYXRoMTBrL3NkaW8uYyBiL2Ry
aXZlcnMvbmV0L3dpcmVsZXNzL2F0aC9hdGgxMGsvc2Rpby5jDQppbmRleCA4NTllZDg3MGJkOTcu
LjE5YTUzZTU3NzkzMiAxMDA2NDQNCi0tLSBhL2RyaXZlcnMvbmV0L3dpcmVsZXNzL2F0aC9hdGgx
MGsvc2Rpby5jDQorKysgYi9kcml2ZXJzL25ldC93aXJlbGVzcy9hdGgvYXRoMTBrL3NkaW8uYw0K
QEAgLTY4OSw4ICs2ODksMTAgQEAgc3RhdGljIGludCBhdGgxMGtfc2Rpb19tYm94X3J4bXNnX3Bl
bmRpbmdfaGFuZGxlcihzdHJ1Y3QgYXRoMTBrICphciwNCiAJCSAqLw0KIAkJcmV0ID0gYXRoMTBr
X3NkaW9fbWJveF9yeF9hbGxvYyhhciwgbG9va2FoZWFkcywNCiAJCQkJCQluX2xvb2thaGVhZHMp
Ow0KLQkJaWYgKHJldCkNCi0JCQlicmVhazsNCisJCWlmIChyZXQpIHsNCisJCQlhdGgxMGtfd2Fy
bihhciwgImZhaWxlZCB0byAuLi4uOiAlZCIsIHJldCk7DQorCQkJcmV0dXJuIHJldDsNCisJCX0N
CiANCiAJCWlmIChhcl9zZGlvLT5uX3J4X3BrdHMgPj0gMikNCiAJCQkvKiBBIHJlY3YgYnVuZGxl
IHdhcyBkZXRlY3RlZCwgZm9yY2UgSVJRIHN0YXR1cw0KQEAgLTcwOSw4ICs3MTEsMTAgQEAgc3Rh
dGljIGludCBhdGgxMGtfc2Rpb19tYm94X3J4bXNnX3BlbmRpbmdfaGFuZGxlcihzdHJ1Y3QgYXRo
MTBrICphciwNCiAJCQkJCQkJICBsb29rYWhlYWRzLA0KIAkJCQkJCQkgICZuX2xvb2thaGVhZHMp
Ow0KIA0KLQkJaWYgKCFuX2xvb2thaGVhZHMgfHwgcmV0KQ0KLQkJCWJyZWFrOw0KKwkJaWYgKCFu
X2xvb2thaGVhZHMgfHwgcmV0KSB7DQorCQkJYXRoMTBrX3dhcm4oYXIsICJmYWlsZWQgdG8gLi4u
LiIpOw0KKwkJCXJldHVybiByZXQ7DQorCQl9DQogDQogCQkvKiBGb3IgU1lOQ0ggcHJvY2Vzc2lu
ZywgaWYgd2UgZ2V0IGhlcmUsIHdlIGFyZSBydW5uaW5nDQogCQkgKiB0aHJvdWdoIHRoZSBsb29w
IGFnYWluIGR1ZSB0byB1cGRhdGVkIGxvb2thaGVhZHMuIFNldA0KQEAgLTcyMSwxMSArNzI1LDcg
QEAgc3RhdGljIGludCBhdGgxMGtfc2Rpb19tYm94X3J4bXNnX3BlbmRpbmdfaGFuZGxlcihzdHJ1
Y3QgYXRoMTBrICphciwNCiAJCSpkb25lID0gZmFsc2U7DQogCX0NCiANCi0JaWYgKHJldCAmJiAo
cmV0ICE9IC1FQ0FOQ0VMRUQpKQ0KLQkJYXRoMTBrX3dhcm4oYXIsICJmYWlsZWQgdG8gZ2V0IHBl
bmRpbmcgcmVjdiBtZXNzYWdlczogJWRcbiIsDQotCQkJICAgIHJldCk7DQotDQotCXJldHVybiBy
ZXQ7DQorCXJldHVybiAwOw0KIH0NCiANCiBzdGF0aWMgaW50IGF0aDEwa19zZGlvX21ib3hfcHJv
Y19kYmdfaW50cihzdHJ1Y3QgYXRoMTBrICphcikNCg0KDQotLSANCkthbGxlIFZhbG8=

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
>>> =E2=80=98ath10k_sdio_mbox_rxmsg_pending_handler=E2=80=99:
>>> drivers/net/wireless/ath/ath10k/sdio.c:676: warning: =E2=80=98ret=E2=80=
=99 may be used
>>> uninitialized in this function
>>>
>>>> +
>>>> +       *done =3D true;
>>>> +
>>>> +       /* Copy the lookahead obtained from the HTC register table int=
o our
>>>> +        * temp array as a start value.
>>>> +        */
>>>> +       lookaheads[0] =3D msg_lookahead;
>>>> +
>>>> +       timeout =3D jiffies + SDIO_MBOX_PROCESSING_TIMEOUT_HZ;
>>>
>>> Although very unlikely due to the long timeout, if the code is preempte=
d 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> wrot=
e:
>> Erik Stromdahl <erik.stromdahl@gmail.com> writes:
>>>> With gcc 4.1.2:
>>>>
>>>> drivers/net/wireless/ath/ath10k/sdio.c: In function
>>>> =E2=80=98ath10k_sdio_mbox_rxmsg_pending_handler=E2=80=99:
>>>> drivers/net/wireless/ath/ath10k/sdio.c:676: warning: =E2=80=98ret=E2=
=80=99 may be used
>>>> uninitialized in this function
>>>>
>>>>> +
>>>>> +       *done =3D true;
>>>>> +
>>>>> +       /* Copy the lookahead obtained from the HTC register table in=
to our
>>>>> +        * temp array as a start value.
>>>>> +        */
>>>>> +       lookaheads[0] =3D msg_lookahead;
>>>>> +
>>>>> +       timeout =3D jiffies + SDIO_MBOX_PROCESSING_TIMEOUT_HZ;
>>>>
>>>> Although very unlikely due to the long timeout, if the code is preempt=
ed 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. Bu=
t
when I'm talking to journalists I just say "programmer" or something like t=
hat.
                                -- Linus Torvalds