On Tue, 2008-09-30 at 23:07 +0300, Tomas Winkler wrote: Some code comments first, I'll cover the design below. > +static void ieee80211_send_sm_ps(struct ieee80211_sub_if_data *sdata, u8 mode) Can we use an enum for 'mode'? > + /* Implemented for STA only */ > + if (sdata->vif.type != NL80211_IFTYPE_STATION) > + return; What about mesh, ibss, wds, ...? I see this doesn't make much sense for an AP, but...? > + switch (mode) { > + spurious blank line > + if (ifsta->flags & IEEE80211_STA_ASSOCIATED) > + ieee80211_tx_skb(sdata, skb, 0); Umm. Why don't you check this before the allocation so that it doesn't leak? Also, this probably should return the out of memory status so that the driver can decide to not switch SM PS mode in that case? Although then the system is probably pretty much dead anyway... > + rcu_read_lock(); > + list_for_each_entry_rcu(sdata, &local->interfaces, list) { > + ieee80211_send_sm_ps(sdata, new_mode); > + } no need for braces there So, let's get to the design, it's really confusing. Especially with patch 2 in there as well, this gets _really_ confusing. On the one hand, this is being called by mac80211 when the user changes the power save mode and in that case the driver is notified of the powersave mode, but on the other hand this is also exported to drivers? It seems that for the second use case you're citing you've now done something that might lead to bouncing back and forth because there is no central instance deciding on the powersave mode. You seem to be trying to avoid this by introducing the two new variables sm_ps_psp_mode and sm_ps_cam_mode, but this seems very strange. Is the driver supposed to modify them at runtime? What sort of policy does the driver impose on them? Why is this policy driver-specific? Why doesn't the driver just inform mac80211 of the antenna status and mac80211 then decides based on the antenna status and the requested powersave mode which SM PS mode should be activated, and then notifies the driver of that? I don't like this patch. You're putting some policy into the driver that seems should not be different between drivers. Also, these backward calls from the driver to mac80211 that just update the AP's status make it rather complex to get the API right, with multiple variables that need updating. I'd much rather see you implement this in a different way: * a HW flag that determines whether the driver can wake up with or without an RTS frame (?) * some way to tell mac80211 that MIMO isn't currently effective * mac80211 determines the SM PS mode based on - the two inputs from the driver above - user input - association status (no need for all chains when not associated) - possibly interface modes (maybe no need for MIMO when in mesh etc.) * mac80211 notifies the driver about the SM PS mode it should switch to via the hw config callback I'm worried especially about the third bullet point, you've implemented the fourth but the driver is allowed to override this and mac80211 can't even make a proper decision because it doesn't have the inputs. I don't think having the first two inputs to mac80211 is "opening too much guts", it is, after all, something the policy engine needs, and that policy engine shouldn't be split up between the driver and mac80211. johannes
Attachment:
signature.asc
Description: This is a digitally signed message part