Add basic USB abstractions, consisting of usb::{Device, Interface,
Driver, Adapter, DeviceId} and the module_usb_driver macro. This is the
first step in being able to write USB device drivers, which paves the
way for USB media drivers - for example - among others.

This initial support will then be used by a subsequent sample driver,
which constitutes the only user of the USB abstractions so far.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/usb.c              |   8 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/usb.rs              | 457 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 469 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 69a975da829f0c35760f71a1b32b8fcb12c8a8dc..645afe578668097ae04455d9eefb102d1f1ce4af 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -73,6 +73,7 @@
 #include <linux/security.h>
 #include <linux/slab.h>
 #include <linux/tracepoint.h>
+#include <linux/usb.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/xarray.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 44b2005d50140d34a44ae37d01c2ddbae6aeaa32..da1ee0d3705739e789c7ad21b957bbdb7ca23521 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -48,6 +48,7 @@
 #include "task.c"
 #include "time.c"
 #include "uaccess.c"
+#include "usb.c"
 #include "vmalloc.c"
 #include "wait.c"
 #include "workqueue.c"
diff --git a/rust/helpers/usb.c b/rust/helpers/usb.c
new file mode 100644
index 0000000000000000000000000000000000000000..fb2aad0cbf4d26ac7fb1a3f176ee7a1d30800f92
--- /dev/null
+++ b/rust/helpers/usb.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/usb.h>
+
+struct usb_device *rust_helper_interface_to_usbdev(struct usb_interface *intf)
+{
+	return interface_to_usbdev(intf);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index f8db761c5c95fc66e4c55f539b17fca613161ada..b15277a02028aa1d27480d0630f9f599cacd6e4d 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -127,6 +127,8 @@
 pub mod tracepoint;
 pub mod transmute;
 pub mod types;
+#[cfg(CONFIG_USB)]
+pub mod usb;
 pub mod uaccess;
 pub mod workqueue;
 pub mod xarray;
diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
new file mode 100644
index 0000000000000000000000000000000000000000..8899e7520b58d4e4b08927d54c8912650b78da33
--- /dev/null
+++ b/rust/kernel/usb.rs
@@ -0,0 +1,457 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright (C) 2025 Collabora Ltd.
+
+//! Abstractions for the USB bus.
+//!
+//! C header: [`include/linux/usb.h`](srctree/include/linux/usb.h)
+
+use crate::{
+    bindings, device,
+    device_id::{RawDeviceId, RawDeviceIdIndex},
+    driver,
+    error::{from_result, to_result, Result},
+    prelude::*,
+    str::CStr,
+    types::{AlwaysRefCounted, Opaque},
+    ThisModule,
+};
+use core::{marker::PhantomData, mem::MaybeUninit, ptr::NonNull};
+
+/// An adapter for the registration of USB drivers.
+pub struct Adapter<T: Driver>(T);
+
+// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// a preceding call to `register` has been successful.
+unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
+    type RegType = bindings::usb_driver;
+
+    unsafe fn register(
+        udrv: &Opaque<Self::RegType>,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result {
+        // SAFETY: It's safe to set the fields of `struct usb_driver` on initialization.
+        unsafe {
+            (*udrv.get()).name = name.as_char_ptr();
+            (*udrv.get()).probe = Some(Self::probe_callback);
+            (*udrv.get()).disconnect = Some(Self::disconnect_callback);
+            (*udrv.get()).id_table = T::ID_TABLE.as_ptr();
+        }
+
+        // SAFETY: `udrv` is guaranteed to be a valid `RegType`.
+        to_result(unsafe {
+            bindings::usb_register_driver(udrv.get(), module.0, name.as_char_ptr())
+        })
+    }
+
+    unsafe fn unregister(udrv: &Opaque<Self::RegType>) {
+        // SAFETY: `udrv` is guaranteed to be a valid `RegType`.
+        unsafe { bindings::usb_deregister(udrv.get()) };
+    }
+}
+
+impl<T: Driver + 'static> Adapter<T> {
+    extern "C" fn probe_callback(
+        intf: *mut bindings::usb_interface,
+        id: *const bindings::usb_device_id,
+    ) -> kernel::ffi::c_int {
+        // SAFETY: The USB core only ever calls the probe callback with a valid pointer to a
+        // `struct usb_interface` and `struct usb_device_id`.
+        //
+        // INVARIANT: `intf` is valid for the duration of `probe_callback()`.
+        let intf = unsafe { &*intf.cast::<Interface<device::CoreInternal>>() };
+
+        from_result(|| {
+            // SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `struct usb_device_id` and
+            // does not add additional invariants, so it's safe to transmute.
+            let id = unsafe { &*id.cast::<DeviceId>() };
+
+            let info = T::ID_TABLE.info(id.index());
+            let data = T::probe(intf, id, info)?;
+
+            let dev: &device::Device<device::CoreInternal> = intf.as_ref();
+            dev.set_drvdata(data);
+            Ok(0)
+        })
+    }
+
+    extern "C" fn disconnect_callback(intf: *mut bindings::usb_interface) {
+        // SAFETY: The USB core only ever calls the disconnect callback with a valid pointer to a
+        // `struct usb_interface`.
+        //
+        // INVARIANT: `intf` is valid for the duration of `disconnect_callback()`.
+        let intf = unsafe { &*intf.cast::<Interface<device::CoreInternal>>() };
+
+        let dev: &device::Device<device::CoreInternal> = intf.as_ref();
+
+        // SAFETY: `disconnect_callback` is only ever called after a successful call to
+        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
+        // and stored a `Pin<KBox<T>>`.
+        let data = unsafe { dev.drvdata_obtain::<Pin<KBox<T>>>() };
+
+        T::disconnect(intf, data.as_ref());
+    }
+}
+
+/// Abstraction for the USB device ID structure, i.e. [`struct usb_device_id`].
+///
+/// [`struct usb_device_id`]: https://docs.kernel.org/driver-api/basics.html#c.usb_device_id
+#[repr(transparent)]
+#[derive(Clone, Copy)]
+pub struct DeviceId(bindings::usb_device_id);
+
+impl DeviceId {
+    /// Equivalent to C's `USB_DEVICE` macro.
+    pub const fn from_id(vendor: u16, product: u16) -> Self {
+        Self(bindings::usb_device_id {
+            match_flags: bindings::USB_DEVICE_ID_MATCH_DEVICE as u16,
+            idVendor: vendor,
+            idProduct: product,
+            // SAFETY: It is safe to use all zeroes for the other fields of `usb_device_id`.
+            ..unsafe { MaybeUninit::zeroed().assume_init() }
+        })
+    }
+
+    /// Equivalent to C's `USB_DEVICE_VER` macro.
+    pub const fn from_device_ver(vendor: u16, product: u16, bcd_lo: u16, bcd_hi: u16) -> Self {
+        Self(bindings::usb_device_id {
+            match_flags: bindings::USB_DEVICE_ID_MATCH_DEVICE_AND_VERSION as u16,
+            idVendor: vendor,
+            idProduct: product,
+            bcdDevice_lo: bcd_lo,
+            bcdDevice_hi: bcd_hi,
+            // SAFETY: It is safe to use all zeroes for the other fields of `usb_device_id`.
+            ..unsafe { MaybeUninit::zeroed().assume_init() }
+        })
+    }
+
+    /// Equivalent to C's `USB_DEVICE_INFO` macro.
+    pub const fn from_device_info(class: u8, subclass: u8, protocol: u8) -> Self {
+        Self(bindings::usb_device_id {
+            match_flags: bindings::USB_DEVICE_ID_MATCH_DEV_INFO as u16,
+            bDeviceClass: class,
+            bDeviceSubClass: subclass,
+            bDeviceProtocol: protocol,
+            // SAFETY: It is safe to use all zeroes for the other fields of `usb_device_id`.
+            ..unsafe { MaybeUninit::zeroed().assume_init() }
+        })
+    }
+
+    /// Equivalent to C's `USB_INTERFACE_INFO` macro.
+    pub const fn from_interface_info(class: u8, subclass: u8, protocol: u8) -> Self {
+        Self(bindings::usb_device_id {
+            match_flags: bindings::USB_DEVICE_ID_MATCH_INT_INFO as u16,
+            bInterfaceClass: class,
+            bInterfaceSubClass: subclass,
+            bInterfaceProtocol: protocol,
+            // SAFETY: It is safe to use all zeroes for the other fields of `usb_device_id`.
+            ..unsafe { MaybeUninit::zeroed().assume_init() }
+        })
+    }
+
+    /// Equivalent to C's `USB_DEVICE_INTERFACE_CLASS` macro.
+    pub const fn from_device_interface_class(vendor: u16, product: u16, class: u8) -> Self {
+        Self(bindings::usb_device_id {
+            match_flags: (bindings::USB_DEVICE_ID_MATCH_DEVICE
+                | bindings::USB_DEVICE_ID_MATCH_INT_CLASS) as u16,
+            idVendor: vendor,
+            idProduct: product,
+            bInterfaceClass: class,
+            // SAFETY: It is safe to use all zeroes for the other fields of `usb_device_id`.
+            ..unsafe { MaybeUninit::zeroed().assume_init() }
+        })
+    }
+
+    /// Equivalent to C's `USB_DEVICE_INTERFACE_PROTOCOL` macro.
+    pub const fn from_device_interface_protocol(vendor: u16, product: u16, protocol: u8) -> Self {
+        Self(bindings::usb_device_id {
+            match_flags: (bindings::USB_DEVICE_ID_MATCH_DEVICE
+                | bindings::USB_DEVICE_ID_MATCH_INT_PROTOCOL) as u16,
+            idVendor: vendor,
+            idProduct: product,
+            bInterfaceProtocol: protocol,
+            // SAFETY: It is safe to use all zeroes for the other fields of `usb_device_id`.
+            ..unsafe { MaybeUninit::zeroed().assume_init() }
+        })
+    }
+
+    /// Equivalent to C's `USB_DEVICE_INTERFACE_NUMBER` macro.
+    pub const fn from_device_interface_number(vendor: u16, product: u16, number: u8) -> Self {
+        Self(bindings::usb_device_id {
+            match_flags: (bindings::USB_DEVICE_ID_MATCH_DEVICE
+                | bindings::USB_DEVICE_ID_MATCH_INT_NUMBER) as u16,
+            idVendor: vendor,
+            idProduct: product,
+            bInterfaceNumber: number,
+            // SAFETY: It is safe to use all zeroes for the other fields of `usb_device_id`.
+            ..unsafe { MaybeUninit::zeroed().assume_init() }
+        })
+    }
+
+    /// Equivalent to C's `USB_DEVICE_AND_INTERFACE_INFO` macro.
+    pub const fn from_device_and_interface_info(
+        vendor: u16,
+        product: u16,
+        class: u8,
+        subclass: u8,
+        protocol: u8,
+    ) -> Self {
+        Self(bindings::usb_device_id {
+            match_flags: (bindings::USB_DEVICE_ID_MATCH_INT_INFO
+                | bindings::USB_DEVICE_ID_MATCH_DEVICE) as u16,
+            idVendor: vendor,
+            idProduct: product,
+            bInterfaceClass: class,
+            bInterfaceSubClass: subclass,
+            bInterfaceProtocol: protocol,
+            // SAFETY: It is safe to use all zeroes for the other fields of `usb_device_id`.
+            ..unsafe { MaybeUninit::zeroed().assume_init() }
+        })
+    }
+}
+
+// SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `usb_device_id` and does not add
+// additional invariants, so it's safe to transmute to `RawType`.
+unsafe impl RawDeviceId for DeviceId {
+    type RawType = bindings::usb_device_id;
+}
+
+// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `driver_info` field.
+unsafe impl RawDeviceIdIndex for DeviceId {
+    const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::usb_device_id, driver_info);
+
+    fn index(&self) -> usize {
+        self.0.driver_info
+    }
+}
+
+/// [`IdTable`](kernel::device_id::IdTable) type for USB.
+pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>;
+
+/// Create a USB `IdTable` with its alias for modpost.
+#[macro_export]
+macro_rules! usb_device_table {
+    ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => {
+        const $table_name: $crate::device_id::IdArray<
+            $crate::usb::DeviceId,
+            $id_info_type,
+            { $table_data.len() },
+        > = $crate::device_id::IdArray::new($table_data);
+
+        $crate::module_device_table!("usb", $module_table_name, $table_name);
+    };
+}
+
+/// 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>);
+}
+
+/// A USB interface.
+///
+/// This structure represents the Rust abstraction for a C [`struct usb_interface`].
+/// The implementation abstracts the usage of a C [`struct usb_interface`] passed
+/// in from the C side.
+///
+/// # Invariants
+///
+/// An [`Interface`] instance represents a valid [`struct usb_interface`] created
+/// by the C portion of the kernel.
+///
+/// [`struct usb_interface`]: https://www.kernel.org/doc/html/latest/driver-api/usb/usb.html#c.usb_interface
+#[repr(transparent)]
+pub struct Interface<Ctx: device::DeviceContext = device::Normal>(
+    Opaque<bindings::usb_interface>,
+    PhantomData<Ctx>,
+);
+
+impl<Ctx: device::DeviceContext> Interface<Ctx> {
+    fn as_raw(&self) -> *mut bindings::usb_interface {
+        self.0.get()
+    }
+}
+
+// SAFETY: `Interface` is a transparent wrapper of a type that doesn't depend on
+// `Interface`'s generic argument.
+kernel::impl_device_context_deref!(unsafe { Interface });
+kernel::impl_device_context_into_aref!(Interface);
+
+impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Interface<Ctx> {
+    fn as_ref(&self) -> &device::Device<Ctx> {
+        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
+        // `struct usb_interface`.
+        let dev = unsafe { &raw mut ((*self.as_raw()).dev) };
+
+        // SAFETY: `dev` points to a valid `struct device`.
+        unsafe { device::Device::from_raw(dev) }
+    }
+}
+
+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()) }
+    }
+}
+
+// SAFETY: Instances of `Interface` are always reference-counted.
+unsafe impl AlwaysRefCounted for Interface {
+    fn inc_ref(&self) {
+        // SAFETY: The invariants of `Interface` guarantee that `self.as_raw()`
+        // returns a valid `struct usb_interface` pointer, for which we will
+        // acquire a new refcount.
+        unsafe { bindings::usb_get_intf(self.as_raw()) };
+    }
+
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::usb_put_intf(obj.cast().as_ptr()) }
+    }
+}
+
+// SAFETY: A `Interface` is always reference-counted and can be released from any thread.
+unsafe impl Send for Interface {}
+
+// SAFETY: It is safe to send a &Interface to another thread because we do not
+// allow any mutation through a shared reference.
+unsafe impl Sync for Interface {}
+
+/// 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>,
+);
+
+impl<Ctx: device::DeviceContext> Device<Ctx> {
+    fn as_raw(&self) -> *mut bindings::usb_device {
+        self.0.get()
+    }
+}
+
+// SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s generic
+// argument.
+kernel::impl_device_context_deref!(unsafe { Device });
+kernel::impl_device_context_into_aref!(Device);
+
+// SAFETY: Instances of `Device` are always reference-counted.
+unsafe impl AlwaysRefCounted for Device {
+    fn inc_ref(&self) {
+        // SAFETY: The invariants of `Device` guarantee that `self.as_raw()`
+        // returns a valid `struct usb_device` pointer, for which we will
+        // acquire a new refcount.
+        unsafe { bindings::usb_get_dev(self.as_raw()) };
+    }
+
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::usb_put_dev(obj.cast().as_ptr()) }
+    }
+}
+
+impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
+    fn as_ref(&self) -> &device::Device<Ctx> {
+        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
+        // `struct usb_device`.
+        let dev = unsafe { &raw mut ((*self.as_raw()).dev) };
+
+        // SAFETY: `dev` points to a valid `struct device`.
+        unsafe { device::Device::from_raw(dev) }
+    }
+}
+
+// SAFETY: A `Device` is always reference-counted and can be released from any thread.
+unsafe impl Send for Device {}
+
+// SAFETY: It is safe to send a &Device to another thread because we do not
+// allow any mutation through a shared reference.
+unsafe impl Sync for Device {}
+
+/// Declares a kernel module that exposes a single USB driver.
+///
+/// # Examples
+///
+/// ```ignore
+/// module_usb_driver! {
+///     type: MyDriver,
+///     name: "Module name",
+///     author: ["Author name"],
+///     description: "Description",
+///     license: "GPL v2",
+/// }
+/// ```
+#[macro_export]
+macro_rules! module_usb_driver {
+    ($($f:tt)*) => {
+        $crate::module_driver!(<T>, $crate::usb::Adapter<T>, { $($f)* });
+    }
+}

-- 
2.50.1

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()?

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.

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

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

On 9/23/25 4:13 PM, Greg Kroah-Hartman wrote:
> Functions like usb_fill_bulk_urb() takes a pointer to a usb_device, not
> an interface.
Yes, I'm aware, please see my reply [1] regarding that.

[1] https://lore.kernel.org/all/DD08HWM0M68R.2R74OSODBIWSZ@kernel.org/

On 23.09.25 16:13, Greg Kroah-Hartman wrote:

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

How would we do that? We need to be able to send at least control
request to devices before we have established which configurations
or interfaces the device has.

	Regards
		Oliver

On Tue, Sep 23, 2025 at 04:16:36PM +0200, Oliver Neukum wrote:
> On 23.09.25 16:13, Greg Kroah-Hartman wrote:
> 
> > 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 :)
> 
> How would we do that? We need to be able to send at least control
> request to devices before we have established which configurations
> or interfaces the device has.

Oops, I thought that usb_dev in struct usb_interface was a pointer to
struct usb_device, but no, it's something else...  So no, we need to
keep that as-is for now.

thanks,

greg k-h

On 9/23/25 4:22 PM, Greg Kroah-Hartman wrote:
> On Tue, Sep 23, 2025 at 04:16:36PM +0200, Oliver Neukum wrote:
>> On 23.09.25 16:13, Greg Kroah-Hartman wrote:
>>
>>> 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 :)
>>
>> How would we do that? We need to be able to send at least control
>> request to devices before we have established which configurations
>> or interfaces the device has.
> 
> Oops, I thought that usb_dev in struct usb_interface was a pointer to
> struct usb_device, but no, it's something else...  So no, we need to
> keep that as-is for now.

You can still get it via interface_to_usbdev(), no?

On Tue, Sep 23, 2025 at 04:25:46PM +0200, Danilo Krummrich wrote:
> On 9/23/25 4:22 PM, Greg Kroah-Hartman wrote:
> > On Tue, Sep 23, 2025 at 04:16:36PM +0200, Oliver Neukum wrote:
> >> On 23.09.25 16:13, Greg Kroah-Hartman wrote:
> >>
> >>> 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 :)
> >>
> >> How would we do that? We need to be able to send at least control
> >> request to devices before we have established which configurations
> >> or interfaces the device has.
> > 
> > Oops, I thought that usb_dev in struct usb_interface was a pointer to
> > struct usb_device, but no, it's something else...  So no, we need to
> > keep that as-is for now.
> 
> You can still get it via interface_to_usbdev(), no?

Sigh, this is what I get for writing emails while sitting in a
conference...

Yes, you are right, it can be gotten that way.  But I can't wait to see
how you wrap that C macro in rust :)

thanks,

greg k-h

On 9/23/25 4:37 PM, Greg Kroah-Hartman wrote:
> Yes, you are right, it can be gotten that way.  But I can't wait to see
> how you wrap that C macro in rust :)

We can either create a Rust helper function for it, or just re-implement it; in
the end it boils down to just a container_of() on the parent device.

On Tue, Sep 23, 2025 at 04:42:11PM +0200, Danilo Krummrich wrote:
> On 9/23/25 4:37 PM, Greg Kroah-Hartman wrote:
> > Yes, you are right, it can be gotten that way.  But I can't wait to see
> > how you wrap that C macro in rust :)
> 
> We can either create a Rust helper function for it, or just re-implement it; in
> the end it boils down to just a container_of() on the parent device.

Yes, and it preserves the "const" of the pointer going into the function
call, can we do that in rust as well?

On Tue Sep 23, 2025 at 4:49 PM CEST, Greg Kroah-Hartman wrote:
> On Tue, Sep 23, 2025 at 04:42:11PM +0200, Danilo Krummrich wrote:
>> On 9/23/25 4:37 PM, Greg Kroah-Hartman wrote:
>> > Yes, you are right, it can be gotten that way.  But I can't wait to see
>> > how you wrap that C macro in rust :)
>> 
>> We can either create a Rust helper function for it, or just re-implement it; in
>> the end it boils down to just a container_of() on the parent device.
>
> Yes, and it preserves the "const" of the pointer going into the function
> call, can we do that in rust as well?

Yes, the Rust container_of!() macro should preserve that.

But despite that, I think it doesn't matter too much in this specific case.

Abstractions of C structures are usually contained within the kernel's Opaque<T>
type, which allows for interior mutability.

Actual mutability is controlled by the corresponding abstraction around the
Opaque<T>.

For instance, a struct device representation looks like this:

	 struct Device<Ctx: DeviceContext = Normal>(Opaque<bindings::device>, PhantomData<Ctx>);

In this case, we never give out a mutable reference to a Device, but rather
control mutability internally with the help of the device context.

For instance, if we have a &Device<Core>, we're guranteed that we're called from
a context where the device_lock() is guaranteed to be held, so we can allow for
some interior mutability.

Hi Danilo,


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

— Daniel

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?

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

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.

On Tue, Sep 23, 2025 at 04:38:34PM +0200, Danilo Krummrich wrote:
> >> @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.

The intention of the USB stack is that yes, a usb_device cannot have 
children if it isn't bound to a usb_device_driver.  However, we don't 
try to guarantee that this is true; a particular driver might not 
enforce this restriction.

There is a surprisingly large number of calls to 
usb_register_device_driver() in the kernel (four in addition to the 
standard one).  I suppose a little auditing could ensure that these 
drivers do deconfigure their devices when they unbind.

Alan Stern

On Tue, Sep 23, 2025 at 04:38:34PM +0200, Danilo Krummrich wrote:
> 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).

Wait, usb_device_driver shouldn't be used here, that's only for
"special" things like hubs and an odd Apple device.

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

Interfaces are bound to usb_driver, and are a child device of a struct
usb_device.  There is no need to worry if a driver is bound to a struct
usb_device at any time, it should be independent if a driver is bound to
a struct interface.  All that we should care about is the driver that is
bound to a usb_interface as that is what the rust binding should be for
here.

Sorry for the naming confusion, usb is messy in places.

thanks,

greg k-h

On 9/23/25 4:52 PM, Greg Kroah-Hartman wrote:
>> 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).
> 
> Wait, usb_device_driver shouldn't be used here, that's only for
> "special" things like hubs and an odd Apple device.

I only mentioned it because if a struct usb_device is bound, then it is bound to
a struct usb_device_driver.

And because we're not doing this, I doubt that a *public* usb::Device
abstraction is required (at least for now).

The cases where we need a struct usb_device for operations on USB interface
(such as usb_fill_bulk_urb(), usb_submit_urb(), etc.) can be dealt with
internally in the abstraction itself, such that drivers only need to know about
the interface.

So, I wouldn't expose it just yet. If we see that further down the road we need
usb_interface drivers to mess with the usb_device directly for some reason, we
can still expose it.

>> 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).
> 
> Interfaces are bound to usb_driver, and are a child device of a struct
> usb_device.  There is no need to worry if a driver is bound to a struct
> usb_device at any time, it should be independent if a driver is bound to
> a struct interface.  All that we should care about is the driver that is
> bound to a usb_interface as that is what the rust binding should be for
> here.

Right, that's why I said I doubt that it is semantically useful to derive a
&usb::Device<Bound> from a &usb::Interface<Bound> or a &usb::Device<Core> from a
&usb::Interface<Core>.

But besides that I also think it's also just wrong, so we should remove the
corresponding code.

On Mon Aug 25, 2025 at 8:18 PM CEST, Daniel Almeida wrote:
> +impl DeviceId {
> +    /// Equivalent to C's `USB_DEVICE` macro.
> +    pub const fn from_id(vendor: u16, product: u16) -> Self {
> +        Self(bindings::usb_device_id {
> +            match_flags: bindings::USB_DEVICE_ID_MATCH_DEVICE as u16,
> +            idVendor: vendor,
> +            idProduct: product,
> +            // SAFETY: It is safe to use all zeroes for the other fields of `usb_device_id`.
> +            ..unsafe { MaybeUninit::zeroed().assume_init() }

You can avoid this usage of `unsafe` with this patch series:

    https://lore.kernel.org/all/20250814093046.2071971-1-lossin@kernel.org

I'd like to avoid introducing any new one of these.

---
Cheers,
Benno

> +        })
> +    }

Hi Benno,

> On 25 Aug 2025, at 17:49, Benno Lossin <lossin@kernel.org> wrote:
> 
> On Mon Aug 25, 2025 at 8:18 PM CEST, Daniel Almeida wrote:
>> +impl DeviceId {
>> +    /// Equivalent to C's `USB_DEVICE` macro.
>> +    pub const fn from_id(vendor: u16, product: u16) -> Self {
>> +        Self(bindings::usb_device_id {
>> +            match_flags: bindings::USB_DEVICE_ID_MATCH_DEVICE as u16,
>> +            idVendor: vendor,
>> +            idProduct: product,
>> +            // SAFETY: It is safe to use all zeroes for the other fields of `usb_device_id`.
>> +            ..unsafe { MaybeUninit::zeroed().assume_init() }
> 
> You can avoid this usage of `unsafe` with this patch series:
> 
>    https://lore.kernel.org/all/20250814093046.2071971-1-lossin@kernel.org
> 
> I'd like to avoid introducing any new one of these.
> 
> ---
> Cheers,
> Benno
> 
>> +        })
>> +    }
> 

Ah, nice, you spoke about this in the last RFL call, I remember it now.

Ok, I will address this in the next version.

— Daniel