[PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[Larry Finger]

A typo in the specs interchanges the branches in an if statement, which
breaks operations for a BCM4306/rev 2 that has phy->analog == 1.

Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
---

John and Michael,

This patch is made for the wireless-dev tree after the bcm43xx-mac80211
directory has been moved into drivers/net/wireless, but it needs to be
applied to the wireless-mb tree as well.

Larry

Index: wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_phy.c
===================================================================
--- wireless-dev/net/wireless/bcm43xx-mac80211/bcm43xx_phy.c
+++ wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_phy.c
@@ -1895,7 +1895,7 @@ void bcm43xx_phy_set_baseband_attenuatio
 		bcm43xx_write16(dev, BCM43xx_MMIO_PHY0,
 				(bcm43xx_read16(dev, BCM43xx_MMIO_PHY0)
 				 & 0xFFF0) | baseband_attenuation);
-	} else if (phy->analog == 1) {
+	} else if (phy->analog != 1) {
 		bcm43xx_phy_write(dev, BCM43xx_PHY_DACCTL,
 				  (bcm43xx_phy_read(dev, BCM43xx_PHY_DACCTL)
 				   & 0xFFC3) | (baseband_attenuation << 2));

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[John W. Linville]

On Thu, Jul 26, 2007 at 11:33:01AM -0500, Larry Finger wrote:
> A typo in the specs interchanges the branches in an if statement, which
> breaks operations for a BCM4306/rev 2 that has phy->analog == 1.

> @@ -1895,7 +1895,7 @@ void bcm43xx_phy_set_baseband_attenuatio
>  		bcm43xx_write16(dev, BCM43xx_MMIO_PHY0,
>  				(bcm43xx_read16(dev, BCM43xx_MMIO_PHY0)
>  				 & 0xFFF0) | baseband_attenuation);
> -	} else if (phy->analog == 1) {
> +	} else if (phy->analog != 1) {
>  		bcm43xx_phy_write(dev, BCM43xx_PHY_DACCTL,
>  				  (bcm43xx_phy_read(dev, BCM43xx_PHY_DACCTL)
>  				   & 0xFFC3) | (baseband_attenuation << 2));

Larry,

How does this relate to the bcm43xx patch you asked me to revert
(and has been reverted in F-7)?  That one change "==" to ">", while
this one changes "==" to "!=".  Instead of reverting the other,
should it do the same thing as this?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[Larry Finger]

John W. Linville wrote:
> On Thu, Jul 26, 2007 at 11:33:01AM -0500, Larry Finger wrote:
>> A typo in the specs interchanges the branches in an if statement, which
>> breaks operations for a BCM4306/rev 2 that has phy->analog == 1.
> 
>> @@ -1895,7 +1895,7 @@ void bcm43xx_phy_set_baseband_attenuatio
>>  		bcm43xx_write16(dev, BCM43xx_MMIO_PHY0,
>>  				(bcm43xx_read16(dev, BCM43xx_MMIO_PHY0)
>>  				 & 0xFFF0) | baseband_attenuation);
>> -	} else if (phy->analog == 1) {
>> +	} else if (phy->analog != 1) {
>>  		bcm43xx_phy_write(dev, BCM43xx_PHY_DACCTL,
>>  				  (bcm43xx_phy_read(dev, BCM43xx_PHY_DACCTL)
>>  				   & 0xFFC3) | (baseband_attenuation << 2));
> 
> Larry,
> 
> How does this relate to the bcm43xx patch you asked me to revert
> (and has been reverted in F-7)?  That one change "==" to ">", while
> this one changes "==" to "!=".  Instead of reverting the other,
> should it do the same thing as this?

It really doesn't matter whether one uses ">" or "!=" here. The number in question is >= 0 and the 
test for for zero occurs earlier in the routine and ends with a return. Now that you mention it, it 
would be best to make bcm43xx nad bcm43xx-mac80211 look the same. I'll modify and resubmit the patch.

Larry

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[John W. Linville]

On Thu, Jul 26, 2007 at 12:56:20PM -0500, Larry Finger wrote:
> John W. Linville wrote:
> >On Thu, Jul 26, 2007 at 11:33:01AM -0500, Larry Finger wrote:
> >>A typo in the specs interchanges the branches in an if statement, which
> >>breaks operations for a BCM4306/rev 2 that has phy->analog == 1.
> >
> >>@@ -1895,7 +1895,7 @@ void bcm43xx_phy_set_baseband_attenuatio
> >> 		bcm43xx_write16(dev, BCM43xx_MMIO_PHY0,
> >> 				(bcm43xx_read16(dev, BCM43xx_MMIO_PHY0)
> >> 				 & 0xFFF0) | baseband_attenuation);
> >>-	} else if (phy->analog == 1) {
> >>+	} else if (phy->analog != 1) {
> >> 		bcm43xx_phy_write(dev, BCM43xx_PHY_DACCTL,
> >> 				  (bcm43xx_phy_read(dev, BCM43xx_PHY_DACCTL)
> >> 				   & 0xFFC3) | (baseband_attenuation << 2));
> >
> >Larry,
> >
> >How does this relate to the bcm43xx patch you asked me to revert
> >(and has been reverted in F-7)?  That one change "==" to ">", while
> >this one changes "==" to "!=".  Instead of reverting the other,
> >should it do the same thing as this?
> 
> It really doesn't matter whether one uses ">" or "!=" here. The number in 
> question is >= 0 and the test for for zero occurs earlier in the routine 
> and ends with a return. Now that you mention it, it would be best to make 
> bcm43xx nad bcm43xx-mac80211 look the same. I'll modify and resubmit the 
> patch.

Hmmm...well, if you asked to revert it on bcm43xx, why is it appropriate here?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[Larry Finger]

John W. Linville wrote:
> 
> Hmmm...well, if you asked to revert it on bcm43xx, why is it appropriate here?

AFAIK, the V4 specs have always had the typo, thus there was no patch introducing this error - it is 
original. On the other hand, the V3 specs were modified fairly late in the process, which led to the 
bcm43xx patch that needed to be reverted. I suspect that the V3 changes may have been the result of 
making them match the V4 specs. If that is the case, it made both of them wrong.

No matter, the change is needed to make some older cards work. Now I know why bcm43xx-mac80211 never 
worked on my BCM4306 until today.

Larry

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[John W. Linville]

On Thu, Jul 26, 2007 at 01:49:58PM -0500, Larry Finger wrote:
> John W. Linville wrote:
> >
> >Hmmm...well, if you asked to revert it on bcm43xx, why is it appropriate 
> >here?
> 
> AFAIK, the V4 specs have always had the typo, thus there was no patch 
> introducing this error - it is original. On the other hand, the V3 specs 
> were modified fairly late in the process, which led to the bcm43xx patch 
> that needed to be reverted. I suspect that the V3 changes may have been the 
> result of making them match the V4 specs. If that is the case, it made both 
> of them wrong.
> 
> No matter, the change is needed to make some older cards work. Now I know 
> why bcm43xx-mac80211 never worked on my BCM4306 until today.

OK, I get it...I was looking at the patch dwmw2 slipped into F-7 and
then in my head reverting that (i.e. reversing the reverse patch) --
my bad!

Michael, unless you plan to push soon I'll just apply the new patch
to wireless-dev more-or-less immediately?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[Michael Buesch]

On Thursday 26 July 2007 21:26:01 John W. Linville wrote:
> On Thu, Jul 26, 2007 at 01:49:58PM -0500, Larry Finger wrote:
> > John W. Linville wrote:
> > >
> > >Hmmm...well, if you asked to revert it on bcm43xx, why is it appropriate 
> > >here?
> > 
> > AFAIK, the V4 specs have always had the typo, thus there was no patch 
> > introducing this error - it is original. On the other hand, the V3 specs 
> > were modified fairly late in the process, which led to the bcm43xx patch 
> > that needed to be reverted. I suspect that the V3 changes may have been the 
> > result of making them match the V4 specs. If that is the case, it made both 
> > of them wrong.
> > 
> > No matter, the change is needed to make some older cards work. Now I know 
> > why bcm43xx-mac80211 never worked on my BCM4306 until today.
> 
> OK, I get it...I was looking at the patch dwmw2 slipped into F-7 and
> then in my head reverting that (i.e. reversing the reverse patch) --
> my bad!
> 
> Michael, unless you plan to push soon I'll just apply the new patch
> to wireless-dev more-or-less immediately?

Ok, please apply.

-- 
Greetings Michael.

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[David Woodhouse]

On Thu, 2007-07-26 at 13:49 -0500, Larry Finger wrote:
> No matter, the change is needed to make some older cards work. Now I
> know why bcm43xx-mac80211 never worked on my BCM4306 until today. 

Hm, it has worked for me in the past, and your patch fixes it so it
works again (at least as well as it ever did).

I still get constant complaints of 'eth1: duplicate address detected'
due to the fact that it sees its own outgoing packets, and IPv6 doesn't
work.

Then when unloading the module I get...

bcm43xx_mac80211: Radio turned off
Freeing alive inet6 address c1cefc80
unregister_netdevice: waiting for eth1 to become free. Usage count = 2


... and I have to reboot before anything related to networking works
again. When I unload the module from X rather than from a vt, the
machine sometimes locks up hard -- which I suspect is a different
problem, but I have no clue what.

-- 
dwmw2

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[Michael Buesch]

On Friday 27 July 2007 11:18:52 David Woodhouse wrote:
> I still get constant complaints of 'eth1: duplicate address detected'
> due to the fact that it sees its own outgoing packets, and IPv6 doesn't
> work.
> 
> Then when unloading the module I get...
> 
> bcm43xx_mac80211: Radio turned off
> Freeing alive inet6 address c1cefc80
> unregister_netdevice: waiting for eth1 to become free. Usage count = 2

Sounds like mac80211 problems.
Jiri, Michael?


-- 
Greetings Michael.

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[Paul TBBle Hampson]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]

On Fri, Jul 27, 2007 at 10:18:52AM +0100, David Woodhouse wrote:
> I still get constant complaints of 'eth1: duplicate address detected'
> due to the fact that it sees its own outgoing packets, and IPv6 doesn't
> work.

Last time I looked at this, I believed it was a mac80211 issue since it
affected both the bcm43xx-mac80211 and dadwifi drivers, which otherwise
functioned fine, but didn't affect (the non-mac80211) bcm43xx driver.

-- 
-----------------------------------------------------------
Paul "TBBle" Hampson, B.Sc, LPI, MCSE
On-hiatus Asian Studies student, ANU
The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361)
Paul.Hampson@Pobox.com

Of course Pacman didn't influence us as kids. If it did,
we'd be running around in darkened rooms, popping pills and
listening to repetitive music.
 -- Kristian Wilson, Nintendo, Inc, 1989

License: http://creativecommons.org/licenses/by/2.1/au/
-----------------------------------------------------------

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[Michael Buesch]

On Sunday 29 July 2007 13:59, Paul TBBle Hampson wrote:
> On Fri, Jul 27, 2007 at 10:18:52AM +0100, David Woodhouse wrote:
> > I still get constant complaints of 'eth1: duplicate address detected'
> > due to the fact that it sees its own outgoing packets, and IPv6 doesn't
> > work.
> 
> Last time I looked at this, I believed it was a mac80211 issue since it
> affected both the bcm43xx-mac80211 and dadwifi drivers, which otherwise
> functioned fine, but didn't affect (the non-mac80211) bcm43xx driver.
> 

This is almost certainly a mac80211 issue.
The TX and RX paths are completely seperate in bcm43xx-mac80211.

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[Michael Buesch]

On Thursday 26 July 2007 18:33:01 Larry Finger wrote:
> A typo in the specs interchanges the branches in an if statement, which
> breaks operations for a BCM4306/rev 2 that has phy->analog == 1.
> 
> Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
> ---
> 
> John and Michael,
> 
> This patch is made for the wireless-dev tree after the bcm43xx-mac80211
> directory has been moved into drivers/net/wireless, but it needs to be
> applied to the wireless-mb tree as well.
> 
> Larry
> 
> Index: wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_phy.c
> ===================================================================
> --- wireless-dev/net/wireless/bcm43xx-mac80211/bcm43xx_phy.c
> +++ wireless-dev/drivers/net/wireless/bcm43xx-mac80211/bcm43xx_phy.c
> @@ -1895,7 +1895,7 @@ void bcm43xx_phy_set_baseband_attenuatio
>  		bcm43xx_write16(dev, BCM43xx_MMIO_PHY0,
>  				(bcm43xx_read16(dev, BCM43xx_MMIO_PHY0)
>  				 & 0xFFF0) | baseband_attenuation);
> -	} else if (phy->analog == 1) {
> +	} else if (phy->analog != 1) {
>  		bcm43xx_phy_write(dev, BCM43xx_PHY_DACCTL,
>  				  (bcm43xx_phy_read(dev, BCM43xx_PHY_DACCTL)
>  				   & 0xFFC3) | (baseband_attenuation << 2));
> 
> 

Last time I checked this code it matched the specs. And that's not too
long ago.
Did the specs change?

-- 
Greetings Michael.

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[Larry Finger]

Michael Buesch wrote:
> 
> Last time I checked this code it matched the specs. And that's not too
> long ago.
> Did the specs change?

We had a complaint that a change in the V3 specs broke a BCM4306/rev 2 card using bcm43xx - it 
received but did not transmit. I verified the breakage and found that reverting a patch fixed the 
problem. I contacted Joseph, who found that he had mixed the branches when doing the specs.

This morning, I found that bcm43xx-mac80211 has the same problem. I have not yet asked Joseph to 
recheck the specs; however, the patch I submitted allows my phy->rev == 1 card to work. Without it, 
it does not. I think there is an error in the specs in that place.

Larry

Re: [PATCH] bcm43xx-mac80211: Fix specs typo for baseband attenuation

[Michael Buesch]

On Thursday 26 July 2007 20:03:04 Larry Finger wrote:
> Michael Buesch wrote:
> > 
> > Last time I checked this code it matched the specs. And that's not too
> > long ago.
> > Did the specs change?
> 
> We had a complaint that a change in the V3 specs broke a BCM4306/rev 2 card using bcm43xx - it 
> received but did not transmit. I verified the breakage and found that reverting a patch fixed the 
> problem. I contacted Joseph, who found that he had mixed the branches when doing the specs.
> 
> This morning, I found that bcm43xx-mac80211 has the same problem. I have not yet asked Joseph to 
> recheck the specs; however, the patch I submitted allows my phy->rev == 1 card to work. Without it, 
> it does not. I think there is an error in the specs in that place.

Ok, nice.
I always like some nice explanation like this for this kind of patches.
Because changing code based on "Specs changed, dunno why, so let's change this too"
are always hairy and broke stuff in the past. :)

I'll test and merge this as soon as you resubmitted it.

-- 
Greetings Michael.