netdev - Re: [PATCH] net: usb: asix: avoid to call phylink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c3e7m63qcff6dazjzualk7v2n3jtxujl43ynw7jtfuf34njt6w@5sml5vvq57gh>
Date: Fri, 15 Aug 2025 16:15:09 +0800
From: Xu Yang <xu.yang_2@....com>
To: Andrew Lunn <andrew@...n.ch>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, max.schulze@...ine.de, khalasa@...p.pl, 
	o.rempel@...gutronix.de, linux-usb@...r.kernel.org, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, imx@...ts.linux.dev, jun.li@....com
Subject: Re: [PATCH] net: usb: asix: avoid to call phylink_stop() a second
 time

On Thu, Aug 07, 2025 at 12:03:39PM +0800, Xu Yang wrote:
> Hi Andrew,
> 
> Thanks for your comments!
> 
> On Wed, Aug 06, 2025 at 05:58:18PM +0200, Andrew Lunn wrote:
> > On Wed, Aug 06, 2025 at 04:30:17PM +0800, Xu Yang wrote:
> > > The kernel will have below dump when system resume if the USB net device
> > > was already disconnected during system suspend.
> > 
> > By disconnected, you mean pulled out?
> 
> Yes.
> 
> > 
> > > It's because usb_resume_interface() will be skipped if the USB core found
> > > the USB device was already disconnected. In this case, asix_resume() will
> > > not be called anymore. So asix_suspend/resume() can't be balanced. When
> > > ax88772_stop() is called, the phy device was already stopped. To avoid
> > > calling phylink_stop() a second time, check whether usb net device is
> > > already in suspend state.
> > > 
> > > Fixes: e0bffe3e6894 ("net: asix: ax88772: migrate to phylink")
> > > Cc: stable@...r.kernel.org
> > > Signed-off-by: Xu Yang <xu.yang_2@....com>
> > > ---
> > >  drivers/net/usb/asix_devices.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> > > index 9b0318fb50b5..ac28f5fe7ac2 100644
> > > --- a/drivers/net/usb/asix_devices.c
> > > +++ b/drivers/net/usb/asix_devices.c
> > > @@ -932,7 +932,8 @@ static int ax88772_stop(struct usbnet *dev)
> > >  {
> > >  	struct asix_common_private *priv = dev->driver_priv;
> > >  
> > > -	phylink_stop(priv->phylink);
> > > +	if (!dev->suspend_count)
> > > +		phylink_stop(priv->phylink);
> > 
> > Looking at ax88172a.c, lan78xx.c and smsc95xx.c, they don't have
> > anything like this. Is asix special, or are all the others broken as
> > well?
> 
> I have limited USB net devices. So I can't test others now.
> 
> But based on the error path, only below driver call phy_stop() or phylink_stop()
> in their stop() callback:
> 
> drivers/net/usb/asix_devices.c
>   ax88772_stop()
>     phylink_stop()
> 
> drivers/net/usb/ax88172a.c
>   ax88172a_stop()
>     phy_stop()
> 
> drivers/net/usb/lan78xx.c
>   lan78xx_stop()
>     phylink_stop()
> 
> drivers/net/usb/smsc95xx.c
>   smsc95xx_stop()
>     phy_stop()
> 
> However, only asix_devices.c and lan78xx.c call phylink_suspend() in suspend()
> callback. So I think lan78xx.c has this issue too.
> 
> Should I change usbnet common code like below?
> 
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index c39dfa17813a..44a8d325dfb1 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -839,7 +839,7 @@ int usbnet_stop (struct net_device *net)
>         pm = usb_autopm_get_interface(dev->intf);
>         /* allow minidriver to stop correctly (wireless devices to turn off
>          * radio etc) */
> -       if (info->stop) {
> +       if (info->stop && !dev->suspend_count) {
>                 retval = info->stop(dev);
>                 if (retval < 0)
>                         netif_info(dev, ifdown, dev->net,

Do you mind sharing some suggestions on this? Thanks in advance!

Thanks,
Xu Yang

> 
> Thanks,
> Xu Yang
> 
> > 
> > 	Andrew