* [PATCH 1/8] staging: wfx: improve error handling of hif_join()
2020-10-09 17:12 [PATCH 0/8] staging: wfx: fix issues reported by Smatch Jerome Pouiller
@ 2020-10-09 17:13 ` Jerome Pouiller
2020-10-09 17:13 ` [PATCH 2/8] staging: wfx: check memory allocation Jerome Pouiller
` (6 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Jerome Pouiller @ 2020-10-09 17:13 UTC (permalink / raw)
To: devel, linux-wireless
Cc: netdev, linux-kernel, Kalle Valo, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
From: Jérôme Pouiller <jerome.pouiller@silabs.com>
Smatch complains:
hif_tx.c:319 hif_join() error: we previously assumed 'channel' could be null (see line 315)
311 if (!hif)
312 return -ENOMEM;
313 body->infrastructure_bss_mode = !conf->ibss_joined;
314 body->short_preamble = conf->use_short_preamble;
315 if (channel && channel->flags & IEEE80211_CHAN_NO_IR)
^^^^^^^
316 body->probe_for_join = 0;
317 else
318 body->probe_for_join = 1;
319 body->channel_number = channel->hw_value;
^^^^^^^^^^^^^^^^^
320 body->beacon_interval = cpu_to_le32(conf->beacon_int);
321 body->basic_rate_set =
Indeed, channel can't be NULL (else I would have seen plenty of Ooops
this past year). This patch explicitly claims this restriction.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
drivers/staging/wfx/hif_tx.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index e61cc2486761..63b437261eb7 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -308,11 +308,13 @@ int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
WARN_ON(!conf->basic_rates);
WARN_ON(sizeof(body->ssid) < ssidlen);
WARN(!conf->ibss_joined && !ssidlen, "joining an unknown BSS");
+ if (WARN_ON(!channel))
+ return -EINVAL;
if (!hif)
return -ENOMEM;
body->infrastructure_bss_mode = !conf->ibss_joined;
body->short_preamble = conf->use_short_preamble;
- if (channel && channel->flags & IEEE80211_CHAN_NO_IR)
+ if (channel->flags & IEEE80211_CHAN_NO_IR)
body->probe_for_join = 0;
else
body->probe_for_join = 1;
--
2.28.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 2/8] staging: wfx: check memory allocation
2020-10-09 17:12 [PATCH 0/8] staging: wfx: fix issues reported by Smatch Jerome Pouiller
2020-10-09 17:13 ` [PATCH 1/8] staging: wfx: improve error handling of hif_join() Jerome Pouiller
@ 2020-10-09 17:13 ` Jerome Pouiller
2020-10-09 18:51 ` Kalle Valo
2020-10-09 17:13 ` [PATCH 3/8] staging: wfx: standardize the error when vif does not exist Jerome Pouiller
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Jerome Pouiller @ 2020-10-09 17:13 UTC (permalink / raw)
To: devel, linux-wireless
Cc: netdev, linux-kernel, Kalle Valo, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
From: Jérôme Pouiller <jerome.pouiller@silabs.com>
Smatch complains:
main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
227 tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
228 ret = wfx_send_pds(wdev, tmp_buf, pds->size);
^^^^^^^
229 kfree(tmp_buf);
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
drivers/staging/wfx/main.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
index df11c091e094..a8dc2c033410 100644
--- a/drivers/staging/wfx/main.c
+++ b/drivers/staging/wfx/main.c
@@ -222,12 +222,18 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
if (ret) {
dev_err(wdev->dev, "can't load PDS file %s\n",
wdev->pdata.file_pds);
- return ret;
+ goto err1;
}
tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
+ if (!tmp_buf) {
+ ret = -ENOMEM;
+ goto err2;
+ }
ret = wfx_send_pds(wdev, tmp_buf, pds->size);
kfree(tmp_buf);
+err2:
release_firmware(pds);
+err1:
return ret;
}
--
2.28.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 2/8] staging: wfx: check memory allocation
2020-10-09 17:13 ` [PATCH 2/8] staging: wfx: check memory allocation Jerome Pouiller
@ 2020-10-09 18:51 ` Kalle Valo
2020-10-10 12:07 ` Jérôme Pouiller
0 siblings, 1 reply; 18+ messages in thread
From: Kalle Valo @ 2020-10-09 18:51 UTC (permalink / raw)
To: Jerome Pouiller
Cc: devel, netdev, linux-wireless, linux-kernel, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
Jerome Pouiller <Jerome.Pouiller@silabs.com> writes:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
>
> Smatch complains:
>
> main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
> 227 tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> 228 ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> ^^^^^^^
> 229 kfree(tmp_buf);
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> ---
> drivers/staging/wfx/main.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> index df11c091e094..a8dc2c033410 100644
> --- a/drivers/staging/wfx/main.c
> +++ b/drivers/staging/wfx/main.c
> @@ -222,12 +222,18 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> if (ret) {
> dev_err(wdev->dev, "can't load PDS file %s\n",
> wdev->pdata.file_pds);
> - return ret;
> + goto err1;
> }
> tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> + if (!tmp_buf) {
> + ret = -ENOMEM;
> + goto err2;
> + }
> ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> kfree(tmp_buf);
> +err2:
> release_firmware(pds);
> +err1:
> return ret;
> }
A minor style issue but using more descriptive error labels make the
code more readable and maintainable, especially in a bigger function.
For example, err2 could be called err_release_firmware.
And actually err1 could be removed and the goto replaced with just
"return ret;". Then err2 could be renamed to a simple err.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/8] staging: wfx: check memory allocation
2020-10-09 18:51 ` Kalle Valo
@ 2020-10-10 12:07 ` Jérôme Pouiller
2020-10-10 13:18 ` Dan Carpenter
0 siblings, 1 reply; 18+ messages in thread
From: Jérôme Pouiller @ 2020-10-10 12:07 UTC (permalink / raw)
To: Kalle Valo
Cc: devel, netdev, linux-wireless, linux-kernel, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
On Friday 9 October 2020 20:51:01 CEST Kalle Valo wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> Jerome Pouiller <Jerome.Pouiller@silabs.com> writes:
>
> > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> >
> > Smatch complains:
> >
> > main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
> > 227 tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> > 228 ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > ^^^^^^^
> > 229 kfree(tmp_buf);
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > ---
> > drivers/staging/wfx/main.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> > index df11c091e094..a8dc2c033410 100644
> > --- a/drivers/staging/wfx/main.c
> > +++ b/drivers/staging/wfx/main.c
> > @@ -222,12 +222,18 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> > if (ret) {
> > dev_err(wdev->dev, "can't load PDS file %s\n",
> > wdev->pdata.file_pds);
> > - return ret;
> > + goto err1;
> > }
> > tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> > + if (!tmp_buf) {
> > + ret = -ENOMEM;
> > + goto err2;
> > + }
> > ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > kfree(tmp_buf);
> > +err2:
> > release_firmware(pds);
> > +err1:
> > return ret;
> > }
>
> A minor style issue but using more descriptive error labels make the
> code more readable and maintainable, especially in a bigger function.
> For example, err2 could be called err_release_firmware.
>
> And actually err1 could be removed and the goto replaced with just
> "return ret;". Then err2 could be renamed to a simple err.
It was the case in the initial code. However, I have preferred to not
mix 'return' and 'goto' inside the same function. Probably a matter of
taste.
Greg has already applied the series, but I don't forget this review. I
will take it into account in the series I am going to send you (probably
in the v2, in order to not defer the v1).
--
Jérôme Pouiller
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/8] staging: wfx: check memory allocation
2020-10-10 12:07 ` Jérôme Pouiller
@ 2020-10-10 13:18 ` Dan Carpenter
2020-10-10 13:54 ` Dan Carpenter
0 siblings, 1 reply; 18+ messages in thread
From: Dan Carpenter @ 2020-10-10 13:18 UTC (permalink / raw)
To: Jérôme Pouiller
Cc: devel, netdev, linux-wireless, linux-kernel, Greg Kroah-Hartman,
David S . Miller, Kalle Valo
On Sat, Oct 10, 2020 at 02:07:13PM +0200, Jérôme Pouiller wrote:
> On Friday 9 October 2020 20:51:01 CEST Kalle Valo wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> >
> >
> > Jerome Pouiller <Jerome.Pouiller@silabs.com> writes:
> >
> > > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > >
> > > Smatch complains:
> > >
> > > main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
> > > 227 tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> > > 228 ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > > ^^^^^^^
> > > 229 kfree(tmp_buf);
> > >
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > > ---
> > > drivers/staging/wfx/main.c | 8 +++++++-
> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> > > index df11c091e094..a8dc2c033410 100644
> > > --- a/drivers/staging/wfx/main.c
> > > +++ b/drivers/staging/wfx/main.c
> > > @@ -222,12 +222,18 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> > > if (ret) {
> > > dev_err(wdev->dev, "can't load PDS file %s\n",
> > > wdev->pdata.file_pds);
> > > - return ret;
> > > + goto err1;
> > > }
> > > tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> > > + if (!tmp_buf) {
> > > + ret = -ENOMEM;
> > > + goto err2;
> > > + }
> > > ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > > kfree(tmp_buf);
> > > +err2:
> > > release_firmware(pds);
> > > +err1:
> > > return ret;
> > > }
> >
> > A minor style issue but using more descriptive error labels make the
> > code more readable and maintainable, especially in a bigger function.
> > For example, err2 could be called err_release_firmware.
> >
> > And actually err1 could be removed and the goto replaced with just
> > "return ret;". Then err2 could be renamed to a simple err.
>
> It was the case in the initial code. However, I have preferred to not
> mix 'return' and 'goto' inside the same function. Probably a matter of
> taste.
>
Ideally you can read a function from top to bottom and understand with
out skipping around. Imagine if novels were written like that "goto
bottom_of_page;" but then at the bottom it just said "Just kidding".
"return ret;" is more readable than "goto err;"
These sorts of rules where "there is only one return per function" are
meant to make people think about cleanup before returning. But most of
my work is in error handling code and it doesn't help. If people don't
think about cleanup, changing the style won't make them start thinking
about it. There was one driver which was written with locked code
indented one tab and the inventor of that style still introduced a
locking bug in his code.
spin_lock(); {
frob();
frob();
if (ret)
return ret; // <-- forgot to unlock;
frob();
} spin_unlock();
Btw, I have created a new Smatch check to find unwind bugs. It's called
check_unwind.c and it's easy to add new alloc/free pairings to that
code. This is the best way to prevent unwind bugs. The style changes
don't make a measurable difference in real life and they make the code
messy.
And GW-BASIC label names are a pox upon the earth.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/8] staging: wfx: check memory allocation
2020-10-10 13:18 ` Dan Carpenter
@ 2020-10-10 13:54 ` Dan Carpenter
0 siblings, 0 replies; 18+ messages in thread
From: Dan Carpenter @ 2020-10-10 13:54 UTC (permalink / raw)
To: Jérôme Pouiller
Cc: devel, netdev, linux-wireless, linux-kernel, Greg Kroah-Hartman,
David S . Miller, Kalle Valo
On Sat, Oct 10, 2020 at 04:18:11PM +0300, Dan Carpenter wrote:
> On Sat, Oct 10, 2020 at 02:07:13PM +0200, Jérôme Pouiller wrote:
> > On Friday 9 October 2020 20:51:01 CEST Kalle Valo wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > >
> > >
> > > Jerome Pouiller <Jerome.Pouiller@silabs.com> writes:
> > >
> > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > > >
> > > > Smatch complains:
> > > >
> > > > main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
> > > > 227 tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> > > > 228 ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > > > ^^^^^^^
> > > > 229 kfree(tmp_buf);
> > > >
> > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > > > ---
> > > > drivers/staging/wfx/main.c | 8 +++++++-
> > > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> > > > index df11c091e094..a8dc2c033410 100644
> > > > --- a/drivers/staging/wfx/main.c
> > > > +++ b/drivers/staging/wfx/main.c
> > > > @@ -222,12 +222,18 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> > > > if (ret) {
> > > > dev_err(wdev->dev, "can't load PDS file %s\n",
> > > > wdev->pdata.file_pds);
> > > > - return ret;
> > > > + goto err1;
> > > > }
> > > > tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> > > > + if (!tmp_buf) {
> > > > + ret = -ENOMEM;
> > > > + goto err2;
> > > > + }
> > > > ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > > > kfree(tmp_buf);
> > > > +err2:
> > > > release_firmware(pds);
> > > > +err1:
> > > > return ret;
> > > > }
> > >
> > > A minor style issue but using more descriptive error labels make the
> > > code more readable and maintainable, especially in a bigger function.
> > > For example, err2 could be called err_release_firmware.
> > >
> > > And actually err1 could be removed and the goto replaced with just
> > > "return ret;". Then err2 could be renamed to a simple err.
> >
> > It was the case in the initial code. However, I have preferred to not
> > mix 'return' and 'goto' inside the same function. Probably a matter of
> > taste.
> >
>
> Ideally you can read a function from top to bottom and understand with
> out skipping around. Imagine if novels were written like that "goto
> bottom_of_page;" but then at the bottom it just said "Just kidding".
> "return ret;" is more readable than "goto err;"
More unasked for exposition: "goto err;" is too vague. It could be one
of three things. 1) Do nothing (like this code). 2) Do something
specific (choose a better name like goto unlock). 3) Do everything.
Do everything code is the most buggy style of error handling.
The common bug introduced by type 1 and 2 are "Forgot to set the error
code" bugs. Type 3 is a whole nother level of bugginess. Too much bugs
to explain.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/8] staging: wfx: standardize the error when vif does not exist
2020-10-09 17:12 [PATCH 0/8] staging: wfx: fix issues reported by Smatch Jerome Pouiller
2020-10-09 17:13 ` [PATCH 1/8] staging: wfx: improve error handling of hif_join() Jerome Pouiller
2020-10-09 17:13 ` [PATCH 2/8] staging: wfx: check memory allocation Jerome Pouiller
@ 2020-10-09 17:13 ` Jerome Pouiller
2020-10-09 18:52 ` Kalle Valo
2020-10-09 17:13 ` [PATCH 4/8] staging: wfx: wfx_init_common() returns NULL on error Jerome Pouiller
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Jerome Pouiller @ 2020-10-09 17:13 UTC (permalink / raw)
To: devel, linux-wireless
Cc: netdev, linux-kernel, Kalle Valo, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
From: Jérôme Pouiller <jerome.pouiller@silabs.com>
Smatch complains:
drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif'
drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif'
Indeed, if the vif id returned by the device does not exist anymore,
wdev_to_wvif() could return NULL.
In add, the error is not handled uniformly in the code, sometime a
WARN() is displayed but code continue, sometime a dev_warn() is
displayed, sometime it is just not tested, ...
This patch standardize that.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
drivers/staging/wfx/data_tx.c | 5 ++++-
drivers/staging/wfx/hif_rx.c | 34 ++++++++++++++++++++++++----------
drivers/staging/wfx/sta.c | 4 ++++
3 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
index b4d5dd3d2d23..8db0be08daf8 100644
--- a/drivers/staging/wfx/data_tx.c
+++ b/drivers/staging/wfx/data_tx.c
@@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb)
sizeof(struct hif_req_tx) +
req->fc_offset;
- WARN_ON(!wvif);
+ if (!wvif) {
+ pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__);
+ return;
+ }
wfx_tx_policy_put(wvif, req->retry_policy_index);
skb_pull(skb, offset);
ieee80211_tx_status_irqsafe(wvif->wdev->hw, skb);
diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c
index d6dfab094b03..ca09467cba05 100644
--- a/drivers/staging/wfx/hif_rx.c
+++ b/drivers/staging/wfx/hif_rx.c
@@ -110,9 +110,9 @@ static int hif_receive_indication(struct wfx_dev *wdev,
const struct hif_ind_rx *body = buf;
if (!wvif) {
- dev_warn(wdev->dev, "ignore rx data for non-existent vif %d\n",
- hif->interface);
- return 0;
+ dev_warn(wdev->dev, "%s: ignore rx data for non-existent vif %d\n",
+ __func__, hif->interface);
+ return -EIO;
}
skb_pull(skb, sizeof(struct hif_msg) + sizeof(struct hif_ind_rx));
wfx_rx_cb(wvif, body, skb);
@@ -128,8 +128,8 @@ static int hif_event_indication(struct wfx_dev *wdev,
int type = le32_to_cpu(body->event_id);
if (!wvif) {
- dev_warn(wdev->dev, "received event for non-existent vif\n");
- return 0;
+ dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+ return -EIO;
}
switch (type) {
@@ -161,7 +161,10 @@ static int hif_pm_mode_complete_indication(struct wfx_dev *wdev,
{
struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface);
- WARN_ON(!wvif);
+ if (!wvif) {
+ dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+ return -EIO;
+ }
complete(&wvif->set_pm_mode_complete);
return 0;
@@ -173,7 +176,11 @@ static int hif_scan_complete_indication(struct wfx_dev *wdev,
{
struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface);
- WARN_ON(!wvif);
+ if (!wvif) {
+ dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+ return -EIO;
+ }
+
wfx_scan_complete(wvif);
return 0;
@@ -185,7 +192,10 @@ static int hif_join_complete_indication(struct wfx_dev *wdev,
{
struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface);
- WARN_ON(!wvif);
+ if (!wvif) {
+ dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+ return -EIO;
+ }
dev_warn(wdev->dev, "unattended JoinCompleteInd\n");
return 0;
@@ -195,11 +205,15 @@ static int hif_suspend_resume_indication(struct wfx_dev *wdev,
const struct hif_msg *hif,
const void *buf)
{
- struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface);
const struct hif_ind_suspend_resume_tx *body = buf;
+ struct wfx_vif *wvif;
if (body->bc_mc_only) {
- WARN_ON(!wvif);
+ wvif = wdev_to_wvif(wdev, hif->interface);
+ if (!wvif) {
+ dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+ return -EIO;
+ }
if (body->resume)
wfx_suspend_resume_mc(wvif, STA_NOTIFY_AWAKE);
else
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index a246f0d1d6e9..2320a81eae0b 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -619,6 +619,10 @@ int wfx_set_tim(struct ieee80211_hw *hw, struct ieee80211_sta *sta, bool set)
struct wfx_sta_priv *sta_dev = (struct wfx_sta_priv *)&sta->drv_priv;
struct wfx_vif *wvif = wdev_to_wvif(wdev, sta_dev->vif_id);
+ if (!wvif) {
+ dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__);
+ return -EIO;
+ }
schedule_work(&wvif->update_tim_work);
return 0;
}
--
2.28.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 3/8] staging: wfx: standardize the error when vif does not exist
2020-10-09 17:13 ` [PATCH 3/8] staging: wfx: standardize the error when vif does not exist Jerome Pouiller
@ 2020-10-09 18:52 ` Kalle Valo
2020-10-10 12:22 ` Jérôme Pouiller
0 siblings, 1 reply; 18+ messages in thread
From: Kalle Valo @ 2020-10-09 18:52 UTC (permalink / raw)
To: Jerome Pouiller
Cc: devel, netdev, linux-wireless, linux-kernel, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
Jerome Pouiller <Jerome.Pouiller@silabs.com> writes:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
>
> Smatch complains:
>
> drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif'
> drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif'
>
> Indeed, if the vif id returned by the device does not exist anymore,
> wdev_to_wvif() could return NULL.
>
> In add, the error is not handled uniformly in the code, sometime a
> WARN() is displayed but code continue, sometime a dev_warn() is
> displayed, sometime it is just not tested, ...
>
> This patch standardize that.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> ---
> drivers/staging/wfx/data_tx.c | 5 ++++-
> drivers/staging/wfx/hif_rx.c | 34 ++++++++++++++++++++++++----------
> drivers/staging/wfx/sta.c | 4 ++++
> 3 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
> index b4d5dd3d2d23..8db0be08daf8 100644
> --- a/drivers/staging/wfx/data_tx.c
> +++ b/drivers/staging/wfx/data_tx.c
> @@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb)
> sizeof(struct hif_req_tx) +
> req->fc_offset;
>
> - WARN_ON(!wvif);
> + if (!wvif) {
> + pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__);
> + return;
> + }
I'm not really a fan of using function names in warning or error
messages as it clutters the log. In debug messages I think they are ok.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/8] staging: wfx: standardize the error when vif does not exist
2020-10-09 18:52 ` Kalle Valo
@ 2020-10-10 12:22 ` Jérôme Pouiller
2020-10-10 12:40 ` Greg Kroah-Hartman
0 siblings, 1 reply; 18+ messages in thread
From: Jérôme Pouiller @ 2020-10-10 12:22 UTC (permalink / raw)
To: Kalle Valo
Cc: devel, netdev, linux-wireless, linux-kernel, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
On Friday 9 October 2020 20:52:47 CEST Kalle Valo wrote:
> Jerome Pouiller <Jerome.Pouiller@silabs.com> writes:
>
> > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> >
> > Smatch complains:
> >
> > drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif'
> > drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif'
> >
> > Indeed, if the vif id returned by the device does not exist anymore,
> > wdev_to_wvif() could return NULL.
> >
> > In add, the error is not handled uniformly in the code, sometime a
> > WARN() is displayed but code continue, sometime a dev_warn() is
> > displayed, sometime it is just not tested, ...
> >
> > This patch standardize that.
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > ---
> > drivers/staging/wfx/data_tx.c | 5 ++++-
> > drivers/staging/wfx/hif_rx.c | 34 ++++++++++++++++++++++++----------
> > drivers/staging/wfx/sta.c | 4 ++++
> > 3 files changed, 32 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
> > index b4d5dd3d2d23..8db0be08daf8 100644
> > --- a/drivers/staging/wfx/data_tx.c
> > +++ b/drivers/staging/wfx/data_tx.c
> > @@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb)
> > sizeof(struct hif_req_tx) +
> > req->fc_offset;
> >
> > - WARN_ON(!wvif);
> > + if (!wvif) {
> > + pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__);
> > + return;
> > + }
>
> I'm not really a fan of using function names in warning or error
> messages as it clutters the log. In debug messages I think they are ok.
In the initial code, I used WARN() that far more clutters the log (I
have stated that a backtrace won't provide any useful information, so
pr_warn() was better suited).
In add, in my mind, these warnings are debug messages. If they appears,
the user should probably report a bug.
Finally, in this patch, I use the same message several times (ok, not
this particular one). So the function name is a way to differentiate
them.
--
Jérôme Pouiller
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/8] staging: wfx: standardize the error when vif does not exist
2020-10-10 12:22 ` Jérôme Pouiller
@ 2020-10-10 12:40 ` Greg Kroah-Hartman
2020-10-10 13:29 ` Jérôme Pouiller
0 siblings, 1 reply; 18+ messages in thread
From: Greg Kroah-Hartman @ 2020-10-10 12:40 UTC (permalink / raw)
To: Jérôme Pouiller
Cc: devel, netdev, linux-wireless, linux-kernel, Dan Carpenter,
David S . Miller, Kalle Valo
On Sat, Oct 10, 2020 at 02:22:13PM +0200, Jérôme Pouiller wrote:
> On Friday 9 October 2020 20:52:47 CEST Kalle Valo wrote:
> > Jerome Pouiller <Jerome.Pouiller@silabs.com> writes:
> >
> > > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > >
> > > Smatch complains:
> > >
> > > drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif'
> > > drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif'
> > >
> > > Indeed, if the vif id returned by the device does not exist anymore,
> > > wdev_to_wvif() could return NULL.
> > >
> > > In add, the error is not handled uniformly in the code, sometime a
> > > WARN() is displayed but code continue, sometime a dev_warn() is
> > > displayed, sometime it is just not tested, ...
> > >
> > > This patch standardize that.
> > >
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > > ---
> > > drivers/staging/wfx/data_tx.c | 5 ++++-
> > > drivers/staging/wfx/hif_rx.c | 34 ++++++++++++++++++++++++----------
> > > drivers/staging/wfx/sta.c | 4 ++++
> > > 3 files changed, 32 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
> > > index b4d5dd3d2d23..8db0be08daf8 100644
> > > --- a/drivers/staging/wfx/data_tx.c
> > > +++ b/drivers/staging/wfx/data_tx.c
> > > @@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb)
> > > sizeof(struct hif_req_tx) +
> > > req->fc_offset;
> > >
> > > - WARN_ON(!wvif);
> > > + if (!wvif) {
> > > + pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__);
> > > + return;
> > > + }
> >
> > I'm not really a fan of using function names in warning or error
> > messages as it clutters the log. In debug messages I think they are ok.
>
> In the initial code, I used WARN() that far more clutters the log (I
> have stated that a backtrace won't provide any useful information, so
> pr_warn() was better suited).
>
> In add, in my mind, these warnings are debug messages. If they appears,
> the user should probably report a bug.
>
> Finally, in this patch, I use the same message several times (ok, not
> this particular one). So the function name is a way to differentiate
> them.
You should use dev_*() for these, that way you can properly determine
the exact device as well.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/8] staging: wfx: standardize the error when vif does not exist
2020-10-10 12:40 ` Greg Kroah-Hartman
@ 2020-10-10 13:29 ` Jérôme Pouiller
0 siblings, 0 replies; 18+ messages in thread
From: Jérôme Pouiller @ 2020-10-10 13:29 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel, netdev, linux-wireless, linux-kernel, Dan Carpenter,
David S . Miller, Kalle Valo
On Saturday 10 October 2020 14:40:34 CEST Greg Kroah-Hartman wrote:
> On Sat, Oct 10, 2020 at 02:22:13PM +0200, Jérôme Pouiller wrote:
> > On Friday 9 October 2020 20:52:47 CEST Kalle Valo wrote:
> > > Jerome Pouiller <Jerome.Pouiller@silabs.com> writes:
> > >
> > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > > >
> > > > Smatch complains:
> > > >
> > > > drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif'
> > > > drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif'
> > > >
> > > > Indeed, if the vif id returned by the device does not exist anymore,
> > > > wdev_to_wvif() could return NULL.
> > > >
> > > > In add, the error is not handled uniformly in the code, sometime a
> > > > WARN() is displayed but code continue, sometime a dev_warn() is
> > > > displayed, sometime it is just not tested, ...
> > > >
> > > > This patch standardize that.
> > > >
> > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > > > ---
> > > > drivers/staging/wfx/data_tx.c | 5 ++++-
> > > > drivers/staging/wfx/hif_rx.c | 34 ++++++++++++++++++++++++----------
> > > > drivers/staging/wfx/sta.c | 4 ++++
> > > > 3 files changed, 32 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
> > > > index b4d5dd3d2d23..8db0be08daf8 100644
> > > > --- a/drivers/staging/wfx/data_tx.c
> > > > +++ b/drivers/staging/wfx/data_tx.c
> > > > @@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb)
> > > > sizeof(struct hif_req_tx) +
> > > > req->fc_offset;
> > > >
> > > > - WARN_ON(!wvif);
> > > > + if (!wvif) {
> > > > + pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__);
> > > > + return;
> > > > + }
> > >
> > > I'm not really a fan of using function names in warning or error
> > > messages as it clutters the log. In debug messages I think they are ok.
> >
> > In the initial code, I used WARN() that far more clutters the log (I
> > have stated that a backtrace won't provide any useful information, so
> > pr_warn() was better suited).
> >
> > In add, in my mind, these warnings are debug messages. If they appears,
> > the user should probably report a bug.
> >
> > Finally, in this patch, I use the same message several times (ok, not
> > this particular one). So the function name is a way to differentiate
> > them.
>
> You should use dev_*() for these, that way you can properly determine
> the exact device as well.
Totally agree. I initially did that. However, the device is a field of
wvif which is NULL in this case.
I could have changed the code to get the real pointer to the device. But
I didn't want to clutter the code just for a debug message (and also
because I was a bit lazy).
--
Jérôme Pouiller
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/8] staging: wfx: wfx_init_common() returns NULL on error
2020-10-09 17:12 [PATCH 0/8] staging: wfx: fix issues reported by Smatch Jerome Pouiller
` (2 preceding siblings ...)
2020-10-09 17:13 ` [PATCH 3/8] staging: wfx: standardize the error when vif does not exist Jerome Pouiller
@ 2020-10-09 17:13 ` Jerome Pouiller
2020-10-09 17:13 ` [PATCH 5/8] staging: wfx: increase robustness of hif_generic_confirm() Jerome Pouiller
` (3 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Jerome Pouiller @ 2020-10-09 17:13 UTC (permalink / raw)
To: devel, linux-wireless
Cc: netdev, linux-kernel, Kalle Valo, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
From: Jérôme Pouiller <jerome.pouiller@silabs.com>
Smatch complains:
bus_spi.c:228 wfx_spi_probe() warn: 'bus->core' could be an error pointer
bus_sdio.c:221 wfx_sdio_probe() warn: 'bus->core' could be an error pointer
bus->core contains the result of wfx_init_common(). With this patch,
wfx_init_common() returns a valid pointer or NULL.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
drivers/staging/wfx/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
index a8dc2c033410..e7bc1988124a 100644
--- a/drivers/staging/wfx/main.c
+++ b/drivers/staging/wfx/main.c
@@ -309,7 +309,7 @@ struct wfx_dev *wfx_init_common(struct device *dev,
wdev->pdata.gpio_wakeup = devm_gpiod_get_optional(dev, "wakeup",
GPIOD_OUT_LOW);
if (IS_ERR(wdev->pdata.gpio_wakeup))
- return ERR_CAST(wdev->pdata.gpio_wakeup);
+ return NULL;
if (wdev->pdata.gpio_wakeup)
gpiod_set_consumer_name(wdev->pdata.gpio_wakeup, "wfx wakeup");
--
2.28.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 5/8] staging: wfx: increase robustness of hif_generic_confirm()
2020-10-09 17:12 [PATCH 0/8] staging: wfx: fix issues reported by Smatch Jerome Pouiller
` (3 preceding siblings ...)
2020-10-09 17:13 ` [PATCH 4/8] staging: wfx: wfx_init_common() returns NULL on error Jerome Pouiller
@ 2020-10-09 17:13 ` Jerome Pouiller
2020-10-09 17:13 ` [PATCH 6/8] staging: wfx: gpiod_get_value() can return an error Jerome Pouiller
` (2 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Jerome Pouiller @ 2020-10-09 17:13 UTC (permalink / raw)
To: devel, linux-wireless
Cc: netdev, linux-kernel, Kalle Valo, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
From: Jérôme Pouiller <jerome.pouiller@silabs.com>
Smatch complains:
drivers/staging/wfx/hif_rx.c:26 hif_generic_confirm() warn: negative user subtract: 0-u16max - 4
20 static int hif_generic_confirm(struct wfx_dev *wdev,
21 const struct hif_msg *hif, const void *buf)
22 {
23 // All confirm messages start with status
24 int status = le32_to_cpup((__le32 *)buf);
25 int cmd = hif->id;
26 int len = le16_to_cpu(hif->len) - 4; // drop header
^^^^^
27
28 WARN(!mutex_is_locked(&wdev->hif_cmd.lock), "data locking error");
In fact, rx_helper() already make the necessary checks on the value of
hif->len. Never mind, add an explicit check to make Smatch happy.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
drivers/staging/wfx/hif_rx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c
index ca09467cba05..2d4265257112 100644
--- a/drivers/staging/wfx/hif_rx.c
+++ b/drivers/staging/wfx/hif_rx.c
@@ -40,10 +40,10 @@ static int hif_generic_confirm(struct wfx_dev *wdev,
}
if (wdev->hif_cmd.buf_recv) {
- if (wdev->hif_cmd.len_recv >= len)
+ if (wdev->hif_cmd.len_recv >= len && len > 0)
memcpy(wdev->hif_cmd.buf_recv, buf, len);
else
- status = -ENOMEM;
+ status = -EIO;
}
wdev->hif_cmd.ret = status;
--
2.28.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 6/8] staging: wfx: gpiod_get_value() can return an error
2020-10-09 17:12 [PATCH 0/8] staging: wfx: fix issues reported by Smatch Jerome Pouiller
` (4 preceding siblings ...)
2020-10-09 17:13 ` [PATCH 5/8] staging: wfx: increase robustness of hif_generic_confirm() Jerome Pouiller
@ 2020-10-09 17:13 ` Jerome Pouiller
2020-10-09 17:13 ` [PATCH 7/8] staging: wfx: drop unicode characters from strings Jerome Pouiller
2020-10-09 17:13 ` [PATCH 8/8] staging: wfx: improve robustness of wfx_get_hw_rate() Jerome Pouiller
7 siblings, 0 replies; 18+ messages in thread
From: Jerome Pouiller @ 2020-10-09 17:13 UTC (permalink / raw)
To: devel, linux-wireless
Cc: netdev, linux-kernel, Kalle Valo, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
From: Jérôme Pouiller <jerome.pouiller@silabs.com>
Smatch complains:
hif_rx.c:98 hif_wakeup_indication() warn: 'gpiod_get_value(wdev->pdata.gpio_wakeup)' returns positive and negative
bh.c:24 device_wakeup() warn: 'gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup)' returns positive and negative
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
drivers/staging/wfx/bh.c | 2 +-
drivers/staging/wfx/hif_rx.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
index 586b769c0446..2ffa587aefaa 100644
--- a/drivers/staging/wfx/bh.c
+++ b/drivers/staging/wfx/bh.c
@@ -21,7 +21,7 @@ static void device_wakeup(struct wfx_dev *wdev)
if (!wdev->pdata.gpio_wakeup)
return;
- if (gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup))
+ if (gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup) >= 0)
return;
if (wfx_api_older_than(wdev, 1, 4)) {
diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c
index 2d4265257112..f99921e76059 100644
--- a/drivers/staging/wfx/hif_rx.c
+++ b/drivers/staging/wfx/hif_rx.c
@@ -94,8 +94,8 @@ static int hif_startup_indication(struct wfx_dev *wdev,
static int hif_wakeup_indication(struct wfx_dev *wdev,
const struct hif_msg *hif, const void *buf)
{
- if (!wdev->pdata.gpio_wakeup
- || !gpiod_get_value(wdev->pdata.gpio_wakeup)) {
+ if (!wdev->pdata.gpio_wakeup ||
+ gpiod_get_value(wdev->pdata.gpio_wakeup) == 0) {
dev_warn(wdev->dev, "unexpected wake-up indication\n");
return -EIO;
}
--
2.28.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 7/8] staging: wfx: drop unicode characters from strings
2020-10-09 17:12 [PATCH 0/8] staging: wfx: fix issues reported by Smatch Jerome Pouiller
` (5 preceding siblings ...)
2020-10-09 17:13 ` [PATCH 6/8] staging: wfx: gpiod_get_value() can return an error Jerome Pouiller
@ 2020-10-09 17:13 ` Jerome Pouiller
2020-10-09 17:13 ` [PATCH 8/8] staging: wfx: improve robustness of wfx_get_hw_rate() Jerome Pouiller
7 siblings, 0 replies; 18+ messages in thread
From: Jerome Pouiller @ 2020-10-09 17:13 UTC (permalink / raw)
To: devel, linux-wireless
Cc: netdev, linux-kernel, Kalle Valo, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
From: Jérôme Pouiller <jerome.pouiller@silabs.com>
Smatch complains:
hif_rx.c:235 hif_generic_indication() warn: format string contains non-ascii character '\xc2'
hif_rx.c:235 hif_generic_indication() warn: format string contains non-ascii character '\xb0'
234 if (!wfx_api_older_than(wdev, 1, 4))
235 dev_info(wdev->dev, "Rx test ongoing. Temperature: %d°C\n",
^
236 body->data.rx_stats.current_temp);
So, replace the unicode character.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
drivers/staging/wfx/hif_rx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c
index f99921e76059..56a5f891447b 100644
--- a/drivers/staging/wfx/hif_rx.c
+++ b/drivers/staging/wfx/hif_rx.c
@@ -246,7 +246,7 @@ static int hif_generic_indication(struct wfx_dev *wdev,
mutex_lock(&wdev->rx_stats_lock);
// Older firmware send a generic indication beside RxStats
if (!wfx_api_older_than(wdev, 1, 4))
- dev_info(wdev->dev, "Rx test ongoing. Temperature: %d°C\n",
+ dev_info(wdev->dev, "Rx test ongoing. Temperature: %d degrees C\n",
body->data.rx_stats.current_temp);
memcpy(&wdev->rx_stats, &body->data.rx_stats,
sizeof(wdev->rx_stats));
--
2.28.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 8/8] staging: wfx: improve robustness of wfx_get_hw_rate()
2020-10-09 17:12 [PATCH 0/8] staging: wfx: fix issues reported by Smatch Jerome Pouiller
` (6 preceding siblings ...)
2020-10-09 17:13 ` [PATCH 7/8] staging: wfx: drop unicode characters from strings Jerome Pouiller
@ 2020-10-09 17:13 ` Jerome Pouiller
2020-10-16 1:50 ` Nathan Chancellor
7 siblings, 1 reply; 18+ messages in thread
From: Jerome Pouiller @ 2020-10-09 17:13 UTC (permalink / raw)
To: devel, linux-wireless
Cc: netdev, linux-kernel, Kalle Valo, Greg Kroah-Hartman,
David S . Miller, Dan Carpenter
From: Jérôme Pouiller <jerome.pouiller@silabs.com>
Smatch complains:
data_tx.c:37 wfx_get_hw_rate() warn: constraint '(struct ieee80211_supported_band)->bitrates' overflow 'band->bitrates' 0 <= abs_rl '0-127' user_rl '' required = '(struct ieee80211_supported_band)->n_bitrates'
23 struct ieee80211_supported_band *band;
24
25 if (rate->idx < 0)
26 return -1;
27 if (rate->flags & IEEE80211_TX_RC_MCS) {
28 if (rate->idx > 7) {
29 WARN(1, "wrong rate->idx value: %d", rate->idx);
30 return -1;
31 }
32 return rate->idx + 14;
33 }
34 // WFx only support 2GHz, else band information should be retrieved
35 // from ieee80211_tx_info
36 band = wdev->hw->wiphy->bands[NL80211_BAND_2GHZ];
37 return band->bitrates[rate->idx].hw_value;
Add a simple check to make Smatch happy.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
drivers/staging/wfx/data_tx.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
index 8db0be08daf8..41f6a604a697 100644
--- a/drivers/staging/wfx/data_tx.c
+++ b/drivers/staging/wfx/data_tx.c
@@ -31,6 +31,10 @@ static int wfx_get_hw_rate(struct wfx_dev *wdev,
}
return rate->idx + 14;
}
+ if (rate->idx >= band->n_bitrates) {
+ WARN(1, "wrong rate->idx value: %d", rate->idx);
+ return -1;
+ }
// WFx only support 2GHz, else band information should be retrieved
// from ieee80211_tx_info
band = wdev->hw->wiphy->bands[NL80211_BAND_2GHZ];
--
2.28.0
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 8/8] staging: wfx: improve robustness of wfx_get_hw_rate()
2020-10-09 17:13 ` [PATCH 8/8] staging: wfx: improve robustness of wfx_get_hw_rate() Jerome Pouiller
@ 2020-10-16 1:50 ` Nathan Chancellor
0 siblings, 0 replies; 18+ messages in thread
From: Nathan Chancellor @ 2020-10-16 1:50 UTC (permalink / raw)
To: Jerome Pouiller
Cc: devel, netdev, linux-wireless, linux-kernel, Dan Carpenter,
Greg Kroah-Hartman, David S . Miller, Kalle Valo
On Fri, Oct 09, 2020 at 07:13:07PM +0200, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
>
> Smatch complains:
>
> data_tx.c:37 wfx_get_hw_rate() warn: constraint '(struct ieee80211_supported_band)->bitrates' overflow 'band->bitrates' 0 <= abs_rl '0-127' user_rl '' required = '(struct ieee80211_supported_band)->n_bitrates'
> 23 struct ieee80211_supported_band *band;
> 24
> 25 if (rate->idx < 0)
> 26 return -1;
> 27 if (rate->flags & IEEE80211_TX_RC_MCS) {
> 28 if (rate->idx > 7) {
> 29 WARN(1, "wrong rate->idx value: %d", rate->idx);
> 30 return -1;
> 31 }
> 32 return rate->idx + 14;
> 33 }
> 34 // WFx only support 2GHz, else band information should be retrieved
> 35 // from ieee80211_tx_info
> 36 band = wdev->hw->wiphy->bands[NL80211_BAND_2GHZ];
> 37 return band->bitrates[rate->idx].hw_value;
>
> Add a simple check to make Smatch happy.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> ---
> drivers/staging/wfx/data_tx.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
> index 8db0be08daf8..41f6a604a697 100644
> --- a/drivers/staging/wfx/data_tx.c
> +++ b/drivers/staging/wfx/data_tx.c
> @@ -31,6 +31,10 @@ static int wfx_get_hw_rate(struct wfx_dev *wdev,
> }
> return rate->idx + 14;
> }
> + if (rate->idx >= band->n_bitrates) {
> + WARN(1, "wrong rate->idx value: %d", rate->idx);
> + return -1;
> + }
> // WFx only support 2GHz, else band information should be retrieved
> // from ieee80211_tx_info
> band = wdev->hw->wiphy->bands[NL80211_BAND_2GHZ];
> --
> 2.28.0
>
This now causes a clang warning:
drivers/staging/wfx/data_tx.c:34:19: warning: variable 'band' is uninitialized when used here [-Wuninitialized]
if (rate->idx >= band->n_bitrates) {
^~~~
drivers/staging/wfx/data_tx.c:23:39: note: initialize the variable 'band' to silence this warning
struct ieee80211_supported_band *band;
^
= NULL
1 warning generated.
Cheers,
Nathan
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread