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

["Danilo Krummrich" <dakr@kernel.org>]

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).

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).

It also does establish the same with other device contexts, such as the Core
context.

Despite the question whether this is sematically useful, I doubt that this is
a correct assumption to take.