[PATCH 0/8] staging: wfx: fix issues reported by Smatch

[PATCH 0/8] staging: wfx: fix issues reported by Smatch

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Fix the issues reported by Dan using Smatch[1].

[1] https://lore.kernel.org/lkml/20201008131320.GA1042@kadam/

Jérôme Pouiller (8):
  staging: wfx: improve error handling of hif_join()
  staging: wfx: check memory allocation
  staging: wfx: standardize the error when vif does not exist
  staging: wfx: wfx_init_common() returns NULL on error
  staging: wfx: increase robustness of hif_generic_confirm()
  staging: wfx: gpiod_get_value() can return an error
  staging: wfx: drop unicode characters from strings
  staging: wfx: improve robustness of wfx_get_hw_rate()

 drivers/staging/wfx/bh.c      |  2 +-
 drivers/staging/wfx/data_tx.c |  9 ++++++-
 drivers/staging/wfx/hif_rx.c  | 44 +++++++++++++++++++++++------------
 drivers/staging/wfx/hif_tx.c  |  4 +++-
 drivers/staging/wfx/main.c    | 10 ++++++--
 drivers/staging/wfx/sta.c     |  4 ++++
 6 files changed, 53 insertions(+), 20 deletions(-)

-- 
2.28.0

[PATCH 1/8] staging: wfx: improve error handling of hif_join()

[Jerome Pouiller]

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

[PATCH 2/8] staging: wfx: check memory allocation

[Jerome Pouiller]

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

[PATCH 3/8] staging: wfx: standardize the error when vif does not exist

[Jerome Pouiller]

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

[PATCH 4/8] staging: wfx: wfx_init_common() returns NULL on error

[Jerome Pouiller]

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

[PATCH 5/8] staging: wfx: increase robustness of hif_generic_confirm()

[Jerome Pouiller]

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

[PATCH 6/8] staging: wfx: gpiod_get_value() can return an error

[Jerome Pouiller]

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

[PATCH 7/8] staging: wfx: drop unicode characters from strings

[Jerome Pouiller]

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

[PATCH 8/8] staging: wfx: improve robustness of wfx_get_hw_rate()

[Jerome Pouiller]

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

Re: [PATCH 2/8] staging: wfx: check memory allocation

[Kalle Valo]

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

Re: [PATCH 3/8] staging: wfx: standardize the error when vif does not exist

[Kalle Valo]

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

Re: [PATCH 2/8] staging: wfx: check memory allocation

[Jérôme Pouiller]

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

Re: [PATCH 3/8] staging: wfx: standardize the error when vif does not exist

[Jérôme Pouiller]

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

Re: [PATCH 3/8] staging: wfx: standardize the error when vif does not exist

[Greg Kroah-Hartman]

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

Re: [PATCH 2/8] staging: wfx: check memory allocation

[Dan Carpenter]

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

Re: [PATCH 3/8] staging: wfx: standardize the error when vif does not exist

[Jérôme Pouiller]

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

Re: [PATCH 2/8] staging: wfx: check memory allocation

[Dan Carpenter]

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

Re: [PATCH 8/8] staging: wfx: improve robustness of wfx_get_hw_rate()

[Nathan Chancellor]

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