On Tue, 2023-11-14 at 13:50 +0100, Jose Ignacio Tornos Martinez wrote:
> The device is always reset two consecutive times (ax88179_reset is called
> twice), one from usbnet_probe during the device binding and the other from
> usbnet_open.
>
> Let only the reset during the device binding to prepare the device as soon
> as possible and not repeat the reset operation (tested with generic ASIX
> Electronics Corp. AX88179 Gigabit Ethernet device).
>
> Reported-by: Herb Wei <[email protected]>
> Tested-by: Herb Wei <[email protected]>
> Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
We need a suitable Fixes tag even here ;)
> ---
> drivers/net/usb/ax88179_178a.c | 13 -------------
> 1 file changed, 13 deletions(-)
>
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 4ea0e155bb0d..864c6fc2db33 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1678,7 +1678,6 @@ static const struct driver_info ax88179_info = {
> .unbind = ax88179_unbind,
> .status = ax88179_status,
> .link_reset = ax88179_link_reset,
> - .reset = ax88179_reset,
> .stop = ax88179_stop,
> .flags = FLAG_ETHER | FLAG_FRAMING_AX,
> .rx_fixup = ax88179_rx_fixup,
This looks potentially dangerous, as the device will not get a reset in
down/up cycles; *possibly* dropping the reset call from ax88179_bind()
would be safer.
In both cases touching so many H/W variant with testing available on a
single one sounds dangerous. Is the unneeded 2nd reset causing any
specific issue?
Thanks,
Paolo
On Thu, Nov 16, 2023 at 10:42 AM Paolo Abeni <[email protected]> wrote:
> We need a suitable Fixes tag even here ;)
Ok, I will add it in my next version.
> > ---
> > drivers/net/usb/ax88179_178a.c | 13 -------------
> > 1 file changed, 13 deletions(-)
> >
> > diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> > index 4ea0e155bb0d..864c6fc2db33 100644
> > --- a/drivers/net/usb/ax88179_178a.c
> > +++ b/drivers/net/usb/ax88179_178a.c
> > @@ -1678,7 +1678,6 @@ static const struct driver_info ax88179_info = {
> > .unbind = ax88179_unbind,
> > .status = ax88179_status,
> > .link_reset = ax88179_link_reset,
> > - .reset = ax88179_reset,
> > .stop = ax88179_stop,
> > .flags = FLAG_ETHER | FLAG_FRAMING_AX,
> > .rx_fixup = ax88179_rx_fixup,
>
> This looks potentially dangerous, as the device will not get a reset in
> down/up cycles; *possibly* dropping the reset call from ax88179_bind()
> would be safer.
Ok, I had the doubt about which reset would be the best, because it seemed
to me that reset would be better as soon as possible.
I will try what you say to avoid down/up cycles.
> In both cases touching so many H/W variant with testing available on a
> single one sounds dangerous. Is the unneeded 2nd reset causing any
> specific issue?
Actually, this double reboot somewhat masked the first problem, because the
probability of getting a successful initialization, if there is a previous
problem seems to be higher. So, it is not strictly needed but I think it is
better to avoid a second unnecessary reset.
Ok, if I modify the call from ax88179_bind() I will be respecting the reset
operation of all devices.
Thanks
Best regards
José Ignacio
Using generic ASIX Electronics Corp. AX88179 Gigabit Ethernet device,
the following test cycle has been implemented:
- power on
- check logs
- shutdown
- after detecting the system shutdown, disconnect power
- after approximately 60 seconds of sleep, power is restored
Running some cycles, sometimes error logs like this appear:
kernel: ax88179_178a 2-9:1.0 (unnamed net_device) (uninitialized): Failed to write reg index 0x0001: -19
kernel: ax88179_178a 2-9:1.0 (unnamed net_device) (uninitialized): Failed to read reg index 0x0001: -19
...
These failed operation are happening during ax88179_reset execution, so
the initialization could not be correct.
In order to avoid this, we need to increase the delay after reset and
clock initial operations. By using these larger values, many cycles
have been run and no failed operations appear.
It would be better to check some status register to verify when the
operation has finished, but I do not have found any available information
(neither in the public datasheets nor in the manufacturer's driver). The
only available information for the necessary delays is the maufacturer's
driver (original values) but the proposed values are not enough for the
tested devices.
Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
Reported-by: Herb Wei <[email protected]>
Tested-by: Herb Wei <[email protected]>
Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
---
V1 -> V2:
- Add Fixes tag.
- Comments about the available information and manufacturer's driver
reference to complete why the new values are needed.
drivers/net/usb/ax88179_178a.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index aff39bf3161d..4ea0e155bb0d 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1583,11 +1583,11 @@ static int ax88179_reset(struct usbnet *dev)
*tmp16 = AX_PHYPWR_RSTCTL_IPRL;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
- msleep(200);
+ msleep(500);
*tmp = AX_CLK_SELECT_ACS | AX_CLK_SELECT_BCS;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, tmp);
- msleep(100);
+ msleep(200);
/* Ethernet PHY Auto Detach*/
ax88179_auto_detach(dev);
--
2.42.0
The device is always reset two consecutive times (ax88179_reset is called
twice), one from usbnet_probe during the device binding and the other from
usbnet_open.
Remove the non-necessary reset during the device binding and let the reset
operation from open to keep the normal behavior (tested with generic ASIX
Electronics Corp. AX88179 Gigabit Ethernet device).
Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
Reported-by: Herb Wei <[email protected]>
Tested-by: Herb Wei <[email protected]>
Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
---
V1 -> V2:
- Add Fixes tag.
- Follow Paolo Abeni's suggestion and remove the binding reset, not the
reset operation to keep the normal behavior.
drivers/net/usb/ax88179_178a.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 4ea0e155bb0d..8d835fbc4316 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1298,8 +1298,6 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
netif_set_tso_max_size(dev->net, 16384);
- ax88179_reset(dev);
-
return 0;
}
--
2.42.0
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Mon, 20 Nov 2023 13:06:29 +0100 you wrote:
> Using generic ASIX Electronics Corp. AX88179 Gigabit Ethernet device,
> the following test cycle has been implemented:
> - power on
> - check logs
> - shutdown
> - after detecting the system shutdown, disconnect power
> - after approximately 60 seconds of sleep, power is restored
> Running some cycles, sometimes error logs like this appear:
> kernel: ax88179_178a 2-9:1.0 (unnamed net_device) (uninitialized): Failed to write reg index 0x0001: -19
> kernel: ax88179_178a 2-9:1.0 (unnamed net_device) (uninitialized): Failed to read reg index 0x0001: -19
> ...
> These failed operation are happening during ax88179_reset execution, so
> the initialization could not be correct.
>
> [...]
Here is the summary with links:
- [v2,1/2] net: usb: ax88179_178a: fix failed operations during ax88179_reset
(no matching commit)
- [v2,2/2] net: usb: ax88179_178a: avoid two consecutive device resets
https://git.kernel.org/netdev/net-next/c/d2689b6a86b9
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hello:
This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
On Mon, 20 Nov 2023 13:06:29 +0100 you wrote:
> Using generic ASIX Electronics Corp. AX88179 Gigabit Ethernet device,
> the following test cycle has been implemented:
> - power on
> - check logs
> - shutdown
> - after detecting the system shutdown, disconnect power
> - after approximately 60 seconds of sleep, power is restored
> Running some cycles, sometimes error logs like this appear:
> kernel: ax88179_178a 2-9:1.0 (unnamed net_device) (uninitialized): Failed to write reg index 0x0001: -19
> kernel: ax88179_178a 2-9:1.0 (unnamed net_device) (uninitialized): Failed to read reg index 0x0001: -19
> ...
> These failed operation are happening during ax88179_reset execution, so
> the initialization could not be correct.
>
> [...]
Here is the summary with links:
- [v2,1/2] net: usb: ax88179_178a: fix failed operations during ax88179_reset
https://git.kernel.org/netdev/net/c/0739af07d1d9
- [v2,2/2] net: usb: ax88179_178a: avoid two consecutive device resets
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html