lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 17 Nov 2018 21:31:35 -0600 From: Larry Finger <Larry.Finger@...inger.net> To: Priit Laes <plaes@...es.org>, Kees Cook <keescook@...omium.org>, Jia-Ju Bai <baijiaju1990@...il.com>, Kalle Valo <kvalo@...eaurora.org>, "Gustavo A. R. Silva" <gustavo@...eddedor.com>, Colin Ian King <colin.king@...onical.com>, Arend van Spriel <arend.vanspriel@...adcom.com>, Varsha Rao <rvarsha016@...il.com>, linux-wireless@...r.kernel.org, b43-dev@...ts.infradead.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library 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@...es.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
Powered by blists - more mailing lists