by Phillip Potter

[permalink] [raw]

Subject: Re: [PATCH 3/7] staging: r8188eu: convert DBG_88E calls in core/rtw_iol.c

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 <[email protected]>
> > > > ---
> > > > 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