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 17:32:36 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Fabio Aiuto <fabioaiuto83@gmail.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: <20210401143235.GV2065@kadam>
References: <cover.1617268327.git.fabioaiuto83@gmail.com>
 <7f51432d99459d79742639341f107115f0c224c5.1617268327.git.fabioaiuto83@gmail.com>
 <20210401095017.GR2065@kadam>
 <20210401135536.GA1691@agape.jhs>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210401135536.GA1691@agape.jhs>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Thu, Apr 01, 2021 at 03:55:37PM +0200, Fabio Aiuto wrote:
> 
> 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.

rtw_createbss_cmd() can only fail if this allocation fails:

	pcmd = rtw_zmalloc(sizeof(struct cmd_obj));

In current kernels, that size of small allocation will never fail.  But
we alway write code as if every allocation can fail.

Normally when an allocation fails then we want to return -ENOMEM and
clean up.  But this code is an event handler for firmware events and
there isn't any real clean up to do.  Since there is nothing we can do
then this is basically working and fine.

How I would write this is:

			ret = rtw_createbss_cmd(adapter);
			if (ret != _SUCCESS)
				goto unlock;
		}
	}
unlock:
	spin_unlock_bh(&pmlmepriv->lock);
}

That doesn't change how the code works but it signals to the the reader
what your intention is.  If we just remove the error handling then it's
ambiguous.

			rtw_createbss_cmd(adapter);
		}
	}
	<-- Futurue programmer decides to add code here then figuring
            that rtw_createbss_cmd() can fail is a problem.

	spin_unlock_bh(&pmlmepriv->lock);
}

But for something like this which is maybe more subtle than just a
straight delete of lines of code, then consider pulling it out into its
own separate patch.  That makes it easier to review.  Put all the stuff
that I said in the commit message:

---
[PATCH] tidy up some error handling

The RT_TRACE() output is not useful so we want to delete it.  In this
case there is no cleanup for rtw_createbss_cmd() required or even
possible.  I've deleted the RT_TRACE() output and added a goto unlock
to show that we can't continue if rtw_createbss_cmd() fails.

---

> 
> 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.

Don't send 40 patches.  Just send 10 at a time until you get a better
feel for which ones are going to get applied or not. :P  It's not
arbitrary, and I'm definitely not trying to NAK your patches.  Once you
learn the rules I hope that it's predictable and straight forward.

regards,
dan carpenter