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@xxxxxxxxxxxx>
> > > > ---
> > > >
> > > > 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
