[PATCH] libertas: fix error handling

[Marcelo Tosatti]

Hi Holger,

On Sat, Feb 24, 2007 at 04:58:33PM +0100, Holger Schurig wrote:
> * renamed wlan_add/remove_XXX into libertas_add/remove_XXX
> * change libertas_remove_card() so that it can clean up even
>   half-initialized netdev/adapter/wlan_private objects
> * add some doxygen comments about this
> 
> Signed-off-by: Holger Schurig <hs4233 at mail.mn-solutions.de>
> ---
>  drivers/net/wireless/libertas/decl.h   |    8 +-
>  drivers/net/wireless/libertas/if_usb.c |   22 +++---
>  drivers/net/wireless/libertas/main.c   |  117 +++++++++++++++++--------------
>  3 files changed, 80 insertions(+), 67 deletions(-)
> 

>  dealloc:
> +	libertas_remove_mesh(priv);
> +	libertas_remove_card(priv);
>  	if_usb_free(usb_cardp);
>  

>  
>  	adapter = priv->adapter;
> -
> -	if (!adapter)
> -		goto out;
> -
>  	dev = priv->wlan_dev.netdev;
>  
> -	netif_stop_queue(priv->wlan_dev.netdev);
> -	netif_carrier_off(priv->wlan_dev.netdev);
> +	if (adapter) {
> +		if (dev) {
> +			netif_stop_queue(dev);
> +			netif_carrier_off(dev);
> +		}
>  
> -	wake_pending_cmdnodes(priv);
> +		wake_pending_cmdnodes(priv);
>  
> -	unregister_netdev(dev);
> +		if (dev && dev->reg_state)
> +			unregister_netdev(dev);

This is a layering violation... Shouldnt poke into dev->reg_state from
driver context like this.

>  
> -	cancel_delayed_work(&priv->assoc_work);
> -	destroy_workqueue(priv->assoc_thread);
> +		cancel_delayed_work(&priv->assoc_work);
> +		if (priv->assoc_thread)
> +			destroy_workqueue(priv->assoc_thread);
>  
> -	if (adapter->psmode == wlan802_11powermodemax_psp) {
> -		adapter->psmode = wlan802_11powermodecam;
> -		libertas_ps_wakeup(priv, cmd_option_waitforrsp);
> -	}
> +		if (adapter->psmode == wlan802_11powermodemax_psp) {
> +			adapter->psmode = wlan802_11powermodecam;
> +			libertas_ps_wakeup(priv, cmd_option_waitforrsp);
> +		}
>  
> -	memset(wrqu.ap_addr.sa_data, 0xaa, ETH_ALEN);
> -	wrqu.ap_addr.sa_family = ARPHRD_ETHER;
> -	wireless_send_event(priv->wlan_dev.netdev, SIOCGIWAP, &wrqu, NULL);
> +		memset(wrqu.ap_addr.sa_data, 0xaa, ETH_ALEN);
> +		wrqu.ap_addr.sa_family = ARPHRD_ETHER;
> +		wireless_send_event(dev, SIOCGIWAP, &wrqu, NULL);

And I really don't like to have libertas_remove_card handle "half
initialized" devices like this.

Undoing initialization steps (error handling) should be done at the
initialization sites. Having a single function for both half-initialized
and full removal/unregister cases is confusing and bug-prone.

Can you please cook a patch to handle failures at initialization instead
of this?

TIA