Re: [PATCH v4 3/6] rust: kernel: Add greybus abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] rust: greybus: AlwaysRefCounted implemented for all Device<Ctx> contexts
- [Medium] rust: greybus: Interface implements AlwaysRefCounted without Send and Sync
- [Low] rust: greybus: broken rustdoc links and outdated HdDriver example
--

commit 31981649b53fe9bea7a30ae6afc505468e2c2736
Author: Ayush Singh <ayush@beagleboard.org>

rust: kernel: Add greybus abstractions

This patch adds Rust abstractions for the Greybus core, host devices, and
wire protocol definitions to support implementing Greybus host drivers in
Rust.

> diff --git a/rust/kernel/greybus/hd.rs b/rust/kernel/greybus/hd.rs
> new file mode 100644
> index 0000000000000..45a2d53ee80f5
> --- /dev/null
> +++ b/rust/kernel/greybus/hd.rs
> @@ -0,0 +1,362 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Greybus host device abstractions.

[ ... ]

> +//! # Examples
> +//!
> +//! ```ignore
> +//! use kernel::{device, prelude::*};
> +//!
> +//! struct MyHd {
> +//!     // transport state
> +//! }
> +//!
> +//! #[vtable]
> +//! impl HdDriver for MyHd {
> +//!     fn message_send(data: &Self, hd: &Device, dest_cport_id: u16, msg: Message) -> Result {
> +//!         // Copy what is needed out of `msg`, queue it, and return without sleeping.
> +//!         // Call `HostDevice::message_sent()` once the core may release the message.
> +//!         Ok(())
> +//!     }
> +//!
> +//!     fn message_cancel(_msg: Message) {}\n> +//! }
> +//!
> +//! fn probe(parent: &device::Device) -> Result<Registration<MyHd>> {
> +//!     Registration::new(parent, BUFFER_SIZE_MAX, NUM_CPORTS, try_pin_init!(MyHd {}))
> +//! }
> +//! ```

[Severity: Low]
Are this HdDriver example and the intra-doc links in rust/kernel/greybus/hd.rs
and rust/kernel/greybus/mod.rs out of date with the current API?

In rust/kernel/greybus/hd.rs, the HdDriver example uses outdated signatures
for message_send() (missing gfp_mask: Flags and taking msg: Message by value
instead of &Message) and message_cancel() (taking _msg: Message by value
instead of &Message). It also instructs the driver to call a nonexistent
HostDevice::message_sent() method, whereas HdDriverVTable::message_send()
automatically calls msg.sent(hd, 0) when T::message_send() returns Ok(()).

In addition, the doc comment on Device in rust/kernel/greybus/hd.rs links to
ARef<HostDevice> instead of ARef<Device>, and the doc comments on the module
and on Message in rust/kernel/greybus/mod.rs contain unresolved intra-doc
links to HostDevice and HdDriver, neither of which is in scope in
rust/kernel/greybus/mod.rs (where HostDevice was renamed to hd::Device and
hd::Registration, and HdDriver is at hd::HdDriver).

[ ... ]

> +/// A Greybus host device.
> +///
> +/// # Invariants
> +///
> +/// The wrapped value is a valid `struct gb_host_device` created by `gb_hd_create()`, and every
> +/// [`ARef<HostDevice>`](crate::sync::aref::ARef) owns an increment on its reference count.
> +#[repr(transparent)]
> +pub struct Device<Ctx = device::Normal> {
> +    ptr: Opaque<bindings::gb_host_device>,
> +    _ctx: PhantomData<Ctx>,
> +}
> +
> +// SAFETY: `gb_host_device` is reference counted through its embedded `struct device`, which may be
> +// used from any thread.
> +unsafe impl<Ctx> Send for Device<Ctx> {}
> +
> +// SAFETY: `gb_host_device` has its own internal locking, so it is safe to share references to it
> +// across threads.
> +unsafe impl<Ctx> Sync for Device<Ctx> {}
> +
> +// SAFETY: The embedded `struct device` carries the reference count, and `gb_hd_put()` is just
> +// `put_device()` on it, so the object stays alive for as long as increments are outstanding.
> +unsafe impl<Ctx> AlwaysRefCounted for Device<Ctx> {
> +    #[inline]
> +    fn inc_ref(&self) {
> +        // SAFETY: By the type invariant there is a live reference to the host device, and `dev` is
> +        // its embedded `struct device`.
> +        unsafe { bindings::get_device(&raw mut (*self.ptr.get()).dev) };
> +    }
> +
> +    #[inline]
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: The caller guarantees it owns an increment on the reference count.
> +        unsafe { bindings::gb_hd_put(obj.as_ptr().cast()) }
> +    }
> +}

[ ... ]

> +impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> +    #[inline]
> +    fn as_ref(&self) -> &device::Device<Ctx> {
> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> +        // `struct gb_host_device`. `dev` points to a valid `struct device`.
> +        unsafe { device::Device::from_raw(addr_of_mut!((*self.as_raw()).dev)) }
> +    }
> +}

[ ... ]

> +impl<T> Drop for Registration<T> {
> +    fn drop<'a>(&'a mut self) {
> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> +        // `struct gb_host_device`.
> +        unsafe { bindings::gb_hd_del(self.as_raw()) }
> +
> +        {
> +            // SAFETY: `hd` is a valid pointer to `gb_host_device`.
> +            let hd_dev = unsafe { Device::<device::CoreInternal<'_>>::from_raw(self.as_raw()) };
> +            // SAFETY: The driver data was set to a `T` in `Registration::new()` and has not been
> +            // taken since, and this is the only place that takes it.
> +            drop(unsafe { hd_dev.as_ref().drvdata_obtain::<T>() });
> +        }
> +
> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> +        // `struct gb_host_device`.
> +        unsafe { bindings::gb_hd_put(self.as_raw()) };
> +    }
> +}

[Severity: Medium]
Does implementing AlwaysRefCounted, Send, and Sync generically for Device<Ctx>
across all Ctx type parameters in rust/kernel/greybus/hd.rs violate the
device::DeviceContext soundness requirement?

In rust/kernel/device.rs, the DeviceContext guidelines state that
AlwaysRefCounted must only be implemented for Device<device::Normal> and never
for any other DeviceContext, because non-Normal contexts (such as Bound,
BoundInternal, Core, and CoreInternal) are only valid within a specific
scope.

Here in rust/kernel/greybus/hd.rs, pub struct Device<Ctx = device::Normal>
does not constrain Ctx: device::DeviceContext and implements AlwaysRefCounted
for all Ctx, while Device::as_ref() implements AsRef<device::Device<Ctx>> for
all Ctx: device::DeviceContext.

If a scoped reference &Device<Ctx> (such as Device<device::CoreInternal<'_>>
in HdDriverVTable::message_send() or Device<device::Bound>) is converted via
ARef::from() into an ARef<Device<Ctx>>, it only increments the underlying
struct device reference count via get_device(). Once Registration<T>::drop()
runs gb_hd_del() and frees the driver private data via drvdata_obtain::<T>(),
any surviving ARef<Device<Ctx>> can still be converted via Device::as_ref()
into &device::Device<Ctx> with the scoped context preserved, allowing access
to driver private data after it has been freed.

Should Device instead bound Ctx: device::DeviceContext = device::Normal,
implement AlwaysRefCounted only for Device (that is, Device<device::Normal>),
and use kernel::impl_device_context_deref!(unsafe { Device }) and
kernel::impl_device_context_into_aref!(Device)?

> diff --git a/rust/kernel/greybus/mod.rs b/rust/kernel/greybus/mod.rs
> new file mode 100644
> index 0000000000000..ea9dc954d0634
> --- /dev/null
> +++ b/rust/kernel/greybus/mod.rs
> @@ -0,0 +1,220 @@

[ ... ]

> +/// A Greybus interface.
> +///
> +/// # Invariants
> +///
> +/// The wrapped value is a valid `struct gb_interface`.
> +#[repr(transparent)]
> +pub struct Interface(Opaque<bindings::gb_interface>);
> +
> +impl Interface {
> +    /// Returns the interface id, unique within its host device.
> +    #[inline]
> +    pub const fn id(&self) -> u8 {
> +        // SAFETY: By the type invariant the interface is valid.
> +        unsafe { (*self.0.get()).interface_id }
> +    }
> +}
> +
> +// SAFETY: The embedded `struct device` carries the reference count, which `get_device()` and
> +// `put_device()` manipulate, so the object stays alive for as long as increments are outstanding.
> +unsafe impl AlwaysRefCounted for Interface {
> +    #[inline]
> +    fn inc_ref(&self) {
> +        // SAFETY: By the type invariant there is a live reference to the interface, and `dev` is
> +        // its embedded `struct device`.
> +        unsafe { bindings::get_device(&raw mut (*self.0.get()).dev) };
> +    }
> +
> +    #[inline]
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: The caller guarantees it owns an increment on the reference count.
> +        unsafe {
> +            bindings::put_device(&raw mut (*obj.as_ptr().cast::<bindings::gb_interface>()).dev)
> +        }
> +    }
> +}

[Severity: Medium]
In rust/kernel/greybus/mod.rs, Interface implements AlwaysRefCounted, but is
it missing Send and Sync implementations?

Because Interface wraps Opaque<bindings::gb_interface>, which contains an
UnsafeCell and raw pointers, Interface is neither Send nor Sync by default.
In rust/kernel/sync/aref.rs, ARef<T> only implements Send and Sync when
T: AlwaysRefCounted + Sync + Send:

    unsafe impl<T: AlwaysRefCounted + Sync + Send> Send for ARef<T> {}
    unsafe impl<T: AlwaysRefCounted + Sync + Send> Sync for ARef<T> {}

As a result, if a Greybus host driver implementing HdDriver (which requires
Send + Sync + 'static) obtains an &Interface from Connection::interface() and
converts it into an ARef<Interface>, that ARef<Interface> cannot be stored in
the driver state (even inside a Mutex or SpinLock) or sent across threads.

Also, the commit message states that "some types such as Greybus Interface do
not implement AlwaysRefCounted at the moment", which contradicts this impl.
Should Interface implement Send and Sync here (and the commit message be
updated), or was AlwaysRefCounted not intended to be implemented on Interface
yet?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-gb-uart-transport-v4-0-c308ce821d45@beagleboard.org?part=3