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!
