| Message ID | 20231130154337.1.Ie00e07f07f87149c9ce0b27ae4e26991d307e14b@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: core: Save the config when a device is deauthorized+authorized | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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
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)
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(-)