On Thu, Oct 2, 2008 at 11:37 AM, Johannes Berg
<johannes@xxxxxxxxxxxxxxxx> wrote:
> 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'?
We use spec numbers here why to invent more numbers.
>> + /* 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...?
This is explicitly defined in spec for STA mode.
>
>> + 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
Will fix.
>
> 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 should not be exported to driver, I think I was clear in the last
mail that I didn't like it myself
and I'm looking for popper way to do that.
> 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?
No it should be set on registration only
What sort of policy does the driver impose on
> them? Why is this policy driver-specific?
Depends on radio ability rather then policy. The only policy that is
set here is that we coupling SM PS and PS
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?
Correct that's the solution, just keep in mind that driver change rx
chain configuration upon SM PS request as well.
> 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 don't like it either, the development strategy is first make it work
and then beautify and optimize
> 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 (?)
Not enough there are 3 states you may request in SM_PS
> * some way to tell mac80211 that MIMO isn't currently effective
Yes that we shell implement.
> * mac80211 determines the SM PS mode based on
> - the two inputs from the driver above
> - user input
Correct
> - association status (no need for all chains when not associated)
SM_PS mode has meaning only in association state, before association
it will only make sense in what state we enter association and we may
announce our SM_PS state already in association.
> - possibly interface modes (maybe no need for MIMO when in mesh etc.)
Out of scope, HT is not enabled at all in these modes.
> * mac80211 notifies the driver about the SM PS mode it should switch to
> via the hw config callback
That's the current design
> 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.
I didn't submit any patch for that yet, believe me if I thought it's
closed issue you will have it in your mailbox already :)
Thanks for your valuable input
Tomas
--
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
