[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