Re: [PATCH v4 08/27] rust: usb: implement Sync for Device<Bound>

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch completely fails to achieve its stated goal for USB drivers due to subsystem architectural mismatches (private types and missing trait implementations).
- [Medium] Piecemeal typestate trait implementations for thread-safety instead of generic blanket implementations.
--

commit f4c2e06f2819d0d1a95f07b0e3275550140ba681
Author: Danilo Krummrich <dakr@kernel.org>

rust: usb: implement Sync for Device<Bound>

This commit implements the Sync trait for Device<Bound> in the USB subsystem.
The goal is to allow drivers to safely store bound USB device references inside
their private data structures.

> This is needed for drivers to store &'bound usb::Device<Bound> in their
> private data while remaining Send.

[Severity: High]
Does this change actually allow drivers to store the device in their private
data?

Currently, usb::Device is strictly private to the kernel::usb module, so
external drivers cannot declare fields of this type. Even if it were public,
AsRef<Device> for Interface<Ctx> returns &usb::Device<Normal>, which discards
the typestate context and prevents drivers from obtaining a Bound reference.

Additionally, drivers in the USB subsystem typically bind to and receive
&usb::Interface rather than usb::Device. Would we also need to add Sync to
usb::Interface<Bound> so that drivers can store the interface they receive?

> diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
> index 88721970afcb7..6c917d8fa8839 100644
> --- a/rust/kernel/usb.rs
> +++ b/rust/kernel/usb.rs

[ ... ]

> @@ -468,6 +468,10 @@ unsafe impl Send for Device {}
>  // allow any mutation through a shared reference.
>  unsafe impl Sync for Device {}
>  
> +// SAFETY: Same as `Device<Normal>` -- the underlying `struct usb_device` is the same;
> +// `Bound` is a zero-sized type-state marker that does not affect thread safety.
> +unsafe impl Sync for Device<device::Bound> {}

[Severity: Medium]
Would it be more robust to use a generic blanket implementation for thread
safety instead of piecemeal implementations for specific contexts?

The thread safety of usb::Device depends on the underlying C pointer rather
than the zero-sized PhantomData marker. By implementing Sync manually for
each context, this omits Send for Device<Bound> and leaves other valid
contexts like Core and CoreInternal without Sync implementations.

Could we use something like this instead?

unsafe impl<Ctx: device::DeviceContext> Sync for Device<Ctx> {}

This would ensure correct system-wide thread safety for all typestates.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521233501.1191842-1-dakr@kernel.org?part=8