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
