On Wed, Jul 5, 2017 at 9:52 PM, Linux Kernel Mailing List
<[email protected]> wrote:
> Web: https://git.kernel.org/torvalds/c/d96db25d20256208ce47d71b9f6=
73a1de4c6fd7e
> Commit: d96db25d20256208ce47d71b9f673a1de4c6fd7e
> Parent: f008d1537bf88396cf41a7c7a831e3acd1ee92a1
> Refname: refs/heads/master
> Author: Erik Stromdahl <[email protected]>
> AuthorDate: Wed Apr 26 12:18:00 2017 +0300
> Committer: Kalle Valo <[email protected]>
> 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 <[email protected]>
> [[email protected]: refactoring, cleanup, commit log]
> Signed-off-by: Kalle Valo <[email protected]>
> --- /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
> 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 -- [email protected]
>
> 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
>
On Fri, Jul 7, 2017 at 12:04 PM, Kalle Valo <[email protected]> wrote:
> Erik Stromdahl <[email protected]> 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
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=
Hi Arnd,
On Fri, Jul 7, 2017 at 4:14 PM, Arnd Bergmann <[email protected]> wrote:
> On Fri, Jul 7, 2017 at 12:04 PM, Kalle Valo <[email protected]> wrot=
e:
>> Erik Stromdahl <[email protected]> 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