[PATCH] USB: document requirements of usb_set_interface on disconnection

[PATCH] USB: document requirements of usb_set_interface on disconnection

[Michal Pecio]

A call to usb_set_interface() crashes if the device is deallocated
concurrently, such as due to physical removal or serious IO error.
It could also interfere with other drivers using the device if the
current driver is unbound before the call is done.

Document the need to delay driver unbinding until this call returns,
which solves both issues.

Explicitly mention finishing pending operations in the documentation
of the driver disconnect callback.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 Documentation/driver-api/usb/callbacks.rst | 4 +++-
 drivers/usb/core/message.c                 | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/driver-api/usb/callbacks.rst b/Documentation/driver-api/usb/callbacks.rst
index 2b80cf54bcc3..c7fa55375e9a 100644
--- a/Documentation/driver-api/usb/callbacks.rst
+++ b/Documentation/driver-api/usb/callbacks.rst
@@ -100,7 +100,9 @@ This callback is a signal to break any connection with an interface.
 You are not allowed any IO to a device after returning from this
 callback. You also may not do any other operation that may interfere
 with another driver bound the interface, eg. a power management
-operation.
+operation. Outstanding operations on the device must complete or be
+aborted before this callback returns.
+
 If you are called due to a physical disconnection, all your URBs will be
 killed by usbcore. Note that in this case disconnect will be called some
 time after the physical disconnection. Thus your driver must be prepared
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 077dfe48d01c..08acebd51823 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1516,7 +1516,8 @@ void usb_enable_interface(struct usb_device *dev,
  * This call is synchronous, and may not be used in an interrupt context.
  * Also, drivers must not change altsettings while urbs are scheduled for
  * endpoints in that interface; all such urbs must first be completed
- * (perhaps forced by unlinking).
+ * (perhaps forced by unlinking). If a thread in your driver uses this call,
+ * make sure your disconnect() method can wait for it to complete.
  *
  * Return: Zero on success, or else the status code returned by the
  * underlying usb_control_msg() call.
-- 
2.43.0

Re: [PATCH] USB: document requirements of usb_set_interface on disconnection

[Alan Stern]

On Sun, Feb 18, 2024 at 12:54:36AM +0100, Michal Pecio wrote:
> A call to usb_set_interface() crashes if the device is deallocated
> concurrently, such as due to physical removal or serious IO error.
> It could also interfere with other drivers using the device if the
> current driver is unbound before the call is done.
> 
> Document the need to delay driver unbinding until this call returns,
> which solves both issues.
> 
> Explicitly mention finishing pending operations in the documentation
> of the driver disconnect callback.
> 
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> ---

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

>  Documentation/driver-api/usb/callbacks.rst | 4 +++-
>  drivers/usb/core/message.c                 | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/driver-api/usb/callbacks.rst b/Documentation/driver-api/usb/callbacks.rst
> index 2b80cf54bcc3..c7fa55375e9a 100644
> --- a/Documentation/driver-api/usb/callbacks.rst
> +++ b/Documentation/driver-api/usb/callbacks.rst
> @@ -100,7 +100,9 @@ This callback is a signal to break any connection with an interface.
>  You are not allowed any IO to a device after returning from this
>  callback. You also may not do any other operation that may interfere
>  with another driver bound the interface, eg. a power management
> -operation.
> +operation. Outstanding operations on the device must complete or be
> +aborted before this callback returns.
> +
>  If you are called due to a physical disconnection, all your URBs will be
>  killed by usbcore. Note that in this case disconnect will be called some
>  time after the physical disconnection. Thus your driver must be prepared
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 077dfe48d01c..08acebd51823 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1516,7 +1516,8 @@ void usb_enable_interface(struct usb_device *dev,
>   * This call is synchronous, and may not be used in an interrupt context.
>   * Also, drivers must not change altsettings while urbs are scheduled for
>   * endpoints in that interface; all such urbs must first be completed
> - * (perhaps forced by unlinking).
> + * (perhaps forced by unlinking). If a thread in your driver uses this call,
> + * make sure your disconnect() method can wait for it to complete.
>   *
>   * Return: Zero on success, or else the status code returned by the
>   * underlying usb_control_msg() call.
> -- 
> 2.43.0

[PATCH v2] USB: document some API requirements on disconnection

[Michal Pecio]

A call to usb_set_interface() crashes if the device is deallocated
concurrently, such as due to physical removal or a serious IO error.
It could also interfere with other drivers using the device if the
current driver is unbound before the call is finished.

Document the need to delay driver unbinding until this call returns,
which solves both issues.

Document the same regarding usb_clear_halt(), which is equally known
to be routinely called by drivers.

Explicitly mention finishing pending operations in the documentation
of the driver disconnect callback.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---

v2 changes:
- I noticed and fixed a typo in existing documentation near my change
- reworded the callbacks.rst change to focus on actions, not conditions
- I looked for drivers calling other functions from message.c and found
  that usb_clear_halt() is in widespread use and has the same needs


 Documentation/driver-api/usb/callbacks.rst | 6 ++++--
 drivers/usb/core/message.c                 | 5 ++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/usb/callbacks.rst b/Documentation/driver-api/usb/callbacks.rst
index 2b80cf54bcc3..927da49b8f00 100644
--- a/Documentation/driver-api/usb/callbacks.rst
+++ b/Documentation/driver-api/usb/callbacks.rst
@@ -99,8 +99,10 @@ The disconnect() callback
 This callback is a signal to break any connection with an interface.
 You are not allowed any IO to a device after returning from this
 callback. You also may not do any other operation that may interfere
-with another driver bound the interface, eg. a power management
-operation.
+with another driver bound to the interface, eg. a power management
+operation. Outstanding operations on the device must be completed or
+aborted before this callback may return.
+
 If you are called due to a physical disconnection, all your URBs will be
 killed by usbcore. Note that in this case disconnect will be called some
 time after the physical disconnection. Thus your driver must be prepared
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 077dfe48d01c..dc7c8c258870 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1198,6 +1198,8 @@ EXPORT_SYMBOL_GPL(usb_get_status);
  * same status code used to report a true stall.
  *
  * This call is synchronous, and may not be used in an interrupt context.
+ * If a thread in your driver uses this call, make sure your disconnect()
+ * method can wait for it to complete.
  *
  * Return: Zero on success, or else the status code returned by the
  * underlying usb_control_msg() call.
@@ -1516,7 +1518,8 @@ void usb_enable_interface(struct usb_device *dev,
  * This call is synchronous, and may not be used in an interrupt context.
  * Also, drivers must not change altsettings while urbs are scheduled for
  * endpoints in that interface; all such urbs must first be completed
- * (perhaps forced by unlinking).
+ * (perhaps forced by unlinking). If a thread in your driver uses this call,
+ * make sure your disconnect() method can wait for it to complete.
  *
  * Return: Zero on success, or else the status code returned by the
  * underlying usb_control_msg() call.
-- 
2.43.0