From: Nishant Sarmukadam <[email protected]>
When configuring rx antennas using CMD_RF_ANTENNA_,
the argument input is the number of antennas to be enabled. For AP,
we support 3 rx antennas and hence set the field to 3. For tx antennas,
value is a bitmap, so 0x7 enables all three.
Signed-off-by: Nishant Sarmukadam <[email protected]>
Signed-off-by: Pradeep Nemavat <[email protected]>
Signed-off-by: Thomas Pedersen <[email protected]>
---
drivers/net/wireless/mwl8k.c | 10 +++++++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index af4f2c6..f79da1b 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -3945,9 +3945,13 @@ static int mwl8k_config(struct ieee80211_hw *hw, u32 changed)
if (rc)
goto out;
- rc = mwl8k_cmd_rf_antenna(hw, MWL8K_RF_ANTENNA_RX, 0x7);
- if (!rc)
- rc = mwl8k_cmd_rf_antenna(hw, MWL8K_RF_ANTENNA_TX, 0x7);
+ rc = mwl8k_cmd_rf_antenna(hw, MWL8K_RF_ANTENNA_RX, 0x3);
+ if (rc)
+ wiphy_warn(hw->wiphy, "failed to set # of RX antennas");
+ rc = mwl8k_cmd_rf_antenna(hw, MWL8K_RF_ANTENNA_TX, 0x7);
+ if (rc)
+ wiphy_warn(hw->wiphy, "failed to set # of TX antennas");
+
} else {
rc = mwl8k_cmd_rf_tx_power(hw, conf->power_level);
if (rc)
--
1.7.0.4
On Mon, Feb 14, 2011 at 11:18:31AM +0100, Johannes Berg wrote:
> > > /* Set rssi values to dBm */
> > > hw->flags |= IEEE80211_HW_SIGNAL_DBM;
> > > + /* AP FW has rate control */
> > > + if (priv->ap_fw)
> > > + hw->flags |= IEEE80211_HW_HAS_RATE_CONTROL;
> > > hw->vif_data_size = sizeof(struct mwl8k_vif);
> > > hw->sta_data_size = sizeof(struct mwl8k_sta);
> >
> > I got this same patch (almost exactly the same -- STA firmware actually
> > also has rate control so the added AP check in this version of the patch
> > is bogus) sent to me a number of times now, the last time from Pradeep
> > Nemavat <[email protected]> in private email on Feb 26 2010. This
> > is what I wrote back to him at the time:
> >
> > | mac80211.h says:
> > |
> > | * @IEEE80211_HW_HAS_RATE_CONTROL:
> > | * The hardware or firmware includes rate control, and cannot be
> > | * controlled by the stack. As such, no rate control algorithm
> > | * should be instantiated, and the TX rate reported to userspace
> > | * will be taken from the TX status instead of the rate control
> > | * algorithm.
> > | * Note that this requires that the driver implement a number of
> > | * callbacks so it has the correct information, it needs to have
> > | * the @set_rts_threshold callback and must look at the BSS config
> > | * @use_cts_prot for G/N protection, @use_short_slot for slot
> > | * timing in 2.4 GHz and @use_short_preamble for preambles for
> > | * CCK frames.
> > |
> > | In particular, the "need to report TX rate used in the TX status"
> > | requirement is something that we don't do, which is why I didn't merge
> > | the IEEE80211_HW_HAS_RATE_CONTROL change upstream yet. (I know that
> > | it improves performance, but performance is secondary to correctness.)
>
> Well, this objection is bogus. If you ask for minstrel rate control, and
> then don't use what it does, then not only will it perform less well and
> minstrel will converge to some arbitrary value, it will also mean that
> the rate reported to userspace (say in tcpdump) is *WRONG*. That's even
> worse than not reporting it.
Well, OK, but after this patch, it will still report bogus rates to
userspace (at least it did when I last tried it). If we're addressing
this anyway, let's fix that properly.
thanks,
Lennert
Lennert,
We appreciate your feedback and are working on a correct solution,
thanks for the heads up.
Thomas
On Fri, Feb 11, 2011 at 4:34 PM, Lennert Buytenhek
<[email protected]> wrote:
> [ adding Nishant Sarmukadam and Sandesh Goel to CC ]
>
>
> On Fri, Feb 11, 2011 at 04:03:51PM -0800, Thomas Pedersen wrote:
>
>> From: Nishant Sarmukadam <[email protected]>
>>
>> Minstrel rate control algorithm is enabled by default by mac80211.
>> This is not needed since the firmware does the rate control in our case.
>>
>> Signed-off-by: Nishant Sarmukadam <[email protected]>
>> Signed-off-by: Thomas Pedersen <[email protected]>
>> ---
>> ?drivers/net/wireless/mwl8k.c | ? ?3 +++
>> ?1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
>> index f79da1b..840b9e4 100644
>> --- a/drivers/net/wireless/mwl8k.c
>> +++ b/drivers/net/wireless/mwl8k.c
>> @@ -4765,6 +4765,9 @@ static int mwl8k_firmware_load_success(struct mwl8k_priv *priv)
>>
>> ? ? ? /* Set rssi values to dBm */
>> ? ? ? hw->flags |= IEEE80211_HW_SIGNAL_DBM;
>> + ? ? /* AP FW has rate control */
>> + ? ? if (priv->ap_fw)
>> + ? ? ? ? ? ? hw->flags |= IEEE80211_HW_HAS_RATE_CONTROL;
>> ? ? ? hw->vif_data_size = sizeof(struct mwl8k_vif);
>> ? ? ? hw->sta_data_size = sizeof(struct mwl8k_sta);
>
> I got this same patch (almost exactly the same -- STA firmware actually
> also has rate control so the added AP check in this version of the patch
> is bogus) sent to me a number of times now, the last time from Pradeep
> Nemavat <[email protected]> in private email on Feb 26 2010. ?This
> is what I wrote back to him at the time:
>
> | mac80211.h says:
> |
> | ?* @IEEE80211_HW_HAS_RATE_CONTROL:
> | ?* ? ? ?The hardware or firmware includes rate control, and cannot be
> | ?* ? ? ?controlled by the stack. As such, no rate control algorithm
> | ?* ? ? ?should be instantiated, and the TX rate reported to userspace
> | ?* ? ? ?will be taken from the TX status instead of the rate control
> | ?* ? ? ?algorithm.
> | ?* ? ? ?Note that this requires that the driver implement a number of
> | ?* ? ? ?callbacks so it has the correct information, it needs to have
> | ?* ? ? ?the @set_rts_threshold callback and must look at the BSS config
> | ?* ? ? ?@use_cts_prot for G/N protection, @use_short_slot for slot
> | ?* ? ? ?timing in 2.4 GHz and @use_short_preamble for preambles for
> | ?* ? ? ?CCK frames.
> |
> | In particular, the "need to report TX rate used in the TX status"
> | requirement is something that we don't do, which is why I didn't merge
> | the IEEE80211_HW_HAS_RATE_CONTROL change upstream yet. ?(I know that
> | it improves performance, but performance is secondary to correctness.)
> |
> | One case where this is important is when you use tcpdump to look at
> | transmitted packets and at which rates they were transmitted for
> | debugging purposes -- tcpdump will report rates for each transmitted
> | packet just like for received packets, a la:
> |
> | ? ? ? ? [...]
> |
> | In the situation before this patch, transmitted packets would be
> | reported at whatever rate minstrel chose for it (while the actual rate
> | on the air might be different), but after this patch, they will all
> | just be reported at 1 Mb/s, which is hardly better.
> |
> | If there is no way to find out what rate the firmware decided to
> | transmit at, what we should perhaps do is prevent a transmit rate from
> | being sent to userspace at all. ?I.e. in the packet that tcpdump sniffs
> | from the packet socket, there should just not be a "Rate" field in the
> | Radiotap (http://www.radiotap.org) header at all. ?We might be able to do this
> | by choosing a special value in the TX status indicating that we don't
> | know the TX rate, and then having mac80211 not generate a Rate field
> | when it encounters an invalid value -- at least that would be better
> | than reporting TX rates we know to be wrong.
>
> The general strategy of the s/w team on this project at the time
> appeared to be to send me patches, to totally ignore the feedback I
> would then provide on those patches, and then to submit the exact
> same patches again two months later in the hope that I would at some
> point just get tired of NAKing them and they would get merged by way
> of reviewer exhaustion.
>
> E.g. it's probably the 3th or 4th time that I get sent this patch now,
> and while I freely admit that my reasoning above for not having merged
> this patch yet may be faulty, noone has bothered to try to convince me
> that it is, all I get is just the exact same patch resubmitted every
> N months without any indication that the previous feedback has even
> registered.
>
> I have explained my objections against this patch in detail to Nishant
> in the past, and more than once. ?Why has all that feedback just gone
> straight to /dev/null every single time?
>
>
> thanks,
> Lennert
>
From: Nishant Sarmukadam <[email protected]>
Minstrel rate control algorithm is enabled by default by mac80211.
This is not needed since the firmware does the rate control in our case.
Signed-off-by: Nishant Sarmukadam <[email protected]>
Signed-off-by: Thomas Pedersen <[email protected]>
---
drivers/net/wireless/mwl8k.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index f79da1b..840b9e4 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -4765,6 +4765,9 @@ static int mwl8k_firmware_load_success(struct mwl8k_priv *priv)
/* Set rssi values to dBm */
hw->flags |= IEEE80211_HW_SIGNAL_DBM;
+ /* AP FW has rate control */
+ if (priv->ap_fw)
+ hw->flags |= IEEE80211_HW_HAS_RATE_CONTROL;
hw->vif_data_size = sizeof(struct mwl8k_vif);
hw->sta_data_size = sizeof(struct mwl8k_sta);
--
1.7.0.4
On Sat, 2011-02-12 at 01:34 +0100, Lennert Buytenhek wrote:
> > /* Set rssi values to dBm */
> > hw->flags |= IEEE80211_HW_SIGNAL_DBM;
> > + /* AP FW has rate control */
> > + if (priv->ap_fw)
> > + hw->flags |= IEEE80211_HW_HAS_RATE_CONTROL;
> > hw->vif_data_size = sizeof(struct mwl8k_vif);
> > hw->sta_data_size = sizeof(struct mwl8k_sta);
>
> I got this same patch (almost exactly the same -- STA firmware actually
> also has rate control so the added AP check in this version of the patch
> is bogus) sent to me a number of times now, the last time from Pradeep
> Nemavat <[email protected]> in private email on Feb 26 2010. This
> is what I wrote back to him at the time:
>
> | mac80211.h says:
> |
> | * @IEEE80211_HW_HAS_RATE_CONTROL:
> | * The hardware or firmware includes rate control, and cannot be
> | * controlled by the stack. As such, no rate control algorithm
> | * should be instantiated, and the TX rate reported to userspace
> | * will be taken from the TX status instead of the rate control
> | * algorithm.
> | * Note that this requires that the driver implement a number of
> | * callbacks so it has the correct information, it needs to have
> | * the @set_rts_threshold callback and must look at the BSS config
> | * @use_cts_prot for G/N protection, @use_short_slot for slot
> | * timing in 2.4 GHz and @use_short_preamble for preambles for
> | * CCK frames.
> |
> | In particular, the "need to report TX rate used in the TX status"
> | requirement is something that we don't do, which is why I didn't merge
> | the IEEE80211_HW_HAS_RATE_CONTROL change upstream yet. (I know that
> | it improves performance, but performance is secondary to correctness.)
Well, this objection is bogus. If you ask for minstrel rate control, and
then don't use what it does, then not only will it perform less well and
minstrel will converge to some arbitrary value, it will also mean that
the rate reported to userspace (say in tcpdump) is *WRONG*. That's even
worse than not reporting it.
johannes
[ adding Nishant Sarmukadam and Sandesh Goel to CC ]
On Fri, Feb 11, 2011 at 04:03:51PM -0800, Thomas Pedersen wrote:
> From: Nishant Sarmukadam <[email protected]>
>
> Minstrel rate control algorithm is enabled by default by mac80211.
> This is not needed since the firmware does the rate control in our case.
>
> Signed-off-by: Nishant Sarmukadam <[email protected]>
> Signed-off-by: Thomas Pedersen <[email protected]>
> ---
> drivers/net/wireless/mwl8k.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
> index f79da1b..840b9e4 100644
> --- a/drivers/net/wireless/mwl8k.c
> +++ b/drivers/net/wireless/mwl8k.c
> @@ -4765,6 +4765,9 @@ static int mwl8k_firmware_load_success(struct mwl8k_priv *priv)
>
> /* Set rssi values to dBm */
> hw->flags |= IEEE80211_HW_SIGNAL_DBM;
> + /* AP FW has rate control */
> + if (priv->ap_fw)
> + hw->flags |= IEEE80211_HW_HAS_RATE_CONTROL;
> hw->vif_data_size = sizeof(struct mwl8k_vif);
> hw->sta_data_size = sizeof(struct mwl8k_sta);
I got this same patch (almost exactly the same -- STA firmware actually
also has rate control so the added AP check in this version of the patch
is bogus) sent to me a number of times now, the last time from Pradeep
Nemavat <[email protected]> in private email on Feb 26 2010. This
is what I wrote back to him at the time:
| mac80211.h says:
|
| * @IEEE80211_HW_HAS_RATE_CONTROL:
| * The hardware or firmware includes rate control, and cannot be
| * controlled by the stack. As such, no rate control algorithm
| * should be instantiated, and the TX rate reported to userspace
| * will be taken from the TX status instead of the rate control
| * algorithm.
| * Note that this requires that the driver implement a number of
| * callbacks so it has the correct information, it needs to have
| * the @set_rts_threshold callback and must look at the BSS config
| * @use_cts_prot for G/N protection, @use_short_slot for slot
| * timing in 2.4 GHz and @use_short_preamble for preambles for
| * CCK frames.
|
| In particular, the "need to report TX rate used in the TX status"
| requirement is something that we don't do, which is why I didn't merge
| the IEEE80211_HW_HAS_RATE_CONTROL change upstream yet. (I know that
| it improves performance, but performance is secondary to correctness.)
|
| One case where this is important is when you use tcpdump to look at
| transmitted packets and at which rates they were transmitted for
| debugging purposes -- tcpdump will report rates for each transmitted
| packet just like for received packets, a la:
|
| [...]
|
| In the situation before this patch, transmitted packets would be
| reported at whatever rate minstrel chose for it (while the actual rate
| on the air might be different), but after this patch, they will all
| just be reported at 1 Mb/s, which is hardly better.
|
| If there is no way to find out what rate the firmware decided to
| transmit at, what we should perhaps do is prevent a transmit rate from
| being sent to userspace at all. I.e. in the packet that tcpdump sniffs
| from the packet socket, there should just not be a "Rate" field in the
| Radiotap (http://www.radiotap.org) header at all. We might be able to do this
| by choosing a special value in the TX status indicating that we don't
| know the TX rate, and then having mac80211 not generate a Rate field
| when it encounters an invalid value -- at least that would be better
| than reporting TX rates we know to be wrong.
The general strategy of the s/w team on this project at the time
appeared to be to send me patches, to totally ignore the feedback I
would then provide on those patches, and then to submit the exact
same patches again two months later in the hope that I would at some
point just get tired of NAKing them and they would get merged by way
of reviewer exhaustion.
E.g. it's probably the 3th or 4th time that I get sent this patch now,
and while I freely admit that my reasoning above for not having merged
this patch yet may be faulty, noone has bothered to try to convince me
that it is, all I get is just the exact same patch resubmitted every
N months without any indication that the previous feedback has even
registered.
I have explained my objections against this patch in detail to Nishant
in the past, and more than once. Why has all that feedback just gone
straight to /dev/null every single time?
thanks,
Lennert