On 05.06.24 00:55, Dmitry wrote:
From: Dmitry <dimonija@xxxxxxxxx>
Sorry,
first thank you for the patch. It is good to see patches,
but that is a NACK.
Let me explain.
---
drivers/usb/serial/mos7840.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 8b0308d84270..7443d2f018b4 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -770,6 +770,7 @@ static void mos7840_close(struct usb_serial_port *port)
if (mos7840_port->write_urb_pool[j]) {
kfree(mos7840_port->write_urb_pool[j]->transfer_buffer);
usb_free_urb(mos7840_port->write_urb_pool[j]);
+ mos7840_port->write_urb_pool[j] = NULL;
Why is that needed? It is a good idea, but is it needed?
}
}
@@ -1737,6 +1738,37 @@ static void mos7840_port_remove(struct usb_serial_port *port)
kfree(mos7840_port);
}
+static int mos7840_suspend(struct usb_serial *serial, pm_message_t message)
+{
+ dev_dbg(&serial->interface->dev, "mos7840_suspend\n");
+
+ return 0;
+}
You are providing a suspend() method that does nothing.
If we look at usbserial:
int usb_serial_suspend(struct usb_interface *intf, pm_message_t message)
{
struct usb_serial *serial = usb_get_intfdata(intf);
int i, r;
/* suspend when called for first sibling interface */
if (serial->suspend_count++)
return 0;
/*
* serial->type->suspend() MUST return 0 in system sleep context,
* otherwise, the resume callback has to recover device from
* previous suspend failure.
*/
if (serial->type->suspend) {
r = serial->type->suspend(serial, message);
if (r < 0) {
serial->suspend_count--;
return r;
}
}
for (i = 0; i < serial->num_ports; ++i)
usb_serial_port_poison_urbs(serial->port[i]);
return 0;
}
EXPORT_SYMBOL(usb_serial_suspend);
There is no point in doing so.
+static int mos7840_resume(struct usb_serial *serial)
+{
+ struct moschip_port *mos7840_port;
+ struct usb_serial_port *port;
+ int i;
+ dev_dbg(&serial->interface->dev, "mos7840_resume\n");
+
+ for (i = 0; i < serial->num_ports; ++i) {
+ port = serial->port[i];
+ if (!tty_port_initialized(&port->port))
+ continue;
+
+ mos7840_port = usb_get_serial_port_data(port);
+
+ if (port->bulk_in_size)
+ usb_submit_urb(mos7840_port->read_urb, GFP_NOIO);
This can fail.
And much worse, if this is necessary, the generic usbserial code does not handle
the resubmission of the read URB. If that is the case, it will not be killed if
the system suspends. That means if the suspend run fails further down the chain,
you will resubmit a running URB.
And, worse, there will be IO to a suspended device, which is very bad
and can corrupt memory with S4.
If you need to manage the URB in the individual driver you need to handle
it both upon suspend() and resume(). Hence you need some like the attached
patch in addition to your patch.
Regards
Oliver
From 48b9e8cac49760877b153a2b0bedb0c34a0460fa Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@xxxxxxxx>
Date: Wed, 5 Jun 2024 14:11:49 +0200
Subject: [PATCH] usb: serial: Implement suspend for mos7840
The read URB needs to be killed before we suspend.
Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
---
drivers/usb/serial/mos7840.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 7443d2f018b4..75cf33deb02a 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -1740,9 +1740,21 @@ static void mos7840_port_remove(struct usb_serial_port *port)
static int mos7840_suspend(struct usb_serial *serial, pm_message_t message)
{
- dev_dbg(&serial->interface->dev, "mos7840_suspend\n");
+ int i;
+ struct moschip_port *mos7840_port;
+ struct usb_serial_port *port;
+ dev_dbg(&serial->interface->dev, "mos7840_suspend\n");
- return 0;
+ for (i = 0; i < serial->num_ports; ++i) {
+ port = serial->port[i];
+ if (!tty_port_initialized(&port->port))
+ continue;
+
+ mos7840_port = usb_get_serial_port_data(port);
+
+ usb_kill_urb(mos7840_port->read_urb);
+ }
+ return 0;
}
static int mos7840_resume(struct usb_serial *serial)
@@ -1766,7 +1778,7 @@ static int mos7840_resume(struct usb_serial *serial)
usb_submit_urb(mos7840_port->read_urb, GFP_ATOMIC);*/
}
- return 0;
+ return 0;
}
static struct usb_serial_driver moschip7840_4port_device = {
--
2.45.1
