2011-07-17 07:51:56

by Rafał Miłecki

Subject: [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl

Old masks were causing ugly, delayed lock ups.

Signed-off-by: Rafał Miłecki <[email protected]>
---
drivers/net/wireless/b43/phy_ht.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
index 2982103..0cc293a 100644
--- a/drivers/net/wireless/b43/phy_ht.c
+++ b/drivers/net/wireless/b43/phy_ht.c
@@ -277,12 +277,12 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
b43err(dev->wl, "MAC not suspended\n");

if (blocked) {
- b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
+ b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
} else {
- b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
- b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x1);
- b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
- b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x2);
+ b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
+ b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x1);
+ b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
+ b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x2);

if (dev->phy.radio_ver == 0x2059)
b43_radio_2059_init(dev);
--
1.7.3.4

2011-07-17 15:05:09

by Larry Finger

Subject: Re: [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl

On 07/17/2011 03:43 AM, Michael Büsch wrote:
> On Sun, 17 Jul 2011 10:30:31 +0200
> Rafał Miłecki<[email protected]> wrote:
>
>> Old masks were causing ugly, delayed lock ups.
>>
>> Signed-off-by: Rafał Miłecki<[email protected]>
>> ---
>> drivers/net/wireless/b43/phy_ht.c | 10 +++++-----
>> 1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
>> index 2982103..0cc293a 100644
>> --- a/drivers/net/wireless/b43/phy_ht.c
>> +++ b/drivers/net/wireless/b43/phy_ht.c
>> @@ -277,12 +277,12 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
>> b43err(dev->wl, "MAC not suspended\n");
>>
>> if (blocked) {
>> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> } else {
>> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> - b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x1);
>> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> - b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x2);
>> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> + b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x1);
>> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> + b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x2);
>>
>> if (dev->phy.radio_ver == 0x2059)
>> b43_radio_2059_init(dev);
>
> Why are we using mask/maskset here at all, if the mask is zero?
> You could just use phy_write.
>
> (And mask() with ~0 should always ring a bell anyway, because it does
> nothing except for possible side effects of the register read/write).

All of the Broadcom mask and maskset routines are called with the complement of
the mask. It has been a constant battle during the RE work to keep that straight.

Larry

2011-07-17 07:52:01

by Rafał Miłecki

Subject: [PATCH 2/4] b43: HT-PHY: switch to channel after enabling radio

Also change the default channel to 11. This is the first channel closed
driver switches to.

Signed-off-by: Rafał Miłecki <[email protected]>
---
drivers/net/wireless/b43/phy_ht.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
index 0cc293a..f903498 100644
--- a/drivers/net/wireless/b43/phy_ht.c
+++ b/drivers/net/wireless/b43/phy_ht.c
@@ -288,6 +288,8 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
b43_radio_2059_init(dev);
else
B43_WARN_ON(1);
+
+ b43_switch_channel(dev, dev->phy.channel);
}
}

@@ -329,7 +331,7 @@ static int b43_phy_ht_op_switch_channel(struct b43_wldev *dev,
static unsigned int b43_phy_ht_op_get_default_chan(struct b43_wldev *dev)
{
if (b43_current_band(dev->wl) == IEEE80211_BAND_2GHZ)
- return 1;
+ return 11;
return 36;
}

--
1.7.3.4

2011-07-17 16:56:51

by Rafał Miłecki

Subject: Re: [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl

W dniu 17 lipca 2011 10:43 użytkownik Michael Büsch <[email protected]> napisał:
> On Sun, 17 Jul 2011 10:30:31 +0200
> Rafał Miłecki <[email protected]> wrote:
>
>> Old masks were causing ugly, delayed lock ups.
>>
>> Signed-off-by: Rafał Miłecki <[email protected]>
>> ---
>>  drivers/net/wireless/b43/phy_ht.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
>> index 2982103..0cc293a 100644
>> --- a/drivers/net/wireless/b43/phy_ht.c
>> +++ b/drivers/net/wireless/b43/phy_ht.c
>> @@ -277,12 +277,12 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
>>               b43err(dev->wl, "MAC not suspended\n");
>>
>>       if (blocked) {
>> -             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> +             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>>       } else {
>> -             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> -             b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x1);
>> -             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> -             b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x2);
>> +             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> +             b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x1);
>> +             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> +             b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x2);
>>
>>               if (dev->phy.radio_ver == 0x2059)
>>                       b43_radio_2059_init(dev);
>
> Why are we using mask/maskset here at all, if the mask is zero?
> You could just use phy_write.
>
> (And mask() with ~0 should always ring a bell anyway, because it does
> nothing except for possible side effects of the register read/write).

Well, after re-thinking it, I've some doubts.

Thanks to using maskset we got read after every write. We have no idea
it this is needed or no. What we are really sure is that this part of
code is really sensitive. Writing 0x3 cause horrible lock ups, really
ugly to track.

Should we use read-after-write then? Or maybe just keep wl's dummy way
of operating on this register?

--
Rafał

2011-07-17 08:44:09

by Michael Büsch

Subject: Re: [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl

On Sun, 17 Jul 2011 10:30:31 +0200
Rafał Miłecki <[email protected]> wrote:

> Old masks were causing ugly, delayed lock ups.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> drivers/net/wireless/b43/phy_ht.c | 10 +++++-----
> 1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
> index 2982103..0cc293a 100644
> --- a/drivers/net/wireless/b43/phy_ht.c
> +++ b/drivers/net/wireless/b43/phy_ht.c
> @@ -277,12 +277,12 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
> b43err(dev->wl, "MAC not suspended\n");
>
> if (blocked) {
> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
> } else {
> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
> - b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x1);
> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
> - b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x2);
> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
> + b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x1);
> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
> + b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x2);
>
> if (dev->phy.radio_ver == 0x2059)
> b43_radio_2059_init(dev);

Why are we using mask/maskset here at all, if the mask is zero?
You could just use phy_write.

(And mask() with ~0 should always ring a bell anyway, because it does
nothing except for possible side effects of the register read/write).

2011-07-17 16:42:51

by Rafał Miłecki

Subject: Re: [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl

W dniu 17 lipca 2011 10:43 użytkownik Michael Büsch <[email protected]> napisał:
> On Sun, 17 Jul 2011 10:30:31 +0200
> Rafał Miłecki <[email protected]> wrote:
>
>> Old masks were causing ugly, delayed lock ups.
>>
>> Signed-off-by: Rafał Miłecki <[email protected]>
>> ---
>>  drivers/net/wireless/b43/phy_ht.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
>> index 2982103..0cc293a 100644
>> --- a/drivers/net/wireless/b43/phy_ht.c
>> +++ b/drivers/net/wireless/b43/phy_ht.c
>> @@ -277,12 +277,12 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
>>               b43err(dev->wl, "MAC not suspended\n");
>>
>>       if (blocked) {
>> -             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> +             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>>       } else {
>> -             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> -             b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x1);
>> -             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> -             b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x2);
>> +             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> +             b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x1);
>> +             b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> +             b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x2);
>>
>>               if (dev->phy.radio_ver == 0x2059)
>>                       b43_radio_2059_init(dev);
>
> Why are we using mask/maskset here at all, if the mask is zero?
> You could just use phy_write.
>
> (And mask() with ~0 should always ring a bell anyway, because it does
> nothing except for possible side effects of the register read/write).

Yeah, I implemented it this way as the result of my rush in writing HT
code. We can ignore wl's ops and be smarter in such a cases.

--
Rafał

2011-07-17 07:52:05

by Rafał Miłecki

Subject: [PATCH 3/4] b43: HT-PHY: find channel entry with regs data

Signed-off-by: Rafał Miłecki <[email protected]>
---
drivers/net/wireless/b43/radio_2059.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/b43/radio_2059.c b/drivers/net/wireless/b43/radio_2059.c
index 23dea4b..f029f6e 100644
--- a/drivers/net/wireless/b43/radio_2059.c
+++ b/drivers/net/wireless/b43/radio_2059.c
@@ -161,5 +161,14 @@ static const struct b43_phy_ht_channeltab_e_radio2059 b43_phy_ht_channeltab_radi
const struct b43_phy_ht_channeltab_e_radio2059
*b43_phy_ht_get_channeltab_e_r2059(struct b43_wldev *dev, u16 freq)
{
+ const struct b43_phy_ht_channeltab_e_radio2059 *e;
+ unsigned int i;
+
+ e = b43_phy_ht_channeltab_radio2059;
+ for (i = 0; i < ARRAY_SIZE(b43_phy_ht_channeltab_radio2059); i++, e++) {
+ if (e->freq == freq)
+ return e;
+ }
+
return NULL;
}
--
1.7.3.4

2011-07-17 07:52:10

by Rafał Miłecki

Subject: [PATCH 4/4] b43: HT-PHY: fix typo in 0x2059 radio init

Following operation was incorrectly translated:
radio_read(0x0011) -> 0xffff
radio_write(0x0011) <- 0xfff7

Signed-off-by: Rafał Miłecki <[email protected]>
---
drivers/net/wireless/b43/phy_ht.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
index f903498..2105f9c 100644
--- a/drivers/net/wireless/b43/phy_ht.c
+++ b/drivers/net/wireless/b43/phy_ht.c
@@ -148,7 +148,7 @@ static void b43_radio_2059_init(struct b43_wldev *dev)
b43_radio_mask(dev, 0x17F, ~0x1);
}

- b43_radio_mask(dev, 0x11, 0x0008);
+ b43_radio_mask(dev, 0x11, ~0x0008);
}

/**************************************************
--
1.7.3.4