* [PATCH v4 1/6] greybus: connection: Add greybus_data_rcvd_by_intf()
2026-10-21 11:37 [PATCH v4 0/6] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
@ 2026-10-21 11:37 ` Ayush Singh
2026-10-21 11:37 ` [PATCH v4 2/6] rust: crc_ccitt: add CRC-CCITT abstraction Ayush Singh
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Ayush Singh @ 2026-10-21 11:37 UTC (permalink / raw)
To: Jason Kridner, robertcnelson, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Eric Biggers, Ard Biesheuvel,
Ayush Singh, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Lorenzo Stoakes, Vlastimil Babka, Liam R. Howlett,
Uladzislau Rezki, Markus Probst
Cc: greybus-dev, linux-kernel, rust-for-linux, linux-crypto,
devicetree, Ayush Singh
greybus_data_rcvd() finds the receiving connection by its host-side
CPort id. A host driver such as a software SVC sees only the remote end
of a connection, the interface id and the interface CPort id, and cannot
deliver data that way.
Add greybus_data_rcvd_by_intf(), which looks the connection up by
interface id and interface CPort id and passes the data to it.
It returns -EINVAL if no such connection exists. The first user is the
gb-softsvc driver, added later in this series.
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
drivers/greybus/connection.c | 47 ++++++++++++++++++++++++++++++++++++++
include/linux/greybus/connection.h | 2 ++
2 files changed, 49 insertions(+)
diff --git a/drivers/greybus/connection.c b/drivers/greybus/connection.c
index bd04485decb3..ce8646299ce8 100644
--- a/drivers/greybus/connection.c
+++ b/drivers/greybus/connection.c
@@ -69,6 +69,32 @@ gb_connection_hd_find(struct gb_host_device *hd, u16 cport_id)
return connection;
}
+/*
+ * Returns a reference-counted pointer to the connection if found.
+ */
+static struct gb_connection *
+gb_connection_hd_find_by_intf(struct gb_host_device *hd, u8 intf_id,
+ u16 intf_cport)
+{
+ struct gb_connection *connection;
+ unsigned long flags;
+
+ spin_lock_irqsave(&gb_connections_lock, flags);
+ list_for_each_entry(connection, &hd->connections, hd_links) {
+ if (connection->intf &&
+ connection->intf->interface_id == intf_id &&
+ connection->intf_cport_id == intf_cport) {
+ gb_connection_get(connection);
+ goto found;
+ }
+ }
+ connection = NULL;
+found:
+ spin_unlock_irqrestore(&gb_connections_lock, flags);
+
+ return connection;
+}
+
/*
* Callback from the host driver to let us know that data has been
* received on the bundle.
@@ -91,6 +117,27 @@ void greybus_data_rcvd(struct gb_host_device *hd, u16 cport_id,
}
EXPORT_SYMBOL_GPL(greybus_data_rcvd);
+/*
+ * Callback from a host driver that only knows the remote end of a connection
+ * (interface id and interface CPort id), such as a software SVC.
+ */
+int greybus_data_rcvd_by_intf(struct gb_host_device *hd, u8 intf_id,
+ u16 intf_cport, u8 *data, size_t length)
+{
+ struct gb_connection *connection;
+
+ trace_gb_hd_in(hd);
+
+ connection = gb_connection_hd_find_by_intf(hd, intf_id, intf_cport);
+ if (!connection)
+ return -EINVAL;
+ gb_connection_recv(connection, data, length);
+ gb_connection_put(connection);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(greybus_data_rcvd_by_intf);
+
static void gb_connection_kref_release(struct kref *kref)
{
struct gb_connection *connection;
diff --git a/include/linux/greybus/connection.h b/include/linux/greybus/connection.h
index d59b7fc1de3e..e8be67a936a6 100644
--- a/include/linux/greybus/connection.h
+++ b/include/linux/greybus/connection.h
@@ -92,6 +92,8 @@ void gb_connection_mode_switch_complete(struct gb_connection *connection);
void greybus_data_rcvd(struct gb_host_device *hd, u16 cport_id,
u8 *data, size_t length);
+int greybus_data_rcvd_by_intf(struct gb_host_device *hd, u8 intf_id,
+ u16 intf_cport, u8 *data, size_t length);
void gb_connection_latency_tag_enable(struct gb_connection *connection);
void gb_connection_latency_tag_disable(struct gb_connection *connection);
--
2.56.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v4 2/6] rust: crc_ccitt: add CRC-CCITT abstraction
2026-10-21 11:37 [PATCH v4 0/6] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
2026-10-21 11:37 ` [PATCH v4 1/6] greybus: connection: Add greybus_data_rcvd_by_intf() Ayush Singh
@ 2026-10-21 11:37 ` Ayush Singh
2026-10-21 11:37 ` [PATCH v4 3/6] rust: kernel: Add greybus abstractions Ayush Singh
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Ayush Singh @ 2026-10-21 11:37 UTC (permalink / raw)
To: Jason Kridner, robertcnelson, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Eric Biggers, Ard Biesheuvel,
Ayush Singh, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Lorenzo Stoakes, Vlastimil Babka, Liam R. Howlett,
Uladzislau Rezki, Markus Probst
Cc: greybus-dev, linux-kernel, rust-for-linux, linux-crypto,
devicetree, Ayush Singh
The module is gated behind the new RUST_CRC_CCITT_ABSTRACTIONS Kconfig
symbol, which selects CRC_CCITT, so the C library is only built when a
Rust user actually needs it.
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
MAINTAINERS | 1 +
lib/crc/Kconfig | 7 +++++++
rust/bindings/bindings_helper.h | 1 +
rust/kernel/crc_ccitt.rs | 26 ++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 ++
5 files changed, 37 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 252b6d7ee163..772023e0e5a4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7036,6 +7036,7 @@ T: git https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git crc-ne
F: Documentation/staging/crc*
F: include/linux/crc*
F: lib/crc/
+F: rust/kernel/crc*
F: scripts/gen-crc-consts.py
CREATIVE SB0540
diff --git a/lib/crc/Kconfig b/lib/crc/Kconfig
index 927fc6a6b2b9..628da2d30fc8 100644
--- a/lib/crc/Kconfig
+++ b/lib/crc/Kconfig
@@ -32,6 +32,13 @@ config CRC_CCITT
The CRC-CCITT library functions. Select this if your module uses any
of the functions from <linux/crc-ccitt.h>.
+config RUST_CRC_CCITT_ABSTRACTIONS
+ bool
+ depends on RUST
+ select CRC_CCITT
+ help
+ This enables the Rust abstraction for the CRC-CCITT API.
+
config CRC_ITU_T
tristate
help
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 3791af0aadc7..55d1a2d7655a 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -49,6 +49,7 @@
#include <linux/cpu.h>
#include <linux/cpufreq.h>
#include <linux/cpumask.h>
+#include <linux/crc-ccitt.h>
#include <linux/cred.h>
#include <linux/debugfs.h>
#include <linux/device/faux.h>
diff --git a/rust/kernel/crc_ccitt.rs b/rust/kernel/crc_ccitt.rs
new file mode 100644
index 000000000000..6042ada16967
--- /dev/null
+++ b/rust/kernel/crc_ccitt.rs
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! CRC-CCITT computation.
+//!
+//! C header: [`include/linux/crc-ccitt.h`](srctree/include/linux/crc-ccitt.h)
+
+/// Computes the CRC-CCITT of `data`, starting from the seed value `crc`.
+///
+/// Pass the result back in as `crc` to compute the checksum of a buffer
+/// incrementally.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::crc_ccitt::crc_ccitt;
+///
+/// let one_shot = crc_ccitt(0xffff, b"hello world");
+/// let split = crc_ccitt(crc_ccitt(0xffff, b"hello "), b"world");
+/// assert_eq!(one_shot, split);
+/// ```
+#[inline]
+pub fn crc_ccitt(crc: u16, data: &[u8]) -> u16 {
+ // SAFETY: `data.as_ptr()` is valid for reads of `data.len()` bytes for the
+ // duration of the call, since it is derived from a live shared slice.
+ unsafe { bindings::crc_ccitt(crc, data.as_ptr(), data.len()) }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index f9ef36217bb5..637442761d34 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -61,6 +61,8 @@
#[cfg(CONFIG_CPU_FREQ)]
pub mod cpufreq;
pub mod cpumask;
+#[cfg(CONFIG_RUST_CRC_CCITT_ABSTRACTIONS)]
+pub mod crc_ccitt;
pub mod cred;
pub mod debugfs;
pub mod device;
--
2.56.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v4 3/6] rust: kernel: Add greybus abstractions
2026-10-21 11:37 [PATCH v4 0/6] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
2026-10-21 11:37 ` [PATCH v4 1/6] greybus: connection: Add greybus_data_rcvd_by_intf() Ayush Singh
2026-10-21 11:37 ` [PATCH v4 2/6] rust: crc_ccitt: add CRC-CCITT abstraction Ayush Singh
@ 2026-10-21 11:37 ` Ayush Singh
2026-10-21 11:55 ` sashiko-bot
2026-10-21 11:37 ` [PATCH v4 4/6] drivers: greybus: Add software SVC implementation Ayush Singh
` (2 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Ayush Singh @ 2026-10-21 11:37 UTC (permalink / raw)
To: Jason Kridner, robertcnelson, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Eric Biggers, Ard Biesheuvel,
Ayush Singh, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Lorenzo Stoakes, Vlastimil Babka, Liam R. Howlett,
Uladzislau Rezki, Markus Probst
Cc: greybus-dev, linux-kernel, rust-for-linux, linux-crypto,
devicetree, Ayush Singh
Add Rust abstractions for the Greybus core, enough to implement a
Greybus host driver in Rust.
The abstractions are split into 3 modules:
- Top level mod.rs: Basic abstractions greybus structures.
- hd.rs: Greybus Host Device specific abstractions.
- protocols.rs: Abstractions for greybus_protocols.h, i.e. greybus
protocol types and constants.
Only the APIs used by gb-softsvc and gb-uart-node driver are covered.
As such, protocols.rs also only contains abstractions for greybus
structures that are used by SVC and some types such as Greybus Interface
do not implement AlwaysRefCounted at the moment.
HdDriver Note
**************
- In C message_send callback, it is possible for the caller to defer
calling gb_message_sent. This is used in es2 driver. However, looking at
the internals, it does not seem like the message owenership is actually
transferred to the caller in real terms. Instead, it is more like the
caller is shared ownership. This is evident by the fact that
message_cancel can be called at any point, and depending on wheather the
message is request or response, the message can become invalid after the
message_cancel callback. Thus the driver is expected to ensure that no
dangling references to that message exist.
Since gb_message is not an internally ref counted structure in true
terms (it does seem to use the ref counting in the internal operation),
and since double calls to gb_message_sent are not really allowed, I have
made the decision to call gb_message_sent on a message in the
abstractions rather than making the driver call it. This means the
driver only gets a reference to the message, and never owns it. This
does prevent a driver to use it in the way es2 currently does, but seems
much less of a footgun.
Another possibility would be to implement AlwaysRefCounted on Message
and pass the calls to gb_operation_get/put, but I have not gone through
the internals enough to figure out if this is correct in all cases.
- At the moment, nothing in-tree calls message_send from an atomic
context. However, since it does have gfp_mask in param, and an
exported function, it is possible to call it from atomic context. So
not sure if the doc comment should specific non-sleep requirement.
- message_cancel does not directly provide access to driver data. This
was flagged by sashiko-bot. It actually is possible for the Rust
abstractions to provide the driver data though. This is because we can
do gb_message->operation->connection->host_device->priv_data. However,
not sure if we should do this just in Rust abstractions. Maybe it
would be preferable to change the C API itself instead.
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
MAINTAINERS | 1 +
drivers/greybus/Kconfig | 8 +
rust/bindings/bindings_helper.h | 1 +
rust/kernel/alloc.rs | 6 +
rust/kernel/greybus/hd.rs | 362 ++++++++++++++++++++++++++++++++
rust/kernel/greybus/mod.rs | 220 +++++++++++++++++++
rust/kernel/greybus/protocols.rs | 442 +++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
8 files changed, 1042 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 772023e0e5a4..4d98db7618c3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11526,6 +11526,7 @@ F: drivers/greybus/
F: drivers/staging/greybus/
F: include/linux/greybus.h
F: include/linux/greybus/
+F: rust/kernel/greybus/
GREYBUS UART PROTOCOLS DRIVERS
M: David Lin <dtwlin@gmail.com>
diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig
index c3f056d28b01..d9f52024d1c2 100644
--- a/drivers/greybus/Kconfig
+++ b/drivers/greybus/Kconfig
@@ -15,6 +15,14 @@ menuconfig GREYBUS
To compile this code as a module, choose M here: the module
will be called greybus.ko
+config RUST_GREYBUS_ABSTRACTIONS
+ bool "Rust greybus abstractions"
+ depends on RUST
+ depends on SYSFS
+ select GREYBUS
+ help
+ This enables the Rust abstraction for the Greybus API.
+
if GREYBUS
config GREYBUS_BEAGLEPLAY
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 55d1a2d7655a..91ac64687d23 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -64,6 +64,7 @@
#include <linux/firmware.h>
#include <linux/fwctl.h>
#include <linux/fs.h>
+#include <linux/greybus.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
#include <linux/io-pgtable.h>
diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index 68d80f3fa6c8..b3c55fb2c2f1 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -38,6 +38,12 @@
pub struct Flags(u32);
impl Flags {
+ /// Creates [`Flags`] from a raw `gfp_t`.
+ #[cfg_attr(not(CONFIG_RUST_GREYBUS_ABSTRACTIONS), expect(dead_code))]
+ pub(crate) fn new(v: bindings::gfp_t) -> Self {
+ Self(v)
+ }
+
/// Get the raw representation of this flag.
pub(crate) fn as_raw(self) -> u32 {
self.0
diff --git a/rust/kernel/greybus/hd.rs b/rust/kernel/greybus/hd.rs
new file mode 100644
index 000000000000..45a2d53ee80f
--- /dev/null
+++ b/rust/kernel/greybus/hd.rs
@@ -0,0 +1,362 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Greybus host device abstractions.
+//!
+//! A driver implements the transmit path through [`HdDriver`] and feeds received data back into
+//! the core with [`Device::data_rcvd`].
+//!
+//! A host device is created, populated with driver private data and added to the Greybus bus by
+//! constructing a [`Registration`], which owns the underlying `struct gb_host_device`. Dropping
+//! the registration drops the private data and removes the device from the bus, so it is normally
+//! stored in the driver data of the parent device and torn down implicitly when that device goes
+//! away.
+//!
+//! Individual references to a live host device are represented by
+//! [`ARef<Device>`](crate::sync::aref::ARef), which keeps the embedded `struct device` reference
+//! count balanced.
+//!
+//! C header: [`include/linux/greybus/hd.h`](srctree/include/linux/greybus/hd.h)
+//!
+//! # 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) {}
+//! }
+//!
+//! fn probe(parent: &device::Device) -> Result<Registration<MyHd>> {
+//! Registration::new(parent, BUFFER_SIZE_MAX, NUM_CPORTS, try_pin_init!(MyHd {}))
+//! }
+//! ```
+
+use core::{
+ marker::PhantomData,
+ ptr::{
+ addr_of_mut,
+ NonNull, //
+ },
+};
+
+use crate::{
+ alloc::Flags,
+ device,
+ error::{
+ code,
+ from_err_ptr,
+ to_result, //
+ },
+ greybus::Message,
+ prelude::*,
+ sync::aref::AlwaysRefCounted,
+ types::Opaque,
+};
+
+/// The set of callbacks a Greybus host driver can provide.
+#[vtable]
+pub trait HdDriver: Send + Sync + Sized + 'static {
+ /// Transmits `msg` to `dest_cport_id`.
+ ///
+ /// The message is only borrowed for the duration of this call: copy what is needed out of it,
+ /// do not stash the reference.
+ ///
+ /// `hd` is the host device the message is sent on. It is only guaranteed to be alive for the
+ /// duration of this call.
+ fn message_send(
+ data: &Self,
+ hd: &Device,
+ dest_cport_id: u16,
+ msg: &Message,
+ gfp_mask: Flags,
+ ) -> Result;
+
+ /// Aborts the transmission of a message previously handed to [`HdDriver::message_send`].
+ ///
+ /// Always called in process context.
+ fn message_cancel(msg: &Message);
+}
+
+/// Builds the C callback table for a [`HdDriver`] implementation.
+struct HdDriverVTable<T: HdDriver>(PhantomData<T>);
+
+impl<T: HdDriver> HdDriverVTable<T> {
+ const DRIVER: bindings::gb_hd_driver = bindings::gb_hd_driver {
+ // Both are mandatory, hence no `HAS_*` check. See [`HdDriver`].
+ message_send: Some(Self::message_send),
+ message_cancel: Some(Self::message_cancel),
+ // Every other callback is left `NULL`, which the Greybus core takes as "use the default
+ // behaviour".
+ ..pin_init::zeroed()
+ };
+
+ const fn build() -> &'static bindings::gb_hd_driver {
+ &Self::DRIVER
+ }
+
+ /// # Safety
+ ///
+ /// `hd` must point at a registered host device whose private area holds a pointer to a live
+ /// `T`, and `msg` must point at a valid message.
+ unsafe extern "C" fn message_send(
+ hd: *mut bindings::gb_host_device,
+ dest_cport_id: u16,
+ msg: *mut bindings::gb_message,
+ gfp_mask: bindings::gfp_t,
+ ) -> c_int {
+ // SAFETY: `gb_host_device` and `HostDevice` have the same layout.
+ let hd = unsafe { Device::<device::CoreInternal<'_>>::from_raw(hd) };
+ // SAFETY: `message_send` is only ever called after a successful call to
+ // `gb_hd_add`, hence it's guaranteed that `Device::set_drvdata()` has been called
+ // and stored a `Pin<KBox<T>>`.
+ let data = unsafe { hd.as_ref().drvdata_borrow() };
+ // SAFETY: The caller guarantees `msg` is valid for the duration of this call.
+ let msg = unsafe { Message::from_raw(msg) };
+
+ // SAFETY: The caller guarantees `hd` is a registered host device, alive for the duration
+ // of this call.
+ let hd_normal = unsafe { Device::<device::Normal>::from_raw(hd.as_raw()) };
+
+ match T::message_send(&data, hd_normal, dest_cport_id, msg, Flags::new(gfp_mask)) {
+ Ok(()) => {
+ // SAFETY: `msg` is still valid since `sent` is not called anywhere else.
+ unsafe { msg.sent(hd, 0) };
+ 0
+ }
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ ///
+ /// `msg` must point at a valid message of a registered host device of this driver.
+ unsafe extern "C" fn message_cancel(msg: *mut bindings::gb_message) {
+ // SAFETY: The caller guarantees `msg` is valid for the duration of this call.
+ let msg = unsafe { Message::from_raw(msg) };
+ T::message_cancel(msg);
+ }
+}
+
+/// 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<Ctx> {
+ /// # Safety
+ ///
+ /// `ptr` must be a valid pointer to a `struct gb_host_device`.
+ #[inline]
+ pub(crate) const unsafe fn from_raw<'a>(ptr: *mut bindings::gb_host_device) -> &'a Self {
+ // SAFETY: `Device` is a transparent wrapper of `Opaque<bindings::gb_host_device>`.
+ unsafe { &*ptr.cast() }
+ }
+
+ #[inline]
+ pub(crate) fn as_raw(&self) -> *mut bindings::gb_host_device {
+ self.ptr.get()
+ }
+
+ /// Hands a message received on `hd_cport_id` to the Greybus core.
+ #[inline]
+ pub fn data_rcvd(&self, cport_id: u16, msg: &[u8]) {
+ // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
+ // `struct gb_host_device`. `msg` is valid for reads of ``msg.len()` bytes for the duration
+ // of the call, and the core only reads through the pointer — it copies the payload into
+ // the operation before returning — so handing it a `*mut` derived from a shared reference
+ // is sound.
+ //
+ // TODO: The C signature of this function should be changed to take const pointer for msg.
+ unsafe {
+ bindings::greybus_data_rcvd(self.as_raw(), cport_id, msg.as_ptr().cast_mut(), msg.len())
+ }
+ }
+
+ /// Hands `msg`, received from CPort `cport` of interface `intf_id`, to the Greybus core.
+ ///
+ /// Fails with `EINVAL` if no connection is bound to that pair.
+ #[inline]
+ pub fn data_rcvd_by_intf(&self, intf_id: u8, cport: u16, msg: &[u8]) -> Result {
+ // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
+ // `struct gb_host_device`. `msg` is valid for reads of `msg.len()` bytes for the duration
+ // of the call, and the core only reads through the pointer — it copies the payload into
+ // the operation before returning — so handing it a `*mut` derived from a shared reference
+ // is sound.
+ //
+ // TODO: The C signature of this function should be changed to take const pointer for msg.
+ to_result(unsafe {
+ bindings::greybus_data_rcvd_by_intf(
+ self.as_raw(),
+ intf_id,
+ cport,
+ msg.as_ptr().cast_mut(),
+ msg.len(),
+ )
+ })
+ }
+}
+
+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)) }
+ }
+}
+
+/// A host device owned by its driver, together with the driver's private data `T`.
+///
+/// Created and added to the Greybus bus on construction, removed on drop.
+///
+/// # Invariants
+///
+/// `ptr` points at a valid `struct gb_host_device` obtained from `gb_hd_create()`, whose driver
+/// data holds a live `T`
+#[repr(transparent)]
+pub struct Registration<T> {
+ ptr: NonNull<bindings::gb_host_device>,
+ _data: PhantomData<T>,
+}
+
+impl<T: HdDriver> Registration<T> {
+ /// Creates a host device under `parent` and adds it to the Greybus bus.
+ ///
+ /// `buffer_size_max` is the largest message the transport can carry in one go, header
+ /// included. `num_cports` is the number of cports that the greybus host device can connect to.
+ ///
+ /// `data` is initialised in place before the device is added, so callbacks may run against it
+ /// from the moment `gb_hd_add()` succeeds.
+ pub fn new(
+ parent: &device::Device,
+ buffer_size_max: usize,
+ num_cports: usize,
+ data: impl PinInit<T, Error>,
+ ) -> Result<Self> {
+ // SAFETY: `parent` is a valid device, and the driver table is `'static`. The core only
+ // ever reads through the driver pointer, so casting away `const` is fine.
+ let hd = from_err_ptr(unsafe {
+ bindings::gb_hd_create(
+ core::ptr::from_ref(HdDriverVTable::<T>::build()).cast_mut(),
+ parent.as_raw(),
+ buffer_size_max,
+ num_cports,
+ )
+ })?;
+
+ // SAFETY: `hd` is a valid pointer to `gb_host_device`.
+ let hd_dev: &Device<device::CoreInternal<'_>> = unsafe { Device::from_raw(hd) };
+ if let Err(e) = hd_dev.as_ref().set_drvdata(data) {
+ // SAFETY: `hd` is a pointer to a valid `struct gb_host_device`.
+ unsafe { bindings::gb_hd_put(hd) };
+
+ return Err(e);
+ }
+
+ // SAFETY: By the type invariant the host device is valid, and it has not been added yet.
+ let res = to_result(unsafe { bindings::gb_hd_add(hd) });
+ if let Err(e) = res {
+ // SAFETY: The driver data was set to a `T` above and has not been taken since.
+ drop(unsafe { hd_dev.as_ref().drvdata_obtain::<T>() });
+ // SAFETY: `hd` is a pointer to a valid `struct gb_host_device`.
+ unsafe { bindings::gb_hd_put(hd) };
+
+ return Err(e);
+ }
+
+ // INVARIANT: `gb_hd_create()` returns a valid `gb_host_device` on success,
+ // and we own the reference it returns until `Drop`.
+ Ok(Self {
+ ptr: NonNull::new(hd).ok_or(code::ENOMEM)?,
+ _data: PhantomData,
+ })
+ }
+}
+
+impl<T> Registration<T> {
+ #[inline]
+ fn as_raw(&self) -> *mut bindings::gb_host_device {
+ self.ptr.as_ptr()
+ }
+}
+
+impl<T> AsRef<Device> for Registration<T> {
+ #[inline]
+ fn as_ref(&self) -> &Device {
+ // SAFETY: By the type invariant the host device is valid.
+ unsafe { Device::from_raw(self.as_raw()) }
+ }
+}
+
+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()) };
+ }
+}
+
+// SAFETY: The greybus host device API is thread-safe as guaranteed by the device core, as long as
+// gb_hd_del() is guaranteed to only be called once - which is guaranteed by our type not
+// having Copy/Clone.
+unsafe impl<T: Send> Send for Registration<T> {}
+
+// SAFETY: The greybus device API is thread-safe as guaranteed by the device core, as long as
+// gb_hd_del() is guaranteed to only be called once - which is guaranteed by our type not
+// having Copy/Clone.
+unsafe impl<T: Sync> Sync for Registration<T> {}
diff --git a/rust/kernel/greybus/mod.rs b/rust/kernel/greybus/mod.rs
new file mode 100644
index 000000000000..ea9dc954d063
--- /dev/null
+++ b/rust/kernel/greybus/mod.rs
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for the Greybus core.
+//!
+//! C header: [`include/linux/greybus.h`](srctree/include/linux/greybus.h)
+//!
+//! Greybus host drivers ("host devices") bridge the Greybus core to whatever transport actually
+//! carries the traffic. A host driver creates a [`HostDevice`], registers it, and implements
+//! [`HdDriver`] to transmit the messages the core hands it.
+
+use core::ptr::NonNull;
+
+use crate::{
+ sync::aref::AlwaysRefCounted,
+ transmute::FromBytes,
+ types::Opaque, //
+};
+
+pub mod hd;
+pub mod protocols;
+
+/// The largest Greybus message, header included.
+///
+/// Bounded by the 16-bit `size` field in the operation header.
+pub const GB_OPERATION_SIZE_MAX: usize = u16::MAX as usize;
+
+/// The largest valid CPort id.
+///
+/// Ids above this are reserved by the protocol; a host device's `num_cports` cannot exceed
+/// `CPORT_ID_MAX + 1`.
+pub const CPORT_ID_MAX: usize = bindings::CPORT_ID_MAX as usize;
+
+/// A Greybus message handed to a host driver for transmission.
+///
+/// # Invariants
+///
+/// The shared reference is only ever handed out for the duration of a [`HdDriver`] callback, during
+/// which the Greybus core guarantees the message and its buffer stay alive.
+#[repr(transparent)]
+pub struct Message(Opaque<bindings::gb_message>);
+
+impl Message {
+ /// # Safety
+ ///
+ /// `ptr` must be non-null and point at a valid `struct gb_message` which outlives the
+ /// returned `Self`.
+ #[inline]
+ pub(crate) const unsafe fn from_raw<'a>(ptr: *mut bindings::gb_message) -> &'a Self {
+ // SAFETY: `Message` is a transparent wrapper of `Opaque<bindings::gb_message>`.
+ unsafe { &*ptr.cast() }
+ }
+
+ /// Returns the operation header at the start of the message.
+ #[inline]
+ pub const fn header(&self) -> &protocols::GbOperationMsgHdr {
+ // SAFETY: `header` points at a valid `gb_operation_msg_hdr` for as long as the message is
+ // alive, and `GbOperationMsgHdr` is a transparent wrapper of it.
+ unsafe { &*(*self.0.get()).header.cast() }
+ }
+
+ /// # Safety
+ ///
+ /// `Message` is not valid after this call.
+ #[inline]
+ pub(crate) unsafe fn sent<Ctx>(&self, hd: &hd::Device<Ctx>, status: i32) {
+ // SAFETY: By the type invariant the message is valid and still owned by the transport,
+ // and its connection and host device are alive for as long as it is.
+ unsafe {
+ bindings::greybus_message_sent(hd.as_raw(), self.0.get(), status);
+ }
+ }
+
+ /// Returns the payload, i.e. the message without its operation header.
+ #[inline]
+ pub const fn payload_bytes(&self) -> &[u8] {
+ // SAFETY: By the type invariant the message is valid.
+ let payload = unsafe { (*self.0.get()).payload };
+ // SAFETY: By the type invariant the message is valid.
+ let payload_size = unsafe { (*self.0.get()).payload_size };
+
+ if payload.is_null() || payload_size == 0 {
+ return &[];
+ }
+
+ // SAFETY: A non-null `payload` points at `payload_size` initialized bytes.
+ unsafe { core::slice::from_raw_parts(payload.cast::<u8>(), payload_size) }
+ }
+
+ /// Interprets the message payload as a `T`.
+ ///
+ /// Returns `None` if the payload is too short or misaligned for `T`.
+ #[inline]
+ pub fn payload<T: FromBytes>(&self) -> Option<&T> {
+ T::from_bytes(self.payload_bytes())
+ }
+
+ /// Returns the operation this message belongs to.
+ #[inline]
+ pub const fn operation(&self) -> &Operation {
+ // SAFETY: By the type invariant the message is valid, and its `operation` is set for as
+ // long as the message is alive.
+ unsafe { Operation::from_raw((*self.0.get()).operation) }
+ }
+}
+
+/// A Greybus operation.
+///
+/// # Invariants
+///
+/// The wrapped value is a valid `struct gb_operation`.
+#[repr(transparent)]
+pub struct Operation(Opaque<bindings::gb_operation>);
+
+impl Operation {
+ /// # Safety
+ ///
+ /// `ptr` must be a valid pointer to a `struct gb_operation`.
+ #[inline]
+ pub(crate) const unsafe fn from_raw<'a>(ptr: *mut bindings::gb_operation) -> &'a Self {
+ // SAFETY: `Operation` is a transparent wrapper of `Opaque<bindings::gb_operation>`.
+ unsafe { &*ptr.cast() }
+ }
+
+ /// Returns the connection this operation travels on.
+ #[inline]
+ pub const fn connection(&self) -> &Connection {
+ // SAFETY: By the type invariant the operation is valid, and its `connection` is alive for
+ // as long as the operation is. The returned borrow is tied to `&self`, so it cannot
+ // outlive the host-device callback that handed out the operation, as `Connection`'s
+ // invariant requires.
+ unsafe { Connection::from_raw((*self.0.get()).connection) }
+ }
+}
+
+/// A Greybus connection.
+///
+/// # Invariants
+///
+/// The wrapped value is a valid `struct gb_connection`.
+///
+/// References to a `Connection` are only handed out for the duration of a host-device callback on
+/// a message travelling over it. During that time the connection is enabled, and its interface,
+/// if any, is alive: the core disables and destroys a bundle's connections before it tears down
+/// their interface.
+#[repr(transparent)]
+pub struct Connection(Opaque<bindings::gb_connection>);
+
+impl Connection {
+ /// # Safety
+ ///
+ /// `ptr` must be a valid pointer to a `struct gb_connection`.
+ #[inline]
+ pub(crate) const unsafe fn from_raw<'a>(ptr: *mut bindings::gb_connection) -> &'a Self {
+ // SAFETY: `Connection` is a transparent wrapper of `Opaque<bindings::gb_connection>`.
+ unsafe { &*ptr.cast() }
+ }
+
+ /// Returns the interface at the far end of the connection. In cases like SVC connection,
+ /// interface can be NULL.
+ #[inline]
+ pub fn interface(&self) -> Option<&Interface> {
+ // SAFETY: By the type invariant the connection is valid.
+ let intf_ptr = NonNull::new(unsafe { (*self.0.get()).intf })?;
+
+ // SAFETY: `intf_ptr` is non-NULL, and by the type invariant the interface is alive for as
+ // long as `&self` is. `Interface` is a transparent wrapper of
+ // `Opaque<bindings::gb_interface>`.
+ Some(unsafe { intf_ptr.cast().as_ref() })
+ }
+
+ /// Returns the CPort id this connection uses on the interface.
+ #[inline]
+ pub const fn intf_cport_id(&self) -> u16 {
+ // SAFETY: By the type invariant the connection is valid.
+ unsafe { (*self.0.get()).intf_cport_id }
+ }
+
+ /// Returns the CPort id this connection uses on the host device.
+ #[inline]
+ pub const fn hd_cport_id(&self) -> u16 {
+ // SAFETY: By the type invariant the connection is valid.
+ unsafe { (*self.0.get()).hd_cport_id }
+ }
+}
+
+/// 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)
+ }
+ }
+}
diff --git a/rust/kernel/greybus/protocols.rs b/rust/kernel/greybus/protocols.rs
new file mode 100644
index 000000000000..21c35b9d8ca1
--- /dev/null
+++ b/rust/kernel/greybus/protocols.rs
@@ -0,0 +1,442 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Greybus wire format definitions.
+//!
+//! Thin `repr(transparent)` wrappers over the generated bindings for the operation header and the
+//! SVC protocol messages, plus the constants that go in their type and result fields. The
+//! wrappers exist to keep the byte-order conversions in one place: constructors take native-endian
+//! values and store little-endian, accessors convert back.
+
+/// Set in the header type field to mark a message as a response to the operation of the same id.
+pub const MESSAGE_TYPE_RESPONSE: u8 = 0x80;
+
+/// The CPort id reserved for the SVC connection on every host device.
+pub const GB_SVC_CPORT_ID: u16 = bindings::GB_SVC_CPORT_ID as u16;
+
+/// Defines `u8` constants from same-named `bindings` values.
+macro_rules! gb_u8_consts {
+ ($($name:ident),* $(,)?) => {
+ $(
+ #[allow(missing_docs)]
+ pub const $name: u8 = bindings::$name as u8;
+ )*
+ };
+}
+
+// SVC Operation Types
+gb_u8_consts! {
+ GB_SVC_TYPE_PROTOCOL_VERSION,
+ GB_SVC_TYPE_SVC_HELLO,
+ GB_SVC_TYPE_INTF_DEVICE_ID,
+ GB_SVC_TYPE_INTF_RESET,
+ GB_SVC_TYPE_CONN_CREATE,
+ GB_SVC_TYPE_CONN_DESTROY,
+ GB_SVC_TYPE_DME_PEER_GET,
+ GB_SVC_TYPE_DME_PEER_SET,
+ GB_SVC_TYPE_ROUTE_CREATE,
+ GB_SVC_TYPE_ROUTE_DESTROY,
+ GB_SVC_TYPE_TIMESYNC_ENABLE,
+ GB_SVC_TYPE_TIMESYNC_DISABLE,
+ GB_SVC_TYPE_TIMESYNC_AUTHORITATIVE,
+ GB_SVC_TYPE_INTF_SET_PWRM,
+ GB_SVC_TYPE_INTF_EJECT,
+ GB_SVC_TYPE_PING,
+ GB_SVC_TYPE_PWRMON_RAIL_COUNT_GET,
+ GB_SVC_TYPE_PWRMON_RAIL_NAMES_GET,
+ GB_SVC_TYPE_PWRMON_SAMPLE_GET,
+ GB_SVC_TYPE_PWRMON_INTF_SAMPLE_GET,
+ GB_SVC_TYPE_TIMESYNC_WAKE_PINS_ACQUIRE,
+ GB_SVC_TYPE_TIMESYNC_WAKE_PINS_RELEASE,
+ GB_SVC_TYPE_TIMESYNC_PING,
+ GB_SVC_TYPE_MODULE_INSERTED,
+ GB_SVC_TYPE_MODULE_REMOVED,
+ GB_SVC_TYPE_INTF_VSYS_ENABLE,
+ GB_SVC_TYPE_INTF_VSYS_DISABLE,
+ GB_SVC_TYPE_INTF_REFCLK_ENABLE,
+ GB_SVC_TYPE_INTF_REFCLK_DISABLE,
+ GB_SVC_TYPE_INTF_UNIPRO_ENABLE,
+ GB_SVC_TYPE_INTF_UNIPRO_DISABLE,
+ GB_SVC_TYPE_INTF_ACTIVATE,
+ GB_SVC_TYPE_INTF_RESUME,
+ GB_SVC_TYPE_INTF_MAILBOX_EVENT,
+ GB_SVC_TYPE_INTF_OOPS,
+}
+
+// UNIPRO modes
+gb_u8_consts! {
+ GB_SVC_UNIPRO_FAST_MODE,
+ GB_SVC_UNIPRO_SLOW_MODE,
+ GB_SVC_UNIPRO_FAST_AUTO_MODE,
+ GB_SVC_UNIPRO_SLOW_AUTO_MODE,
+ GB_SVC_UNIPRO_MODE_UNCHANGED,
+ GB_SVC_UNIPRO_HIBERNATE_MODE,
+ GB_SVC_UNIPRO_OFF_MODE,
+}
+
+// PWR States
+gb_u8_consts! {
+ GB_SVC_SETPWRM_PWR_OK,
+ GB_SVC_SETPWRM_PWR_LOCAL,
+ GB_SVC_SETPWRM_PWR_REMOTE,
+ GB_SVC_SETPWRM_PWR_BUSY,
+ GB_SVC_SETPWRM_PWR_ERROR_CAP,
+ GB_SVC_SETPWRM_PWR_FATAL_ERROR,
+}
+
+// Vsys Result
+gb_u8_consts! {
+ GB_SVC_INTF_VSYS_OK,
+ GB_SVC_INTF_VSYS_FAIL,
+}
+
+// Refclk Result
+gb_u8_consts! {
+ GB_SVC_INTF_REFCLK_OK,
+ GB_SVC_INTF_REFCLK_FAIL,
+}
+
+// Unipro Result
+gb_u8_consts! {
+ GB_SVC_INTF_UNIPRO_OK,
+ GB_SVC_INTF_UNIPRO_FAIL,
+ GB_SVC_INTF_UNIPRO_NOT_OFF,
+}
+
+// Op Codes
+gb_u8_consts! {
+ GB_SVC_OP_SUCCESS,
+ GB_SVC_OP_UNKNOWN_ERROR,
+ GB_SVC_INTF_NOT_DETECTED,
+ GB_SVC_INTF_NO_UPRO_LINK,
+ GB_SVC_INTF_UPRO_NOT_DOWN,
+ GB_SVC_INTF_UPRO_NOT_HIBERNATED,
+ GB_SVC_INTF_NO_V_SYS,
+ GB_SVC_INTF_V_CHG,
+ GB_SVC_INTF_WAKE_BUSY,
+ GB_SVC_INTF_NO_REFCLK,
+ GB_SVC_INTF_RELEASING,
+ GB_SVC_INTF_NO_ORDER,
+ GB_SVC_INTF_MBOX_SET,
+ GB_SVC_INTF_BAD_MBOX,
+ GB_SVC_INTF_OP_TIMEOUT,
+ GB_SVC_PWRMON_OP_NOT_PRESENT,
+}
+
+// Greybus Interface Types
+gb_u8_consts! {
+ GB_SVC_INTF_TYPE_UNKNOWN,
+ GB_SVC_INTF_TYPE_DUMMY,
+ GB_SVC_INTF_TYPE_UNIPRO,
+ GB_SVC_INTF_TYPE_GREYBUS,
+}
+
+/// The header every Greybus message starts with.
+///
+/// Its `size` field is the size of the whole message, this header included.
+#[repr(transparent)]
+pub struct GbOperationMsgHdr(bindings::gb_operation_msg_hdr);
+
+// SAFETY: `gb_operation_msg_hdr` is a POD type with no padding and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbOperationMsgHdr {}
+
+impl GbOperationMsgHdr {
+ /// Builds a header. `size` is the whole message, this header included.
+ #[inline]
+ pub const fn new(size: u16, operation_id: u16, type_: u8, result: u8) -> Self {
+ Self(bindings::gb_operation_msg_hdr {
+ size: size.to_le(),
+ operation_id: operation_id.to_le(),
+ type_,
+ result,
+ pad: [0u8; 2],
+ })
+ }
+
+ /// Returns the type field, response bit included.
+ #[inline]
+ pub const fn msg_type(&self) -> u8 {
+ self.0.type_
+ }
+
+ /// Returns whether this is a response rather than a request.
+ #[inline]
+ pub const fn is_response(&self) -> bool {
+ self.0.type_ & MESSAGE_TYPE_RESPONSE != 0
+ }
+
+ /// Returns the operation id pairing a response with its request. Zero for unidirectional
+ /// messages.
+ #[inline]
+ pub const fn operation_id(&self) -> u16 {
+ u16::from_le(self.0.operation_id)
+ }
+
+ /// Returns the type field with the response bit cleared.
+ #[inline]
+ pub const fn request_type(&self) -> u8 {
+ self.msg_type() & !MESSAGE_TYPE_RESPONSE
+ }
+
+ /// Returns the whole message size, this header included.
+ #[inline]
+ pub const fn size(&self) -> u16 {
+ u16::from_le(self.0.size)
+ }
+}
+
+/// Request for [`GB_SVC_TYPE_PROTOCOL_VERSION`].
+#[repr(transparent)]
+pub struct GbSvcVersionRequest(bindings::gb_svc_version_request);
+
+// SAFETY: `gb_svc_version_request` is a packed struct of integer fields, so it has no padding and
+// no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcVersionRequest {}
+
+impl GbSvcVersionRequest {
+ /// Creates a request advertising SVC protocol version `major`.`minor`.
+ #[inline]
+ pub const fn new(major: u8, minor: u8) -> Self {
+ Self(bindings::gb_svc_version_request { major, minor })
+ }
+}
+
+/// Request for [`GB_SVC_TYPE_SVC_HELLO`].
+#[repr(transparent)]
+pub struct GbSvcHelloRequest(bindings::gb_svc_hello_request);
+
+// SAFETY: `gb_svc_hello_request` is a packed struct of integer fields, so it has no padding and
+// no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcHelloRequest {}
+
+impl GbSvcHelloRequest {
+ /// Creates a hello request identifying the endo as `endo_id` and the AP's own interface as
+ /// `interface_id`.
+ #[inline]
+ pub const fn new(endo_id: u16, interface_id: u8) -> Self {
+ Self(bindings::gb_svc_hello_request {
+ endo_id: endo_id.to_le(),
+ interface_id,
+ })
+ }
+}
+
+/// Request for [`GB_SVC_TYPE_INTF_SET_PWRM`].
+#[repr(transparent)]
+pub struct GbSvcIntfSetPwrmRequest(bindings::gb_svc_intf_set_pwrm_request);
+
+// SAFETY: `gb_svc_intf_set_pwrm_request` is a packed struct of sized fields, so every bit pattern
+// of its size is a valid instance.
+unsafe impl kernel::transmute::FromBytes for GbSvcIntfSetPwrmRequest {}
+
+impl GbSvcIntfSetPwrmRequest {
+ /// Returns the requested TX gear, one of the `GB_SVC_UNIPRO_*` modes.
+ #[inline]
+ pub const fn tx_mode(&self) -> u8 {
+ self.0.tx_mode
+ }
+
+ /// Returns the requested RX gear, one of the `GB_SVC_UNIPRO_*` modes.
+ #[inline]
+ pub const fn rx_mode(&self) -> u8 {
+ self.0.rx_mode
+ }
+}
+
+/// Response to [`GB_SVC_TYPE_INTF_SET_PWRM`].
+#[repr(transparent)]
+pub struct GbSvcIntfSetPwrmResponse(bindings::gb_svc_intf_set_pwrm_response);
+
+// SAFETY: `gb_svc_intf_set_pwrm_response` is a packed struct of integer fields, so it has no
+// padding and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcIntfSetPwrmResponse {}
+
+impl GbSvcIntfSetPwrmResponse {
+ /// `result_code` is one of the `GB_SVC_SETPWRM_PWR_*` codes.
+ #[inline]
+ pub const fn new(result_code: u8) -> Self {
+ Self(bindings::gb_svc_intf_set_pwrm_response { result_code })
+ }
+}
+
+/// Response to [`GB_SVC_TYPE_DME_PEER_GET`].
+#[repr(transparent)]
+pub struct GbSvcDmePeerGetResponse(bindings::gb_svc_dme_peer_get_response);
+
+// SAFETY: `gb_svc_dme_peer_get_response` is a packed struct of integer fields, so it has no
+// padding and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcDmePeerGetResponse {}
+
+impl GbSvcDmePeerGetResponse {
+ /// `result_code` is the UniPro `ConfigResultCode`; `attr_value` is the UniPro attribute
+ /// value.
+ #[inline]
+ pub const fn new(result_code: u16, attr_value: u32) -> Self {
+ Self(bindings::gb_svc_dme_peer_get_response {
+ result_code: result_code.to_le(),
+ attr_value: attr_value.to_le(),
+ })
+ }
+}
+
+/// Response to [`GB_SVC_TYPE_DME_PEER_SET`].
+#[repr(transparent)]
+pub struct GbSvcDmePeerSetResponse(bindings::gb_svc_dme_peer_set_response);
+
+// SAFETY: `gb_svc_dme_peer_set_response` is a packed struct of integer fields, so it has no padding
+// and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcDmePeerSetResponse {}
+
+impl GbSvcDmePeerSetResponse {
+ /// `result_code` is the UniPro `ConfigResultCode`.
+ #[inline]
+ pub const fn new(result_code: u16) -> Self {
+ Self(bindings::gb_svc_dme_peer_set_response {
+ result_code: result_code.to_le(),
+ })
+ }
+}
+
+/// Response to [`GB_SVC_TYPE_PWRMON_RAIL_COUNT_GET`].
+#[repr(transparent)]
+pub struct GbSvcPwrmonRailCountGetResponse(bindings::gb_svc_pwrmon_rail_count_get_response);
+
+// SAFETY: `gb_svc_pwrmon_rail_count_get_response` is a packed struct of integer fields, so it has
+// no padding and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcPwrmonRailCountGetResponse {}
+
+impl GbSvcPwrmonRailCountGetResponse {
+ /// Creates a response reporting `rail_count` available rails.
+ #[inline]
+ pub const fn new(rail_count: u8) -> Self {
+ Self(bindings::gb_svc_pwrmon_rail_count_get_response { rail_count })
+ }
+}
+
+/// Response to [`GB_SVC_TYPE_INTF_VSYS_ENABLE`] and [`GB_SVC_TYPE_INTF_VSYS_DISABLE`].
+#[repr(transparent)]
+pub struct GbSvcIntfVsysResponse(bindings::gb_svc_intf_vsys_response);
+
+// SAFETY: `gb_svc_intf_vsys_response` is a packed struct of integer fields, so it has no padding
+// and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcIntfVsysResponse {}
+
+impl GbSvcIntfVsysResponse {
+ /// `result_code` is [`GB_SVC_INTF_VSYS_OK`] or [`GB_SVC_INTF_VSYS_FAIL`].
+ #[inline]
+ pub const fn new(result_code: u8) -> Self {
+ Self(bindings::gb_svc_intf_vsys_response { result_code })
+ }
+}
+
+/// Response to [`GB_SVC_TYPE_INTF_REFCLK_ENABLE`] and [`GB_SVC_TYPE_INTF_REFCLK_DISABLE`].
+#[repr(transparent)]
+pub struct GbSvcIntfRefclkResponse(bindings::gb_svc_intf_refclk_response);
+
+// SAFETY: `gb_svc_intf_refclk_response` is a packed struct of integer fields, so it has no padding
+// and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcIntfRefclkResponse {}
+
+impl GbSvcIntfRefclkResponse {
+ /// `result_code` is [`GB_SVC_INTF_REFCLK_OK`] or [`GB_SVC_INTF_REFCLK_FAIL`].
+ #[inline]
+ pub const fn new(result_code: u8) -> Self {
+ Self(bindings::gb_svc_intf_refclk_response { result_code })
+ }
+}
+
+/// Response to [`GB_SVC_TYPE_INTF_UNIPRO_ENABLE`] and [`GB_SVC_TYPE_INTF_UNIPRO_DISABLE`].
+#[repr(transparent)]
+pub struct GbSvcIntfUniproResponse(bindings::gb_svc_intf_unipro_response);
+
+// SAFETY: `gb_svc_intf_unipro_response` is a packed struct of integer fields, so it has no padding
+// and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcIntfUniproResponse {}
+
+impl GbSvcIntfUniproResponse {
+ /// `result_code` is one of the `GB_SVC_INTF_UNIPRO_*` codes.
+ #[inline]
+ pub const fn new(result_code: u8) -> Self {
+ Self(bindings::gb_svc_intf_unipro_response { result_code })
+ }
+}
+
+/// Response to [`GB_SVC_TYPE_INTF_ACTIVATE`].
+#[repr(transparent)]
+pub struct GbSvcIntfActivateResponse(bindings::gb_svc_intf_activate_response);
+
+// SAFETY: `gb_svc_intf_activate_response` is a packed struct of integer fields, so it has no
+// padding and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcIntfActivateResponse {}
+
+impl GbSvcIntfActivateResponse {
+ /// `status` is one of the `GB_SVC_OP_*` codes; `intf_type` is one of the
+ /// `GB_SVC_INTF_TYPE_*` values and is only meaningful when `status` is
+ /// [`GB_SVC_OP_SUCCESS`].
+ #[inline]
+ pub const fn new(status: u8, intf_type: u8) -> Self {
+ Self(bindings::gb_svc_intf_activate_response { status, intf_type })
+ }
+}
+
+/// Response to [`GB_SVC_TYPE_INTF_RESUME`].
+#[repr(transparent)]
+pub struct GbSvcIntfResumeResponse(bindings::gb_svc_intf_resume_response);
+
+// SAFETY: `gb_svc_intf_resume_response` is a packed struct of integer fields, so it has no padding
+// and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcIntfResumeResponse {}
+
+impl GbSvcIntfResumeResponse {
+ /// `status` is one of the `GB_SVC_OP_*` codes.
+ #[inline]
+ pub const fn new(status: u8) -> Self {
+ Self(bindings::gb_svc_intf_resume_response { status })
+ }
+}
+
+/// Request for [`GB_SVC_TYPE_MODULE_INSERTED`].
+#[repr(transparent)]
+pub struct GbSvcModuleInsertedRequest(bindings::gb_svc_module_inserted_request);
+
+// SAFETY: `gb_svc_module_inserted_request` is a packed struct of integer fields, so it has no
+// padding and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcModuleInsertedRequest {}
+
+impl GbSvcModuleInsertedRequest {
+ /// The module spans `intf_count` consecutive interfaces starting at `primary_intf_id`.
+ /// `flags` is a mask of `GB_SVC_MODULE_INSERTED_FLAG_*` values.
+ #[inline]
+ pub const fn new(primary_intf_id: u8, intf_count: u8, flags: u16) -> Self {
+ Self(bindings::gb_svc_module_inserted_request {
+ primary_intf_id,
+ intf_count,
+ flags: flags.to_le(),
+ })
+ }
+}
+
+/// Request for [`GB_SVC_TYPE_MODULE_REMOVED`].
+#[repr(transparent)]
+pub struct GbSvcModuleRemovedRequest(bindings::gb_svc_module_removed_request);
+
+// SAFETY: `gb_svc_module_removed_request` is a packed struct of integer fields, so it has no
+// padding and no interior mutability.
+unsafe impl kernel::transmute::AsBytes for GbSvcModuleRemovedRequest {}
+
+// SAFETY: `gb_svc_module_removed_request` is a struct of `u8` field, so every bit pattern of its
+// size is a valid instance.
+unsafe impl kernel::transmute::FromBytes for GbSvcModuleRemovedRequest {}
+
+impl GbSvcModuleRemovedRequest {
+ /// `primary_intf_id` identifies the module, and matches the one given when it was inserted.
+ #[inline]
+ pub const fn new(primary_intf_id: u8) -> Self {
+ Self(bindings::gb_svc_module_removed_request { primary_intf_id })
+ }
+
+ /// Returns the primary_intf_id field.
+ #[inline]
+ pub const fn primary_intf_id(&self) -> u8 {
+ self.0.primary_intf_id
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 637442761d34..931831343fd3 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -84,6 +84,8 @@
pub mod fwctl;
#[cfg(CONFIG_GPU_BUDDY = "y")]
pub mod gpu;
+#[cfg(CONFIG_RUST_GREYBUS_ABSTRACTIONS)]
+pub mod greybus;
#[cfg(CONFIG_I2C = "y")]
pub mod i2c;
pub mod id_pool;
--
2.56.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v4 3/6] rust: kernel: Add greybus abstractions
2026-10-21 11:37 ` [PATCH v4 3/6] rust: kernel: Add greybus abstractions Ayush Singh
@ 2026-10-21 11:55 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-21 11:55 UTC (permalink / raw)
To: Ayush Singh; +Cc: devicetree, robh, conor+dt, ojeda, gary
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 4/6] drivers: greybus: Add software SVC implementation
2026-10-21 11:37 [PATCH v4 0/6] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
` (2 preceding siblings ...)
2026-10-21 11:37 ` [PATCH v4 3/6] rust: kernel: Add greybus abstractions Ayush Singh
@ 2026-10-21 11:37 ` Ayush Singh
2026-10-21 11:57 ` sashiko-bot
2026-10-21 11:37 ` [PATCH v4 5/6] dt-bindings: beagle: Add BeagleConnect Freedom Ayush Singh
2026-10-21 11:37 ` [PATCH v4 6/6] greybus: Add Rust UART node driver Ayush Singh
5 siblings, 1 reply; 10+ messages in thread
From: Ayush Singh @ 2026-10-21 11:37 UTC (permalink / raw)
To: Jason Kridner, robertcnelson, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Eric Biggers, Ard Biesheuvel,
Ayush Singh, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Lorenzo Stoakes, Vlastimil Babka, Liam R. Howlett,
Uladzislau Rezki, Markus Probst
Cc: greybus-dev, linux-kernel, rust-for-linux, linux-crypto,
devicetree, Ayush Singh
A Greybus network requires an SVC (Supervisory Controller) to bring
interfaces up, assign device IDs and connect CPorts to the AP. On a
UniPro network the SVC is a real entity on the bus, but transports that
merely carry Greybus messages - a UART, an I2C bus, a network link -
have no such entity, so every host driver for those transports ends up
open-coding an SVC emulation of its own.
Add gb-softsvc, a Rust implementation of the SVC that lives in the
kernel and registers itself as a Greybus host device. It answers the
SVC-side operations the core expects during interface bring-up and
teardown.
Host drivers use it through a small in-kernel interface. A driver
implements the NodeOps trait to send data towards its node and then
calls module_insert() to announce a new node, submit_message() to hand
incoming Greybus messages back to the core, and module_remove() on
disconnect. Interface IDs are allocated by the SVC and returned from
module_insert().
Connection create/destroy and interface activate/resume currently just
acknowledge the request; callbacks into NodeOps can be added later if
transports need to act on them.
gb_softsvc_exports.c exports the Rust symbols for dependent modules, as
the build system does not yet support Rust cross-module dependencies
natively. This hack is taken from nova-core [0].
[0]: https://lore.kernel.org/all/20260622-nova-exports-v5-0-6191773fc977@nvidia.com/
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
MAINTAINERS | 6 +
drivers/greybus/Kconfig | 12 +
drivers/greybus/Makefile | 40 +++
drivers/greybus/gb_softsvc.rs | 520 +++++++++++++++++++++++++++++++++++
drivers/greybus/gb_softsvc_exports.c | 15 +
5 files changed, 593 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 4d98db7618c3..c9126dc205db 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11516,6 +11516,12 @@ S: Maintained
F: Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
F: drivers/greybus/gb-beagleplay.c
+GREYBUS SOFTSVC DRIVERS
+M: Ayush Singh <ayush@beagleboard.org>
+L: greybus-dev@lists.linaro.org (moderated for non-subscribers)
+S: Maintained
+F: drivers/greybus/gb_softsvc.rs
+
GREYBUS SUBSYSTEM
M: Johan Hovold <johan@kernel.org>
M: Alex Elder <elder@kernel.org>
diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig
index d9f52024d1c2..709563f8c0ad 100644
--- a/drivers/greybus/Kconfig
+++ b/drivers/greybus/Kconfig
@@ -49,5 +49,17 @@ config GREYBUS_ES2
To compile this code as a module, choose M here: the module
will be called gb-es2.ko
+config GREYBUS_SOFTSVC
+ tristate "Greybus software SVC"
+ depends on RUST_GREYBUS_ABSTRACTIONS
+ help
+ Select this option to enable a software implementation of the
+ Greybus SVC (Supervisory Controller) running in the kernel. It can
+ be used by host drivers that talk to a Greybus network which does
+ not provide an SVC of its own.
+
+ To compile this code as a module, choose M here: the module
+ will be called gb-softsvc.ko
+
endif # GREYBUS
diff --git a/drivers/greybus/Makefile b/drivers/greybus/Makefile
index d986e94f8897..0fa802fdf6ca 100644
--- a/drivers/greybus/Makefile
+++ b/drivers/greybus/Makefile
@@ -25,4 +25,44 @@ gb-es2-y := es2.o
obj-$(CONFIG_GREYBUS_ES2) += gb-es2.o
+obj-$(CONFIG_GREYBUS_SOFTSVC) += gb-softsvc.o
+gb-softsvc-y += gb_softsvc.o gb_softsvc_exports.o
+rust_needed_exports = \
+ { $(if $(strip $(2)),$(NM) -u $(2);,) echo "__DEFINED_RUST_SYMBOLS__"; \
+ $(NM) -p --defined-only $(1); } | \
+ awk -v fmt='$(3)' ' \
+ /^__DEFINED_RUST_SYMBOLS__$$/ { defs = 1; next } \
+ !defs { if ($$NF ~ /^_R/) needed[$$NF] = 1; next } \
+ defs && $$2 ~ /(T|R|D|B)/ && $$3 ~ /^_R/ && \
+ $$3 !~ /_(init|cleanup)_module$$/ && \
+ $$3 !~ /__(pfx|cfi|odr_asan)/ && \
+ $$3 in needed { printf fmt, $$3 } \
+ '
+
+quiet_cmd_exports = EXPORTS $@
+ cmd_exports = \
+ $(call rust_needed_exports,$<,$(gb-softsvc-export-deps),EXPORT_SYMBOL_RUST_GPL(%s);\n) > $@
+
+$(obj)/exports_gb_softsvc_generated.h: $(obj)/gb_softsvc.o $(gb-softsvc-export-deps) FORCE
+ $(call if_changed,exports)
+
+targets += exports_gb_softsvc_generated.h
+
+$(obj)/gb_softsvc_exports.o: $(obj)/exports_gb_softsvc_generated.h
+CFLAGS_gb_softsvc_exports.o := -I $(objtree)/$(obj)
+
+ifdef CONFIG_MODVERSIONS
+# The C export shim declares Rust symbols as `extern int`, so reuse its export
+# list but generate symbol CRCs from the Rust object instead of the shim's DWARF.
+$(obj)/gb_softsvc_exports.o: private cmd_gensymtypes_c = \
+ $(call getexportsymbols,\1) | \
+ $(objtree)/scripts/gendwarfksyms/gendwarfksyms \
+ $(if $(KBUILD_GENDWARFKSYMS_STABLE), --stable) \
+ $(if $(KBUILD_SYMTYPES), --symtypes $(@:.o=.symtypes),) \
+ $(obj)/gb_softsvc.o
+endif
+
+# Output gb_softsvc's crate metadata for use by gb_uart_node at compile time.
+RUSTFLAGS_gb_softsvc.o += \
+ --emit=metadata=$(objtree)/$(obj)/libgb_softsvc.rmeta
diff --git a/drivers/greybus/gb_softsvc.rs b/drivers/greybus/gb_softsvc.rs
new file mode 100644
index 000000000000..f88d9b3fd565
--- /dev/null
+++ b/drivers/greybus/gb_softsvc.rs
@@ -0,0 +1,520 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Greybus software SVC.
+//!
+//! A Greybus network needs an SVC to bring interfaces up and to connect their CPorts to the AP.
+//! Transports that carry Greybus without one - a UART or an I2C bus rather than a UniPro network -
+//! have to provide it themselves.
+//!
+//! This module implements that SVC in software.
+
+use kernel::{
+ alloc::Flags,
+ c_str,
+ error::code,
+ faux,
+ greybus::{
+ self,
+ hd,
+ protocols,
+ CPORT_ID_MAX,
+ GB_OPERATION_SIZE_MAX, //
+ },
+ id_pool::IdPool,
+ new_spinlock,
+ prelude::*,
+ sync::{
+ aref::ARef,
+ Arc,
+ SpinLock, //
+ },
+ transmute::AsBytes,
+};
+use pin_init::init_array_from_fn;
+
+const SVC_VERSION_MAJOR: u8 = 0;
+const SVC_VERSION_MINOR: u8 = 1;
+
+const ENDO_ID: u16 = 0;
+const AP_INF_ID: u8 = 0;
+
+const TOSHIBA_DMID: u32 = 0x0126;
+
+const MAX_INTF_ID: u8 = u8::MAX;
+const INTF_ID_START: u8 = 1;
+const INTF_MAP_LEN: usize = (MAX_INTF_ID - INTF_ID_START) as usize;
+
+#[repr(C, packed)]
+struct Msg<P> {
+ hdr: protocols::GbOperationMsgHdr,
+ payload: P,
+}
+
+// SAFETY: `Msg` is `repr(C, packed)`, so it has no padding between its fields, and both fields
+// are `AsBytes`, so neither has uninitialized bytes of its own.
+unsafe impl<P: AsBytes> AsBytes for Msg<P> {}
+
+impl<P> Msg<P> {
+ const GB_SIZE: u16 = size_of::<Self>() as u16;
+
+ /// Builds a request of type `ty`.
+ ///
+ /// An `operation_id` of zero marks a unidirectional request, for which the AP sends no
+ /// response.
+ const fn request(operation_id: u16, ty: u8, payload: P) -> Self {
+ Self {
+ hdr: protocols::GbOperationMsgHdr::new(Self::GB_SIZE, operation_id, ty, 0),
+ payload,
+ }
+ }
+
+ /// Builds the response to `request`, carrying `result`.
+ const fn response(request: &protocols::GbOperationMsgHdr, result: u8, payload: P) -> Self {
+ Self {
+ hdr: protocols::GbOperationMsgHdr::new(
+ Self::GB_SIZE,
+ request.operation_id(),
+ request.msg_type() | protocols::MESSAGE_TYPE_RESPONSE,
+ result,
+ ),
+ payload,
+ }
+ }
+}
+
+const _: () = assert!(size_of::<Msg<()>>() == size_of::<protocols::GbOperationMsgHdr>());
+
+module! {
+ type: GreybusSoftSvc,
+ name: "gb_softsvc",
+ authors: ["Ayush Singh <ayush@beagleboard.org>"],
+ description: "Greybus software SVC implementation",
+ license: "GPL",
+}
+
+kernel::sync::global_lock! {
+ unsafe(uninit) static GLOBAL_STATE: Mutex<Option<GlobalState>> = None;
+}
+
+struct GlobalState {
+ svc: Arc<Svc>,
+ gb_hd: ARef<hd::Device>,
+}
+
+impl GlobalState {
+ fn new(svc: Arc<Svc>, gb_hd: ARef<hd::Device>) -> Self {
+ Self { svc, gb_hd }
+ }
+}
+
+#[pin_data]
+struct Svc {
+ #[pin]
+ svc_operation_id: SpinLock<IdPool>,
+ // Pos 0 = INTF_ID_START
+ #[pin]
+ intf_map: SpinLock<[Option<Arc<dyn InterfaceOps>>; INTF_MAP_LEN]>,
+}
+
+impl Svc {
+ fn new() -> impl PinInit<Self, Error> {
+ pin_init!(Self {
+ intf_map <- new_spinlock!(init_array_from_fn(|_| None)),
+ svc_operation_id <- new_spinlock!(IdPool::with_capacity(u16::MAX.into(), GFP_KERNEL)?)
+ }? Error)
+ }
+
+ fn new_operation_id(&self) -> Result<u16> {
+ let mut ida = self.svc_operation_id.lock();
+ let res = ida.find_unused_id(1).ok_or(code::EOVERFLOW)?.acquire() as u16;
+
+ Ok(res)
+ }
+
+ fn release_operation_id(&self, id: u16) {
+ let mut ida = self.svc_operation_id.lock();
+ ida.release_id(id as usize);
+ }
+
+ fn map_insert(&self, intfs: &[Arc<dyn InterfaceOps>]) -> Result<u8> {
+ fn find_empty_window(
+ intf_map: &[Option<Arc<dyn InterfaceOps>>; INTF_MAP_LEN],
+ count: usize,
+ ) -> Result<u8> {
+ for (id, window) in intf_map.windows(count).enumerate() {
+ if window.iter().all(|x| x.is_none()) {
+ return Ok(id as u8);
+ }
+ }
+
+ Err(code::EOVERFLOW)
+ }
+
+ let mut guard = self.intf_map.lock();
+
+ let pos = find_empty_window(&guard, intfs.len())?;
+ for (i, intf) in intfs.iter().cloned().enumerate() {
+ guard[usize::from(pos) + i] = Some(intf);
+ }
+
+ Ok(pos + INTF_ID_START)
+ }
+
+ fn map_remove(&self, primary_id: u8, intf_count: u8, dev: &kernel::device::Device) {
+ let id = match primary_id.checked_sub(INTF_ID_START) {
+ Some(x) => x,
+ None => {
+ dev_warn!(dev, "Invalid interace id");
+ return;
+ }
+ };
+
+ for i in 0..intf_count {
+ let _ = self.intf_map.lock()[usize::from(id + i)].take();
+ }
+ }
+
+ fn intf_by_id(&self, id: u8) -> Result<Arc<dyn InterfaceOps>> {
+ let id = id - INTF_ID_START;
+ let guard = self.intf_map.lock();
+
+ guard
+ .get(usize::from(id))
+ .ok_or(code::ENODEV)?
+ .as_ref()
+ .ok_or(code::ENODEV)
+ .cloned()
+ }
+
+ fn send_request<P: AsBytes>(&self, hd: &hd::Device, ty: u8, payload: P) -> Result<()> {
+ let msg = Msg::request(self.new_operation_id()?, ty, payload);
+ hd.data_rcvd(protocols::GB_SVC_CPORT_ID, msg.as_bytes());
+ Ok(())
+ }
+
+ fn module_insert(&self, hd: &hd::Device, intfs: &[Arc<dyn InterfaceOps>]) -> Result<u8> {
+ let intf_id = self.map_insert(intfs)?;
+ // map_insert already checks that intf len fits in u8
+ let intf_count = intfs.len() as u8;
+
+ let payload = protocols::GbSvcModuleInsertedRequest::new(intf_id, intf_count, 0);
+ let res = self.send_request(hd, protocols::GB_SVC_TYPE_MODULE_INSERTED, payload);
+
+ if res.is_err() {
+ self.map_remove(intf_id, intf_count, hd.as_ref());
+ }
+
+ res.map(|()| intf_id)
+ }
+
+ fn module_remove(&self, hd: &hd::Device, intf_id: u8, intf_count: u8) -> Result<()> {
+ let payload = protocols::GbSvcModuleRemovedRequest::new(intf_id);
+ let res = self.send_request(hd, protocols::GB_SVC_TYPE_MODULE_REMOVED, payload);
+
+ if res.is_err() {
+ dev_warn!(
+ hd.as_ref(),
+ "Failed to send module_removed svc request for {}",
+ intf_id
+ );
+ }
+
+ self.map_remove(intf_id, intf_count, hd.as_ref());
+
+ Ok(())
+ }
+
+ fn send_version(&self, hd: &hd::Device) -> Result<()> {
+ dev_info!(hd.as_ref(), "Sending SVC version request");
+
+ let payload = protocols::GbSvcVersionRequest::new(SVC_VERSION_MAJOR, SVC_VERSION_MINOR);
+ self.send_request(hd, protocols::GB_SVC_TYPE_PROTOCOL_VERSION, payload)?;
+
+ Ok(())
+ }
+
+ fn send_svc_hello(&self, hd: &hd::Device) -> Result<()> {
+ dev_info!(hd.as_ref(), "Sending SVC Hello request");
+
+ let payload = protocols::GbSvcHelloRequest::new(ENDO_ID, AP_INF_ID);
+ self.send_request(hd, protocols::GB_SVC_TYPE_SVC_HELLO, payload)?;
+
+ Ok(())
+ }
+
+ fn send_response<P: AsBytes>(
+ &self,
+ hd: &hd::Device,
+ msg: &protocols::GbOperationMsgHdr,
+ payload: P,
+ ) {
+ let msg = Msg::response(msg, 0, payload);
+ hd.data_rcvd(protocols::GB_SVC_CPORT_ID, msg.as_bytes());
+ }
+
+ fn intf_set_pwrm(&self, hd: &hd::Device, msg: &greybus::Message) -> Result<()> {
+ let req_msg: &protocols::GbSvcIntfSetPwrmRequest = msg.payload().ok_or(code::EINVAL)?;
+ let result_code = if req_msg.tx_mode() == protocols::GB_SVC_UNIPRO_HIBERNATE_MODE
+ && req_msg.rx_mode() == protocols::GB_SVC_UNIPRO_HIBERNATE_MODE
+ {
+ protocols::GB_SVC_SETPWRM_PWR_OK
+ } else {
+ protocols::GB_SVC_SETPWRM_PWR_LOCAL
+ };
+
+ let payload = protocols::GbSvcIntfSetPwrmResponse::new(result_code);
+ self.send_response(hd, msg.header(), payload);
+
+ Ok(())
+ }
+
+ fn dme_peer_get(&self, hd: &hd::Device, hdr: &protocols::GbOperationMsgHdr) {
+ let payload = protocols::GbSvcDmePeerGetResponse::new(0, TOSHIBA_DMID);
+ self.send_response(hd, hdr, payload);
+ }
+
+ fn dme_peer_set(&self, hd: &hd::Device, hdr: &protocols::GbOperationMsgHdr) {
+ let payload = protocols::GbSvcDmePeerSetResponse::new(0);
+ self.send_response(hd, hdr, payload);
+ }
+
+ fn pwrmon_rail_count_get(&self, hd: &hd::Device, hdr: &protocols::GbOperationMsgHdr) {
+ let payload = protocols::GbSvcPwrmonRailCountGetResponse::new(0);
+ self.send_response(hd, hdr, payload);
+ }
+
+ fn intf_vsys_enable_disable(&self, hd: &hd::Device, hdr: &protocols::GbOperationMsgHdr) {
+ let payload = protocols::GbSvcIntfVsysResponse::new(protocols::GB_SVC_INTF_VSYS_OK);
+ self.send_response(hd, hdr, payload);
+ }
+
+ fn intf_refclk_enable_disable(&self, hd: &hd::Device, hdr: &protocols::GbOperationMsgHdr) {
+ let payload = protocols::GbSvcIntfRefclkResponse::new(protocols::GB_SVC_INTF_VSYS_OK);
+ self.send_response(hd, hdr, payload);
+ }
+
+ fn intf_unipro_enable_disable(&self, hd: &hd::Device, hdr: &protocols::GbOperationMsgHdr) {
+ let payload = protocols::GbSvcIntfUniproResponse::new(protocols::GB_SVC_INTF_UNIPRO_OK);
+ self.send_response(hd, hdr, payload);
+ }
+
+ fn intf_activate(&self, hd: &hd::Device, msg: &greybus::Message) {
+ let payload = protocols::GbSvcIntfActivateResponse::new(
+ protocols::GB_SVC_OP_SUCCESS,
+ protocols::GB_SVC_INTF_TYPE_GREYBUS,
+ );
+
+ // TODO: Maybe call a callback?
+
+ self.send_response(hd, msg.header(), payload);
+ }
+
+ fn intf_resume(&self, hd: &hd::Device, msg: &greybus::Message) {
+ let payload = protocols::GbSvcIntfResumeResponse::new(protocols::GB_SVC_OP_SUCCESS);
+
+ // TODO: Maybe call a callback?
+
+ self.send_response(hd, msg.header(), payload);
+ }
+
+ fn conn_create(&self, hd: &hd::Device, msg: &greybus::Message) -> Result<()> {
+ // TODO: Evaluate if we need to add callback to NodeOps
+ self.send_response(hd, msg.header(), ());
+
+ Ok(())
+ }
+
+ fn conn_destroy(&self, hd: &hd::Device, msg: &greybus::Message) -> Result<()> {
+ // TODO: Evaluate if we need to add callback to NodeOps
+ self.send_response(hd, msg.header(), ());
+
+ Ok(())
+ }
+
+ fn handler(&self, hd: &hd::Device, msg: &greybus::Message) -> Result<()> {
+ let hdr = msg.header();
+
+ if hdr.is_response() {
+ self.release_operation_id(hdr.operation_id());
+ }
+
+ match (hdr.is_response(), hdr.request_type()) {
+ (
+ false,
+ protocols::GB_SVC_TYPE_INTF_DEVICE_ID
+ | protocols::GB_SVC_TYPE_ROUTE_CREATE
+ | protocols::GB_SVC_TYPE_ROUTE_DESTROY
+ | protocols::GB_SVC_TYPE_PING,
+ ) => self.send_response(hd, hdr, ()),
+ (false, protocols::GB_SVC_TYPE_CONN_CREATE) => self.conn_create(hd, msg)?,
+ (false, protocols::GB_SVC_TYPE_CONN_DESTROY) => self.conn_destroy(hd, msg)?,
+ (false, protocols::GB_SVC_TYPE_DME_PEER_GET) => self.dme_peer_get(hd, hdr),
+ (false, protocols::GB_SVC_TYPE_DME_PEER_SET) => self.dme_peer_set(hd, hdr),
+ (false, protocols::GB_SVC_TYPE_INTF_SET_PWRM) => self.intf_set_pwrm(hd, msg)?,
+ (false, protocols::GB_SVC_TYPE_PWRMON_RAIL_COUNT_GET) => {
+ self.pwrmon_rail_count_get(hd, hdr)
+ }
+ (
+ false,
+ protocols::GB_SVC_TYPE_INTF_VSYS_ENABLE | protocols::GB_SVC_TYPE_INTF_VSYS_DISABLE,
+ ) => self.intf_vsys_enable_disable(hd, hdr),
+ (
+ false,
+ protocols::GB_SVC_TYPE_INTF_REFCLK_ENABLE
+ | protocols::GB_SVC_TYPE_INTF_REFCLK_DISABLE,
+ ) => self.intf_refclk_enable_disable(hd, hdr),
+ (
+ false,
+ protocols::GB_SVC_TYPE_INTF_UNIPRO_ENABLE
+ | protocols::GB_SVC_TYPE_INTF_UNIPRO_DISABLE,
+ ) => self.intf_unipro_enable_disable(hd, hdr),
+ (false, protocols::GB_SVC_TYPE_INTF_ACTIVATE) => self.intf_activate(hd, msg),
+ (false, protocols::GB_SVC_TYPE_INTF_RESUME) => self.intf_resume(hd, msg),
+ (true, protocols::GB_SVC_TYPE_PROTOCOL_VERSION) => self.send_svc_hello(hd)?,
+ (
+ true,
+ protocols::GB_SVC_TYPE_MODULE_INSERTED
+ | protocols::GB_SVC_TYPE_SVC_HELLO
+ | protocols::GB_SVC_TYPE_MODULE_REMOVED,
+ ) => {}
+ _ => return Err(code::ENOTSUPP),
+ };
+
+ Ok(())
+ }
+}
+
+struct GbHdDriver(Arc<Svc>);
+
+#[vtable]
+impl hd::HdDriver for GbHdDriver {
+ fn message_send(
+ data: &Self,
+ hd: &hd::Device,
+ dest_cport_id: u16,
+ msg: &greybus::Message,
+ gfp_mask: Flags,
+ ) -> Result {
+ if dest_cport_id == protocols::GB_SVC_CPORT_ID {
+ data.0.handler(hd, msg)
+ } else {
+ let conn = msg.operation().connection();
+ let intf = data.0.intf_by_id(conn.interface().unwrap().id())?;
+
+ let mut buf = KVec::with_capacity(msg.header().size().into(), gfp_mask)?;
+
+ buf.extend_from_slice(msg.header().as_bytes(), gfp_mask)?;
+ buf.extend_from_slice(msg.payload_bytes(), gfp_mask)?;
+
+ intf.write(&buf, conn.intf_cport_id(), gfp_mask)
+ }
+ }
+
+ fn message_cancel(_msg: &greybus::Message) {}
+}
+
+struct GreybusSoftSvc {
+ _hd: hd::Registration<GbHdDriver>,
+ _faux: faux::Registration,
+}
+
+impl kernel::Module for GreybusSoftSvc {
+ fn init(_module: &'static ThisModule) -> Result<Self> {
+ pr_info!("gb_softsvc (init)\n");
+
+ // SAFETY: This runs once at module init, before anything else can reach `GLOBAL_STATE`.
+ unsafe { GLOBAL_STATE.init() };
+
+ let faux = faux::Registration::new(c_str!("gb-softsvc"), None)?;
+ let svc = Arc::pin_init(Svc::new(), GFP_KERNEL)?;
+ let data = GbHdDriver(svc.clone());
+
+ let dev = faux.as_ref().as_ref();
+ let hd = hd::Registration::new(dev, GB_OPERATION_SIZE_MAX, CPORT_ID_MAX + 1, Ok(data))?;
+
+ svc.send_version(hd.as_ref())?;
+
+ let global_state = GlobalState::new(svc.clone(), hd.as_ref().into());
+ let _ = GLOBAL_STATE.lock().replace(global_state);
+
+ Ok(GreybusSoftSvc {
+ _hd: hd,
+ _faux: faux,
+ })
+ }
+}
+
+impl Drop for GreybusSoftSvc {
+ fn drop(&mut self) {
+ let _ = GLOBAL_STATE.lock().take();
+ pr_info!("gb_softsvc (exit)\n");
+ }
+}
+
+/// The operations a Greybus interface provides.
+///
+/// A node is whatever sits behind an interface — a real transport, an in-kernel emulation, or
+/// anything else that can accept Greybus traffic. This module does not care which.
+pub trait InterfaceOps: Send + Sync {
+ /// Delivers `data` to the node's `cport`.
+ fn write(&self, data: &[u8], cport: u16, gfp_mask: Flags) -> Result<()>;
+}
+
+/// A module attached to the Greybus network.
+pub struct Module {
+ id: u8,
+ intf_count: u8,
+ gb_hd: ARef<hd::Device>,
+ svc: Arc<Svc>,
+}
+
+impl Module {
+ /// Attaches `node` as a new module and returns the interface id assigned to it.
+ pub fn new(intfs: &[Arc<dyn InterfaceOps>]) -> Result<Self> {
+ let Ok(intf_count) = u8::try_from(intfs.len()) else {
+ return Err(code::E2BIG);
+ };
+
+ if intf_count == 0 {
+ return Err(code::EINVAL);
+ }
+
+ let guard = GLOBAL_STATE.lock();
+ let state = guard.as_ref().ok_or(code::EAGAIN)?;
+ let id = state.svc.module_insert(&state.gb_hd, intfs)?;
+
+ Ok(Self {
+ id,
+ intf_count,
+ gb_hd: state.gb_hd.clone(),
+ svc: state.svc.clone(),
+ })
+ }
+
+ /// Delivers `msg`, received from interface `id` on its `cport`, to the Greybus core.
+ ///
+ /// `intf_id_rel` is the id relative to the actual interface. Eq, if an Module has 3 interfaces
+ /// then the relative id will be 0 or 1 or 2
+ /// `cport` is the interface-side CPort id.
+ ///
+ /// Fails with `EINVAL` if no connection is bound to that pair, or `EAGAIN` if the
+ /// host device has not been brought up yet.
+ pub fn submit_message(&self, intf_id_rel: u8, cport: u16, msg: &[u8]) -> Result<()> {
+ if intf_id_rel >= self.intf_count {
+ return Err(code::EINVAL);
+ }
+
+ self.gb_hd
+ .data_rcvd_by_intf(self.id + intf_id_rel, cport, msg)
+ }
+}
+
+/// Dropping the module drops all the interfaces of the module. So none of the interface ops should
+/// be called after this point.
+impl Drop for Module {
+ fn drop(&mut self) {
+ let _ = self
+ .svc
+ .module_remove(&self.gb_hd, self.id, self.intf_count);
+ }
+}
diff --git a/drivers/greybus/gb_softsvc_exports.c b/drivers/greybus/gb_softsvc_exports.c
new file mode 100644
index 000000000000..a0e360c4d137
--- /dev/null
+++ b/drivers/greybus/gb_softsvc_exports.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright (c) 2026 BeagleBoard.org. All rights reserved.
+
+/*
+ * Exports Rust symbols from the `gb_softsvc` crate for use by dependent modules.
+ *
+ * This is a workaround until the build system supports Rust cross-module
+ * dependencies natively.
+ */
+
+#include <linux/export.h>
+
+#define EXPORT_SYMBOL_RUST_GPL(sym) extern int sym; EXPORT_SYMBOL_GPL(sym)
+
+#include "exports_gb_softsvc_generated.h"
--
2.56.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v4 4/6] drivers: greybus: Add software SVC implementation
2026-10-21 11:37 ` [PATCH v4 4/6] drivers: greybus: Add software SVC implementation Ayush Singh
@ 2026-10-21 11:57 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-21 11:57 UTC (permalink / raw)
To: Ayush Singh; +Cc: devicetree, robh, conor+dt, ojeda, gary
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] greybus: gb_softsvc: synchronous data_rcvd() in message_send() races timer
- [High] greybus: gb_softsvc: GLOBAL_STATE published before SVC handshake in init()
- [High] greybus: gb_softsvc: premature intf_map removal and race in module_remove()
- [Medium] greybus: gb_softsvc: drop under spinlock and non-atomic loop in map_remove()
- [Medium] greybus: gb_softsvc: Svc::handler() leaks internal ENOTSUPP to userspace
- [Low] greybus: gb_softsvc: stale API docs and wrong constant in refclk helper
--
commit 911de474c435921e03dc8049b26a79b5e9a04e61
Author: Ayush Singh <ayush@beagleboard.org>
drivers: greybus: Add software SVC implementation
This commit adds gb-softsvc, an in-kernel Rust implementation of the Greybus
supervisory controller for transports that do not have a hardware controller
on the bus. It registers a Greybus host device and exposes an interface for
transport drivers to attach modules and exchange Greybus messages.
> diff --git a/drivers/greybus/gb_softsvc.rs b/drivers/greybus/gb_softsvc.rs
> new file mode 100644
> index 0000000000000..f88d9b3fd5655
> --- /dev/null
> +++ b/drivers/greybus/gb_softsvc.rs
> @@ -0,0 +1,520 @@
[ ... ]
> +impl Svc {
[ ... ]
> + fn map_remove(&self, primary_id: u8, intf_count: u8, dev: &kernel::device::Device) {
> + let id = match primary_id.checked_sub(INTF_ID_START) {
> + Some(x) => x,
> + None => {
> + dev_warn!(dev, "Invalid interace id");
> + return;
> + }
> + };
> +
> + for i in 0..intf_count {
> + let _ = self.intf_map.lock()[usize::from(id + i)].take();
> + }
> + }
[Severity: Medium]
In Svc::map_remove(), does the loop drop the removed Arc<dyn InterfaceOps>
while holding the intf_map spinlock, and clear multi-interface ranges
non-atomically?
Because let _ = self.intf_map.lock()[usize::from(id + i)].take(); uses a
wildcard pattern that does not bind the returned Option<Arc<dyn InterfaceOps>>,
Rust drops the temporary Option<Arc<dyn InterfaceOps>> at the end of the
initializer expression before dropping the statement-scoped temporary spinlock
Guard returned by self.intf_map.lock() at the semicolon. When intf_map holds
the last Arc reference, the transport driver's InterfaceOps destructor and
kfree run inside the SpinLock section.
In addition, acquiring and releasing self.intf_map on each iteration of the
0..intf_count loop rather than holding the lock across the entire range allows
a concurrent Svc::map_insert() to observe a partially removed multi-interface
window.
[ ... ]
> + fn module_remove(&self, hd: &hd::Device, intf_id: u8, intf_count: u8) -> Result<()> {
> + let payload = protocols::GbSvcModuleRemovedRequest::new(intf_id);
> + let res = self.send_request(hd, protocols::GB_SVC_TYPE_MODULE_REMOVED, payload);
> +
> + if res.is_err() {
> + dev_warn!(
> + hd.as_ref(),
> + "Failed to send module_removed svc request for {}",
> + intf_id
> + );
> + }
> +
> + self.map_remove(intf_id, intf_count, hd.as_ref());
> +
> + Ok(())
> + }
[Severity: High]
When Module::drop() calls Svc::module_remove(), can clearing self.intf_map
immediately after queuing GB_SVC_TYPE_MODULE_REMOVED race with asynchronous
Greybus core teardown and concurrent GbHdDriver::message_send() calls?
Svc::send_request() calls hd.data_rcvd(), which only queues operation->work
onto svc->connection->wq, and gb_svc_request_handler() then queues
gb_svc_process_module_removed() onto svc->wq. Until
gb_svc_process_module_removed() runs gb_module_del() on svc->wq, the old
module's struct gb_module, struct gb_interface, and struct gb_connection
objects remain active in hd->modules and hd->connections:
1. If Module::new() is called before gb_svc_process_module_removed() finishes
(or after send_request() in Svc::module_remove() fails), Svc::map_insert() can
reallocate the cleared intf_id to the new module while the old module is still
registered. Outbound messages on the old module's connections then look up
intf_by_id(intf_id) in GbHdDriver::message_send() and are misrouted to the new
module's InterfaceOps::write(), inbound messages from the new module via
Module::submit_message() match the old module's connections in
hd->connections, and gb_svc_process_module_inserted() rejects the new module
if the old gb_module is still present in hd->modules.
2. While gb_module_del() tears down the old module's connections on svc->wq,
clearing intf_map beforehand causes outgoing teardown messages to fail with
ENODEV.
3. In GbHdDriver::message_send(), data.0.intf_by_id() clones the
Arc<dyn InterfaceOps> and drops the intf_map spinlock before calling
intf.write(&buf, conn.intf_cport_id(), gfp_mask). Because Module::drop() does
not synchronize with in-flight GbHdDriver::message_send() calls or wait for
connection teardown, InterfaceOps::write() can still execute after
Module::drop() returns, contrary to the doc comment on impl Drop for Module.
[ ... ]
> + fn send_response<P: AsBytes>(
> + &self,
> + hd: &hd::Device,
> + msg: &protocols::GbOperationMsgHdr,
> + payload: P,
> + ) {
> + let msg = Msg::response(msg, 0, payload);
> + hd.data_rcvd(protocols::GB_SVC_CPORT_ID, msg.as_bytes());
> + }
[Severity: Critical]
When GbHdDriver::message_send() handles a request for GB_SVC_CPORT_ID by
synchronously calling Svc::handler() -> Svc::send_response() ->
hd.data_rcvd(), can the response completion race with timer setup in
gb_operation_request_send() and leave an active timer on a freed
struct gb_operation?
Every synchronous request sent by the Greybus core with a non-zero timeout
(such as gb_svc_pwrmon_rail_count_get(), gb_svc_intf_device_id(),
gb_svc_intf_set_power_mode(), gb_svc_intf_vsys_set(),
gb_svc_intf_refclk_set(), gb_svc_intf_unipro_set(), gb_svc_intf_activate(),
gb_svc_dme_peer_get(), gb_svc_dme_peer_set(), gb_svc_route_create(), and
gb_svc_connection_create()) calls gb_operation_sync_timeout() ->
gb_operation_request_send().
In drivers/greybus/operation.c:gb_operation_request_send(), gb_message_send()
is invoked before operation->timer is armed:
drivers/greybus/operation.c:gb_operation_request_send() {
...
ret = gb_message_send(operation->request, gfp);
if (ret)
goto err_put_active;
if (timeout) {
operation->timer.expires = jiffies + msecs_to_jiffies(timeout);
add_timer(&operation->timer);
}
...
}
Because Svc::send_response() calls hd.data_rcvd() synchronously inside
GbHdDriver::message_send() before gb_message_send() returns,
gb_connection_recv_response() immediately queues operation->work
(gb_operation_work()) onto gb_operation_completion_wq (WQ_PERCPU):
drivers/greybus/operation.c:gb_operation_work() {
...
} else {
ret = timer_delete_sync(&operation->timer);
if (!ret) {
/* Cancel request message if scheduled by timeout. */
if (gb_operation_result(operation) == -ETIMEDOUT)
gb_message_cancel(operation->request);
}
operation->callback(operation);
}
gb_operation_put_active(operation);
gb_operation_put(operation);
}
If gb_operation_work() runs on another processor or preempts the sending
thread before gb_operation_request_send() reaches add_timer(&operation->timer),
timer_delete_sync(&operation->timer) runs while the timer is still unarmed,
operation->callback() completes operation->completion, and the active
reference is dropped.
When the sending thread resumes in gb_operation_request_send(), it calls
add_timer(&operation->timer) on the already-completed operation;
wait_for_completion_interruptible() then returns immediately and
gb_operation_sync_timeout() calls gb_operation_put(operation), freeing
struct gb_operation to gb_operation_cache while operation->timer remains
queued in the timer wheel and causing a use-after-free in
gb_operation_timeout() when the timer expires.
[ ... ]
> + fn intf_refclk_enable_disable(&self, hd: &hd::Device, hdr: &protocols::GbOperationMsgHdr) {
> + let payload = protocols::GbSvcIntfRefclkResponse::new(protocols::GB_SVC_INTF_VSYS_OK);
> + self.send_response(hd, hdr, payload);
> + }
[Severity: Low]
This isn't a runtime bug since GB_SVC_INTF_VSYS_OK and GB_SVC_INTF_REFCLK_OK
both evaluate to 0x00, but should Svc::intf_refclk_enable_disable() pass
protocols::GB_SVC_INTF_REFCLK_OK to protocols::GbSvcIntfRefclkResponse::new()?
There are also a few minor comment and documentation mismatches in
drivers/greybus/gb_softsvc.rs and the commit description:
1. The commit description and the comments in Svc::conn_create() and
Svc::conn_destroy() refer to NodeOps, module_insert(), and module_remove(),
whereas the public trait and lifecycle interface are InterfaceOps,
Module::new(), and Drop for Module.
2. The doc comment on Module::new() refers to parameter node (now intfs) and
states that it returns the interface id rather than Result<Self>, while the
doc comment on Module::submit_message() states that it fails with EAGAIN if
the host device has not been brought up yet (which is checked in Module::new()
via GLOBAL_STATE, whereas Module::submit_message() calls
hd::Device::data_rcvd_by_intf() and only returns Ok(()) or EINVAL).
3. Svc::map_remove() logs "Invalid interace id" (missing 'f' in "interface").
[ ... ]
> + (true, protocols::GB_SVC_TYPE_PROTOCOL_VERSION) => self.send_svc_hello(hd)?,
> + (
> + true,
> + protocols::GB_SVC_TYPE_MODULE_INSERTED
> + | protocols::GB_SVC_TYPE_SVC_HELLO
> + | protocols::GB_SVC_TYPE_MODULE_REMOVED,
> + ) => {}
> + _ => return Err(code::ENOTSUPP),
> + };
> +
> + Ok(())
> + }
> +}
[Severity: Medium]
Should the fallback arm in Svc::handler() return a standard user space error
code such as EOPNOTSUPP or EPROTONOSUPPORT instead of internal kernel errno
ENOTSUPP (524)?
When gb_softsvc sends GB_SVC_TYPE_SVC_HELLO and attaches a module via
Module::new() -> Svc::module_insert(), the Greybus core exposes the
intf_eject sysfs attribute on the greybus_svc device, the eject sysfs
attribute on the greybus_module device, and (because Svc::intf_activate()
reports GB_SVC_INTF_TYPE_GREYBUS and interface_power_is_visible() in
drivers/greybus/interface.c checks only intf->type) the voltage_now,
current_now, and power_now sysfs attributes on the interface device.
When user space reads voltage_now, current_now, or power_now or writes to
eject or intf_eject, the Greybus core sends
GB_SVC_TYPE_PWRMON_INTF_SAMPLE_GET or GB_SVC_TYPE_INTF_EJECT synchronously via
gb_operation_sync_timeout() -> gb_operation_request_send_sync_timeout() ->
gb_operation_request_send() -> gb_message_send() ->
HdDriverVTable::<GbHdDriver>::message_send() -> GbHdDriver::message_send() ->
Svc::handler().
Unlike wire responses that pass through gb_operation_status_map(), the
Err(code::ENOTSUPP) returned from GbHdDriver::message_send() is propagated
directly by gb_operation_request_send() to the sysfs callbacks, leaking -524
(-ENOTSUPP) to user space.
> +
> +struct GbHdDriver(Arc<Svc>);
> +
> +#[vtable]
> +impl hd::HdDriver for GbHdDriver {
> + fn message_send(
> + data: &Self,
> + hd: &hd::Device,
> + dest_cport_id: u16,
> + msg: &greybus::Message,
> + gfp_mask: Flags,
> + ) -> Result {
> + if dest_cport_id == protocols::GB_SVC_CPORT_ID {
> + data.0.handler(hd, msg)
> + } else {
> + let conn = msg.operation().connection();
> + let intf = data.0.intf_by_id(conn.interface().unwrap().id())?;
> +
> + let mut buf = KVec::with_capacity(msg.header().size().into(), gfp_mask)?;
> +
> + buf.extend_from_slice(msg.header().as_bytes(), gfp_mask)?;
> + buf.extend_from_slice(msg.payload_bytes(), gfp_mask)?;
> +
> + intf.write(&buf, conn.intf_cport_id(), gfp_mask)
> + }
> + }
> +
> + fn message_cancel(_msg: &greybus::Message) {}
> +}
> +
> +struct GreybusSoftSvc {
> + _hd: hd::Registration<GbHdDriver>,
> + _faux: faux::Registration,
> +}
> +
> +impl kernel::Module for GreybusSoftSvc {
> + fn init(_module: &'static ThisModule) -> Result<Self> {
> + pr_info!("gb_softsvc (init)\n");
> +
> + // SAFETY: This runs once at module init, before anything else can reach `GLOBAL_STATE`.
> + unsafe { GLOBAL_STATE.init() };
> +
> + let faux = faux::Registration::new(c_str!("gb-softsvc"), None)?;
> + let svc = Arc::pin_init(Svc::new(), GFP_KERNEL)?;
> + let data = GbHdDriver(svc.clone());
> +
> + let dev = faux.as_ref().as_ref();
> + let hd = hd::Registration::new(dev, GB_OPERATION_SIZE_MAX, CPORT_ID_MAX + 1, Ok(data))?;
> +
> + svc.send_version(hd.as_ref())?;
> +
> + let global_state = GlobalState::new(svc.clone(), hd.as_ref().into());
> + let _ = GLOBAL_STATE.lock().replace(global_state);
[Severity: High]
In GreybusSoftSvc::init(), is GLOBAL_STATE published too early, before the
asynchronous GB_SVC_TYPE_PROTOCOL_VERSION and GB_SVC_TYPE_SVC_HELLO handshake
completes?
svc.send_version(hd.as_ref()) calls hd.data_rcvd(GB_SVC_CPORT_ID, ...), which
invokes gb_connection_recv_request() and only queues operation->work onto the
ordered workqueue svc->connection->wq without processing it synchronously.
In drivers/greybus/svc.c:gb_svc_request_handler(), the state machine remains
in GB_SVC_STATE_RESET until the work item for GB_SVC_TYPE_PROTOCOL_VERSION
runs, sends its response back to Svc::handler() to queue
Svc::send_svc_hello(), and the subsequent work item for GB_SVC_TYPE_SVC_HELLO
transitions svc->state to GB_SVC_STATE_SVC_HELLO:
drivers/greybus/svc.c:gb_svc_request_handler() {
...
switch (type) {
case GB_SVC_TYPE_PROTOCOL_VERSION:
if (svc->state != GB_SVC_STATE_RESET)
ret = -EINVAL;
break;
case GB_SVC_TYPE_SVC_HELLO:
if (svc->state != GB_SVC_STATE_PROTOCOL_VERSION)
ret = -EINVAL;
break;
default:
if (svc->state != GB_SVC_STATE_SVC_HELLO)
ret = -EINVAL;
break;
}
...
}
If a consumer driver calls Module::new() after GLOBAL_STATE is populated in
GreybusSoftSvc::init() but before the GB_SVC_TYPE_PROTOCOL_VERSION response
triggers Svc::send_svc_hello(), Module::new() sees GLOBAL_STATE is Some
(instead of returning EAGAIN) and calls state.svc.module_insert(), queuing
GB_SVC_TYPE_MODULE_INSERTED onto svc->connection->wq ahead of
GB_SVC_TYPE_SVC_HELLO.
When gb_svc_request_handler() processes GB_SVC_TYPE_MODULE_INSERTED while
svc->state != GB_SVC_STATE_SVC_HELLO, it rejects the request with -EINVAL.
Because Svc::handler() ignores the response status for
GB_SVC_TYPE_MODULE_INSERTED, Module::new() returns Ok(Module) while no
gb_module or gb_interface is created in the Greybus core, and the allocated
interface ids and Arc<dyn InterfaceOps> entries remain occupied in
Svc::intf_map until Module is dropped.
[ ... ]
> +impl Module {
> + /// Attaches `node` as a new module and returns the interface id assigned to it.
> + pub fn new(intfs: &[Arc<dyn InterfaceOps>]) -> Result<Self> {
> + let Ok(intf_count) = u8::try_from(intfs.len()) else {
> + return Err(code::E2BIG);
> + };
> +
> + if intf_count == 0 {
> + return Err(code::EINVAL);
> + }
> +
> + let guard = GLOBAL_STATE.lock();
> + let state = guard.as_ref().ok_or(code::EAGAIN)?;
> + let id = state.svc.module_insert(&state.gb_hd, intfs)?;
> +
> + Ok(Self {
> + id,
> + intf_count,
> + gb_hd: state.gb_hd.clone(),
> + svc: state.svc.clone(),
> + })
> + }
[ ... ]
> +/// Dropping the module drops all the interfaces of the module. So none of the interface ops should
> +/// be called after this point.
> +impl Drop for Module {
> + fn drop(&mut self) {
> + let _ = self
> + .svc
> + .module_remove(&self.gb_hd, self.id, self.intf_count);
> + }
> +}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-gb-uart-transport-v4-0-c308ce821d45@beagleboard.org?part=4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 5/6] dt-bindings: beagle: Add BeagleConnect Freedom
2026-10-21 11:37 [PATCH v4 0/6] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
` (3 preceding siblings ...)
2026-10-21 11:37 ` [PATCH v4 4/6] drivers: greybus: Add software SVC implementation Ayush Singh
@ 2026-10-21 11:37 ` Ayush Singh
2026-10-21 11:37 ` [PATCH v4 6/6] greybus: Add Rust UART node driver Ayush Singh
5 siblings, 0 replies; 10+ messages in thread
From: Ayush Singh @ 2026-10-21 11:37 UTC (permalink / raw)
To: Jason Kridner, robertcnelson, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Eric Biggers, Ard Biesheuvel,
Ayush Singh, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Lorenzo Stoakes, Vlastimil Babka, Liam R. Howlett,
Uladzislau Rezki, Markus Probst
Cc: greybus-dev, linux-kernel, rust-for-linux, linux-crypto,
devicetree, Ayush Singh, Conor Dooley
Add devicetree bindings for the BeagleBoard.org BeagleConnect Freedom,
a wireless-enabled board based on the TI CC1352P7. The board exposes an
MCU that can be attached to a host over USB, UART, SPI or I2C, and is
described as a child node of the corresponding bus.
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
.../beagle/beagle,beagleconnect-freedom.yaml | 62 ++++++++++++++++++++++
MAINTAINERS | 6 +++
2 files changed, 68 insertions(+)
diff --git a/Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml b/Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml
new file mode 100644
index 000000000000..81dfdace33e2
--- /dev/null
+++ b/Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/beagle/beagle,beagleconnect-freedom.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: BeagleBoard.org BeagleConnect Freedom
+
+description:
+ The BeagleBoard.org BeagleConnect Freedom board connected over USB, UART, SPI or I2C.
+
+maintainers:
+ - Ayush Singh <ayush@beagleboard.org>
+
+properties:
+ compatible:
+ const: beagle,beagleconnect-freedom
+
+ reg:
+ maxItems: 1
+
+ vbat-supply: true
+
+required:
+ - compatible
+
+allOf:
+ - if:
+ required:
+ - reg
+ then:
+ $ref: /schemas/spi/spi-peripheral-props.yaml#
+ else:
+ $ref: /schemas/serial/serial-peripheral-props.yaml#
+
+ properties:
+ current-speed:
+ default: 115200
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ serial {
+ mcu {
+ compatible = "beagle,beagleconnect-freedom";
+ vbat-supply = <&vbat>;
+ };
+ };
+
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ mcu@0 {
+ compatible = "beagle,beagleconnect-freedom";
+ reg = <0>;
+ spi-max-frequency = <1000000>;
+ vbat-supply = <&vbat>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index c9126dc205db..521bc6c67979 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11522,6 +11522,12 @@ L: greybus-dev@lists.linaro.org (moderated for non-subscribers)
S: Maintained
F: drivers/greybus/gb_softsvc.rs
+GREYBUS UART NODE DRIVERS
+M: Ayush Singh <ayush@beagleboard.org>
+L: greybus-dev@lists.linaro.org (moderated for non-subscribers)
+S: Maintained
+F: Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml
+
GREYBUS SUBSYSTEM
M: Johan Hovold <johan@kernel.org>
M: Alex Elder <elder@kernel.org>
--
2.56.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v4 6/6] greybus: Add Rust UART node driver
2026-10-21 11:37 [PATCH v4 0/6] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
` (4 preceding siblings ...)
2026-10-21 11:37 ` [PATCH v4 5/6] dt-bindings: beagle: Add BeagleConnect Freedom Ayush Singh
@ 2026-10-21 11:37 ` Ayush Singh
2026-10-21 11:57 ` sashiko-bot
5 siblings, 1 reply; 10+ messages in thread
From: Ayush Singh @ 2026-10-21 11:37 UTC (permalink / raw)
To: Jason Kridner, robertcnelson, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Eric Biggers, Ard Biesheuvel,
Ayush Singh, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Lorenzo Stoakes, Vlastimil Babka, Liam R. Howlett,
Uladzislau Rezki, Markus Probst
Cc: greybus-dev, linux-kernel, rust-for-linux, linux-crypto,
devicetree, Ayush Singh
Add a driver for Greybus nodes attached over a plain serial port. The
node is registered with the software SVC (gb-softsvc), which handles the
SVC protocol on behalf of the AP, so no dedicated coprocessor running
SVC firmware is needed.
Greybus messages are carried over HDLC framing on the wire. Each frame
carries a one-byte address (0x01 for Greybus) and control byte, followed
by the 16-bit CPort ID and the Greybus message itself.
Port parameters are taken from the firmware node: "baudrate" if
present, otherwise 115200, with flow control and parity disabled.
Since gb-uart-node imports types from gb-softsvc, Rust to Rust calling
setup from nova-core [0] is being used.
[0]: https://lore.kernel.org/all/20260622-nova-exports-v5-0-6191773fc977@nvidia.com/
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
MAINTAINERS | 1 +
drivers/greybus/.gitignore | 1 +
drivers/greybus/Kconfig | 15 +++
drivers/greybus/Makefile | 10 ++
drivers/greybus/gb_uart_node.rs | 257 ++++++++++++++++++++++++++++++++++++++++
5 files changed, 284 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 521bc6c67979..5bdd500ef63e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11527,6 +11527,7 @@ M: Ayush Singh <ayush@beagleboard.org>
L: greybus-dev@lists.linaro.org (moderated for non-subscribers)
S: Maintained
F: Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml
+F: drivers/greybus/gb_uart_node.rs
GREYBUS SUBSYSTEM
M: Johan Hovold <johan@kernel.org>
diff --git a/drivers/greybus/.gitignore b/drivers/greybus/.gitignore
new file mode 100644
index 000000000000..ff9c4a3539b4
--- /dev/null
+++ b/drivers/greybus/.gitignore
@@ -0,0 +1 @@
+exports_gb_softsvc_generated.h
diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig
index 709563f8c0ad..ce2030161f30 100644
--- a/drivers/greybus/Kconfig
+++ b/drivers/greybus/Kconfig
@@ -61,5 +61,20 @@ config GREYBUS_SOFTSVC
To compile this code as a module, choose M here: the module
will be called gb-softsvc.ko
+config GREYBUS_UART_NODE
+ tristate "Greybus UART node transport"
+ depends on GREYBUS_SOFTSVC
+ depends on RUST_SERIAL_DEV_BUS_ABSTRACTIONS
+ depends on RUST_GREYBUS_ABSTRACTIONS
+ select RUST_CRC_CCITT_ABSTRACTIONS
+ help
+ Select this option if you have a Greybus node connected over a
+ serial port. The node is registered with the software SVC, which
+ handles the SVC protocol on behalf of the AP, so no dedicated
+ coprocessor running SVC firmware is required.
+
+ To compile this code as a module, choose M here: the module
+ will be called gb-uart-node.ko
+
endif # GREYBUS
diff --git a/drivers/greybus/Makefile b/drivers/greybus/Makefile
index 0fa802fdf6ca..bf4d08f728fe 100644
--- a/drivers/greybus/Makefile
+++ b/drivers/greybus/Makefile
@@ -28,6 +28,12 @@ obj-$(CONFIG_GREYBUS_ES2) += gb-es2.o
obj-$(CONFIG_GREYBUS_SOFTSVC) += gb-softsvc.o
gb-softsvc-y += gb_softsvc.o gb_softsvc_exports.o
+obj-$(CONFIG_GREYBUS_UART_NODE) += gb-uart-node.o
+gb-uart-node-y += gb_uart_node.o
+
+# Export Rust symbols from gb-softsvc only if gb-uart-node actually references them.
+gb-softsvc-export-deps := $(if $(CONFIG_GREYBUS_UART_NODE),$(obj)/gb_uart_node.o)
+
rust_needed_exports = \
{ $(if $(strip $(2)),$(NM) -u $(2);,) echo "__DEFINED_RUST_SYMBOLS__"; \
$(NM) -p --defined-only $(1); } | \
@@ -66,3 +72,7 @@ endif
# Output gb_softsvc's crate metadata for use by gb_uart_node at compile time.
RUSTFLAGS_gb_softsvc.o += \
--emit=metadata=$(objtree)/$(obj)/libgb_softsvc.rmeta
+
+# Allow nova-drm to import nova-core's types.
+$(obj)/gb_uart_node.o: $(obj)/gb_softsvc.o
+RUSTFLAGS_gb_uart_node.o := -L $(objtree)/$(obj) --extern gb_softsvc
diff --git a/drivers/greybus/gb_uart_node.rs b/drivers/greybus/gb_uart_node.rs
new file mode 100644
index 000000000000..f3779737c75e
--- /dev/null
+++ b/drivers/greybus/gb_uart_node.rs
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Greybus UART Node driver
+
+use kernel::{
+ alloc::Flags,
+ crc_ccitt::crc_ccitt,
+ device::{
+ AsBusDevice,
+ Bound,
+ Core, //
+ },
+ error::code,
+ new_spinlock,
+ of, //
+ prelude::*,
+ serdev,
+ sync::{
+ aref::ARef,
+ Arc,
+ SpinLock, //
+ },
+};
+
+use zerocopy::little_endian;
+use zerocopy_derive::{FromBytes, Immutable, KnownLayout};
+
+/// Most greybus implementations will have a limit on greybus message length. The theoretical limit
+/// is [`u16::MAX`], but the real limits are much more modest. 256 seems to be the limit I use on
+/// most of my peers, but let's use 512 to be on safe side in kernel. The only real big request that
+/// I frequently encounter is manifest_get. However, if camera protocol ends up being supported at
+/// some point, this might need to be increased.
+const HDLC_MAX_FRAME_LEN: usize = 512;
+const HDLC_FRAME_HEADER_LEN: usize =
+ 2 * size_of_val(&HDLC_FRAME) + 2 * size_of::<u16>() + 2 * size_of::<u8>();
+const HDLC_MAX_MSG_LEN: usize = HDLC_MAX_FRAME_LEN - HDLC_FRAME_HEADER_LEN;
+
+const HDLC_FRAME: u8 = 0x7E;
+const HDLC_ESC: u8 = 0x7D;
+const HDLC_XOR: u8 = 0x20;
+const HDLC_EXPECTED_CRC: u16 = 0xf0b8;
+
+const ADDRESS_GREYBUS: u8 = 0x01;
+
+#[repr(C, packed)]
+#[derive(FromBytes, Immutable, KnownLayout)]
+struct GreybusFrame {
+ cport: little_endian::U16,
+ msg: [u8],
+}
+
+struct HdlcRx {
+ rx_buf: KVec<u8>,
+ rx_in_esc: bool,
+ rx_overflow: bool,
+ sdev: ARef<serdev::Device>,
+ node: gb_softsvc::Module,
+}
+
+impl HdlcRx {
+ fn new(sdev: ARef<serdev::Device>, node: gb_softsvc::Module) -> Result<Self> {
+ Ok(Self {
+ node,
+ sdev,
+ rx_buf: KVec::with_capacity(HDLC_MAX_FRAME_LEN, GFP_KERNEL)?,
+ rx_in_esc: false,
+ rx_overflow: false,
+ })
+ }
+
+ fn frame_finish(&self) -> Result<()> {
+ if self.rx_buf.len() < 4 {
+ return Err(code::EFAULT);
+ }
+
+ let crc = crc_ccitt(0xffff, &self.rx_buf);
+ if crc != HDLC_EXPECTED_CRC {
+ dev_warn!(self.sdev.as_ref(), "CRC failed {}", crc);
+ return Ok(());
+ }
+
+ let addr = self.rx_buf[0];
+ let _ctrl = self.rx_buf[1];
+ let payload = &self.rx_buf[2..self.rx_buf.len() - size_of::<u16>()];
+
+ match addr {
+ ADDRESS_GREYBUS => {
+ let frame = GreybusFrame::ref_from_bytes(payload).map_err(|_| code::EINVAL)?;
+ self.node.submit_message(0, frame.cport.into(), &frame.msg)
+ }
+ _ => Err(code::EINVAL),
+ }
+ }
+
+ fn rx(&mut self, data: &[u8]) -> usize {
+ for i in data.iter() {
+ match *i {
+ HDLC_FRAME => {
+ if !self.rx_buf.is_empty() {
+ if let Err(e) = self.frame_finish() {
+ dev_warn!(self.sdev.as_ref(), "bad frame: {e:?}\n");
+ }
+ }
+
+ self.rx_buf.clear();
+ self.rx_in_esc = false;
+ self.rx_overflow = false;
+ }
+ HDLC_ESC => self.rx_in_esc = true,
+ // Skip data if buffer has overflown.
+ _ if self.rx_overflow => {}
+ _ => {
+ let c = if self.rx_in_esc { *i ^ HDLC_XOR } else { *i };
+ self.rx_in_esc = false;
+
+ if self.rx_buf.push_within_capacity(c).is_err() {
+ dev_warn!(self.sdev.as_ref(), "buffer overflow. Dropping frame");
+
+ self.rx_overflow = true;
+ self.rx_buf.clear();
+ }
+ }
+ }
+ }
+
+ data.len()
+ }
+}
+
+struct GbNode {
+ sdev: ARef<serdev::Device>,
+}
+
+impl GbNode {
+ const fn new(sdev: ARef<serdev::Device>) -> Self {
+ Self { sdev }
+ }
+
+ fn fill_buf(mut crc: u16, data: &[u8], buf: &mut KVec<u8>) -> Result<u16> {
+ for i in data {
+ crc = crc_ccitt(crc, &[*i]);
+ if *i == HDLC_ESC || *i == HDLC_FRAME {
+ buf.push_within_capacity(HDLC_ESC)?;
+ buf.push_within_capacity(i ^ HDLC_XOR)?;
+ } else {
+ buf.push_within_capacity(*i)?;
+ }
+ }
+
+ Ok(crc)
+ }
+}
+
+impl gb_softsvc::InterfaceOps for GbNode {
+ fn write(&self, data: &[u8], cport: u16, gfp_mask: Flags) -> Result<()> {
+ // SAFETY: `GbNode` only exists while its serdev driver is bound, so the device is in the
+ // `Bound` state for the duration of this call.
+ let bound: &serdev::Device<Bound> =
+ unsafe { serdev::Device::from_device(self.sdev.as_ref().as_bound()) };
+
+ // Drop messages which are bigger than what is supported.
+ if data.len() > HDLC_MAX_MSG_LEN {
+ return Err(code::E2BIG);
+ }
+
+ let mut buf = KVec::with_capacity(HDLC_MAX_FRAME_LEN, gfp_mask)?;
+
+ let mut crc = 0xffff;
+
+ buf.push_within_capacity(HDLC_FRAME)?;
+
+ crc = Self::fill_buf(crc, &[ADDRESS_GREYBUS, 0x03], &mut buf)?;
+ crc = Self::fill_buf(crc, &cport.to_le_bytes(), &mut buf)?;
+ crc = Self::fill_buf(crc, data, &mut buf)?;
+
+ crc ^= 0xffff;
+ Self::fill_buf(crc, &crc.to_le_bytes(), &mut buf)?;
+
+ buf.push_within_capacity(HDLC_FRAME)?;
+
+ bound.write_all(&buf, 0)?;
+
+ Ok(())
+ }
+}
+
+#[pin_data]
+struct GbUartNode {
+ #[pin]
+ rx: SpinLock<HdlcRx>,
+}
+
+impl GbUartNode {
+ fn init(sdev: &serdev::Device<Core<'_>>) -> Result<HdlcRx> {
+ if sdev
+ .set_baudrate(
+ sdev.as_ref()
+ .fwnode()
+ .and_then(|fwnode| fwnode.property_read(c"current-speed").optional())
+ .unwrap_or(115200),
+ )
+ .is_err()
+ {
+ return Err(EINVAL);
+ }
+ sdev.set_flow_control(false);
+ sdev.set_parity(serdev::Parity::None)?;
+
+ let node = gb_softsvc::Module::new(&[Arc::new(GbNode::new(sdev.into()), GFP_KERNEL)?])?;
+
+ HdlcRx::new(sdev.into(), node)
+ }
+}
+
+kernel::of_device_table!(
+ OF_TABLE,
+ <GbUartNode as serdev::Driver>::IdInfo,
+ [(of::DeviceId::new(c"beagle,beagleconnect-freedom"), ())]
+);
+
+#[vtable]
+impl serdev::Driver for GbUartNode {
+ type IdInfo = ();
+ type Data<'bound> = Self;
+ const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+ fn probe<'bound>(
+ sdev: &'bound serdev::Device<Core<'_>>,
+ _info: Option<&'bound Self::IdInfo>,
+ ) -> impl PinInit<Self, Error> + 'bound {
+ dev_dbg!(sdev.as_ref(), "Probe gb_uart_node.\n");
+
+ try_pin_init!(Self {
+ rx <- new_spinlock!(Self::init(sdev)?, "gb_uart_node::rx"),
+ }? Error)
+ }
+
+ fn receive<'bound>(
+ _sdev: &'bound serdev::Device<Bound>,
+ this: Pin<&Self>,
+ data: &[u8],
+ ) -> usize {
+ if let Some(mut guard) = this.rx.try_lock() {
+ return guard.rx(data);
+ }
+
+ 0
+ }
+}
+
+kernel::module_serdev_device_driver! {
+ type: GbUartNode,
+ name: "gb_uart_node",
+ authors: ["Ayush Singh <ayush@beagleboard.org>"],
+ description: "Greybus node connected over UART",
+ license: "GPL v2",
+}
--
2.56.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v4 6/6] greybus: Add Rust UART node driver
2026-10-21 11:37 ` [PATCH v4 6/6] greybus: Add Rust UART node driver Ayush Singh
@ 2026-10-21 11:57 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-21 11:57 UTC (permalink / raw)
To: Ayush Singh; +Cc: devicetree, robh, conor+dt, ojeda, gary
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] greybus: gb_uart_node: unsound as_bound() cast in GbNode::write() races unbind
- [High] greybus: gb_uart_node: unescaped TX buffer size in GbNode::write()
- [Medium] greybus: gb_uart_node: check for partial writes in GbNode::write()
- [Low] greybus: gb_uart_node: import style and stale comments in Makefile and commit
--
commit 7c4e3ad2ab622fa6f666582d5e991d3653169d06
Author: Ayush Singh <ayush@beagleboard.org>
greybus: Add Rust UART node driver
This commit adds a Rust serdev driver for Greybus nodes attached over a
serial port using Hdlc framing, registering the interface with the software
Greybus service (gb-softsvc).
> diff --git a/drivers/greybus/Makefile b/drivers/greybus/Makefile
> index 0fa802fdf6caf..bf4d08f728fe2 100644
> --- a/drivers/greybus/Makefile
> +++ b/drivers/greybus/Makefile
[ ... ]
> @@ -66,3 +72,7 @@ endif
> # Output gb_softsvc's crate metadata for use by gb_uart_node at compile time.
> RUSTFLAGS_gb_softsvc.o += \
> --emit=metadata=$(objtree)/$(obj)/libgb_softsvc.rmeta
> +
> +# Allow nova-drm to import nova-core's types.
> +$(obj)/gb_uart_node.o: $(obj)/gb_softsvc.o
> +RUSTFLAGS_gb_uart_node.o := -L $(objtree)/$(obj) --extern gb_softsvc
[Severity: Low]
This isn't a bug, but should this comment in drivers/greybus/Makefile refer
to gb-uart-node and gb-softsvc instead of nova-drm and nova-core?
> diff --git a/drivers/greybus/gb_uart_node.rs b/drivers/greybus/gb_uart_node.rs
> new file mode 100644
> index 0000000000000..f3779737c75eb
> --- /dev/null
> +++ b/drivers/greybus/gb_uart_node.rs
> @@ -0,0 +1,257 @@
[ ... ]
> +use zerocopy::little_endian;
> +use zerocopy_derive::{FromBytes, Immutable, KnownLayout};
[Severity: Low]
Should this zerocopy_derive import in drivers/greybus/gb_uart_node.rs use the
vertical import style from Documentation/rust/coding-guidelines.rst?
> +
> +/// Most greybus implementations will have a limit on greybus message length. The theoretical limit
> +/// is [`u16::MAX`], but the real limits are much more modest. 256 seems to be the limit I use on
> +/// most of my peers, but let's use 512 to be on safe side in kernel. The only real big request that
> +/// I frequently encounter is manifest_get. However, if camera protocol ends up being supported at
> +/// some point, this might need to be increased.
> +const HDLC_MAX_FRAME_LEN: usize = 512;
> +const HDLC_FRAME_HEADER_LEN: usize =
> + 2 * size_of_val(&HDLC_FRAME) + 2 * size_of::<u16>() + 2 * size_of::<u8>();
> +const HDLC_MAX_MSG_LEN: usize = HDLC_MAX_FRAME_LEN - HDLC_FRAME_HEADER_LEN;
> +
> +const HDLC_FRAME: u8 = 0x7E;
> +const HDLC_ESC: u8 = 0x7D;
> +const HDLC_XOR: u8 = 0x20;
[ ... ]
> +struct GbNode {
> + sdev: ARef<serdev::Device>,
> +}
> +
> +impl GbNode {
> + const fn new(sdev: ARef<serdev::Device>) -> Self {
> + Self { sdev }
> + }
> +
> + fn fill_buf(mut crc: u16, data: &[u8], buf: &mut KVec<u8>) -> Result<u16> {
> + for i in data {
> + crc = crc_ccitt(crc, &[*i]);
> + if *i == HDLC_ESC || *i == HDLC_FRAME {
> + buf.push_within_capacity(HDLC_ESC)?;
> + buf.push_within_capacity(i ^ HDLC_XOR)?;
> + } else {
> + buf.push_within_capacity(*i)?;
> + }
> + }
> +
> + Ok(crc)
> + }
> +}
> +
> +impl gb_softsvc::InterfaceOps for GbNode {
> + fn write(&self, data: &[u8], cport: u16, gfp_mask: Flags) -> Result<()> {
> + // SAFETY: `GbNode` only exists while its serdev driver is bound, so the device is in the
> + // `Bound` state for the duration of this call.
> + let bound: &serdev::Device<Bound> =
> + unsafe { serdev::Device::from_device(self.sdev.as_ref().as_bound()) };
[Severity: High]
Can GbNode::write() in drivers/greybus/gb_uart_node.rs outlive the bound
state of the serdev device and race with driver unbind?
In GbUartNode::init(), GbNode is wrapped in an Arc and registered with
gb_softsvc::Module::new(). When GbHdDriver::message_send() in
drivers/greybus/gb_softsvc.rs sends a message, Svc::intf_by_id() clones the
Arc<dyn InterfaceOps> and drops intf_map's spinlock before calling
intf.write():
drivers/greybus/gb_softsvc.rs:Svc::intf_by_id() {
let id = id - INTF_ID_START;
let guard = self.intf_map.lock();
guard
.get(usize::from(id))
.ok_or(code::ENODEV)?
.as_ref()
.ok_or(code::ENODEV)
.cloned()
}
If the serdev device is unbound concurrently (or if HdlcRx::new() fails in
GbUartNode::init() after gb_softsvc::Module::new() succeeds),
serdev::PrivateData::drop() in rust/kernel/serdev.rs drops GbUartNode ->
HdlcRx -> gb_softsvc::Module. Module::drop() -> Svc::module_remove() ->
Svc::map_remove() removes the entry from intf_map without waiting for cloned
Arc<GbNode> references to finish, and PrivateData::drop() immediately closes
the serdev device:
rust/kernel/serdev.rs:PrivateData::drop() {
unsafe { (*self.driver.get()).assume_init_drop() };
*active = false;
}
drop(active);
// SAFETY: We have exclusive access to `self.open`.
if unsafe { *self.open.get() } {
// SAFETY: `self.sdev.as_raw()` is guaranteed to be a pointer to a valid
// `struct serdev_device`.
unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
}
}
Can an in-flight GbNode::write() then call bound.write_all(&buf, 0) (which
calls serdev_device_write() with an infinite timeout) concurrently with or
after serdev_device_close(), violating the Device::as_bound() safety
requirement and racing against ttyport_close() tearing down serport->tty?
> +
> + // Drop messages which are bigger than what is supported.
> + if data.len() > HDLC_MAX_MSG_LEN {
> + return Err(code::E2BIG);
> + }
> +
> + let mut buf = KVec::with_capacity(HDLC_MAX_FRAME_LEN, gfp_mask)?;
> +
> + let mut crc = 0xffff;
> +
> + buf.push_within_capacity(HDLC_FRAME)?;
> +
> + crc = Self::fill_buf(crc, &[ADDRESS_GREYBUS, 0x03], &mut buf)?;
> + crc = Self::fill_buf(crc, &cport.to_le_bytes(), &mut buf)?;
> + crc = Self::fill_buf(crc, data, &mut buf)?;
> +
> + crc ^= 0xffff;
> + Self::fill_buf(crc, &crc.to_le_bytes(), &mut buf)?;
> +
> + buf.push_within_capacity(HDLC_FRAME)?;
[Severity: High]
Does GbNode::write() in drivers/greybus/gb_uart_node.rs fail when byte
stuffing expands a valid frame beyond 512 bytes?
HDLC_MAX_MSG_LEN is computed from HDLC_MAX_FRAME_LEN (512) minus the 8-byte
unescaped header/trailer overhead (504 bytes), and buf is allocated with
KVec::with_capacity(HDLC_MAX_FRAME_LEN, gfp_mask).
However, GbNode::fill_buf() escapes every 0x7d or 0x7e byte in cport, data,
and crc into two bytes using buf.push_within_capacity(), which does not
reallocate. Won't any valid message up to 504 bytes whose escaped length
exceeds 512 bytes fail in push_within_capacity()?
> +
> + bound.write_all(&buf, 0)?;
> +
> + Ok(())
> + }
> +}
[Severity: Medium]
Can bound.write_all(&buf, 0) in GbNode::write() return a partial write count
without returning an error?
In drivers/tty/serdev/core.c, if serdev_device_write() writes part of the
buffer and is then interrupted while waiting for space, it returns the number
of bytes written so far:
drivers/tty/serdev/core.c:serdev_device_write() {
if (timeout <= 0 && written == 0) {
if (timeout == -ERESTARTSYS)
return -ERESTARTSYS;
else
return -ETIMEDOUT;
}
return written;
}
serdev::Device::write_all() in rust/kernel/serdev.rs wraps that positive count
as Ok(written). Since GbNode::write() ignores the returned usize and returns
Ok(()), won't a partial write leave a truncated frame on the wire without the
closing 0x7e flag while reporting to gb-softsvc that the message was sent?
> +
> +#[pin_data]
> +struct GbUartNode {
> + #[pin]
> + rx: SpinLock<HdlcRx>,
> +}
> +
> +impl GbUartNode {
> + fn init(sdev: &serdev::Device<Core<'_>>) -> Result<HdlcRx> {
> + if sdev
> + .set_baudrate(
> + sdev.as_ref()
> + .fwnode()
> + .and_then(|fwnode| fwnode.property_read(c"current-speed").optional())
> + .unwrap_or(115200),
> + )
> + .is_err()
> + {
> + return Err(EINVAL);
> + }
[Severity: Low]
In GbUartNode::init() in drivers/greybus/gb_uart_node.rs, the driver reads
the "current-speed" property, whereas the commit description states that port
parameters are taken from "baudrate" if present. Should the commit message be
updated to say "current-speed"?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-gb-uart-transport-v4-0-c308ce821d45@beagleboard.org?part=6
^ permalink raw reply [flat|nested] 10+ messages in thread