On Fri, 2009-05-15 at 00:48 -0400, Luis R. Rodriguez wrote:
[...]
Looks pretty good, some comments.
> static inline int __ieee80211_resume(struct ieee80211_hw *hw)
> {
> + hw_to_local(hw)->suspended = false;
> return ieee80211_reconfig(hw_to_local(hw));
Shouldn't we do that only somewhere in the middle of reconfig? i.e.
after we actually reconfig the hw, but before we restart our timers?
> @@ -2196,6 +2207,18 @@ static void ieee80211_restart_sta_timer(struct ieee80211_sub_if_data *sdata)
> }
> }
>
> +void ieee80211_sta_setup_sdata_timers(struct ieee80211_sub_if_data *sdata)
> +{
> + struct ieee80211_if_managed *ifmgd;
> +
> + ifmgd = &sdata->u.mgd;
> +
> + setup_timer(&ifmgd->timer, ieee80211_sta_timer,
> + (unsigned long) sdata);
> + setup_timer(&ifmgd->chswitch_timer, ieee80211_chswitch_timer,
> + (unsigned long) sdata);
> +}
That doesn't seem necessary? The timers should still be set up after you
delete them.
> +++ b/net/mac80211/pm.c
> @@ -18,6 +18,10 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
>
> flush_workqueue(local->hw.workqueue);
>
> + /* Don't stuff our workqueue during suspend->resume cycle */
> + del_timer_sync(&local->dynamic_ps_timer);
> + cancel_work_sync(&local->dynamic_ps_enable_work);
> +
ack, but you forgot to add a comment why dynamic_ps_disable_work doesn't
need to be handled (actually it does need to handled with the current
code but shouldn't be necessary... up for a challenge? -- why? and how
to fix that better than handling it here?)
If you ever suspend in AP mode there's also an issue with each station's
cleanup timer, and for mesh each station's plink timer.
> @@ -52,6 +56,18 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
>
> /* remove all interfaces */
> list_for_each_entry(sdata, &local->interfaces, list) {
> + /*
> + * This prevents us from stuffing our workqueue during
> + * during the suspend->resume cycle.
> + */
> + if (sdata->vif.type == NL80211_IFTYPE_STATION) {
> + struct ieee80211_if_managed *ifmgd;
> + ifmgd = &sdata->u.mgd;
> +
> + del_timer_sync(&ifmgd->timer);
> + del_timer_sync(&ifmgd->chswitch_timer);
> + }
Might make sense to use a switch statement, put the break; from
netif_running after it and for the two types the driver doesn't care
into it, like this
if (!netif_running())
continue;
switch (type) {
case station:
del_timer_sync etc
break;
case ap_vlan:
case monitor:
continue;
case ...
}
conf stuff
because you forgot IBSS and mesh.
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -1121,12 +1121,21 @@ int ieee80211_reconfig(struct ieee80211_local *local)
> }
>
> /* add back keys */
> - list_for_each_entry(sdata, &local->interfaces, list)
> + list_for_each_entry(sdata, &local->interfaces, list) {
> if (netif_running(sdata->dev))
> ieee80211_enable_keys(sdata);
> + if (local->suspended &&
> + sdata->vif.type == NL80211_IFTYPE_STATION)
> + ieee80211_sta_setup_sdata_timers(sdata);
> + }
I don't think that helps, like I said the timers are still set up, you
need to actually queue the work now. However that kinda sucks for small
suspend times. Not really sure what to do.
What we should do is probably add new functions in mlme.c, ibss.c,
mesh.c for suspend() and resume() that contain all this logic instead of
putting it here -- that way mesh is easier to handle without ifdefs in
the code too.
> + if (local->suspended) {
> + setup_timer(&local->dynamic_ps_timer,
> + ieee80211_dynamic_ps_timer, (unsigned long) local);
> + }
> +
I think that one is completely unnecessary since now traffic is flowing
which will call mod_timer anyway.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
