* [PATCH 1/7] greybus: connection: Export gb_connection_get() and gb_connection_put()
2026-08-20 9:25 [PATCH 0/7] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
@ 2026-08-20 9:25 ` Ayush Singh
2026-08-20 9:41 ` sashiko-bot
2026-08-20 9:25 ` [PATCH 2/7] greybus: connection: Add gb_connection_hd_find_by_intf() Ayush Singh
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Ayush Singh @ 2026-08-20 9:25 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
Cc: greybus-dev, linux-kernel, rust-for-linux, linux-crypto,
devicetree, Ayush Singh
gb_connection_get() and gb_connection_put() are currently private to
the core. Drivers that hold a connection reference obtained from a
lookup helper need to be able to drop it, and drivers that cache a
connection pointer need to be able to take one.
Make both functions non-static, declare them in the greybus connection
header, and export them so modular drivers can manage connection
references. The first user is Rust greybus abstractions, added later in
this series.
No functional change.
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
drivers/greybus/connection.c | 6 ++++--
include/linux/greybus/connection.h | 3 +++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/greybus/connection.c b/drivers/greybus/connection.c
index bd04485decb3..f8fda22bc466 100644
--- a/drivers/greybus/connection.c
+++ b/drivers/greybus/connection.c
@@ -33,19 +33,21 @@ static bool gb_connection_cport_in_use(struct gb_interface *intf, u16 cport_id)
return false;
}
-static void gb_connection_get(struct gb_connection *connection)
+void gb_connection_get(struct gb_connection *connection)
{
kref_get(&connection->kref);
trace_gb_connection_get(connection);
}
+EXPORT_SYMBOL_GPL(gb_connection_get);
-static void gb_connection_put(struct gb_connection *connection)
+void gb_connection_put(struct gb_connection *connection)
{
trace_gb_connection_put(connection);
kref_put(&connection->kref, gb_connection_kref_release);
}
+EXPORT_SYMBOL_GPL(gb_connection_put);
/*
* Returns a reference-counted pointer to the connection if found.
diff --git a/include/linux/greybus/connection.h b/include/linux/greybus/connection.h
index d59b7fc1de3e..b53aad2270d6 100644
--- a/include/linux/greybus/connection.h
+++ b/include/linux/greybus/connection.h
@@ -128,4 +128,7 @@ static inline void gb_connection_set_data(struct gb_connection *connection,
connection->private = data;
}
+void gb_connection_get(struct gb_connection *connection);
+void gb_connection_put(struct gb_connection *connection);
+
#endif /* __CONNECTION_H */
--
2.55.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/7] greybus: connection: Export gb_connection_get() and gb_connection_put()
2026-08-20 9:25 ` [PATCH 1/7] greybus: connection: Export gb_connection_get() and gb_connection_put() Ayush Singh
@ 2026-08-20 9:41 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-08-20 9:41 UTC (permalink / raw)
To: Ayush Singh; +Cc: robh, conor+dt, devicetree, ojeda
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Exporting `gb_connection_get/put` allows connections to outlive their parent objects, causing a Use-After-Free in tracepoints.
--
--- Patch [1]: [PATCH 1/7] greybus: connection: Export gb_connection_get() and gb_connection_put() ---
commit d7d6f3dcc2daeb414806a0f2b9dbabd440f8ec1e
Author: Ayush Singh <ayush@beagleboard.org>
greybus: connection: Export gb_connection_get() and gb_connection_put()
This commit exports gb_connection_get() and gb_connection_put() to allow
modular drivers to manage connection references. It changes them to non-static
and adds EXPORT_SYMBOL_GPL declarations.
> diff --git a/drivers/greybus/connection.c b/drivers/greybus/connection.c
> index bd04485decb3d..f8fda22bc4667 100644
> --- a/drivers/greybus/connection.c
> +++ b/drivers/greybus/connection.c
[ ... ]
> -static void gb_connection_get(struct gb_connection *connection)
> +void gb_connection_get(struct gb_connection *connection)
> {
> kref_get(&connection->kref);
>
> trace_gb_connection_get(connection);
> }
> +EXPORT_SYMBOL_GPL(gb_connection_get);
>
> -static void gb_connection_put(struct gb_connection *connection)
> +void gb_connection_put(struct gb_connection *connection)
> {
> trace_gb_connection_put(connection);
>
> kref_put(&connection->kref, gb_connection_kref_release);
> }
> +EXPORT_SYMBOL_GPL(gb_connection_put);
[Severity: High]
Does exporting this function introduce a regression where a connection can
outlive its parent host device or bundle, potentially causing a use-after-free
in the tracepoints?
If an external module caches a connection reference and drops it asynchronously
after the Greybus core has torn down and freed the gb_bundle and
gb_host_device, calling gb_connection_put() will trigger
trace_gb_connection_put() and potentially trace_gb_connection_release().
These trace events rely on a shared TP_fast_assign block in
drivers/greybus/greybus_trace.h that unconditionally dereferences the parent
objects:
drivers/greybus/greybus_trace.h:gb_connection() {
TP_fast_assign(
__entry->hd_bus_id = connection->hd->bus_id;
__entry->bundle_id = connection->bundle ?
connection->bundle->id : BUNDLE_ID_NONE;
...
}
If the parent hd or bundle has already been freed, will these tracepoints
dereference freed memory?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-gb-uart-transport-v1-0-282da14ab7b7@beagleboard.org?part=1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/7] greybus: connection: Add gb_connection_hd_find_by_intf()
2026-08-20 9:25 [PATCH 0/7] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
2026-08-20 9:25 ` [PATCH 1/7] greybus: connection: Export gb_connection_get() and gb_connection_put() Ayush Singh
@ 2026-08-20 9:25 ` Ayush Singh
2026-08-20 9:32 ` sashiko-bot
2026-08-20 9:25 ` [PATCH 3/7] rust: crc_ccitt: add CRC-CCITT abstraction Ayush Singh
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Ayush Singh @ 2026-08-20 9:25 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
Cc: greybus-dev, linux-kernel, rust-for-linux, linux-crypto,
devicetree, Ayush Singh
gb_connection_hd_find() looks up a connection on a host device by its
host-side CPort id. Callers that only know the remote end of a
connection (the interface id and the CPort id) have no way to find it.
Add gb_connection_hd_find_by_intf(), which walks the host device's
connection list and returns a reference-counted pointer to the matching
connection, or NULL if there is none. The caller is responsible for
dropping the reference with gb_connection_put(). The first user is
gb-softsvc driver, added later in this series.
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
drivers/greybus/connection.c | 22 ++++++++++++++++++++++
include/linux/greybus/connection.h | 3 +++
2 files changed, 25 insertions(+)
diff --git a/drivers/greybus/connection.c b/drivers/greybus/connection.c
index f8fda22bc466..262161fa4721 100644
--- a/drivers/greybus/connection.c
+++ b/drivers/greybus/connection.c
@@ -71,6 +71,28 @@ gb_connection_hd_find(struct gb_host_device *hd, u16 cport_id)
return connection;
}
+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->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;
+}
+EXPORT_SYMBOL_GPL(gb_connection_hd_find_by_intf);
+
/*
* Callback from the host driver to let us know that data has been
* received on the bundle.
diff --git a/include/linux/greybus/connection.h b/include/linux/greybus/connection.h
index b53aad2270d6..589721d46015 100644
--- a/include/linux/greybus/connection.h
+++ b/include/linux/greybus/connection.h
@@ -131,4 +131,7 @@ static inline void gb_connection_set_data(struct gb_connection *connection,
void gb_connection_get(struct gb_connection *connection);
void gb_connection_put(struct gb_connection *connection);
+struct gb_connection *gb_connection_hd_find_by_intf(struct gb_host_device *hd,
+ u8 intf_id, u16 intf_cport);
+
#endif /* __CONNECTION_H */
--
2.55.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/7] greybus: connection: Add gb_connection_hd_find_by_intf()
2026-08-20 9:25 ` [PATCH 2/7] greybus: connection: Add gb_connection_hd_find_by_intf() Ayush Singh
@ 2026-08-20 9:32 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-08-20 9:32 UTC (permalink / raw)
To: Ayush Singh; +Cc: devicetree, robh, conor+dt, ojeda
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional dereference of `connection->intf` in `gb_connection_hd_find_by_intf` causes a kernel panic when encountering static connections.
--
--- Patch [2]: [PATCH 2/7] greybus: connection: Add gb_connection_hd_find_by_intf() ---
commit aaa6e0238f2ed224615968bb243b01db7948e7a8
Author: Ayush Singh <ayush@beagleboard.org>
greybus: connection: Add gb_connection_hd_find_by_intf()
This commit introduces a new function to look up a connection on a host
device using its remote interface id and CPort id. The function walks
the host device's connection list and returns a reference-counted pointer
to the matching connection.
> diff --git a/drivers/greybus/connection.c b/drivers/greybus/connection.c
> index f8fda22bc4667..262161fa4721c 100644
> --- a/drivers/greybus/connection.c
> +++ b/drivers/greybus/connection.c
[ ... ]
> +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->interface_id == intf_id &&
[Severity: High]
Could this cause a NULL pointer dereference for static connections?
Static connections, such as the core SVC connection created in gb_svc_add(),
are instantiated via gb_connection_create_static() which passes NULL for the
intf parameter. This leaves connection->intf as NULL.
Since static connections are appended to the hd->connections list, will
this loop crash when evaluating connection->intf->interface_id if it
encounters a static connection before finding a match?
> + connection->intf_cport_id == intf_cport) {
> + gb_connection_get(connection);
> + goto found;
> + }
> + }
> + connection = NULL;
> +found:
> + spin_unlock_irqrestore(&gb_connections_lock, flags);
> +
> + return connection;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-gb-uart-transport-v1-0-282da14ab7b7@beagleboard.org?part=2
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/7] rust: crc_ccitt: add CRC-CCITT abstraction
2026-08-20 9:25 [PATCH 0/7] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
2026-08-20 9:25 ` [PATCH 1/7] greybus: connection: Export gb_connection_get() and gb_connection_put() Ayush Singh
2026-08-20 9:25 ` [PATCH 2/7] greybus: connection: Add gb_connection_hd_find_by_intf() Ayush Singh
@ 2026-08-20 9:25 ` Ayush Singh
2026-08-20 9:33 ` sashiko-bot
2026-08-20 9:25 ` [PATCH 4/7] rust: kernel: Add greybus abstractions Ayush Singh
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Ayush Singh @ 2026-08-20 9:25 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
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 6008f16ae2ca..1e3b42eff741 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6921,6 +6921,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..aceb8fe8f0ed 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 "Rust CRC-CCITT abstractions"
+ 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 24d659a87c1d..6eb3caaee497 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 3edc343caba5..45c4f4db51d2 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.55.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 3/7] rust: crc_ccitt: add CRC-CCITT abstraction
2026-08-20 9:25 ` [PATCH 3/7] rust: crc_ccitt: add CRC-CCITT abstraction Ayush Singh
@ 2026-08-20 9:33 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-08-20 9:33 UTC (permalink / raw)
To: Ayush Singh; +Cc: ojeda, conor+dt, robh, devicetree
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing import for `bindings` in `crc_ccitt.rs`, leading to a compilation failure when `CONFIG_RUST_CRC_CCITT_ABSTRACTIONS=y`.
--
--- Patch [3]: [PATCH 3/7] rust: crc_ccitt: add CRC-CCITT abstraction ---
commit 07276b496fabcc3fbd341c5cd0116734faf9ec26
Author: Ayush Singh <ayush@beagleboard.org>
rust: crc_ccitt: add CRC-CCITT abstraction
This commit adds a Rust abstraction for the CRC-CCITT library functions. The
module is gated behind the new CONFIG_RUST_CRC_CCITT_ABSTRACTIONS Kconfig
symbol so the C library is only built when required by a Rust user.
> diff --git a/rust/kernel/crc_ccitt.rs b/rust/kernel/crc_ccitt.rs
> new file mode 100644
> index 0000000000000..6042ada16967d
> --- /dev/null
> +++ b/rust/kernel/crc_ccitt.rs
[ ... ]
> +#[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()) }
[Severity: Low]
Does this code fail to compile when CONFIG_RUST_CRC_CCITT_ABSTRACTIONS=y?
It looks like the bindings module is not imported in this file, which
would cause a build failure when resolving bindings::crc_ccitt here.
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-gb-uart-transport-v1-0-282da14ab7b7@beagleboard.org?part=3
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/7] rust: kernel: Add greybus abstractions
2026-08-20 9:25 [PATCH 0/7] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
` (2 preceding siblings ...)
2026-08-20 9:25 ` [PATCH 3/7] rust: crc_ccitt: add CRC-CCITT abstraction Ayush Singh
@ 2026-08-20 9:25 ` Ayush Singh
2026-08-20 9:42 ` sashiko-bot
2026-08-20 9:25 ` [PATCH 5/7] drivers: greybus: Add software SVC implementation Ayush Singh
` (2 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Ayush Singh @ 2026-08-20 9:25 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
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.
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
MAINTAINERS | 1 +
rust/bindings/bindings_helper.h | 1 +
rust/kernel/greybus/hd.rs | 315 +++++++++++++++++++++++++++++++
rust/kernel/greybus/mod.rs | 230 +++++++++++++++++++++++
rust/kernel/greybus/protocols.rs | 392 +++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
6 files changed, 941 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 1e3b42eff741..80247a031353 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11338,6 +11338,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/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 6eb3caaee497..ded5300e75da 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -62,6 +62,7 @@
#include <linux/file.h>
#include <linux/firmware.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/greybus/hd.rs b/rust/kernel/greybus/hd.rs
new file mode 100644
index 000000000000..56435074f8dc
--- /dev/null
+++ b/rust/kernel/greybus/hd.rs
@@ -0,0 +1,315 @@
+// 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>`], 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, 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::ptr::addr_of_mut;
+use core::{marker::PhantomData, ptr::NonNull};
+
+use kernel::{device, prelude::*};
+
+use crate::error::{code, from_err_ptr, to_result};
+use crate::greybus::Connection;
+use crate::sync::aref::{ARef, AlwaysRefCounted};
+use crate::{greybus::Message, 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`.
+ ///
+ /// This may be called in atomic context and therefore must not sleep; queue the message and
+ /// return. Once the core is allowed to release the message, call
+ /// [`HostDevice::message_sent`].
+ ///
+ /// The message is only borrowed for the duration of this call: copy what is needed out of it,
+ /// do not stash the reference.
+ fn message_send(data: &Self, dest_cport_id: u16, msg: Message) -> 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) };
+
+ match T::message_send(&data, dest_cport_id, msg) {
+ Ok(()) => 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>`] 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())
+ }
+ }
+
+ /// Looks up the connection bound to `cport` on interface `id`.
+ pub fn find_connection_by_intf(&self, id: u8, cport: u16) -> Option<ARef<Connection>> {
+ // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
+ // `struct gb_host_device`.
+ let ptr = NonNull::new(unsafe {
+ bindings::gb_connection_hd_find_by_intf(self.as_raw(), id, cport)
+ })?;
+
+ // SAFETY: ptr is a valid gb_connection
+ Some(unsafe { ARef::from_raw(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)) }
+ }
+}
+
+/// 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: `gb_host_device` and `HostDevice` have the same layout.
+ let hd_dev = unsafe { &*hd.cast::<Device<device::CoreInternal<'_>>>() };
+ hd_dev.as_ref().set_drvdata(data)?;
+
+ let res = Self {
+ ptr: NonNull::new(hd).ok_or(code::ENOMEM)?,
+ _data: PhantomData,
+ };
+
+ res.add()?;
+
+ Ok(res)
+ }
+}
+
+impl<T> Registration<T> {
+ fn add(&self) -> Result<()> {
+ // SAFETY: By the type invariant the host device is valid, and it has not been added yet.
+ to_result(unsafe { bindings::gb_hd_add(self.as_raw()) })
+ }
+
+ 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) {
+ {
+ let hd = self.as_raw();
+ // SAFETY: By the type invariant `hd` points at a valid host device, and
+ // `gb_host_device` and `Device` have the same layout.
+ let hd_dev = unsafe { &*hd.cast::<Device<device::CoreInternal<'a>>>() };
+ // 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.
+ let data = unsafe { hd_dev.as_ref().drvdata_obtain::<T>() };
+ drop(data);
+ }
+
+ // 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: 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 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 for Registration<T> {}
diff --git a/rust/kernel/greybus/mod.rs b/rust/kernel/greybus/mod.rs
new file mode 100644
index 000000000000..5dd1941574cf
--- /dev/null
+++ b/rust/kernel/greybus/mod.rs
@@ -0,0 +1,230 @@
+// 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 kernel::sync::aref::AlwaysRefCounted;
+use kernel::transmute::FromBytes;
+use kernel::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(NonNull<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(ptr: *mut bindings::gb_message) -> Self {
+ // SAFETY: The caller guarantees `ptr` is non-null.
+ Self(unsafe { NonNull::new_unchecked(ptr) })
+ }
+
+ /// Returns the operation header at the start of the message.
+ #[inline]
+ pub const fn header(&self) -> &protocols::GbOperationMsgHdr {
+ // SAFETY: By the type invariant the message is valid, and so is its header.
+ let msg = unsafe { &*self.0.as_ptr() };
+ // 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 { &*msg.header.cast() }
+ }
+
+ /// Tells the Greybus core the transport is done with this message.
+ ///
+ /// `status` is `0` on success or a negative errno describing the transmit failure.
+ #[inline]
+ pub fn sent(self, 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(
+ self.operation().connection().host_device().as_raw(),
+ self.0.as_ptr(),
+ status,
+ );
+ }
+ }
+
+ /// Returns the payload, i.e. the message without its operation header.
+ pub const fn payload_bytes(&self) -> &[u8] {
+ // SAFETY: By the type invariant the message is valid.
+ let msg = unsafe { &*self.0.as_ptr() };
+
+ if msg.payload.is_null() || msg.payload_size == 0 {
+ return &[];
+ }
+
+ // SAFETY: A non-null `payload` points at `payload_size` initialized bytes.
+ unsafe { core::slice::from_raw_parts(msg.payload.cast::<u8>(), msg.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.as_ptr()).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.
+ unsafe { Connection::from_raw((*self.0.get()).connection) }
+ }
+}
+
+/// A Greybus connection.
+///
+/// # Invariants
+///
+/// The wrapped value is a valid `struct gb_connection`.
+#[repr(transparent)]
+pub struct Connection(Opaque<bindings::gb_connection>);
+
+// SAFETY: `gb_connection_put()` drops the reference acquired by `gb_connection_get()`, so the
+// connection stays alive for as long as increments are outstanding.
+unsafe impl AlwaysRefCounted for Connection {
+ #[inline]
+ fn inc_ref(&self) {
+ // SAFETY: By the type invariant there is a live reference to the connection.
+ unsafe { bindings::gb_connection_get(self.0.get()) }
+ }
+
+ #[inline]
+ unsafe fn dec_ref(obj: NonNull<Self>) {
+ // SAFETY: The caller guarantees it owns an increment on the reference count.
+ unsafe { bindings::gb_connection_put(obj.as_ptr().cast()) }
+ }
+}
+
+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 const fn interface(&self) -> Option<&Interface> {
+ // SAFETY: By the type invariant the connection is valid.
+ let intf_ptr = unsafe { (*self.0.get()).intf };
+
+ if intf_ptr.is_null() {
+ None
+ } else {
+ // SAFETY: By the previous check, intf_ptr is valid.
+ Some(unsafe { Interface::from_raw(intf_ptr) })
+ }
+ }
+
+ /// 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 }
+ }
+
+ /// Returns the host device this connection belongs to.
+ #[inline]
+ pub const fn host_device(&self) -> &hd::Device {
+ // SAFETY: By the type invariant the connection is valid, and its `hd` is alive for as
+ // long as the connection is.
+ unsafe { hd::Device::from_raw((*self.0.get()).hd) }
+ }
+}
+
+/// A Greybus interface.
+///
+/// # Invariants
+///
+/// The wrapped value is a valid `struct gb_interface`.
+#[repr(transparent)]
+pub struct Interface(Opaque<bindings::gb_interface>);
+
+impl Interface {
+ /// # Safety
+ ///
+ /// `ptr` must be a valid pointer to a `struct gb_interface`.
+ #[inline]
+ pub(crate) const unsafe fn from_raw<'a>(ptr: *mut bindings::gb_interface) -> &'a Self {
+ // SAFETY: `Interface` is a transparent wrapper of `Opaque<bindings::gb_interface>`.
+ unsafe { &*ptr.cast() }
+ }
+
+ /// 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 }
+ }
+}
diff --git a/rust/kernel/greybus/protocols.rs b/rust/kernel/greybus/protocols.rs
new file mode 100644
index 000000000000..4658ace5e323
--- /dev/null
+++ b/rust/kernel/greybus/protocols.rs
@@ -0,0 +1,392 @@
+// 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.
+///
+/// # Invariants
+///
+/// The `size` field covers the header and the payload that follows it.
+#[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);
+
+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);
+
+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 struct of `u8` 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);
+
+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);
+
+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);
+
+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);
+
+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);
+
+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);
+
+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);
+
+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);
+
+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);
+
+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);
+
+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 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 45c4f4db51d2..3c4bdad53de1 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -80,6 +80,8 @@
pub mod fs;
#[cfg(CONFIG_GPU_BUDDY = "y")]
pub mod gpu;
+#[cfg(CONFIG_GREYBUS)]
+pub mod greybus;
#[cfg(CONFIG_I2C = "y")]
pub mod i2c;
pub mod id_pool;
--
2.55.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 4/7] rust: kernel: Add greybus abstractions
2026-08-20 9:25 ` [PATCH 4/7] rust: kernel: Add greybus abstractions Ayush Singh
@ 2026-08-20 9:42 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-08-20 9:42 UTC (permalink / raw)
To: Ayush Singh; +Cc: conor+dt, ojeda, robh, devicetree
Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] `Registration::drop` frees driver private data before calling `gb_hd_del()`, allowing Use-After-Free in concurrent callbacks.
- [High] Missing and incorrect `Send`/`Sync` bounds on Greybus types break async usage and Rust thread safety.
- [High] `Message` does not implement `Drop`, allowing drivers to silently leak active operations.
- [High] `message_cancel` bypasses ownership guarantees, allowing an aliased `Message` to be constructed by value.
- [High] The `Registration` API leaks the `gb_host_device` memory and crashes by calling `device_del` on an un-added device on error paths.
- [High] Safe Rust API allows double-free of gb_operation if a driver completes a message and then returns an error.
- [Medium] `HdDriver::message_send` ignores the provided `gfp_mask`, breaking context-aware memory allocation.
- [Low] Conditional compilation of `greybus` abstraction causes linker errors when `CONFIG_GREYBUS=m`.
- [Low] Missing `// INVARIANT:` comments on struct constructions.
- [Low] Missing `#[inline]` annotations on abstraction wrappers.
--
--- Patch [4]: [PATCH 4/7] rust: kernel: Add greybus abstractions ---
commit a8c7543823c875208896bddcd8e1e440536e1645
Author: Ayush Singh <ayush@beagleboard.org>
rust: kernel: Add greybus abstractions
Add Rust abstractions for the Greybus core, enough to implement a
Greybus host driver in Rust.
> diff --git a/rust/kernel/greybus/hd.rs b/rust/kernel/greybus/hd.rs
> index 0000000000000..56435074f8dc8
> --- /dev/null
> +++ b/rust/kernel/greybus/hd.rs
[ ... ]
> + 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 {
[Severity: Medium]
Does this ignore the provided _gfp_mask? If it is not passed to the driver
implementation, safe Rust drivers lack the context to determine whether they
can sleep (e.g., if they need to allocate memory to queue the message),
which could lead to might_sleep() panics.
> + // 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) };
> +
> + match T::message_send(&data, dest_cport_id, msg) {
> + Ok(()) => 0,
> + Err(e) => e.to_errno(),
> + }
> + }
[Severity: High]
Does this safe Rust API allow a double-free of gb_operation?
If a driver completes a message by calling msg.sent() (which drops the
operation refcount) and then returns Err(e), this wrapper propagates the
error back to the C core.
The C core will then unconditionally drop the operation refcount again in
gb_operation_request_send(), resulting in a use-after-free.
> +
> + /// # 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);
> + }
[Severity: High]
Does message_cancel bypass ownership guarantees by constructing an aliased
Message by value?
If the driver had previously received this same gb_message and stashed it
for asynchronous processing, there are now two owned Message instances
representing the same underlying C struct. A safe driver could call
msg.sent() on both, leading to a double-free.
[ ... ]
> + pub fn new(
> + parent: &device::Device,
> + buffer_size_max: usize,
> + num_cports: usize,
> + data: impl PinInit<T, Error>,
> + ) -> Result<Self> {
[ ... ]
> + let res = Self {
> + ptr: NonNull::new(hd).ok_or(code::ENOMEM)?,
> + _data: PhantomData,
> + };
[Severity: Low]
Are we missing an // INVARIANT: comment on this struct construction, as
required by subsystem/rust.md?
> +
> + res.add()?;
> +
> + Ok(res)
> + }
[Severity: High]
If res.add() fails, res is dropped and Registration::drop() is invoked,
which calls gb_hd_del().
Since gb_hd_add() never succeeded, does calling device_del() via
gb_hd_del() on an un-added device trigger a kobject crash?
[ ... ]
> +impl<T> Registration<T> {
> + fn add(&self) -> Result<()> {
[Severity: Low]
Should this small forwarding function have an #[inline] annotation, as per
the subsystem guidelines?
[ ... ]
> +impl<T> Drop for Registration<T> {
> + fn drop<'a>(&'a mut self) {
> + {
> + let hd = self.as_raw();
> + // SAFETY: By the type invariant `hd` points at a valid host device, and
> + // `gb_host_device` and `Device` have the same layout.
> + let hd_dev = unsafe { &*hd.cast::<Device<device::CoreInternal<'a>>>() };
> + // 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.
> + let data = unsafe { hd_dev.as_ref().drvdata_obtain::<T>() };
> + drop(data);
> + }
> +
> + // 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()) }
> + }
> +}
[Severity: Critical]
By destroying the private data T before calling gb_hd_del(), could this lead
to a use-after-free in concurrent callbacks?
gb_hd_del() is synchronous and flushes active connections. This flushing can
trigger the C core to send responses via message_send. The message_send
callback borrows the driver data, which was already freed here.
[Severity: High]
After removing the device, does Registration::drop() need to call
gb_hd_put() to release the refcount?
gb_hd_create() returns a device with a refcount of 1. By never calling
gb_hd_put(), it appears the host device memory is permanently leaked upon
unregistering.
> +
> +// 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 for Registration<T> {}
[Severity: High]
Does this Send implementation require a T: Send bound?
Without it, a safe consumer could create a Registration<T> where T is !Send,
move it to another thread, and drop it, causing a !Send type to be dropped
concurrently.
> diff --git a/rust/kernel/greybus/mod.rs b/rust/kernel/greybus/mod.rs
> index 0000000000000..5dd1941574cf8
> --- /dev/null
> +++ b/rust/kernel/greybus/mod.rs
[ ... ]
> +/// 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(NonNull<bindings::gb_message>);
[Severity: High]
Is there a reason this struct doesn't implement Send? Without it, safe
asynchronous drivers cannot queue operations to worker threads.
[Severity: High]
Does this struct need a Drop implementation to prevent silent resource leaks?
If a driver simply drops the Message struct without calling sent(), the
underlying C gb_operation is permanently leaked.
> +
> +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(ptr: *mut bindings::gb_message) -> Self {
> + // SAFETY: The caller guarantees `ptr` is non-null.
> + Self(unsafe { NonNull::new_unchecked(ptr) })
> + }
[Severity: Low]
Is this struct construction missing an // INVARIANT: comment, as required
by the subsystem guidelines?
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 17af6e7969751..6d6378ade24ea 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
[ ... ]
> @@ -77,6 +77,8 @@
> pub mod fs;
> #[cfg(CONFIG_GPU_BUDDY = "y")]
> pub mod gpu;
> +#[cfg(CONFIG_GREYBUS)]
> +pub mod greybus;
[Severity: Low]
Does this cause linker errors when CONFIG_GREYBUS is built as a loadable
module?
When it is configured as =m, the build system still passes
--cfg=CONFIG_GREYBUS to rustc. This statically compiles the module into
vmlinux, leading to undefined references to C symbols like gb_hd_create
which only exist in the dynamically loadable greybus.ko module.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-gb-uart-transport-v1-0-282da14ab7b7@beagleboard.org?part=4
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/7] drivers: greybus: Add software SVC implementation
2026-08-20 9:25 [PATCH 0/7] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
` (3 preceding siblings ...)
2026-08-20 9:25 ` [PATCH 4/7] rust: kernel: Add greybus abstractions Ayush Singh
@ 2026-08-20 9:25 ` Ayush Singh
2026-08-20 9:40 ` sashiko-bot
2026-08-20 9:25 ` [PATCH 6/7] greybus: Add Rust UART node driver Ayush Singh
2026-08-20 9:25 ` [PATCH 7/7] dt-bindings: beagle: Add BeagleConnect Freedom Ayush Singh
6 siblings, 1 reply; 16+ messages in thread
From: Ayush Singh @ 2026-08-20 9:25 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
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 | 2 +
drivers/greybus/gb_softsvc.rs | 472 +++++++++++++++++++++++++++++++++++
drivers/greybus/gb_softsvc_exports.c | 15 ++
5 files changed, 507 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 80247a031353..3ba56cdbf056 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11328,6 +11328,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.com>
+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 c3f056d28b01..30bc491841e1 100644
--- a/drivers/greybus/Kconfig
+++ b/drivers/greybus/Kconfig
@@ -41,5 +41,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
+ 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..e6f594128802 100644
--- a/drivers/greybus/Makefile
+++ b/drivers/greybus/Makefile
@@ -25,4 +25,6 @@ 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
diff --git a/drivers/greybus/gb_softsvc.rs b/drivers/greybus/gb_softsvc.rs
new file mode 100644
index 000000000000..0d125a1a2cc8
--- /dev/null
+++ b/drivers/greybus/gb_softsvc.rs
@@ -0,0 +1,472 @@
+// 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::error::code;
+use kernel::greybus::{self, hd, protocols, CPORT_ID_MAX, GB_OPERATION_SIZE_MAX};
+use kernel::id_pool::IdPool;
+use kernel::sync::{Arc, Mutex, SpinLock};
+use kernel::transmute::AsBytes;
+use kernel::{c_str, faux, prelude::*, sync::aref::ARef};
+use kernel::{new_mutex, new_spinlock};
+use pin_init::init_array_from_fn;
+
+const SVC_VERSION_MAJOR: u8 = 0;
+const SVC_VERSION_MINOR: u8 = 1;
+const ENDO_ID: u16 = u16::from_le(0x4755);
+const AP_INF_ID: u8 = 0;
+
+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 a POD type with no padding and no interior mutability.
+unsafe impl<P> kernel::transmute::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: Mutex<[Option<Arc<dyn InterfaceOps>>; INTF_MAP_LEN]>,
+}
+
+impl Svc {
+ fn new() -> impl PinInit<Self, Error> {
+ pin_init!(Self {
+ intf_map <- new_mutex!(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 inner(
+ 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 = inner(&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) {
+ let id = primary_id - INTF_ID_START;
+ let mut guard = self.intf_map.lock();
+
+ for i in 0..intf_count {
+ let _ = guard[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>(&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)?;
+ let payload = protocols::GbSvcModuleInsertedRequest::new(intf_id, 1, 0);
+ self.send_request(hd, protocols::GB_SVC_TYPE_MODULE_INSERTED, payload)?;
+
+ Ok(intf_id)
+ }
+
+ fn module_remove(&self, hd: &hd::Device, intf_id: u8, intf_count: u8) -> Result<()> {
+ self.map_remove(intf_id, intf_count);
+ let payload = protocols::GbSvcModuleRemovedRequest::new(intf_id);
+ self.send_request(hd, protocols::GB_SVC_TYPE_MODULE_REMOVED, payload)?;
+
+ 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>(&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, u32::from_le(0x0126));
+ 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, msg: &greybus::Message) -> Result<()> {
+ let hdr = msg.header();
+ let hd = msg.operation().connection().host_device();
+
+ 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>);
+
+impl GbHdDriver {
+ fn message_send_inner(&self, dest_cport_id: u16, msg: &greybus::Message) -> Result {
+ if dest_cport_id == protocols::GB_SVC_CPORT_ID {
+ self.0.handler(msg)
+ } else {
+ let conn = msg.operation().connection();
+ let intf = self.0.intf_by_id(conn.interface().unwrap().id())?;
+
+ let mut buf = KVec::with_capacity(msg.header().size().into(), GFP_KERNEL)?;
+
+ buf.extend_from_slice(msg.header().as_bytes(), GFP_KERNEL)?;
+ buf.extend_from_slice(msg.payload_bytes(), GFP_KERNEL)?;
+
+ intf.write(&buf, conn.intf_cport_id())
+ }
+ }
+}
+
+#[vtable]
+impl hd::HdDriver for GbHdDriver {
+ fn message_send(data: &Self, dest_cport_id: u16, msg: greybus::Message) -> Result {
+ let res = data.message_send_inner(dest_cport_id, &msg);
+ msg.sent(0);
+
+ res
+ }
+
+ 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))?;
+
+ let global_state = GlobalState::new(svc.clone(), hd.as_ref().into());
+ let _ = GLOBAL_STATE.lock().replace(global_state);
+
+ svc.send_version(hd.as_ref())?;
+
+ 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) -> 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);
+ };
+
+ 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.
+ ///
+ /// `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, cport: u16, msg: &[u8]) -> Result<()> {
+ let intf = self
+ .gb_hd
+ .find_connection_by_intf(self.id, cport)
+ .ok_or(code::EINVAL)?;
+ self.gb_hd.data_rcvd(intf.hd_cport_id(), msg);
+
+ Ok(())
+ }
+}
+
+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..38b99c4de6ee
--- /dev/null
+++ b/drivers/greybus/gb_softsvc_exports.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+
+/*
+ * Exports Rust symbols from the `nova_core` 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.55.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 5/7] drivers: greybus: Add software SVC implementation
2026-08-20 9:25 ` [PATCH 5/7] drivers: greybus: Add software SVC implementation Ayush Singh
@ 2026-08-20 9:40 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-08-20 9:40 UTC (permalink / raw)
To: Ayush Singh; +Cc: conor+dt, devicetree, ojeda, robh
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Calling `Module::new` with an empty slice panics the kernel.
- [High] Out-of-bounds bitmap access in `release_operation_id` crashes the kernel.
- [High] Unconditionally calling `msg.sent(0)` followed by returning an error causes a double-free of the operation.
- [High] Initialization error path permanently leaks `GLOBAL_STATE`.
- [High] InterfaceOps are leaked in Svc::module_insert() if send_request() fails.
- [High] Incorrect endianness conversions for Greybus SVC constant values break big-endian platforms.
- [Medium] `module_insert` hardcodes `intf_count` to 1, breaking support for modules with multiple interfaces.
- [Low] Copy-paste errors in `gb_softsvc_exports.c` metadata.
--
--- Patch [5]: [PATCH 5/7] drivers: greybus: Add software SVC implementation ---
commit ae94a156cc024cc7dae613127a7c0c6694200c02
Author: Ayush Singh <ayush@beagleboard.org>
drivers: greybus: Add software SVC implementation
A Greybus network requires an SVC (Supervisory Controller) to bring
interfaces up, assign device IDs and connect CPorts to the AP. This adds
gb-softsvc, a Rust implementation of the SVC that lives in the kernel
and registers itself as a Greybus host device.
> diff --git a/drivers/greybus/gb_softsvc.rs b/drivers/greybus/gb_softsvc.rs
> new file mode 100644
> index 0000000000000..0d125a1a2cc8f
> --- /dev/null
> +++ b/drivers/greybus/gb_softsvc.rs
> @@ -0,0 +1,472 @@
[ ... ]
> +const SVC_VERSION_MAJOR: u8 = 0;
> +const SVC_VERSION_MINOR: u8 = 1;
> +const ENDO_ID: u16 = u16::from_le(0x4755);
[Severity: High]
Are u16::from_le() and u32::from_le() being used correctly here?
Constructor functions like GbSvcHelloRequest::new() expect native-endian
arguments and internally convert them to little-endian. Applying from_le()
followed by the internal to_le() cancels out the byte swap on big-endian
platforms, causing the data to be transmitted in native big-endian format
instead of the required little-endian.
[ ... ]
> + fn new() -> impl PinInit<Self, Error> {
> + pin_init!(Self {
> + intf_map <- new_mutex!(init_array_from_fn(|_| None)),
> + svc_operation_id <- new_spinlock!(IdPool::with_capacity(u16::MAX.into(), GFP_KERNEL)?)
[Severity: High]
The ID pool is initialized with a capacity of u16::MAX (65535), making valid
indices 0 to 65534. If an untrusted remote endpoint sends a message with
operation_id = 65535, does this pass the out-of-bounds ID directly to
release_id() in release_operation_id()?
[ ... ]
> + 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 inner(
> + intf_map: &[Option<Arc<dyn InterfaceOps>>; INTF_MAP_LEN],
> + count: usize,
> + ) -> Result<u8> {
> + for (id, window) in intf_map.windows(count).enumerate() {
[Severity: High]
If a caller passes an empty slice of interfaces to Module::new(), won't
count be 0? Calling .windows(0) on a slice unconditionally panics in
Rust, which would crash the kernel.
> + if window.iter().all(|x| x.is_none()) {
> + return Ok(id as u8);
> + }
> + }
[ ... ]
> + fn module_insert(&self, hd: &hd::Device, intfs: &[Arc<dyn InterfaceOps>]) -> Result<u8> {
> + let intf_id = self.map_insert(intfs)?;
> + let payload = protocols::GbSvcModuleInsertedRequest::new(intf_id, 1, 0);
[Severity: Medium]
Does hardcoding the intf_count to 1 prevent the Greybus core from
discovering subsequent interfaces when intfs.len() > 1?
> + self.send_request(hd, protocols::GB_SVC_TYPE_MODULE_INSERTED, payload)?;
[Severity: High]
If self.send_request() fails, the function returns an error, but the
interfaces were already inserted into intf_map via self.map_insert().
Since Module::new() aborts on this error path, Module::drop will never
be called to clean up. Does this permanently leak the slots in intf_map
and the Arc<dyn InterfaceOps> references?
> +
> + Ok(intf_id)
> + }
[ ... ]
> + fn dme_peer_get(&self, hd: &hd::Device, hdr: &protocols::GbOperationMsgHdr) {
> + let payload = protocols::GbSvcDmePeerGetResponse::new(0, u32::from_le(0x0126));
[Severity: High]
This seems to have the same endianness cancellation issue as ENDO_ID when
used on big-endian architectures.
> + self.send_response(hd, hdr, payload);
> + }
[ ... ]
> +impl hd::HdDriver for GbHdDriver {
> + fn message_send(data: &Self, dest_cport_id: u16, msg: greybus::Message) -> Result {
> + let res = data.message_send_inner(dest_cport_id, &msg);
> + msg.sent(0);
[Severity: High]
If message_send_inner() returns an error, msg.sent(0) will incorrectly
report success to the Greybus core, causing it to drop its reference to the
operation structure. When the error is then returned to the caller, it may also
drop the reference. Could this sequence result in a double-free?
> +
> + res
> + }
[ ... ]
> +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))?;
> +
> + let global_state = GlobalState::new(svc.clone(), hd.as_ref().into());
> + let _ = GLOBAL_STATE.lock().replace(global_state);
> +
> + svc.send_version(hd.as_ref())?;
[Severity: High]
If svc.send_version() fails, module initialization is aborted and
GreybusSoftSvc::drop() will not be called. Does this leave GLOBAL_STATE
populated, permanently leaking the Arc<Svc> and ARef<hd::Device> references?
> +
> + Ok(GreybusSoftSvc {
> + _hd: hd,
> + _faux: faux,
> + })
> + }
> +}
[ ... ]
> + pub fn submit_message(&self, cport: u16, msg: &[u8]) -> Result<()> {
> + let intf = self
> + .gb_hd
> + .find_connection_by_intf(self.id, cport)
[Severity: Medium]
In addition to hardcoding the interface count to 1 earlier, using self.id
here instead of allowing the caller to specify the interface ID seems to
prevent routing messages for any interfaces beyond the first one.
> + .ok_or(code::EINVAL)?;
> + self.gb_hd.data_rcvd(intf.hd_cport_id(), msg);
> +
> + Ok(())
> + }
[ ... ]
> diff --git a/drivers/greybus/gb_softsvc_exports.c b/drivers/greybus/gb_softsvc_exports.c
> new file mode 100644
> index 0000000000000..38b99c4de6eec
> --- /dev/null
> +++ b/drivers/greybus/gb_softsvc_exports.c
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> +
> +/*
> + * Exports Rust symbols from the nova_core crate for use by dependent modules.
[Severity: Low]
Are the references to NVIDIA and the nova_core crate here accidental
copy-paste errors from another driver?
> + *
> + * This is a workaround until the build system supports Rust cross-module
> + * dependencies natively.
> + */
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-gb-uart-transport-v1-0-282da14ab7b7@beagleboard.org?part=5
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 6/7] greybus: Add Rust UART node driver
2026-08-20 9:25 [PATCH 0/7] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
` (4 preceding siblings ...)
2026-08-20 9:25 ` [PATCH 5/7] drivers: greybus: Add software SVC implementation Ayush Singh
@ 2026-08-20 9:25 ` Ayush Singh
2026-08-20 9:38 ` sashiko-bot
2026-08-20 9:25 ` [PATCH 7/7] dt-bindings: beagle: Add BeagleConnect Freedom Ayush Singh
6 siblings, 1 reply; 16+ messages in thread
From: Ayush Singh @ 2026-08-20 9:25 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
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 | 6 ++
drivers/greybus/.gitignore | 1 +
drivers/greybus/Kconfig | 15 +++
drivers/greybus/Makefile | 48 +++++++++
drivers/greybus/gb_uart_node.rs | 231 ++++++++++++++++++++++++++++++++++++++++
5 files changed, 301 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 3ba56cdbf056..037a87b74800 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11334,6 +11334,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.com>
+L: greybus-dev@lists.linaro.org (moderated for non-subscribers)
+S: Maintained
+F: drivers/greybus/gb_uart_node.rs
+
GREYBUS SUBSYSTEM
M: Johan Hovold <johan@kernel.org>
M: Alex Elder <elder@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 30bc491841e1..b6f687e6da1a 100644
--- a/drivers/greybus/Kconfig
+++ b/drivers/greybus/Kconfig
@@ -53,5 +53,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 RUST
+ depends on GREYBUS_SOFTSVC
+ depends on RUST_SERIAL_DEV_BUS_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 e6f594128802..81151963c01e 100644
--- a/drivers/greybus/Makefile
+++ b/drivers/greybus/Makefile
@@ -28,3 +28,51 @@ 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); } | \
+ 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 nova-core's crate metadata for use by nova-drm 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..125812246758
--- /dev/null
+++ b/drivers/greybus/gb_uart_node.rs
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Greybus UART Node driver
+
+use kernel::crc_ccitt::crc_ccitt;
+use kernel::device::AsBusDevice;
+use kernel::device::{Bound, Core};
+use kernel::error::code;
+use kernel::sync::aref::ARef;
+use kernel::sync::{Arc, SpinLock};
+use kernel::{new_spinlock, of, prelude::*, serdev};
+
+use zerocopy::little_endian;
+use zerocopy_derive::{FromBytes, Immutable, KnownLayout};
+
+const HDLC_MAX_FRAME_LEN: usize = 256;
+
+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,
+ 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,
+ })
+ }
+
+ 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::<u8>()];
+
+ match addr {
+ ADDRESS_GREYBUS => {
+ let frame = GreybusFrame::ref_from_bytes(payload).map_err(|_| code::EINVAL)?;
+ self.node.submit_message(frame.cport.into(), &frame.msg)
+ }
+ _ => Err(code::EINVAL),
+ }
+ }
+
+ fn rx(&mut self, data: &[u8]) -> usize {
+ for (count, i) in data.iter().enumerate() {
+ 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;
+ }
+ HDLC_ESC => self.rx_in_esc = true,
+ _ => {
+ let c = if self.rx_in_esc { *i ^ HDLC_XOR } else { *i };
+ self.rx_in_esc = false;
+
+ if self.rx_buf.push(c, GFP_KERNEL).is_err() {
+ return count;
+ }
+ }
+ }
+ }
+
+ data.len()
+ }
+}
+
+struct GbNode {
+ sdev: ARef<serdev::Device>,
+}
+
+impl GbNode {
+ const fn new(sdev: ARef<serdev::Device>) -> Self {
+ Self { sdev }
+ }
+
+ fn write_all(mut crc: u16, bound: &serdev::Device<Bound>, data: &[u8]) -> Result<u16> {
+ for i in data {
+ if *i == HDLC_ESC || *i == HDLC_FRAME {
+ let buf = &[HDLC_ESC, i ^ HDLC_XOR];
+ bound.write_all(buf, 0)?;
+ crc = crc_ccitt(crc, buf);
+ } else {
+ bound.write_all(&[*i], 0)?;
+ crc = crc_ccitt(crc, &[*i]);
+ }
+ }
+
+ Ok(crc)
+ }
+}
+
+impl gb_softsvc::InterfaceOps for GbNode {
+ fn write(&self, data: &[u8], cport: u16) -> 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()) };
+
+ let mut crc = 0xffff;
+
+ bound.write_all(&[HDLC_FRAME], 0)?;
+
+ crc = Self::write_all(crc, bound, &[ADDRESS_GREYBUS, 0x03])?;
+ crc = Self::write_all(crc, bound, &cport.to_le_bytes())?;
+ crc = Self::write_all(crc, bound, data)?;
+
+ crc ^= 0xffff;
+ Self::write_all(crc, bound, &crc.to_le_bytes())?;
+
+ bound.write_all(&[HDLC_FRAME], 0)?;
+
+ Ok(())
+ }
+}
+
+#[pin_data(PinnedDrop)]
+struct GbUartNode {
+ sdev: ARef<serdev::Device>,
+ #[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"baudrate").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");
+ let rx = Self::init(sdev);
+
+ try_pin_init!(Self {
+ sdev: sdev.into(),
+ rx <- new_spinlock!(rx?, "gb_uart_node::rx"),
+ }? Error)
+ }
+
+ fn receive<'bound>(
+ _sdev: &'bound serdev::Device<Bound>,
+ this: Pin<&Self>,
+ data: &[u8],
+ ) -> usize {
+ let Some(mut guard) = this.rx.try_lock() else {
+ return 0;
+ };
+
+ guard.rx(data)
+ }
+}
+
+#[pinned_drop]
+impl PinnedDrop for GbUartNode {
+ fn drop(self: Pin<&mut Self>) {
+ dev_dbg!(self.sdev.as_ref(), "Remove gb_uart_node.\n");
+ }
+}
+
+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.55.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 6/7] greybus: Add Rust UART node driver
2026-08-20 9:25 ` [PATCH 6/7] greybus: Add Rust UART node driver Ayush Singh
@ 2026-08-20 9:38 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-08-20 9:38 UTC (permalink / raw)
To: Ayush Singh; +Cc: ojeda, robh, devicetree, conor+dt
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Incorrect extraction of payload leaves part of the CRC in the message due to an off-by-one calculation.
- [High] The transmit CRC is incorrectly computed over the HDLC-escaped byte stream instead of the raw payload.
- [High] Unbounded memory allocation in the serial receive path allows an attacker to exhaust kernel memory.
- [High] Allocating memory with `GFP_KERNEL` while holding a spinlock causes a 'scheduling while atomic' kernel panic.
- [High] GbNode is registered with the Greybus core in `probe` before the serdev device is fully initialized, allowing premature and unsafe calls to `write`.
- [High] `GbNode::write` performs multiple non-atomic `write_all` operations to the UART without serialization, risking interleaved frames if called concurrently.
- [Medium] Returning early upon a buffer push failure clears the HDLC escape state prematurely, causing an infinite receive stall.
- [Low] Rust imports do not follow the kernel vertical import style.
--
--- Patch [6]: [PATCH 6/7] greybus: Add Rust UART node driver ---
commit 763ce224c179bf0f26e68cd794db14b2b9027390
Author: Ayush Singh <ayush@beagleboard.org>
greybus: Add Rust UART node driver
This commit adds 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. Greybus messages are carried
over HDLC framing on the wire.
[ ... ]
> diff --git a/drivers/greybus/gb_uart_node.rs b/drivers/greybus/gb_uart_node.rs
> new file mode 100644
> index 0000000000000..1258122467585
> --- /dev/null
> +++ b/drivers/greybus/gb_uart_node.rs
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Greybus UART Node driver
> +
> +use kernel::crc_ccitt::crc_ccitt;
> +use kernel::device::AsBusDevice;
> +use kernel::device::{Bound, Core};
> +use kernel::error::code;
> +use kernel::sync::aref::ARef;
> +use kernel::sync::{Arc, SpinLock};
> +use kernel::{new_spinlock, of, prelude::*, serdev};
[Severity: Low]
Do these imports violate the kernel's vertical import guidelines for Rust
code?
[ ... ]
> + 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::<u8>()];
[Severity: High]
Does this slice operation incorrectly leave 1 byte of the 2-byte CRC appended
to the payload? The CRC is 16-bit, so it appears subtracting size_of::<u8>()
would retain part of the CRC in the message.
[ ... ]
> + fn rx(&mut self, data: &[u8]) -> usize {
> + for (count, i) in data.iter().enumerate() {
> + 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;
> + }
> + HDLC_ESC => self.rx_in_esc = true,
> + _ => {
> + let c = if self.rx_in_esc { *i ^ HDLC_XOR } else { *i };
> + self.rx_in_esc = false;
> +
> + if self.rx_buf.push(c, GFP_KERNEL).is_err() {
[Severity: High]
Is there a bounds or MTU capacity check missing here? It looks like a
malfunctioning or malicious device on the serial port streaming endless
non-framing bytes could cause the vector to grow infinitely until kernel
memory is exhausted.
[Severity: Medium]
If the buffer push fails and we return early, does this clear the HDLC
escape state prematurely?
The escape state is mutated right before the error check:
self.rx_in_esc = false;
if self.rx_buf.push(c, GFP_KERNEL).is_err() {
When the serdev core retries passing the failed byte, the escape state
would already be lost, potentially causing an infinite receive stall.
[Severity: High]
Is it safe to use GFP_KERNEL here?
GbUartNode::receive() acquires a spinlock using this.rx.try_lock(), which
disables preemption. It then calls guard.rx(data), which invokes this
buffer push allocation.
Will allocating memory with GFP_KERNEL while holding a spinlock cause a
scheduling while atomic kernel panic?
> + return count;
> + }
> + }
> + }
> + }
[ ... ]
> + fn write_all(mut crc: u16, bound: &serdev::Device<Bound>, data: &[u8]) -> Result<u16> {
> + for i in data {
> + if *i == HDLC_ESC || *i == HDLC_FRAME {
> + let buf = &[HDLC_ESC, i ^ HDLC_XOR];
> + bound.write_all(buf, 0)?;
> + crc = crc_ccitt(crc, buf);
[Severity: High]
Is the transmit CRC incorrectly computed over the HDLC-escaped byte stream
instead of the raw payload?
GbNode::write_all() computes the CRC using crc_ccitt() on the escaped byte
sequence rather than the original raw byte. Could this result in an invalid
CRC that gets rejected by standard receivers?
[ ... ]
> +impl gb_softsvc::InterfaceOps for GbNode {
> + fn write(&self, data: &[u8], cport: u16) -> 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]
Could this unsafe block invalidly assume the serdev device is fully bound
before probe completes?
GbUartNode::probe() calls Self::init(), which registers the module with
gb_softsvc. If a write callback is invoked synchronously or asynchronously
before probe finishes, this assumption might be violated.
> +
> + let mut crc = 0xffff;
> +
> + bound.write_all(&[HDLC_FRAME], 0)?;
> +
> + crc = Self::write_all(crc, bound, &[ADDRESS_GREYBUS, 0x03])?;
> + crc = Self::write_all(crc, bound, &cport.to_le_bytes())?;
> + crc = Self::write_all(crc, bound, data)?;
[Severity: High]
Does GbNode::write() perform multiple non-atomic write_all operations to the
UART without serialization?
Since the Greybus core can send messages concurrently from different threads
for different CPorts, could interleaving these writes result in corrupted
HDLC framing and dropped packets on the wire?
[ ... ]
> + 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");
> + let rx = Self::init(sdev);
[Severity: High]
Is GbNode prematurely registered with the Greybus core here?
Calling Self::init() registers the module before the serdev device is
fully initialized, allowing premature and unsafe calls to GbNode::write().
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-gb-uart-transport-v1-0-282da14ab7b7@beagleboard.org?part=6
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 7/7] dt-bindings: beagle: Add BeagleConnect Freedom
2026-08-20 9:25 [PATCH 0/7] Add Greybus Sotfsvc and UART Node drivers Ayush Singh
` (5 preceding siblings ...)
2026-08-20 9:25 ` [PATCH 6/7] greybus: Add Rust UART node driver Ayush Singh
@ 2026-08-20 9:25 ` Ayush Singh
2026-08-20 9:31 ` sashiko-bot
2026-08-20 18:33 ` Conor Dooley
6 siblings, 2 replies; 16+ messages in thread
From: Ayush Singh @ 2026-08-20 9:25 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
Cc: greybus-dev, linux-kernel, rust-for-linux, linux-crypto,
devicetree, Ayush Singh
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.
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
.../beagle/beagle,beagleconnect-freedom.yaml | 30 ++++++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 31 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..5a246f81261e
--- /dev/null
+++ b/Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml
@@ -0,0 +1,30 @@
+# 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
+
+required:
+ - compatible
+
+additionalProperties: false
+
+examples:
+ - |
+ serial {
+ mcu {
+ compatible = "beagle,beagleconnect-freedom";
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 037a87b74800..49c6dac72748 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11338,6 +11338,7 @@ GREYBUS UART NODE DRIVERS
M: Ayush Singh <ayush@beagleboard.com>
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
--
2.55.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 7/7] dt-bindings: beagle: Add BeagleConnect Freedom
2026-08-20 9:25 ` [PATCH 7/7] dt-bindings: beagle: Add BeagleConnect Freedom Ayush Singh
@ 2026-08-20 9:31 ` sashiko-bot
2026-08-20 18:33 ` Conor Dooley
1 sibling, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-08-20 9:31 UTC (permalink / raw)
To: Ayush Singh; +Cc: conor+dt, devicetree, robh, ojeda
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The binding enforces `additionalProperties: false` but fails to define `reg` and other standard bus properties, preventing its use on SPI, I2C, and USB buses.
--
--- Patch [7]: [PATCH 7/7] dt-bindings: beagle: Add BeagleConnect Freedom ---
commit d458668c4d5561c44786c09e0ae03714baa79492
Author: Ayush Singh <ayush@beagleboard.org>
dt-bindings: beagle: Add BeagleConnect Freedom
This commit adds devicetree bindings for the BeagleBoard.org BeagleConnect
Freedom board. 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.
> diff --git a/Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml b/Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml
[ ... ]
> +properties:
> + compatible:
> + const: beagle,beagleconnect-freedom
> +
> +required:
> + - compatible
> +
> +additionalProperties: false
[Severity: Low]
Will this reject standard bus properties like "reg" during schema validation?
Since the commit message mentions this node can be attached to SPI, I2C, or
USB buses, it will likely need standard bus properties. With
additionalProperties set to false and "reg" not defined in the properties
list, does this mean defining the node as a child on those buses will fail
dtbs_check?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-gb-uart-transport-v1-0-282da14ab7b7@beagleboard.org?part=7
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/7] dt-bindings: beagle: Add BeagleConnect Freedom
2026-08-20 9:25 ` [PATCH 7/7] dt-bindings: beagle: Add BeagleConnect Freedom Ayush Singh
2026-08-20 9:31 ` sashiko-bot
@ 2026-08-20 18:33 ` Conor Dooley
1 sibling, 0 replies; 16+ messages in thread
From: Conor Dooley @ 2026-08-20 18:33 UTC (permalink / raw)
To: Ayush Singh
Cc: 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,
greybus-dev, linux-kernel, rust-for-linux, linux-crypto,
devicetree
[-- Attachment #1: Type: text/plain, Size: 2595 bytes --]
On Thu, Aug 20, 2026 at 02:55:49PM +0530, Ayush Singh wrote:
> 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.
>
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
> .../beagle/beagle,beagleconnect-freedom.yaml | 30 ++++++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 31 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..5a246f81261e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml
> @@ -0,0 +1,30 @@
> +# 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
These compatible only devices always look suspect to me, I feel like
these things probably at least have an optional regulator if the power
source for the freedom is provided by the device running linux.
You're also short the required properties for dealing with anything
other than uart, so spi or i2c. (I assume USB doesn't need devicetree to
function).
pw-bot: changes-requested
Thanks,
Conor.
> +
> +required:
> + - compatible
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + serial {
> + mcu {
> + compatible = "beagle,beagleconnect-freedom";
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 037a87b74800..49c6dac72748 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11338,6 +11338,7 @@ GREYBUS UART NODE DRIVERS
> M: Ayush Singh <ayush@beagleboard.com>
> 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
>
> --
> 2.55.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread