[PATCH] net: usbnet: skip info->stop() callback if suspend_count is not 0

[PATCH] net: usbnet: skip info->stop() callback if suspend_count is not 0

[Xu Yang]

Once suspend_count is greater than 0, it means the USB device is
already suspended. Before calling usbnet_stop(), normally the USB
device should be in working state or already be resumed. If not,
the USB device may be already disconnected. In this case, we should
skip info->stop() callback to avoid abnormal behaviors.

Closes: https://lore.kernel.org/netdev/20250806083017.3289300-1-xu.yang_2@nxp.com/
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/net/usb/usbnet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 511c4154cf74..27642b76a3eb 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,
-- 
2.34.1

Re: [PATCH] net: usbnet: skip info->stop() callback if suspend_count is not 0

[Oliver Neukum]

On 8/18/25 09:57, Xu Yang wrote:

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

This needs to fail ...

>   	/* allow minidriver to stop correctly (wireless devices to turn off
>   	 * radio etc) */
> -	if (info->stop) {
> +	if (info->stop && !dev->suspend_count) {

... for !dev->suspend_count to be false

>   		retval = info->stop(dev);
>   		if (retval < 0)
>   			netif_info(dev, ifdown, dev->net,

In other words, this means that the driver has insufficient
error handling in this method. This needs to be fixed and it
needs to be fixed explicitly. We do not hide error handling.

Please use a literal "if (pm < 0)" to skip the parts we need to skip
if the resumption fails.

	Regards
		Oliver

NACKED-BY: Oliver Neukum <oneukum@suse.com>

Re: [PATCH] net: usbnet: skip info->stop() callback if suspend_count is not 0

[Xu Yang]

On Mon, Aug 18, 2025 at 11:04:55AM +0200, Oliver Neukum wrote:
> On 8/18/25 09:57, Xu Yang wrote:
> 
> > --- 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);
> 
> This needs to fail ...

It returns 0, so "pm = 0" here which means it succeed.

> 
> >   	/* allow minidriver to stop correctly (wireless devices to turn off
> >   	 * radio etc) */
> > -	if (info->stop) {
> > +	if (info->stop && !dev->suspend_count) {
> 
> ... for !dev->suspend_count to be false

The suspend_count won't go to 0 because there is no chance to call
usbnet_resume() if the USB device is physically disconnected from the 
USB port during system suspend.

> 
> >   		retval = info->stop(dev);
> >   		if (retval < 0)
> >   			netif_info(dev, ifdown, dev->net,
> 
> In other words, this means that the driver has insufficient
> error handling in this method. This needs to be fixed and it
> needs to be fixed explicitly. We do not hide error handling.

Do you mean info->stop() should be called and return error code if something
is wrong?

> 
> Please use a literal "if (pm < 0)" to skip the parts we need to skip
> if the resumption fails.

pm = 0 here.

Thanks,
Xu Yang

> 
> 	Regards
> 		Oliver
> 
> NACKED-BY: Oliver Neukum <oneukum@suse.com>

Re: [PATCH] net: usbnet: skip info->stop() callback if suspend_count is not 0

[Xu Yang]

Hi Oliver,

Is this mail been missed?

On Thu, Aug 21, 2025 at 11:00:46AM +0800, Xu Yang wrote:
> On Mon, Aug 18, 2025 at 11:04:55AM +0200, Oliver Neukum wrote:
> > On 8/18/25 09:57, Xu Yang wrote:
> > 
> > > --- 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);
> > 
> > This needs to fail ...
> 
> It returns 0, so "pm = 0" here which means it succeed.
> 
> > 
> > >   	/* allow minidriver to stop correctly (wireless devices to turn off
> > >   	 * radio etc) */
> > > -	if (info->stop) {
> > > +	if (info->stop && !dev->suspend_count) {
> > 
> > ... for !dev->suspend_count to be false
> 
> The suspend_count won't go to 0 because there is no chance to call
> usbnet_resume() if the USB device is physically disconnected from the 
> USB port during system suspend.
> 
> > 
> > >   		retval = info->stop(dev);
> > >   		if (retval < 0)
> > >   			netif_info(dev, ifdown, dev->net,
> > 
> > In other words, this means that the driver has insufficient
> > error handling in this method. This needs to be fixed and it
> > needs to be fixed explicitly. We do not hide error handling.
> 
> Do you mean info->stop() should be called and return error code if something
> is wrong?

Do you mean this way? Or do you have any other suggestions?

Thanks,
Xu Yang

> 
> > 
> > Please use a literal "if (pm < 0)" to skip the parts we need to skip
> > if the resumption fails.
> 
> pm = 0 here.
> 
> Thanks,
> Xu Yang
> 
> > 
> > 	Regards
> > 		Oliver
> > 
> > NACKED-BY: Oliver Neukum <oneukum@suse.com>

Re: [PATCH] net: usbnet: skip info->stop() callback if suspend_count is not 0

[Oliver Neukum]

On 9/4/25 09:26, Xu Yang wrote:
>>>    	/* allow minidriver to stop correctly (wireless devices to turn off
>>>    	 * radio etc) */
>>> -	if (info->stop) {
>>> +	if (info->stop && !dev->suspend_count) {
>> ... for !dev->suspend_count to be false
> The suspend_count won't go to 0 because there is no chance to call
> usbnet_resume() if the USB device is physically disconnected from the
> USB port during system suspend.

Sorry for the delay.

While you are correct that a physical disconnect
will make the PM call fail, you cannot assume that
a physical disconnect is the only reason disconnect()
would be called.
It would also be called if

- the module is unloaded
- usbfs is used to disconnect the driver from the device

Hence it seems to me that using suspend_count is false.
You need to use the return of the PM operation.

	Regards
		Oliver