| Message ID | 20221122141603.70242-1-andrzej.p@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: function: Simplify error messaging in printer open/close | expand |
On Tue, Nov 22, 2022 at 03:16:03PM +0100, Andrzej Pietrasiewicz wrote: > Don't issue any messages if printer_open() is successful. > Also don't issue them if unsuccessful - the error code is propagated > to the calling layers and should be acted on appropriately there. Just as > it is with the -ENODEV case. > > For those who really want this message leave an option to compile-in > with composite framework's VDBG() by uncommenting #define VERBOSE_DEBUG. > > While at it, visually detach the "return ret;" statement. > > Use __func__ instead of explicitly hardcoding the function name. This, in > turn makes checkpatch issue this for the message in printer_close(): > > WARNING: Unnecessary ftrace-like logging - prefer using ftrace > 54: FILE: drivers/usb/gadget/function/f_printer.c:387: > + VDBG(dev, "%s\n", __func__); > > which lets us eliminate the debug message from printer_close() altogether. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > Attention > > This patch depends on a recent patch from Dan Carpenter: > > usb: gadget: function: use after free in printer_close() > > drivers/usb/gadget/function/f_printer.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c > index 01e842e1ba2f..d686c7be4fb5 100644 > --- a/drivers/usb/gadget/function/f_printer.c > +++ b/drivers/usb/gadget/function/f_printer.c > @@ -11,6 +11,8 @@ > * Copyright (C) 2006 Craig W. Nadler > */ > > +/* #define VERBOSE_DEBUG */ > + > #include <linux/module.h> > #include <linux/kernel.h> > #include <linux/delay.h> > @@ -364,7 +366,8 @@ printer_open(struct inode *inode, struct file *fd) > spin_unlock_irqrestore(&dev->lock, flags); > > kref_get(&dev->kref); > - DBG(dev, "printer_open returned %x\n", ret); > + VDBG(dev, "%s returned %x\n", __func__, ret); This is what ftrace is for, please just delete this line entirely. thanks, greg k-h
diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c index 01e842e1ba2f..d686c7be4fb5 100644 --- a/drivers/usb/gadget/function/f_printer.c +++ b/drivers/usb/gadget/function/f_printer.c @@ -11,6 +11,8 @@ * Copyright (C) 2006 Craig W. Nadler */ +/* #define VERBOSE_DEBUG */ + #include <linux/module.h> #include <linux/kernel.h> #include <linux/delay.h> @@ -364,7 +366,8 @@ printer_open(struct inode *inode, struct file *fd) spin_unlock_irqrestore(&dev->lock, flags); kref_get(&dev->kref); - DBG(dev, "printer_open returned %x\n", ret); + VDBG(dev, "%s returned %x\n", __func__, ret); + return ret; } @@ -381,7 +384,6 @@ printer_close(struct inode *inode, struct file *fd) dev->printer_status &= ~PRINTER_SELECTED; spin_unlock_irqrestore(&dev->lock, flags); - DBG(dev, "printer_close\n"); kref_put(&dev->kref, printer_dev_free); return 0;
Don't issue any messages if printer_open() is successful. Also don't issue them if unsuccessful - the error code is propagated to the calling layers and should be acted on appropriately there. Just as it is with the -ENODEV case. For those who really want this message leave an option to compile-in with composite framework's VDBG() by uncommenting #define VERBOSE_DEBUG. While at it, visually detach the "return ret;" statement. Use __func__ instead of explicitly hardcoding the function name. This, in turn makes checkpatch issue this for the message in printer_close(): WARNING: Unnecessary ftrace-like logging - prefer using ftrace 54: FILE: drivers/usb/gadget/function/f_printer.c:387: + VDBG(dev, "%s\n", __func__); which lets us eliminate the debug message from printer_close() altogether. Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> --- Attention This patch depends on a recent patch from Dan Carpenter: usb: gadget: function: use after free in printer_close() drivers/usb/gadget/function/f_printer.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)