[questions] about usb_set_intfdata(intf, NULL) and if race is possible between ->disconnect() and ->suspend()

[questions] about usb_set_intfdata(intf, NULL) and if race is possible between ->disconnect() and ->suspend()

[Alexey Klimov]

For instance, let's say we have simple usb driver with:

static struct usb_driver usb_some_driver = {
	.name		= DRIVER_NAME,
	.probe		= yet_another_probe,
	.disconnect	= yet_another_disconnect,
	.suspend	= yet_another_suspend,
	.resume		= yet_another_resume,
	.reset_resume	= yet_another_resume,
	.id_table	= yet_another_device_table,
};

1. Can ->suspend and ->disconnect methods race?

Documentation/driver-api/usb/power-management.rst says:
This implies that external suspend/resume events are mutually exclusive
with calls to ``probe``, ``disconnect``, ``pre_reset``, and
``post_reset``;


Comment for usb_suspend_both() says that:
 * ...	Usbcore will insure that
 * method calls do not arrive during bind, unbind, or reset operations.
and that:
 * However drivers must be prepared to handle suspend calls arriving at
 * unpredictable times.

Also I was asked couple of years back what I am going to do if
disconnect() and suspend() will race, so seems never hurts to double-
check.

2. Do I need to usb_set_intfdata(intf, NULL) in disconnect method and
in probe() function if registration with another subsystem fails?
Like:
static int usb_streamlabs_wdt_probe(struct usb_interface *intf,
                                        const struct usb_device_id *id)
{
	...
	usb_set_intfdata(intf, &data);
	...
	retval = devm_subsystem_register_device(&intf->dev, ...);
	if (retval) {
		dev_err(&intf->dev, "error message\n");
		usb_set_intfdata(intf, NULL)
		return retval;
	}
}

I saw some patches that clear stale dev->driver_data pointer in
disconnect but doesn't seem that all usb drivers do that hence the
question.

Thanks,
Alexey

Re: [questions] about usb_set_intfdata(intf, NULL) and if race is possible between ->disconnect() and ->suspend()

[Alan Stern]

On Thu, Jul 14, 2022 at 02:40:03AM +0100, Alexey Klimov wrote:
> For instance, let's say we have simple usb driver with:
> 
> static struct usb_driver usb_some_driver = {
> 	.name		= DRIVER_NAME,
> 	.probe		= yet_another_probe,
> 	.disconnect	= yet_another_disconnect,
> 	.suspend	= yet_another_suspend,
> 	.resume		= yet_another_resume,
> 	.reset_resume	= yet_another_resume,
> 	.id_table	= yet_another_device_table,
> };
> 
> 1. Can ->suspend and ->disconnect methods race?

In short, no.  usb_unbind_interface() does usb_autoresume_device(), so 
it is guaranteed that the device will not go into autosuspend while the 
driver's disconnect method is running.

> Documentation/driver-api/usb/power-management.rst says:
> This implies that external suspend/resume events are mutually exclusive
> with calls to ``probe``, ``disconnect``, ``pre_reset``, and
> ``post_reset``;
> 
> 
> Comment for usb_suspend_both() says that:
>  * ...	Usbcore will insure that
>  * method calls do not arrive during bind, unbind, or reset operations.
> and that:
>  * However drivers must be prepared to handle suspend calls arriving at
>  * unpredictable times.
> 
> Also I was asked couple of years back what I am going to do if
> disconnect() and suspend() will race, so seems never hurts to double-
> check.
> 
> 2. Do I need to usb_set_intfdata(intf, NULL) in disconnect method and
> in probe() function if registration with another subsystem fails?

No.  usb_unbind_interface() does this call for you, and 
usb_probe_interface() does it if the probe method fails.

Alan Stern

> Like:
> static int usb_streamlabs_wdt_probe(struct usb_interface *intf,
>                                         const struct usb_device_id *id)
> {
> 	...
> 	usb_set_intfdata(intf, &data);
> 	...
> 	retval = devm_subsystem_register_device(&intf->dev, ...);
> 	if (retval) {
> 		dev_err(&intf->dev, "error message\n");
> 		usb_set_intfdata(intf, NULL)
> 		return retval;
> 	}
> }
> 
> I saw some patches that clear stale dev->driver_data pointer in
> disconnect but doesn't seem that all usb drivers do that hence the
> question.
> 
> Thanks,
> Alexey
> 
>