On 24 March 2014 14:56, Luca Coelho <luca@xxxxxxxxx> wrote:
> On Fri, 2014-03-21 at 14:31 +0100, Michal Kazior wrote:
>> This exports a new cfg80211_stop_iface() function.
>>
>> This is intended for driver internal interface
>> combination management and channel switching.
>>
>> Due to locking issues (it re-enters driver) the
>> call is asynchronous and uses cfg80211 event
>> list/worker.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@xxxxxxxxx>
>> ---
>
> Looks good to me, with a couple of nitpicks.
>
> [...]
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index 2f28ac4..7f0cbfb 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -4705,6 +4705,21 @@ int cfg80211_check_combinations(struct wiphy *wiphy,
>> const u8 radar_detect,
>> const int iftype_num[NUM_NL80211_IFTYPES]);
>>
>> +/*
>> + * cfg80211_stop_iface - stop given interface
>> + *
>> + * @wiphy: the wiphy
>> + * @wdev: wireless device
>> + *
>> + * Stop interface as if AP was stopped, IBSS/mesh left, STA disconnected.
>
> This is not very clear. Maybe you should say that this allows the
> driver to _trigger_ an interface to be stopped, so that it's clear that
> all the stopping flow will take place.
Yeah, it's async so "trigger" makes more sense.
>> + *
>> + * This is intended for channel switching and internal driver interface
>> + * combination management.
>
> Not sure you need to say when this is needed. The driver could call it
> for any internal reason.
I'll remove this sentence then unless someone has an objection? :-)
> [...]
>> @@ -789,6 +788,37 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
>> wdev->beacon_interval = 0;
>> }
>>
>> +void cfg80211_leave(struct cfg80211_registered_device *rdev,
>> + struct wireless_dev *wdev)
>> +{
>> + ASSERT_RTNL();
>> +
>> + wdev_lock(wdev);
>> + __cfg80211_leave(rdev, wdev);
>> + wdev_unlock(wdev);
>> +}
>> +
>> +void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
>> +{
>> + struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
>> + struct cfg80211_event *ev;
>> + unsigned long flags;
>> +
>> + trace_cfg80211_stop_iface(wiphy, wdev);
>> +
>> + ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
>
> Is there any reason why you don't pass the GFP type to this function, as
> in all other functions that allow the driver to queue events?
Not really. I'll add the argument.
Michał
--
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
