On 7/9/23 01:27, Geert Uytterhoeven wrote:
> Hi Randy,
>
> On Sun, Jul 9, 2023 at 4:46 AM Randy Dunlap <rdunlap@xxxxxxxxxxxxx> wrote:
>> On 6/26/23 01:24, Geert Uytterhoeven wrote:
>>> On Mon, 26 Jun 2023, Geert Uytterhoeven wrote:
>>>> JFYI, when comparing v6.4[1] to v6.4-rc7[3], the summaries are:
>>>> - build errors: +1/-0
>>>
>>> + /kisskb/src/drivers/net/wireless/cisco/airo.c: error: 'status_rid.currentXmitRate' is used uninitialized [-Werror=uninitialized]: => 6163:45
>>
>> I cannot reproduce this build error. (I don't doubt the problem, just having a problem
>> making it happen for me.)
>> I have tried it with gcc-11.1.0, gcc-11.3.0, and gcc-13.1.0.
>
> Which is similar to kisskb, using sh4-linux-gcc (GCC) 11.3.0...
>
>> I'm surprised that this is the only instance that gcc found/warned about.
>
> Indeed.
>
>>
>> I have a patch for this one instance, but there are 40+ instances of
>> readStatusRid()
>> readConfigRid()
>> readSsidRid()
>> readStatsRid()
>> readCapabilityRid()
>> that don't check the return status of the function calls.
>>
>> I suppose that a patch can quieten the compiler error/warning, but given
>> the 40+ other problems, it won't make the driver any noticeably better IMO.
>
> Indeed...
>
>>> sh4-gcc11/sh-allmodconfig
>>> seen before
>>>
>>> This is actually a real issue, and it's been here since basically forever.
>>>
>>> drivers/net/wireless/cisco/airo.c:
>>>
>>> static int airo_get_rate(struct net_device *dev,
>>> struct iw_request_info *info,
>>> union iwreq_data *wrqu,
>>> char *extra)
>>> {
>>> struct iw_param *vwrq = &wrqu->bitrate;
>>> struct airo_info *local = dev->ml_priv;
>>> StatusRid status_rid; /* Card status info */
>>>
>>> readStatusRid(local, &status_rid, 1);
>>>
>>> ==> vwrq->value = le16_to_cpu(status_rid.currentXmitRate) * 500000;
>>> ...
>>> }
>>>
>>> static int readStatusRid(struct airo_info *ai, StatusRid *statr, int lock)
>>> {
>>> return PC4500_readrid(ai, RID_STATUS, statr, sizeof(*statr), lock);
>>> }
>>>
>>> static int PC4500_readrid(struct airo_info *ai, u16 rid, void *pBuf, int len, int lock)
>>> {
>>> u16 status;
>>> int rc = SUCCESS;
>>>
>>> if (lock) {
>>> if (down_interruptible(&ai->sem))
>>> return ERROR;
>>>
>>> pBuf output buffer contents not initialized.
>>>
>>> }
>>> ...
>>> }
>>>
>>>
>>>> [1] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/6995e2de6891c724bfeb2db33d7b87775f913ad1/ (all 160 configs)
>>>> [3] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/45a3e24f65e90a047bef86f927ebdc4c710edaa1/ (all 160 configs)
>>
>> I appreciate the synopsis. Here's a patch. WDYT?
>> ---
>> From: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
>> Subject: [PATCH] wifi: airo: avoid uninitialized warning in airo_get_rate()
>>
>> Quieten a gcc (11.3.0) build error or warning by checking the function
>> call status and returning -EIO if the function call failed.
>> This is similar to what several other wireless drivers do for the
>> SIOCGIWRATE ioctl call.
>>
>> drivers/net/wireless/cisco/airo.c: error: 'status_rid.currentXmitRate' is used uninitialized [-Werror=uninitialized]
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
>> Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
>> Link: lore.kernel.org/r/39abf2c7-24a-f167-91da-ed4c5435d1c4@xxxxxxxxxxxxxx
>> Cc: Kalle Valo <kvalo@xxxxxxxxxx>
>> Cc: linux-wireless@xxxxxxxxxxxxxxx
>> ---
>> drivers/net/wireless/cisco/airo.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff -- a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c
>> --- a/drivers/net/wireless/cisco/airo.c
>> +++ b/drivers/net/wireless/cisco/airo.c
>> @@ -6157,8 +6157,11 @@ static int airo_get_rate(struct net_devi
>> struct iw_param *vwrq = &wrqu->bitrate;
>> struct airo_info *local = dev->ml_priv;
>> StatusRid status_rid; /* Card status info */
>> + int ret;
>>
>> - readStatusRid(local, &status_rid, 1);
>> + ret = readStatusRid(local, &status_rid, 1);
>> + if (ret)
>> + return -EIO;
>
> That's about the best we can do without further surgery.
> E.g. PC4500_readrid() should return a proper error code instead of
> just ERROR/SUCCESS.
> The case gcc complains about is when lock = 1 and the call to
> down_interruptible() fails, for which -EBUSY would be appropriate.
Yes, I saw that return value as one of the options.
I'll change it to that and submit it.
Thanks.
--
~Randy
