Re: [PATCH 5/7] mac80211: Expand powersave configuration flag to be two bits

[Seth Forshee <seth.forshee@canonical.com>]

On Thu, Jan 31, 2013 at 06:50:34PM +0100, Johannes Berg wrote:
> On Thu, 2013-01-31 at 11:18 -0600, Seth Forshee wrote:
> 
> > > > Actually one of the last bugs I fixed before sending these was a place
> > > > where I had used disabled instead of !enabled, and the frames ended up
> > > > with PM set when it shouldn't have been.
> > > > 
> > > > I agree though that the distinction is confusing. Maybe some better
> > > > state names are needed. Perhaps awake, offchannel, and doze?
> > > 
> > > I think what you really want is to distinguish between "HW can go to
> > > powersave" and "PM bit should be set"? That's pretty much what your
> > > CONF_PS_ENABLED and CONF_PS_OFFCHANNEL means, respectively, but maybe
> > 
> > Correct, with the understanding that "HW can go to powersave" also
> > implies "PM bit should be set."
> > 
> > Another approach would be to keep the CONF_PS flag the same and add a
> > CONF_PM flag or similar. I didn't go with this approach because CONF_PS
> > && !CONF_PM really doesn't make any sense, which really doesn't help
> > with reducing confusion. The advantage is that it separates setting PM
> > from PS for those driver that don't support PS but need to configure the
> > hardware to set PM for off-channel.
> 
> Good point, that'd work too. PS && !PM would never be used, I guess?
> It'd also have the advantage of not having to touch all the drivers?
> 
> It doesn't really matter all that much to me though, I just think what
> you have right now is (too) confusing.

Hi Johannes,

I've been thinking about and playing around with these ideas. I've
implemented the CONF_PM idea, and it does end up with fewer changes, but
I just don't think separating powersave from setting PM makes much
sense. In the end it just seems like a kludge to fix a problem with
Broadcom chips, and if I want a kludge I can do it entirely within the
driver.

So what I'm planning to do know is implement the awake/doze/offchannel
powersave modes like I had mentioned, but taking things a bit further.
I'd change IEEE80211_HW_SUPPORTS_PS to SUPPORTS_PS_DOZE, indicating
support for a low-power powersave state rather than support for
powersave in general. All hardware will be reconfigured for
awake<->offchannel transitions (though most drivers can simply ignore
these transitions), but hardware will only be put in the doze state if
it indicates PS_DOZE support.

This will make it compulsory for all drivers to indicate whether or not
they require IEEE802111_HW_PS_NULLFUNC_STACK. I'll simply set this flag
for any drivers not currently supporting powersave.

In practice the changes shouldn't end up much different than what I have
in these patches, but I think it's conceptually cleaner (and less
confusing!). Does this sound reasonable to you?

Thanks,
Seth