Return-path: Received: from mail-oi0-f66.google.com ([209.85.218.66]:35341 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753803AbdDEQcM (ORCPT ); Wed, 5 Apr 2017 12:32:12 -0400 Received: by mail-oi0-f66.google.com with SMTP id d2so2888437oig.2 for ; Wed, 05 Apr 2017 09:32:12 -0700 (PDT) Subject: Re: [PATCH] staging: Add rtl8723bs sdio wifi driver To: Hans de Goede , Arend Van Spriel References: <20170329174751.13184-1-hdegoede@redhat.com> <4d1d3c02-eff6-0410-cee0-9360ec645e13@lwfinger.net> <049673a4-647b-a270-bb9c-15bb2ab49fc4@redhat.com> <560981e0-e1b0-7a06-80e4-14d677c7d64a@redhat.com> Cc: Bastien Nocera , Jes Sorensen , linux-wireless@vger.kernel.org From: Larry Finger Message-ID: <15871468-0086-2d66-328b-0699ef6f4b31@lwfinger.net> (sfid-20170405_183456_757725_4F8B1B9B) Date: Wed, 5 Apr 2017 11:32:10 -0500 MIME-Version: 1.0 In-Reply-To: <560981e0-e1b0-7a06-80e4-14d677c7d64a@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: 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