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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >
> > 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