diff mbox series

[RFC,v1] hack: suspend: usb: option: add reset_resume callback

Message ID 20221130121552.1560379-1-martin.kepplinger@puri.sm
State New
Headers show
Series [RFC,v1] hack: suspend: usb: option: add reset_resume callback | expand

Commit Message

Martin Kepplinger Nov. 30, 2022, 12:15 p.m. UTC 
Currently to be the same as resume(). This will just avoid re-enumeration
of the modem device on every system resume.

This exists only because the usb core will re-enumerate *any* device
whos' driver doesn't have reset_resume() implemented. A call trace:

Jun 23 11:15:43 pureos kernel:  usb_serial_disconnect+0x58/0x180 [usbserial]
Jun 23 11:15:43 pureos kernel:  usb_unbind_interface+0x84/0x290 [usbcore]
Jun 23 11:15:43 pureos kernel:  device_remove+0x78/0x90
Jun 23 11:15:43 pureos kernel:  device_release_driver_internal+0x1e4/0x250
Jun 23 11:15:43 pureos kernel:  device_release_driver+0x24/0x30
Jun 23 11:15:43 pureos kernel:  usb_forced_unbind_intf+0xac/0xd4 [usbcore]
Jun 23 11:15:43 pureos kernel:  unbind_marked_interfaces.isra.0+0x5c/0x80 [usbcore]
Jun 23 11:15:43 pureos kernel:  usb_resume+0x78/0x8c [usbcore]
Jun 23 11:15:43 pureos kernel:  usb_dev_resume+0x20/0x30 [usbcore]
Jun 23 11:15:43 pureos kernel:  dpm_run_callback+0x60/0x1f0
Jun 23 11:15:43 pureos kernel:  device_resume+0x9c/0x1f4

where usb_resume_inteface() sets needs_binding here
https://elixir.bootlin.com/linux/latest/source/drivers/usb/core/driver.c#L1353
because of no reset_resume() implementation.

This hack is even suggested in some modems' application notes as can
be found in discussion here
https://invent.kde.org/teams/plasma-mobile/issues/-/issues/3#note_218386

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 drivers/usb/serial/option.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Oliver Neukum Nov. 30, 2022, 2:04 p.m. UTC | #1 
On 30.11.22 13:15, Martin Kepplinger wrote:
> Currently to be the same as resume(). This will just avoid re-enumeration
> of the modem device on every system resume.
> 
> This exists only because the usb core will re-enumerate *any* device
> whos' driver doesn't have reset_resume() implemented. A call trace:

Hi,

this is deeply problematic. Strictly speaking it introduces a bug.
By implementing reset_resume() you are telling the rest of the system
that all settings have survived the hibernation. That is likely wrong in
your case.

	Regards
		Oliver
diff mbox series

Patch

diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index dee79c7d82d5..58526bf8684d 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -46,6 +46,7 @@  static int  option_probe(struct usb_serial *serial,
 static int option_attach(struct usb_serial *serial);
 static void option_release(struct usb_serial *serial);
 static void option_instat_callback(struct urb *urb);
+static int option_reset_resume(struct usb_serial *serial);
 
 /* Vendor and product IDs */
 #define OPTION_VENDOR_ID			0x0AF0
@@ -2227,6 +2228,7 @@  static struct usb_serial_driver option_1port_device = {
 #ifdef CONFIG_PM
 	.suspend           = usb_wwan_suspend,
 	.resume            = usb_wwan_resume,
+	.reset_resume      = option_reset_resume,
 #endif
 };
 
@@ -2373,6 +2375,18 @@  static void option_instat_callback(struct urb *urb)
 	}
 }
 
+static int option_reset_resume(struct usb_serial *serial)
+{
+	/*
+	 * We simply call resume() because this implementation only
+	 * exists because the USB core will un- and rebind any driver
+	 * during system resume that does *not* have a reset_resume()
+	 * implementation; see usb_resume_interface() in core/driver.c
+	 * We want to avoid that unconditional removal/addition.
+	 */
+	return usb_wwan_resume(serial);
+}
+
 MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL v2");