Re: [PATCH] usb: musb: add printf attribute to log function

[Bin Liu]

Hi,

On Tue, Dec 22, 2020 at 01:52:48AM -0800, Joe Perches wrote:
> On Tue, 2020-12-22 at 09:52 +0100, Greg KH wrote:
> > On Mon, Dec 21, 2020 at 08:25:47AM -0800, trix@xxxxxxxxxx wrote:
> > > From: Tom Rix <trix@xxxxxxxxxx>
> > > 
> > > Attributing the function allows the compiler to more thoroughly
> > > check the use of the function with -Wformat and similar flags.
> > > 
> > > Signed-off-by: Tom Rix <trix@xxxxxxxxxx>
> > > ---
> > >  drivers/usb/musb/musb_debug.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/usb/musb/musb_debug.h b/drivers/usb/musb/musb_debug.h
> > > index e5b3506c7b3f..dfc0d02695fa 100644
> > > --- a/drivers/usb/musb/musb_debug.h
> > > +++ b/drivers/usb/musb/musb_debug.h
> > > @@ -17,6 +17,7 @@
> > >  #define INFO(fmt, args...) yprintk(KERN_INFO, fmt, ## args)
> > >  #define ERR(fmt, args...) yprintk(KERN_ERR, fmt, ## args)

These should be converted to dev_info or dev_err. I believe the work was only
done on those actively used platform drivers.

Further cleanup patches are welcomed.

> > >  
> > > 
> > > +__printf(2, 3)
> > >  void musb_dbg(struct musb *musb, const char *fmt, ...);
> > 
> > While I understand the need for this, did this find any problems?
> > If not, then it's not worth adding,
> 
> I have to disagree with that Greg.  While the driver isn't in active
> development, a trivial mod to make it less likely a defect is introduced
> by any additional code is still a useful addition.
> 
> > the driver-specific debugging macros
> > should be removed entirely and just use dev_dbg() and friends instead.
> 
> Read the suggested change I posted in reply.
> 
> btw: the musb_dbg function is actually a trace function and not a
> dmesg/logging mechanism.

Yes, musb_dbg() generates ftrace logs instead.

> 
> drivers/usb/musb/musb_trace.c:void musb_dbg(struct musb *musb, const char *fmt, ...)
> drivers/usb/musb/musb_trace.c-{
> drivers/usb/musb/musb_trace.c-  struct va_format vaf;
> drivers/usb/musb/musb_trace.c-  va_list args;
> drivers/usb/musb/musb_trace.c-
> drivers/usb/musb/musb_trace.c-  va_start(args, fmt);
> drivers/usb/musb/musb_trace.c-  vaf.fmt = fmt;
> drivers/usb/musb/musb_trace.c-  vaf.va = &args;
> drivers/usb/musb/musb_trace.c-
> drivers/usb/musb/musb_trace.c-  trace_musb_log(musb, &vaf);
> drivers/usb/musb/musb_trace.c-
> drivers/usb/musb/musb_trace.c-  va_end(args);
> drivers/usb/musb/musb_trace.c-}
> 
> drivers/usb/musb/musb_trace.h:TRACE_EVENT(musb_log,
> drivers/usb/musb/musb_trace.h-  TP_PROTO(struct musb *musb, struct va_format *vaf),
> drivers/usb/musb/musb_trace.h-  TP_ARGS(musb, vaf),
> drivers/usb/musb/musb_trace.h-  TP_STRUCT__entry(
> drivers/usb/musb/musb_trace.h-          __string(name, dev_name(musb->controller))
> drivers/usb/musb/musb_trace.h-          __dynamic_array(char, msg, MUSB_MSG_MAX)
> drivers/usb/musb/musb_trace.h-  ),
> drivers/usb/musb/musb_trace.h-  TP_fast_assign(
> drivers/usb/musb/musb_trace.h-          __assign_str(name, dev_name(musb->controller));
> drivers/usb/musb/musb_trace.h-          vsnprintf(__get_str(msg), MUSB_MSG_MAX, vaf->fmt, *vaf->va);
> drivers/usb/musb/musb_trace.h-  ),
> drivers/usb/musb/musb_trace.h-  TP_printk("%s: %s", __get_str(name), __get_str(msg))
> drivers/usb/musb/musb_trace.h-);
> 
> Is that trace mechanism useful though?  I think it's somewhat odd.

The intention was to convert runtime debug log to ftrace for efficiency.
Some of the original printk() are converted to trace points. Other
unclassified prints are converted to musb_dbg() for further clean up.

-Bin.