diff mbox series

[v2,25/26] staging: wilc1000: refactor wilc_netdev_init() to handle memory free in error path

Message ID 1536043182-19735-26-git-send-email-ajay.kathat@microchip.com (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show
Series staging: wilc1000: avoid use of static and global variable | expand

Commit Message

Ajay Singh Sept. 4, 2018, 6:39 a.m. UTC 
Refactor the wilc_netdev_init() to cleanup the memory for error
scenario and remove unnecessary 'dev' pointer check.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/linux_wlan.c             | 36 ++++++++++++++++-------
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c |  6 +++-
 2 files changed, 30 insertions(+), 12 deletions(-)

Comments

Ajay Singh Sept. 10, 2018, 2:24 p.m. UTC | #1 
Hi Claudiu,

On Tue, 11 Sep 2018 12:21:13 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 04.09.2018 09:39, Ajay Singh wrote:
> > Refactor the wilc_netdev_init() to cleanup the memory for error
> > scenario and remove unnecessary 'dev' pointer check.
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/linux_wlan.c             | 36
> > ++++++++++++++++-------
> > drivers/staging/wilc1000/wilc_wfi_cfgoperations.c |  6 +++- 2 files
> > changed, 30 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/linux_wlan.c
> > b/drivers/staging/wilc1000/linux_wlan.c index d7d43fd..91a45a7
> > 100644 --- a/drivers/staging/wilc1000/linux_wlan.c
> > +++ b/drivers/staging/wilc1000/linux_wlan.c
> > @@ -1073,10 +1073,8 @@ int wilc_netdev_init(struct wilc **wilc,
> > struct device *dev, int io_type, INIT_LIST_HEAD(&wl->rxq_head.list);
> >  
> >  	wl->hif_workqueue =
> > create_singlethread_workqueue("WILC_wq");
> > -	if (!wl->hif_workqueue) {
> > -		kfree(wl);
> > -		return -ENOMEM;
> > -	}
> > +	if (!wl->hif_workqueue)
> > +		goto free_wl;
> >  
> >  	register_inetaddr_notifier(&g_dev_notifier);
> >  
> > @@ -1085,7 +1083,7 @@ int wilc_netdev_init(struct wilc **wilc,
> > struct device *dev, int io_type, 
> >  		ndev = alloc_etherdev(sizeof(struct wilc_vif));
> >  		if (!ndev)
> > -			return -ENOMEM;
> > +			goto free_ndev;
> >  
> >  		vif = netdev_priv(ndev);
> >  		memset(vif, 0, sizeof(struct wilc_vif));
> > @@ -1106,15 +1104,13 @@ int wilc_netdev_init(struct wilc **wilc,
> > struct device *dev, int io_type, ndev->netdev_ops =
> > &wilc_netdev_ops; 
> >  		wdev = wilc_create_wiphy(ndev, dev);
> > -
> > -		if (dev)
> > -			SET_NETDEV_DEV(ndev, dev);
> > -
> >  		if (!wdev) {
> >  			netdev_err(ndev, "Can't register WILC
> > Wiphy\n");
> > -			return -1;
> > +			goto free_ndev;
> >  		}
> >  
> > +		SET_NETDEV_DEV(ndev, dev);
> > +
> >  		vif->ndev->ieee80211_ptr = wdev;
> >  		vif->ndev->ml_priv = vif;
> >  		wdev->netdev = vif->ndev;
> > @@ -1125,11 +1121,29 @@ int wilc_netdev_init(struct wilc **wilc,
> > struct device *dev, int io_type, 
> >  		ret = register_netdev(ndev);
> >  		if (ret)
> > -			return ret;
> > +			goto free_ndev;  
> 
> In case this happens you will loose the return code of
> register_netdev() and you will return instead -ENOMEM. Maybe, the
> best approach will be to initialize ret = -ENOMEM while declaring it

Thanks for your suggestion to handle the return code.
I will work on it and submit the changes in different series. 

Regards,
Ajay
Claudiu Beznea Sept. 11, 2018, 9:21 a.m. UTC | #2 
On 04.09.2018 09:39, Ajay Singh wrote:
> Refactor the wilc_netdev_init() to cleanup the memory for error
> scenario and remove unnecessary 'dev' pointer check.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/linux_wlan.c             | 36 ++++++++++++++++-------
>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c |  6 +++-
>  2 files changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
> index d7d43fd..91a45a7 100644
> --- a/drivers/staging/wilc1000/linux_wlan.c
> +++ b/drivers/staging/wilc1000/linux_wlan.c
> @@ -1073,10 +1073,8 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
>  	INIT_LIST_HEAD(&wl->rxq_head.list);
>  
>  	wl->hif_workqueue = create_singlethread_workqueue("WILC_wq");
> -	if (!wl->hif_workqueue) {
> -		kfree(wl);
> -		return -ENOMEM;
> -	}
> +	if (!wl->hif_workqueue)
> +		goto free_wl;
>  
>  	register_inetaddr_notifier(&g_dev_notifier);
>  
> @@ -1085,7 +1083,7 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
>  
>  		ndev = alloc_etherdev(sizeof(struct wilc_vif));
>  		if (!ndev)
> -			return -ENOMEM;
> +			goto free_ndev;
>  
>  		vif = netdev_priv(ndev);
>  		memset(vif, 0, sizeof(struct wilc_vif));
> @@ -1106,15 +1104,13 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
>  		ndev->netdev_ops = &wilc_netdev_ops;
>  
>  		wdev = wilc_create_wiphy(ndev, dev);
> -
> -		if (dev)
> -			SET_NETDEV_DEV(ndev, dev);
> -
>  		if (!wdev) {
>  			netdev_err(ndev, "Can't register WILC Wiphy\n");
> -			return -1;
> +			goto free_ndev;
>  		}
>  
> +		SET_NETDEV_DEV(ndev, dev);
> +
>  		vif->ndev->ieee80211_ptr = wdev;
>  		vif->ndev->ml_priv = vif;
>  		wdev->netdev = vif->ndev;
> @@ -1125,11 +1121,29 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
>  
>  		ret = register_netdev(ndev);
>  		if (ret)
> -			return ret;
> +			goto free_ndev;

In case this happens you will loose the return code of register_netdev()
and you will return instead -ENOMEM. Maybe, the best approach will be to
initialize ret = -ENOMEM while declaring it

>  
>  		vif->iftype = STATION_MODE;
>  		vif->mac_opened = 0;
>  	}
>  
>  	return 0;
> +
> +free_ndev:
> +	for (; i >= 0; i--) {
> +		if (wl->vif[i]) {
> +			if (wl->vif[i]->iftype == STATION_MODE)
> +				unregister_netdev(wl->vif[i]->ndev);
> +
> +			if (wl->vif[i]->ndev) {
> +				wilc_free_wiphy(wl->vif[i]->ndev);
> +				free_netdev(wl->vif[i]->ndev);
> +			}
> +		}
> +	}
> +	unregister_inetaddr_notifier(&g_dev_notifier);
> +	destroy_workqueue(wl->hif_workqueue);
> +free_wl:
> +	kfree(wl);
> +	return -ENOMEM;

and here to just return ret.

>  }
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index d103dce2..37c26d4 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -2145,8 +2145,12 @@ struct wireless_dev *wilc_create_wiphy(struct net_device *net,
>  	set_wiphy_dev(wdev->wiphy, dev);
>  
>  	ret = wiphy_register(wdev->wiphy);
> -	if (ret)
> +	if (ret) {
>  		netdev_err(net, "Cannot register wiphy device\n");
> +		wiphy_free(wdev->wiphy);
> +		kfree(wdev);
> +		return NULL;
> +	}
>  
>  	priv->dev = net;
>  	return wdev;
>
diff mbox series

Patch

diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index d7d43fd..91a45a7 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1073,10 +1073,8 @@  int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
 	INIT_LIST_HEAD(&wl->rxq_head.list);
 
 	wl->hif_workqueue = create_singlethread_workqueue("WILC_wq");
-	if (!wl->hif_workqueue) {
-		kfree(wl);
-		return -ENOMEM;
-	}
+	if (!wl->hif_workqueue)
+		goto free_wl;
 
 	register_inetaddr_notifier(&g_dev_notifier);
 
@@ -1085,7 +1083,7 @@  int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
 
 		ndev = alloc_etherdev(sizeof(struct wilc_vif));
 		if (!ndev)
-			return -ENOMEM;
+			goto free_ndev;
 
 		vif = netdev_priv(ndev);
 		memset(vif, 0, sizeof(struct wilc_vif));
@@ -1106,15 +1104,13 @@  int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
 		ndev->netdev_ops = &wilc_netdev_ops;
 
 		wdev = wilc_create_wiphy(ndev, dev);
-
-		if (dev)
-			SET_NETDEV_DEV(ndev, dev);
-
 		if (!wdev) {
 			netdev_err(ndev, "Can't register WILC Wiphy\n");
-			return -1;
+			goto free_ndev;
 		}
 
+		SET_NETDEV_DEV(ndev, dev);
+
 		vif->ndev->ieee80211_ptr = wdev;
 		vif->ndev->ml_priv = vif;
 		wdev->netdev = vif->ndev;
@@ -1125,11 +1121,29 @@  int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
 
 		ret = register_netdev(ndev);
 		if (ret)
-			return ret;
+			goto free_ndev;
 
 		vif->iftype = STATION_MODE;
 		vif->mac_opened = 0;
 	}
 
 	return 0;
+
+free_ndev:
+	for (; i >= 0; i--) {
+		if (wl->vif[i]) {
+			if (wl->vif[i]->iftype == STATION_MODE)
+				unregister_netdev(wl->vif[i]->ndev);
+
+			if (wl->vif[i]->ndev) {
+				wilc_free_wiphy(wl->vif[i]->ndev);
+				free_netdev(wl->vif[i]->ndev);
+			}
+		}
+	}
+	unregister_inetaddr_notifier(&g_dev_notifier);
+	destroy_workqueue(wl->hif_workqueue);
+free_wl:
+	kfree(wl);
+	return -ENOMEM;
 }
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index d103dce2..37c26d4 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -2145,8 +2145,12 @@  struct wireless_dev *wilc_create_wiphy(struct net_device *net,
 	set_wiphy_dev(wdev->wiphy, dev);
 
 	ret = wiphy_register(wdev->wiphy);
-	if (ret)
+	if (ret) {
 		netdev_err(net, "Cannot register wiphy device\n");
+		wiphy_free(wdev->wiphy);
+		kfree(wdev);
+		return NULL;
+	}
 
 	priv->dev = net;
 	return wdev;