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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025092356-rounding-eligibly-c4b7@gregkh>
Date: Tue, 23 Sep 2025 16:13:24 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Daniel Almeida <daniel.almeida@...labora.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	linux-usb@...r.kernel.org
Subject: Re: [PATCH 1/2] rust: usb: add basic USB abstractions

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