Re: [PATCH V3 1/2] usb: gadget: u_ether: Convert prints to device prints

From: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
To: Jon Hunter <jonathanh@nvidia.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-tegra@vger.kernel.org
Subject: Re: [PATCH V3 1/2] usb: gadget: u_ether: Convert prints to device prints
Date: Mon, 13 Feb 2023 14:49:46 +0100	[thread overview]
Message-ID: <bcbf9d22-c90a-a4d1-2931-0da43bc7371e@collabora.com> (raw)
In-Reply-To: <20230209125319.18589-1-jonathanh@nvidia.com>

Hi Jon,

W dniu 9.02.2023 o 13:53, Jon Hunter pisze:
> The USB ethernet gadget driver implements its own print macros which
> call printk. Device drivers should use the device prints that print the
> device name. Fortunately, the same macro names are defined in the header
> file 'linux/usb/composite.h' and these use the device prints. Therefore,
> remove the local definitions in the USB ethernet gadget driver and use
> those in 'linux/usb/composite.h'. The only difference is that now the
> device name is printed instead of the ethernet interface name.
> 
> Tested using ethernet gadget on Jetson AGX Orin.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> Tested-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> V3: Added this patch and dropped the patch in V2 that improved some of
>      the prints.
> 
>   drivers/usb/gadget/function/u_ether.c | 36 +--------------------------
>   1 file changed, 1 insertion(+), 35 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index 8f12f3f8f6ee..740331882e8d 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -17,6 +17,7 @@
>   #include <linux/etherdevice.h>
>   #include <linux/ethtool.h>
>   #include <linux/if_vlan.h>
> +#include <linux/usb/composite.h>
>   
>   #include "u_ether.h"
>   
> @@ -103,41 +104,6 @@ static inline int qlen(struct usb_gadget *gadget, unsigned qmult)
>   
>   /*-------------------------------------------------------------------------*/
>   
> -/* REVISIT there must be a better way than having two sets
> - * of debug calls ...
> - */
> -
> -#undef DBG
> -#undef VDBG
> -#undef ERROR
> -#undef INFO
> -
> -#define xprintk(d, level, fmt, args...) \
> -	printk(level "%s: " fmt , (d)->net->name , ## args)
> -
> -#ifdef DEBUG
> -#undef DEBUG
> -#define DBG(dev, fmt, args...) \
> -	xprintk(dev , KERN_DEBUG , fmt , ## args)
> -#else
> -#define DBG(dev, fmt, args...) \
> -	do { } while (0)
> -#endif /* DEBUG */

Actually there are more (at least hypothetical) changes than the declared
change of printed device name.

If DEBUG is not set there can be _more_ messages printed
when your patch is applied as-is (i.e. all DBG() invocations will
expand into some dev_dbg(), whereas before the patch is applied
they compile into nothing).

> -
> -#ifdef VERBOSE_DEBUG
> -#define VDBG	DBG
> -#else
> -#define VDBG(dev, fmt, args...) \
> -	do { } while (0)
> -#endif /* DEBUG */

In the same spirit, the above block could be considered valid.

Not sure, how much of an issue it is, though.

Regards,

Andrzej

> -
> -#define ERROR(dev, fmt, args...) \
> -	xprintk(dev , KERN_ERR , fmt , ## args)
> -#define INFO(dev, fmt, args...) \
> -	xprintk(dev , KERN_INFO , fmt , ## args)
> -
> -/*-------------------------------------------------------------------------*/
> -
>   /* NETWORK DRIVER HOOKUP (to the layer above this driver) */
>   
>   static void eth_get_drvinfo(struct net_device *net, struct ethtool_drvinfo *p)