brcmsmac uses info->control.sta while doing ampdu aggregation. This patch
changes the usage of the structure info->control.sta, as it is going to be
removed soon from struct ieee80211_tx_info. This patch is a pre-requisit in
order to add transmission power control (TPC) to the mac80211 subsystem.
Suggested-by: Felix Fietkau <[email protected]>
Signed-off-by: Thomas Huehn <[email protected]>
---
Change sequence of overwriting data. Thx to Johannes.
---
drivers/net/wireless/brcm80211/brcmsmac/ampdu.c | 10 +++++-----
drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c | 2 ++
drivers/net/wireless/brcm80211/brcmsmac/main.c | 2 +-
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
index 01b190a..d341c91 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
@@ -665,7 +665,7 @@ brcms_c_sendampdu(struct ampdu_info *ampdu, struct brcms_txq_info *qi,
u8 plcp0, plcp3, is40, sgi;
struct ieee80211_sta *sta;
- sta = tx_info->control.sta;
+ sta = tx_info->rate_driver_data[0];
if (rr) {
plcp0 = plcp[0];
@@ -1195,8 +1195,8 @@ static bool cb_del_ampdu_pkt(struct sk_buff *mpdu, void *arg_a)
bool rc;
rc = tx_info->flags & IEEE80211_TX_CTL_AMPDU ? true : false;
- rc = rc && (tx_info->control.sta == NULL || ampdu_pars->sta == NULL ||
- tx_info->control.sta == ampdu_pars->sta);
+ rc = rc && (tx_info->rate_driver_data[0] == NULL || ampdu_pars->sta == NULL ||
+ tx_info->rate_driver_data[0] == ampdu_pars->sta);
rc = rc && ((u8)(mpdu->priority) == ampdu_pars->tid);
return rc;
}
@@ -1210,8 +1210,8 @@ static void dma_cb_fn_ampdu(void *txi, void *arg_a)
struct ieee80211_tx_info *tx_info = (struct ieee80211_tx_info *)txi;
if ((tx_info->flags & IEEE80211_TX_CTL_AMPDU) &&
- (tx_info->control.sta == sta || sta == NULL))
- tx_info->control.sta = NULL;
+ (tx_info->rate_driver_data[0] == sta || sta == NULL))
+ tx_info->rate_driver_data[0] = NULL;
}
/*
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
index 2d5a404..f3ab8a6 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
@@ -267,6 +267,7 @@ static void brcms_set_basic_rate(struct brcm_rateset *rs, u16 rate, bool is_br)
static void brcms_ops_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
{
struct brcms_info *wl = hw->priv;
+ struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
spin_lock_bh(&wl->lock);
if (!wl->pub->up) {
@@ -275,6 +276,7 @@ static void brcms_ops_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
goto done;
}
brcms_c_sendpkt_mac80211(wl->wlc, skb, hw);
+ tx_info->rate_driver_data[0] = tx_info->control.sta;
done:
spin_unlock_bh(&wl->lock);
}
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
index 8776fbc..28dd37e3 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
@@ -879,7 +879,7 @@ brcms_c_dotxstatus(struct brcms_c_info *wlc, struct tx_status *txs)
tx_info = IEEE80211_SKB_CB(p);
h = (struct ieee80211_hdr *)((u8 *) (txh + 1) + D11_PHY_HDR_LEN);
- if (tx_info->control.sta)
+ if (tx_info->rate_driver_data[0])
scb = &wlc->pri_scb;
if (tx_info->flags & IEEE80211_TX_CTL_AMPDU) {
--
1.7.10.4
On Fri, 2012-06-29 at 05:56 -0700, Thomas Huehn wrote:
> brcmsmac uses info->control.sta while doing ampdu aggregation. This patch
> changes the usage of the structure info->control.sta, as it is going to be
> removed soon from struct ieee80211_tx_info. This patch is a pre-requisit in
> order to add transmission power control (TPC) to the mac80211 subsystem.
>
> Suggested-by: Felix Fietkau <[email protected]>
> Signed-off-by: Thomas Huehn <[email protected]>
> ---
> Change sequence of overwriting data. Thx to Johannes.
> @@ -665,7 +665,7 @@ brcms_c_sendampdu(struct ampdu_info *ampdu, struct brcms_txq_info *qi,
> u8 plcp0, plcp3, is40, sgi;
> struct ieee80211_sta *sta;
>
> - sta = tx_info->control.sta;
> + sta = tx_info->rate_driver_data[0];
Are you sure it actually works now?
johannes
Johannes Berg schrieb:
> On Fri, 2012-06-29 at 05:56 -0700, Thomas Huehn wrote:
>> brcmsmac uses info->control.sta while doing ampdu aggregation. This patch
>> changes the usage of the structure info->control.sta, as it is going to be
>> removed soon from struct ieee80211_tx_info. This patch is a pre-requisit in
>> order to add transmission power control (TPC) to the mac80211 subsystem.
>>
>> Suggested-by: Felix Fietkau <[email protected]>
>> Signed-off-by: Thomas Huehn <[email protected]>
>> ---
>> Change sequence of overwriting data. Thx to Johannes.
>
>
>> @@ -665,7 +665,7 @@ brcms_c_sendampdu(struct ampdu_info *ampdu, struct brcms_txq_info *qi,
>> u8 plcp0, plcp3, is40, sgi;
>> struct ieee80211_sta *sta;
>>
>> - sta = tx_info->control.sta;
>> + sta = tx_info->rate_driver_data[0];
>
> Are you sure it actually works now?
>From locking at the code, sta is never used after it gets overridden.
The brcmsmac driver does not use the vif pointer at all. I asked Felix
to double check as I do not have a Broadcom device over here.
Bye Thomas
Hi Arend,
>> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
>> index 01b190a..d341c91 100644
>> --- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
>> +++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
>> @@ -665,7 +665,7 @@ brcms_c_sendampdu(struct ampdu_info *ampdu, struct brcms_txq_info *qi,
>> u8 plcp0, plcp3, is40, sgi;
>> struct ieee80211_sta *sta;
>>
>> - sta = tx_info->control.sta;
>> + sta = tx_info->rate_driver_data[0];
>
> The sta pointer is not used in this function so this assignment is not
> needed. Also the rate_driver_data[0] does not yet hold the sta pointer
> upon reaching this function. brcms_c_sendampdu is called from
> brcms_c_sendpkt_mac80211 (see below).
>
> Just remove the sta pointer here.
Good catch ! v2 is already sent.
>
>> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
>> index 2d5a404..f3ab8a6 100644
>> --- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
>> +++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
>> @@ -267,6 +267,7 @@ static void brcms_set_basic_rate(struct brcm_rateset *rs, u16 rate, bool is_br)
>> static void brcms_ops_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
>> {
>> struct brcms_info *wl = hw->priv;
>> + struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
>>
>> spin_lock_bh(&wl->lock);
>> if (!wl->pub->up) {
>> @@ -275,6 +276,7 @@ static void brcms_ops_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
>> goto done;
>> }
>> brcms_c_sendpkt_mac80211(wl->wlc, skb, hw);
>> + tx_info->rate_driver_data[0] = tx_info->control.sta;
>> done:
>> spin_unlock_bh(&wl->lock);
>> }
>
> Gr. AvS
>
Greetings Thomas
On Fri, Jun 29, 2012 at 11:04:16AM -0400, thomas wrote:
> Johannes Berg schrieb:
>
> > On Fri, 2012-06-29 at 05:56 -0700, Thomas Huehn wrote:
> >> brcmsmac uses info->control.sta while doing ampdu aggregation. This patch
> >> changes the usage of the structure info->control.sta, as it is going to be
> >> removed soon from struct ieee80211_tx_info. This patch is a pre-requisit in
> >> order to add transmission power control (TPC) to the mac80211 subsystem.
> >>
> >> Suggested-by: Felix Fietkau <[email protected]>
> >> Signed-off-by: Thomas Huehn <[email protected]>
> >> ---
> >> Change sequence of overwriting data. Thx to Johannes.
> >
> >
> >> @@ -665,7 +665,7 @@ brcms_c_sendampdu(struct ampdu_info *ampdu, struct brcms_txq_info *qi,
> >> u8 plcp0, plcp3, is40, sgi;
> >> struct ieee80211_sta *sta;
> >>
> >> - sta = tx_info->control.sta;
> >> + sta = tx_info->rate_driver_data[0];
> >
> > Are you sure it actually works now?
>
>
> From locking at the code, sta is never used after it gets overridden.
> The brcmsmac driver does not use the vif pointer at all. I asked Felix
> to double check as I do not have a Broadcom device over here.
>
> Bye Thomas
Any comments on this one from the Broadcom camp?
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On 07/06/2012 09:37 PM, John W. Linville wrote:
> On Fri, Jun 29, 2012 at 11:04:16AM -0400, thomas wrote:
>> Johannes Berg schrieb:
>>
>>> On Fri, 2012-06-29 at 05:56 -0700, Thomas Huehn wrote:
>>>> brcmsmac uses info->control.sta while doing ampdu aggregation. This patch
>>>> changes the usage of the structure info->control.sta, as it is going to be
>>>> removed soon from struct ieee80211_tx_info. This patch is a pre-requisit in
>>>> order to add transmission power control (TPC) to the mac80211 subsystem.
>>>>
>>>> Suggested-by: Felix Fietkau <[email protected]>
>>>> Signed-off-by: Thomas Huehn <[email protected]>
>>>> ---
>>>> Change sequence of overwriting data. Thx to Johannes.
>>>
>>>
>>>> @@ -665,7 +665,7 @@ brcms_c_sendampdu(struct ampdu_info *ampdu, struct brcms_txq_info *qi,
>>>> u8 plcp0, plcp3, is40, sgi;
>>>> struct ieee80211_sta *sta;
>>>>
>>>> - sta = tx_info->control.sta;
>>>> + sta = tx_info->rate_driver_data[0];
>>>
>>> Are you sure it actually works now?
>>
>>
>> From locking at the code, sta is never used after it gets overridden.
>> The brcmsmac driver does not use the vif pointer at all. I asked Felix
>> to double check as I do not have a Broadcom device over here.
>>
>> Bye Thomas
>
> Any comments on this one from the Broadcom camp?
>
Hi John,
Just applied the patch to have a closer look at the changes, but I am
missing details on the whole story. I see tx_info->rate_driver_data[0]
being set in the tx callback and reset to NULL in some other path which
I believe is only called upon tid or sta removal so it does not make
sense to me.
I have pushed the patch to our internal review server as I would like
some co-workers to have a look. If anyone can provide any more details
what this patch is trying to solve, please do.
Gr. AvS
On 06/29/2012 02:56 PM, Thomas Huehn wrote:
> brcmsmac uses info->control.sta while doing ampdu aggregation. This patch
> changes the usage of the structure info->control.sta, as it is going to be
> removed soon from struct ieee80211_tx_info. This patch is a pre-requisit in
> order to add transmission power control (TPC) to the mac80211 subsystem.
>
> Suggested-by: Felix Fietkau <[email protected]>
> Signed-off-by: Thomas Huehn <[email protected]>
> ---
> Change sequence of overwriting data. Thx to Johannes.
> ---
> drivers/net/wireless/brcm80211/brcmsmac/ampdu.c | 10 +++++-----
> drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c | 2 ++
> drivers/net/wireless/brcm80211/brcmsmac/main.c | 2 +-
> 3 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
> index 01b190a..d341c91 100644
> --- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
> +++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
> @@ -665,7 +665,7 @@ brcms_c_sendampdu(struct ampdu_info *ampdu, struct brcms_txq_info *qi,
> u8 plcp0, plcp3, is40, sgi;
> struct ieee80211_sta *sta;
>
> - sta = tx_info->control.sta;
> + sta = tx_info->rate_driver_data[0];
The sta pointer is not used in this function so this assignment is not
needed. Also the rate_driver_data[0] does not yet hold the sta pointer
upon reaching this function. brcms_c_sendampdu is called from
brcms_c_sendpkt_mac80211 (see below).
Just remove the sta pointer here.
> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
> index 2d5a404..f3ab8a6 100644
> --- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
> +++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
> @@ -267,6 +267,7 @@ static void brcms_set_basic_rate(struct brcm_rateset *rs, u16 rate, bool is_br)
> static void brcms_ops_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
> {
> struct brcms_info *wl = hw->priv;
> + struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
>
> spin_lock_bh(&wl->lock);
> if (!wl->pub->up) {
> @@ -275,6 +276,7 @@ static void brcms_ops_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
> goto done;
> }
> brcms_c_sendpkt_mac80211(wl->wlc, skb, hw);
> + tx_info->rate_driver_data[0] = tx_info->control.sta;
> done:
> spin_unlock_bh(&wl->lock);
> }
Gr. AvS
On 07/10/2012 12:11 AM, Thomas Huehn wrote:
> Hi Arend,
>
>>> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
>>> index 01b190a..d341c91 100644
>>> --- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
>>> +++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
>>> @@ -665,7 +665,7 @@ brcms_c_sendampdu(struct ampdu_info *ampdu, struct brcms_txq_info *qi,
>>> u8 plcp0, plcp3, is40, sgi;
>>> struct ieee80211_sta *sta;
>>>
>>> - sta = tx_info->control.sta;
>>> + sta = tx_info->rate_driver_data[0];
>>
>> The sta pointer is not used in this function so this assignment is not
>> needed. Also the rate_driver_data[0] does not yet hold the sta pointer
>> upon reaching this function. brcms_c_sendampdu is called from
>> brcms_c_sendpkt_mac80211 (see below).
>>
>> Just remove the sta pointer here.
>
>
> Good catch ! v2 is already sent.
>
Given the subject of this one, I suspect it should have been v3. Hope
John can keep them apart.
Gr. AvS
Hi Arend,
This patch has the following goal. To implement Transmission Power
Control (TPC) in the mac80211 subsystem, I remove the STA pointer from
the ieee_tx_info structure to gain free mem for TPC annotation and I put
the sta on the stack to pass it to each driver function in the tx-path.
The broadcom brcmsmac is the only driver where the ieee_tx_info is
somewhat stored and therefore the sta point is not as straight removable
as for the other drivers. This patch resolved that issue.
Please have a look at my recent patch [PATCH] mac80211: Add transmit
power control support (TPC) where I have included this one again with
the full story in the code.
Greetings Thomas
Arend van Spriel schrieb:
> On 07/06/2012 09:37 PM, John W. Linville wrote:
>> On Fri, Jun 29, 2012 at 11:04:16AM -0400, thomas wrote:
>>> Johannes Berg schrieb:
>>>
>>>> On Fri, 2012-06-29 at 05:56 -0700, Thomas Huehn wrote:
>>>>> brcmsmac uses info->control.sta while doing ampdu aggregation. This patch
>>>>> changes the usage of the structure info->control.sta, as it is going to be
>>>>> removed soon from struct ieee80211_tx_info. This patch is a pre-requisit in
>>>>> order to add transmission power control (TPC) to the mac80211 subsystem.
>>>>>
>>>>> Suggested-by: Felix Fietkau <[email protected]>
>>>>> Signed-off-by: Thomas Huehn <[email protected]>
>>>>> ---
>>>>> Change sequence of overwriting data. Thx to Johannes.
>>>>
>>>>> @@ -665,7 +665,7 @@ brcms_c_sendampdu(struct ampdu_info *ampdu, struct brcms_txq_info *qi,
>>>>> u8 plcp0, plcp3, is40, sgi;
>>>>> struct ieee80211_sta *sta;
>>>>>
>>>>> - sta = tx_info->control.sta;
>>>>> + sta = tx_info->rate_driver_data[0];
>>>> Are you sure it actually works now?
>>>
>>> From locking at the code, sta is never used after it gets overridden.
>>> The brcmsmac driver does not use the vif pointer at all. I asked Felix
>>> to double check as I do not have a Broadcom device over here.
>>>
>>> Bye Thomas
>> Any comments on this one from the Broadcom camp?
>>
>
> Hi John,
>
> Just applied the patch to have a closer look at the changes, but I am
> missing details on the whole story. I see tx_info->rate_driver_data[0]
> being set in the tx callback and reset to NULL in some other path which
> I believe is only called upon tid or sta removal so it does not make
> sense to me.
>
> I have pushed the patch to our internal review server as I would like
> some co-workers to have a look. If anyone can provide any more details
> what this patch is trying to solve, please do.
>
> Gr. AvS
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/08/2012 09:47 PM, Thomas Huehn wrote:
> Hi Arend,
>
> This patch has the following goal. To implement Transmission Power
> Control (TPC) in the mac80211 subsystem, I remove the STA pointer from
> the ieee_tx_info structure to gain free mem for TPC annotation and I put
> the sta on the stack to pass it to each driver function in the tx-path.
> The broadcom brcmsmac is the only driver where the ieee_tx_info is
> somewhat stored and therefore the sta point is not as straight removable
> as for the other drivers. This patch resolved that issue.
> Please have a look at my recent patch [PATCH] mac80211: Add transmit
> power control support (TPC) where I have included this one again with
> the full story in the code.
>
> Greetings Thomas
>
Hi Thomas,
I understand now. I just found the rate_driver_info somewhat obscure. I
looked into mac80211.h for the description and found that those fields
are intended for internal driver use. I will comment on the original patch.
Gr. AvS