DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 12CC920831
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 7B22F60452
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Larry Finger <Larry.Finger@lwfinger.net>
Cc:     Priit Laes <plaes@plaes.org>, Kees Cook <keescook@chromium.org>,
        Jia-Ju Bai <baijiaju1990@gmail.com>,
        "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        Colin Ian King <colin.king@canonical.com>,
        Arend van Spriel <arend.vanspriel@broadcom.com>,
        Varsha Rao <rvarsha016@gmail.com>,
        linux-wireless@vger.kernel.org, b43-dev@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library
References: <cover.c0b1738f3f06bbf87f4ad3200ece93322a5f135f.1542219596.git-series.plaes@plaes.org>
        <d93962cdc5a32fe99e6e5a25172920d28583aa6a.1542219596.git-series.plaes@plaes.org>
        <d630ca3e-4070-5395-54c2-e20e0c552dc3@lwfinger.net>
        <20181118082327.ttrz2nl5owi2hoqv@plaes.org>
        <df798743-10bb-0455-be83-2fb59eca3209@lwfinger.net>
Date:   Mon, 19 Nov 2018 12:43:32 +0200
In-Reply-To: <df798743-10bb-0455-be83-2fb59eca3209@lwfinger.net> (Larry
        Finger's message of "Sun, 18 Nov 2018 13:35:57 -0600")
Message-ID: <87lg5pcppn.fsf@kamboji.qca.qualcomm.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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