rewriting powersave

rewriting powersave

[Johannes Berg]

All,

Let's more seriously think about rewriting powersave. We need to be able
to handle multi-vif powersave, and nobody even claims to understand the
current code any more, let alone knows how to fix the bugs in it.

We have three flags:
* IEEE80211_HW_SUPPORTS_PS
* IEEE80211_HW_PS_NULLFUNC_STACK
* IEEE80211_HW_SUPPORTS_DYNAMIC_PS
but obviously not all combinations make sense. The valid combinations
are:

1) !SUPPORTS_PS (d'oh)
2) SUPPORTS_PS
3) SUPPORTS_PS | NULLFUNC_STACK
4) SUPPORTS_PS | SUPPORTS_DYNAMIC_PS

The drivers seem to use them:
1) didn't check, probably plenty
2) wl1251, wl12xx (though the latter is moving to 4) I believe)
3) ath9k_htc, ath9k, carl9170, p54, rt2x00, rtlwifi
4) iwlwifi, iwlegacy

So by far the most common are 1 and 3, with 2 being a dying breed and 4
being obviously trivial.


The interesting thing here is that the difference between 2 and 3 is
relatively small, except for the off-channel case where case 2 is really
confusing. But wl1251 has hardware scan, so we don't have to worry about
that all that much. (The auth/assoc work case is curious but I think we
need to do that differently soon enough anyway)

Since clearly it is possible to implement this in the lower layers, I'm
thinking we should provide an implementation that the drivers can use to
implement powersave and be in category 4 from mac80211's POV.

I think the API for case 3 could look something like this:

struct powersave {
	...
};

struct powersave_ops {
	void (*tx_nulldata)(void *ctx, ...);
	void (*set_radio)(void *ctx, bool on);
	void (*timeout_radio_off)(void *ctx);
};

void powersave_init(struct powersave *ps, struct powersave_ops *ops, void *ctx);
void powersave_deinit(struct powersave *ps);

void powersave_start(struct powersave *ps);
void powersave_stop(struct powersave *ps);
bool powersave_tx(struct powersave *ps);
void powersave_rx(struct powersave *ps);
void powersave_nulldata_status(struct powersave *ps, bool ack);

/* maybe for wl12xx's BT implementation: */
void powersave_disable_dynps(struct powersave *ps);
void powersave_enable_dynps(struct powersave *ps);


I think this would be created per (managed) interface by the driver. I
don't think we need to hide the internals, and that makes it easier to
just have such a struct in the vif struct.

Additional APIs might be needed for some of the configuration
parameters, not really sure yet.

The API rules would be that the set_radio() callback is always called
within a function call, which is why timeout_radio_off() is a separate
callback (locking would be different).


I haven't really full thought this through but it doesn't seem
impossible to do. Thoughts?

johannes

Re: rewriting powersave

[Johannes Berg]

On Thu, 2011-11-10 at 13:44 +0100, Johannes Berg wrote:

> Since clearly it is possible to implement this in the lower layers, I'm
> thinking we should provide an implementation that the drivers can use to
> implement powersave and be in category 4 from mac80211's POV.
> 
> I think the API for case 3 could look something like this:
> 
> struct powersave {
> 	...
> };
> 
> struct powersave_ops {
> 	void (*tx_nulldata)(void *ctx, ...);
> 	void (*set_radio)(void *ctx, bool on);
> 	void (*timeout_radio_off)(void *ctx);
> };

[...]

Also, the stick will be I'm going to remove the existing implementation
eventually, and the carrot will be that you'll get PS for multiple
virtual interfaces with this and I'm not going to take it directly into
mac80211 :-)

johannes

Re: rewriting powersave

[Kalle Valo]

Hi Johannes,

Johannes Berg <johannes@sipsolutions.net> writes:

> Since clearly it is possible to implement this in the lower layers, I'm
> thinking we should provide an implementation that the drivers can use to
> implement powersave and be in category 4 from mac80211's POV.

I think this is the way to go, even I was skeptic when you first
mentioned about. We really need to make mac80211 powersave
implementation more simple and this is a clean way to do it. And I
would assume that all new (sane) hardware designs belong to category 4
so this would only affect existing drivers.

-- 
Kalle Valo