From nobody Wed Dec 11 01:24:49 2024 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Y7Hvj4wzCz5g8MW; Wed, 11 Dec 2024 01:24:49 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Y7Hvj3WH5z3xcV; Wed, 11 Dec 2024 01:24:49 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1733880289; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=aUiweBJLG0+TaHKPCfXYryh4ImCJv8jduWpl+MWioso=; b=LPST/vz8AckybF2qTs64ziIO65nrgMGzVgvIzOpsh2rnu0JfDvrJ/Rfv12cc0ZHFCc/T26 Po4Bjj30a72mkrE8FIic3SF4x83o9vEUr4sQKA3UUVB4QAYaET1xQT6jHTQpDADbzxSW5i IWbcCd+zrHEBfTSlywIz3QjgtQjFKeiPmHYrDj3SRzSzFV8LUd3Kpm96TaVJKv5fPK6wL4 claFUzjJr0Tv/NdZrt45vVUwXbQUAa5rMdxzRUX2540v6KkZ+dDdVMX5v6CFNiTzS+bksi I2HPsbVL6cHLea27r+AB8ga/Cle0nSbP3FyVT3RMnBnuPkvvKmGkcYspZ5wuNA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1733880289; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=aUiweBJLG0+TaHKPCfXYryh4ImCJv8jduWpl+MWioso=; b=Gt0ZyLAskgwmHaTcr8zpOIKxQrIvOmqLSjEF2dLaKnxqPhQOeif5mjRMet44TltFY7LdzT dr3bT0D2+6QkpCvi4eJVim0Gbz3eNdDAlVFw+D9xUMyGweBYbV2QQARXETen0bKBDbcxNJ 02i1SRmZiAB+p95EAbUkkXto9zVgDddgYs2ogv08eDY2TnBazXNHncLA7pwzyqW8xBuclI kBoSICigNUGBSPtOugGbCrKQJC+Nlg4DssLQ+JhvewrO8hlb+8qH0Bzz419H1A3qtSEEXD 8CrQdDwxnhLnSkVaJ47b4w2nSdGjqdnfiq4yOYaTPshhncYRZcPvz/U8JRpHdA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1733880289; a=rsa-sha256; cv=none; b=H3D1HeX5icsfVwF926cNCIfeF8FIldZa/giQe5Lnd69gTDejmVXrIvolGZ3AhOtn6IAo2Y tSQMIYGal5MfoPQ7anRXYAVPSDe5G7Y+4B60rFThvprRlOV3D6E0O7QTHHA7WVgBGMawa2 TPQq3v18Hd/K7cgsNkdTPgvSTc+cyVf7Jjwac7iQWMw0T13TBalfytI55rLMqWyD0T4xBO KA54XPGcHVq0QCrnjESY2b8jtpQu1Jgxbp/DNs+zsD2f3o24XU7DaH3zekQ4FdBTiaegmg s2Ux4RelrV1lFt9ggriPVYTRUWDuriI8Wqyqph9+tMfgGXp87n5qB7TqbFlm+w== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4Y7Hvj2LWtznVN; Wed, 11 Dec 2024 01:24:49 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 4BB1OnsT024329; Wed, 11 Dec 2024 01:24:49 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 4BB1OnwP024326; Wed, 11 Dec 2024 01:24:49 GMT (envelope-from git) Date: Wed, 11 Dec 2024 01:24:49 GMT Message-Id: <202412110124.4BB1OnwP024326@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Kyle Evans Subject: git: 36a80f426435 - main - usb: serial: make more commands execute synchronously List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: kevans X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 36a80f4264350a2f4f0686eb91ae7f5943d40327 Auto-Submitted: auto-generated The branch main has been updated by kevans: URL: https://cgit.FreeBSD.org/src/commit/?id=36a80f4264350a2f4f0686eb91ae7f5943d40327 commit 36a80f4264350a2f4f0686eb91ae7f5943d40327 Author: Kyle Evans AuthorDate: 2024-12-11 01:23:10 +0000 Commit: Kyle Evans 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) {