Re: [PATCH v5 09/24] 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:
- [Medium] The implementation of `Sync` for `usb::Device<Bound>` is unusable and conceptually incorrect because `Device` is private and cannot be obtained.
- [Medium] The targeted implementation of `Sync` for `Device<Bound>` propagates an anti-pattern by failing to implement thread safety traits generically over all device contexts.
--

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

rust: usb: implement Sync for Device<Bound>

This commit implements the Sync trait for Device<Bound> to allow drivers
to store a bound USB device in their private data.

> 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]
The commit message states this is needed for drivers to store
&'bound usb::Device<Bound> in their private data. However, since struct
Device is defined without the pub visibility modifier in rust/kernel/usb.rs,
how can external drivers name this type in their structs?

Additionally, there does not seem to be an API to obtain a Device<Bound>.
Calling as_ref() on an Interface explicitly returns &Device (which defaults
to &Device<device::Normal>). Since USB drivers bind to struct usb_interface
rather than struct usb_device, shouldn't drivers be storing
&'bound usb::Interface<Bound> instead?

[Severity: Medium]
Would it be better to implement these traits generically across all contexts,
such as using unsafe impl<Ctx> Sync for Device<Ctx>?

Implementing this piecemeal leaves Device<Bound> without a Send
implementation, whereas Device<Normal> has one. It also entirely omits
both Send and Sync for usb::Interface<Bound>, which is the structure
drivers actually bind to.

Since the Ctx type-state marker is a zero-sized PhantomData that does not
affect the thread-safety semantics of the underlying C structures, a generic
implementation might prevent downstream drivers from breaking when requiring
Send or Sync bounds.

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