From nobody Wed Jan 01 05:48:16 2025 X-Original-To: dev-commits-src-all@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 4YNJm12Ggkz5jNZQ; Wed, 01 Jan 2025 05:48:17 +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 "R11" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4YNJm05bVvz4QC9; Wed, 1 Jan 2025 05:48:16 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1735710496; 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=6sII4LDjYh/4SOGp+PNzJBJzs9tAuGoeo26FbTVrcF8=; b=CmD3nVM/f6Ct9+r5EFQ/KG6ZFD7+xmyZ2YMkYxkjz7Bsarv7LTKq4FjglPsurGy/KY2HkT EQurNjs5Gyx888BJXEzGA0N9ppLwUiKaexugs1DmvrzyxHIuTXWAbZzbWS5VGVcSL5r70j dWQRofuGHSAFIv25Gcvw3nlyb6s/nkXLeW6+P5SCow7ogQnJEJziepDqHURbAdsEBYld9y sbgYP6F4VPajj/gcWjrtd2UmaJ1E2GqSN9r69PddPZSGZDVFQWX/oJY1m+8i0UmBrZf6ah dw7Qi+TQLpfJEaxAWW5U4XkTOr985yVrmJ+kFOfWEomnL8st8e13NMX2LaIMsQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1735710496; 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=6sII4LDjYh/4SOGp+PNzJBJzs9tAuGoeo26FbTVrcF8=; b=n6nPV8jfxMmF82F0FyvAYid9Bt+WILNK9k7e1LDG06lwQtdUik8JEGwMYzr15tjV5sdFyi QTplhMLTpptGIAUkhtWi/urX9P/pAq++2BxxaNsF68OmZ1dSbq+TK+qkDZCk3X6kgltO4p JOOXD8li3/ixHdxTrSR++e53944oKMeSLaODGxWUKG2id3PC12QpXHhhzqsrQQmyAVyWtz So7iPYWTooR9ORhsrgS/oJDoBrWhF5LW/emba+2+yipoYTwPCcrl33y5rzP5iPUsdXd6xl Va93u/RvBgfT6e+z0v/4N2IVjzRiBEn4lTdlv40gM3l+syBisqIWjG3fWriGhw== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1735710496; a=rsa-sha256; cv=none; b=VTQvCcMcuD55U3MCUo5HNRtKYdFtfQMXX0CHDEjWu09LGM7d+LvLQ9GvM1NA6U4UqQxNSX 2g43z4C3yoVE8llEQ8ew0RJHGW1eaBVcU2cnHnn+S9pfdc0VuRlpvZPzCr4AwB82XYee2h 4mjXarJqlF1J31qCGEyOwvM6iwdaaQltRLy5ZcaZ3Wj8BHuI6Oy1R58+MHTzClA2syQjoH axByzGjiOmtaNFkQJY73Gv5hlVM8FYteQvGTjZfyyORvbz+ECLnukzGR/a1rLqHwOsiotl JQLHH/izKRZulK8I/t/z3lH+rF/WAW/J8BQJGiohejtQl1EZIl1AZSznGqbHcQ== 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 4YNJm05B1dz1BDc; Wed, 1 Jan 2025 05:48:16 +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 5015mGou039357; Wed, 1 Jan 2025 05:48:16 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 5015mGBk039354; Wed, 1 Jan 2025 05:48:16 GMT (envelope-from git) Date: Wed, 1 Jan 2025 05:48:16 GMT Message-Id: <202501010548.5015mGBk039354@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Kyle Evans Subject: git: e374f096e7b6 - stable/13 - usb: serial: make more commands execute synchronously List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@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/stable/13 X-Git-Reftype: branch X-Git-Commit: e374f096e7b61c814a2011d881b002cf406a838c Auto-Submitted: auto-generated The branch stable/13 has been updated by kevans: URL: https://cgit.FreeBSD.org/src/commit/?id=e374f096e7b61c814a2011d881b002cf406a838c commit e374f096e7b61c814a2011d881b002cf406a838c Author: Kyle Evans AuthorDate: 2024-12-11 01:23:10 +0000 Commit: Kyle Evans CommitDate: 2025-01-01 05:47:48 +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 (cherry picked from commit 36a80f4264350a2f4f0686eb91ae7f5943d40327) --- 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 ce9e0ec62bb5..e5c1288166f1 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) {