All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kalle Valo <kvalo@qca.qualcomm.com>
To: Erik Stromdahl <erik.stromdahl@gmail.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>,
	Arnd Bergmann <arnd@arndb.de>,
	"ath10k@lists.infradead.org" <ath10k@lists.infradead.org>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: ath10k: ret used but uninitialized
Date: Fri, 7 Jul 2017 10:04:24 +0000	[thread overview]
Message-ID: <87bmowd0mh.fsf@kamboji.qca.qualcomm.com> (raw)
In-Reply-To: <c72d2e7a-195d-bcac-4ce5-f3051531664c@gmail.com> (Erik Stromdahl's message of "Thu, 6 Jul 2017 22:01:57 +0200")

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=

WARNING: multiple messages have this Message-ID (diff)
From: Kalle Valo <kvalo@qca.qualcomm.com>
To: Erik Stromdahl <erik.stromdahl@gmail.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>,
	Arnd Bergmann <arnd@arndb.de>,
	"ath10k@lists.infradead.org" <ath10k@lists.infradead.org>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: ath10k: ret used but uninitialized
Date: Fri, 7 Jul 2017 10:04:24 +0000	[thread overview]
Message-ID: <87bmowd0mh.fsf@kamboji.qca.qualcomm.com> (raw)
In-Reply-To: <c72d2e7a-195d-bcac-4ce5-f3051531664c@gmail.com> (Erik Stromdahl's message of "Thu, 6 Jul 2017 22:01:57 +0200")

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

WARNING: multiple messages have this Message-ID (diff)
From: Kalle Valo <kvalo@qca.qualcomm.com>
To: Erik Stromdahl <erik.stromdahl@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"ath10k@lists.infradead.org" <ath10k@lists.infradead.org>,
	Geert Uytterhoeven <geert@linux-m68k.org>
Subject: Re: ath10k: ret used but uninitialized
Date: Fri, 7 Jul 2017 10:04:24 +0000	[thread overview]
Message-ID: <87bmowd0mh.fsf@kamboji.qca.qualcomm.com> (raw)
In-Reply-To: <c72d2e7a-195d-bcac-4ce5-f3051531664c@gmail.com> (Erik Stromdahl's message of "Thu, 6 Jul 2017 22:01:57 +0200")

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
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

  reply	other threads:[~2017-07-07 10:04 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-06  8:27 ath10k: ret used but uninitialized (was: Re: ath10k: add initial SDIO support) Geert Uytterhoeven
2017-07-06  8:27 ` Geert Uytterhoeven
2017-07-06  8:27 ` Geert Uytterhoeven
2017-07-06 20:01 ` Erik Stromdahl
2017-07-06 20:01   ` Erik Stromdahl
2017-07-06 20:01   ` Erik Stromdahl
2017-07-07 10:04   ` Kalle Valo [this message]
2017-07-07 10:04     ` ath10k: ret used but uninitialized Kalle Valo
2017-07-07 10:04     ` Kalle Valo
2017-07-07 14:14     ` Arnd Bergmann
2017-07-07 14:14       ` Arnd Bergmann
2017-07-07 14:14       ` Arnd Bergmann
2017-07-07 14:15       ` Geert Uytterhoeven
2017-07-07 14:15         ` Geert Uytterhoeven
2017-07-07 14:15         ` Geert Uytterhoeven
2017-07-07 14:15         ` Geert Uytterhoeven

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87bmowd0mh.fsf@kamboji.qca.qualcomm.com \
    --to=kvalo@qca.qualcomm.com \
    --cc=arnd@arndb.de \
    --cc=ath10k@lists.infradead.org \
    --cc=erik.stromdahl@gmail.com \
    --cc=geert@linux-m68k.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.