Re: [PATCH 3/5] staging: r8188eu: (trivial) remove a duplicate debug print

From: Phillip Potter <phil@philpotter.co.uk>
To: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc: Martin Kaiser <martin@kaiser.cx>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Larry Finger <Larry.Finger@lwfinger.net>,
	Michael Straube <straube.linux@gmail.com>,
	linux-staging@lists.linux.dev,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/5] staging: r8188eu: (trivial) remove a duplicate debug print
Date: Sat, 14 Aug 2021 17:54:40 +0100	[thread overview]
Message-ID: <CAA=Fs0kyzRjR1b_QfseyKE4mAp4W-Bxa97esf5QDoUFiOhA-zQ@mail.gmail.com> (raw)
In-Reply-To: <2555683.U4YhqVPOqN@localhost.localdomain>

On Fri, 13 Aug 2021 at 13:42, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> On Friday, August 13, 2021 12:05:36 PM CEST Martin Kaiser wrote:
> > Hi Dan and Phil,
> >
> > Thus wrote Dan Carpenter (dan.carpenter@oracle.com):
> > > Please think of the subject and the commit message as two different
> > > things.  Often it's people reviewing on email will only read one or the
> >
> > > other.  In other words just restate the subject:
> > OK, I'll keep that in mind for further patches.
> >
> > > > Dear Martin,
> > > >
> > > > Just my personal opinion, but I'd be inclined to strip out all DBG_88E
> > > > calls totally. If there are necessary functions being called such as
> > > > device_may_wakeup() we can always just keep this part and remove the
> > > > macro call (not checked this function out myself yet). Thanks.
> >
> > I'd agree with you, Phil. Most DBG_88E prints don't say anything useful.
> >
> > This comment from Greg made me drop the DBG_88E removal for now
> >
> > https://lore.kernel.org/linux-staging/20210803201511.29000-1-martin@kaiser.cx/T/#m05d82a
> > 0ca8ed36180ebdc987114b4d892445c52d
> >
> Hi Martin,
>
> I think you misunderstood what Greg was trying to convey with the above-
> mentioned message.
>
> Well, he doesn't like to feed developers with little spoons :-)
>
> I'm pretty sure that, by "Why not use the proper debugging calls instead of
> just deleting them?", he meant you should research, understand, and use the
> proper APIs for printing debug messages.
>
> Please check out pr_debug(), dev_dbg(), netdev_dbg(). Use them appropriately,
> according to the subsystem you're working in and to the different types of
> arguments they take.
>
> Thanks,
>
> Fabio
> >
> > A compromise would be to remove only those DBG_88E prints which are
> > really not helpful.
> >
> > Best regards,
> > Martin
>
>
>
>

The problem I see is that this driver is so littered with unnecessary
macro calls, how do we decide which ones to keep? In my mind, the
better option is to remove them all and then come up with some new
ones in the vein of netdev_dbg() and friends. I could be wrong of
course :-) I tried going down the route of keeping/converting some to
proper calls such as netdev_dbg() and the issue is a lot of the calls
don't have an obvious value anyway.

Regards,
Phil