Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Larry Finger <Larry.Finger@lwfinger.net>]

On 04/05/2017 04:36 AM, Hans de Goede wrote:
> Hi,
>
> On 05-04-17 01:41, Larry Finger wrote:
>> On 04/04/2017 04:53 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 04/04/2017 11:38 PM, Arend Van Spriel wrote:
>>>>
>>>>
>>>> On 4-4-2017 20:53, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 04/04/2017 08:31 PM, Larry Finger wrote:
>>>>>> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>>>>>>> The rtl8723bs is found on quite a few systems used by Linux users,
>>>>>>> such as on Atom systems (Intel Computestick and various other
>>>>>>> Atom based devices) and on many (budget) ARM boards such as
>>>>>>> the CHIP.
>>>>>>>
>>>>>>> The plan moving forward with this is for the new clean,
>>>>>>> written from scratch, rtl8xxxu driver to eventually gain
>>>>>>> support for sdio devices. But there is no clear timeline
>>>>>>> for that, so lets add this driver included in staging for now.
>>>>>>
>>>>>> Hans,
>>>>>>
>>>>>> I started looking at the Smatch errors. This one may be the result of
>>>>>> a serious problem:
>>>>>>
>>>>>>   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
>>>>>> drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info()
>>>>>> error: we previously assumed 'phead' could be null (see line 453)
>>>>>> drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info()
>>>>>> warn: variable dereferenced before check 'phead' (see line 454)
>>>>>>
>>>>>> A snippet of the code in question is as follows:
>>>>>>
>>>>>>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>>>>>>         phead = get_list_head(queue);
>>>>>>         plist = phead ? get_next(phead) : NULL;
>>>>>>         plist = get_next(phead);
>>>>>>         if ((!phead) || (!plist)) {
>>>>>>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>>>>>>                 return 0;
>>>>>>         }
>>>>>>
>>>>>> This code comes directly from the hadess repo, but I am suspicious of
>>>>>> the double call to get_next(phead). I cannot imagine any valid reason
>>>>>> to skip every other entry on that list.
>>>>>
>>>>> If you look closer and simplify the first getnext line what is written is:
>>>>>
>>>>>          plist = get_next(phead);
>>>>>          plist = get_next(phead);
>>>>>
>>>>> Which indeed looks like it could use improvement, but I don't
>>>>> think it is seriously broken.
>>>>
>>>> So get_list_head(queue) can never return NULL?
>>>
>>> I don't know.
>>>
>>>> Otherwise I don't know
>>>> how close I need to get for a simplified look ;-)
>>>
>>> I simplified things so as to make clear that Larry's worry was
>>> not really a problem, I do agree the smatch complaint is valid.
>>
>> I think the following fixes the problem:
>>
>> diff --git a/drivers/staging/rtl8723bs/core/rtw_debug.c
>> b/drivers/staging/rtl8723bs/core/rtw_debug.c
>> index d34747b29309..51cef55d3f76 100644
>> --- a/drivers/staging/rtl8723bs/core/rtw_debug.c
>> +++ b/drivers/staging/rtl8723bs/core/rtw_debug.c
>> @@ -451,7 +451,6 @@ int proc_get_survey_info(struct seq_file *m, void *v)
>>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>>         phead = get_list_head(queue);
>>         plist = phead ? get_next(phead) : NULL;
>> -       plist = get_next(phead);
>>         if ((!phead) || (!plist)) {
>>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>>                 return 0;
>>
>> Pointer plist is set in the 3rd line, thus the second set of it can be removed.
>
> Agreed, when I've some time I plan to do a follow-up patch
> to my initial submission fixing all the Smatch errors. But feel
> free to beat me to it.
>
> Greg, if I understood you correctly you plan to merge my initial
> submission as is and then we can fix this (and other things) with
> follow up patches, right ?

Hans,

I took GregKH out of the Cc list when I started the discussion of the Smatch 
errors/warnings, and he probably has not seen the recent E-mails in this thread. 
It it was my understanding that he planned to apply your submission in the form 
you sent.

I have several patches nearly ready to fix most of the Smatch warnings and 
errors. I will send them as soon as the original submission is applied.

Larry