[PATCH 0/3] wlcore: Adjustments for wl1271_acx_sta_rate

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 0/3] wlcore: Adjustments for wl1271_acx_sta_rate_policies()
@ 2018-03-10 21:30 SF Markus Elfring
  2018-03-10 21:31 ` [PATCH 1/3] wlcore: Delete an unnecessary variable initialisation in wl1271_acx_sta_rate_policies() SF Markus Elfring
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: SF Markus Elfring @ 2018-03-10 21:30 UTC (permalink / raw)
  To: linux-wireless, netdev, Colin Ian King, Kalle Valo; +Cc: LKML, kernel-janitors

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Mar 2018 22:25:45 +0100

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Delete an unnecessary variable initialisation
  Return directly after a failed kzalloc()
  Use common error handling code

 drivers/net/wireless/ti/wlcore/acx.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

-- 
2.16.2

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/3] wlcore: Delete an unnecessary variable initialisation in wl1271_acx_sta_rate_policies()
  2018-03-10 21:30 [PATCH 0/3] wlcore: Adjustments for wl1271_acx_sta_rate_policies() SF Markus Elfring
@ 2018-03-10 21:31 ` SF Markus Elfring
  2018-03-10 21:32 ` [PATCH 2/3] wlcore: Return directly after a failed kzalloc() " SF Markus Elfring
  2018-03-10 21:33 ` [PATCH 3/3] wlcore: Use common error handling code " SF Markus Elfring
  2 siblings, 0 replies; 5+ messages in thread
From: SF Markus Elfring @ 2018-03-10 21:31 UTC (permalink / raw)
  To: linux-wireless, netdev, Colin Ian King, Kalle Valo; +Cc: LKML, kernel-janitors

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Mar 2018 21:51:17 +0100

The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/ti/wlcore/acx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
index 3ca9167d6146..6991fee8fe61 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -738,7 +738,7 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct wl12xx_vif *wlvif)
 {
 	struct acx_rate_policy *acx;
 	struct conf_tx_rate_class *c = &wl->conf.tx.sta_rc_conf;
-	int ret = 0;
+	int ret;
 
 	wl1271_debug(DEBUG_ACX, "acx rate policies");
 
-- 
2.16.2

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/3] wlcore: Return directly after a failed kzalloc() in wl1271_acx_sta_rate_policies()
  2018-03-10 21:30 [PATCH 0/3] wlcore: Adjustments for wl1271_acx_sta_rate_policies() SF Markus Elfring
  2018-03-10 21:31 ` [PATCH 1/3] wlcore: Delete an unnecessary variable initialisation in wl1271_acx_sta_rate_policies() SF Markus Elfring
@ 2018-03-10 21:32 ` SF Markus Elfring
  2018-03-10 21:33 ` [PATCH 3/3] wlcore: Use common error handling code " SF Markus Elfring
  2 siblings, 0 replies; 5+ messages in thread
From: SF Markus Elfring @ 2018-03-10 21:32 UTC (permalink / raw)
  To: linux-wireless, netdev, Colin Ian King, Kalle Valo; +Cc: LKML, kernel-janitors

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Mar 2018 22:00:31 +0100

Return directly after a call of the function "kzalloc" failed
at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/ti/wlcore/acx.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
index 6991fee8fe61..1cc5bba670e1 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -743,11 +743,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct wl12xx_vif *wlvif)
 	wl1271_debug(DEBUG_ACX, "acx rate policies");
 
 	acx = kzalloc(sizeof(*acx), GFP_KERNEL);
-
-	if (!acx) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!acx)
+		return -ENOMEM;
 
 	wl1271_debug(DEBUG_ACX, "basic_rate: 0x%x, full_rate: 0x%x",
 		wlvif->basic_rate, wlvif->rate_set);
-- 
2.16.2

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 3/3] wlcore: Use common error handling code in wl1271_acx_sta_rate_policies()
  2018-03-10 21:30 [PATCH 0/3] wlcore: Adjustments for wl1271_acx_sta_rate_policies() SF Markus Elfring
  2018-03-10 21:31 ` [PATCH 1/3] wlcore: Delete an unnecessary variable initialisation in wl1271_acx_sta_rate_policies() SF Markus Elfring
  2018-03-10 21:32 ` [PATCH 2/3] wlcore: Return directly after a failed kzalloc() " SF Markus Elfring
@ 2018-03-10 21:33 ` SF Markus Elfring
  2018-03-11 23:00   ` Arend van Spriel
  2 siblings, 1 reply; 5+ messages in thread
From: SF Markus Elfring @ 2018-03-10 21:33 UTC (permalink / raw)
  To: linux-wireless, netdev, Colin Ian King, Kalle Valo; +Cc: LKML, kernel-janitors

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Mar 2018 22:18:45 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/ti/wlcore/acx.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
index 1cc5bba670e1..7d37a417c756 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -757,10 +757,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct wl12xx_vif *wlvif)
 	acx->rate_policy.aflags = c->aflags;
 
 	ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
-	if (ret < 0) {
-		wl1271_warning("Setting of rate policies failed: %d", ret);
-		goto out;
-	}
+	if (ret < 0)
+		goto report_failure;
 
 	/* configure one AP supported rate class */
 	acx->rate_policy_idx = cpu_to_le32(wlvif->sta.ap_rate_idx);
@@ -773,10 +771,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct wl12xx_vif *wlvif)
 	acx->rate_policy.aflags = c->aflags;
 
 	ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
-	if (ret < 0) {
-		wl1271_warning("Setting of rate policies failed: %d", ret);
-		goto out;
-	}
+	if (ret < 0)
+		goto report_failure;
 
 	/*
 	 * configure one rate class for basic p2p operations.
@@ -791,14 +787,16 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct wl12xx_vif *wlvif)
 	acx->rate_policy.aflags = c->aflags;
 
 	ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
-	if (ret < 0) {
-		wl1271_warning("Setting of rate policies failed: %d", ret);
-		goto out;
-	}
+	if (ret < 0)
+		goto report_failure;
 
-out:
+free_acx:
 	kfree(acx);
 	return ret;
+
+report_failure:
+	wl1271_warning("Setting of rate policies failed: %d", ret);
+	goto free_acx;
 }
 
 int wl1271_acx_ap_rate_policy(struct wl1271 *wl, struct conf_tx_rate_class *c,
-- 
2.16.2

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 3/3] wlcore: Use common error handling code in wl1271_acx_sta_rate_policies()
  2018-03-10 21:33 ` [PATCH 3/3] wlcore: Use common error handling code " SF Markus Elfring
@ 2018-03-11 23:00   ` Arend van Spriel
  0 siblings, 0 replies; 5+ messages in thread
From: Arend van Spriel @ 2018-03-11 23:00 UTC (permalink / raw)
  To: SF Markus Elfring, linux-wireless, netdev, Colin Ian King,
	Kalle Valo
  Cc: LKML, kernel-janitors

On 3/10/2018 10:33 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 10 Mar 2018 22:18:45 +0100
>
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
>
> This issue was detected by using the Coccinelle software.

You call this an issue?

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/net/wireless/ti/wlcore/acx.c | 24 +++++++++++-------------
>   1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
> index 1cc5bba670e1..7d37a417c756 100644
> --- a/drivers/net/wireless/ti/wlcore/acx.c
> +++ b/drivers/net/wireless/ti/wlcore/acx.c

[...]

>   	ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
> -	if (ret < 0) {
> -		wl1271_warning("Setting of rate policies failed: %d", ret);
> -		goto out;
> -	}
> +	if (ret < 0)
> +		goto report_failure;
>
> -out:
> +free_acx:
>   	kfree(acx);
>   	return ret;
> +
> +report_failure:
> +	wl1271_warning("Setting of rate policies failed: %d", ret);
> +	goto free_acx;

In my opinion you are introducing a new issue. I don't call this 
"common" in any way. It is leaning more towards "spaghetti code" [1]. 
Jumping over a label to return to it with another jump. They are not 
long jumps, but it sure does not make thing more readable. Always aim 
for simple top-to-bottom.

Regards,
Arend

[1] https://en.wikipedia.org/wiki/Spaghetti_code

>   }
>
>   int wl1271_acx_ap_rate_policy(struct wl1271 *wl, struct conf_tx_rate_class *c,
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-03-11 23:00 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-10 21:30 [PATCH 0/3] wlcore: Adjustments for wl1271_acx_sta_rate_policies() SF Markus Elfring
2018-03-10 21:31 ` [PATCH 1/3] wlcore: Delete an unnecessary variable initialisation in wl1271_acx_sta_rate_policies() SF Markus Elfring
2018-03-10 21:32 ` [PATCH 2/3] wlcore: Return directly after a failed kzalloc() " SF Markus Elfring
2018-03-10 21:33 ` [PATCH 3/3] wlcore: Use common error handling code " SF Markus Elfring
2018-03-11 23:00   ` Arend van Spriel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).