[ resending - html mail rejected ]
On Sun, Jan 16, 2011 at 10:50, Johannes Berg <[email protected]> wrote:
>
> > +/**
> > + * ieee80211_start_ps - start PS for connected sta
> > + *
> > + * When operating in AP mode, use this function to inform mac80211
> > + * about a station entering PS mode.
> > + * Note that by default mac80211 will automatically call the internal
> > + * equivalent of this function according to the PM bit of incoming frames.
> > + * Use the IEEE80211_HW_AP_LINK_PS flag to change this.
>
> I think that explanation should be clearer and say that you must call
> this only when the flag is set and vice versa. Also, there should be a
>
> WARN_ON(hw->flags & HW_AP_LINK_PS)
>
> in the functions.
will fix in v2.
>
> Also, maybe just a single function with a bool argument would be more
> appropriate? Call it "ieee80211_sta_ps_transition()" or something like
> that. You should also remove the text about calling the internal
> equivalent -- the internal details might change at any time but the API
> should be documented in a way that makes sense without the internal
> details for the benefit of both sides -- people implementing a driver
> don't need to know about the internal details, and people changing the
> internal details know what semantics to keep.
a single function sounds good. v2.
>
> > + * This function may not be called in IRQ context or process context.
>
> This can't be true -- the latter part you must mean "or with softirqs
> enabled".
Yes you're right.
(While looking at this I discovered that I forgot to disable softirqs
myself. These functions are called from a workqueue in wl12xx - will
be fixed in v2)
>
> Also, I'm worried about races between this and RX. All of the RX path
> assumes that it is running at the same time. The ap_sta_ps_{start,end}
> functions won't be called from the RX path when the flag is set, and
> this is dependent on setting the flag, but is there really nothing in
> them that could race?
They might race if the user is not careful. On a SMP machine a user
can call ieee80211_rx() and?ap_sta_ps_start() from two different
workqueues for example.
I can add an explicit mutex, but a documentation warning will suffice here no?
>
> Finally, I'm worried about this calling back into the driver's
> sta_notify callback. If that is to remain that way, it needs to be
> *very* clearly documented, I'd *much* prefer it not doing that but
> handing off to a work struct or so internally.
>
sta_notify is pretty useless when the low level driver calls toggles
the PS-mode by itself.?How about I simply remove the call to
sta_notify in case?IEEE80211_HW_AP_LINK_PS is set?
If a low level driver needs to do some deferred processing after a
PS-mode transition, it can queue a work on its own..
Regards,
Arik
On Tue, Jan 18, 2011 at 11:05, Johannes Berg <[email protected]> wrote:
> On Tue, 2011-01-18 at 00:47 +0200, Arik Nemtsov wrote:
>
>
> Well if for instance the station happens to send two frames with the PM
> bit set, meaning that it is in PS-poll mode but doesn't want to wake up
> even though it's transmitting a frame, mac80211 will only call
> sta_notify once for the first frame to tell the driver the station went
> to sleep -- the second frame just indicates that it's still asleep.
>
> The same might happen with this, but of course it depends on what the
> device does. If the driver will call the new API function twice, then
> mac80211 will be very confused. Therefore, I think the new API function
> should do some checks about whether the station is already asleep/awake
> before invoking ap_sta_ps_start/end so that the counters are correct.
>
> In this case, the driver might want to know if the station was already
> in the indicated state -- this might be indicated by the return value.
>
Yea i missed the double atomic_inc/dec in case the function is called
twice. I'll add a EINVAL return value in case the requested PM state
is already in effect.
Thanks,
Arik
On Tue, 2011-01-18 at 00:47 +0200, Arik Nemtsov wrote:
> > anyway. I'd appreciate if you could actually see which things would
> > potentially race -- I took a brief look into these functions and it
> > didn't look like there are actually races except of these two against
> > each other maybe?
>
> Upon closer examination, I could find no races in the RX path. A race
> of ps_start/stop is possible of course, but I guess we can demand the
> calls to be synchronized against each other?
Yeah that seems fair. It'd be a strange driver too anyway, come to think
of it, since this is all from the device so needs to be processed from a
tasklet or whatever anyway.
> I did notice some concurrency that can happen in the TX path. I think
> the same essential race is described in
> ieee80211_handle_filtered_frame() between RX and TX paths. Only this
> time the PS state is changed manually and not in the RX handler. The
> warning in ieee80211_handle_filtered_frame() can be expanded to
> include:
> "always change PS state of connected stations before TX status events
> if ordering can be unknown, for example with different interrupt
> status bits."
Right, that was RX first, then TX status, I guess it can be extended a
bit. But really, this race doesn't apply since the entire PS state
management is done by the device here, and mac80211 just follows it.
> > > sta_notify is pretty useless when the low level driver calls toggles
> > > the PS-mode by itself. How about I simply remove the call to
> > > sta_notify in case IEEE80211_HW_AP_LINK_PS is set?
> > > If a low level driver needs to do some deferred processing after a
> > > PS-mode transition, it can queue a work on its own..
> >
> > Yes, that's probably a good idea -- but definitely needs to be
> > documented in the sta_notify docs. OTOH, mac80211 de-bounces sta_notify
> > in a way. Maybe that needs to be done for the function call as well,
> > maybe via a return value?
> >
>
> I'm afraid I didn't catch your meaning here.
Well if for instance the station happens to send two frames with the PM
bit set, meaning that it is in PS-poll mode but doesn't want to wake up
even though it's transmitting a frame, mac80211 will only call
sta_notify once for the first frame to tell the driver the station went
to sleep -- the second frame just indicates that it's still asleep.
The same might happen with this, but of course it depends on what the
device does. If the driver will call the new API function twice, then
mac80211 will be very confused. Therefore, I think the new API function
should do some checks about whether the station is already asleep/awake
before invoking ap_sta_ps_start/end so that the counters are correct.
In this case, the driver might want to know if the station was already
in the indicated state -- this might be indicated by the return value.
johannes
On Mon, Jan 17, 2011 at 11:35, Johannes Berg <[email protected]> wrote:
>
> On Sun, 2011-01-16 at 23:53 +0200, Arik Nemtsov wrote:
>
> > > Also, I'm worried about races between this and RX. All of the RX path
> > > assumes that it is running at the same time. The ap_sta_ps_{start,end}
> > > functions won't be called from the RX path when the flag is set, and
> > > this is dependent on setting the flag, but is there really nothing in
> > > them that could race?
> >
> > They might race if the user is not careful. On a SMP machine a user
> > can call ieee80211_rx() and ap_sta_ps_start() from two different
> > workqueues for example.
> > I can add an explicit mutex, but a documentation warning will suffice here no?
>
> Well, a mutex won't be possible, and we don't do "code path locking"
I meant a spinlock of course (but its not really a good idea)
>
> anyway. I'd appreciate if you could actually see which things would
> potentially race -- I took a brief look into these functions and it
> didn't look like there are actually races except of these two against
> each other maybe?
Upon closer examination, I could find no races in the RX path. A race
of ps_start/stop is possible of course, but I guess we can demand the
calls to be synchronized against each other?
I did notice some concurrency that can happen in the TX path. I think
the same essential race is described in
ieee80211_handle_filtered_frame() between RX and TX paths. Only this
time the PS state is changed manually and not in the RX handler. The
warning in?ieee80211_handle_filtered_frame() can be expanded?to
include:
"always change PS state of connected stations before TX status events
if ordering can be unknown, for example with different interrupt
status bits."
> > sta_notify is pretty useless when the low level driver calls toggles
> > the PS-mode by itself. How about I simply remove the call to
> > sta_notify in case IEEE80211_HW_AP_LINK_PS is set?
> > If a low level driver needs to do some deferred processing after a
> > PS-mode transition, it can queue a work on its own..
>
> Yes, that's probably a good idea -- but definitely needs to be
> documented in the sta_notify docs. OTOH, mac80211 de-bounces sta_notify
> in a way. Maybe that needs to be done for the function call as well,
> maybe via a return value?
>
I'm afraid I didn't catch your meaning here.
Regards,
Arik
On Sun, 2011-01-16 at 23:53 +0200, Arik Nemtsov wrote:
> > Also, I'm worried about races between this and RX. All of the RX path
> > assumes that it is running at the same time. The ap_sta_ps_{start,end}
> > functions won't be called from the RX path when the flag is set, and
> > this is dependent on setting the flag, but is there really nothing in
> > them that could race?
>
> They might race if the user is not careful. On a SMP machine a user
> can call ieee80211_rx() and ap_sta_ps_start() from two different
> workqueues for example.
> I can add an explicit mutex, but a documentation warning will suffice here no?
Well, a mutex won't be possible, and we don't do "code path locking"
anyway. I'd appreciate if you could actually see which things would
potentially race -- I took a brief look into these functions and it
didn't look like there are actually races except of these two against
each other maybe?
> > Finally, I'm worried about this calling back into the driver's
> > sta_notify callback. If that is to remain that way, it needs to be
> > *very* clearly documented, I'd *much* prefer it not doing that but
> > handing off to a work struct or so internally.
> >
>
> sta_notify is pretty useless when the low level driver calls toggles
> the PS-mode by itself. How about I simply remove the call to
> sta_notify in case IEEE80211_HW_AP_LINK_PS is set?
> If a low level driver needs to do some deferred processing after a
> PS-mode transition, it can queue a work on its own..
Yes, that's probably a good idea -- but definitely needs to be
documented in the sta_notify docs. OTOH, mac80211 de-bounces sta_notify
in a way. Maybe that needs to be done for the function call as well,
maybe via a return value?
johannes