Re: [RFC/RFT] rtl8187: Protect the config callback from mac80211 with a mutex

[Larry Finger <Larry.Finger@lwfinger.net>]

Herton Ronaldo Krzesinski wrote:
> 
> Ah ok, only protect the section, like this?
> 
> diff --git a/drivers/net/wireless/rtl8187.h b/drivers/net/wireless/rtl8187.h
> index 3afb49f..a4e42dc 100644
> --- a/drivers/net/wireless/rtl8187.h
> +++ b/drivers/net/wireless/rtl8187.h
> @@ -92,6 +92,10 @@ struct rtl8187_priv {
>  	const struct rtl818x_rf_ops *rf;
>  	struct ieee80211_vif *vif;
>  	int mode;
> +	/* The mutex protects the TX loopback state.
> +	 * Avoid concurrent channel changes with loopback enabled.
> +	 */
> +	struct mutex loopback_mutex;
>  
>  	/* rtl8187 specific */
>  	struct ieee80211_channel channels[14];
> diff --git a/drivers/net/wireless/rtl8187_dev.c b/drivers/net/wireless/rtl8187_dev.c
> index d3067b1..cfa048b 100644
> --- a/drivers/net/wireless/rtl8187_dev.c
> +++ b/drivers/net/wireless/rtl8187_dev.c
> @@ -835,6 +835,7 @@ static int rtl8187_config(struct ieee80211_hw *dev, struct ieee80211_conf *conf)
>  	struct rtl8187_priv *priv = dev->priv;
>  	u32 reg;
>  
> +	mutex_lock(&priv->loopback_mutex);
>  	reg = rtl818x_ioread32(priv, &priv->map->TX_CONF);
>  	/* Enable TX loopback on MAC level to avoid TX during channel
>  	 * changes, as this has be seen to causes problems and the
> @@ -846,6 +847,7 @@ static int rtl8187_config(struct ieee80211_hw *dev, struct ieee80211_conf *conf)
>  	priv->rf->set_chan(dev, conf);
>  	msleep(10);
>  	rtl818x_iowrite32(priv, &priv->map->TX_CONF, reg);
> +	mutex_unlock(&priv->loopback_mutex);
>  
>  	if (!priv->is_rtl8187b) {
>  		rtl818x_iowrite8(priv, &priv->map->SIFS, 0x22);
> @@ -1154,6 +1156,7 @@ static int __devinit rtl8187_probe(struct usb_interface *intf,
>  		printk(KERN_ERR "rtl8187: Cannot register device\n");
>  		goto err_free_dev;
>  	}
> +	mutex_init(&priv->loopback_mutex);
>  
>  	printk(KERN_INFO "%s: hwaddr %s, %s V%d + %s\n",
>  	       wiphy_name(dev->wiphy), print_mac(mac, dev->wiphy->perm_addr),
> 
> 

This scheme would probably work; however, I still think we should protect all 
the configuration data. That way we won't have the case where some of the data 
come from one call and the rest from a different one. In any event, most of the 
time that we are locked comes from the 2 msleeps that have to be within the lock 
anyway.

Larry