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: [email protected] # 6.6+
Fixes: d2689b6a86b9 ("net: usb: ax88179_178a: avoid two consecutive device resets")
Reported-by: Dave Stevenson <[email protected]>
Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
---
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);
--
2.44.0
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: [email protected] # 6.6+
> Fixes: d2689b6a86b9 ("net: usb: ax88179_178a: avoid two consecutive device resets")
> Reported-by: Dave Stevenson <[email protected]>
> Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
> ---
> 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
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: [email protected] # 6.6+
Fixes: 9fb137aef34e ("net: usb: ax88179_178a: allow optionally getting mac address from device tree")
Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
---
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,
--
2.44.0
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: [email protected] # 6.6+
Fixes: d2689b6a86b9 ("net: usb: ax88179_178a: avoid two consecutive device resets")
Reported-by: Dave Stevenson <[email protected]>
Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
---
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);
--
2.44.0
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: [email protected] # 6.6+
> Fixes: 9fb137aef34e ("net: usb: ax88179_178a: allow optionally getting mac address from device tree")
> Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
> ---
> 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