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

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

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?  It will always exist in the device "tree" as a valid
device as the interfaces are children of these "devices".  (hint, usb
is odd, we have "devices" and "interfaces", a device consists of 1-N
child interfaces, and a normal USB driver binds to an interface, not a
device.)

thanks,

greg k-h