Re: [PATCH 1/5] rt2x00: set registers based on current band
On Wed, Oct 10, 2018 at 12:14:28AM +0200, Tom Psyborg wrote: > On 09/10/2018, Stanislaw Gruszka wrote: > > Patches 3-5 looks ok to me, I'll rebase and resend them. > > > > Thanks > > Stanislaw > > > > and what about patches 1-2 ? did you find any regression? I don't think they are correct nor needed. I belive you got my comments on those patches. Thanks Stanislaw
Re: [PATCH 1/5] rt2x00: set registers based on current band
On 09/10/2018, Stanislaw Gruszka wrote: > Patches 3-5 looks ok to me, I'll rebase and resend them. > > Thanks > Stanislaw > and what about patches 1-2 ? did you find any regression?
Re: [PATCH 1/5] rt2x00: set registers based on current band
Patches 3-5 looks ok to me, I'll rebase and resend them. Thanks Stanislaw
Re: [PATCH 1/5] rt2x00: set registers based on current band
On 19/09/2018, Stanislaw Gruszka wrote: > On Wed, Sep 19, 2018 at 02:47:18PM +0200, Stanislaw Gruszka wrote: >> > Can you show us how will the problem trigger on dual band devices? >> >> When you switch from some 2.4GHz channel to 5GHz channel (or vice versa) >> ->curr_band will point to old band not the new one. To fix that you >> have to move curr_band assignemt before ->config() in >> rt2x00lib_config() i.e: >> >> rt2x00dev->curr_band = conf->chandef.chan->band; >> rt2x00dev->ops->lib->config(rt2x00dev, , ieee80211_flags); >> >> However I do not see the point of replacyng rf->channel check >> to ->curr_band check. What you can do is oposite thing, replace >> wrong usage of ->curr_band in very few places in rt2800_config() >> subroutines to rf->channel check. > > Actually ->curr_band is used in rt2800_config_ant() subroutines > not in rt2800_config() subroutines so things looks ok. > > Stanislaw > Things should be ok, still if you have some of these cards, it'd be better to verify that. At least my MS150N usb card would panic when I did the very same change in rf53xx channel config, probably because of the way the btcoex idx value has been set. Luckily I figured that chipset is single-band only and the patch3 was enough to handle that: https://www.spinics.net/lists/linux-wireless/msg177430.html
Re: [PATCH 1/5] rt2x00: set registers based on current band
On Wed, Sep 19, 2018 at 02:47:18PM +0200, Stanislaw Gruszka wrote: > > Can you show us how will the problem trigger on dual band devices? > > When you switch from some 2.4GHz channel to 5GHz channel (or vice versa) > ->curr_band will point to old band not the new one. To fix that you > have to move curr_band assignemt before ->config() in > rt2x00lib_config() i.e: > > rt2x00dev->curr_band = conf->chandef.chan->band; > rt2x00dev->ops->lib->config(rt2x00dev, , ieee80211_flags); > > However I do not see the point of replacyng rf->channel check > to ->curr_band check. What you can do is oposite thing, replace > wrong usage of ->curr_band in very few places in rt2800_config() > subroutines to rf->channel check. Actually ->curr_band is used in rt2800_config_ant() subroutines not in rt2800_config() subroutines so things looks ok. Stanislaw
Re: [PATCH 1/5] rt2x00: set registers based on current band
On Wed, Sep 19, 2018 at 02:17:30PM +0200, Tomislav Požega wrote: > On Wed, 19 Sep 2018 13:05:41 +0200, Stanislaw Gruszka wrote: > > >Driver should provide on what channels are supported to mac80211, but > >user space decide what channel to use and that imply band 2.4GHz or > >5GHz. ->curr_band is just shortcut for band of current channel. Is set > >in rt2x00lib_config() after we call rt2x00dev->ops->lib->config() > >[rt2800_config() for rt2800] . So patch is wrong. Either ->curr_band > >should be set before ->config call or we need to consistently use > >rf->channel <= 14 for band check in any rt2800_config() function and > >all it's subroutines. I prefer the second solution (i.e. rf->channel) > >and now I can see few places when we use ->curr_band, what is a bug. > > Works fine, no any kind of regression, especially not performance ones. Did you test on dual band devices ? > So I don't see a reason to claim it is wrong or bug just because you > prefer current solution. It's not wrong bacause I prefer current solution. It's because ->curr_band is not updated when you call rt2800_config(). > >It's because ->curr_band initialize to 0 and NL80211_BAND_2GHZ > >happen to be 0. Also problem will not trigger on single band > >2.4GHz devices. > > Can you show us how will the problem trigger on dual band devices? When you switch from some 2.4GHz channel to 5GHz channel (or vice versa) ->curr_band will point to old band not the new one. To fix that you have to move curr_band assignemt before ->config() in rt2x00lib_config() i.e: rt2x00dev->curr_band = conf->chandef.chan->band; rt2x00dev->ops->lib->config(rt2x00dev, , ieee80211_flags); However I do not see the point of replacyng rf->channel check to ->curr_band check. What you can do is oposite thing, replace wrong usage of ->curr_band in very few places in rt2800_config() subroutines to rf->channel check. Also replacing spec->num_channels is wrong because this will stop reporting device support 5GHz band. Regards Stanislaw
Re: [PATCH 1/5] rt2x00: set registers based on current band
On Wed, 19 Sep 2018 13:05:41 +0200, Stanislaw Gruszka wrote: >Driver should provide on what channels are supported to mac80211, but >user space decide what channel to use and that imply band 2.4GHz or >5GHz. ->curr_band is just shortcut for band of current channel. Is set >in rt2x00lib_config() after we call rt2x00dev->ops->lib->config() >[rt2800_config() for rt2800] . So patch is wrong. Either ->curr_band >should be set before ->config call or we need to consistently use >rf->channel <= 14 for band check in any rt2800_config() function and >all it's subroutines. I prefer the second solution (i.e. rf->channel) >and now I can see few places when we use ->curr_band, what is a bug. Works fine, no any kind of regression, especially not performance ones. So I don't see a reason to claim it is wrong or bug just because you prefer current solution. >It's because ->curr_band initialize to 0 and NL80211_BAND_2GHZ >happen to be 0. Also problem will not trigger on single band >2.4GHz devices. Can you show us how will the problem trigger on dual band devices?
Re: [PATCH 1/5] rt2x00: set registers based on current band
On Tue, Sep 18, 2018 at 04:14:33PM +0200, Tomislav Požega wrote:
> On Tue, 18 Sep 2018 14:20:16 +0200, Stanislaw Gruszka wrote:
>
> >On Mon, Sep 17, 2018 at 06:32:51PM +0200, Tomislav Požega wrote:
> >> Use curr_band instead of rf->channel among various subroutines -
> >> mostly for 2.4GHz band but in some circumstances for 5GHz band too.
> >
> >What is the reason for that change ?
>
> Operating band should be fetched from device capabilities, not from userspace
> variables. More changes will needed to be made to accomplish that.
Driver should provide on what channels are supported to mac80211, but
user space decide what channel to use and that imply band 2.4GHz or
5GHz. ->curr_band is just shortcut for band of current channel. Is set
in rt2x00lib_config() after we call rt2x00dev->ops->lib->config()
[rt2800_config() for rt2800] . So patch is wrong. Either ->curr_band
should be set before ->config call or we need to consistently use
rf->channel <= 14 for band check in any rt2800_config() function and
all it's subroutines. I prefer the second solution (i.e. rf->channel)
and now I can see few places when we use ->curr_band, what is a bug.
> >> - if (spec->num_channels > 14) {
> >> + if (rt2x00dev->curr_band == NL80211_BAND_5GHZ) {
> >>default_power1 = rt2800_eeprom_addr(rt2x00dev,
> >>EEPROM_TXPOWER_A1);
> >>default_power2 = rt2800_eeprom_addr(rt2x00dev,
> >
> >Above looks wrong.
> >
> >Thanks
> >Stanislaw
>
> Worked fine when I tried run two USB cards (RT3070-RF0005, RT5390-RF5370).
> Why do you think it's wrong? Is there a dual-band card that operates on
> both bands at the same time?
It's because ->curr_band initialize to 0 and NL80211_BAND_2GHZ
happen to be 0. Also problem will not trigger on single band
2.4GHz devices.
Regards
Stanislaw
Re: [PATCH 1/5] rt2x00: set registers based on current band
On Tue, 18 Sep 2018 14:20:16 +0200, Stanislaw Gruszka wrote:
>On Mon, Sep 17, 2018 at 06:32:51PM +0200, Tomislav Požega wrote:
>> Use curr_band instead of rf->channel among various subroutines -
>> mostly for 2.4GHz band but in some circumstances for 5GHz band too.
>
>What is the reason for that change ?
Operating band should be fetched from device capabilities, not from userspace
variables. More changes will needed to be made to accomplish that.
>
>> @@ -9265,8 +9278,9 @@ static int rt2800_probe_hw_mode(struct rt2x00_dev
>> *rt2x00dev)
>> if (WARN_ON_ONCE(!spec->channels))
>> return -ENODEV;
>>
>> -spec->supported_bands = SUPPORT_BAND_2GHZ;
>> -if (spec->num_channels > 14)
>> +if (rt2x00dev->curr_band == NL80211_BAND_2GHZ)
>> +spec->supported_bands = SUPPORT_BAND_2GHZ;
>> +if (rt2x00dev->curr_band == NL80211_BAND_5GHZ)
>> spec->supported_bands |= SUPPORT_BAND_5GHZ;
>>
>> /*
>> @@ -9336,7 +9350,7 @@ static int rt2800_probe_hw_mode(struct rt2x00_dev
>> *rt2x00dev)
>> info[i].default_power3 = default_power3[i];
>> }
>>
>> -if (spec->num_channels > 14) {
>> +if (rt2x00dev->curr_band == NL80211_BAND_5GHZ) {
>> default_power1 = rt2800_eeprom_addr(rt2x00dev,
>> EEPROM_TXPOWER_A1);
>> default_power2 = rt2800_eeprom_addr(rt2x00dev,
>
>Above looks wrong.
>
>Thanks
>Stanislaw
Worked fine when I tried run two USB cards (RT3070-RF0005, RT5390-RF5370).
Why do you think it's wrong? Is there a dual-band card that operates on
both bands at the same time?
Re: [PATCH 1/5] rt2x00: set registers based on current band
On Mon, Sep 17, 2018 at 06:32:51PM +0200, Tomislav Požega wrote:
> Use curr_band instead of rf->channel among various subroutines -
> mostly for 2.4GHz band but in some circumstances for 5GHz band too.
What is the reason for that change ?
> @@ -9265,8 +9278,9 @@ static int rt2800_probe_hw_mode(struct rt2x00_dev
> *rt2x00dev)
> if (WARN_ON_ONCE(!spec->channels))
> return -ENODEV;
>
> - spec->supported_bands = SUPPORT_BAND_2GHZ;
> - if (spec->num_channels > 14)
> + if (rt2x00dev->curr_band == NL80211_BAND_2GHZ)
> + spec->supported_bands = SUPPORT_BAND_2GHZ;
> + if (rt2x00dev->curr_band == NL80211_BAND_5GHZ)
> spec->supported_bands |= SUPPORT_BAND_5GHZ;
>
> /*
> @@ -9336,7 +9350,7 @@ static int rt2800_probe_hw_mode(struct rt2x00_dev
> *rt2x00dev)
> info[i].default_power3 = default_power3[i];
> }
>
> - if (spec->num_channels > 14) {
> + if (rt2x00dev->curr_band == NL80211_BAND_5GHZ) {
> default_power1 = rt2800_eeprom_addr(rt2x00dev,
> EEPROM_TXPOWER_A1);
> default_power2 = rt2800_eeprom_addr(rt2x00dev,
Above looks wrong.
Thanks
Stanislaw
[PATCH 1/5] rt2x00: set registers based on current band
Use curr_band instead of rf->channel among various subroutines -
mostly for 2.4GHz band but in some circumstances for 5GHz band too.
Signed-off-by: Tomislav Požega
---
drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 130 +---
1 files changed, 72 insertions(+), 58 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index a567bc2..33968ab 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -2033,7 +2033,7 @@ static void rt2800_config_lna_gain(struct rt2x00_dev
*rt2x00dev,
u16 eeprom;
short lna_gain;
- if (libconf->rf.channel <= 14) {
+ if (rt2x00dev->curr_band == NL80211_BAND_2GHZ) {
eeprom = rt2800_eeprom_read(rt2x00dev, EEPROM_LNA);
lna_gain = rt2x00_get_field16(eeprom, EEPROM_LNA_BG);
} else if (libconf->rf.channel <= 64) {
@@ -2122,7 +2122,7 @@ static void rt2800_config_channel_rf2xxx(struct
rt2x00_dev *rt2x00dev,
} else if (rt2x00dev->default_ant.rx_chain_num == 2)
rt2x00_set_field32(>rf2, RF2_ANTENNA_RX2, 1);
- if (rf->channel > 14) {
+ if (rt2x00dev->curr_band == NL80211_BAND_5GHZ) {
/*
* When TX power is below 0, we should increase it by 7 to
* make it a positive value (Minimum value is -7).
@@ -2271,21 +2271,21 @@ static void rt2800_config_channel_rf3052(struct
rt2x00_dev *rt2x00dev,
rfcsr = rt2800_rfcsr_read(rt2x00dev, 6);
rt2x00_set_field8(, RFCSR6_R1, rf->rf2);
- if (rf->channel <= 14)
+ if (rt2x00dev->curr_band == NL80211_BAND_2GHZ)
rt2x00_set_field8(, RFCSR6_TXDIV, 2);
else
rt2x00_set_field8(, RFCSR6_TXDIV, 1);
rt2800_rfcsr_write(rt2x00dev, 6, rfcsr);
rfcsr = rt2800_rfcsr_read(rt2x00dev, 5);
- if (rf->channel <= 14)
+ if (rt2x00dev->curr_band == NL80211_BAND_2GHZ)
rt2x00_set_field8(, RFCSR5_R1, 1);
else
rt2x00_set_field8(, RFCSR5_R1, 2);
rt2800_rfcsr_write(rt2x00dev, 5, rfcsr);
rfcsr = rt2800_rfcsr_read(rt2x00dev, 12);
- if (rf->channel <= 14) {
+ if (rt2x00dev->curr_band == NL80211_BAND_2GHZ) {
rt2x00_set_field8(, RFCSR12_DR0, 3);
rt2x00_set_field8(, RFCSR12_TX_POWER,
info->default_power1);
@@ -2298,7 +2298,7 @@ static void rt2800_config_channel_rf3052(struct
rt2x00_dev *rt2x00dev,
rt2800_rfcsr_write(rt2x00dev, 12, rfcsr);
rfcsr = rt2800_rfcsr_read(rt2x00dev, 13);
- if (rf->channel <= 14) {
+ if (rt2x00dev->curr_band == NL80211_BAND_2GHZ) {
rt2x00_set_field8(, RFCSR13_DR0, 3);
rt2x00_set_field8(, RFCSR13_TX_POWER,
info->default_power2);
@@ -2318,7 +2318,7 @@ static void rt2800_config_channel_rf3052(struct
rt2x00_dev *rt2x00dev,
rt2x00_set_field8(, RFCSR1_RX2_PD, 0);
rt2x00_set_field8(, RFCSR1_TX2_PD, 0);
if (rt2x00_has_cap_bt_coexist(rt2x00dev)) {
- if (rf->channel <= 14) {
+ if (rt2x00dev->curr_band == NL80211_BAND_2GHZ) {
rt2x00_set_field8(, RFCSR1_RX0_PD, 1);
rt2x00_set_field8(, RFCSR1_TX0_PD, 1);
}
@@ -2355,7 +2355,7 @@ static void rt2800_config_channel_rf3052(struct
rt2x00_dev *rt2x00dev,
rt2800_rfcsr_write(rt2x00dev, 31, drv_data->calibration_bw20);
}
- if (rf->channel <= 14) {
+ if (rt2x00dev->curr_band == NL80211_BAND_2GHZ) {
rt2800_rfcsr_write(rt2x00dev, 7, 0xd8);
rt2800_rfcsr_write(rt2x00dev, 9, 0xc3);
rt2800_rfcsr_write(rt2x00dev, 10, 0xf1);
@@ -2408,7 +2408,7 @@ static void rt2800_config_channel_rf3052(struct
rt2x00_dev *rt2x00dev,
reg = rt2800_register_read(rt2x00dev, GPIO_CTRL);
rt2x00_set_field32(, GPIO_CTRL_DIR7, 0);
- if (rf->channel <= 14)
+ if (rt2x00dev->curr_band == NL80211_BAND_2GHZ)
rt2x00_set_field32(, GPIO_CTRL_VAL7, 1);
else
rt2x00_set_field32(, GPIO_CTRL_VAL7, 0);
@@ -2441,7 +2441,7 @@ static void rt2800_config_channel_rf3053(struct
rt2x00_dev *rt2x00dev,
rt2x00_set_field8(, BBP110_TX2_POWER, 0);
rt2800_bbp_write(rt2x00dev, 110, bbp);
- if (rf->channel <= 14) {
+ if (rt2x00dev->curr_band == NL80211_BAND_2GHZ) {
/* Restore BBP 25 & 26 for 2.4 GHz */
rt2800_bbp_write(rt2x00dev, 25, drv_data->bbp25);
rt2800_bbp_write(rt2x00dev, 26, drv_data->bbp26);
@@ -2463,14 +2463,14 @@ static void rt2800_config_channel_rf3053(struct
rt2x00_dev *rt2x00dev,
rfcsr = rt2800_rfcsr_read(rt2x00dev, 11);
rt2x00_set_field8(, RFCSR11_PLL_IDOH, 1);
- if (rf->channel <= 14)
+
