[opentitantool] Avoid repeated opening/closing

Introduce means for users of opentitanlib to declare that for a period,
no other users will be simultaneously accessing the same debugger
device, allowing the driver to keep USB handles open across multiple
operations.

The primary motivation for this is an observation that the repeated
opening and closing of the `/dev/ttyUSBn` HyperDebug console in
conjuction with other activity on the USB bus may trigger what could be
a bug in the Linux USB driver stack.  Even absent concerns for such a
bug, this change reduces the time taken by `transport init` from 0.4s to
0.1s for a set of Google configuration files, which could become
significant, when running many small test cases in a row.

In this first iteration, `opentitansession` is made to claim exclusive
use of the USB backend all the time, and `opentitantool` is meade to
claim exclusive use any time, except when a sub-command explicitly
declares that it does not want exclusive use.  Only the `console`
command is declared in that way, as that is the only one that stays
running for an unbounded period, and is often left running while
opentitantool is separately invoked to manipulate GPIO or other things.

Signed-off-by: Jes B. Klinke <[email protected]>
Change-Id: I1c3625d6efa87a531ae20f5f1cdb576e1096bde9
(cherry picked from commit 3e4b973)

sw/host/opentitanlib/opentitantool_derive/src/lib.rs

@@ -64,6 +64,12 @@ fn dispatch_enum(name: &Ident, e: &DataEnum) -> Result<TokenStream> {
         .map(|variant| dispatch_variant(name, variant))
         .collect::<Result<Vec<_>>>()?;
 
+    let exclusive_arms = e
+        .variants
+        .iter()
+        .map(|variant| exclusive_variant(name, variant))
+        .collect::<Result<Vec<_>>>()?;
+
     // We wrap the derived code inside an anonymous const block to give the
     // `extern crate` references a local namespace that wont pollute the
     // global namespace.
@@ -83,6 +89,12 @@ fn dispatch_enum(name: &Ident, e: &DataEnum) -> Result<TokenStream> {
                         #(#arms),*
                     }
                 }
+
+        fn exclusive_use_of_transport(&self) -> bool {
+                    match self {
+                        #(#exclusive_arms),*
+                    }
+        }
             }
         };
     })
@@ -110,3 +122,26 @@ fn dispatch_variant(name: &Ident, variant: &Variant) -> Result<TokenStream> {
             opentitanlib::app::command::CommandDispatch::run(__field, context, backend)
     })
 }
+
+fn exclusive_variant(name: &Ident, variant: &Variant) -> Result<TokenStream> {
+    let ident = &variant.ident;
+    let unnamed_len = match &variant.fields {
+        Fields::Unnamed(u) => u.unnamed.len(),
+        _ => {
+            return Err(Error::new(
+                variant.ident.span(),
+                "CommandDispatch is only implemented for Newtype variants",
+            ));
+        }
+    };
+    if unnamed_len != 1 {
+        return Err(Error::new(
+            variant.ident.span(),
+            "CommandDispatch is only implemented for Newtype variants",
+        ));
+    }
+    Ok(quote! {
+        #name::#ident(ref __field) =>
+            opentitanlib::app::command::CommandDispatch::exclusive_use_of_transport(__field)
+    })
+}

sw/host/opentitanlib/src/app/command.rs

@@ -22,4 +22,14 @@ pub trait CommandDispatch {
         context: &dyn Any,
         transport: &TransportWrapper,
     ) -> Result<Option<Box<dyn Annotate>>>;
+
+    /// For optimization.  Indicates whether this command expects to not run concurrently with
+    /// other manipulations of the backend debugger.  Only long-running commands such as `console`
+    /// will return `false` to indicate that to the contrary they expect other invocations of
+    /// `opentitantool` to run during their lifespan.  Returning `true` here will allow
+    /// opentitanlib the optimization of keeping USB handles open for the duration of the `run()`
+    /// call, and even across `run()` of multiple commands if `--exec` is used.
+    fn exclusive_use_of_transport(&self) -> bool {
+        true
+    }
 }

sw/host/opentitanlib/src/app/mod.rs

@@ -20,8 +20,8 @@ use crate::io::nonblocking_help::NonblockingHelp;
 use crate::io::spi::{Target, TransferMode};
 use crate::io::uart::Uart;
 use crate::transport::{
-    ioexpander, Capability, ProgressIndicator, ProxyOps, Transport, TransportError,
-    TransportInterfaceType,
+    ioexpander, Capability, MaintainConnection, ProgressIndicator, ProxyOps, Transport,
+    TransportError, TransportInterfaceType,
 };
 
 use anyhow::{bail, ensure, Result};
@@ -937,6 +937,11 @@ impl TransportWrapper {
     /// Configure all pins as input/output, pullup, etc. as declared in configuration files.
     /// Also configure SPI port mode/speed, and other similar settings.
     pub fn apply_default_configuration(&self, strapping_name: Option<&str>) -> Result<()> {
+        // Telling the transport that this function is the exclusive user of the debugger device
+        // for the duration of this function, will allow the transport to keep USB handles open,
+        // for optimization.  (For transports such as the proxy, which does not have any
+        // such optimization, this is a no-op.)
+        let _maintain_connection = self.transport.maintain_connection()?;
         if let Some(strapping_name) = strapping_name {
             if self.capabilities()?.request(Capability::PROXY).ok().is_ok() {
                 self.proxy_ops()?
@@ -969,6 +974,11 @@ impl TransportWrapper {
     }
 
     pub fn reset_target(&self, reset_delay: Duration, clear_uart_rx: bool) -> Result<()> {
+        // Telling the transport that this function is the exclusive user of the debugger device
+        // for the duration of this function, will allow the transport to keep USB handles open,
+        // for optimization.  (For transports such as the proxy, which does not have any
+        // such optimization, this is a no-op.)
+        let _maintain_connection = self.transport.maintain_connection()?;
         log::info!("Asserting the reset signal");
         if self.disable_dft_on_reset.get() {
             self.pin_strapping("PRERESET_DFT_DISABLE")?.apply()?;
@@ -990,6 +1000,13 @@ impl TransportWrapper {
         std::thread::sleep(reset_delay);
         Ok(())
     }
+
+    /// As long as the returned `MaintainConnection` object is kept by the caller, this driver may
+    /// assume that no other `opentitantool` processes attempt to access the same debugger device.
+    /// This allows for optimzations such as keeping USB handles open across function invocations.
+    pub fn maintain_connection(&self) -> Result<Rc<dyn MaintainConnection>> {
+        self.transport.maintain_connection()
+    }
 }
 
 /// Given an pin/uart/spi/i2c port name, if the name is a known alias, return the underlying

sw/host/opentitanlib/src/transport/hyperdebug/mod.rs

@@ -17,7 +17,8 @@ use std::io::Read;
 use std::io::Write;
 use std::marker::PhantomData;
 use std::path::{Path, PathBuf};
-use std::rc::Rc;
+use std::rc::{Rc, Weak};
+use std::time::Duration;
 
 use crate::debug::openocd::OpenOcdJtagChain;
 use crate::io::gpio::{GpioBitbanging, GpioMonitoring, GpioPin};
@@ -29,6 +30,7 @@ use crate::transport::chip_whisperer::board::Board;
 use crate::transport::chip_whisperer::ChipWhisperer;
 use crate::transport::common::fpga::{ClearBitstream, FpgaProgram};
 use crate::transport::common::uart::flock_serial;
+use crate::transport::MaintainConnection;
 use crate::transport::{
     Capabilities, Capability, SetJtagPins, Transport, TransportError, TransportInterfaceType,
     UpdateFirmware,
@@ -215,7 +217,7 @@ impl<T: Flavor> Hyperdebug<T> {
                     if !device.kernel_driver_active(interface.number())? {
                         device.attach_kernel_driver(interface.number())?;
                         // Wait for udev rules to apply proper permissions to new device.
-                        std::thread::sleep(std::time::Duration::from_millis(100));
+                        std::thread::sleep(Duration::from_millis(100));
                     }
 
                     if interface_name.ends_with("Shell") {
@@ -300,6 +302,7 @@ impl<T: Flavor> Hyperdebug<T> {
                 console_tty: console_tty.ok_or_else(|| {
                     TransportError::CommunicationError("Missing console interface".to_string())
                 })?,
+                conn: RefCell::new(Weak::new()),
                 usb_device: RefCell::new(device),
                 selected_spi: Cell::new(0),
             }),
@@ -421,6 +424,7 @@ impl<T: Flavor> Hyperdebug<T> {
 /// even if the caller lets the outer Hyperdebug struct run out of scope.
 pub struct Inner {
     console_tty: PathBuf,
+    conn: RefCell<Weak<Conn>>,
     usb_device: RefCell<UsbBackend>,
     selected_spi: Cell<u8>,
 }
@@ -436,7 +440,44 @@ pub struct CachedIo {
     uarts: RefCell<HashMap<PathBuf, Rc<dyn Uart>>>,
 }
 
+pub struct Conn {
+    console_port: RefCell<TTYPort>,
+}
+
+// The way that the HyperDebug allows callers to request optimization for a sequence of operations
+// without other `opentitantool` processes meddling with the USB devices, is to let the caller
+// hold an `Rc`-reference to the `Conn` struct, thereby keeping the USB connection alive.
+impl MaintainConnection for Conn {}
+
 impl Inner {
+    /// Establish connection with HyperDebug console USB interface.
+    pub fn connect(&self) -> Result<Rc<Conn>> {
+        if let Some(conn) = self.conn.borrow().upgrade() {
+            // The driver already has a connection, use it.
+            return Ok(conn);
+        }
+        // Establish a new connection.
+        let port_name = self
+            .console_tty
+            .to_str()
+            .ok_or(TransportError::UnicodePathError)?;
+        let port =
+            TTYPort::open(&serialport::new(port_name, 115_200).timeout(Duration::from_millis(100)))
+                .context("Failed to open HyperDebug console")?;
+        flock_serial(&port, port_name)?;
+        let conn = Rc::new(Conn {
+            console_port: RefCell::new(port),
+        });
+        // Return a (strong) reference to the newly opened connection, while keeping a weak
+        // reference to the same in this `Inner` object.  The result is that if the caller keeps
+        // the strong reference alive long enough, the next invocation of `connect()` will be able
+        // to re-use the same instance.  If on the other hand, the caller drops their reference,
+        // then the weak reference will not keep the instance alive, and next time a new
+        // connection will be made.
+        *self.conn.borrow_mut() = Rc::downgrade(&conn);
+        Ok(conn)
+    }
+
     /// Send a command to HyperDebug firmware, expecting to receive no output.  Any output will be
     /// reported through an `Err()` return.
     pub fn cmd_no_output(&self, cmd: &str) -> Result<()> {
@@ -505,17 +546,19 @@ impl Inner {
         Ok(captures)
     }
 
+    /// Send a command to HyperDebug firmware, with a callback to receive any output.
+    fn execute_command(&self, cmd: &str, callback: impl FnMut(&str)) -> Result<()> {
+        // Open console device, if not already open.
+        let conn = self.connect()?;
+        // Perform requested command, passing any output to callback.
+        conn.execute_command(cmd, callback)
+    }
+}
+
+impl Conn {
     /// Send a command to HyperDebug firmware, with a callback to receive any output.
     fn execute_command(&self, cmd: &str, mut callback: impl FnMut(&str)) -> Result<()> {
-        let port_name = self
-            .console_tty
-            .to_str()
-            .ok_or(TransportError::UnicodePathError)?;
-        let mut port = TTYPort::open(
-            &serialport::new(port_name, 115_200).timeout(std::time::Duration::from_millis(100)),
-        )
-        .context("Failed to open HyperDebug console")?;
-        flock_serial(&port, port_name)?;
+        let port: &mut TTYPort = &mut self.console_port.borrow_mut();
 
         // Send Ctrl-C, followed by the command, then newline.  This will discard any previous
         // partial input, before executing our command.
@@ -831,6 +874,16 @@ impl<T: Flavor> Transport for Hyperdebug<T> {
         )?);
         Ok(new_jtag)
     }
+
+    /// The way that the HyperDebug driver allows callers to request optimization for a sequence
+    /// of operations without other `opentitantool` processes meddling with the USB devices, is to
+    /// let the caller hold an `Rc`-reference to the `Conn` struct, thereby keeping the USB
+    /// connection alive.  Callers should only hold ond to the object as long as they can
+    /// guarantee that no other `opentitantool` processes simultaneously attempt to access the
+    /// same HyperDebug USB device.
+    fn maintain_connection(&self) -> Result<Rc<dyn MaintainConnection>> {
+        Ok(self.inner.connect()?)
+    }
 }
 
 /// A `StandardFlavor` is a plain Hyperdebug board.

sw/host/opentitanlib/src/transport/mod.rs

@@ -156,6 +156,14 @@ pub trait Transport {
         Err(TransportError::UnsupportedOperation.into())
     }
 
+    /// As long as the returned `MaintainConnection` object is kept by the caller, this driver may
+    /// assume that no other `opentitantool` processes attempt to access the same debugger device.
+    /// This allows for optimzations such as keeping USB handles open across function invocations.
+    fn maintain_connection(&self) -> Result<Rc<dyn MaintainConnection>> {
+        // For implementations that have not implemented any optimizations, return a no-op object.
+        Ok(Rc::new(()))
+    }
+
     /// Before nonblocking operations can be used on `Uart` or other traits, this
     /// `NonblockingHelp` object must be invoked, in order to get the `Transport` implementation a
     /// chance to register its internal event sources with the main event loop.
@@ -164,6 +172,15 @@ pub trait Transport {
     }
 }
 
+/// As long as this object is kept alive, the `Transport` driver may assume that no other
+/// `opentitantool` processes attempt to access the same debugger device.  This allows for
+/// optimzations such as keeping USB handles open across function invocations.
+pub trait MaintainConnection {}
+
+/// No-op implmentation of the trait, for use by `Transport` implementations that do not do
+/// any optimizations to maintain connection between method calls.
+impl MaintainConnection for () {}
+
 /// Methods available only on the Proxy implementation of the Transport trait.
 pub trait ProxyOps {
     /// Returns a string->string map containing user-defined aspects "provided" by the testbed

sw/host/opentitansession/src/main.rs

@@ -146,8 +146,20 @@ fn start_session(run_file_fn: impl FnOnce(u16) -> PathBuf) -> Result<Box<dyn Ser
 // `SessionStartResult` sent through the stdout anonymous pipe, and finally enter an infnite
 // loop, processing connections on that socket
 fn session_child(listen_port: Option<u16>, backend_opts: &backend::BackendOpts) -> Result<()> {
+    // Open connection to transport backend (HyperDebug or other debugger device) based on
+    // command line arguments.
     let transport = backend::create(backend_opts)?;
+
+    // We do not need other invocations of `opentitantool` to directly access the debugger device
+    // while this session process runs (as any such invocations ought to instead establish TCP/IP
+    // connection and go through this session.)  Hence, we can inform the driver that it is free
+    // to e.g. hold on to open USB handles between function calls, or perform other similar
+    // optimizations.
+    let _maintain_connection = transport.maintain_connection()?;
+
+    // Bind to TCP socket, in preparation for servicing requests from network.
     let mut session = SessionHandler::init(&transport, listen_port)?;
+
     // Instantiation of Transport backend, and binding to a socket was successful, now go
     // through the process of making this process a daemon, disconnected from the
     // terminal that was used to start it.

sw/host/opentitantool/src/command/console.rs

@@ -135,4 +135,12 @@ impl CommandDispatch for Console {
             }
         }
     }
+
+    /// For optiimzation.  Indicates that this command expects other invocations of
+    /// `opentitantool` to run during the lifespan of the `run()` function above.  Returning
+    /// `false` here will prevent opentitanlib from keeping USB handles open for the duration of
+    /// the `run()` call.
+    fn exclusive_use_of_transport(&self) -> bool {
+        false
+    }
 }

sw/host/opentitantool/src/main.rs

@@ -15,12 +15,14 @@ use std::io::ErrorKind;
 use std::io::IsTerminal;
 use std::iter::{IntoIterator, Iterator};
 use std::path::PathBuf;
+use std::rc::Rc;
 use std::str::FromStr;
 
 mod command;
 use opentitanlib::app::command::CommandDispatch;
 use opentitanlib::app::TransportWrapper;
 use opentitanlib::backend;
+use opentitanlib::transport::MaintainConnection;
 
 #[allow(clippy::large_enum_variant)]
 #[derive(Debug, Parser, CommandDispatch)]
@@ -214,13 +216,25 @@ fn print_command_result(opts: &Opts, result: Result<Option<Box<dyn Annotate>>>)
 
 // Execute is a convenience function for taking a list of strings,
 // parsing them into a command, executing the command and printing the result.
-fn execute<I>(args: I, opts: &Opts, transport: &TransportWrapper) -> Result<()>
+fn execute<I>(
+    args: I,
+    opts: &Opts,
+    transport: &TransportWrapper,
+    maintain_connection: &mut Option<Rc<dyn MaintainConnection>>,
+) -> Result<()>
 where
     I: IntoIterator<Item = OsString>,
 {
     let command = RootCommandHierarchy::parse_from(
         std::iter::once(OsString::from("opentitantool")).chain(args),
     );
+    if command.exclusive_use_of_transport() {
+        if maintain_connection.is_none() {
+            *maintain_connection = Some(transport.maintain_connection()?);
+        }
+    } else {
+        *maintain_connection = None;
+    }
     print_command_result(opts, command.run(opts, transport))?;
     Ok(())
 }
@@ -230,13 +244,23 @@ fn main() -> Result<()> {
 
     let transport = backend::create(&opts.backend_opts)?;
 
+    let mut _maintain_connection = None;
+
     for command in &opts.exec {
         execute(
             shellwords::split(command)?.iter().map(OsString::from),
             &opts,
             &transport,
+            &mut _maintain_connection,
         )?;
     }
+    if opts.command.exclusive_use_of_transport() {
+        if _maintain_connection.is_none() {
+            _maintain_connection = Some(transport.maintain_connection()?);
+        }
+    } else {
+        _maintain_connection = None;
+    }
     print_command_result(&opts, opts.command.run(&opts, &transport))?;
     Ok(())
 }