Re: [PATCH] usb: gadget: function: Simplify error messaging in printer open/close - Greg Kroah-Hartman

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: linux-usb@vger.kernel.org, kernel@collabora.com
Subject: Re: [PATCH] usb: gadget: function: Simplify error messaging in printer open/close
Date: Tue, 22 Nov 2022 16:54:03 +0100	[thread overview]
Message-ID: <Y3zwm70hEmH7RQP3@kroah.com> (raw)
In-Reply-To: <20221122141603.70242-1-andrzej.p@collabora.com>

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