Date: Thu, 30 Nov 2023 15:43:47 -0800
From: Douglas Anderson <dianders@...omium.org>
To: linux-usb@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Simon Horman <horms@...nel.org>,
	Grant Grundler <grundler@...omium.org>,
	Alan Stern <stern@...land.harvard.edu>,
	Hayes Wang <hayeswang@...ltek.com>,
	Bjørn Mork <bjorn@...k.no>,
	Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org,
	"David S . Miller" <davem@...emloft.net>,
	Brian Geffon <bgeffon@...gle.com>,
	Douglas Anderson <dianders@...omium.org>,
	Bastien Nocera <hadess@...ess.net>,
	Benjamin Tissoires <benjamin.tissoires@...hat.com>,
	Flavio Suligoi <f.suligoi@...m.it>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Ricardo Cañuelo <ricardo.canuelo@...labora.com>,
	Rob Herring <robh@...nel.org>,
	Roy Luo <royluo@...gle.com>,
	Stanley Chang <stanley_chang@...ltek.com>,
	Vincent Mailhol <mailhol.vincent@...adoo.fr>,
	linux-kernel@...r.kernel.org
Subject: [PATCH] usb: core: Save the config when a device is deauthorized+authorized

Right now, when a USB device is deauthorized (by writing 0 to the
"authorized" field in sysfs) and then reauthorized (by writing a 1) it
loses any configuration it might have had. This is because
usb_deauthorize_device() calls:
  usb_set_configuration(usb_dev, -1);
...and then usb_authorize_device() calls:
  usb_choose_configuration(udev);
...to choose the "best" configuration.

This generally works OK and it looks like the above design was chosen
on purpose. In commit 93993a0a3e52 ("usb: introduce
usb_authorize/deauthorize()") we can see some discussion about keeping
the old config but it was decided not to bother since we can't save it
for wireless USB anyway. It can be noted that as of commit
1e4c574225cc ("USB: Remove remnants of Wireless USB and UWB") wireless
USB is removed anyway, so there's really not a good reason not to keep
the old config.

Unfortunately, throwing away the old config breaks when something has
decided to choose a config other than the normal "best" config.
Specifically, it can be noted that as of commit ec51fbd1b8a2 ("r8152:
add USB device driver for config selection") that the r8152 driver
subclasses the generic USB driver and selects a config other than the
one that would have been selected by usb_choose_configuration(). This
logic isn't re-run after a deauthorize + authorize and results in the
r8152 driver not being re-bound.

Let's change things to save the old config when we deauthorize and
then restore it when we re-authorize. We'll disable this logic for
wireless USB where we re-fetch the descriptor after authorization.

Fixes: ec51fbd1b8a2 ("r8152: add USB device driver for config selection")
Signed-off-by: Douglas Anderson <dianders@...omium.org>
---
Although this seems to work for me and doesn't seem too terrible, I
could certainly imagine that USB folks might want something different
in terms of style or general approach. If there's some other way we
should be tackling this problem then please yell. I guess worst case
we could also revert the r8152 changes to subclass the generic USB
driver until this problem is solved, though that doesn't seem
wonderful.

 drivers/usb/core/hub.c     | 19 ++++++++++++++++---
 drivers/usb/core/message.c |  2 ++
 include/linux/usb.h        |  3 +++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b4584a0cd484..4afbbfa279ae 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2653,12 +2653,20 @@ int usb_new_device(struct usb_device *udev)
  */
 int usb_deauthorize_device(struct usb_device *usb_dev)
 {
+	int old_configuration;
+
 	usb_lock_device(usb_dev);
 	if (usb_dev->authorized == 0)
 		goto out_unauthorized;
 
+	/*
+	 * Keep the `saved_configuration` in a local since
+	 * usb_set_configuration() will clobber it.
+	 */
+	old_configuration = usb_dev->saved_configuration;
 	usb_dev->authorized = 0;
 	usb_set_configuration(usb_dev, -1);
+	usb_dev->saved_configuration = old_configuration;
 
 out_unauthorized:
 	usb_unlock_device(usb_dev);
@@ -2685,7 +2693,10 @@ int usb_authorize_device(struct usb_device *usb_dev)
 	/* Choose and set the configuration.  This registers the interfaces
 	 * with the driver core and lets interface drivers bind to them.
 	 */
-	c = usb_choose_configuration(usb_dev);
+	if (usb_dev->saved_configuration != -1)
+		c = usb_dev->saved_configuration;
+	else
+		c = usb_choose_configuration(usb_dev);
 	if (c >= 0) {
 		result = usb_set_configuration(usb_dev, c);
 		if (result) {
@@ -5077,10 +5088,12 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
 					retval);
 		goto fail;
 	}
-	if (initial)
+	if (initial) {
 		udev->descriptor = *descr;
-	else
+		udev->saved_configuration = -1;
+	} else {
 		*dev_descr = *descr;
+	}
 	kfree(descr);
 
 	/*
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 077dfe48d01c..015522068300 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1998,6 +1998,8 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 	struct usb_hcd *hcd = bus_to_hcd(dev->bus);
 	int n, nintf;
 
+	dev->saved_configuration = configuration;
+
 	if (dev->authorized == 0 || configuration == -1)
 		configuration = 0;
 	else {
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 8c61643acd49..6b989b8b2f01 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -644,6 +644,7 @@ struct usb3_lpm_parameters {
  *	parent->hub_delay + wHubDelay + tTPTransmissionDelay (40ns)
  *	Will be used as wValue for SetIsochDelay requests.
  * @use_generic_driver: ask driver core to reprobe using the generic driver.
+ * @saved_configuration: The last value passed to usb_set_configuration().
  *
  * Notes:
  * Usbcore drivers should not set usbdev->state directly.  Instead use
@@ -729,6 +730,8 @@ struct usb_device {
 
 	u16 hub_delay;
 	unsigned use_generic_driver:1;
+
+	int saved_configuration;
 };
 
 #define to_usb_device(__dev)	container_of_const(__dev, struct usb_device, dev)
-- 
2.43.0.rc2.451.g8631bc7472-goog

Powered by blists - more mailing lists