[RFC] Unify messaging in gadget functions

[RFC] Unify messaging in gadget functions

[Andrzej Pietrasiewicz]

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

Questions:

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

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, ....);

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).

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

Regards,

Andrzej

Re: [RFC] Unify messaging in gadget functions

[Greg KH]

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

Re: [RFC] Unify messaging in gadget functions

[Andrzej Pietrasiewicz]

Hi Greg,

W dniu 5.12.2022 o 13:20, Greg KH pisze:
> 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:

<snip>

>> Questions:
>>
>> 1) Should we make them use the messaging utils consitently?
> 
> Yes, converting to use the dev_*() calls is good to do.

Heh, I sent this RFC to prevent learning the hard way (by actually creating
incorrect patches), that we want to be consistent, but using dev_*() calls
rather than composite.h utils. That's ok.

Which brings an interesting question: should the ultimate goal be to remove the
messaging utils altogether from composite.h? It _seems_ their purpose is mainly
to wrap dereferencing a pointer two pointers away: &(d)->gadget->dev to make
invocations shorter. With the default of 100 columns in checkpatch nowadays it
is maybe a less important goal? Or maybe we should prevent such long
dereferencing by introducing helper variables just like below in
u_audio_start_capture()?

> 
>> 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?

Nothing? If making messaging consistent meant using the utils from composite.h,
then in this particular case we would unnecessarily go through & -> -> each
time, which is described in the following paragraph, which is avoided by using
the local variable "dev". Given that the preferred way to unify messaging
is using dev_*() calls, my question 2 becomes irrelevant.

Andrzej

> 
>> 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!

Re: [RFC] Unify messaging in gadget functions

[Greg KH]

On Mon, Dec 05, 2022 at 03:13:46PM +0100, Andrzej Pietrasiewicz wrote:
> Hi Greg,
> 
> W dniu 5.12.2022 o 13:20, Greg KH pisze:
> > 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:
> 
> <snip>
> 
> > > Questions:
> > > 
> > > 1) Should we make them use the messaging utils consitently?
> > 
> > Yes, converting to use the dev_*() calls is good to do.
> 
> Heh, I sent this RFC to prevent learning the hard way (by actually creating
> incorrect patches), that we want to be consistent, but using dev_*() calls
> rather than composite.h utils. That's ok.
> 
> Which brings an interesting question: should the ultimate goal be to remove the
> messaging utils altogether from composite.h? It _seems_ their purpose is mainly
> to wrap dereferencing a pointer two pointers away: &(d)->gadget->dev to make
> invocations shorter. With the default of 100 columns in checkpatch nowadays it
> is maybe a less important goal? Or maybe we should prevent such long
> dereferencing by introducing helper variables just like below in
> u_audio_start_capture()?

Yes, that should be the ultimate goal.  We did that in the USB drivers a
decade or so ago, but no one spent the time to do the same for the USB
gadget code.

thanks,

greg k-h