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.
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.
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); - }
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN, ETH_ALEN, dev->net->dev_addr);
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
Hello Simon,
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?
You are right, really it is a separate improvement or simplification. Right now, it is not affecting the users and it is not producing any problem, just a second random address is generated if there is any problem, and this is not necessary, because there is a random address generated previously. When the extra reset was done during binding operation, as we were modifying the pregenerated random address, the check in usbnet_probe was useless. Ok, I will split the patch in two to be considered separately, the first with the important fix and the second with the commented improvement or clean-up.
nit: AFAIK, if one arm of a conditional has curly-brackets, then all should. So there is no need to drop them here.
I didn't know the related criteria, I will do as you say.
Thank you
Best regards José Ignacio
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.
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 --- V1 -> V2: - Split the fix and the improvement in two patches as Simon Horman suggests.
drivers/net/usb/ax88179_178a.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index 88e084534853..8ca8ace93d9c 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -1273,6 +1273,7 @@ static void ax88179_get_mac_addr(struct usbnet *dev)
if (is_valid_ether_addr(mac)) { eth_hw_addr_set(dev->net, mac); + 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);
If the mac address can not be read from the device registers or the devicetree, a random address is generated, but this was already done from usbnet_probe, so it is not necessary to call eth_hw_addr_random from here again to generate another random address.
Indeed, when reset was also executed from bind, generate another random mac address invalidated the check from usbnet_probe to configure if the assigned mac address for the interface was random or not, because it is comparing with the initial generated random address. Now, with only a reset from open operation, it is just a harmless simplification.
cc: stable@vger.kernel.org # 6.6+ Fixes: 9fb137aef34e ("net: usb: ax88179_178a: allow optionally getting mac address from device tree") Signed-off-by: Jose Ignacio Tornos Martinez jtornosm@redhat.com --- V1 -> V2: - Split the fix and the improvement in two patches and keep curly-brackets as Simon Horman suggests.
drivers/net/usb/ax88179_178a.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index 8ca8ace93d9c..08c9b2ab9711 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -1276,7 +1276,6 @@ static void ax88179_get_mac_addr(struct usbnet *dev) 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); }
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN, ETH_ALEN,
On Tue, Mar 26, 2024 at 05:31:07PM +0100, Jose Ignacio Tornos Martinez wrote:
If the mac address can not be read from the device registers or the devicetree, a random address is generated, but this was already done from usbnet_probe, so it is not necessary to call eth_hw_addr_random from here again to generate another random address.
Indeed, when reset was also executed from bind, generate another random mac address invalidated the check from usbnet_probe to configure if the assigned mac address for the interface was random or not, because it is comparing with the initial generated random address. Now, with only a reset from open operation, it is just a harmless simplification.
cc: stable@vger.kernel.org # 6.6+ Fixes: 9fb137aef34e ("net: usb: ax88179_178a: allow optionally getting mac address from device tree") Signed-off-by: Jose Ignacio Tornos Martinez jtornosm@redhat.com
V1 -> V2:
- Split the fix and the improvement in two patches and keep curly-brackets
as Simon Horman suggests.
Hi Jose,
Thanks for splitting the patches. They look good, but there are a few process issues. Sorry for not being clearer in my previous email.
As the 2nd patch of the series is not a fix it: - Should not have a fixes tag - Should not be sent to stable - Should be targeted at the net-next tree rather than the net tree
As the granularity of patch handling on netdev is generally at the patchset level I believe that this means that you need to separately, in different, new, email threads, repost:
1. Patch 1/2 of this series, targeted at net, with a Fixes tag
[PATCH net v3] net: usb: ax88179_178a: avoid the interface always configured as random address
2. Patch 2/2 of this series, targeted at net-next, without a Fixes tag
[PATCH net-next v3] et: usb: ax88179_178a: non necessary second random mac address
Also, please be sure to wait 24 hours since the posting of this patch-set before reposting.
Some more information can be found here: https://docs.kernel.org/process/maintainer-netdev.html
...
-- pw-bot: changes-requested
Hello Simon,
Returning after Easter.
I'm sorry I didn't understand you, I have to learn more about the procedure. I hope to do it better now.
Thanks for your help
Best regards José Ignacio
linux-stable-mirror@lists.linaro.org
-
Jose Ignacio Tornos Martinez -
Simon Horman