Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 1 Apr 2021 15:55:37 +0200
From:   Fabio Aiuto <fabioaiuto83@gmail.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     gregkh@linuxfoundation.org, joe@perches.com,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 08/49] staging: rtl8723bs: remove RT_TRACE logs in
 core/rtw_cmd.c
Message-ID: <20210401135536.GA1691@agape.jhs>
References: <cover.1617268327.git.fabioaiuto83@gmail.com>
 <7f51432d99459d79742639341f107115f0c224c5.1617268327.git.fabioaiuto83@gmail.com>
 <20210401095017.GR2065@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210401095017.GR2065@kadam>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Thu, Apr 01, 2021 at 12:50:18PM +0300, Dan Carpenter wrote:
> On Thu, Apr 01, 2021 at 11:20:38AM +0200, Fabio Aiuto wrote:
> > @@ -677,9 +663,8 @@ u8 rtw_createbss_cmd(struct adapter  *padapter)
> >  	u8 res = _SUCCESS;
> >  
> >  	if (pmlmepriv->assoc_ssid.SsidLength == 0)
> > -		RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, (" createbss for Any SSid:%s\n", pmlmepriv->assoc_ssid.Ssid));
> > +		;
> >  	else
> > -		RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, (" createbss for SSid:%s\n", pmlmepriv->assoc_ssid.Ssid));
> >  
> >  	pcmd = rtw_zmalloc(sizeof(struct cmd_obj));
> 
> This is a bug.  Smatch has a check for this which hopefully would have
> detected it (I haven't tested).
> 
> There are some more similar issues below as well.  So generally the rule
> is don't adjust the indenting if it's not related to your patch.  In
> some cases you have been fixing the indenting but it should be done in
> a separate patch.  But the other rule is that if your patch introduces
> a checkpatch warning then you need to fix it in the same patch.  In
> this block the whole if statement should be removed.  But also if you
> have something like:
> 
> 	if (foo) {
> 		RT_TRACE(blha blah blah);
> 		return;
> 	}
> 
> Then checkpatch will complain that the the curly braces are not
> required.  (Checkpatch might not complain for your patch but it will
> complain when we re-run it with the -f option over the whole file).  So
> you should update this to:
> 
> 	if (foo)
> 		return;
> 
> That's all considered part of deleting the RT_TRACE().  Also if there
> are empty curly braces then delete those in the same patch.
> 
> I have looked over patches 1-7 and those seem basically fine.  I'm not
> going to review any further into this patchset because you're going to
> have to redo them and I will be reviewing the v2 set later anyway.  So
> just look it over yourself and check for any similar issues.
> 
> regards,
> dan carpenter
> 

Hi Dan,

I have the following:

 	if (rtw_createbss_cmd(adapter) != _SUCCESS)
-		RT_TRACE(_module_rtl871x_mlme_c_, _drv_err_, ("Error =>rtw_createbss_cmd status FAIL\n"));
+	;

will I leave

	if (rtw_createbss_cmd(adapter) != _SUCCESS)
		;

or just

	rtw_createbss_cmd(adapter);

?

what's best from the static analysis point of view?

smatch and sparse says nothing about that.

Checkpatch too seems to ignore it, maybe the first one is good,
but I would like to be sure before sending another over 40 patches
long patchset.

thank you,

fabio