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
