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.
Larry
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.
Regards,
Hans
Hi,
On 06-04-17 11:04, Bastien Nocera wrote:
> On Thu, 2017-04-06 at 08:55 +0200, Hans de Goede wrote:
>>
> <snip>
>> Good thank you. So what is the plan with the github version ?
>>
>> Note that my submission contains a few small fixes on top of
>> the github version, for which I intended to submit a pull-req
>> but I've not gotten around to that yet, I've done so now:
>>
>> https://github.com/hadess/rtl8723bs/pull/125
>>
>> But do we want to keep maintaining the github version (for a while
>> at least) I wonder, as that does mean double work?
>
> My plan is to remove everything and point to the upstream tree as soon
> as the support has landed in Linus' tree.
>
> While there is some use in keeping it around for older kernels, we keep
> getting asked to support even older kernels than reasonable, or have
> users complaining about non-working machines once they use the driver,
> which simply uncovers kernel bugs that were fixed upstream.
>
> I don't think it's a good use of our time to carry on supporting this
> out-of-tree. I'll however make sure to try and document the migration
> in a way that's helpful to users.
Ok, that works for me.
Regards,
Hans
On Thu, 2017-04-06 at 08:55 +0200, Hans de Goede wrote:
>
<snip>
> Good thank you. So what is the plan with the github version ?
>
> Note that my submission contains a few small fixes on top of
> the github version, for which I intended to submit a pull-req
> but I've not gotten around to that yet, I've done so now:
>
> https://github.com/hadess/rtl8723bs/pull/125
>
> But do we want to keep maintaining the github version (for a while
> at least) I wonder, as that does mean double work?
My plan is to remove everything and point to the upstream tree as soon
as the support has landed in Linus' tree.
While there is some use in keeping it around for older kernels, we keep
getting asked to support even older kernels than reasonable, or have
users complaining about non-working machines once they use the driver,
which simply uncovers kernel bugs that were fixed upstream.
I don't think it's a good use of our time to carry on supporting this
out-of-tree. I'll however make sure to try and document the migration
in a way that's helpful to users.
Cheers
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? Otherwise I don't know
how close I need to get for a simplified look ;-)
Gr. AvS
> Regards,
>
> Hans
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.
Regards,
Hans
Hi,
On 05-04-17 18:32, Larry Finger wrote:
> 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.
Ah I missed that (clearly I did).
> It it was my understanding that he planned to apply your submission in the form you sent.
That is my understanding too.
> 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.
Good thank you. So what is the plan with the github version ?
Note that my submission contains a few small fixes on top of
the github version, for which I intended to submit a pull-req
but I've not gotten around to that yet, I've done so now:
https://github.com/hadess/rtl8723bs/pull/125
But do we want to keep maintaining the github version (for a while
at least) I wonder, as that does mean double work?
Regards,
Hans
On 04/06/2017 02:32 PM, Larry Finger wrote:
> On 04/06/2017 04:49 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 06-04-17 11:04, Bastien Nocera wrote:
>>> On Thu, 2017-04-06 at 08:55 +0200, Hans de Goede wrote:
>>>>
>>> <snip>
>>>> Good thank you. So what is the plan with the github version ?
>>>>
>>>> Note that my submission contains a few small fixes on top of
>>>> the github version, for which I intended to submit a pull-req
>>>> but I've not gotten around to that yet, I've done so now:
>>>>
>>>> https://github.com/hadess/rtl8723bs/pull/125
>>>>
>>>> But do we want to keep maintaining the github version (for a while
>>>> at least) I wonder, as that does mean double work?
>>>
>>> My plan is to remove everything and point to the upstream tree as soon
>>> as the support has landed in Linus' tree.
>>>
>>> While there is some use in keeping it around for older kernels, we keep
>>> getting asked to support even older kernels than reasonable, or have
>>> users complaining about non-working machines once they use the driver,
>>> which simply uncovers kernel bugs that were fixed upstream.
>>>
>>> I don't think it's a good use of our time to carry on supporting this
>>> out-of-tree. I'll however make sure to try and document the migration
>>> in a way that's helpful to users.
>>
>> Ok, that works for me.
>
> I agree. I have enough to do.
You could simply break the build, and have it spit out a warning saying
the git repo is kept there to track the old source.
It might be useful to have a repository of the original code to go look at.
Cheers,
Jes
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 ?
Regards,
Hans
On 04/06/2017 04:49 AM, Hans de Goede wrote:
> Hi,
>
> On 06-04-17 11:04, Bastien Nocera wrote:
>> On Thu, 2017-04-06 at 08:55 +0200, Hans de Goede wrote:
>>>
>> <snip>
>>> Good thank you. So what is the plan with the github version ?
>>>
>>> Note that my submission contains a few small fixes on top of
>>> the github version, for which I intended to submit a pull-req
>>> but I've not gotten around to that yet, I've done so now:
>>>
>>> https://github.com/hadess/rtl8723bs/pull/125
>>>
>>> But do we want to keep maintaining the github version (for a while
>>> at least) I wonder, as that does mean double work?
>>
>> My plan is to remove everything and point to the upstream tree as soon
>> as the support has landed in Linus' tree.
>>
>> While there is some use in keeping it around for older kernels, we keep
>> getting asked to support even older kernels than reasonable, or have
>> users complaining about non-working machines once they use the driver,
>> which simply uncovers kernel bugs that were fixed upstream.
>>
>> I don't think it's a good use of our time to carry on supporting this
>> out-of-tree. I'll however make sure to try and document the migration
>> in a way that's helpful to users.
>
> Ok, that works for me.
I agree. I have enough to do.
Larry
On 04/04/2017 01:53 PM, 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.
My problem was that I thought that get_next() took an entry off the list. Now
that I actually looked, I see that it just gets the next pointer.
Sorry for the noise,
Larry
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
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.
Larry