Date:   Fri, 14 Jan 2022 00:42:50 +0000
From:   Phillip Potter <phil@philpotter.co.uk>
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        gregkh@linuxfoundation.org
Cc:     Larry.Finger@lwfinger.net, straube.linux@gmail.com,
        martin@kaiser.cx, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/7] staging: r8188eu: convert DBG_88E calls in
 core/rtw_iol.c
Message-ID: <YeDHCtZ+hy0Q+ZXV@equinox>
References: <20220109215427.887-1-phil@philpotter.co.uk>
 <20220109215427.887-4-phil@philpotter.co.uk>
 <20220110100843.GC1951@kadam>
 <YdydYlpVzHoLy2Wh@equinox>
 <20220111050644.GD1951@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220111050644.GD1951@kadam>
Precedence: bulk

On Tue, Jan 11, 2022 at 08:06:44AM +0300, Dan Carpenter wrote:
> On Mon, Jan 10, 2022 at 08:56:02PM +0000, Phillip Potter wrote:
> > On Mon, Jan 10, 2022 at 01:08:43PM +0300, Dan Carpenter wrote:
> > > On Sun, Jan 09, 2022 at 09:54:23PM +0000, Phillip Potter wrote:
> > > > Convert the DBG_88E macro calls in core/rtw_iol.c to use pr_debug
> > > > or netdev_dbg appropriately, as their information may be useful to
> > > > observers, and this gets the driver closer to the point of being
> > > > able to remove DBG_88E itself.
> > > > 
> > > > Some calls are at points in the call chain where use of dev_dbg or
> > > > netdev_dbg isn't possible due to lack of device pointer, so plain
> > > > pr_debug is appropriate here.
> > > > 
> > > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> > > > ---
> > > >  drivers/staging/r8188eu/core/rtw_iol.c | 10 ++++++----
> > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/r8188eu/core/rtw_iol.c b/drivers/staging/r8188eu/core/rtw_iol.c
> > > > index 7e78b47c1284..923da2a9f6ae 100644
> > > > --- a/drivers/staging/r8188eu/core/rtw_iol.c
> > > > +++ b/drivers/staging/r8188eu/core/rtw_iol.c
> > > > @@ -12,13 +12,15 @@ struct xmit_frame	*rtw_IOL_accquire_xmit_frame(struct adapter  *adapter)
> > > >  
> > > >  	xmit_frame = rtw_alloc_xmitframe(pxmitpriv);
> > > >  	if (!xmit_frame) {
> > > > -		DBG_88E("%s rtw_alloc_xmitframe return null\n", __func__);
> > > > +		netdev_dbg(adapter->pnetdev,
> > > > +			   "rtw_alloc_xmitframe return null\n");
> > > 
> > > You're going to have to send this anyway because of the compile issue.
> > > 
> > > I feel like you are not being aggressive enough in the debug messages
> > > that you delete.  For example, this one should definitely be deleted.
> > > Don't print an error message for alloc failures.
> > > 
> > > It would be easier to Ack a mass delete of these messages.
> > > 
> > > regards,
> > > dan carpenter
> > > 
> > 
> > Dear Dan,
> > 
> > Thank you for your feedback. I already sent a V2 series to fix the empty case
> > label I left in core/rtw_mlme_ext.c, sounds like a V3 is needed though 
> > based on this feedback - admittedly I have tried to be conservative and
> > basically only removed commented DBG_88E calls or calls which just print the
> > function name/line number so far.
> 
> Yeah.  I saw v3.  It's fine.  I'm not really trying to nak your patches.
> 
> > 
> > I get what you're saying about deleting them all just being easier,
> > but I've already converted several in previous series that have
> > made it in. It would make sense to delete these converted calls as well
> > if going for the total deletion approach. Also, I do worry some of the
> > info could be useful. I'd appreciate your thoughts on this.
> > 
> > I am happy to delete it all by all means, just want to make sure majority
> > would be happy with that approach, as opposed to a refinement of this
> > approach and being more judicious with deletion of more DBG_88E calls.
> 
> In the original code DBG_88E was kind of an error level severity message
> not a debug level severity.  Of course, you had to use a module option
> to turn on any output at all so it's hard to judge how that works in
> real life.  By making them debug level severity, you've basically
> deleted them already...  Don't be a hoarder.
> 
> Once you change it to dev_dbg() then it becomes more difficult
> emotionally to do a mass delete.
> 
> There is a real value to just deleting stuff.
> 
> regards,
> dan carpenter

Thanks Dan,

OK - based on what you've said, I will make a series to just remove all
DBG_88E calls, including any I've already converted that have been
merged, and also the aliased versions that are defined via additional
preprocessor directives.

Greg - please could you disregard this series and the previous smaller
one I sent? The new series will supersede them both when it's ready.
Many thanks.

Regards,
Phil