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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The patch is unusable for its stated purpose because `usb::Device` is private and cannot be obtained as a `<Bound>` instance.
- [High] Missing `Sync` implementation for `usb::Interface<device::Bound>`, which is the type USB drivers actually bind to.
- [Medium] The patch omits the `Send` trait for `Device<device::Bound>` and fails to use generic bounds for context markers.
--

commit 58da801d401502ae94fc21b502a93555301c6b9c
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 bound device references in their private data while maintaining thread
safety.

> rust: usb: implement Sync for Device<Bound>
>
> Implement Sync for Device<Bound> in addition to Device<Normal>.
>
> Device<Bound> uses the same underlying struct usb_device as
> Device<Normal>; Bound is a zero-sized type-state marker that does not
> affect thread safety.
>
> This is needed for drivers to store &'bound usb::Device<Bound> in their
> private data while remaining Send.

Since drivers bind to a usb::Interface rather than a usb::Device, do they
actually need to store usb::Interface<device::Bound> instead? If so, it
appears Sync is not implemented for usb::Interface<device::Bound>, which
might leave driver data structures unable to be sent across threads.

Additionally, it looks like struct Device in rust/kernel/usb.rs is private
since it lacks the pub modifier. Because drivers reside outside the
kernel::usb module, are they able to name usb::Device as a field type?

Even if it were public, is there a way for drivers to obtain a Bound
instance? usb::Interface implements AsRef<Device>, but this returns a
reference to Device<Normal>, and there does not seem to be an as_bound()
method available.

> diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
> index 794abe06f5bd7..995221d327b89 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> {}

Should Send also be implemented for Device<device::Bound>? Bound is a
zero-sized marker, and Device<Normal> implements both Send and Sync.

Would it be better to implement these thread-safety traits generically over
all context markers to avoid omissions? For example, implementing them for
Device<Ctx> where Ctx: device::DeviceContext.

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