[PATCH v3 0/3] wireless: Use common cordic algorithm for b43 driver

[PATCH v3 0/3] wireless: Use common cordic algorithm for b43 driver

[Priit Laes]

b43 wireless driver includes an internal implementation of
cordic algorithm, although there's a common cordic library
which was split out from brcmsmac driver. Use that and drop
internal implementation.

During the process, cordic-algorithm related macros in
brcmfmac driver were also removed and use general macros.

Please note that this series is only compile-tested, as I
do not have access to the hardware.

Changes since v2:
- Improvements to commit messages. No functional changes.
- Collect reviewed-by bits.

Changes since v1:
- Merged brcmsmac driver patches into single patch
- Merged b43 driver patches into single patch

Priit Laes (3):
  lib: cordic: Move cordic macros and defines to header file
  brcmsmac: Use cordic-related macros from common cordic library
  b43: Use cordic algorithm from kernel library

 drivers/net/wireless/broadcom/b43/Kconfig                      |  1 +-
 drivers/net/wireless/broadcom/b43/phy_common.c                 | 47 +-------
 drivers/net/wireless/broadcom/b43/phy_common.h                 |  9 +-
 drivers/net/wireless/broadcom/b43/phy_lp.c                     | 13 +-
 drivers/net/wireless/broadcom/b43/phy_n.c                      | 13 +-
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_int.h |  7 +-
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c |  4 +-
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c   |  4 +-
 include/linux/cordic.h                                         |  9 +-
 lib/cordic.c                                                   | 23 +---
 10 files changed, 35 insertions(+), 95 deletions(-)

base-commit: ccda4af0f4b92f7b4c308d3acc262f4a7e3affad
-- 
git-series 0.9.1

[PATCH v3 1/3] lib: cordic: Move cordic macros and defines to header file

[Priit Laes]

Now that these macros are in header file, we can eventually
clean up the duplicate macros present in the drivers that
utilize the same cordic algorithm implementation.

Also add CORDIC_ prefix to nonprefixed macros.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Priit Laes <plaes@plaes.org>
---
 include/linux/cordic.h |  9 +++++++++
 lib/cordic.c           | 23 +++++++----------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/linux/cordic.h b/include/linux/cordic.h
index cf68ca4..3d656f5 100644
--- a/include/linux/cordic.h
+++ b/include/linux/cordic.h
@@ -18,6 +18,15 @@
 
 #include <linux/types.h>
 
+#define CORDIC_ANGLE_GEN	39797
+#define CORDIC_PRECISION_SHIFT	16
+#define CORDIC_NUM_ITER	(CORDIC_PRECISION_SHIFT + 2)
+
+#define CORDIC_FIXED(X)	((s32)((X) << CORDIC_PRECISION_SHIFT))
+#define CORDIC_FLOAT(X)	(((X) >= 0) \
+		? ((((X) >> (CORDIC_PRECISION_SHIFT - 1)) + 1) >> 1) \
+		: -((((-(X)) >> (CORDIC_PRECISION_SHIFT - 1)) + 1) >> 1))
+
 /**
  * struct cordic_iq - i/q coordinate.
  *
diff --git a/lib/cordic.c b/lib/cordic.c
index 6cf4778..8ef27c1 100644
--- a/lib/cordic.c
+++ b/lib/cordic.c
@@ -16,15 +16,6 @@
 #include <linux/module.h>
 #include <linux/cordic.h>
 
-#define CORDIC_ANGLE_GEN	39797
-#define CORDIC_PRECISION_SHIFT	16
-#define	CORDIC_NUM_ITER		(CORDIC_PRECISION_SHIFT + 2)
-
-#define	FIXED(X)	((s32)((X) << CORDIC_PRECISION_SHIFT))
-#define	FLOAT(X)	(((X) >= 0) \
-		? ((((X) >> (CORDIC_PRECISION_SHIFT - 1)) + 1) >> 1) \
-		: -((((-(X)) >> (CORDIC_PRECISION_SHIFT - 1)) + 1) >> 1))
-
 static const s32 arctan_table[] = {
 	2949120,
 	1740967,
@@ -64,16 +55,16 @@ struct cordic_iq cordic_calc_iq(s32 theta)
 	coord.q = 0;
 	angle = 0;
 
-	theta = FIXED(theta);
+	theta = CORDIC_FIXED(theta);
 	signtheta = (theta < 0) ? -1 : 1;
-	theta = ((theta + FIXED(180) * signtheta) % FIXED(360)) -
-		FIXED(180) * signtheta;
+	theta = ((theta + CORDIC_FIXED(180) * signtheta) % CORDIC_FIXED(360)) -
+		CORDIC_FIXED(180) * signtheta;
 
-	if (FLOAT(theta) > 90) {
-		theta -= FIXED(180);
+	if (CORDIC_FLOAT(theta) > 90) {
+		theta -= CORDIC_FIXED(180);
 		signx = -1;
-	} else if (FLOAT(theta) < -90) {
-		theta += FIXED(180);
+	} else if (CORDIC_FLOAT(theta) < -90) {
+		theta += CORDIC_FIXED(180);
 		signx = -1;
 	}
 
-- 
git-series 0.9.1

[PATCH v3 2/3] brcmsmac: Use cordic-related macros from common cordic library

[Priit Laes]

Current driver includes macro that is available from general cordic
library. Use that and drop unused duplicate and unneeded internal
definitions.

Signed-off-by: Priit Laes <plaes@plaes.org>
---
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_int.h | 7 +-------
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c | 4 ++--
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c   | 4 ++--
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_int.h b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_int.h
index 4960f7d..e9e8337 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_int.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_int.h
@@ -220,13 +220,6 @@ enum phy_cal_mode {
 #define BB_MULT_MASK		0x0000ffff
 #define BB_MULT_VALID_MASK	0x80000000
 
-#define CORDIC_AG	39797
-#define	CORDIC_NI	18
-#define	FIXED(X)	((s32)((X) << 16))
-
-#define	FLOAT(X) \
-	(((X) >= 0) ? ((((X) >> 15) + 1) >> 1) : -((((-(X)) >> 15) + 1) >> 1))
-
 #define PHY_CHAIN_TX_DISABLE_TEMP	115
 #define PHY_HYSTERESIS_DELTATEMP	5
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
index 9fb0d9f..e78a93a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
@@ -3447,8 +3447,8 @@ wlc_lcnphy_start_tx_tone(struct brcms_phy *pi, s32 f_kHz, u16 max_val,
 
 		theta += rot;
 
-		i_samp = (u16) (FLOAT(tone_samp.i * max_val) & 0x3ff);
-		q_samp = (u16) (FLOAT(tone_samp.q * max_val) & 0x3ff);
+		i_samp = (u16)(CORDIC_FLOAT(tone_samp.i * max_val) & 0x3ff);
+		q_samp = (u16)(CORDIC_FLOAT(tone_samp.q * max_val) & 0x3ff);
 		data_buf[t] = (i_samp << 10) | q_samp;
 	}
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
index a57f271..f4f5e90 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
@@ -23089,8 +23089,8 @@ wlc_phy_gen_load_samples_nphy(struct brcms_phy *pi, u32 f_kHz, u16 max_val,
 
 		theta += rot;
 
-		tone_buf[t].q = (s32) FLOAT(tone_buf[t].q * max_val);
-		tone_buf[t].i = (s32) FLOAT(tone_buf[t].i * max_val);
+		tone_buf[t].q = (s32)CORDIC_FLOAT(tone_buf[t].q * max_val);
+		tone_buf[t].i = (s32)CORDIC_FLOAT(tone_buf[t].i * max_val);
 	}
 
 	wlc_phy_loadsampletable_nphy(pi, tone_buf, num_samps);
-- 
git-series 0.9.1

[PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Priit Laes]

Kernel library has a common cordic algorithm which is identical
to internally implementatd one, so use it and drop the duplicate
implementation.

Signed-off-by: Priit Laes <plaes@plaes.org>
---
 drivers/net/wireless/broadcom/b43/Kconfig      |  1 +-
 drivers/net/wireless/broadcom/b43/phy_common.c | 47 +-------------------
 drivers/net/wireless/broadcom/b43/phy_common.h |  9 +----
 drivers/net/wireless/broadcom/b43/phy_lp.c     | 13 ++---
 drivers/net/wireless/broadcom/b43/phy_n.c      | 13 ++---
 5 files changed, 15 insertions(+), 68 deletions(-)

diff --git a/drivers/net/wireless/broadcom/b43/Kconfig b/drivers/net/wireless/broadcom/b43/Kconfig
index fba8560..3e41457 100644
--- a/drivers/net/wireless/broadcom/b43/Kconfig
+++ b/drivers/net/wireless/broadcom/b43/Kconfig
@@ -4,6 +4,7 @@ config B43
 	select BCMA if B43_BCMA
 	select SSB if B43_SSB
 	select FW_LOADER
+	select CORDIC
 	---help---
 	  b43 is a driver for the Broadcom 43xx series wireless devices.
 
diff --git a/drivers/net/wireless/broadcom/b43/phy_common.c b/drivers/net/wireless/broadcom/b43/phy_common.c
index 85f2ca9..98c4fa5 100644
--- a/drivers/net/wireless/broadcom/b43/phy_common.c
+++ b/drivers/net/wireless/broadcom/b43/phy_common.c
@@ -604,50 +604,3 @@ void b43_phy_force_clock(struct b43_wldev *dev, bool force)
 #endif
 	}
 }
-
-/* http://bcm-v4.sipsolutions.net/802.11/PHY/Cordic */
-struct b43_c32 b43_cordic(int theta)
-{
-	static const u32 arctg[] = {
-		2949120, 1740967, 919879, 466945, 234379, 117304,
-		  58666,   29335,  14668,   7334,   3667,   1833,
-		    917,     458,    229,    115,     57,     29,
-	};
-	u8 i;
-	s32 tmp;
-	s8 signx = 1;
-	u32 angle = 0;
-	struct b43_c32 ret = { .i = 39797, .q = 0, };
-
-	while (theta > (180 << 16))
-		theta -= (360 << 16);
-	while (theta < -(180 << 16))
-		theta += (360 << 16);
-
-	if (theta > (90 << 16)) {
-		theta -= (180 << 16);
-		signx = -1;
-	} else if (theta < -(90 << 16)) {
-		theta += (180 << 16);
-		signx = -1;
-	}
-
-	for (i = 0; i <= 17; i++) {
-		if (theta > angle) {
-			tmp = ret.i - (ret.q >> i);
-			ret.q += ret.i >> i;
-			ret.i = tmp;
-			angle += arctg[i];
-		} else {
-			tmp = ret.i + (ret.q >> i);
-			ret.q -= ret.i >> i;
-			ret.i = tmp;
-			angle -= arctg[i];
-		}
-	}
-
-	ret.i *= signx;
-	ret.q *= signx;
-
-	return ret;
-}
diff --git a/drivers/net/wireless/broadcom/b43/phy_common.h b/drivers/net/wireless/broadcom/b43/phy_common.h
index 57a1ad8..4213cac 100644
--- a/drivers/net/wireless/broadcom/b43/phy_common.h
+++ b/drivers/net/wireless/broadcom/b43/phy_common.h
@@ -7,13 +7,6 @@
 
 struct b43_wldev;
 
-/* Complex number using 2 32-bit signed integers */
-struct b43_c32 { s32 i, q; };
-
-#define CORDIC_CONVERT(value)	(((value) >= 0) ? \
-				 ((((value) >> 15) + 1) >> 1) : \
-				 -((((-(value)) >> 15) + 1) >> 1))
-
 /* PHY register routing bits */
 #define B43_PHYROUTE			0x0C00 /* PHY register routing bits mask */
 #define  B43_PHYROUTE_BASE		0x0000 /* Base registers */
@@ -450,6 +443,4 @@ bool b43_is_40mhz(struct b43_wldev *dev);
 
 void b43_phy_force_clock(struct b43_wldev *dev, bool force);
 
-struct b43_c32 b43_cordic(int theta);
-
 #endif /* LINUX_B43_PHY_COMMON_H_ */
diff --git a/drivers/net/wireless/broadcom/b43/phy_lp.c b/drivers/net/wireless/broadcom/b43/phy_lp.c
index 6922cbb..1718e3b 100644
--- a/drivers/net/wireless/broadcom/b43/phy_lp.c
+++ b/drivers/net/wireless/broadcom/b43/phy_lp.c
@@ -23,6 +23,7 @@
 
 */
 
+#include <linux/cordic.h>
 #include <linux/slab.h>
 
 #include "b43.h"
@@ -1780,9 +1781,9 @@ static void lpphy_start_tx_tone(struct b43_wldev *dev, s32 freq, u16 max)
 {
 	struct b43_phy_lp *lpphy = dev->phy.lp;
 	u16 buf[64];
-	int i, samples = 0, angle = 0;
+	int i, samples = 0, theta = 0;
 	int rotation = (((36 * freq) / 20) << 16) / 100;
-	struct b43_c32 sample;
+	struct cordic_iq sample;
 
 	lpphy->tx_tone_freq = freq;
 
@@ -1798,10 +1799,10 @@ static void lpphy_start_tx_tone(struct b43_wldev *dev, s32 freq, u16 max)
 	}
 
 	for (i = 0; i < samples; i++) {
-		sample = b43_cordic(angle);
-		angle += rotation;
-		buf[i] = CORDIC_CONVERT((sample.i * max) & 0xFF) << 8;
-		buf[i] |= CORDIC_CONVERT((sample.q * max) & 0xFF);
+		sample = cordic_calc_iq(theta);
+		theta += rotation;
+		buf[i] = CORDIC_FLOAT((sample.i * max) & 0xFF) << 8;
+		buf[i] |= CORDIC_FLOAT((sample.q * max) & 0xFF);
 	}
 
 	b43_lptab_write_bulk(dev, B43_LPTAB16(5, 0), samples, buf);
diff --git a/drivers/net/wireless/broadcom/b43/phy_n.c b/drivers/net/wireless/broadcom/b43/phy_n.c
index 44ab080..1f9378a 100644
--- a/drivers/net/wireless/broadcom/b43/phy_n.c
+++ b/drivers/net/wireless/broadcom/b43/phy_n.c
@@ -23,6 +23,7 @@
 
 */
 
+#include <linux/cordic.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/types.h>
@@ -1513,7 +1514,7 @@ static void b43_radio_init2055(struct b43_wldev *dev)
 
 /* http://bcm-v4.sipsolutions.net/802.11/PHY/N/LoadSampleTable */
 static int b43_nphy_load_samples(struct b43_wldev *dev,
-					struct b43_c32 *samples, u16 len) {
+					struct cordic_iq *samples, u16 len) {
 	struct b43_phy_n *nphy = dev->phy.n;
 	u16 i;
 	u32 *data;
@@ -1544,7 +1545,7 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
 {
 	int i;
 	u16 bw, len, rot, angle;
-	struct b43_c32 *samples;
+	struct cordic_iq *samples;
 
 	bw = b43_is_40mhz(dev) ? 40 : 20;
 	len = bw << 3;
@@ -1561,7 +1562,7 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
 		len = bw << 1;
 	}
 
-	samples = kcalloc(len, sizeof(struct b43_c32), GFP_KERNEL);
+	samples = kcalloc(len, sizeof(struct cordic_iq), GFP_KERNEL);
 	if (!samples) {
 		b43err(dev->wl, "allocation for samples generation failed\n");
 		return 0;
@@ -1570,10 +1571,10 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
 	angle = 0;
 
 	for (i = 0; i < len; i++) {
-		samples[i] = b43_cordic(angle);
+		samples[i] = cordic_calc_iq(angle);
 		angle += rot;
-		samples[i].q = CORDIC_CONVERT(samples[i].q * max);
-		samples[i].i = CORDIC_CONVERT(samples[i].i * max);
+		samples[i].q = CORDIC_FLOAT(samples[i].q * max);
+		samples[i].i = CORDIC_FLOAT(samples[i].i * max);
 	}
 
 	i = b43_nphy_load_samples(dev, samples, len);
-- 
git-series 0.9.1

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Michael Büsch]

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

On Wed, 14 Nov 2018 20:27:52 +0200
Priit Laes <plaes@plaes.org> wrote:

> Kernel library has a common cordic algorithm which is identical
> to internally implementatd one, so use it and drop the duplicate
> implementation.


In v2 of the series it has been said that:

Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
> I recall doing a comparison between the algorithms and thought I put 
> that in the original commit message. However, it is not there. It is not 
> exactly the same as in b43 so there are difference for certain angles, 
> most results are the same however. This implementation is slightly more 
> accurate on the full scale.


That's not my definition of "identical".

Please do not apply this patch without doing a thorough regression test
on actual b43 LP hardware.


-- 
Michael

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Priit Laes]

On Wed, Nov 14, 2018 at 07:46:28PM +0100, Michael Büsch wrote:
> On Wed, 14 Nov 2018 20:27:52 +0200
> Priit Laes <plaes@plaes.org> wrote:
> 
> > Kernel library has a common cordic algorithm which is identical
> > to internally implementatd one, so use it and drop the duplicate
> > implementation.
> 
> 
> In v2 of the series it has been said that:
> 
> Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
> > I recall doing a comparison between the algorithms and thought I put 
> > that in the original commit message. However, it is not there. It is not 
> > exactly the same as in b43 so there are difference for certain angles, 
> > most results are the same however. This implementation is slightly more 
> > accurate on the full scale.
> 
> 
> That's not my definition of "identical".
> 
> Please do not apply this patch without doing a thorough regression test
> on actual b43 LP hardware.

Indeed, there's a big discrepancy in the results of both algorithms.

Here's the test script:
https://gist.github.com/plaes/284993a4fc65e0926d0628a11f0cf874

So at current state, this is self-NAK from me too and this patch should
be dropped.

> 
> -- 
> Michael

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Kalle Valo]

Priit Laes <plaes@plaes.org> writes:

> On Wed, Nov 14, 2018 at 07:46:28PM +0100, Michael Büsch wrote:
>> On Wed, 14 Nov 2018 20:27:52 +0200
>> Priit Laes <plaes@plaes.org> wrote:
>> 
>> > Kernel library has a common cordic algorithm which is identical
>> > to internally implementatd one, so use it and drop the duplicate
>> > implementation.
>> 
>> 
>> In v2 of the series it has been said that:
>> 
>> Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
>> > I recall doing a comparison between the algorithms and thought I put 
>> > that in the original commit message. However, it is not there. It is not 
>> > exactly the same as in b43 so there are difference for certain angles, 
>> > most results are the same however. This implementation is slightly more 
>> > accurate on the full scale.
>> 
>> 
>> That's not my definition of "identical".
>> 
>> Please do not apply this patch without doing a thorough regression test
>> on actual b43 LP hardware.
>
> Indeed, there's a big discrepancy in the results of both algorithms.
>
> Here's the test script:
> https://gist.github.com/plaes/284993a4fc65e0926d0628a11f0cf874
>
> So at current state, this is self-NAK from me too and this patch should
> be dropped.

Ok, I'll drop patch 3 but keep patches 1-2 still in review. Thanks for
verifying this!

But of course it would be better if somebody could test this properly on
a real device. When that happens, please resubmit patch 3.

-- 
Kalle Valo

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Larry Finger]

On 11/14/18 12:27 PM, Priit Laes wrote:
> Kernel library has a common cordic algorithm which is identical
> to internally implementatd one, so use it and drop the duplicate
> implementation.
> 
> Signed-off-by: Priit Laes <plaes@plaes.org>
> ---
>   drivers/net/wireless/broadcom/b43/Kconfig      |  1 +-
>   drivers/net/wireless/broadcom/b43/phy_common.c | 47 +-------------------
>   drivers/net/wireless/broadcom/b43/phy_common.h |  9 +----
>   drivers/net/wireless/broadcom/b43/phy_lp.c     | 13 ++---
>   drivers/net/wireless/broadcom/b43/phy_n.c      | 13 ++---
>   5 files changed, 15 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43/Kconfig b/drivers/net/wireless/broadcom/b43/Kconfig
> index fba8560..3e41457 100644
> --- a/drivers/net/wireless/broadcom/b43/Kconfig
> +++ b/drivers/net/wireless/broadcom/b43/Kconfig
> @@ -4,6 +4,7 @@ config B43
>   	select BCMA if B43_BCMA
>   	select SSB if B43_SSB
>   	select FW_LOADER
> +	select CORDIC
>   	---help---
>   	  b43 is a driver for the Broadcom 43xx series wireless devices.
>   
> diff --git a/drivers/net/wireless/broadcom/b43/phy_common.c b/drivers/net/wireless/broadcom/b43/phy_common.c
> index 85f2ca9..98c4fa5 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_common.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_common.c
> @@ -604,50 +604,3 @@ void b43_phy_force_clock(struct b43_wldev *dev, bool force)
>   #endif
>   	}
>   }
> -
> -/* http://bcm-v4.sipsolutions.net/802.11/PHY/Cordic */
> -struct b43_c32 b43_cordic(int theta)
> -{
> -	static const u32 arctg[] = {
> -		2949120, 1740967, 919879, 466945, 234379, 117304,
> -		  58666,   29335,  14668,   7334,   3667,   1833,
> -		    917,     458,    229,    115,     57,     29,
> -	};
> -	u8 i;
> -	s32 tmp;
> -	s8 signx = 1;
> -	u32 angle = 0;
> -	struct b43_c32 ret = { .i = 39797, .q = 0, };
> -
> -	while (theta > (180 << 16))
> -		theta -= (360 << 16);
> -	while (theta < -(180 << 16))
> -		theta += (360 << 16);
> -
> -	if (theta > (90 << 16)) {
> -		theta -= (180 << 16);
> -		signx = -1;
> -	} else if (theta < -(90 << 16)) {
> -		theta += (180 << 16);
> -		signx = -1;
> -	}
> -
> -	for (i = 0; i <= 17; i++) {
> -		if (theta > angle) {
> -			tmp = ret.i - (ret.q >> i);
> -			ret.q += ret.i >> i;
> -			ret.i = tmp;
> -			angle += arctg[i];
> -		} else {
> -			tmp = ret.i + (ret.q >> i);
> -			ret.q -= ret.i >> i;
> -			ret.i = tmp;
> -			angle -= arctg[i];
> -		}
> -	}
> -
> -	ret.i *= signx;
> -	ret.q *= signx;
> -
> -	return ret;
> -}
> diff --git a/drivers/net/wireless/broadcom/b43/phy_common.h b/drivers/net/wireless/broadcom/b43/phy_common.h
> index 57a1ad8..4213cac 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_common.h
> +++ b/drivers/net/wireless/broadcom/b43/phy_common.h
> @@ -7,13 +7,6 @@
>   
>   struct b43_wldev;
>   
> -/* Complex number using 2 32-bit signed integers */
> -struct b43_c32 { s32 i, q; };
> -
> -#define CORDIC_CONVERT(value)	(((value) >= 0) ? \
> -				 ((((value) >> 15) + 1) >> 1) : \
> -				 -((((-(value)) >> 15) + 1) >> 1))
> -
>   /* PHY register routing bits */
>   #define B43_PHYROUTE			0x0C00 /* PHY register routing bits mask */
>   #define  B43_PHYROUTE_BASE		0x0000 /* Base registers */
> @@ -450,6 +443,4 @@ bool b43_is_40mhz(struct b43_wldev *dev);
>   
>   void b43_phy_force_clock(struct b43_wldev *dev, bool force);
>   
> -struct b43_c32 b43_cordic(int theta);
> -
>   #endif /* LINUX_B43_PHY_COMMON_H_ */
> diff --git a/drivers/net/wireless/broadcom/b43/phy_lp.c b/drivers/net/wireless/broadcom/b43/phy_lp.c
> index 6922cbb..1718e3b 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_lp.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_lp.c
> @@ -23,6 +23,7 @@
>   
>   */
>   
> +#include <linux/cordic.h>
>   #include <linux/slab.h>
>   
>   #include "b43.h"
> @@ -1780,9 +1781,9 @@ static void lpphy_start_tx_tone(struct b43_wldev *dev, s32 freq, u16 max)
>   {
>   	struct b43_phy_lp *lpphy = dev->phy.lp;
>   	u16 buf[64];
> -	int i, samples = 0, angle = 0;
> +	int i, samples = 0, theta = 0;
>   	int rotation = (((36 * freq) / 20) << 16) / 100;
> -	struct b43_c32 sample;
> +	struct cordic_iq sample;
>   
>   	lpphy->tx_tone_freq = freq;
>   
> @@ -1798,10 +1799,10 @@ static void lpphy_start_tx_tone(struct b43_wldev *dev, s32 freq, u16 max)
>   	}
>   
>   	for (i = 0; i < samples; i++) {
> -		sample = b43_cordic(angle);
> -		angle += rotation;
> -		buf[i] = CORDIC_CONVERT((sample.i * max) & 0xFF) << 8;
> -		buf[i] |= CORDIC_CONVERT((sample.q * max) & 0xFF);
> +		sample = cordic_calc_iq(theta);
> +		theta += rotation;
> +		buf[i] = CORDIC_FLOAT((sample.i * max) & 0xFF) << 8;
> +		buf[i] |= CORDIC_FLOAT((sample.q * max) & 0xFF);
>   	}
>   
>   	b43_lptab_write_bulk(dev, B43_LPTAB16(5, 0), samples, buf);
> diff --git a/drivers/net/wireless/broadcom/b43/phy_n.c b/drivers/net/wireless/broadcom/b43/phy_n.c
> index 44ab080..1f9378a 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_n.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_n.c
> @@ -23,6 +23,7 @@
>   
>   */
>   
> +#include <linux/cordic.h>
>   #include <linux/delay.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> @@ -1513,7 +1514,7 @@ static void b43_radio_init2055(struct b43_wldev *dev)
>   
>   /* http://bcm-v4.sipsolutions.net/802.11/PHY/N/LoadSampleTable */
>   static int b43_nphy_load_samples(struct b43_wldev *dev,
> -					struct b43_c32 *samples, u16 len) {
> +					struct cordic_iq *samples, u16 len) {
>   	struct b43_phy_n *nphy = dev->phy.n;
>   	u16 i;
>   	u32 *data;
> @@ -1544,7 +1545,7 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
>   {
>   	int i;
>   	u16 bw, len, rot, angle;
> -	struct b43_c32 *samples;
> +	struct cordic_iq *samples;
>   
>   	bw = b43_is_40mhz(dev) ? 40 : 20;
>   	len = bw << 3;
> @@ -1561,7 +1562,7 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
>   		len = bw << 1;
>   	}
>   
> -	samples = kcalloc(len, sizeof(struct b43_c32), GFP_KERNEL);
> +	samples = kcalloc(len, sizeof(struct cordic_iq), GFP_KERNEL);
>   	if (!samples) {
>   		b43err(dev->wl, "allocation for samples generation failed\n");
>   		return 0;
> @@ -1570,10 +1571,10 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
>   	angle = 0;
>   
>   	for (i = 0; i < len; i++) {
> -		samples[i] = b43_cordic(angle);
> +		samples[i] = cordic_calc_iq(angle);
>   		angle += rot;
> -		samples[i].q = CORDIC_CONVERT(samples[i].q * max);
> -		samples[i].i = CORDIC_CONVERT(samples[i].i * max);
> +		samples[i].q = CORDIC_FLOAT(samples[i].q * max);
> +		samples[i].i = CORDIC_FLOAT(samples[i].i * max);
>   	}
>   
>   	i = b43_nphy_load_samples(dev, samples, len);

There is a fundamental flaw in this patch. Routine b43_cordic() takes an angle 
in degrees scaled by 2^16, whereas cordic_calc_iq() takes an angle in degrees. 
For a given input, the two routines must get different answers. At a minimum, 
the calculation of rot would need to remove the left shift of 16.

 From what I see of the two algorithms, the method is identical once the 
differences in scaling are handled. Even so, applying this patch to b43 leads to 
a series of B43 error messages followed by a crash in kfree.

Just to add to the level of rejection: NACK.

Larry

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Priit Laes]

On Sat, Nov 17, 2018 at 09:31:35PM -0600, Larry Finger wrote:
> On 11/14/18 12:27 PM, Priit Laes wrote:
> > Kernel library has a common cordic algorithm which is identical
> > to internally implementatd one, so use it and drop the duplicate
> > implementation.
> > 
> > Signed-off-by: Priit Laes <plaes@plaes.org>
> > ---
> >   drivers/net/wireless/broadcom/b43/Kconfig      |  1 +-
> >   drivers/net/wireless/broadcom/b43/phy_common.c | 47 +-------------------
> >   drivers/net/wireless/broadcom/b43/phy_common.h |  9 +----
> >   drivers/net/wireless/broadcom/b43/phy_lp.c     | 13 ++---
> >   drivers/net/wireless/broadcom/b43/phy_n.c      | 13 ++---
> >   5 files changed, 15 insertions(+), 68 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/broadcom/b43/Kconfig b/drivers/net/wireless/broadcom/b43/Kconfig
> > index fba8560..3e41457 100644
> > --- a/drivers/net/wireless/broadcom/b43/Kconfig
> > +++ b/drivers/net/wireless/broadcom/b43/Kconfig
> > @@ -4,6 +4,7 @@ config B43
> >   	select BCMA if B43_BCMA
> >   	select SSB if B43_SSB
> >   	select FW_LOADER
> > +	select CORDIC
> >   	---help---
> >   	  b43 is a driver for the Broadcom 43xx series wireless devices.
> > diff --git a/drivers/net/wireless/broadcom/b43/phy_common.c b/drivers/net/wireless/broadcom/b43/phy_common.c
> > index 85f2ca9..98c4fa5 100644
> > --- a/drivers/net/wireless/broadcom/b43/phy_common.c
> > +++ b/drivers/net/wireless/broadcom/b43/phy_common.c
> > @@ -604,50 +604,3 @@ void b43_phy_force_clock(struct b43_wldev *dev, bool force)
> >   #endif
> >   	}
> >   }
> > -
> > -/* http://bcm-v4.sipsolutions.net/802.11/PHY/Cordic */
> > -struct b43_c32 b43_cordic(int theta)
> > -{
> > -	static const u32 arctg[] = {
> > -		2949120, 1740967, 919879, 466945, 234379, 117304,
> > -		  58666,   29335,  14668,   7334,   3667,   1833,
> > -		    917,     458,    229,    115,     57,     29,
> > -	};
> > -	u8 i;
> > -	s32 tmp;
> > -	s8 signx = 1;
> > -	u32 angle = 0;
> > -	struct b43_c32 ret = { .i = 39797, .q = 0, };
> > -
> > -	while (theta > (180 << 16))
> > -		theta -= (360 << 16);
> > -	while (theta < -(180 << 16))
> > -		theta += (360 << 16);
> > -
> > -	if (theta > (90 << 16)) {
> > -		theta -= (180 << 16);
> > -		signx = -1;
> > -	} else if (theta < -(90 << 16)) {
> > -		theta += (180 << 16);
> > -		signx = -1;
> > -	}
> > -
> > -	for (i = 0; i <= 17; i++) {
> > -		if (theta > angle) {
> > -			tmp = ret.i - (ret.q >> i);
> > -			ret.q += ret.i >> i;
> > -			ret.i = tmp;
> > -			angle += arctg[i];
> > -		} else {
> > -			tmp = ret.i + (ret.q >> i);
> > -			ret.q -= ret.i >> i;
> > -			ret.i = tmp;
> > -			angle -= arctg[i];
> > -		}
> > -	}
> > -
> > -	ret.i *= signx;
> > -	ret.q *= signx;
> > -
> > -	return ret;
> > -}
> > diff --git a/drivers/net/wireless/broadcom/b43/phy_common.h b/drivers/net/wireless/broadcom/b43/phy_common.h
> > index 57a1ad8..4213cac 100644
> > --- a/drivers/net/wireless/broadcom/b43/phy_common.h
> > +++ b/drivers/net/wireless/broadcom/b43/phy_common.h
> > @@ -7,13 +7,6 @@
> >   struct b43_wldev;
> > -/* Complex number using 2 32-bit signed integers */
> > -struct b43_c32 { s32 i, q; };
> > -
> > -#define CORDIC_CONVERT(value)	(((value) >= 0) ? \
> > -				 ((((value) >> 15) + 1) >> 1) : \
> > -				 -((((-(value)) >> 15) + 1) >> 1))
> > -
> >   /* PHY register routing bits */
> >   #define B43_PHYROUTE			0x0C00 /* PHY register routing bits mask */
> >   #define  B43_PHYROUTE_BASE		0x0000 /* Base registers */
> > @@ -450,6 +443,4 @@ bool b43_is_40mhz(struct b43_wldev *dev);
> >   void b43_phy_force_clock(struct b43_wldev *dev, bool force);
> > -struct b43_c32 b43_cordic(int theta);
> > -
> >   #endif /* LINUX_B43_PHY_COMMON_H_ */
> > diff --git a/drivers/net/wireless/broadcom/b43/phy_lp.c b/drivers/net/wireless/broadcom/b43/phy_lp.c
> > index 6922cbb..1718e3b 100644
> > --- a/drivers/net/wireless/broadcom/b43/phy_lp.c
> > +++ b/drivers/net/wireless/broadcom/b43/phy_lp.c
> > @@ -23,6 +23,7 @@
> >   */
> > +#include <linux/cordic.h>
> >   #include <linux/slab.h>
> >   #include "b43.h"
> > @@ -1780,9 +1781,9 @@ static void lpphy_start_tx_tone(struct b43_wldev *dev, s32 freq, u16 max)
> >   {
> >   	struct b43_phy_lp *lpphy = dev->phy.lp;
> >   	u16 buf[64];
> > -	int i, samples = 0, angle = 0;
> > +	int i, samples = 0, theta = 0;
> >   	int rotation = (((36 * freq) / 20) << 16) / 100;
> > -	struct b43_c32 sample;
> > +	struct cordic_iq sample;
> >   	lpphy->tx_tone_freq = freq;
> > @@ -1798,10 +1799,10 @@ static void lpphy_start_tx_tone(struct b43_wldev *dev, s32 freq, u16 max)
> >   	}
> >   	for (i = 0; i < samples; i++) {
> > -		sample = b43_cordic(angle);
> > -		angle += rotation;
> > -		buf[i] = CORDIC_CONVERT((sample.i * max) & 0xFF) << 8;
> > -		buf[i] |= CORDIC_CONVERT((sample.q * max) & 0xFF);
> > +		sample = cordic_calc_iq(theta);
> > +		theta += rotation;
> > +		buf[i] = CORDIC_FLOAT((sample.i * max) & 0xFF) << 8;
> > +		buf[i] |= CORDIC_FLOAT((sample.q * max) & 0xFF);
> >   	}
> >   	b43_lptab_write_bulk(dev, B43_LPTAB16(5, 0), samples, buf);
> > diff --git a/drivers/net/wireless/broadcom/b43/phy_n.c b/drivers/net/wireless/broadcom/b43/phy_n.c
> > index 44ab080..1f9378a 100644
> > --- a/drivers/net/wireless/broadcom/b43/phy_n.c
> > +++ b/drivers/net/wireless/broadcom/b43/phy_n.c
> > @@ -23,6 +23,7 @@
> >   */
> > +#include <linux/cordic.h>
> >   #include <linux/delay.h>
> >   #include <linux/slab.h>
> >   #include <linux/types.h>
> > @@ -1513,7 +1514,7 @@ static void b43_radio_init2055(struct b43_wldev *dev)
> >   /* http://bcm-v4.sipsolutions.net/802.11/PHY/N/LoadSampleTable */
> >   static int b43_nphy_load_samples(struct b43_wldev *dev,
> > -					struct b43_c32 *samples, u16 len) {
> > +					struct cordic_iq *samples, u16 len) {
> >   	struct b43_phy_n *nphy = dev->phy.n;
> >   	u16 i;
> >   	u32 *data;
> > @@ -1544,7 +1545,7 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
> >   {
> >   	int i;
> >   	u16 bw, len, rot, angle;
> > -	struct b43_c32 *samples;
> > +	struct cordic_iq *samples;
> >   	bw = b43_is_40mhz(dev) ? 40 : 20;
> >   	len = bw << 3;
> > @@ -1561,7 +1562,7 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
> >   		len = bw << 1;
> >   	}
> > -	samples = kcalloc(len, sizeof(struct b43_c32), GFP_KERNEL);
> > +	samples = kcalloc(len, sizeof(struct cordic_iq), GFP_KERNEL);
> >   	if (!samples) {
> >   		b43err(dev->wl, "allocation for samples generation failed\n");
> >   		return 0;
> > @@ -1570,10 +1571,10 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
> >   	angle = 0;
> >   	for (i = 0; i < len; i++) {
> > -		samples[i] = b43_cordic(angle);
> > +		samples[i] = cordic_calc_iq(angle);
> >   		angle += rot;
> > -		samples[i].q = CORDIC_CONVERT(samples[i].q * max);
> > -		samples[i].i = CORDIC_CONVERT(samples[i].i * max);
> > +		samples[i].q = CORDIC_FLOAT(samples[i].q * max);
> > +		samples[i].i = CORDIC_FLOAT(samples[i].i * max);
> >   	}
> >   	i = b43_nphy_load_samples(dev, samples, len);
> 
> There is a fundamental flaw in this patch. Routine b43_cordic() takes an
> angle in degrees scaled by 2^16, whereas cordic_calc_iq() takes an angle in
> degrees. For a given input, the two routines must get different answers. At
> a minimum, the calculation of rot would need to remove the left shift of 16.

Thanks for the hint. I modified my "test harness" a bit to plot out values
from -360 .. 360 and transformed the theta for b43_cordic argument using CORDIC_FIXED macro:

b43_cordic(CORDIC_FIXED(theta));
cordic_calc_iq(theta);

Then I plotted the results and well.. they are not that pretty. While the results give
identical answers between certain ranges of degrees, the cordic algorithm for b43 seems
to be broken for certain ranges: (-270..-180 ; -90 .. 0; 90.. 180 and 270..360).

You can find my test harnesses here:

https://gist.github.com/plaes/284993a4fc65e0926d0628a11f0cf874

> 
> From what I see of the two algorithms, the method is identical once the
> differences in scaling are handled. Even so, applying this patch to b43
> leads to a series of B43 error messages followed by a crash in kfree.
> 
> Just to add to the level of rejection: NACK.
> 
> Larry

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Larry Finger]

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

On 11/18/18 2:23 AM, Priit Laes wrote:
> On Sat, Nov 17, 2018 at 09:31:35PM -0600, Larry Finger wrote:
>> On 11/14/18 12:27 PM, Priit Laes wrote:
>>> Kernel library has a common cordic algorithm which is identical
>>> to internally implementatd one, so use it and drop the duplicate
>>> implementation.
>>>
>>> Signed-off-by: Priit Laes <plaes@plaes.org>
>>> ---
>>>    drivers/net/wireless/broadcom/b43/Kconfig      |  1 +-
>>>    drivers/net/wireless/broadcom/b43/phy_common.c | 47 +-------------------
>>>    drivers/net/wireless/broadcom/b43/phy_common.h |  9 +----
>>>    drivers/net/wireless/broadcom/b43/phy_lp.c     | 13 ++---
>>>    drivers/net/wireless/broadcom/b43/phy_n.c      | 13 ++---
>>>    5 files changed, 15 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/b43/Kconfig b/drivers/net/wireless/broadcom/b43/Kconfig
>>> index fba8560..3e41457 100644
>>> --- a/drivers/net/wireless/broadcom/b43/Kconfig
>>> +++ b/drivers/net/wireless/broadcom/b43/Kconfig
>>> @@ -4,6 +4,7 @@ config B43
>>>    	select BCMA if B43_BCMA
>>>    	select SSB if B43_SSB
>>>    	select FW_LOADER
>>> +	select CORDIC
>>>    	---help---
>>>    	  b43 is a driver for the Broadcom 43xx series wireless devices.
>>> diff --git a/drivers/net/wireless/broadcom/b43/phy_common.c b/drivers/net/wireless/broadcom/b43/phy_common.c
>>> index 85f2ca9..98c4fa5 100644
>>> --- a/drivers/net/wireless/broadcom/b43/phy_common.c
>>> +++ b/drivers/net/wireless/broadcom/b43/phy_common.c
>>> @@ -604,50 +604,3 @@ void b43_phy_force_clock(struct b43_wldev *dev, bool force)
>>>    #endif
>>>    	}
>>>    }
>>> -
>>> -/* http://bcm-v4.sipsolutions.net/802.11/PHY/Cordic */
>>> -struct b43_c32 b43_cordic(int theta)
>>> -{
>>> -	static const u32 arctg[] = {
>>> -		2949120, 1740967, 919879, 466945, 234379, 117304,
>>> -		  58666,   29335,  14668,   7334,   3667,   1833,
>>> -		    917,     458,    229,    115,     57,     29,
>>> -	};
>>> -	u8 i;
>>> -	s32 tmp;
>>> -	s8 signx = 1;
>>> -	u32 angle = 0;
>>> -	struct b43_c32 ret = { .i = 39797, .q = 0, };
>>> -
>>> -	while (theta > (180 << 16))
>>> -		theta -= (360 << 16);
>>> -	while (theta < -(180 << 16))
>>> -		theta += (360 << 16);
>>> -
>>> -	if (theta > (90 << 16)) {
>>> -		theta -= (180 << 16);
>>> -		signx = -1;
>>> -	} else if (theta < -(90 << 16)) {
>>> -		theta += (180 << 16);
>>> -		signx = -1;
>>> -	}
>>> -
>>> -	for (i = 0; i <= 17; i++) {
>>> -		if (theta > angle) {
>>> -			tmp = ret.i - (ret.q >> i);
>>> -			ret.q += ret.i >> i;
>>> -			ret.i = tmp;
>>> -			angle += arctg[i];
>>> -		} else {
>>> -			tmp = ret.i + (ret.q >> i);
>>> -			ret.q -= ret.i >> i;
>>> -			ret.i = tmp;
>>> -			angle -= arctg[i];
>>> -		}
>>> -	}
>>> -
>>> -	ret.i *= signx;
>>> -	ret.q *= signx;
>>> -
>>> -	return ret;
>>> -}
>>> diff --git a/drivers/net/wireless/broadcom/b43/phy_common.h b/drivers/net/wireless/broadcom/b43/phy_common.h
>>> index 57a1ad8..4213cac 100644
>>> --- a/drivers/net/wireless/broadcom/b43/phy_common.h
>>> +++ b/drivers/net/wireless/broadcom/b43/phy_common.h
>>> @@ -7,13 +7,6 @@
>>>    struct b43_wldev;
>>> -/* Complex number using 2 32-bit signed integers */
>>> -struct b43_c32 { s32 i, q; };
>>> -
>>> -#define CORDIC_CONVERT(value)	(((value) >= 0) ? \
>>> -				 ((((value) >> 15) + 1) >> 1) : \
>>> -				 -((((-(value)) >> 15) + 1) >> 1))
>>> -
>>>    /* PHY register routing bits */
>>>    #define B43_PHYROUTE			0x0C00 /* PHY register routing bits mask */
>>>    #define  B43_PHYROUTE_BASE		0x0000 /* Base registers */
>>> @@ -450,6 +443,4 @@ bool b43_is_40mhz(struct b43_wldev *dev);
>>>    void b43_phy_force_clock(struct b43_wldev *dev, bool force);
>>> -struct b43_c32 b43_cordic(int theta);
>>> -
>>>    #endif /* LINUX_B43_PHY_COMMON_H_ */
>>> diff --git a/drivers/net/wireless/broadcom/b43/phy_lp.c b/drivers/net/wireless/broadcom/b43/phy_lp.c
>>> index 6922cbb..1718e3b 100644
>>> --- a/drivers/net/wireless/broadcom/b43/phy_lp.c
>>> +++ b/drivers/net/wireless/broadcom/b43/phy_lp.c
>>> @@ -23,6 +23,7 @@
>>>    */
>>> +#include <linux/cordic.h>
>>>    #include <linux/slab.h>
>>>    #include "b43.h"
>>> @@ -1780,9 +1781,9 @@ static void lpphy_start_tx_tone(struct b43_wldev *dev, s32 freq, u16 max)
>>>    {
>>>    	struct b43_phy_lp *lpphy = dev->phy.lp;
>>>    	u16 buf[64];
>>> -	int i, samples = 0, angle = 0;
>>> +	int i, samples = 0, theta = 0;
>>>    	int rotation = (((36 * freq) / 20) << 16) / 100;
>>> -	struct b43_c32 sample;
>>> +	struct cordic_iq sample;
>>>    	lpphy->tx_tone_freq = freq;
>>> @@ -1798,10 +1799,10 @@ static void lpphy_start_tx_tone(struct b43_wldev *dev, s32 freq, u16 max)
>>>    	}
>>>    	for (i = 0; i < samples; i++) {
>>> -		sample = b43_cordic(angle);
>>> -		angle += rotation;
>>> -		buf[i] = CORDIC_CONVERT((sample.i * max) & 0xFF) << 8;
>>> -		buf[i] |= CORDIC_CONVERT((sample.q * max) & 0xFF);
>>> +		sample = cordic_calc_iq(theta);
>>> +		theta += rotation;
>>> +		buf[i] = CORDIC_FLOAT((sample.i * max) & 0xFF) << 8;
>>> +		buf[i] |= CORDIC_FLOAT((sample.q * max) & 0xFF);
>>>    	}
>>>    	b43_lptab_write_bulk(dev, B43_LPTAB16(5, 0), samples, buf);
>>> diff --git a/drivers/net/wireless/broadcom/b43/phy_n.c b/drivers/net/wireless/broadcom/b43/phy_n.c
>>> index 44ab080..1f9378a 100644
>>> --- a/drivers/net/wireless/broadcom/b43/phy_n.c
>>> +++ b/drivers/net/wireless/broadcom/b43/phy_n.c
>>> @@ -23,6 +23,7 @@
>>>    */
>>> +#include <linux/cordic.h>
>>>    #include <linux/delay.h>
>>>    #include <linux/slab.h>
>>>    #include <linux/types.h>
>>> @@ -1513,7 +1514,7 @@ static void b43_radio_init2055(struct b43_wldev *dev)
>>>    /* http://bcm-v4.sipsolutions.net/802.11/PHY/N/LoadSampleTable */
>>>    static int b43_nphy_load_samples(struct b43_wldev *dev,
>>> -					struct b43_c32 *samples, u16 len) {
>>> +					struct cordic_iq *samples, u16 len) {
>>>    	struct b43_phy_n *nphy = dev->phy.n;
>>>    	u16 i;
>>>    	u32 *data;
>>> @@ -1544,7 +1545,7 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
>>>    {
>>>    	int i;
>>>    	u16 bw, len, rot, angle;
>>> -	struct b43_c32 *samples;
>>> +	struct cordic_iq *samples;
>>>    	bw = b43_is_40mhz(dev) ? 40 : 20;
>>>    	len = bw << 3;
>>> @@ -1561,7 +1562,7 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
>>>    		len = bw << 1;
>>>    	}
>>> -	samples = kcalloc(len, sizeof(struct b43_c32), GFP_KERNEL);
>>> +	samples = kcalloc(len, sizeof(struct cordic_iq), GFP_KERNEL);
>>>    	if (!samples) {
>>>    		b43err(dev->wl, "allocation for samples generation failed\n");
>>>    		return 0;
>>> @@ -1570,10 +1571,10 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
>>>    	angle = 0;
>>>    	for (i = 0; i < len; i++) {
>>> -		samples[i] = b43_cordic(angle);
>>> +		samples[i] = cordic_calc_iq(angle);
>>>    		angle += rot;
>>> -		samples[i].q = CORDIC_CONVERT(samples[i].q * max);
>>> -		samples[i].i = CORDIC_CONVERT(samples[i].i * max);
>>> +		samples[i].q = CORDIC_FLOAT(samples[i].q * max);
>>> +		samples[i].i = CORDIC_FLOAT(samples[i].i * max);
>>>    	}
>>>    	i = b43_nphy_load_samples(dev, samples, len);
>>
>> There is a fundamental flaw in this patch. Routine b43_cordic() takes an
>> angle in degrees scaled by 2^16, whereas cordic_calc_iq() takes an angle in
>> degrees. For a given input, the two routines must get different answers. At
>> a minimum, the calculation of rot would need to remove the left shift of 16.
> 
> Thanks for the hint. I modified my "test harness" a bit to plot out values
> from -360 .. 360 and transformed the theta for b43_cordic argument using CORDIC_FIXED macro:
> 
> b43_cordic(CORDIC_FIXED(theta));
> cordic_calc_iq(theta);
> 
> Then I plotted the results and well.. they are not that pretty. While the results give
> identical answers between certain ranges of degrees, the cordic algorithm for b43 seems
> to be broken for certain ranges: (-270..-180 ; -90 .. 0; 90.. 180 and 270..360).
> 
> You can find my test harnesses here:
> 
> https://gist.github.com/plaes/284993a4fc65e0926d0628a11f0cf874

I found a problem with the b43 implementation. The local variables for that 
routine includes

         u32 angle = 0;

If one looks further down in the algorithm, if the reduced value of "theta" is 
less than 0, then "angle" should be negative. That causes the calculation to 
blow up. This explains why some ranges of angles got the same result for both 
routines. When that declaration is changed to "int angle = 0", the two routines 
give the same answer for all inputs.

My test setup has a hardware failure, thus I cannot test your patch, but I now 
believe it to be correct. Thus your first and third patches may be annotated with
ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>

One thing that should be done is to fix the error in the b43 code for stable as 
it was introduced in 2.6.34. I propose adding the attached patched to your 
series placed between your current 2nd and 3rd patches so that the old kernels 
get fixed. Of course, your 3rd patch will need to be revised. If all 4 of the 
patches get submitted together there will be no problems with the timing. My 
change will exist for seconds in the mainline kernel, but it will get propagated 
back through stable.

Thanks,

Larry





















[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-b43-Fix-error-in-cordic-routine.patch --]
[-- Type: text/x-patch; name="0001-b43-Fix-error-in-cordic-routine.patch", Size: 1537 bytes --]

From b42ae73ef7505de93e4c66fb9f66930e3f3d969a Mon Sep 17 00:00:00 2001
From: Larry Finger <Larry.Finger@lwfinger.net>
Date: Sun, 18 Nov 2018 13:15:07 -0600
Subject: [PATCH] b43: Fix error in cordic routine
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
To: kvalo@codeaurora.org
Cc: linux-wireless@vger.kernel.org

The cordic routine for calculating sines and cosines that was added in
commit 986504540306 ("b43: make cordic common (LP-PHY and N-PHY need it)")
contains an error whereby a quantity declared u32 can in fact go negative.

This problem was detected by Priit Laes who is switching b43 to use the
routine in the library functions of the kernel.

Fixes: 986504540306 ("b43: make cordic common (LP-PHY and N-PHY need it)")
Reported-by: Priit Laes <plaes@plaes.org>
Cc: Rafał Miłecki <zajec5@gmail.com>
Cc: Stable <stable@vger.kernel.org> # 2.6.34
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/broadcom/b43/phy_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/b43/phy_common.c b/drivers/net/wireless/broadcom/b43/phy_common.c
index 85f2ca989565..ef3ffa5ad466 100644
--- a/drivers/net/wireless/broadcom/b43/phy_common.c
+++ b/drivers/net/wireless/broadcom/b43/phy_common.c
@@ -616,7 +616,7 @@ struct b43_c32 b43_cordic(int theta)
 	u8 i;
 	s32 tmp;
 	s8 signx = 1;
-	u32 angle = 0;
+	s32 angle = 0;
 	struct b43_c32 ret = { .i = 39797, .q = 0, };
 
 	while (theta > (180 << 16))
-- 
2.16.4

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Kalle Valo]

Larry Finger <Larry.Finger@lwfinger.net> writes:

>>>> @@ -1570,10 +1571,10 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
>>>>    	angle = 0;
>>>>    	for (i = 0; i < len; i++) {
>>>> -		samples[i] = b43_cordic(angle);
>>>> +		samples[i] = cordic_calc_iq(angle);
>>>>    		angle += rot;
>>>> -		samples[i].q = CORDIC_CONVERT(samples[i].q * max);
>>>> -		samples[i].i = CORDIC_CONVERT(samples[i].i * max);
>>>> +		samples[i].q = CORDIC_FLOAT(samples[i].q * max);
>>>> +		samples[i].i = CORDIC_FLOAT(samples[i].i * max);
>>>>    	}
>>>>    	i = b43_nphy_load_samples(dev, samples, len);
>>>
>>> There is a fundamental flaw in this patch. Routine b43_cordic() takes an
>>> angle in degrees scaled by 2^16, whereas cordic_calc_iq() takes an angle in
>>> degrees. For a given input, the two routines must get different answers. At
>>> a minimum, the calculation of rot would need to remove the left shift of 16.
>>
>> Thanks for the hint. I modified my "test harness" a bit to plot out values
>> from -360 .. 360 and transformed the theta for b43_cordic argument
>> using CORDIC_FIXED macro:
>>
>> b43_cordic(CORDIC_FIXED(theta));
>> cordic_calc_iq(theta);
>>
>> Then I plotted the results and well.. they are not that pretty.
>> While the results give
>> identical answers between certain ranges of degrees, the cordic
>> algorithm for b43 seems
>> to be broken for certain ranges: (-270..-180 ; -90 .. 0; 90.. 180 and 270..360).
>>
>> You can find my test harnesses here:
>>
>> https://gist.github.com/plaes/284993a4fc65e0926d0628a11f0cf874
>
> I found a problem with the b43 implementation. The local variables for
> that routine includes
>
>         u32 angle = 0;
>
> If one looks further down in the algorithm, if the reduced value of
> "theta" is less than 0, then "angle" should be negative. That causes
> the calculation to blow up. This explains why some ranges of angles
> got the same result for both routines. When that declaration is
> changed to "int angle = 0", the two routines give the same answer for
> all inputs.
>
> My test setup has a hardware failure, thus I cannot test your patch,
> but I now believe it to be correct. Thus your first and third patches
> may be annotated with
> ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>
>
> One thing that should be done is to fix the error in the b43 code for
> stable as it was introduced in 2.6.34. I propose adding the attached
> patched to your series placed between your current 2nd and 3rd patches
> so that the old kernels get fixed. Of course, your 3rd patch will need
> to be revised. If all 4 of the patches get submitted together there
> will be no problems with the timing. My change will exist for seconds
> in the mainline kernel, but it will get propagated back through
> stable.

Sorry Larry, I'm not fully understanding what you mean here. So I'm
going to just drop the whole series and assume that Priit will submit a
new version. Please let me know if I should do something else.

-- 
Kalle Valo

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Priit Laes]

On Mon, Nov 19, 2018 at 12:43:32PM +0200, Kalle Valo wrote:
> Larry Finger <Larry.Finger@lwfinger.net> writes:
> 
> >>>> @@ -1570,10 +1571,10 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
> >>>>    	angle = 0;
> >>>>    	for (i = 0; i < len; i++) {
> >>>> -		samples[i] = b43_cordic(angle);
> >>>> +		samples[i] = cordic_calc_iq(angle);
> >>>>    		angle += rot;
> >>>> -		samples[i].q = CORDIC_CONVERT(samples[i].q * max);
> >>>> -		samples[i].i = CORDIC_CONVERT(samples[i].i * max);
> >>>> +		samples[i].q = CORDIC_FLOAT(samples[i].q * max);
> >>>> +		samples[i].i = CORDIC_FLOAT(samples[i].i * max);
> >>>>    	}
> >>>>    	i = b43_nphy_load_samples(dev, samples, len);
> >>>
> >>> There is a fundamental flaw in this patch. Routine b43_cordic() takes an
> >>> angle in degrees scaled by 2^16, whereas cordic_calc_iq() takes an angle in
> >>> degrees. For a given input, the two routines must get different answers. At
> >>> a minimum, the calculation of rot would need to remove the left shift of 16.
> >>
> >> Thanks for the hint. I modified my "test harness" a bit to plot out values
> >> from -360 .. 360 and transformed the theta for b43_cordic argument
> >> using CORDIC_FIXED macro:
> >>
> >> b43_cordic(CORDIC_FIXED(theta));
> >> cordic_calc_iq(theta);
> >>
> >> Then I plotted the results and well.. they are not that pretty.
> >> While the results give
> >> identical answers between certain ranges of degrees, the cordic
> >> algorithm for b43 seems
> >> to be broken for certain ranges: (-270..-180 ; -90 .. 0; 90.. 180 and 270..360).
> >>
> >> You can find my test harnesses here:
> >>
> >> https://gist.github.com/plaes/284993a4fc65e0926d0628a11f0cf874
> >
> > I found a problem with the b43 implementation. The local variables for
> > that routine includes
> >
> >         u32 angle = 0;
> >
> > If one looks further down in the algorithm, if the reduced value of
> > "theta" is less than 0, then "angle" should be negative. That causes
> > the calculation to blow up. This explains why some ranges of angles
> > got the same result for both routines. When that declaration is
> > changed to "int angle = 0", the two routines give the same answer for
> > all inputs.
> >
> > My test setup has a hardware failure, thus I cannot test your patch,
> > but I now believe it to be correct. Thus your first and third patches
> > may be annotated with
> > ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>
> >
> > One thing that should be done is to fix the error in the b43 code for
> > stable as it was introduced in 2.6.34. I propose adding the attached
> > patched to your series placed between your current 2nd and 3rd patches
> > so that the old kernels get fixed. Of course, your 3rd patch will need
> > to be revised. If all 4 of the patches get submitted together there
> > will be no problems with the timing. My change will exist for seconds
> > in the mainline kernel, but it will get propagated back through
> > stable.
> 
> Sorry Larry, I'm not fully understanding what you mean here. So I'm
> going to just drop the whole series and assume that Priit will submit a
> new version. Please let me know if I should do something else.

Yes, drop this one and I will submit v4 with one extra patch fixing the
cordic algorithm in the stable kernel.

> 
> -- 
> Kalle Valo

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Larry Finger]

On 11/19/18 4:43 AM, Kalle Valo wrote:
> Larry Finger <Larry.Finger@lwfinger.net> writes:
> 
>>>>> @@ -1570,10 +1571,10 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
>>>>>     	angle = 0;
>>>>>     	for (i = 0; i < len; i++) {
>>>>> -		samples[i] = b43_cordic(angle);
>>>>> +		samples[i] = cordic_calc_iq(angle);
>>>>>     		angle += rot;
>>>>> -		samples[i].q = CORDIC_CONVERT(samples[i].q * max);
>>>>> -		samples[i].i = CORDIC_CONVERT(samples[i].i * max);
>>>>> +		samples[i].q = CORDIC_FLOAT(samples[i].q * max);
>>>>> +		samples[i].i = CORDIC_FLOAT(samples[i].i * max);
>>>>>     	}
>>>>>     	i = b43_nphy_load_samples(dev, samples, len);
>>>>
>>>> There is a fundamental flaw in this patch. Routine b43_cordic() takes an
>>>> angle in degrees scaled by 2^16, whereas cordic_calc_iq() takes an angle in
>>>> degrees. For a given input, the two routines must get different answers. At
>>>> a minimum, the calculation of rot would need to remove the left shift of 16.
>>>
>>> Thanks for the hint. I modified my "test harness" a bit to plot out values
>>> from -360 .. 360 and transformed the theta for b43_cordic argument
>>> using CORDIC_FIXED macro:
>>>
>>> b43_cordic(CORDIC_FIXED(theta));
>>> cordic_calc_iq(theta);
>>>
>>> Then I plotted the results and well.. they are not that pretty.
>>> While the results give
>>> identical answers between certain ranges of degrees, the cordic
>>> algorithm for b43 seems
>>> to be broken for certain ranges: (-270..-180 ; -90 .. 0; 90.. 180 and 270..360).
>>>
>>> You can find my test harnesses here:
>>>
>>> https://gist.github.com/plaes/284993a4fc65e0926d0628a11f0cf874
>>
>> I found a problem with the b43 implementation. The local variables for
>> that routine includes
>>
>>          u32 angle = 0;
>>
>> If one looks further down in the algorithm, if the reduced value of
>> "theta" is less than 0, then "angle" should be negative. That causes
>> the calculation to blow up. This explains why some ranges of angles
>> got the same result for both routines. When that declaration is
>> changed to "int angle = 0", the two routines give the same answer for
>> all inputs.
>>
>> My test setup has a hardware failure, thus I cannot test your patch,
>> but I now believe it to be correct. Thus your first and third patches
>> may be annotated with
>> ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>
>>
>> One thing that should be done is to fix the error in the b43 code for
>> stable as it was introduced in 2.6.34. I propose adding the attached
>> patched to your series placed between your current 2nd and 3rd patches
>> so that the old kernels get fixed. Of course, your 3rd patch will need
>> to be revised. If all 4 of the patches get submitted together there
>> will be no problems with the timing. My change will exist for seconds
>> in the mainline kernel, but it will get propagated back through
>> stable.
> 
> Sorry Larry, I'm not fully understanding what you mean here. So I'm
> going to just drop the whole series and assume that Priit will submit a
> new version. Please let me know if I should do something else.

Dropping the entire series is the right thing to do. The complication is that 
with Priit's changes, my fix is irrelevant for HEAD, but it is still needed for 
stable. My patch must be submitted before his 3rd one, but then his will not 
apply cleanly.If he respins his patches and puts mine in the series before he 
changes b43, then my patch will be available for stable even though his next 
patch will replace my new code. That seems to be the best approach.

Larry

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Priit Laes]

On Sun, Nov 18, 2018 at 01:35:57PM -0600, Larry Finger wrote:
> On 11/18/18 2:23 AM, Priit Laes wrote:
> > On Sat, Nov 17, 2018 at 09:31:35PM -0600, Larry Finger wrote:
> > > On 11/14/18 12:27 PM, Priit Laes wrote:
> > > > Kernel library has a common cordic algorithm which is identical
> > > > to internally implementatd one, so use it and drop the duplicate
> > > > implementation.
> > > > 
> 
> My test setup has a hardware failure, thus I cannot test your patch, but I
> now believe it to be correct. Thus your first and third patches may be
> annotated with
> ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>
> 
> One thing that should be done is to fix the error in the b43 code for stable
> as it was introduced in 2.6.34. I propose adding the attached patched to
> your series placed between your current 2nd and 3rd patches so that the old
> kernels get fixed. Of course, your 3rd patch will need to be revised. If all
> 4 of the patches get submitted together there will be no problems with the
> timing. My change will exist for seconds in the mainline kernel, but it will
> get propagated back through stable.

Thanks!

> From b42ae73ef7505de93e4c66fb9f66930e3f3d969a Mon Sep 17 00:00:00 2001
> From: Larry Finger <Larry.Finger@lwfinger.net>
> Date: Sun, 18 Nov 2018 13:15:07 -0600
> Subject: [PATCH] b43: Fix error in cordic routine
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> To: kvalo@codeaurora.org
> Cc: linux-wireless@vger.kernel.org
> 
> The cordic routine for calculating sines and cosines that was added in
> commit 986504540306 ("b43: make cordic common (LP-PHY and N-PHY need it)")
> contains an error whereby a quantity declared u32 can in fact go negative.

It seems to be different commit though:
commit 6f98e62a9 ("b43: update cordic code to match current specs")

> This problem was detected by Priit Laes who is switching b43 to use the
> routine in the library functions of the kernel.
> 
> Fixes: 986504540306 ("b43: make cordic common (LP-PHY and N-PHY need it)")
> Reported-by: Priit Laes <plaes@plaes.org>
> Cc: Rafa?? Mi??ecki <zajec5@gmail.com>
> Cc: Stable <stable@vger.kernel.org> # 2.6.34
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
>  drivers/net/wireless/broadcom/b43/phy_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43/phy_common.c b/drivers/net/wireless/broadcom/b43/phy_common.c
> index 85f2ca989565..ef3ffa5ad466 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_common.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_common.c
> @@ -616,7 +616,7 @@ struct b43_c32 b43_cordic(int theta)
>  	u8 i;
>  	s32 tmp;
>  	s8 signx = 1;
> -	u32 angle = 0;
> +	s32 angle = 0;
>  	struct b43_c32 ret = { .i = 39797, .q = 0, };
>  
>  	while (theta > (180 << 16))
> -- 
> 2.16.4
> 

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Larry Finger]

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

On 11/19/18 5:27 AM, Priit Laes wrote:
> On Sun, Nov 18, 2018 at 01:35:57PM -0600, Larry Finger wrote:
>> On 11/18/18 2:23 AM, Priit Laes wrote:
>>> On Sat, Nov 17, 2018 at 09:31:35PM -0600, Larry Finger wrote:
>>>> On 11/14/18 12:27 PM, Priit Laes wrote:
>>>>> Kernel library has a common cordic algorithm which is identical
>>>>> to internally implementatd one, so use it and drop the duplicate
>>>>> implementation.
>>>>>
>>
>> My test setup has a hardware failure, thus I cannot test your patch, but I
>> now believe it to be correct. Thus your first and third patches may be
>> annotated with
>> ACKed-by: Larry Finger <Larry.Finger@lwfinger.net>
>>
>> One thing that should be done is to fix the error in the b43 code for stable
>> as it was introduced in 2.6.34. I propose adding the attached patched to
>> your series placed between your current 2nd and 3rd patches so that the old
>> kernels get fixed. Of course, your 3rd patch will need to be revised. If all
>> 4 of the patches get submitted together there will be no problems with the
>> timing. My change will exist for seconds in the mainline kernel, but it will
>> get propagated back through stable.
> 
> Thanks!
> 
>>  From b42ae73ef7505de93e4c66fb9f66930e3f3d969a Mon Sep 17 00:00:00 2001
>> From: Larry Finger <Larry.Finger@lwfinger.net>
>> Date: Sun, 18 Nov 2018 13:15:07 -0600
>> Subject: [PATCH] b43: Fix error in cordic routine
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>> To: kvalo@codeaurora.org
>> Cc: linux-wireless@vger.kernel.org
>>
>> The cordic routine for calculating sines and cosines that was added in
>> commit 986504540306 ("b43: make cordic common (LP-PHY and N-PHY need it)")
>> contains an error whereby a quantity declared u32 can in fact go negative.
> 
> It seems to be different commit though:
> commit 6f98e62a9 ("b43: update cordic code to match current specs")

Thanks for catching that mistake. I must have gotten one line off in my copy and 
paste. The respun version of my patch is attached.

I have now been able to test b43 on an LP-PHY device. I do not see any major 
changes, but there has to be some effect.

Larry
Larry


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-b43-Fix-error-in-cordic-routine.patch --]
[-- Type: text/x-patch; name="0001-b43-Fix-error-in-cordic-routine.patch", Size: 1529 bytes --]

From b42ae73ef7505de93e4c66fb9f66930e3f3d969a Mon Sep 17 00:00:00 2001
From: Larry Finger <Larry.Finger@lwfinger.net>
Date: Sun, 18 Nov 2018 13:15:07 -0600
Subject: [PATCH] b43: Fix error in cordic routine
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
To: kvalo@codeaurora.org
Cc: linux-wireless@vger.kernel.org

The cordic routine for calculating sines and cosines that was added in
commit 6f98e62a9f1b ("b43: update cordic code to match current specs")
contains an error whereby a quantity declared u32 can in fact go negative.

This problem was detected by Priit Laes who is switching b43 to use the
routine in the library functions of the kernel.

Fixes: 6f98e62a9f1b ("b43: update cordic code to match current specs")
Reported-by: Priit Laes <plaes@plaes.org>
Cc: Rafał Miłecki <zajec5@gmail.com>
Cc: Stable <stable@vger.kernel.org> # 2.6.34
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/broadcom/b43/phy_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/b43/phy_common.c b/drivers/net/wireless/broadcom/b43/phy_common.c
index 85f2ca989565..ef3ffa5ad466 100644
--- a/drivers/net/wireless/broadcom/b43/phy_common.c
+++ b/drivers/net/wireless/broadcom/b43/phy_common.c
@@ -616,7 +616,7 @@ struct b43_c32 b43_cordic(int theta)
 	u8 i;
 	s32 tmp;
 	s8 signx = 1;
-	u32 angle = 0;
+	s32 angle = 0;
 	struct b43_c32 ret = { .i = 39797, .q = 0, };
 
 	while (theta > (180 << 16))
-- 
2.16.4

Re: [PATCH v3 0/3] wireless: Use common cordic algorithm for b43 driver

[Kalle Valo]

Priit Laes <plaes@plaes.org> writes:

> b43 wireless driver includes an internal implementation of
> cordic algorithm, although there's a common cordic library
> which was split out from brcmsmac driver. Use that and drop
> internal implementation.
>
> During the process, cordic-algorithm related macros in
> brcmfmac driver were also removed and use general macros.
>
> Please note that this series is only compile-tested, as I
> do not have access to the hardware.

Thanks, this time all patches made it to patchwork:

https://patchwork.kernel.org/project/linux-wireless/list/?series=43097

I'll now wait for the review comments.

-- 
Kalle Valo