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

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

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?