On Fri, Feb 07, 2025 at 05:45:33PM +0800, Jeremy Kerr wrote: > Hi Greg, > > > > > > + dev_err(&mctp_usb->usbdev->dev, "%s: urb status: %d\n", > > > > > + __func__, status); > > > > > > > > This could flood the logs, are you sure you need it at dev_err() > > > > level? > > > > > > > > And __func__ is redundant, it's present in dev_*() calls already. > > > > > > am I missing something then? > > > > > > [ 146.130170] usb 2-1: short packet (hdr) 6 > > > > > > emitted from: > > > > > > dev_dbg(&mctp_usb->usbdev->dev, > > > "short packet (hdr) %d\n", > > > hdr->len); > > > > > > Seems like we get the driver name, but not the function. > > > > > > I'm happy to remove the __func__ output either way, but I will also > > > make the logs a little more descriptive for context, if we don't have > > > func data. > > > > Please read Documentation/admin-guide/dynamic-debug-howto.rst, it shows > > how to get the function information from the dev_dbg() lines at runtime. > > > > In short: > > $ alias ddcmd='echo $* > /proc/dynamic_debug/control' > > # add function to all enabled messages > > $ ddcmd '+f' > > Your original comment was on the dev_err() call though (sorry, I've > complicated the discussion by using a dev_dbg() example). Sorry, I got confused here too, I saw it on dev_dbg() calls in my review. > Looks like only dev_dbg (and not _err/_warn/etc) has provision for > __func__, is that right? Yes. > I've since removed the __func__ references anyway, and replaced with > better context on the messages, but keen to make sure I have the correct > understanding in general. That sounds better, avoiding __func__ wherever possible is usually a good idea. thanks, gre gk-h
