[PATCH V3 1/2] usb: gadget: u_ether: Improve print in gether_setup_name_default()

[PATCH V3 1/2] usb: gadget: u_ether: Improve print in gether_setup_name_default()

[Jon Hunter]

The print in in gether_setup_name_default() does not provide any useful
information because a random MAC address will always be generated when
calling this function. Update the print to show MAC address that is
generated which is similar to other ethernet drivers.

Given that the strings 'self' and 'host' are static, we do not need to
pass these strings as arguments.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
V2 -> V3: Updated commit message
V1 -> V2: Added this patch

 drivers/usb/gadget/function/u_ether.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 8f12f3f8f6ee..be8e7b448933 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -845,13 +845,13 @@ struct net_device *gether_setup_name_default(const char *netname)
 	snprintf(net->name, sizeof(net->name), "%s%%d", netname);
 
 	eth_random_addr(dev->dev_mac);
-	pr_warn("using random %s ethernet address\n", "self");
+	pr_warn("using random self ethernet address %pM\n", dev->dev_mac);
 
 	/* by default we always have a random MAC address */
 	net->addr_assign_type = NET_ADDR_RANDOM;
 
 	eth_random_addr(dev->host_mac);
-	pr_warn("using random %s ethernet address\n", "host");
+	pr_warn("using random host ethernet address %pM\n", dev->host_mac);
 
 	net->netdev_ops = &eth_netdev_ops;
 
-- 
2.25.1

[PATCH V3 2/2] usb: gadget: u_ether: Don't warn in gether_setup_name_default()

[Jon Hunter]

The function gether_setup_name_default() is called by various USB
ethernet gadget drivers. Calling this function will select a random
host and device MAC addresses. A properly working driver should be
silent, so don't warn the user about default MAC addresses selection
happening and convert the warnings into debug messages.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
V2 -> V3: Updated commit message.
V1 -> V2: Changed print to debug instead of info.

 drivers/usb/gadget/function/u_ether.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index be8e7b448933..8feb40e38137 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -845,13 +845,13 @@ struct net_device *gether_setup_name_default(const char *netname)
 	snprintf(net->name, sizeof(net->name), "%s%%d", netname);
 
 	eth_random_addr(dev->dev_mac);
-	pr_warn("using random self ethernet address %pM\n", dev->dev_mac);
+	pr_debug("using random self ethernet address %pM\n", dev->dev_mac);
 
 	/* by default we always have a random MAC address */
 	net->addr_assign_type = NET_ADDR_RANDOM;
 
 	eth_random_addr(dev->host_mac);
-	pr_warn("using random host ethernet address %pM\n", dev->host_mac);
+	pr_debug("using random host ethernet address %pM\n", dev->host_mac);
 
 	net->netdev_ops = &eth_netdev_ops;
 
-- 
2.25.1

Re: [PATCH V3 2/2] usb: gadget: u_ether: Don't warn in gether_setup_name_default()

[Greg Kroah-Hartman]

On Thu, Jan 19, 2023 at 10:25:47AM +0000, Jon Hunter wrote:
> The function gether_setup_name_default() is called by various USB
> ethernet gadget drivers. Calling this function will select a random
> host and device MAC addresses. A properly working driver should be
> silent, so don't warn the user about default MAC addresses selection
> happening and convert the warnings into debug messages.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> V2 -> V3: Updated commit message.
> V1 -> V2: Changed print to debug instead of info.
> 
>  drivers/usb/gadget/function/u_ether.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index be8e7b448933..8feb40e38137 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -845,13 +845,13 @@ struct net_device *gether_setup_name_default(const char *netname)
>  	snprintf(net->name, sizeof(net->name), "%s%%d", netname);
>  
>  	eth_random_addr(dev->dev_mac);
> -	pr_warn("using random self ethernet address %pM\n", dev->dev_mac);
> +	pr_debug("using random self ethernet address %pM\n", dev->dev_mac);
>  
>  	/* by default we always have a random MAC address */
>  	net->addr_assign_type = NET_ADDR_RANDOM;
>  
>  	eth_random_addr(dev->host_mac);
> -	pr_warn("using random host ethernet address %pM\n", dev->host_mac);
> +	pr_debug("using random host ethernet address %pM\n", dev->host_mac);

In looking at this further, you are right, the structures are not
initialized properly yet.

So move these to a different function entirely, they should be in
gether_register_netdev() instead, after the device is registered with
the system.

thanks,

greg k-h

Re: [PATCH V3 2/2] usb: gadget: u_ether: Don't warn in gether_setup_name_default()

[Jon Hunter]

On 19/01/2023 10:46, Greg Kroah-Hartman wrote:

...

> In looking at this further, you are right, the structures are not
> initialized properly yet.
> 
> So move these to a different function entirely, they should be in
> gether_register_netdev() instead, after the device is registered with
> the system.


I wonder if this is even necessary. Looking at gether_register_netdev() 
it already has ...

  status = register_netdev(net);
  if (status < 0) {
          dev_dbg(&g->dev, "register_netdev failed, %d\n", status);
          return status;
  } else {
          INFO(dev, "HOST MAC %pM\n", dev->host_mac);
          INFO(dev, "MAC %pM\n", dev->dev_mac);
          ...
  }


Any objection to just removing completely from 
gether_setup_name_default()? If we move to
gether_register_netdev() then I believe we also have to deal with 
gether_register_netdev().

Jon

-- 
nvpublic

Re: [PATCH V3 2/2] usb: gadget: u_ether: Don't warn in gether_setup_name_default()

[Greg Kroah-Hartman]

On Thu, Jan 19, 2023 at 01:38:15PM +0000, Jon Hunter wrote:
> 
> On 19/01/2023 10:46, Greg Kroah-Hartman wrote:
> 
> ...
> 
> > In looking at this further, you are right, the structures are not
> > initialized properly yet.
> > 
> > So move these to a different function entirely, they should be in
> > gether_register_netdev() instead, after the device is registered with
> > the system.
> 
> 
> I wonder if this is even necessary. Looking at gether_register_netdev() it
> already has ...
> 
>  status = register_netdev(net);
>  if (status < 0) {
>          dev_dbg(&g->dev, "register_netdev failed, %d\n", status);
>          return status;
>  } else {
>          INFO(dev, "HOST MAC %pM\n", dev->host_mac);
>          INFO(dev, "MAC %pM\n", dev->dev_mac);

Those need to be moved to be dev_info() and then I'll stop complaining :)
Or dev_dbg() if you want to make it debug only.

>          ...
>  }
> 
> 
> Any objection to just removing completely from gether_setup_name_default()?

Nope!

Remove them and change the above lines and then all should be good.

thanks,

greg k-h

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

[Jon Hunter]

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 */
-
-#ifdef VERBOSE_DEBUG
-#define VDBG	DBG
-#else
-#define VDBG(dev, fmt, args...) \
-	do { } while (0)
-#endif /* DEBUG */
-
-#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)
-- 
2.34.1

[PATCH V3 2/2] usb: gadget: u_ether: Don't warn in gether_setup_name_default()

[Jon Hunter]

The function gether_setup_name_default() is called by various USB
ethernet gadget drivers. Calling this function will select a random
host and device MAC addresses. A properly working driver should be
silent and not warn the user about default MAC addresses selection.
Given that the MAC addresses are also printed when the function
gether_register_netdev() is called, remove these unnecessary warnings.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
V3: Updated patch to remove the prints completely.
V2: Changed print to debug instead of info.

 drivers/usb/gadget/function/u_ether.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 740331882e8d..953d936fbae6 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -811,13 +811,11 @@ struct net_device *gether_setup_name_default(const char *netname)
 	snprintf(net->name, sizeof(net->name), "%s%%d", netname);
 
 	eth_random_addr(dev->dev_mac);
-	pr_warn("using random %s ethernet address\n", "self");
 
 	/* by default we always have a random MAC address */
 	net->addr_assign_type = NET_ADDR_RANDOM;
 
 	eth_random_addr(dev->host_mac);
-	pr_warn("using random %s ethernet address\n", "host");
 
 	net->netdev_ops = &eth_netdev_ops;
 
-- 
2.34.1

Re: [PATCH V3 2/2] usb: gadget: u_ether: Don't warn in gether_setup_name_default()

[Andrzej Pietrasiewicz]

W dniu 9.02.2023 o 13:53, Jon Hunter pisze:
> The function gether_setup_name_default() is called by various USB
> ethernet gadget drivers. Calling this function will select a random
> host and device MAC addresses. A properly working driver should be
> silent and not warn the user about default MAC addresses selection.
> Given that the MAC addresses are also printed when the function
> gether_register_netdev() is called, remove these unnecessary warnings.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> ---
> V3: Updated patch to remove the prints completely.
> V2: Changed print to debug instead of info.
> 
>   drivers/usb/gadget/function/u_ether.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index 740331882e8d..953d936fbae6 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -811,13 +811,11 @@ struct net_device *gether_setup_name_default(const char *netname)
>   	snprintf(net->name, sizeof(net->name), "%s%%d", netname);
>   
>   	eth_random_addr(dev->dev_mac);
> -	pr_warn("using random %s ethernet address\n", "self");
>   
>   	/* by default we always have a random MAC address */
>   	net->addr_assign_type = NET_ADDR_RANDOM;
>   
>   	eth_random_addr(dev->host_mac);
> -	pr_warn("using random %s ethernet address\n", "host");
>   
>   	net->netdev_ops = &eth_netdev_ops;
>