Re: [RFC] Unify messaging in gadget functions

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Dec 05, 2022 at 01:14:21PM +0100, Andrzej Pietrasiewicz wrote:
> Hi All,
> 
> include/linux/usb/composite.h contains:
> 
> /* messaging utils */
> #define DBG(d, fmt, args...) \
> 	dev_dbg(&(d)->gadget->dev , fmt , ## args)
> #define VDBG(d, fmt, args...) \
> 	dev_vdbg(&(d)->gadget->dev , fmt , ## args)
> #define ERROR(d, fmt, args...) \
> 	dev_err(&(d)->gadget->dev , fmt , ## args)
> #define WARNING(d, fmt, args...) \
> 	dev_warn(&(d)->gadget->dev , fmt , ## args)
> #define INFO(d, fmt, args...) \
> 	dev_info(&(d)->gadget->dev , fmt , ## args)
> 
> Gadget functions do use these, but not consistently:
> 
> => DBG() vs dev_dbg():
> $ git grep DBG\( drivers/usb/gadget/function | grep -v VDBG | grep -v LDBG | wc
>     138     871   11619
> 
> $ git grep dev_dbg\( drivers/usb/gadget/function | grep -v "##args" | wc
>      33     151    2831
> 
> => VDBG() vs dev_vdbg():
>  git grep VDBG\( drivers/usb/gadget/function | grep -v "#define" | wc
>      49     269    3954
> 
> $ git grep dev_vdbg\( drivers/usb/gadget/function | wc
>       2       4     135
> 
> => ERROR() vs dev_err():
> $ git grep ERROR\( drivers/usb/gadget/function | grep -v LERROR | wc
>      72     508    6560
> 
> $ git grep dev_err\( drivers/usb/gadget/function | grep -v "##args" | wc
>      65     309    5527
> 
> => WARNING() vs dev_warn():
> $ git grep WARNING\( drivers/usb/gadget/function | wc
>       4      28     383
> 
> $ git grep dev_warn\( drivers/usb/gadget/function | grep -v "##args" | wc
>       3       6     169
> 
> => INFO() vs dev_info():
> $ git grep INFO\( drivers/usb/gadget/function  | grep -v LINFO | grep -v
> u_ether | wc
>      14      64    1167
> 
> $ git grep dev_info\( drivers/usb/gadget/function | grep -v "##args" | wc
>       0       0       0

Drivers that work properly should be quiet, so no INFO() usage should
probably be needed anyway.

> 
> Questions:
> 
> 1) Should we make them use the messaging utils consitently?

Yes, converting to use the dev_*() calls is good to do.

> 2) How consistent should we become, given that some functions in the relevant
> files, for example u_audio_start_capture(), sometimes (but not always) have:
> 
> 	struct usb_gadget *gadget = audio_dev->gadget;
> 	struct device *dev = &gadget->dev;
> 
> and then they use dev_dbg(dev, ....);

dev_dbg() is fine, what's worng with that?

> If we were to use DBG(audio_dev, ....); instead, then we effectively get
> dev_dbg(&audio_dev->gadget->dev, ....); after macro expansion, which means two
> pointer dereferences and taking an address of the result (I'm wondering how
> smart the compiler can get if such a pattern repeats several times in a
> function).

The compiler can get very smart, but this isn't really an issue overall
as USB drivers are very slow due to slow hardware.

> 3) Maybe the amount of various messages is too large in the first place
> and should be reduced before any unifications?

Possibly, many might be able to be removed, look and see!

thanks,

greg k-h