On Tue, 2012-11-20 at 10:11 -0800, Marco Porsch wrote:

quoted

This is ... strange? Can a single station really own *two* num_psp
refcounts?

Yes it can. A station can be both owner and recipient. And it would just 
be overhead to distinguish between num_psp_owner and num_psp_recipient, 
when in the end we only want to know if there is  any PSP ongoing at all.

I'll change the comment to:
	/* number of active PSPs (owner and recipient counted independently) */
	atomic_t num_psp;

Ok.

quoted

quoted

+	nullfunc = (struct ieee80211_hdr *) skb->data;
+	if (!eosp)
+		nullfunc->frame_control |=
+				cpu_to_le16(IEEE80211_FCTL_MOREDATA);

This seems wrong -- EOSP and moredata are orthogonal (with the
restriction that "!EOSP => moredata") -- but if you just have that in
the code the moredata bit won't always be set correctly.

Imho, in the context of PSP trigger frames it does.
Sending a trigger frame to a mesh PS STA with no EOSP implies the start 
of a PSP with the sender as owner -> following data. The other two 
combinations imply that there is no more data following in that direction.

The EOSP bit in a trigger frame should always be 0 unless the frame is
also a PSP response, no?

What you seem to be missing though is the case when there _is_ more
data, but the service period has to end nonetheless, say because it was
limited to a few packets? Nothing here seems to indicate that an MPSP
ends only after all queued packets are transmitted, which would be a
requirement if this was supposed to be correct.

(Btw, maybe it would be worthwhile to call all of this "MPSP" like the
spec, not just "PSP"?)

But now that you mention it... is there any interest in having that 
function used for uAPSD? Because ieee80211_sta_ps_deliver_response sets 
the EOSP flag during uAPSD, but does not enforce a QoS Data frame to 
carry it. But maybe uAPSD just permits transmitting anything else than 
QoS Data frames...

Well, not really, but non-QoS frames won't happen in that case, because
the peer will have QoS enabled. Similarly here I think, why would there
ever be a non-QoS frame? But maybe this can happen with forwarding,
which can't happen in the non-mesh case.

quoted

quoted

+		ieee80211_sta_ps_deliver_response(sta, 1, 0,
+				IEEE80211_FRAME_RELEASE_UAPSD);

uAPSD?

The standard *explicitly* states that ASPD is *not* supported in mesh.

Absolutely correct. The PSP mechanism is just very similar to uAPSD, 
though. So once the PSP is set up, the mechanisms are the same actually. 
What do you advise? Renaming the release reason? Creating a different 
one that is handled equally?

Well so far the more-data bit seems to be handled different, although I
argue above that you're actually not doing that correctly ;-)

But I think doing different reasons could be helpful, if only to
understand the code better.

johannes