Re: [PATCH] net: usb: ax88179_178a: avoid the interface always configured as random address

[Simon Horman <horms@kernel.org>]

On Mon, Mar 25, 2024 at 06:31:50PM +0100, Jose Ignacio Tornos Martinez wrote:
> After the commit d2689b6a86b9 ("net: usb: ax88179_178a: avoid two
> consecutive device resets"), reset is not executed from bind operation and
> mac address is not read from the device registers or the devicetree at that
> moment. Since the check to configure if the assigned mac address is random
> or not for the interface, happens after the bind operation from
> usbnet_probe, the interface keeps configured as random address, although the
> address is correctly read and set during open operation (the only reset
> now).
> 
> In order to keep only one reset for the device and to avoid the interface
> always configured as random address, after reset, configure correctly the
> suitable field from the driver, if the mac address is read successfully from
> the device registers or the devicetree.

Thanks Jose,

The above makes sense to me and I agree with your fix and
corresponding Fixes tag.

> In addition, if mac address can not be read from the driver, a random
> address is configured again, so it is not necessary to call
> eth_hw_addr_random from here. Indeed, in this situtatuon, when reset was
> also executed from bind, this was invalidating the check to configure if the
> assigned mac address for the interface was random or not.

I also agree with your analysis here. However it does seem to be a separate
problem. And perhaps warrants a separate patch. I am also wondering
if this is more of a clean-up than a fix: does it cause a bug
that is observable by users?

> cc: stable@vger.kernel.org # 6.6+
> Fixes: d2689b6a86b9 ("net: usb: ax88179_178a: avoid two consecutive device resets")
> Reported-by: Dave Stevenson  <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
> ---
>  drivers/net/usb/ax88179_178a.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 88e084534853..d2324cc02461 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1273,10 +1273,9 @@ static void ax88179_get_mac_addr(struct usbnet *dev)
>  
>  	if (is_valid_ether_addr(mac)) {
>  		eth_hw_addr_set(dev->net, mac);
> -	} else {
> +		dev->net->addr_assign_type = NET_ADDR_PERM;
> +	} else
>  		netdev_info(dev->net, "invalid MAC address, using random\n");
> -		eth_hw_addr_random(dev->net);
> -	}

nit: AFAIK, if one arm of a conditional has curly-brackets, then all should.
     So there is no need to drop them here.

>  
>  	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN, ETH_ALEN,
>  			  dev->net->dev_addr);
> -- 
> 2.44.0
> 
>