[PATCH] b43: Fix bogus compilation warning for phy_n

[PATCH] b43: Fix bogus compilation warning for phy_n

[Larry Finger]

When cross-compiling the 2.6.39 wireless-testing source using GCC version
(SUSE Linux) 4.3.2 [gcc-4_3-branch revision 141291] on an x86_64 system,
the following warning is issued:

  CC [M]  drivers/net/wireless/b43/phy_n.o
drivers/net/wireless/b43/phy_n.c: In function ?b43_nphy_cal_tx_iq_lo?:
drivers/net/wireless/b43/phy_n.c:3096: warning: ?last? may be used
        uninitialized in this function

A quick look at the code shows that the warning is bogus and a gcc bug,
but to ensure clean compilation for all users, mark the offending variable
as uninitialized.

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

Index: wireless-testing-new/drivers/net/wireless/b43/phy_n.c
===================================================================
--- wireless-testing-new.orig/drivers/net/wireless/b43/phy_n.c
+++ wireless-testing-new/drivers/net/wireless/b43/phy_n.c
@@ -3093,7 +3093,7 @@ static int b43_nphy_cal_tx_iq_lo(struct
 	int freq;
 	bool avoid = false;
 	u8 length;
-	u16 tmp, core, type, count, max, numb, last, cmd;
+	u16 tmp, core, type, count, max, numb, uninitialized_var(last), cmd;
 	const u16 *table;
 	bool phy6or5x;
 

[PATCH] b43: Fix bogus compilation warning for phy_n

[Rafał Miłecki]

2011/5/19 Larry Finger <Larry.Finger@lwfinger.net>:
> When cross-compiling the 2.6.39 wireless-testing source using GCC version
> (SUSE Linux) 4.3.2 [gcc-4_3-branch revision 141291] on an x86_64 system,
> the following warning is issued:
>
> ?CC [M] ?drivers/net/wireless/b43/phy_n.o
> drivers/net/wireless/b43/phy_n.c: In function ?b43_nphy_cal_tx_iq_lo?:
> drivers/net/wireless/b43/phy_n.c:3096: warning: ?last? may be used
> ? ? ? ?uninitialized in this function
>
> A quick look at the code shows that the warning is bogus and a gcc bug,
> but to ensure clean compilation for all users, mark the offending variable
> as uninitialized.

Did you check for both "last" usages on this function? From my quick
review it seems "last" is set in case of
1) mphase_cal_phase_id > 2
xor
2) b43_nphy_tx_tone returning success

I'm not so sure if this patch is correct.

-- 
Rafa?

[PATCH] b43: Fix bogus compilation warning for phy_n

[Larry Finger]

On 05/19/2011 04:43 PM, Rafa? Mi?ecki wrote:
> 2011/5/19 Larry Finger<Larry.Finger@lwfinger.net>:
>> When cross-compiling the 2.6.39 wireless-testing source using GCC version
>> (SUSE Linux) 4.3.2 [gcc-4_3-branch revision 141291] on an x86_64 system,
>> the following warning is issued:
>>
>>   CC [M]  drivers/net/wireless/b43/phy_n.o
>> drivers/net/wireless/b43/phy_n.c: In function ?b43_nphy_cal_tx_iq_lo?:
>> drivers/net/wireless/b43/phy_n.c:3096: warning: ?last? may be used
>>         uninitialized in this function
>>
>> A quick look at the code shows that the warning is bogus and a gcc bug,
>> but to ensure clean compilation for all users, mark the offending variable
>> as uninitialized.
>
> Did you check for both "last" usages on this function? From my quick
> review it seems "last" is set in case of
> 1) mphase_cal_phase_id>  2
> xor
> 2) b43_nphy_tx_tone returning success
>
> I'm not so sure if this patch is correct.

My analysis is as follows: "last" is created in line 3096. In line 3256, it is 
set by the statement "last = (dev->phy.rev < 3) ? 6 : 7;". In line 3258 and 
3300, it is tested for equality with "nphy->mphase_cal_phase_id". As there is no 
path around line 3256, it seems to me that last must be assigned a value at 3256 
and the warning is bogus.

The call in line 3154 to b43_nphy_tx_tone is "error = b43_nphy_tx_tone(dev, 
freq, 250, true, false);" and does not access last.

If this patch is not correct, then last must be initialized to zero and the 
older compiler is correct and the newer ones are buggy for not reporting the 
problem.

Larry

[PATCH] b43: Fix bogus compilation warning for phy_n

[Rafał Miłecki]

W dniu 20 maja 2011 00:12 u?ytkownik Larry Finger
<Larry.Finger@lwfinger.net> napisa?:
> On 05/19/2011 04:43 PM, Rafa? Mi?ecki wrote:
>>
>> 2011/5/19 Larry Finger<Larry.Finger@lwfinger.net>:
>>>
>>> When cross-compiling the 2.6.39 wireless-testing source using GCC version
>>> (SUSE Linux) 4.3.2 [gcc-4_3-branch revision 141291] on an x86_64 system,
>>> the following warning is issued:
>>>
>>> ?CC [M] ?drivers/net/wireless/b43/phy_n.o
>>> drivers/net/wireless/b43/phy_n.c: In function ?b43_nphy_cal_tx_iq_lo?:
>>> drivers/net/wireless/b43/phy_n.c:3096: warning: ?last? may be used
>>> ? ? ? ?uninitialized in this function
>>>
>>> A quick look at the code shows that the warning is bogus and a gcc bug,
>>> but to ensure clean compilation for all users, mark the offending
>>> variable
>>> as uninitialized.
>>
>> Did you check for both "last" usages on this function? From my quick
>> review it seems "last" is set in case of
>> 1) mphase_cal_phase_id> ?2
>> xor
>> 2) b43_nphy_tx_tone returning success
>>
>> I'm not so sure if this patch is correct.
>
> My analysis is as follows: "last" is created in line 3096. In line 3256, it
> is set by the statement "last = (dev->phy.rev < 3) ? 6 : 7;". In line 3258
> and 3300, it is tested for equality with "nphy->mphase_cal_phase_id". As
> there is no path around line 3256, it seems to me that last must be assigned
> a value at 3256 and the warning is bogus.
>
> The call in line 3154 to b43_nphy_tx_tone is "error = b43_nphy_tx_tone(dev,
> freq, 250, true, false);" and does not access last.
>
> If this patch is not correct, then last must be initialized to zero and the
> older compiler is correct and the newer ones are buggy for not reporting the
> problem.

I should have describe where I can see the problem.

If error is other than 0 (it can be as the result of "error =
b43_nphy_tx_tone(dev, freq, 250, true, false);"), then "last" won't be
set in 3256. In 3300 we use "last", no matter what "error" is.

-- 
Rafa?

[PATCH] b43: Fix bogus compilation warning for phy_n

[Larry Finger]

On 05/19/2011 05:40 PM, Rafa? Mi?ecki wrote:
>
> I should have describe where I can see the problem.
>
> If error is other than 0 (it can be as the result of "error =
> b43_nphy_tx_tone(dev, freq, 250, true, false);"), then "last" won't be
> set in 3256. In 3300 we use "last", no matter what "error" is.

You are correct. I missed the level of indent in line 3256. What initialization 
should be used for last?  It clearly needs to be set to something.

John: Please drop my patch. I'll let Rafa? fix it.

Larry

[PATCH] b43: Fix bogus compilation warning for phy_n

[Rafał Miłecki]

W dniu 20 maja 2011 00:48 u?ytkownik Larry Finger
<Larry.Finger@lwfinger.net> napisa?:
> On 05/19/2011 05:40 PM, Rafa? Mi?ecki wrote:
>>
>> I should have describe where I can see the problem.
>>
>> If error is other than 0 (it can be as the result of "error =
>> b43_nphy_tx_tone(dev, freq, 250, true, false);"), then "last" won't be
>> set in 3256. In 3300 we use "last", no matter what "error" is.
>
> You are correct. I missed the level of indent in line 3256. What
> initialization should be used for last? ?It clearly needs to be set to
> something.

http://bcm-v4.sipsolutions.net/802.11/PHY/N/CalTxIqlo does not say
anything about initial value. Guess it should be zero.

-- 
Rafa?

[PATCH] b43: Fix bogus compilation warning for phy_n

[Larry Finger]

On 05/19/2011 05:59 PM, Rafa? Mi?ecki wrote:
> W dniu 20 maja 2011 00:48 u?ytkownik Larry Finger
>> You are correct. I missed the level of indent in line 3256. What
>> initialization should be used for last?  It clearly needs to be set to
>> something.
>
> http://bcm-v4.sipsolutions.net/802.11/PHY/N/CalTxIqlo does not say
> anything about initial value. Guess it should be zero.

I checked the reference driver. It should be zero. The specs are updated.

Larry