| Message ID | 20240510090846.328201-1-jtornosm@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ecf848eb934b03959918f5269f64c0e52bc23998 |
| Headers | show |
| Series | net: usb: ax88179_178a: fix link status when link is set to down/up | expand |
On Fri, May 10, 2024 at 11:08:28AM +0200, Jose Ignacio Tornos Martinez wrote: > The idea was to keep only one reset at initialization stage in order to > reduce the total delay, or the reset from usbnet_probe or the reset from > usbnet_open. > > I have seen that restarting from usbnet_probe is necessary to avoid doing > too complex things. But when the link is set to down/up (for example to > configure a different mac address) the link is not correctly recovered > unless a reset is commanded from usbnet_open. > > So, detect the initialization stage (first call) to not reset from > usbnet_open after the reset from usbnet_probe and after this stage, always > reset from usbnet_open too (when the link needs to be rechecked). > > Apply to all the possible devices, the behavior now is going to be the same. > > cc: stable@vger.kernel.org # 6.6+ > Fixes: 56f78615bcb1 ("net: usb: ax88179_178a: avoid writing the mac address before first reading") > Reported-by: Isaac Ganoung <inventor500@vivaldi.net> > Reported-by: Yongqin Liu <yongqin.liu@linaro.org> > Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> Reviewed-by: Simon Horman <horms@kernel.org>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 10 May 2024 11:08:28 +0200 you wrote: > The idea was to keep only one reset at initialization stage in order to > reduce the total delay, or the reset from usbnet_probe or the reset from > usbnet_open. > > I have seen that restarting from usbnet_probe is necessary to avoid doing > too complex things. But when the link is set to down/up (for example to > configure a different mac address) the link is not correctly recovered > unless a reset is commanded from usbnet_open. > > [...] Here is the summary with links: - net: usb: ax88179_178a: fix link status when link is set to down/up https://git.kernel.org/netdev/net/c/ecf848eb934b You are awesome, thank you!
Hi, Jose On Fri, 10 May 2024 at 17:09, Jose Ignacio Tornos Martinez <jtornosm@redhat.com> wrote: > > The idea was to keep only one reset at initialization stage in order to > reduce the total delay, or the reset from usbnet_probe or the reset from > usbnet_open. > > I have seen that restarting from usbnet_probe is necessary to avoid doing > too complex things. But when the link is set to down/up (for example to > configure a different mac address) the link is not correctly recovered > unless a reset is commanded from usbnet_open. > > So, detect the initialization stage (first call) to not reset from > usbnet_open after the reset from usbnet_probe and after this stage, always > reset from usbnet_open too (when the link needs to be rechecked). > > Apply to all the possible devices, the behavior now is going to be the same. > > cc: stable@vger.kernel.org # 6.6+ > Fixes: 56f78615bcb1 ("net: usb: ax88179_178a: avoid writing the mac address before first reading") > Reported-by: Isaac Ganoung <inventor500@vivaldi.net> > Reported-by: Yongqin Liu <yongqin.liu@linaro.org> > Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> > --- Sorry, I just have a time to test this patch, and it does not help for the issue I reported here: https://lore.kernel.org/all/CAMSo37UN11V8UeDM4cyD+iXyRR1Us53a00e34wTy+zP6vx935A@mail.gmail.com/ Here is the serial console log after I cherry picked this change: https://gist.github.com/liuyq/6255f2ccd98fa98ac0ed296a61f49883 Could you please help to check it again? Please let me know if there is anything I could provide for the investigation. Thanks, Yongqin Liu > drivers/net/usb/ax88179_178a.c | 37 ++++++++++++++++++++++++---------- > 1 file changed, 26 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c > index 377be0d9ef14..a0edb410f746 100644 > --- a/drivers/net/usb/ax88179_178a.c > +++ b/drivers/net/usb/ax88179_178a.c > @@ -174,6 +174,7 @@ struct ax88179_data { > u32 wol_supported; > u32 wolopts; > u8 disconnecting; > + u8 initialized; > }; > > struct ax88179_int_data { > @@ -1672,6 +1673,18 @@ static int ax88179_reset(struct usbnet *dev) > return 0; > } > > +static int ax88179_net_reset(struct usbnet *dev) > +{ > + struct ax88179_data *ax179_data = dev->driver_priv; > + > + if (ax179_data->initialized) > + ax88179_reset(dev); > + else > + ax179_data->initialized = 1; > + > + return 0; > +} > + > static int ax88179_stop(struct usbnet *dev) > { > u16 tmp16; > @@ -1691,6 +1704,7 @@ static const struct driver_info ax88179_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1703,6 +1717,7 @@ static const struct driver_info ax88178a_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1715,7 +1730,7 @@ static const struct driver_info cypress_GX3_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > - .reset = ax88179_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1728,7 +1743,7 @@ static const struct driver_info dlink_dub1312_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > - .reset = ax88179_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1741,7 +1756,7 @@ static const struct driver_info sitecom_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > - .reset = ax88179_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1754,7 +1769,7 @@ static const struct driver_info samsung_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > - .reset = ax88179_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1767,7 +1782,7 @@ static const struct driver_info lenovo_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > - .reset = ax88179_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1780,7 +1795,7 @@ static const struct driver_info belkin_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > - .reset = ax88179_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1793,7 +1808,7 @@ static const struct driver_info toshiba_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > - .reset = ax88179_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1806,7 +1821,7 @@ static const struct driver_info mct_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > - .reset = ax88179_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1819,7 +1834,7 @@ static const struct driver_info at_umc2000_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > - .reset = ax88179_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1832,7 +1847,7 @@ static const struct driver_info at_umc200_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > - .reset = ax88179_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > @@ -1845,7 +1860,7 @@ static const struct driver_info at_umc2000sp_info = { > .unbind = ax88179_unbind, > .status = ax88179_status, > .link_reset = ax88179_link_reset, > - .reset = ax88179_reset, > + .reset = ax88179_net_reset, > .stop = ax88179_stop, > .flags = FLAG_ETHER | FLAG_FRAMING_AX, > .rx_fixup = ax88179_rx_fixup, > -- > 2.44.0 >
Hello Yongqin, Again, not a lot of information from the logs, but perhaps you coud give me more information for your scenario. Could you try to execute the down interface operation, mac assignment and the up interface operation from command line? That works for me. Maybe some synchronization issue is happening in your boot operation. Could you provide more information about how/when you are doing the commented operations to try to reproduce? Best regards José Ignacio
Hi, Jose On Thu, 23 May 2024 at 14:45, Jose Ignacio Tornos Martinez <jtornosm@redhat.com> wrote: > > Hello Yongqin, > > Again, not a lot of information from the logs, but perhaps you coud give me > more information for your scenario. > > Could you try to execute the down interface operation, mac assignment and > the up interface operation from command line? > That works for me. When I tried the down and up operations manually from the command line, it worked. But it only worked after I ran the down and up operations after the boot. It fails to work by default after the boot for both the fresh deployment, and for the later reboot One thing I noticed is that the following message was printed twice "ax88179_178a 2-3:1.0 eth0: ax88179 - Link status is: 1" after I ran the up operation, Is that expected? For details, please check the log here: https://gist.github.com/liuyq/be8f5305d538067a344001f1d35f677b > Maybe some synchronization issue is happening in your boot operation. > Could you provide more information about how/when you are doing the > commented operations to try to reproduce? The scripts are simple, here are the two scripts for Android build: https://android.googlesource.com/device/linaro/dragonboard/+/refs/heads/main/shared/utils/ethaddr/ethaddr.rc https://android.googlesource.com/device/linaro/dragonboard/+/refs/heads/main/shared/utils/ethaddr/set_ethaddr.sh Is the one to run the down/change mac/up operations script. Not sure why the up in the script does not work, but works when run manually. -- Best Regards, Yongqin Liu --------------------------------------------------------------- #mailing list linaro-android@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-android
Hello Yongqin, > When I tried the down and up operations manually from the command line, > it worked. > But it only worked after I ran the down and up operations after the boot. > It fails to work by default after the boot for both the fresh deployment, > and for the later reboot Ok, so it works as well for you after the initialization. > One thing I noticed is that the following message was printed twice > "ax88179_178a 2-3:1.0 eth0: ax88179 - Link status is: 1" > after I ran the up operation, > > Is that expected? > > For details, please check the log here: > https://gist.github.com/liuyq/be8f5305d538067a344001f1d35f677b That is another thing that I am analyzing, to clean those spurious. But they are appearing in my case too, and I am not modifying anything at boot time. > The scripts are simple, here are the two scripts for Android build: > https://android.googlesource.com/device/linaro/dragonboard/+/refs/heads/main/shared/utils/ethaddr/ethaddr.rc > https://android.googlesource.com/device/linaro/dragonboard/+/refs/heads/main/shared/utils/ethaddr/set_ethaddr.sh > > Is the one to run the down/change mac/up operations script. > > Not sure why the up in the script does not work, but works when run manually. Ok, I am not working with Android but it doesn't seem spscial, the only doubt is when the script is executed, if the driver initialization is complete, ... Anyway, I will try to reproduce here and analyze it. Best regards José Ignacio
On Tue, 28 May 2024 at 17:18, Jose Ignacio Tornos Martinez <jtornosm@redhat.com> wrote: > > Hello Yongqin, > > > When I tried the down and up operations manually from the command line, > > it worked. > > But it only worked after I ran the down and up operations after the boot. > > It fails to work by default after the boot for both the fresh deployment, > > and for the later reboot > Ok, so it works as well for you after the initialization. > > > One thing I noticed is that the following message was printed twice > > "ax88179_178a 2-3:1.0 eth0: ax88179 - Link status is: 1" > > after I ran the up operation, > > > > Is that expected? > > > > For details, please check the log here: > > https://gist.github.com/liuyq/be8f5305d538067a344001f1d35f677b > That is another thing that I am analyzing, to clean those spurious. > But they are appearing in my case too, and I am not modifying anything at > boot time. > > > The scripts are simple, here are the two scripts for Android build: > > https://android.googlesource.com/device/linaro/dragonboard/+/refs/heads/main/shared/utils/ethaddr/ethaddr.rc > > https://android.googlesource.com/device/linaro/dragonboard/+/refs/heads/main/shared/utils/ethaddr/set_ethaddr.sh > > > > Is the one to run the down/change mac/up operations script. > > > > Not sure why the up in the script does not work, but works when run manually. > Ok, I am not working with Android but it doesn't seem spscial, the only > doubt is when the script is executed, if the driver initialization is > complete, ... is there any message that I could check to make sure if the initialization is finished? or like with adding some printk lines for some kernel functions to hack > Anyway, I will try to reproduce here and analyze it. Thanks very much! And please feel free to let me know if there is anything I could help with on the Android build.
Hello Yongqin, > is there any message that I could check to make sure if the > initialization is finished? > or like with adding some printk lines for some kernel functions to hack > >> Anyway, I will try to reproduce here and analyze it. > > Thanks very much! And please feel free to let me know if there is > anything I could help with on the Android build. I have finally managed to reproduce an error similar to the one mentioned during the boot stage. I created a systemd service with a similar configuration script to do the same and it works (I can reconfigure the mac address at boot time, the ip address is configured and the interface works), because the driver is completely initialized. In order to reproduce, I have introduced a big delay in the probe operation to get closer in time to the configuration script and the problem is there. Maybe, the script set_ethaddr.sh could be synchronized with the driver, but I think, if possible, it is better to check in a better way in the driver; I will try it. When I have something I can comment you, if you can test it, to be sure about the solution. By the way, I have tried with my other fix to avoid the spurious link messages, but it didn't help. Best regards José Ignacio
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index 377be0d9ef14..a0edb410f746 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -174,6 +174,7 @@ struct ax88179_data { u32 wol_supported; u32 wolopts; u8 disconnecting; + u8 initialized; }; struct ax88179_int_data { @@ -1672,6 +1673,18 @@ static int ax88179_reset(struct usbnet *dev) return 0; } +static int ax88179_net_reset(struct usbnet *dev) +{ + struct ax88179_data *ax179_data = dev->driver_priv; + + if (ax179_data->initialized) + ax88179_reset(dev); + else + ax179_data->initialized = 1; + + return 0; +} + static int ax88179_stop(struct usbnet *dev) { u16 tmp16; @@ -1691,6 +1704,7 @@ static const struct driver_info ax88179_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1703,6 +1717,7 @@ static const struct driver_info ax88178a_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1715,7 +1730,7 @@ static const struct driver_info cypress_GX3_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, - .reset = ax88179_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1728,7 +1743,7 @@ static const struct driver_info dlink_dub1312_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, - .reset = ax88179_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1741,7 +1756,7 @@ static const struct driver_info sitecom_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, - .reset = ax88179_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1754,7 +1769,7 @@ static const struct driver_info samsung_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, - .reset = ax88179_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1767,7 +1782,7 @@ static const struct driver_info lenovo_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, - .reset = ax88179_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1780,7 +1795,7 @@ static const struct driver_info belkin_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, - .reset = ax88179_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1793,7 +1808,7 @@ static const struct driver_info toshiba_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, - .reset = ax88179_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1806,7 +1821,7 @@ static const struct driver_info mct_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, - .reset = ax88179_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1819,7 +1834,7 @@ static const struct driver_info at_umc2000_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, - .reset = ax88179_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1832,7 +1847,7 @@ static const struct driver_info at_umc200_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, - .reset = ax88179_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup, @@ -1845,7 +1860,7 @@ static const struct driver_info at_umc2000sp_info = { .unbind = ax88179_unbind, .status = ax88179_status, .link_reset = ax88179_link_reset, - .reset = ax88179_reset, + .reset = ax88179_net_reset, .stop = ax88179_stop, .flags = FLAG_ETHER | FLAG_FRAMING_AX, .rx_fixup = ax88179_rx_fixup,
The idea was to keep only one reset at initialization stage in order to reduce the total delay, or the reset from usbnet_probe or the reset from usbnet_open. I have seen that restarting from usbnet_probe is necessary to avoid doing too complex things. But when the link is set to down/up (for example to configure a different mac address) the link is not correctly recovered unless a reset is commanded from usbnet_open. So, detect the initialization stage (first call) to not reset from usbnet_open after the reset from usbnet_probe and after this stage, always reset from usbnet_open too (when the link needs to be rechecked). Apply to all the possible devices, the behavior now is going to be the same. cc: stable@vger.kernel.org # 6.6+ Fixes: 56f78615bcb1 ("net: usb: ax88179_178a: avoid writing the mac address before first reading") Reported-by: Isaac Ganoung <inventor500@vivaldi.net> Reported-by: Yongqin Liu <yongqin.liu@linaro.org> Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> --- drivers/net/usb/ax88179_178a.c | 37 ++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 11 deletions(-)