ieee80211_scan_completed() calling config() and possible deadlock

ieee80211_scan_completed() calling config() and possible deadlock

[Kalle Valo]

Hello,

I'm currently wondering how to implement the locking
ieee80211_scan_completed() and I would like hear comments from people.

In wl12xx and at76c50x-usb the locking was originally implemented such
that drivers' config() function acquired a mutex. Also elsewhere the
drivers called ieee80211_scan_completed() under the same mutex. As we
know, ieee80211_scan_completed() calls ieee80211_hw_config() which
again calls driver's config() function. So this ended up to a
deadlock. The problem was easy to fix, just release the mutex in the
driver before calling the completed() function.

Now I need to add a similar function, currently named
ieee80211_beacon_loss(), for the beacon filtering patches I'm working
on. The function will disassociate whenever driver calls it. While
experimenting with stlc45xx I noticed that I have a similar problem as
with ieee80211_scan_completed(), ieee80211_beacon_loss() calls
driver's config() function eventually and I had a deadlock in stlc45xx.

I'm just wondering what's the right way(tm) to handle this. I see two
options:

1. Consider the (possible) deadlock as a feature, document it and let
   the drivers handle it. This is relatively easy.

2. Handle this in mac80211 (eg. schedule a workqueue) and drivers
   don't need to care. This might complicate mac80211 implementation a
   bit, but easier for the drivers.

I myself cannot decide which one is better. What do people think? 

-- 
Kalle Valo

Re: ieee80211_scan_completed() calling config() and possible deadlock

[Michael Buesch]

On Monday 23 February 2009 14:05:20 Kalle Valo wrote:
> Hello,
> 
> I'm currently wondering how to implement the locking
> ieee80211_scan_completed() and I would like hear comments from people.
> 
> In wl12xx and at76c50x-usb the locking was originally implemented such
> that drivers' config() function acquired a mutex. Also elsewhere the
> drivers called ieee80211_scan_completed() under the same mutex. As we
> know, ieee80211_scan_completed() calls ieee80211_hw_config() which
> again calls driver's config() function. So this ended up to a
> deadlock. The problem was easy to fix, just release the mutex in the
> driver before calling the completed() function.
> 
> Now I need to add a similar function, currently named
> ieee80211_beacon_loss(), for the beacon filtering patches I'm working
> on. The function will disassociate whenever driver calls it. While
> experimenting with stlc45xx I noticed that I have a similar problem as
> with ieee80211_scan_completed(), ieee80211_beacon_loss() calls
> driver's config() function eventually and I had a deadlock in stlc45xx.
> 
> I'm just wondering what's the right way(tm) to handle this. I see two
> options:
> 
> 1. Consider the (possible) deadlock as a feature, document it and let
>    the drivers handle it. This is relatively easy.
> 
> 2. Handle this in mac80211 (eg. schedule a workqueue) and drivers
>    don't need to care. This might complicate mac80211 implementation a
>    bit, but easier for the drivers.
> 
> I myself cannot decide which one is better. What do people think? 
> 

I think drivers should not be able to call ieee80211_scan_completed() directly.
Instead they should call a function which schedules ieee80211_scan_completed()
on a workqueue.
In general I consider it broken behavior, if a function called by
the driver can recurse into the driver.
We had that behavior in ieee80211-softmac and it was one of the main
reasons it sucked so much.

The wq schedule code is trivial to implement in mac80211 and it's also OK
to do so. The function is not required to execute synchronously.

-- 
Greetings, Michael.

Re: ieee80211_scan_completed() calling config() and possible deadlock

[Kalle Valo]

Michael Buesch <mb@bu3sch.de> writes:

>> I'm just wondering what's the right way(tm) to handle this. I see
>> two options:
>> 
>> 1. Consider the (possible) deadlock as a feature, document it and
>> let the drivers handle it. This is relatively easy.
>> 
>> 2. Handle this in mac80211 (eg. schedule a workqueue) and drivers
>> don't need to care. This might complicate mac80211 implementation a
>> bit, but easier for the drivers.
>> 
>> I myself cannot decide which one is better. What do people think?
>> 
>
> I think drivers should not be able to call ieee80211_scan_completed()
> directly. Instead they should call a function which schedules
> ieee80211_scan_completed() on a workqueue.

Yes, that was my option two.

> In general I consider it broken behavior, if a function called by
> the driver can recurse into the driver. We had that behavior in
> ieee80211-softmac and it was one of the main reasons it sucked so
> much.

Ok, this is a very strong argument in favor of option 2.

> The wq schedule code is trivial to implement in mac80211 and it's also
> OK to do so. The function is not required to execute synchronously.

I'm leaning on option two then. Thanks for the feedback!

-- 
Kalle Valo