Re: [PATCH 1/2] rust: usb: add basic USB abstractions

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Sep 23, 2025 at 04:38:34PM +0200, Danilo Krummrich wrote:
> On Tue Sep 23, 2025 at 4:30 PM CEST, Greg Kroah-Hartman wrote:
> > On Tue, Sep 23, 2025 at 04:03:01PM +0200, Danilo Krummrich wrote:
> >> On Tue Sep 23, 2025 at 3:31 PM CEST, Daniel Almeida wrote:
> >> >>> +/// A USB device.
> >> >>> +///
> >> >>> +/// This structure represents the Rust abstraction for a C [`struct usb_device`].
> >> >>> +/// The implementation abstracts the usage of a C [`struct usb_device`] passed in
> >> >>> +/// from the C side.
> >> >>> +///
> >> >>> +/// # Invariants
> >> >>> +///
> >> >>> +/// A [`Device`] instance represents a valid [`struct usb_device`] created by the C portion of the
> >> >>> +/// kernel.
> >> >>> +///
> >> >>> +/// [`struct usb_device`]: https://www.kernel.org/doc/html/latest/driver-api/usb/usb.html#c.usb_device
> >> >>> +#[repr(transparent)]
> >> >>> +pub struct Device<Ctx: device::DeviceContext = device::Normal>(
> >> >>> +    Opaque<bindings::usb_device>,
> >> >>> +    PhantomData<Ctx>,
> >> >>> +);
> >> >> 
> >> >> What do you use the struct usb_device abstraction for? I only see the sample
> >> >> driver probing a USB interface instead.
> >> >
> >> > What I was brainstorming with Greg is to submit this initial support, and then
> >> > follow up with all the other abstractions needed to implement a Rust version of
> >> > usb-skeleton.c. IIUC, the plan is to submit any fixes as follow-ups, as we're
> >> > close to the merge window.
> >> >
> >> > struct usb_device would be used for the skeleton driver, so we should keep it if
> >> > we're following the plan above, IMHO.
> >> 
> >> Yes, it's clearly required for the raw accessors for submitting URBs, e.g.
> >> usb_fill_bulk_urb(), usb_submit_urb(), etc.
> >> 
> >> But I'm not sure you actually have to expose a representation of a struct
> >> usb_device (with device context information) publically for that. It seems to me
> >> that this can all be contained within the abstraction.
> >> 
> >> For instance, the public API could look like this:
> >> 
> >> 	let urb = intf.urb_create()?;
> >> 	urb.fill_bulk(buffer, callback_fn, ...)?;
> >> 	urb.submit();
> >> 
> >> The urb_create() method of a usb::Interface can derive the struct usb_device
> >> from the struct usb_interface internally and store it in the Urb structure, i.e.
> >> no need to let drivers mess with this.
> >> 
> >> So, I think for this part it makes more sense to first work out the other
> >> APIs before exposing things speculatively.
> >> 
> >> I also just spotted this:
> >> 
> >> 	impl<Ctx: device::DeviceContext> AsRef<Device<Ctx>> for Interface<Ctx> {
> >> 	    fn as_ref(&self) -> &Device<Ctx> {
> >> 	        // SAFETY: `self.as_raw()` is valid by the type invariants. For a valid interface,
> >> 	        // the helper should always return a valid USB device pointer.
> >> 	        let usb_dev = unsafe { bindings::interface_to_usbdev(self.as_raw()) };
> >> 	
> >> 	        // SAFETY: The helper returns a valid interface pointer that shares the
> >> 	        // same `DeviceContext`.
> >> 	        unsafe { &*(usb_dev.cast()) }
> >> 	    }
> >> 	}
> >> 
> >> which I think is wrong. You can't derive the device context of a usb::Interface
> >> for a usb::Device generically. You probably can for the Bound context, but not
> >> for the Core context.
> >> 
> >> But honestly, I'm even unsure for the Bound context.
> >> 
> >> @Greg: Can we guarantee that a struct usb_device is always bound as long as one
> >> of its interfaces is still bound?
> >
> > Bound to what?
> 
> Well, that's kinda my point. :)
> 
> Having a &usb::Device<Bound> would mean that for the lifetime of the reference
> it is guaranteed that the usb::Device is bound to its USB device driver
> (struct usb_device_driver).

Wait, usb_device_driver shouldn't be used here, that's only for
"special" things like hubs and an odd Apple device.

> The code above establishes that you can get a &usb::Device<Bound> from a
> &usb::Interface<Bound>, i.e. an interface that is bound to a USB driver
> (struct usb_driver).

Interfaces are bound to usb_driver, and are a child device of a struct
usb_device.  There is no need to worry if a driver is bound to a struct
usb_device at any time, it should be independent if a driver is bound to
a struct interface.  All that we should care about is the driver that is
bound to a usb_interface as that is what the rust binding should be for
here.

Sorry for the naming confusion, usb is messy in places.

thanks,

greg k-h