Restart usbdaemon after USB connection closes

[johnnyman727]

We could make successive CLI commands more likely to succeed if we restarted the USBDaemon after a connection closes. That way any unclosed process or broken state in the daemon would be resolved.

According to @kevinmehall we used to have this functionality but we lost it at some point.

[johnnyman727]

@kevinmehall can you give me some guidance on how to implement this?

[kevinmehall]

It doesn't look like this was ever fully implemented.

If you want a quick and dirty solution, send CMD_RESET, which causes usbexecd to exit:

t2-firmware/soc/usbexecd.c

Line 732 in f88e096

exit(0);

. Note that this is not robust against a CLI crash -- if you send it on exit, a crash would simply never get there. If you send it on start, the usbexecd could be waiting for additional bytes of a partially-written command and not interpret this correctly, and you'd also have to be careful not to lose bytes while it restarts.

Looking at the code, the basics of a more robust out-of-band solution were implemented on both ends, but not in the middle:

In the firmware, usbpipe_init and usbpipe_disable are called when the interface alternate setting is claimed / released. The host OS releases the interface automatically if the CLI node process crashes.

t2-firmware/firmware/usbpipe.c

Line 16 in f88e096

void usbpipe_init() {

The SPI bridge currently contains a mechanism to indicate to the coprocessor whether or not the domain socket is opened by a process running on the SoC, which is used to enable and disable the module ports. We need the opposite of that, a way to indicate to the SoC that the connection should be closed. We'll use the top 4 bits of the flags byte (second byte of the header transfer) to indicate the open/close status of each of the 4 channels, just like are currently used in the other direction.

All it has to do is set the bit and raise the IRQ pin to indicate to the SPI daemon that something changed. Add to bridge.c and headers in firmware.h:

void bridge_enable_chan(u8 channel) {
    out_chan_ready |= (0x10<<channel);
    pin_high(PIN_BRIDGE_IRQ);
}

void bridge_disable_chan(u8 channel) {
    out_chan_ready &= ~(0x11<<channel); // Also clears the "ready to accept data" bit
    in_chan_size[channel] = 0; // Clears any data that was waiting to be sent
    pin_high(PIN_BRIDGE_IRQ);
}

Call bridge_enable_chan(BRIDGE_USB) from usbpipe_init() and bridge_disable_chan(BRIDGE_USB) from usbpipe_disable(). As the enable bits default to 0, call bridge_enable_chan from port_init. The port code could use bridge_disable_chan() to recover if it were sent invalid commands, too.

In the SPI daemon, those enable bits are received as the top 4 bits of rx_buf[1]. It should track the state of those bits, and when a bit goes from 0 -> 1, it should enable SOCK_POLL(i).events = POLLIN; to tell poll() to give it an event to accept an incoming connection on the listening socket. When a bit goes 1 -> 0, it should close the existing connection if connected like this, and set SOCK_POLL(i).events = 0; to stop polling to accept connections on that socket.

usbexecd aborts when the domain socket is closed:

t2-firmware/soc/usbexecd.c

Line 710 in f88e096

fatal("Domain Socket has been closed.");

(procd should restart it, but verify that it doesn't give up after too many retries, or have a delay before restarting). You should also make connect retry on timeout (and/or have a very long timeout), so the process isn't constantly restarting whenever the CLI isn't attached.

[johnnyman727]

@kevinmehall thanks for this super in-depth explanation of how it should work. It's very clear how each of the pieces function together.

I started implementing your suggestion but it looks like usbpipe_init and usbpipe_disable don't get called when you think they do. For example, I started printing out when USB is enabled from the SPI Daemon and it seems to be enabled before I run any commands from the CLI. It also does not get disabled after the interface is released.

[kevinmehall]

Remove the usbpipe_init call from main(). It will get enabled on the SET_INTERFACE request when the CLI claims the interface and sets the alternate setting:

t2-firmware/firmware/usb.c

Line 436 in 1becac4

usbpipe_init();

Releasing the interface should also be calling that function with new_altsetting = 0, so break on it with a debugger and see if that happens?

[johnnyman727]

I guess it's time I go find a debugger :)

What should the new_altsetting variable be here? It seems like there is a potential for it to disable USB and then immediately re-enable it.

[johnnyman727]

@kevinmehall it looks like none of debuggers survived the transition with the exception of a handful of questionable Seggers. Do you have any spares I can borrow?

I'll also need to find and solder a JTAG header.

[kevinmehall]

new_altsetting is the altsetting requested by the host in the SET_INTERFACE packet. The case where it disables and immediately re-enables is when the host requests the altsetting that is already set, which doesn't make a lot of sense for it to do.

[johnnyman727]

Fixed by #103 (when it gets merged).

[johnnyman727]

[Finally got a chance to dig back into this]

I have the code in a state where it is reliably opening and closing channels in the SPI Daemon. However, there is an unexpected issue: OpenWRT seems to take 2+ seconds to restart the USB Daemon after it's killed. If the user tries to run another process in those few seconds, something gets confused and doesn't properly close. I'm looking into any documentation for restarting it faster (maybe a shell script... ew) or potentially launching it from the SPI Daemon if it receives data on the USB channel and doesn't detect the process.