[PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[Mark Asselstine]

'struct ieee80211_hw' contains 'extra_tx_headroom' which it defines as
"headroom to reserve in each transmit skb for use by the driver". This
value is properly setup during rt2x00lib_probe_hw() to account for all
the rt2x00lib's purposes, including DMA alignment and L2 pad if
needed. As such under all circumstances the proper amount of headroom
is allocated to a skb such that under any usage we would not ever use
more headroom then is allotted.

However rt2x00queue_write_tx_frame() uses up the headroom (via calls
to skb_push) allotted for L2 padding (with a potential call to
rt2x00queue_insert_l2pad()) and uses up the headroom allotted to DMA
alignment (with a potential call to rt2x00queue_align_frame()) and
then proceeds to use up 'extra_tx_headroom' (in a call to
rt2x00queue_write_tx_data())

So the driver has requested just 'extra_tx_headroom' worth of headroom
and we have used up 'extra_tx_headroom' + DMA + L2PAD worth. As such
it is possible to hit a 'skb_under_panic', where we have used up all
the available headroom.

Since extra_tx_headroom was calculated as a function of winfo_size,
desc_size, RT2X00_L2PAD_SIZE and RT2X00_ALIGN_SIZE we can simply
remove the part for alignment and padding and we will know how much is
left to use up (for txdesc) and only use that much. Keeping the
driver's use of headroom to what it requested via extra_tx_headroom.

Signed-off-by: Mark Asselstine <asselsm@gmail.com>
---
 drivers/net/wireless/rt2x00/rt2x00queue.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
index 8e68f87..2a48bf5 100644
--- a/drivers/net/wireless/rt2x00/rt2x00queue.c
+++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
@@ -522,6 +522,7 @@ static int rt2x00queue_write_tx_data(struct queue_entry *entry,
 				     struct txentry_desc *txdesc)
 {
 	struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
+	unsigned int avail_extra_tx_headroom = rt2x00dev->extra_tx_headroom;
 
 	/*
 	 * This should not happen, we already checked the entry
@@ -538,10 +539,18 @@ static int rt2x00queue_write_tx_data(struct queue_entry *entry,
 	}
 
 	/*
-	 * Add the requested extra tx headroom in front of the skb.
+	 * Add room for data at the front of the buffer for txdesc. The space
+	 * needed for this was accounted for in extra_tx_headroom, we just
+	 * need to remove the amount allocated for padding and alignment
+	 * to get what is left for txdesc.
 	 */
-	skb_push(entry->skb, rt2x00dev->extra_tx_headroom);
-	memset(entry->skb->data, 0, rt2x00dev->extra_tx_headroom);
+	if (test_bit(REQUIRE_L2PAD, &rt2x00dev->cap_flags))
+		avail_extra_tx_headroom -= RT2X00_L2PAD_SIZE;
+	else if (test_bit(REQUIRE_DMA, &rt2x00dev->cap_flags))
+		avail_extra_tx_headroom -= RT2X00_ALIGN_SIZE;
+
+	skb_push(entry->skb, avail_extra_tx_headroom);
+	memset(entry->skb->data, 0, avail_extra_tx_headroom);
 
 	/*
 	 * Call the driver's write_tx_data function, if it exists.
-- 
1.9.1

Re: [PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[Stanislaw Gruszka]

On Tue, Sep 30, 2014 at 11:45:57PM -0400, Mark Asselstine wrote:
> 'struct ieee80211_hw' contains 'extra_tx_headroom' which it defines as
> "headroom to reserve in each transmit skb for use by the driver". This
> value is properly setup during rt2x00lib_probe_hw() to account for all
> the rt2x00lib's purposes, including DMA alignment and L2 pad if
> needed. As such under all circumstances the proper amount of headroom
> is allocated to a skb such that under any usage we would not ever use
> more headroom then is allotted.
> 
> However rt2x00queue_write_tx_frame() uses up the headroom (via calls
> to skb_push) allotted for L2 padding (with a potential call to
> rt2x00queue_insert_l2pad()) and uses up the headroom allotted to DMA
> alignment (with a potential call to rt2x00queue_align_frame()) and
> then proceeds to use up 'extra_tx_headroom' (in a call to
> rt2x00queue_write_tx_data())
> 
> So the driver has requested just 'extra_tx_headroom' worth of headroom
> and we have used up 'extra_tx_headroom' + DMA + L2PAD worth. As such
> it is possible to hit a 'skb_under_panic', where we have used up all
> the available headroom.
> 
> Since extra_tx_headroom was calculated as a function of winfo_size,
> desc_size, RT2X00_L2PAD_SIZE and RT2X00_ALIGN_SIZE we can simply
> remove the part for alignment and padding and we will know how much is
> left to use up (for txdesc) and only use that much. Keeping the
> driver's use of headroom to what it requested via extra_tx_headroom.
> 
> Signed-off-by: Mark Asselstine <asselsm@gmail.com>
> ---
>  drivers/net/wireless/rt2x00/rt2x00queue.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
> index 8e68f87..2a48bf5 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
> @@ -522,6 +522,7 @@ static int rt2x00queue_write_tx_data(struct queue_entry *entry,
>  				     struct txentry_desc *txdesc)
>  {
>  	struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
> +	unsigned int avail_extra_tx_headroom = rt2x00dev->extra_tx_headroom;
>  
>  	/*
>  	 * This should not happen, we already checked the entry
> @@ -538,10 +539,18 @@ static int rt2x00queue_write_tx_data(struct queue_entry *entry,
>  	}
>  
>  	/*
> -	 * Add the requested extra tx headroom in front of the skb.
> +	 * Add room for data at the front of the buffer for txdesc. The space
> +	 * needed for this was accounted for in extra_tx_headroom, we just
> +	 * need to remove the amount allocated for padding and alignment
> +	 * to get what is left for txdesc.
>  	 */
> -	skb_push(entry->skb, rt2x00dev->extra_tx_headroom);
> -	memset(entry->skb->data, 0, rt2x00dev->extra_tx_headroom);
> +	if (test_bit(REQUIRE_L2PAD, &rt2x00dev->cap_flags))
> +		avail_extra_tx_headroom -= RT2X00_L2PAD_SIZE;
> +	else if (test_bit(REQUIRE_DMA, &rt2x00dev->cap_flags))
> +		avail_extra_tx_headroom -= RT2X00_ALIGN_SIZE;
> +
> +	skb_push(entry->skb, avail_extra_tx_headroom);
> +	memset(entry->skb->data, 0, avail_extra_tx_headroom);

I don't think patch is correct. 

We have rt2x00->extra_tx_headroom and rt2x00->hw->extra_tx_headroom.
Second value, which we provide as maximum needed headroom to mac80211
is rt2x00->extra_tx_headrom + RT2X00_L2PAD_SIZE + RT2X00_ALIGN_SIZE.

We really need to reserve rt2x00dev->extra_tx_headroom on TX skb, as
this is room for metadata needed by H/W and if needed we should reserve
RT2x00_L2PAD_SIZE and RTX00_ALIGN_SIZE. There should be room for that,
since we provide bigger rt2x00->hw->extra_tx_headroom to mac80211.

The only possibility to skb_under_panic I can see, is that we retransmit
frame and try to align it many times, but alignment should not be needed
once we aligned frame. Hence I'm not sure how those skb_under_panics can
happen.

Thanks
Stanislaw

Re: [PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[Mark Asselstine]

On Wed, Oct 1, 2014 at 9:42 PM, Mark Asselstine <asselsm@gmail.com> wrote:
>
> Damn, you are right. I thought I had it licked.
>
> Unfortunately with the overloaded variable name it was easy to get turned
> around. The comments in the code didn't prevent me knotting myself up
> either. If you look in in rt2x00.h, struct rt2x00_dev, the comment above the
> extra_tx_headroom member says "Extra TX headroom required for alignment
> purposes." I would say this is incorrect. This variable is initialized
> via rt2x00dev_extra_tx_headroom() with a combination of winfo_size and
> desc_size and has nothing to do with alignment.
>
> At any rate, keeping in mind that rt2x00_dev.hw.extra_tx_headroom
> which is used to set the amount of available headroom includes room
> for alignment and padding as well as winfo and desc space, and that
> rt2x00_dev.extra_tx_headroom doesn't include padding or alignment, you
> are correct in that we aren't over spending our headroom. Back to the
> drawing board.
>
>
> Mark
>
> On Wed, Oct 1, 2014 at 5:12 AM, Stanislaw Gruszka <sgruszka@redhat.com>
> wrote:
>>
>> On Tue, Sep 30, 2014 at 11:45:57PM -0400, Mark Asselstine wrote:
>> > 'struct ieee80211_hw' contains 'extra_tx_headroom' which it defines as
>> > "headroom to reserve in each transmit skb for use by the driver". This
>> > value is properly setup during rt2x00lib_probe_hw() to account for all
>> > the rt2x00lib's purposes, including DMA alignment and L2 pad if
>> > needed. As such under all circumstances the proper amount of headroom
>> > is allocated to a skb such that under any usage we would not ever use
>> > more headroom then is allotted.
>> >
>> > However rt2x00queue_write_tx_frame() uses up the headroom (via calls
>> > to skb_push) allotted for L2 padding (with a potential call to
>> > rt2x00queue_insert_l2pad()) and uses up the headroom allotted to DMA
>> > alignment (with a potential call to rt2x00queue_align_frame()) and
>> > then proceeds to use up 'extra_tx_headroom' (in a call to
>> > rt2x00queue_write_tx_data())
>> >
>> > So the driver has requested just 'extra_tx_headroom' worth of headroom
>> > and we have used up 'extra_tx_headroom' + DMA + L2PAD worth. As such
>> > it is possible to hit a 'skb_under_panic', where we have used up all
>> > the available headroom.
>> >
>> > Since extra_tx_headroom was calculated as a function of winfo_size,
>> > desc_size, RT2X00_L2PAD_SIZE and RT2X00_ALIGN_SIZE we can simply
>> > remove the part for alignment and padding and we will know how much is
>> > left to use up (for txdesc) and only use that much. Keeping the
>> > driver's use of headroom to what it requested via extra_tx_headroom.
>> >
>> > Signed-off-by: Mark Asselstine <asselsm@gmail.com>
>> > ---
>> >  drivers/net/wireless/rt2x00/rt2x00queue.c | 15 ++++++++++++---
>> >  1 file changed, 12 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c
>> > b/drivers/net/wireless/rt2x00/rt2x00queue.c
>> > index 8e68f87..2a48bf5 100644
>> > --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
>> > +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
>> > @@ -522,6 +522,7 @@ static int rt2x00queue_write_tx_data(struct
>> > queue_entry *entry,
>> >                                    struct txentry_desc *txdesc)
>> >  {
>> >       struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
>> > +     unsigned int avail_extra_tx_headroom =
>> > rt2x00dev->extra_tx_headroom;
>> >
>> >       /*
>> >        * This should not happen, we already checked the entry
>> > @@ -538,10 +539,18 @@ static int rt2x00queue_write_tx_data(struct
>> > queue_entry *entry,
>> >       }
>> >
>> >       /*
>> > -      * Add the requested extra tx headroom in front of the skb.
>> > +      * Add room for data at the front of the buffer for txdesc. The
>> > space
>> > +      * needed for this was accounted for in extra_tx_headroom, we just
>> > +      * need to remove the amount allocated for padding and alignment
>> > +      * to get what is left for txdesc.
>> >        */
>> > -     skb_push(entry->skb, rt2x00dev->extra_tx_headroom);
>> > -     memset(entry->skb->data, 0, rt2x00dev->extra_tx_headroom);
>> > +     if (test_bit(REQUIRE_L2PAD, &rt2x00dev->cap_flags))
>> > +             avail_extra_tx_headroom -= RT2X00_L2PAD_SIZE;
>> > +     else if (test_bit(REQUIRE_DMA, &rt2x00dev->cap_flags))
>> > +             avail_extra_tx_headroom -= RT2X00_ALIGN_SIZE;
>> > +
>> > +     skb_push(entry->skb, avail_extra_tx_headroom);
>> > +     memset(entry->skb->data, 0, avail_extra_tx_headroom);
>>
>> I don't think patch is correct.
>>
>> We have rt2x00->extra_tx_headroom and rt2x00->hw->extra_tx_headroom.
>> Second value, which we provide as maximum needed headroom to mac80211
>> is rt2x00->extra_tx_headrom + RT2X00_L2PAD_SIZE + RT2X00_ALIGN_SIZE.
>>
>> We really need to reserve rt2x00dev->extra_tx_headroom on TX skb, as
>> this is room for metadata needed by H/W and if needed we should reserve
>> RT2x00_L2PAD_SIZE and RTX00_ALIGN_SIZE. There should be room for that,
>> since we provide bigger rt2x00->hw->extra_tx_headroom to mac80211.
>>
>> The only possibility to skb_under_panic I can see, is that we retransmit
>> frame and try to align it many times, but alignment should not be needed
>> once we aligned frame. Hence I'm not sure how those skb_under_panics can
>> happen.

I am still digging through trying to find a cause for this, without a
reproducer I am starting to lose hope on finding the cause.

I dug up this old thread
http://rt2x00.serialmonkey.com/pipermail/users_rt2x00.serialmonkey.com/2010-November/002457.html

I am thinking that the cause is an interaction with 80211mac and the
rt2x00queue not doing enough to ensure we are sending the skb back in
the same state as we get it. Looking at rt2x00lib_txdone() we don't
return headroom that may have been taken for frame alignment and we
don't account for the extra 4-bytes taken for header_align when the
payload_align is larger then the header_align while setting up the
l2pad. Do you know why rt2x00 doesn't return frame align space back to
the headroom?

Mark



>>
>> Thanks
>> Stanislaw
>
>

Re: [PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[Helmut Schaa]

Mark Asselstine <asselsm@gmail.com> schrieb:
>On Wed, Oct 1, 2014 at 9:42 PM, Mark Asselstine <asselsm@gmail.com>
>wrote:
>>
>> Damn, you are right. I thought I had it licked.
>>
>> Unfortunately with the overloaded variable name it was easy to get
>turned
>> around. The comments in the code didn't prevent me knotting myself up
>> either. If you look in in rt2x00.h, struct rt2x00_dev, the comment
>above the
>> extra_tx_headroom member says "Extra TX headroom required for
>alignment
>> purposes." I would say this is incorrect. This variable is
>initialized
>> via rt2x00dev_extra_tx_headroom() with a combination of winfo_size
>and
>> desc_size and has nothing to do with alignment.
>>
>> At any rate, keeping in mind that rt2x00_dev.hw.extra_tx_headroom
>> which is used to set the amount of available headroom includes room
>> for alignment and padding as well as winfo and desc space, and that
>> rt2x00_dev.extra_tx_headroom doesn't include padding or alignment,
>you
>> are correct in that we aren't over spending our headroom. Back to the
>> drawing board.
>>
>>
>> Mark
>>
>> On Wed, Oct 1, 2014 at 5:12 AM, Stanislaw Gruszka
><sgruszka@redhat.com>
>> wrote:
>>>
>>> On Tue, Sep 30, 2014 at 11:45:57PM -0400, Mark Asselstine wrote:
>>> > 'struct ieee80211_hw' contains 'extra_tx_headroom' which it
>defines as
>>> > "headroom to reserve in each transmit skb for use by the driver".
>This
>>> > value is properly setup during rt2x00lib_probe_hw() to account for
>all
>>> > the rt2x00lib's purposes, including DMA alignment and L2 pad if
>>> > needed. As such under all circumstances the proper amount of
>headroom
>>> > is allocated to a skb such that under any usage we would not ever
>use
>>> > more headroom then is allotted.
>>> >
>>> > However rt2x00queue_write_tx_frame() uses up the headroom (via
>calls
>>> > to skb_push) allotted for L2 padding (with a potential call to
>>> > rt2x00queue_insert_l2pad()) and uses up the headroom allotted to
>DMA
>>> > alignment (with a potential call to rt2x00queue_align_frame()) and
>>> > then proceeds to use up 'extra_tx_headroom' (in a call to
>>> > rt2x00queue_write_tx_data())
>>> >
>>> > So the driver has requested just 'extra_tx_headroom' worth of
>headroom
>>> > and we have used up 'extra_tx_headroom' + DMA + L2PAD worth. As
>such
>>> > it is possible to hit a 'skb_under_panic', where we have used up
>all
>>> > the available headroom.
>>> >
>>> > Since extra_tx_headroom was calculated as a function of
>winfo_size,
>>> > desc_size, RT2X00_L2PAD_SIZE and RT2X00_ALIGN_SIZE we can simply
>>> > remove the part for alignment and padding and we will know how
>much is
>>> > left to use up (for txdesc) and only use that much. Keeping the
>>> > driver's use of headroom to what it requested via
>extra_tx_headroom.
>>> >
>>> > Signed-off-by: Mark Asselstine <asselsm@gmail.com>
>>> > ---
>>> >  drivers/net/wireless/rt2x00/rt2x00queue.c | 15 ++++++++++++---
>>> >  1 file changed, 12 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c
>>> > b/drivers/net/wireless/rt2x00/rt2x00queue.c
>>> > index 8e68f87..2a48bf5 100644
>>> > --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
>>> > +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
>>> > @@ -522,6 +522,7 @@ static int rt2x00queue_write_tx_data(struct
>>> > queue_entry *entry,
>>> >                                    struct txentry_desc *txdesc)
>>> >  {
>>> >       struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
>>> > +     unsigned int avail_extra_tx_headroom =
>>> > rt2x00dev->extra_tx_headroom;
>>> >
>>> >       /*
>>> >        * This should not happen, we already checked the entry
>>> > @@ -538,10 +539,18 @@ static int rt2x00queue_write_tx_data(struct
>>> > queue_entry *entry,
>>> >       }
>>> >
>>> >       /*
>>> > -      * Add the requested extra tx headroom in front of the skb.
>>> > +      * Add room for data at the front of the buffer for txdesc.
>The
>>> > space
>>> > +      * needed for this was accounted for in extra_tx_headroom,
>we just
>>> > +      * need to remove the amount allocated for padding and
>alignment
>>> > +      * to get what is left for txdesc.
>>> >        */
>>> > -     skb_push(entry->skb, rt2x00dev->extra_tx_headroom);
>>> > -     memset(entry->skb->data, 0, rt2x00dev->extra_tx_headroom);
>>> > +     if (test_bit(REQUIRE_L2PAD, &rt2x00dev->cap_flags))
>>> > +             avail_extra_tx_headroom -= RT2X00_L2PAD_SIZE;
>>> > +     else if (test_bit(REQUIRE_DMA, &rt2x00dev->cap_flags))
>>> > +             avail_extra_tx_headroom -= RT2X00_ALIGN_SIZE;
>>> > +
>>> > +     skb_push(entry->skb, avail_extra_tx_headroom);
>>> > +     memset(entry->skb->data, 0, avail_extra_tx_headroom);
>>>
>>> I don't think patch is correct.
>>>
>>> We have rt2x00->extra_tx_headroom and rt2x00->hw->extra_tx_headroom.
>>> Second value, which we provide as maximum needed headroom to
>mac80211
>>> is rt2x00->extra_tx_headrom + RT2X00_L2PAD_SIZE + RT2X00_ALIGN_SIZE.
>>>
>>> We really need to reserve rt2x00dev->extra_tx_headroom on TX skb, as
>>> this is room for metadata needed by H/W and if needed we should
>reserve
>>> RT2x00_L2PAD_SIZE and RTX00_ALIGN_SIZE. There should be room for
>that,
>>> since we provide bigger rt2x00->hw->extra_tx_headroom to mac80211.
>>>
>>> The only possibility to skb_under_panic I can see, is that we
>retransmit
>>> frame and try to align it many times, but alignment should not be
>needed
>>> once we aligned frame. Hence I'm not sure how those skb_under_panics
>can
>>> happen.
>
>I am still digging through trying to find a cause for this, without a
>reproducer I am starting to lose hope on finding the cause.
>
>I dug up this old thread
>http://rt2x00.serialmonkey.com/pipermail/users_rt2x00.serialmonkey.com/2010-November/002457.html
>
>I am thinking that the cause is an interaction with 80211mac and the
>rt2x00queue not doing enough to ensure we are sending the skb back in
>the same state as we get it. Looking at rt2x00lib_txdone() we don't
>return headroom that may have been taken for frame alignment and we
>don't account for the extra 4-bytes taken for header_align when the
>payload_align is larger then the header_align while setting up the
>l2pad. Do you know why rt2x00 doesn't return frame align space back to
>the headroom?

If rt2x00 does not remove the alignment from the frame before giving it back to mac80211 and the same frame comes into rt2x00 again it should be correctly aligned and no additional header space is required. So this should be fine.

Helmut

Re: [PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[Mark Asselstine]

On Sun, Oct 5, 2014 at 4:39 AM, Helmut Schaa
<helmut.schaa@googlemail.com> wrote:
>
>
>
>
> Mark Asselstine <asselsm@gmail.com> schrieb:
>>On Wed, Oct 1, 2014 at 9:42 PM, Mark Asselstine <asselsm@gmail.com>
>>wrote:
>>>
>>> Damn, you are right. I thought I had it licked.
>>>
>>> Unfortunately with the overloaded variable name it was easy to get
>>turned
>>> around. The comments in the code didn't prevent me knotting myself up
>>> either. If you look in in rt2x00.h, struct rt2x00_dev, the comment
>>above the
>>> extra_tx_headroom member says "Extra TX headroom required for
>>alignment
>>> purposes." I would say this is incorrect. This variable is
>>initialized
>>> via rt2x00dev_extra_tx_headroom() with a combination of winfo_size
>>and
>>> desc_size and has nothing to do with alignment.
>>>
>>> At any rate, keeping in mind that rt2x00_dev.hw.extra_tx_headroom
>>> which is used to set the amount of available headroom includes room
>>> for alignment and padding as well as winfo and desc space, and that
>>> rt2x00_dev.extra_tx_headroom doesn't include padding or alignment,
>>you
>>> are correct in that we aren't over spending our headroom. Back to the
>>> drawing board.
>>>
>>>
>>> Mark
>>>
>>> On Wed, Oct 1, 2014 at 5:12 AM, Stanislaw Gruszka
>><sgruszka@redhat.com>
>>> wrote:
>>>>
>>>> On Tue, Sep 30, 2014 at 11:45:57PM -0400, Mark Asselstine wrote:
>>>> > 'struct ieee80211_hw' contains 'extra_tx_headroom' which it
>>defines as
>>>> > "headroom to reserve in each transmit skb for use by the driver".
>>This
>>>> > value is properly setup during rt2x00lib_probe_hw() to account for
>>all
>>>> > the rt2x00lib's purposes, including DMA alignment and L2 pad if
>>>> > needed. As such under all circumstances the proper amount of
>>headroom
>>>> > is allocated to a skb such that under any usage we would not ever
>>use
>>>> > more headroom then is allotted.
>>>> >
>>>> > However rt2x00queue_write_tx_frame() uses up the headroom (via
>>calls
>>>> > to skb_push) allotted for L2 padding (with a potential call to
>>>> > rt2x00queue_insert_l2pad()) and uses up the headroom allotted to
>>DMA
>>>> > alignment (with a potential call to rt2x00queue_align_frame()) and
>>>> > then proceeds to use up 'extra_tx_headroom' (in a call to
>>>> > rt2x00queue_write_tx_data())
>>>> >
>>>> > So the driver has requested just 'extra_tx_headroom' worth of
>>headroom
>>>> > and we have used up 'extra_tx_headroom' + DMA + L2PAD worth. As
>>such
>>>> > it is possible to hit a 'skb_under_panic', where we have used up
>>all
>>>> > the available headroom.
>>>> >
>>>> > Since extra_tx_headroom was calculated as a function of
>>winfo_size,
>>>> > desc_size, RT2X00_L2PAD_SIZE and RT2X00_ALIGN_SIZE we can simply
>>>> > remove the part for alignment and padding and we will know how
>>much is
>>>> > left to use up (for txdesc) and only use that much. Keeping the
>>>> > driver's use of headroom to what it requested via
>>extra_tx_headroom.
>>>> >
>>>> > Signed-off-by: Mark Asselstine <asselsm@gmail.com>
>>>> > ---
>>>> >  drivers/net/wireless/rt2x00/rt2x00queue.c | 15 ++++++++++++---
>>>> >  1 file changed, 12 insertions(+), 3 deletions(-)
>>>> >
>>>> > diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c
>>>> > b/drivers/net/wireless/rt2x00/rt2x00queue.c
>>>> > index 8e68f87..2a48bf5 100644
>>>> > --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
>>>> > +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
>>>> > @@ -522,6 +522,7 @@ static int rt2x00queue_write_tx_data(struct
>>>> > queue_entry *entry,
>>>> >                                    struct txentry_desc *txdesc)
>>>> >  {
>>>> >       struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
>>>> > +     unsigned int avail_extra_tx_headroom =
>>>> > rt2x00dev->extra_tx_headroom;
>>>> >
>>>> >       /*
>>>> >        * This should not happen, we already checked the entry
>>>> > @@ -538,10 +539,18 @@ static int rt2x00queue_write_tx_data(struct
>>>> > queue_entry *entry,
>>>> >       }
>>>> >
>>>> >       /*
>>>> > -      * Add the requested extra tx headroom in front of the skb.
>>>> > +      * Add room for data at the front of the buffer for txdesc.
>>The
>>>> > space
>>>> > +      * needed for this was accounted for in extra_tx_headroom,
>>we just
>>>> > +      * need to remove the amount allocated for padding and
>>alignment
>>>> > +      * to get what is left for txdesc.
>>>> >        */
>>>> > -     skb_push(entry->skb, rt2x00dev->extra_tx_headroom);
>>>> > -     memset(entry->skb->data, 0, rt2x00dev->extra_tx_headroom);
>>>> > +     if (test_bit(REQUIRE_L2PAD, &rt2x00dev->cap_flags))
>>>> > +             avail_extra_tx_headroom -= RT2X00_L2PAD_SIZE;
>>>> > +     else if (test_bit(REQUIRE_DMA, &rt2x00dev->cap_flags))
>>>> > +             avail_extra_tx_headroom -= RT2X00_ALIGN_SIZE;
>>>> > +
>>>> > +     skb_push(entry->skb, avail_extra_tx_headroom);
>>>> > +     memset(entry->skb->data, 0, avail_extra_tx_headroom);
>>>>
>>>> I don't think patch is correct.
>>>>
>>>> We have rt2x00->extra_tx_headroom and rt2x00->hw->extra_tx_headroom.
>>>> Second value, which we provide as maximum needed headroom to
>>mac80211
>>>> is rt2x00->extra_tx_headrom + RT2X00_L2PAD_SIZE + RT2X00_ALIGN_SIZE.
>>>>
>>>> We really need to reserve rt2x00dev->extra_tx_headroom on TX skb, as
>>>> this is room for metadata needed by H/W and if needed we should
>>reserve
>>>> RT2x00_L2PAD_SIZE and RTX00_ALIGN_SIZE. There should be room for
>>that,
>>>> since we provide bigger rt2x00->hw->extra_tx_headroom to mac80211.
>>>>
>>>> The only possibility to skb_under_panic I can see, is that we
>>retransmit
>>>> frame and try to align it many times, but alignment should not be
>>needed
>>>> once we aligned frame. Hence I'm not sure how those skb_under_panics
>>can
>>>> happen.
>>
>>I am still digging through trying to find a cause for this, without a
>>reproducer I am starting to lose hope on finding the cause.
>>
>>I dug up this old thread
>>http://rt2x00.serialmonkey.com/pipermail/users_rt2x00.serialmonkey.com/2010-November/002457.html
>>
>>I am thinking that the cause is an interaction with 80211mac and the
>>rt2x00queue not doing enough to ensure we are sending the skb back in
>>the same state as we get it. Looking at rt2x00lib_txdone() we don't
>>return headroom that may have been taken for frame alignment and we
>>don't account for the extra 4-bytes taken for header_align when the
>>payload_align is larger then the header_align while setting up the
>>l2pad. Do you know why rt2x00 doesn't return frame align space back to
>>the headroom?
>
> If rt2x00 does not remove the alignment from the frame before giving it back
> to mac80211 and the same frame comes into rt2x00 again it should be correctly
> aligned and no additional header space is required. So this should be fine.

Then I would say this definitely hints at a design flaw in
rt2x00queue_insert_l2pad(). Take the following scenario.

* skb's first arrival in rt2x00queue_insert_l2pad(), 3 bytes needed
for frame alignment, 2 bytes for l2pad results in 3 bytes of headroom
taken.
* rt2x00lib_txdone() returns 2 bytes of headroom
* skb's second arrival in rt2x00queue_insert_l2pad(), 0 bytes needed
for frame alignment, 2 bytes for l2pad results in 4 bytes of headroom
taken.
* rt2x00lib_txdone() returns 2 bytes of headroom

Basically as long as any bytes are required for l2pad the headroom
will lose 4 bytes again and again, never being returned by
rt2x00lib_txdone().

Not having spent enough time with the 80211 code I am still figuring
out when the skb is reused but I will get there.

Mark

>
> Helmut

Re: [PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[Stanislaw Gruszka]

On Wed, Oct 08, 2014 at 07:46:33AM -0400, Mark Asselstine wrote:
> > If rt2x00 does not remove the alignment from the frame before giving it back
> > to mac80211 and the same frame comes into rt2x00 again it should be correctly
> > aligned and no additional header space is required. So this should be fine.
> 
> Then I would say this definitely hints at a design flaw in
> rt2x00queue_insert_l2pad(). Take the following scenario.
> 
> * skb's first arrival in rt2x00queue_insert_l2pad(), 3 bytes needed
> for frame alignment, 2 bytes for l2pad results in 3 bytes of headroom
> taken.
Not quite realistic assumption - header length will have to be odd then.

But if such situation would happen we will have:

header_align=2, payload_align=3, l2pad=3

Since payload_align will be bigger than header_align, header_align will
be increased to 6.

Header will be moved by 6 bytes, frame will be moved by 3 bytes,
between header and frame there will be l2pad equal to 3.

> * rt2x00lib_txdone() returns 2 bytes of headroom
Return 3 bytes.

> * skb's second arrival in rt2x00queue_insert_l2pad(), 0 bytes needed
> for frame alignment, 2 bytes for l2pad results in 4 bytes of headroom
> taken.
Header will be moved by 3 bytes.

> * rt2x00lib_txdone() returns 2 bytes of headroom
Return 3 bytes.

> Basically as long as any bytes are required for l2pad the headroom
> will lose 4 bytes again and again, never being returned by
> rt2x00lib_txdone().

I think that's not true - you made a few mistakes in your scenario,
but perhaps I'm wrong :-)

Stanislaw

Re: [PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[Mark Asselstine]

On Wed, Oct 8, 2014 at 9:00 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
>
> On Wed, Oct 08, 2014 at 07:46:33AM -0400, Mark Asselstine wrote:
> > > If rt2x00 does not remove the alignment from the frame before giving it back
> > > to mac80211 and the same frame comes into rt2x00 again it should be correctly
> > > aligned and no additional header space is required. So this should be fine.
> >
> > Then I would say this definitely hints at a design flaw in
> > rt2x00queue_insert_l2pad(). Take the following scenario.
> >
> > * skb's first arrival in rt2x00queue_insert_l2pad(), 3 bytes needed
> > for frame alignment, 2 bytes for l2pad results in 3 bytes of headroom
> > taken.
> Not quite realistic assumption - header length will have to be odd then.
>
> But if such situation would happen we will have:
>
> header_align=2, payload_align=3, l2pad=3
>
> Since payload_align will be bigger than header_align, header_align will
> be increased to 6.
>
> Header will be moved by 6 bytes, frame will be moved by 3 bytes,
> between header and frame there will be l2pad equal to 3.
>
> > * rt2x00lib_txdone() returns 2 bytes of headroom
> Return 3 bytes.
>
> > * skb's second arrival in rt2x00queue_insert_l2pad(), 0 bytes needed
> > for frame alignment, 2 bytes for l2pad results in 4 bytes of headroom
> > taken.
> Header will be moved by 3 bytes.
>
> > * rt2x00lib_txdone() returns 2 bytes of headroom
> Return 3 bytes.
>
> > Basically as long as any bytes are required for l2pad the headroom
> > will lose 4 bytes again and again, never being returned by
> > rt2x00lib_txdone().
>
> I think that's not true - you made a few mistakes in your scenario,
> but perhaps I'm wrong :-)

No just me being an idiot. I had thought
 frame == header + l2pad + payload
not
 frame == payload

With this straight your numbers makes sense and my scenario is
incorrect. We don't continue to eat in to the headroom but rather take
and return a small bit.

Thanks for your patience, it has been a while since I have been
working on this stuff. I am still motivated to hunt this issue down,
you might just need to correct me along the way.

Mark

>
> Stanislaw

Re: [PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[Mark Asselstine]

On Wed, Oct 8, 2014 at 3:52 PM, Mark Asselstine <asselsm@gmail.com> wrote:
> On Wed, Oct 8, 2014 at 9:00 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
>>
>> On Wed, Oct 08, 2014 at 07:46:33AM -0400, Mark Asselstine wrote:
>> > > If rt2x00 does not remove the alignment from the frame before giving it back
>> > > to mac80211 and the same frame comes into rt2x00 again it should be correctly
>> > > aligned and no additional header space is required. So this should be fine.
>> >
>> > Then I would say this definitely hints at a design flaw in
>> > rt2x00queue_insert_l2pad(). Take the following scenario.
>> >
>> > * skb's first arrival in rt2x00queue_insert_l2pad(), 3 bytes needed
>> > for frame alignment, 2 bytes for l2pad results in 3 bytes of headroom
>> > taken.
>> Not quite realistic assumption - header length will have to be odd then.
>>
>> But if such situation would happen we will have:
>>
>> header_align=2, payload_align=3, l2pad=3
>>
>> Since payload_align will be bigger than header_align, header_align will
>> be increased to 6.
>>
>> Header will be moved by 6 bytes, frame will be moved by 3 bytes,
>> between header and frame there will be l2pad equal to 3.
>>
>> > * rt2x00lib_txdone() returns 2 bytes of headroom
>> Return 3 bytes.
>>
>> > * skb's second arrival in rt2x00queue_insert_l2pad(), 0 bytes needed
>> > for frame alignment, 2 bytes for l2pad results in 4 bytes of headroom
>> > taken.
>> Header will be moved by 3 bytes.
>>
>> > * rt2x00lib_txdone() returns 2 bytes of headroom
>> Return 3 bytes.
>>
>> > Basically as long as any bytes are required for l2pad the headroom
>> > will lose 4 bytes again and again, never being returned by
>> > rt2x00lib_txdone().
>>
>> I think that's not true - you made a few mistakes in your scenario,
>> but perhaps I'm wrong :-)
>
> No just me being an idiot. I had thought
>  frame == header + l2pad + payload
> not
>  frame == payload
>

By the way, I assumed that this due to the name and contents of
rt2x00queue_align_frame(). Where all the data (header and payload) are
aligned to a 4-byte boundary and the function is name 'align_frame'. I
assume your interpretation is the correct one, can you confirm?

Mark

> With this straight your numbers makes sense and my scenario is
> incorrect. We don't continue to eat in to the headroom but rather take
> and return a small bit.
>
> Thanks for your patience, it has been a while since I have been
> working on this stuff. I am still motivated to hunt this issue down,
> you might just need to correct me along the way.
>
> Mark
>
>>
>> Stanislaw

Re: [PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[Stanislaw Gruszka]

On Wed, Oct 08, 2014 at 11:34:22PM -0400, Mark Asselstine wrote:
> On Wed, Oct 8, 2014 at 3:52 PM, Mark Asselstine <asselsm@gmail.com> wrote:
> > On Wed, Oct 8, 2014 at 9:00 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> >>
> >> On Wed, Oct 08, 2014 at 07:46:33AM -0400, Mark Asselstine wrote:
> >> > > If rt2x00 does not remove the alignment from the frame before giving it back
> >> > > to mac80211 and the same frame comes into rt2x00 again it should be correctly
> >> > > aligned and no additional header space is required. So this should be fine.
> >> >
> >> > Then I would say this definitely hints at a design flaw in
> >> > rt2x00queue_insert_l2pad(). Take the following scenario.
> >> >
> >> > * skb's first arrival in rt2x00queue_insert_l2pad(), 3 bytes needed
> >> > for frame alignment, 2 bytes for l2pad results in 3 bytes of headroom
> >> > taken.
> >> Not quite realistic assumption - header length will have to be odd then.
> >>
> >> But if such situation would happen we will have:
> >>
> >> header_align=2, payload_align=3, l2pad=3
> >>
> >> Since payload_align will be bigger than header_align, header_align will
> >> be increased to 6.
> >>
> >> Header will be moved by 6 bytes, frame will be moved by 3 bytes,
> >> between header and frame there will be l2pad equal to 3.
> >>
> >> > * rt2x00lib_txdone() returns 2 bytes of headroom
> >> Return 3 bytes.
> >>
> >> > * skb's second arrival in rt2x00queue_insert_l2pad(), 0 bytes needed
> >> > for frame alignment, 2 bytes for l2pad results in 4 bytes of headroom
> >> > taken.
> >> Header will be moved by 3 bytes.
> >>
> >> > * rt2x00lib_txdone() returns 2 bytes of headroom
> >> Return 3 bytes.
> >>
> >> > Basically as long as any bytes are required for l2pad the headroom
> >> > will lose 4 bytes again and again, never being returned by
> >> > rt2x00lib_txdone().
> >>
> >> I think that's not true - you made a few mistakes in your scenario,
> >> but perhaps I'm wrong :-)
> >
> > No just me being an idiot. I had thought
> >  frame == header + l2pad + payload
> > not
> >  frame == payload
> >
> 
> By the way, I assumed that this due to the name and contents of
> rt2x00queue_align_frame(). Where all the data (header and payload) are
> aligned to a 4-byte boundary and the function is name 'align_frame'. I
> assume your interpretation is the correct one, can you confirm?

Frame term is used mostly to describe all data including header and
payload.

Stanislaw

Re: [PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[Stanislaw Gruszka]

On Sat, Oct 04, 2014 at 07:38:25PM -0400, Mark Asselstine wrote:
> >> The only possibility to skb_under_panic I can see, is that we retransmit
> >> frame and try to align it many times, but alignment should not be needed
> >> once we aligned frame. Hence I'm not sure how those skb_under_panics can
> >> happen.
> 
> I am still digging through trying to find a cause for this, without a
> reproducer I am starting to lose hope on finding the cause.
> 
> I dug up this old thread
> http://rt2x00.serialmonkey.com/pipermail/users_rt2x00.serialmonkey.com/2010-November/002457.html
>
> I am thinking that the cause is an interaction with 80211mac and the
> rt2x00queue not doing enough to ensure we are sending the skb back in
> the same state as we get it. Looking at rt2x00lib_txdone() we don't
> return headroom that may have been taken for frame alignment and we
> don't account for the extra 4-bytes taken for header_align when the
> payload_align is larger then the header_align while setting up the
> l2pad.

I think we reserve sufficient space, 4 bytes for payload align plus
4 bytes for header align.

> Do you know why rt2x00 doesn't return frame align space back to
> the headroom?

For performance reasons I guess.

If we return (payload) aligned skb to mac80211 and mac80211 will
retransmit that skb we will not align it again, since it's already
aligned. But perhaps we really need to return original tx skb to
mac80211, but if so, I would like to know detailed reason why.

Thanks
Stanislaw

Re: [PATCH] rt2x00: rt2x00queue: avoid using more headroom then driver requested

[Stanislaw Gruszka]

On Sun, Oct 05, 2014 at 03:13:12PM +0200, Stanislaw Gruszka wrote:
> On Sat, Oct 04, 2014 at 07:38:25PM -0400, Mark Asselstine wrote:
> > >> The only possibility to skb_under_panic I can see, is that we retransmit
> > >> frame and try to align it many times, but alignment should not be needed
> > >> once we aligned frame. Hence I'm not sure how those skb_under_panics can
> > >> happen.
> > 
> > I am still digging through trying to find a cause for this, without a
> > reproducer I am starting to lose hope on finding the cause.
> > 
> > I dug up this old thread
> > http://rt2x00.serialmonkey.com/pipermail/users_rt2x00.serialmonkey.com/2010-November/002457.html
> >
> > I am thinking that the cause is an interaction with 80211mac and the
> > rt2x00queue not doing enough to ensure we are sending the skb back in
> > the same state as we get it. Looking at rt2x00lib_txdone() we don't
> > return headroom that may have been taken for frame alignment and we
> > don't account for the extra 4-bytes taken for header_align when the
> > payload_align is larger then the header_align while setting up the
> > l2pad.
> 
> I think we reserve sufficient space, 4 bytes for payload align plus
> 4 bytes for header align.
> 
> > Do you know why rt2x00 doesn't return frame align space back to
> > the headroom?
> 
> For performance reasons I guess.
> 
> If we return (payload) aligned skb to mac80211 and mac80211 will
> retransmit that skb we will not align it again, since it's already
> aligned. But perhaps we really need to return original tx skb to
> mac80211, but if so, I would like to know detailed reason why.

There is some old patch from Helmut, which remove payload alignment and
movement:
http://rt2x00.serialmonkey.com/pipermail/users_rt2x00.serialmonkey.com/2011-September/004179.html  
It seems to be provided by default on OpenWRT and AFICT nobody reported
skb underflow panic there. Hence possibly it fix the problem, but still
I would like to know why. However if we apply it, I would relax extra
TX headroom requirement to 4 bytes, as that maximum what we need with
the patch.

Stanislaw