git: 36a80f426435 - main - usb: serial: make more commands execute synchronously

The branch main has been updated by kevans:

URL: https://cgit.FreeBSD.org/src/commit/?id=36a80f4264350a2f4f0686eb91ae7f5943d40327

commit 36a80f4264350a2f4f0686eb91ae7f5943d40327
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2024-12-11 01:23:10 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2024-12-11 01:23:41 +0000

    usb: serial: make more commands execute synchronously
    
    The termios layer wants some level of guarantee that we've actually
    submitted param changes to the hardware when our functions return, so we
    need to do a little more waiting to avoid violating those guarantees.
    
    This is especially important as some hardware has some minimum timing
    specifications around this stuff, and without being less asynchronous
    the software dealing with these devices can't reasonably operate the
    hardware without more excessive delays than they should need.
    
    More specifically, we make sure that:
     - The command to start transfers is finished before we toggle DTR/RTS
     - The status_change command finishes before we return, which may change
        some fields in the softc that we need for a subsequent call into
        usb_serial
     - cfg_param finishes before we re-enable transfers, and we ensure that
        RTS is appropriately toggled before we return to userland
    
    This has been observed to fix some flakiness in connecting to some
    ESP32 devices.
    
    Tested by:      kenrap from Libera
    Reviewed by:    imp, kib
    Differential Revision:  https://reviews.freebsd.org/D47952
---
 sys/dev/usb/serial/usb_serial.c | 69 +++++++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 13 deletions(-)

diff --git a/sys/dev/usb/serial/usb_serial.c b/sys/dev/usb/serial/usb_serial.c
index 30a0c6203086..dfae051ecfd6 100644
--- a/sys/dev/usb/serial/usb_serial.c
+++ b/sys/dev/usb/serial/usb_serial.c
@@ -153,7 +153,7 @@ static int	ucom_attach_tty(struct ucom_super_softc *, struct ucom_softc *);
 static void	ucom_detach_tty(struct ucom_super_softc *, struct ucom_softc *);
 static int	ucom_queue_command(struct ucom_softc *,
 		    usb_proc_callback_t *, struct termios *pt,
-		    struct usb_proc_msg *t0, struct usb_proc_msg *t1);
+		    struct usb_proc_msg *t0, struct usb_proc_msg *t1, bool wait);
 static void	ucom_shutdown(struct ucom_softc *);
 static void	ucom_ring(struct ucom_softc *, uint8_t);
 static void	ucom_break(struct ucom_softc *, uint8_t);
@@ -592,10 +592,43 @@ ucom_set_usb_mode(struct ucom_super_softc *ssc, enum usb_hc_mode usb_mode)
 	}
 }
 
+static void
+ucom_command_barrier_cb(struct usb_proc_msg *msg __unused)
+{
+	/* NOP */
+}
+
+/*
+ * ucom_command_barrier inserts a dummy task and waits for it so that we can be
+ * certain that previously enqueued tasks are finished before returning back to
+ * the tty layer.
+ */
+static int
+ucom_command_barrier(struct ucom_softc *sc)
+{
+	struct ucom_super_softc *ssc = sc->sc_super;
+	struct usb_proc_msg dummy = { .pm_callback = ucom_command_barrier_cb };
+	struct usb_proc_msg *task;
+	int error;
+
+	UCOM_MTX_ASSERT(sc, MA_OWNED);
+
+	if (usb_proc_is_gone(&ssc->sc_tq)) {
+		DPRINTF("proc is gone\n");
+		return (ENXIO);         /* nothing to do */
+	}
+
+	task = usb_proc_msignal(&ssc->sc_tq, &dummy, &dummy);
+	error = usb_proc_mwait_sig(&ssc->sc_tq, task, task);
+	if (error == 0 && sc->sc_tty != NULL && tty_gone(sc->sc_tty))
+		error = ENXIO;
+	return (error);
+}
+
 static int
 ucom_queue_command(struct ucom_softc *sc,
     usb_proc_callback_t *fn, struct termios *pt,
-    struct usb_proc_msg *t0, struct usb_proc_msg *t1)
+    struct usb_proc_msg *t0, struct usb_proc_msg *t1, bool wait)
 {
 	struct ucom_super_softc *ssc = sc->sc_super;
 	struct ucom_param_task *task;
@@ -629,7 +662,7 @@ ucom_queue_command(struct ucom_softc *sc,
 	/*
 	 * Closing or opening the device should be synchronous.
 	 */
-	if (fn == ucom_cfg_close || fn == ucom_cfg_open) {
+	if (wait) {
 		error = usb_proc_mwait_sig(&ssc->sc_tq, t0, t1);
 
 		/* usb_proc_mwait_sig may have dropped the tty lock. */
@@ -793,14 +826,17 @@ ucom_open(struct tty *tp)
 
 	error = ucom_queue_command(sc, ucom_cfg_open, NULL,
 	    &sc->sc_open_task[0].hdr,
-	    &sc->sc_open_task[1].hdr);
+	    &sc->sc_open_task[1].hdr, true);
 	if (error != 0)
 		goto out;
 
-	/* Queue transfer enable command last */
+	/*
+	 * Queue transfer enable command last, we'll have a barrier later so we
+	 * don't need to wait on this to complete specifically.
+	 */
 	error = ucom_queue_command(sc, ucom_cfg_start_transfers, NULL,
 	    &sc->sc_start_task[0].hdr,
-	    &sc->sc_start_task[1].hdr);
+	    &sc->sc_start_task[1].hdr, true);
 	if (error != 0)
 		goto out;
 
@@ -813,6 +849,7 @@ ucom_open(struct tty *tp)
 
 	ucom_status_change(sc);
 
+	error = ucom_command_barrier(sc);
 out:
 	return (error);
 }
@@ -852,7 +889,7 @@ ucom_close(struct tty *tp)
 
 	(void)ucom_queue_command(sc, ucom_cfg_close, NULL,
 	    &sc->sc_close_task[0].hdr,
-	    &sc->sc_close_task[1].hdr);
+	    &sc->sc_close_task[1].hdr, true);
 
 	sc->sc_flag &= ~(UCOM_FLAG_HL_READY | UCOM_FLAG_RTS_IFLOW);
 
@@ -933,11 +970,15 @@ ucom_ioctl(struct tty *tp, u_long cmd, caddr_t data, struct thread *td)
 #endif
 	case TIOCSBRK:
 		ucom_break(sc, 1);
-		error = 0;
+		error = ucom_command_barrier(sc);
+		if (error == ENXIO)
+			error = ENODEV;
 		break;
 	case TIOCCBRK:
 		ucom_break(sc, 0);
-		error = 0;
+		error = ucom_command_barrier(sc);
+		if (error == ENXIO)
+			error = ENODEV;
 		break;
 	default:
 		if (sc->sc_callback->ucom_ioctl) {
@@ -1097,7 +1138,7 @@ ucom_line_state(struct ucom_softc *sc,
 	 */
 	(void)ucom_queue_command(sc, ucom_cfg_line_state, NULL,
 	    &sc->sc_line_state_task[0].hdr,
-	    &sc->sc_line_state_task[1].hdr);
+	    &sc->sc_line_state_task[1].hdr, false);
 }
 
 static void
@@ -1255,7 +1296,7 @@ ucom_status_change(struct ucom_softc *sc)
 
 	(void)ucom_queue_command(sc, ucom_cfg_status_change, NULL,
 	    &sc->sc_status_task[0].hdr,
-	    &sc->sc_status_task[1].hdr);
+	    &sc->sc_status_task[1].hdr, true);
 }
 
 static void
@@ -1329,14 +1370,14 @@ ucom_param(struct tty *tp, struct termios *t)
 	/* Queue baud rate programming command first */
 	error = ucom_queue_command(sc, ucom_cfg_param, t,
 	    &sc->sc_param_task[0].hdr,
-	    &sc->sc_param_task[1].hdr);
+	    &sc->sc_param_task[1].hdr, true);
 	if (error != 0)
 		goto done;
 
 	/* Queue transfer enable command last */
 	error = ucom_queue_command(sc, ucom_cfg_start_transfers, NULL,
 	    &sc->sc_start_task[0].hdr,
-	    &sc->sc_start_task[1].hdr);
+	    &sc->sc_start_task[1].hdr, true);
 	if (error != 0)
 		goto done;
 
@@ -1346,6 +1387,8 @@ ucom_param(struct tty *tp, struct termios *t)
 		sc->sc_flag &= ~UCOM_FLAG_RTS_IFLOW;
 		ucom_modem(tp, SER_RTS, 0);
 	}
+
+	error = ucom_command_barrier(sc);
 done:
 	if (error) {
 		if (opened) {