wl18xx: Handle kfree() in better way when kzalloc fails

Commit Message

Souptick Joarder Oct. 7, 2016, 6:49 a.m. UTC

This patch is added to handle kfree and return error in a better way
if kzalloc fails in wl18xx_scan_send() and wl18xx_scan_sched_scan_config().

Signed-off-by: Souptick joarder <[e-mail]>
---
 drivers/net/wireless/ti/wl18xx/scan.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Johannes Berg Oct. 7, 2016, 7:06 a.m. UTC | #1

On Fri, 2016-10-07 at 12:19 +0530, Souptick Joarder wrote:
> This patch is added to handle kfree and return error in a better way

What's "better" about this?

kfree(NULL) is perfectly valid, adding another label makes the code
harder to read,

> -	struct wl18xx_cmd_scan_params *cmd;
> +	struct wl18xx_cmd_scan_params *cmd = NULL;

that new initialization is actually *completely* pointless since it's
overwritten immediately here:

>  	struct wlcore_scan_channels *cmd_channels = NULL;
>  	int ret;
>  
>  	cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>  	if (!cmd) {
> -		ret = -ENOMEM;
> -		goto out;
> +		return -ENOMEM;
>  	}
> 
...

what gives?

johannes

souptick joarder Oct. 7, 2016, 7:29 a.m. UTC | #2

On Fri, Oct 7, 2016 at 12:36 PM, Johannes Berg
<[e-mail]> wrote:
>
> On Fri, 2016-10-07 at 12:19 +0530, Souptick Joarder wrote:
> > This patch is added to handle kfree and return error in a better way
>
> What's "better" about this?
>
> kfree(NULL) is perfectly valid, adding another label makes the code
> harder to read,

I agree with you.

when kzalloc(sizeof(*cmd), GFP_KERNEL) fails, try to avoid extra
kfree(cmd_channel) function call,
cause anyway it will call kfree with NULL.

I thought from that point of view.

>
> > -     struct wl18xx_cmd_scan_params *cmd;
> > +     struct wl18xx_cmd_scan_params *cmd = NULL;
>
> that new initialization is actually *completely* pointless since it's
> overwritten immediately here:
>
> >       struct wlcore_scan_channels *cmd_channels = NULL;
> >       int ret;
> >
> >       cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> >       if (!cmd) {
> > -             ret = -ENOMEM;
> > -             goto out;
> > +             return -ENOMEM;
> >       }
> >
> ...
>
> what gives?
>

> johannes

Ok, I will drop this patch.

-Souptick

Patch

diff --git a/drivers/net/wireless/ti/wl18xx/scan.c b/drivers/net/wireless/ti/wl18xx/scan.c
index 4e522154..aed22e1 100644
--- a/drivers/net/wireless/ti/wl18xx/scan.c
+++ b/drivers/net/wireless/ti/wl18xx/scan.c
@@ -41,14 +41,13 @@  static void wl18xx_adjust_channels(struct wl18xx_cmd_scan_params *cmd,
 static int wl18xx_scan_send(struct wl1271 *wl, struct wl12xx_vif *wlvif,
 			    struct cfg80211_scan_request *req)
 {
-	struct wl18xx_cmd_scan_params *cmd;
+	struct wl18xx_cmd_scan_params *cmd = NULL;
 	struct wlcore_scan_channels *cmd_channels = NULL;
 	int ret;
 
 	cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
 	if (!cmd) {
-		ret = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 
 	/* scan on the dev role if the regular one is not started */
@@ -59,7 +58,7 @@  static int wl18xx_scan_send(struct wl1271 *wl, struct wl12xx_vif *wlvif,
 
 	if (WARN_ON(cmd->role_id == WL12XX_INVALID_ROLE_ID)) {
 		ret = -EINVAL;
-		goto out;
+		goto err_cmd_free;
 	}
 
 	cmd->scan_type = SCAN_TYPE_SEARCH;
@@ -84,7 +83,7 @@  static int wl18xx_scan_send(struct wl1271 *wl, struct wl12xx_vif *wlvif,
 	cmd_channels = kzalloc(sizeof(*cmd_channels), GFP_KERNEL);
 	if (!cmd_channels) {
 		ret = -ENOMEM;
-		goto out;
+		goto err_cmd_free;
 	}
 
 	wlcore_set_scan_chan_params(wl, cmd_channels, req->channels,
@@ -153,6 +152,7 @@  static int wl18xx_scan_send(struct wl1271 *wl, struct wl12xx_vif *wlvif,
 
 out:
 	kfree(cmd_channels);
+err_cmd_free:
 	kfree(cmd);
 	return ret;
 }
@@ -171,7 +171,7 @@  int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
 				  struct cfg80211_sched_scan_request *req,
 				  struct ieee80211_scan_ies *ies)
 {
-	struct wl18xx_cmd_scan_params *cmd;
+	struct wl18xx_cmd_scan_params *cmd = NULL;
 	struct wlcore_scan_channels *cmd_channels = NULL;
 	struct conf_sched_scan_settings *c = &wl->conf.sched_scan;
 	int ret;
@@ -185,15 +185,14 @@  int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
 
 	cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
 	if (!cmd) {
-		ret = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 
 	cmd->role_id = wlvif->role_id;
 
 	if (WARN_ON(cmd->role_id == WL12XX_INVALID_ROLE_ID)) {
 		ret = -EINVAL;
-		goto out;
+		goto err_cmd_free;
 	}
 
 	cmd->scan_type = SCAN_TYPE_PERIODIC;
@@ -218,7 +217,7 @@  int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
 	cmd_channels = kzalloc(sizeof(*cmd_channels), GFP_KERNEL);
 	if (!cmd_channels) {
 		ret = -ENOMEM;
-		goto out;
+		goto err_cmd_free;
 	}
 
 	/* configure channels */
@@ -296,6 +295,7 @@  int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
 
 out:
 	kfree(cmd_channels);
+err_cmd_free:
 	kfree(cmd);
 	return ret;
 }