Hi all,
in sta_info.c : ieee80211_sta_ps_deliver_response the
IEEE80211_TX_STATUS_EOSP is set for all to-be-sent frames, not only for
the last. But only the last buffered frame actually gets the EOSP flag.
/* set EOSP for the frame */
if (reason == IEEE80211_FRAME_RELEASE_UAPSD &&
qoshdr && skb_queue_empty(&frames))
*qoshdr |= IEEE80211_QOS_CTL_EOSP;
info->flags |= IEEE80211_TX_STATUS_EOSP |
IEEE80211_TX_CTL_REQ_TX_STATUS;
Consequence is, that the WLAN_STA_SP flag gets cleared (multiple times)
in ieee80211_tx_status before the last frame with EOSP has been sent.
Is this correct?
Regards,
Marco
On Fri, 2012-03-16 at 10:26 +0100, Marco Porsch wrote:
> Hi all,
>
> in sta_info.c : ieee80211_sta_ps_deliver_response the
> IEEE80211_TX_STATUS_EOSP is set for all to-be-sent frames, not only for
> the last. But only the last buffered frame actually gets the EOSP flag.
>
>
> /* set EOSP for the frame */
> if (reason == IEEE80211_FRAME_RELEASE_UAPSD &&
> qoshdr && skb_queue_empty(&frames))
> *qoshdr |= IEEE80211_QOS_CTL_EOSP;
>
> info->flags |= IEEE80211_TX_STATUS_EOSP |
> IEEE80211_TX_CTL_REQ_TX_STATUS;
>
>
> Consequence is, that the WLAN_STA_SP flag gets cleared (multiple times)
> in ieee80211_tx_status before the last frame with EOSP has been sent.
> Is this correct?
Looks like the bug is above, the EOSP/TX_STATUS should only be set for
the last frame?
johannes
On Fri, 2012-03-16 at 14:38 +0100, Marco Porsch wrote:
> On 03/16/12 13:44, Johannes Berg wrote:
> > On Fri, 2012-03-16 at 10:26 +0100, Marco Porsch wrote:
> >> Hi all,
> >>
> >> in sta_info.c : ieee80211_sta_ps_deliver_response the
> >> IEEE80211_TX_STATUS_EOSP is set for all to-be-sent frames, not only for
> >> the last. But only the last buffered frame actually gets the EOSP flag.
> >>
> >>
> >> /* set EOSP for the frame */
> >> if (reason == IEEE80211_FRAME_RELEASE_UAPSD&&
> >> qoshdr&& skb_queue_empty(&frames))
> >> *qoshdr |= IEEE80211_QOS_CTL_EOSP;
> >>
> >> info->flags |= IEEE80211_TX_STATUS_EOSP |
> >> IEEE80211_TX_CTL_REQ_TX_STATUS;
> >>
> >>
> >> Consequence is, that the WLAN_STA_SP flag gets cleared (multiple times)
> >> in ieee80211_tx_status before the last frame with EOSP has been sent.
> >> Is this correct?
> >
> > Looks like the bug is above, the EOSP/TX_STATUS should only be set for
> > the last frame?
>
> I Agree. But what about the case, when the last frame is not a QoS
> frame? Can this even happen - or is U-APSD only for QoS STA?
uAPSD can only be used by a QoS STA.
> Then we would have to manually append a QoS Null with the EOSP flag +
> TX_STATUS?
>
> So like this:
>
> /* set EOSP for the _last_ frame or appended a QoS Null when needed */
> if (reason == IEEE80211_FRAME_RELEASE_UAPSD &&
> skb_queue_empty(&frames)) {
> if (qoshdr) {
> *qoshdr |= IEEE80211_QOS_CTL_EOSP;
>
> info->flags |= IEEE80211_TX_STATUS_EOSP |
> IEEE80211_TX_CTL_REQ_TX_STATUS;
> } else {
> ieee80211_send_null_response(sdata, tid, reason);
> }
> }
No, I think it should be more like this:
/* set EOSP for the frame */
if (skb_queue_empty(&frames)) {
if (reason == IEEE80211_FRAME_RELEASE_UAPSD && qoshdr)
*qoshdr |= IEEE80211_QOS_CTL_EOSP;
info->flags |= IEEE80211_TX_STATUS_EOSP |
IEEE80211_TX_CTL_REQ_TX_STATUS;
}
johannes
Signed-off-by: Marco Porsch <[email protected]>
---
net/mac80211/sta_info.c | 16 +++++++++-------
1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index f468eb1..9b63cd7 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1257,13 +1257,15 @@ ieee80211_sta_ps_deliver_response(struct sta_info *sta,
ieee80211_is_qos_nullfunc(hdr->frame_control))
qoshdr = ieee80211_get_qos_ctl(hdr);
- /* set EOSP for the frame */
- if (reason == IEEE80211_FRAME_RELEASE_UAPSD &&
- qoshdr && skb_queue_empty(&frames))
- *qoshdr |= IEEE80211_QOS_CTL_EOSP;
-
- info->flags |= IEEE80211_TX_STATUS_EOSP |
- IEEE80211_TX_CTL_REQ_TX_STATUS;
+ /* end service period after last frame */
+ if (skb_queue_empty(&frames)) {
+ if (reason == IEEE80211_FRAME_RELEASE_UAPSD &&
+ qoshdr)
+ *qoshdr |= IEEE80211_QOS_CTL_EOSP;
+
+ info->flags |= IEEE80211_TX_STATUS_EOSP |
+ IEEE80211_TX_CTL_REQ_TX_STATUS;
+ }
if (qoshdr)
tids |= BIT(*qoshdr & IEEE80211_QOS_CTL_TID_MASK);
--
1.7.5.4
On 03/16/12 14:40, Johannes Berg wrote:
> On Fri, 2012-03-16 at 14:38 +0100, Marco Porsch wrote:
>> On 03/16/12 13:44, Johannes Berg wrote:
>>> On Fri, 2012-03-16 at 10:26 +0100, Marco Porsch wrote:
>>>> Hi all,
>>>>
>>>> in sta_info.c : ieee80211_sta_ps_deliver_response the
>>>> IEEE80211_TX_STATUS_EOSP is set for all to-be-sent frames, not only for
>>>> the last. But only the last buffered frame actually gets the EOSP flag.
>>>>
>>>>
>>>> /* set EOSP for the frame */
>>>> if (reason == IEEE80211_FRAME_RELEASE_UAPSD&&
>>>> qoshdr&& skb_queue_empty(&frames))
>>>> *qoshdr |= IEEE80211_QOS_CTL_EOSP;
>>>>
>>>> info->flags |= IEEE80211_TX_STATUS_EOSP |
>>>> IEEE80211_TX_CTL_REQ_TX_STATUS;
>>>>
>>>>
>>>> Consequence is, that the WLAN_STA_SP flag gets cleared (multiple times)
>>>> in ieee80211_tx_status before the last frame with EOSP has been sent.
>>>> Is this correct?
>>>
>>> Looks like the bug is above, the EOSP/TX_STATUS should only be set for
>>> the last frame?
>>
>> I Agree. But what about the case, when the last frame is not a QoS
>> frame? Can this even happen - or is U-APSD only for QoS STA?
>
> uAPSD can only be used by a QoS STA.
>
>> Then we would have to manually append a QoS Null with the EOSP flag +
>> TX_STATUS?
>>
>> So like this:
>>
>> /* set EOSP for the _last_ frame or appended a QoS Null when needed */
>> if (reason == IEEE80211_FRAME_RELEASE_UAPSD&&
>> skb_queue_empty(&frames)) {
>> if (qoshdr) {
>> *qoshdr |= IEEE80211_QOS_CTL_EOSP;
>>
>> info->flags |= IEEE80211_TX_STATUS_EOSP |
>> IEEE80211_TX_CTL_REQ_TX_STATUS;
>> } else {
>> ieee80211_send_null_response(sdata, tid, reason);
>> }
>> }
>
> No, I think it should be more like this:
>
>
> /* set EOSP for the frame */
>
> if (skb_queue_empty(&frames)) {
> if (reason == IEEE80211_FRAME_RELEASE_UAPSD&& qoshdr)
> *qoshdr |= IEEE80211_QOS_CTL_EOSP;
>
> info->flags |= IEEE80211_TX_STATUS_EOSP |
> IEEE80211_TX_CTL_REQ_TX_STATUS;
> }
Oh, yes, I see. In case of reason==IEEE80211_FRAME_RELEASE_PSPOLL, it is
intended to not send the EOSP, but still set IEEE80211_TX_STATUS_EOSP.
Who sends the patch?
Regards,
Marco
On 03/16/12 13:44, Johannes Berg wrote:
> On Fri, 2012-03-16 at 10:26 +0100, Marco Porsch wrote:
>> Hi all,
>>
>> in sta_info.c : ieee80211_sta_ps_deliver_response the
>> IEEE80211_TX_STATUS_EOSP is set for all to-be-sent frames, not only for
>> the last. But only the last buffered frame actually gets the EOSP flag.
>>
>>
>> /* set EOSP for the frame */
>> if (reason == IEEE80211_FRAME_RELEASE_UAPSD&&
>> qoshdr&& skb_queue_empty(&frames))
>> *qoshdr |= IEEE80211_QOS_CTL_EOSP;
>>
>> info->flags |= IEEE80211_TX_STATUS_EOSP |
>> IEEE80211_TX_CTL_REQ_TX_STATUS;
>>
>>
>> Consequence is, that the WLAN_STA_SP flag gets cleared (multiple times)
>> in ieee80211_tx_status before the last frame with EOSP has been sent.
>> Is this correct?
>
> Looks like the bug is above, the EOSP/TX_STATUS should only be set for
> the last frame?
I Agree. But what about the case, when the last frame is not a QoS
frame? Can this even happen - or is U-APSD only for QoS STA?
Then we would have to manually append a QoS Null with the EOSP flag +
TX_STATUS?
So like this:
/* set EOSP for the _last_ frame or appended a QoS Null when needed */
if (reason == IEEE80211_FRAME_RELEASE_UAPSD &&
skb_queue_empty(&frames)) {
if (qoshdr) {
*qoshdr |= IEEE80211_QOS_CTL_EOSP;
info->flags |= IEEE80211_TX_STATUS_EOSP |
IEEE80211_TX_CTL_REQ_TX_STATUS;
} else {
ieee80211_send_null_response(sdata, tid, reason);
}
}
Open issues:
Which tid?
Would this Null now be sent after or before the last frame?
What if the EOSP frame is lost?
Regards,
Marco