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

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

[Larry Finger]

Herton,

Does this patch help your problem? I haven't done much testing, but things seem 
to be better with it here.

Larry

==================

Some users of the RTL8187B have experienced difficulties since commit
8f87dd7e540d455f8e7f11478133b85edc969c67 that introduced the power
management wext hooks. This difficulty has not made much sense until
it was realized that it was possible for mac80211 to make a call to the
config routine while that routine was already being executed. This patch
protects the critical section with a mutex.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

Index: wireless-testing/drivers/net/wireless/rtl8187.h
===================================================================
--- wireless-testing.orig/drivers/net/wireless/rtl8187.h
+++ wireless-testing/drivers/net/wireless/rtl8187.h
@@ -94,6 +94,7 @@ struct rtl8187_priv {
  	const struct rtl818x_rf_ops *rf;
  	struct ieee80211_vif *vif;
  	int mode;
+	struct mutex mutex;	/* used to lock config callback */

  	/* rtl8187 specific */
  	struct ieee80211_channel channels[14];
Index: wireless-testing/drivers/net/wireless/rtl8187_dev.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/rtl8187_dev.c
+++ wireless-testing/drivers/net/wireless/rtl8187_dev.c
@@ -864,6 +864,7 @@ static int rtl8187_config(struct ieee802
  	struct rtl8187_priv *priv = dev->priv;
  	u32 reg;

+	mutex_lock(&priv->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
@@ -896,6 +897,7 @@ static int rtl8187_config(struct ieee802
  	rtl818x_iowrite16(priv, &priv->map->ATIMTR_INTERVAL, 100);
  	rtl818x_iowrite16(priv, &priv->map->BEACON_INTERVAL, 100);
  	rtl818x_iowrite16(priv, &priv->map->BEACON_INTERVAL_TIME, 100);
+	mutex_unlock(&priv->mutex);
  	return 0;
  }

@@ -1187,6 +1189,7 @@ static int __devinit rtl8187_probe(struc
  		printk(KERN_ERR "rtl8187: Cannot register device\n");
  		goto err_free_dev;
  	}
+	mutex_init(&priv->mutex);

  	printk(KERN_INFO "%s: hwaddr %s, %s V%d + %s\n",
  	       wiphy_name(dev->wiphy), print_mac(mac, dev->wiphy->perm_addr),

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

[Michael Buesch]

On Wednesday 30 July 2008 08:12:36 Larry Finger wrote:
> Herton,
> 
> Does this patch help your problem? I haven't done much testing, but things seem 
> to be better with it here.
> 
> Larry
> 
> ==================
> 
> Some users of the RTL8187B have experienced difficulties since commit
> 8f87dd7e540d455f8e7f11478133b85edc969c67 that introduced the power
> management wext hooks. This difficulty has not made much sense until
> it was realized that it was possible for mac80211 to make a call to the
> config routine while that routine was already being executed. This patch
> protects the critical section with a mutex.
> 
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
> 
> Index: wireless-testing/drivers/net/wireless/rtl8187.h
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/rtl8187.h
> +++ wireless-testing/drivers/net/wireless/rtl8187.h
> @@ -94,6 +94,7 @@ struct rtl8187_priv {
>   	const struct rtl818x_rf_ops *rf;
>   	struct ieee80211_vif *vif;
>   	int mode;
> +	struct mutex mutex;	/* used to lock config callback */

Well, hm.
Using locks to protect code is a bad idea, most of the time.
So that comment makes no sense.

What that lock _probably_ does it to protect the configuration
data in struct rtl8187_priv. So you'd better find out which data
that is and clarify the comment.

> 
>   	/* rtl8187 specific */
>   	struct ieee80211_channel channels[14];
> Index: wireless-testing/drivers/net/wireless/rtl8187_dev.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/rtl8187_dev.c
> +++ wireless-testing/drivers/net/wireless/rtl8187_dev.c
> @@ -864,6 +864,7 @@ static int rtl8187_config(struct ieee802
>   	struct rtl8187_priv *priv = dev->priv;
>   	u32 reg;
> 
> +	mutex_lock(&priv->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
> @@ -896,6 +897,7 @@ static int rtl8187_config(struct ieee802
>   	rtl818x_iowrite16(priv, &priv->map->ATIMTR_INTERVAL, 100);
>   	rtl818x_iowrite16(priv, &priv->map->BEACON_INTERVAL, 100);
>   	rtl818x_iowrite16(priv, &priv->map->BEACON_INTERVAL_TIME, 100);
> +	mutex_unlock(&priv->mutex);
>   	return 0;
>   }
> 
> @@ -1187,6 +1189,7 @@ static int __devinit rtl8187_probe(struc
>   		printk(KERN_ERR "rtl8187: Cannot register device\n");
>   		goto err_free_dev;
>   	}
> +	mutex_init(&priv->mutex);
> 
>   	printk(KERN_INFO "%s: hwaddr %s, %s V%d + %s\n",
>   	       wiphy_name(dev->wiphy), print_mac(mac, dev->wiphy->perm_addr),

-- 
Greetings Michael.

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

[Michael Buesch]

On Wednesday 30 July 2008 15:24:40 Michael Buesch wrote:
> On Wednesday 30 July 2008 08:12:36 Larry Finger wrote:
> > Herton,
> > 
> > Does this patch help your problem? I haven't done much testing, but things seem 
> > to be better with it here.
> > 
> > Larry
> > 
> > ==================
> > 
> > Some users of the RTL8187B have experienced difficulties since commit
> > 8f87dd7e540d455f8e7f11478133b85edc969c67 that introduced the power
> > management wext hooks. This difficulty has not made much sense until
> > it was realized that it was possible for mac80211 to make a call to the
> > config routine while that routine was already being executed. This patch
> > protects the critical section with a mutex.
> > 
> > Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> > ---
> > 
> > Index: wireless-testing/drivers/net/wireless/rtl8187.h
> > ===================================================================
> > --- wireless-testing.orig/drivers/net/wireless/rtl8187.h
> > +++ wireless-testing/drivers/net/wireless/rtl8187.h
> > @@ -94,6 +94,7 @@ struct rtl8187_priv {
> >   	const struct rtl818x_rf_ops *rf;
> >   	struct ieee80211_vif *vif;
> >   	int mode;
> > +	struct mutex mutex;	/* used to lock config callback */
> 
> Well, hm.
> Using locks to protect code is a bad idea, most of the time.
> So that comment makes no sense.
> 
> What that lock _probably_ does it to protect the configuration
> data in struct rtl8187_priv. So you'd better find out which data
> that is and clarify the comment.

and the mutex name, of course.

	/* Mutex to protect the device configuration data,
	 * which is foobar and bizzbaz */
	struct mutex conf_mutex;

-- 
Greetings Michael.

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

[Herton Ronaldo Krzesinski]

Em Wednesday 30 July 2008 10:27:26 Michael Buesch escreveu:
> On Wednesday 30 July 2008 15:24:40 Michael Buesch wrote:
> > On Wednesday 30 July 2008 08:12:36 Larry Finger wrote:
> > > Herton,
> > >
> > > Does this patch help your problem? I haven't done much testing, but
> > > things seem to be better with it here.
> > >
> > > Larry
> > >
> > > ==================
> > >
> > > Some users of the RTL8187B have experienced difficulties since commit
> > > 8f87dd7e540d455f8e7f11478133b85edc969c67 that introduced the power
> > > management wext hooks. This difficulty has not made much sense until
> > > it was realized that it was possible for mac80211 to make a call to the
> > > config routine while that routine was already being executed. This
> > > patch protects the critical section with a mutex.
> > >
> > > Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> > > ---
> > >
> > > Index: wireless-testing/drivers/net/wireless/rtl8187.h
> > > ===================================================================
> > > --- wireless-testing.orig/drivers/net/wireless/rtl8187.h
> > > +++ wireless-testing/drivers/net/wireless/rtl8187.h
> > > @@ -94,6 +94,7 @@ struct rtl8187_priv {
> > >   	const struct rtl818x_rf_ops *rf;
> > >   	struct ieee80211_vif *vif;
> > >   	int mode;
> > > +	struct mutex mutex;	/* used to lock config callback */
> >
> > Well, hm.
> > Using locks to protect code is a bad idea, most of the time.
> > So that comment makes no sense.
> >
> > What that lock _probably_ does it to protect the configuration
> > data in struct rtl8187_priv. So you'd better find out which data
> > that is and clarify the comment.
>
> and the mutex name, of course.
>
> 	/* Mutex to protect the device configuration data,
> 	 * which is foobar and bizzbaz */
> 	struct mutex conf_mutex;

Yes, it's better this way. About the lock, the problem here is you can't set 
the channel while transmitting data on 8187 (the card stops working util 
reset like the comment on the code), so we must enable tx loopback while 
setting channels, but you can't run rtl8187_config concurrently because one 
instance may be disabling tx loopback while other is still setting channel, 
or like the code is today there is a possibility that you set tx loopback 
forever. The lock could be only in that section.

The comment could be:
/* Mutex to protect the device configuration data,
 * we can't set channels concurrently */

--
[]'s
Herton

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

[Michael Buesch]

On Wednesday 30 July 2008 16:53:13 Herton Ronaldo Krzesinski wrote:
> Em Wednesday 30 July 2008 10:27:26 Michael Buesch escreveu:
> > On Wednesday 30 July 2008 15:24:40 Michael Buesch wrote:
> > > On Wednesday 30 July 2008 08:12:36 Larry Finger wrote:
> > > > Herton,
> > > >
> > > > Does this patch help your problem? I haven't done much testing, but
> > > > things seem to be better with it here.
> > > >
> > > > Larry
> > > >
> > > > ==================
> > > >
> > > > Some users of the RTL8187B have experienced difficulties since commit
> > > > 8f87dd7e540d455f8e7f11478133b85edc969c67 that introduced the power
> > > > management wext hooks. This difficulty has not made much sense until
> > > > it was realized that it was possible for mac80211 to make a call to the
> > > > config routine while that routine was already being executed. This
> > > > patch protects the critical section with a mutex.
> > > >
> > > > Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> > > > ---
> > > >
> > > > Index: wireless-testing/drivers/net/wireless/rtl8187.h
> > > > ===================================================================
> > > > --- wireless-testing.orig/drivers/net/wireless/rtl8187.h
> > > > +++ wireless-testing/drivers/net/wireless/rtl8187.h
> > > > @@ -94,6 +94,7 @@ struct rtl8187_priv {
> > > >   	const struct rtl818x_rf_ops *rf;
> > > >   	struct ieee80211_vif *vif;
> > > >   	int mode;
> > > > +	struct mutex mutex;	/* used to lock config callback */
> > >
> > > Well, hm.
> > > Using locks to protect code is a bad idea, most of the time.
> > > So that comment makes no sense.
> > >
> > > What that lock _probably_ does it to protect the configuration
> > > data in struct rtl8187_priv. So you'd better find out which data
> > > that is and clarify the comment.
> >
> > and the mutex name, of course.
> >
> > 	/* Mutex to protect the device configuration data,
> > 	 * which is foobar and bizzbaz */
> > 	struct mutex conf_mutex;
> 
> Yes, it's better this way. About the lock, the problem here is you can't set 
> the channel while transmitting data on 8187 (the card stops working util 
> reset like the comment on the code), so we must enable tx loopback while 
> setting channels, but you can't run rtl8187_config concurrently because one 
> instance may be disabling tx loopback while other is still setting channel, 
> or like the code is today there is a possibility that you set tx loopback 
> forever. The lock could be only in that section.
> 
> The comment could be:
> /* Mutex to protect the device configuration data,
>  * we can't set channels concurrently */

I think you probably want to protect the tx-loopback (enabled or disabled) state.
That's what you're actually doing implicitely. The problem is not any channel
concurrency or something like that, but that the tx-loopback-enable/disable is not
recursive is the real thing we want to lock here, probably.

Note that I didn't read the code.
But in general you get the idea. Don't lock code, but lock state data. (like
the loopback-state data).

-- 
Greetings Michael.

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

[Herton Ronaldo Krzesinski]

Em Wednesday 30 July 2008 12:02:30 Michael Buesch escreveu:
> On Wednesday 30 July 2008 16:53:13 Herton Ronaldo Krzesinski wrote:
> > Em Wednesday 30 July 2008 10:27:26 Michael Buesch escreveu:
> > > On Wednesday 30 July 2008 15:24:40 Michael Buesch wrote:
<snip>
> > > and the mutex name, of course.
> > >
> > > 	/* Mutex to protect the device configuration data,
> > > 	 * which is foobar and bizzbaz */
> > > 	struct mutex conf_mutex;
> >
> > Yes, it's better this way. About the lock, the problem here is you can't
> > set the channel while transmitting data on 8187 (the card stops working
> > util reset like the comment on the code), so we must enable tx loopback
> > while setting channels, but you can't run rtl8187_config concurrently
> > because one instance may be disabling tx loopback while other is still
> > setting channel, or like the code is today there is a possibility that
> > you set tx loopback forever. The lock could be only in that section.
> >
> > The comment could be:
> > /* Mutex to protect the device configuration data,
> >  * we can't set channels concurrently */
>
> I think you probably want to protect the tx-loopback (enabled or disabled)
> state. That's what you're actually doing implicitely. The problem is not
> any channel concurrency or something like that, but that the
> tx-loopback-enable/disable is not recursive is the real thing we want to
> lock here, probably.

No, the issue is that I must enter in tx-loopback mode to set the channels 
inside rtl8187_config, to avoid transmission of packets, so the hardware 
doesn't halt. So that's why the lock, it must cover the entire section 
of "enable loopback"->"set_chan"->"disable loopback", not only the state of 
TX_CONF register.

>
> Note that I didn't read the code.
> But in general you get the idea. Don't lock code, but lock state data.
> (like the loopback-state data).

--
[]'s
Herton

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

[Michael Buesch]

On Wednesday 30 July 2008 18:08:01 Herton Ronaldo Krzesinski wrote:
> Em Wednesday 30 July 2008 12:02:30 Michael Buesch escreveu:
> > On Wednesday 30 July 2008 16:53:13 Herton Ronaldo Krzesinski wrote:
> > > Em Wednesday 30 July 2008 10:27:26 Michael Buesch escreveu:
> > > > On Wednesday 30 July 2008 15:24:40 Michael Buesch wrote:
> <snip>
> > > > and the mutex name, of course.
> > > >
> > > > 	/* Mutex to protect the device configuration data,
> > > > 	 * which is foobar and bizzbaz */
> > > > 	struct mutex conf_mutex;
> > >
> > > Yes, it's better this way. About the lock, the problem here is you can't
> > > set the channel while transmitting data on 8187 (the card stops working
> > > util reset like the comment on the code), so we must enable tx loopback
> > > while setting channels, but you can't run rtl8187_config concurrently
> > > because one instance may be disabling tx loopback while other is still
> > > setting channel, or like the code is today there is a possibility that
> > > you set tx loopback forever. The lock could be only in that section.
> > >
> > > The comment could be:
> > > /* Mutex to protect the device configuration data,
> > >  * we can't set channels concurrently */
> >
> > I think you probably want to protect the tx-loopback (enabled or disabled)
> > state. That's what you're actually doing implicitely. The problem is not
> > any channel concurrency or something like that, but that the
> > tx-loopback-enable/disable is not recursive is the real thing we want to
> > lock here, probably.
> 
> No, the issue is that I must enter in tx-loopback mode to set the channels 
> inside rtl8187_config, to avoid transmission of packets, so the hardware 
> doesn't halt. So that's why the lock, it must cover the entire section 
> of "enable loopback"->"set_chan"->"disable loopback", not only the state of 
> TX_CONF register.

Yeah, I said exactly that.
You protect the loopback stuff. Not any config callback or anything else.

-- 
Greetings Michael.

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

[Herton Ronaldo Krzesinski]

Em Wednesday 30 July 2008 13:16:37 Michael Buesch escreveu:
> On Wednesday 30 July 2008 18:08:01 Herton Ronaldo Krzesinski wrote:
> > Em Wednesday 30 July 2008 12:02:30 Michael Buesch escreveu:
> > > On Wednesday 30 July 2008 16:53:13 Herton Ronaldo Krzesinski wrote:
> > > > Em Wednesday 30 July 2008 10:27:26 Michael Buesch escreveu:
> > > > > On Wednesday 30 July 2008 15:24:40 Michael Buesch wrote:
> >
> > <snip>
> >
> > > > > and the mutex name, of course.
> > > > >
> > > > > 	/* Mutex to protect the device configuration data,
> > > > > 	 * which is foobar and bizzbaz */
> > > > > 	struct mutex conf_mutex;
> > > >
> > > > Yes, it's better this way. About the lock, the problem here is you
> > > > can't set the channel while transmitting data on 8187 (the card stops
> > > > working util reset like the comment on the code), so we must enable
> > > > tx loopback while setting channels, but you can't run rtl8187_config
> > > > concurrently because one instance may be disabling tx loopback while
> > > > other is still setting channel, or like the code is today there is a
> > > > possibility that you set tx loopback forever. The lock could be only
> > > > in that section.
> > > >
> > > > The comment could be:
> > > > /* Mutex to protect the device configuration data,
> > > >  * we can't set channels concurrently */
> > >
> > > I think you probably want to protect the tx-loopback (enabled or
> > > disabled) state. That's what you're actually doing implicitely. The
> > > problem is not any channel concurrency or something like that, but that
> > > the
> > > tx-loopback-enable/disable is not recursive is the real thing we want
> > > to lock here, probably.
> >
> > No, the issue is that I must enter in tx-loopback mode to set the
> > channels inside rtl8187_config, to avoid transmission of packets, so the
> > hardware doesn't halt. So that's why the lock, it must cover the entire
> > section of "enable loopback"->"set_chan"->"disable loopback", not only
> > the state of TX_CONF register.
>
> Yeah, I said exactly that.
> You protect the loopback stuff. Not any config callback or anything else.

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),


-- 
[]'s
Herton

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

[Larry Finger]

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

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

[Michael Buesch]

On Wednesday 30 July 2008 19:13:52 Herton Ronaldo Krzesinski wrote:
> > Yeah, I said exactly that.
> > You protect the loopback stuff. Not any config callback or anything else.
> 
> Ah ok, only protect the section, like this?

Yeah, well. In theory, yes. In practice: Aren't there other races possible, too?
I mean even races with other parts of the driver.
b43 needs to take the global driver mutex in the conf callback to make
sure nothing changes (device init state probably is the most important one.
Device going down while configuring would be a fatal crash).

So in b43 we have a global mutex which protects everything (all data
and all device state), except the data and state that's accessed in the IRQ paths.
(We have more locks for shared device memory and so on... But these are nested
inside of the mutex or the IRQ state lock)

-- 
Greetings Michael.

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

[Herton Ronaldo Krzesinski]

Em Wednesday 30 July 2008 14:46:40 Michael Buesch escreveu:
> On Wednesday 30 July 2008 19:13:52 Herton Ronaldo Krzesinski wrote:
> > > Yeah, I said exactly that.
> > > You protect the loopback stuff. Not any config callback or anything else.
> > 
> > Ah ok, only protect the section, like this?
> 
> Yeah, well. In theory, yes. In practice: Aren't there other races possible, too?
> I mean even races with other parts of the driver.
> b43 needs to take the global driver mutex in the conf callback to make
> sure nothing changes (device init state probably is the most important one.
> Device going down while configuring would be a fatal crash).

In practice I don't get other problems, but better to stick to the patch posted
by Larry, like he said to avoid config data from one call mixed to the other.
rtl8187 is more simpler, besides this issue with configuration it all depends
on how mac80211 calls another functions, I have yet to see but doesn't look like
a general lock is needed.

> 
> So in b43 we have a global mutex which protects everything (all data
> and all device state), except the data and state that's accessed in the IRQ paths.
> (We have more locks for shared device memory and so on... But these are nested
> inside of the mutex or the IRQ state lock)
> 

-- 
[]'s
Herton

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

[Michael Buesch]

On Wednesday 30 July 2008 20:11:11 Herton Ronaldo Krzesinski wrote:
> Em Wednesday 30 July 2008 14:46:40 Michael Buesch escreveu:
> > On Wednesday 30 July 2008 19:13:52 Herton Ronaldo Krzesinski wrote:
> > > > Yeah, I said exactly that.
> > > > You protect the loopback stuff. Not any config callback or anything else.
> > > 
> > > Ah ok, only protect the section, like this?
> > 
> > Yeah, well. In theory, yes. In practice: Aren't there other races possible, too?
> > I mean even races with other parts of the driver.
> > b43 needs to take the global driver mutex in the conf callback to make
> > sure nothing changes (device init state probably is the most important one.
> > Device going down while configuring would be a fatal crash).
> 
> In practice I don't get other problems, but better to stick to the patch posted
> by Larry,

Yeah, OK. I'm fine with that. I was just pointing out possible future problems.

> like he said to avoid config data from one call mixed to the other. 

Nah, we're not locking calls ;P

-- 
Greetings Michael.

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

[Hin-Tak Leung]

On 7/30/08, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:
> Em Wednesday 30 July 2008 14:46:40 Michael Buesch escreveu:
>
> > On Wednesday 30 July 2008 19:13:52 Herton Ronaldo Krzesinski wrote:
>  > > > Yeah, I said exactly that.
>  > > > You protect the loopback stuff. Not any config callback or anything else.
>  > >
>  > > Ah ok, only protect the section, like this?
>  >
>  > Yeah, well. In theory, yes. In practice: Aren't there other races possible, too?
>  > I mean even races with other parts of the driver.
>  > b43 needs to take the global driver mutex in the conf callback to make
>  > sure nothing changes (device init state probably is the most important one.
>  > Device going down while configuring would be a fatal crash).
>
>
> In practice I don't get other problems, but better to stick to the patch posted
>  by Larry, like he said to avoid config data from one call mixed to the other.
>  rtl8187 is more simpler, besides this issue with configuration it all depends
>  on how mac80211 calls another functions, I have yet to see but doesn't look like
>  a general lock is needed.

I have been wondering about two things: (1) I occasionally have problems
modprobe -r (particularly right after a bit of use and the driver is
stuck), ksoftirqd just suck CPU cycles like crazy in some timer code.
It seems to be less often if I just leave it for some time before
modprobe -r. I am on a dual core system. (2) although Larry says
wpa_supplicant works, I can't get it to work for me at all, but the
driver is alright and quite useable with ifconfig/iwconfig, etc.

Had a look at wpa_supplicant itself and it doesn't seem to do much
more than the iwconfig/iwlist stuff, but obviously it does it job
faster (and potentially through two cores).

The wpa_supplicant issue is probably also about config data, but I
wonder if the shutdown also needs some sort of spin lock. Some of the
other drivers has spin locks,
and rtl8187 actually is one with the fewest spin locks.


>
>
>  >
>  > So in b43 we have a global mutex which protects everything (all data
>  > and all device state), except the data and state that's accessed in the IRQ paths.
>  > (We have more locks for shared device memory and so on... But these are nested
>  > inside of the mutex or the IRQ state lock)
>  >
>
>  --
>
> []'s
>
> Herton
>

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

[Herton Ronaldo Krzesinski]

Em Wednesday 30 July 2008 03:12:36 Larry Finger escreveu:
> Herton,
>
> Does this patch help your problem? I haven't done much testing, but things
> seem to be better with it here.
>
> Larry
>

Acked-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>

It works fine here and solves the problem, it was a solution I had in mind 
too.

> ==================
>
> Some users of the RTL8187B have experienced difficulties since commit
> 8f87dd7e540d455f8e7f11478133b85edc969c67 that introduced the power
> management wext hooks. This difficulty has not made much sense until
> it was realized that it was possible for mac80211 to make a call to the
> config routine while that routine was already being executed. This patch
> protects the critical section with a mutex.
>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
>
> Index: wireless-testing/drivers/net/wireless/rtl8187.h
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/rtl8187.h
> +++ wireless-testing/drivers/net/wireless/rtl8187.h
> @@ -94,6 +94,7 @@ struct rtl8187_priv {
>   	const struct rtl818x_rf_ops *rf;
>   	struct ieee80211_vif *vif;
>   	int mode;
> +	struct mutex mutex;	/* used to lock config callback */
>
>   	/* rtl8187 specific */
>   	struct ieee80211_channel channels[14];
> Index: wireless-testing/drivers/net/wireless/rtl8187_dev.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/rtl8187_dev.c
> +++ wireless-testing/drivers/net/wireless/rtl8187_dev.c
> @@ -864,6 +864,7 @@ static int rtl8187_config(struct ieee802
>   	struct rtl8187_priv *priv = dev->priv;
>   	u32 reg;
>
> +	mutex_lock(&priv->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
> @@ -896,6 +897,7 @@ static int rtl8187_config(struct ieee802
>   	rtl818x_iowrite16(priv, &priv->map->ATIMTR_INTERVAL, 100);
>   	rtl818x_iowrite16(priv, &priv->map->BEACON_INTERVAL, 100);
>   	rtl818x_iowrite16(priv, &priv->map->BEACON_INTERVAL_TIME, 100);
> +	mutex_unlock(&priv->mutex);
>   	return 0;
>   }
>
> @@ -1187,6 +1189,7 @@ static int __devinit rtl8187_probe(struc
>   		printk(KERN_ERR "rtl8187: Cannot register device\n");
>   		goto err_free_dev;
>   	}
> +	mutex_init(&priv->mutex);
>
>   	printk(KERN_INFO "%s: hwaddr %s, %s V%d + %s\n",
>   	       wiphy_name(dev->wiphy), print_mac(mac, dev->wiphy->perm_addr),

-- 
[]'s
Herton