[PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Jose Ignacio Tornos Martinez]

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(-)

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

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Simon Horman]

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>

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Yongqin Liu]

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
>


-- 
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Jose Ignacio Tornos Martinez]

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

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Yongqin Liu]

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

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Jose Ignacio Tornos Martinez]

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

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Yongqin Liu]

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.

-- 
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Jose Ignacio Tornos Martinez]

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

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Jose Ignacio Tornos Martinez]

Hello Yongqin,

After some research  and testing, it seems to happen that if initialization
is slower the second reset from open is needed too.
So, I have been working with some reproducers and I think I have the
solution for detecting when there is a problem.
If you can test it in your real environment that would be great.

Here the patch on the latest version of the file:
$ git diff drivers/net/usb/ax88179_178a.c
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 51c295e1e823..60357796be99 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -174,7 +174,6 @@ struct ax88179_data {
        u32 wol_supported;
        u32 wolopts;
        u8 disconnecting;
-       u8 initialized;
 };
 
 struct ax88179_int_data {
@@ -327,7 +326,8 @@ static void ax88179_status(struct usbnet *dev, struct urb *urb)
 
        if (netif_carrier_ok(dev->net) != link) {
                usbnet_link_change(dev, link, 1);
-               netdev_info(dev->net, "ax88179 - Link status is: %d\n", link);
+               if (!link)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 51c295e1e823..60357796be99 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -174,7 +174,6 @@ struct ax88179_data {
        u32 wol_supported;
        u32 wolopts;
        u8 disconnecting;
-       u8 initialized;
 };
 
 struct ax88179_int_data {
@@ -327,7 +326,8 @@ static void ax88179_status(struct usbnet *dev, struct urb *urb)
 
        if (netif_carrier_ok(dev->net) != link) {
                usbnet_link_change(dev, link, 1);
-               netdev_info(dev->net, "ax88179 - Link status is: %d\n", link);
+               if (!link)
+                       netdev_info(dev->net, "ax88179 - Link status is: %d\n", link);
        }
 }
 
@@ -1543,6 +1543,7 @@ static int ax88179_link_reset(struct usbnet *dev)
                         GMII_PHY_PHYSR, 2, &tmp16);
 
        if (!(tmp16 & GMII_PHY_PHYSR_LINK)) {
+               netdev_info(dev->net, "ax88179 - Link status is: 0\n");
                return 0;
        } else if (GMII_PHY_PHYSR_GIGA == (tmp16 & GMII_PHY_PHYSR_SMASK)) {
                mode |= AX_MEDIUM_GIGAMODE | AX_MEDIUM_EN_125MHZ;
@@ -1580,6 +1581,8 @@ static int ax88179_link_reset(struct usbnet *dev)
 
        netif_carrier_on(dev->net);
 
+       netdev_info(dev->net, "ax88179 - Link status is: 1\n");
+
        return 0;
 }
 
@@ -1678,12 +1681,21 @@ static int ax88179_reset(struct usbnet *dev)
 
 static int ax88179_net_reset(struct usbnet *dev)
 {
-       struct ax88179_data *ax179_data = dev->driver_priv;
+       u16 tmp16;
 
-       if (ax179_data->initialized)
+       ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID, GMII_PHY_PHYSR,
+                        2, &tmp16);
+       if (tmp16) {
+               ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+                                2, 2, &tmp16);
+               if (!(tmp16 & AX_MEDIUM_RECEIVE_EN)) {
+                       tmp16 |= AX_MEDIUM_RECEIVE_EN;
+                       ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+                                         2, 2, &tmp16);
+               }
+       } else {
                ax88179_reset(dev);
-       else
-               ax179_data->initialized = 1;
+       }
 
        return 0;
 }

In addition, I have fixed the logs to show the link correclty.

If this is ok, I will submit the patch.

Thanks

Best regards
José Ignacio

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Jose Ignacio Tornos Martinez]

Hello again,

There was a problem copying the patch, sorry, here the good one:

$ git diff drivers/net/usb/ax88179_178a.c
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 51c295e1e823..60357796be99 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -174,7 +174,6 @@ struct ax88179_data {
        u32 wol_supported;
        u32 wolopts;
        u8 disconnecting;
-       u8 initialized;
 };
 
 struct ax88179_int_data {
@@ -327,7 +326,8 @@ static void ax88179_status(struct usbnet *dev, struct urb *urb)
 
        if (netif_carrier_ok(dev->net) != link) {
                usbnet_link_change(dev, link, 1);
-               netdev_info(dev->net, "ax88179 - Link status is: %d\n", link);
+               if (!link)
+                       netdev_info(dev->net, "ax88179 - Link status is: %d\n", link);
        }
 }
 
@@ -1543,6 +1543,7 @@ static int ax88179_link_reset(struct usbnet *dev)
                         GMII_PHY_PHYSR, 2, &tmp16);
 
        if (!(tmp16 & GMII_PHY_PHYSR_LINK)) {
+               netdev_info(dev->net, "ax88179 - Link status is: 0\n");
                return 0;
        } else if (GMII_PHY_PHYSR_GIGA == (tmp16 & GMII_PHY_PHYSR_SMASK)) {
                mode |= AX_MEDIUM_GIGAMODE | AX_MEDIUM_EN_125MHZ;
@@ -1580,6 +1581,8 @@ static int ax88179_link_reset(struct usbnet *dev)
 
        netif_carrier_on(dev->net);
 
+       netdev_info(dev->net, "ax88179 - Link status is: 1\n");
+
        return 0;
 }
 
@@ -1678,12 +1681,21 @@ static int ax88179_reset(struct usbnet *dev)
 
 static int ax88179_net_reset(struct usbnet *dev)
 {
-       struct ax88179_data *ax179_data = dev->driver_priv;
+       u16 tmp16;
 
-       if (ax179_data->initialized)
+       ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID, GMII_PHY_PHYSR,
+                        2, &tmp16);
+       if (tmp16) {
+               ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+                                2, 2, &tmp16);
+               if (!(tmp16 & AX_MEDIUM_RECEIVE_EN)) {
+                       tmp16 |= AX_MEDIUM_RECEIVE_EN;
+                       ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+                                         2, 2, &tmp16);
+               }
+       } else {
                ax88179_reset(dev);
-       else
-               ax179_data->initialized = 1;
+       }
 
        return 0;
 }

Best regards
José Ignacio

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Yongqin Liu]

Hi, Jose

On Thu, 13 Jun 2024 at 17:59, Jose Ignacio Tornos Martinez
<jtornosm@redhat.com> wrote:
>
> Hello again,
>
> There was a problem copying the patch, sorry, here the good one:

Thanks very much for the work!

I will test it tomorrow, and let you know the result then.

Best regards,
Yongqin Liu
>
> $ git diff drivers/net/usb/ax88179_178a.c
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 51c295e1e823..60357796be99 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -174,7 +174,6 @@ struct ax88179_data {
>         u32 wol_supported;
>         u32 wolopts;
>         u8 disconnecting;
> -       u8 initialized;
>  };
>
>  struct ax88179_int_data {
> @@ -327,7 +326,8 @@ static void ax88179_status(struct usbnet *dev, struct urb *urb)
>
>         if (netif_carrier_ok(dev->net) != link) {
>                 usbnet_link_change(dev, link, 1);
> -               netdev_info(dev->net, "ax88179 - Link status is: %d\n", link);
> +               if (!link)
> +                       netdev_info(dev->net, "ax88179 - Link status is: %d\n", link);
>         }
>  }
>
> @@ -1543,6 +1543,7 @@ static int ax88179_link_reset(struct usbnet *dev)
>                          GMII_PHY_PHYSR, 2, &tmp16);
>
>         if (!(tmp16 & GMII_PHY_PHYSR_LINK)) {
> +               netdev_info(dev->net, "ax88179 - Link status is: 0\n");
>                 return 0;
>         } else if (GMII_PHY_PHYSR_GIGA == (tmp16 & GMII_PHY_PHYSR_SMASK)) {
>                 mode |= AX_MEDIUM_GIGAMODE | AX_MEDIUM_EN_125MHZ;
> @@ -1580,6 +1581,8 @@ static int ax88179_link_reset(struct usbnet *dev)
>
>         netif_carrier_on(dev->net);
>
> +       netdev_info(dev->net, "ax88179 - Link status is: 1\n");
> +
>         return 0;
>  }
>
> @@ -1678,12 +1681,21 @@ static int ax88179_reset(struct usbnet *dev)
>
>  static int ax88179_net_reset(struct usbnet *dev)
>  {
> -       struct ax88179_data *ax179_data = dev->driver_priv;
> +       u16 tmp16;
>
> -       if (ax179_data->initialized)
> +       ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID, GMII_PHY_PHYSR,
> +                        2, &tmp16);
> +       if (tmp16) {
> +               ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> +                                2, 2, &tmp16);
> +               if (!(tmp16 & AX_MEDIUM_RECEIVE_EN)) {
> +                       tmp16 |= AX_MEDIUM_RECEIVE_EN;
> +                       ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> +                                         2, 2, &tmp16);
> +               }
> +       } else {
>                 ax88179_reset(dev);
> -       else
> -               ax179_data->initialized = 1;
> +       }
>
>         return 0;
>  }
>
> Best regards
> José Ignacio
>


-- 
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [PATCH] net: usb: ax88179_178a: fix link status when link is set to down/up

[Yongqin Liu]

Hello, Jose

On Thu, 13 Jun 2024 at 19:46, Yongqin Liu <yongqin.liu@linaro.org> wrote:
>
> Hi, Jose
>
> On Thu, 13 Jun 2024 at 17:59, Jose Ignacio Tornos Martinez
> <jtornosm@redhat.com> wrote:
> >
> > Hello again,
> >
> > There was a problem copying the patch, sorry, here the good one:
>
> Thanks very much for the work!
>
> I will test it tomorrow, and let you know the result then.
>

I tested with the ACK android15-6.6 and the android-mainline branches,
which have the issue reported,
after applying this patch, the network works again now.

Here is the console output from the mainline branch, in case you want to check:
https://gist.github.com/liuyq/bd3fdada41411bc89a0cd4acf9ec11cf

Thanks again for all the help!

Best regards,
Yongqin Liu



> >
> > $ git diff drivers/net/usb/ax88179_178a.c
> > diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> > index 51c295e1e823..60357796be99 100644
> > --- a/drivers/net/usb/ax88179_178a.c
> > +++ b/drivers/net/usb/ax88179_178a.c
> > @@ -174,7 +174,6 @@ struct ax88179_data {
> >         u32 wol_supported;
> >         u32 wolopts;
> >         u8 disconnecting;
> > -       u8 initialized;
> >  };
> >
> >  struct ax88179_int_data {
> > @@ -327,7 +326,8 @@ static void ax88179_status(struct usbnet *dev, struct urb *urb)
> >
> >         if (netif_carrier_ok(dev->net) != link) {
> >                 usbnet_link_change(dev, link, 1);
> > -               netdev_info(dev->net, "ax88179 - Link status is: %d\n", link);
> > +               if (!link)
> > +                       netdev_info(dev->net, "ax88179 - Link status is: %d\n", link);
> >         }
> >  }
> >
> > @@ -1543,6 +1543,7 @@ static int ax88179_link_reset(struct usbnet *dev)
> >                          GMII_PHY_PHYSR, 2, &tmp16);
> >
> >         if (!(tmp16 & GMII_PHY_PHYSR_LINK)) {
> > +               netdev_info(dev->net, "ax88179 - Link status is: 0\n");
> >                 return 0;
> >         } else if (GMII_PHY_PHYSR_GIGA == (tmp16 & GMII_PHY_PHYSR_SMASK)) {
> >                 mode |= AX_MEDIUM_GIGAMODE | AX_MEDIUM_EN_125MHZ;
> > @@ -1580,6 +1581,8 @@ static int ax88179_link_reset(struct usbnet *dev)
> >
> >         netif_carrier_on(dev->net);
> >
> > +       netdev_info(dev->net, "ax88179 - Link status is: 1\n");
> > +
> >         return 0;
> >  }
> >
> > @@ -1678,12 +1681,21 @@ static int ax88179_reset(struct usbnet *dev)
> >
> >  static int ax88179_net_reset(struct usbnet *dev)
> >  {
> > -       struct ax88179_data *ax179_data = dev->driver_priv;
> > +       u16 tmp16;
> >
> > -       if (ax179_data->initialized)
> > +       ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID, GMII_PHY_PHYSR,
> > +                        2, &tmp16);
> > +       if (tmp16) {
> > +               ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> > +                                2, 2, &tmp16);
> > +               if (!(tmp16 & AX_MEDIUM_RECEIVE_EN)) {
> > +                       tmp16 |= AX_MEDIUM_RECEIVE_EN;
> > +                       ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> > +                                         2, 2, &tmp16);
> > +               }
> > +       } else {
> >                 ax88179_reset(dev);
> > -       else
> > -               ax179_data->initialized = 1;
> > +       }
> >
> >         return 0;
> >  }
> >
> > Best regards
> > José Ignacio
> >
>
>
> --
> Best Regards,
> Yongqin Liu
> ---------------------------------------------------------------
> #mailing list
> linaro-android@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-android



--
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android