Re: [PATCH v3 0/5] rust: usb: host-side abstractions for a bulk-endpoint driver

[Shuai Liu]

Hi Mike,

I read through the v3 diff (patches 1 and 2 in detail). I could not
apply the series locally because the 8-month-old base no longer matches
current usb.rs, so this review is from the diff, not a build. Two
findings, both around alternate settings:

1. set_alternate_setting() invalidates Endpoint<K> without the type
   system noticing

Endpoint<K>'s invariants say the address names an endpoint of the
interface's *active* alternate setting, with the transfer type and
direction of K. That proof is only valid for one altsetting: after

    interface.set_alternate_setting(1)?

every previously obtained &Endpoint<BulkOut> is still usable, but the
same address in the new setting may name an interrupt endpoint or a
different direction. A transfer method will then happily build a pipe
for an endpoint that no longer matches the tag. C drivers get this
right by re-fetching endpoints after usb_set_interface(); the Rust
abstraction currently lets a driver keep stale proofs across the
switch.

Something that binds the proof to the setting would close it, e.g. an
Endpoint borrowed from an Altsetting token (making it unusable after a
switch), or a generation stamp checked at transfer time. At minimum the
doc comment on set_alternate_setting() and on Endpoint should state
that all previously obtained Endpoint values must be discarded across a
call.

2. set_alternate_setting() with transfers in flight is undefined

usb_set_interface() tears down and reallocates the interface's
endpoints, which unlinks and kills any URBs submitted on them. A
persistent BulkInQueue/BulkOutQueue built in patch 2 therefore loses
its URBs out from under the Rust slot state machine: the queue thinks
a slot is in flight while the C core has killed the URB, and nothing
ever wakes the waiter. The documentation of set_alternate_setting()
does not warn about this.

Either the method should require an empty window (e.g. take &mut self,
or refuse when the window has registered URBs), or it should document
that the driver must drain and drop its queues before switching, with
the queue Drop doing the cancellation.

Two smaller notes:

- queue_reset_device(): I checked hub.c; the "may be called from any
  context, deferred to the core workqueue" documentation matches
  usb_queue_reset_device().

- EndpointKind being sealed with BulkIn/BulkOut/InterruptIn is a nice
  fit; control endpoint 0 going through Pipe instead of Endpoint avoids
  pretending it takes part in the altsetting lifecycle.

Best regards,
Shuai Liu