usb: core: Save the config when a device is deauthorized+authorized
Commit Message
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@chromium.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(-)
Comments
On Thu, Nov 30, 2023 at 03:43:47PM -0800, Douglas Anderson wrote:
> 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.
Would it be better to make the r8152 driver override
usb_choose_configuration()? This is the sort of thing that subclassing
is intended for.
Alan Stern
Hi,
On Fri, Dec 1, 2023 at 7:59 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, Nov 30, 2023 at 03:43:47PM -0800, Douglas Anderson wrote:
> > 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.
>
> Would it be better to make the r8152 driver override
> usb_choose_configuration()? This is the sort of thing that subclassing
> is intended for.
Yes, this is a nice solution. Posted.
https://lore.kernel.org/r/20231201183113.343256-1-dianders@chromium.org
@@ -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);
/*
@@ -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 {
@@ -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)