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

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

On Tue, Sep 23, 2025 at 03:21:09PM +0200, Danilo Krummrich wrote:
> On Mon Aug 25, 2025 at 8:18 PM CEST, Daniel Almeida wrote:
> > +/// The USB driver trait.
> > +///
> > +/// # Examples
> > +///
> > +///```
> > +/// # use kernel::{bindings, device::Core, usb};
> > +/// use kernel::prelude::*;
> > +///
> > +/// struct MyDriver;
> > +///
> > +/// kernel::usb_device_table!(
> > +///     USB_TABLE,
> > +///     MODULE_USB_TABLE,
> > +///     <MyDriver as usb::Driver>::IdInfo,
> > +///     [
> > +///         (usb::DeviceId::from_id(0x1234, 0x5678), ()),
> > +///         (usb::DeviceId::from_id(0xabcd, 0xef01), ()),
> > +///     ]
> > +/// );
> > +///
> > +/// impl usb::Driver for MyDriver {
> > +///     type IdInfo = ();
> > +///     const ID_TABLE: usb::IdTable<Self::IdInfo> = &USB_TABLE;
> > +///
> > +///     fn probe(
> > +///         _interface: &usb::Interface<Core>,
> > +///         _id: &usb::DeviceId,
> > +///         _info: &Self::IdInfo,
> > +///     ) -> Result<Pin<KBox<Self>>> {
> > +///         Err(ENODEV)
> > +///     }
> > +///
> > +///     fn disconnect(_interface: &usb::Interface<Core>, _data: Pin<&Self>) {}
> > +/// }
> > +///```
> > +pub trait Driver {
> > +    /// The type holding information about each one of the device ids supported by the driver.
> > +    type IdInfo: 'static;
> > +
> > +    /// The table of device ids supported by the driver.
> > +    const ID_TABLE: IdTable<Self::IdInfo>;
> > +
> > +    /// USB driver probe.
> > +    ///
> > +    /// Called when a new USB interface is bound to this driver.
> > +    /// Implementers should attempt to initialize the interface here.
> > +    fn probe(
> > +        interface: &Interface<device::Core>,
> > +        id: &DeviceId,
> > +        id_info: &Self::IdInfo,
> > +    ) -> Result<Pin<KBox<Self>>>;
> > +
> > +    /// USB driver disconnect.
> > +    ///
> > +    /// Called when the USB interface is about to be unbound from this driver.
> > +    fn disconnect(interface: &Interface<device::Core>, data: Pin<&Self>);
> 
> I think this callback should be optional, like all the other unbind() we have in
> other busses.
> 
> @Greg: Why is this called disconnect() in the C code instead of remove()?

I don't know, naming is hard, and it was the first, or second, user of
the driver model we ever created :)

> For Rust, I feel like we should align to the unbind() terminology we use
> everywhere else (for the same reasons we chose unbind() over remove() for other
> busses as well).
> 
> We went with unbind(), since the "raw" remove() (or disconnect()) callback does
> more, i.e. it first calls into unbind() and then drops the driver's private data
> for this specific device.
> 
> So, the unbind() callback that goes to the driver is only meant as a place for
> drivers to perform operations to tear down the device. Resources are freed
> subsequently when the driver's private data is dropped.

Yes, we should probably match these up with what PCI does here in the
end, that would be good.

> > +/// 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.

Functions like usb_fill_bulk_urb() takes a pointer to a usb_device, not
an interface.  Yes, we should fix that, but that "mistake" dates way way
way back to the original USB api decades ago.  So much so that I didn't
even remember that we used that pointer there :)

So it's ok to wrap this for now, it will be needed.

thanks,

greg k-h