Re: [PATCH v2 7/7] greybus: Add Rust UART node driver

From: Gary Guo

Date: Sun Sep 06 2026 - 15:02:50 EST

On Sun Sep 6, 2026 at 6:39 PM BST, Markus Probst wrote:
> On Fri, 2026-09-04 at 10:48 +0530, Ayush Singh wrote:
>> On 9/4/26 2:19 AM, Markus Probst wrote:
>>
>> > On Thu, 2026-08-27 at 13:24 +0530, Ayush Singh wrote:
>> > > 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@xxxxxxxxxx/
>> > >
>> > > Signed-off-by: Ayush Singh <ayush@xxxxxxxxxxxxxxx>
>> > > ---
>> > > MAINTAINERS | 1 +
>> > > drivers/greybus/.gitignore | 1 +
>> > > drivers/greybus/Kconfig | 15 +++
>> > > drivers/greybus/Makefile | 48 ++++++++
>> > > drivers/greybus/gb_uart_node.rs | 245 ++++++++++++++++++++++++++++++++++++++++
>> > > 5 files changed, 310 insertions(+)
>> > >
>> > > diff --git a/MAINTAINERS b/MAINTAINERS
>> > > index d047090be5f4..49c6dac72748 100644
>> > > --- a/MAINTAINERS
>> > > +++ b/MAINTAINERS
>> > > @@ -11339,6 +11339,7 @@ M: Ayush Singh <ayush@xxxxxxxxxxxxxxx>
>> > > L: greybus-dev@xxxxxxxxxxxxxxxx (moderated for non-subscribers)
>> > > S: Maintained
>> > > F: Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml
>> > > +F: drivers/greybus/gb_uart_node.rs
>> > >
>> > > GREYBUS SUBSYSTEM
>> > > M: Johan Hovold <johan@xxxxxxxxxx>
>> > > 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 381d1a6ee135..34de913af287 100644
>> > > --- a/drivers/greybus/Kconfig
>> > > +++ b/drivers/greybus/Kconfig
>> > > @@ -60,5 +60,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..3eb4f8ab3655
>> > > --- /dev/null
>> > > +++ b/drivers/greybus/gb_uart_node.rs
>> > > @@ -0,0 +1,245 @@
>> > > +// SPDX-License-Identifier: GPL-2.0
>> > > +
>> > > +//! Greybus UART Node driver
>> > > +
>> > > +use kernel::{
>> > > + alloc::Flags,
>> > > + crc_ccitt::crc_ccitt,
>> > > + device::{
>> > > + AsBusDevice,
>> > > + Bound,
>> > > + Core, //
>> > > + },
>> > > + error::code,
>> > > + new_spinlock, of,
>> > > + prelude::*,
>> > > + serdev,
>> > > + sync::{
>> > > + aref::ARef,
>> > > + Arc,
>> > > + SpinLock, //
>> > > + },
>> > > +};
>> > > +
>> > > +use zerocopy::little_endian;
>> > > +use zerocopy_derive::{FromBytes, Immutable, KnownLayout};
>> > > +
>> > > +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::<u16>()];
>> > > +
>> > > + match addr {
>> > > + ADDRESS_GREYBUS => {
>> > > + let frame = GreybusFrame::ref_from_bytes(payload).map_err(|_| code::EINVAL)?;
>> > > + self.node.submit_message(0, frame.cport.into(), &frame.msg)
>> > > + }
>> > > + _ => Err(code::EINVAL),
>> > > + }
>> > > + }
>> > > +
>> > > + fn rx(&mut self, data: &[u8]) -> usize {
>> > > + for i in data.iter() {
>> > > + match *i {
>> > > + HDLC_FRAME => {
>> > > + if !self.rx_buf.is_empty() {
>> > > + if let Err(e) = self.frame_finish() {
>> > > + dev_warn!(self.sdev.as_ref(), "bad frame: {e:?}\n");
>> > > + }
>> > > + }
>> > > +
>> > > + self.rx_buf.clear();
>> > > + self.rx_in_esc = false;
>> > > + }
>> > > + 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_within_capacity(c).is_err() {
>> > > + dev_warn!(self.sdev.as_ref(), "buffer overflow. Dropping frame");
>> > > +
>> > > + self.rx_buf.clear();
>> > > + self.rx_in_esc = false;
>> > > + }
>> > > + }
>> > > + }
>> > > + }
>> > > +
>> > > + data.len()
>> > > + }
>> > > +}
>> > > +
>> > > +struct GbNode {
>> > > + sdev: ARef<serdev::Device>,
>> > > +}
>> > > +
>> > > +impl GbNode {
>> > > + const fn new(sdev: ARef<serdev::Device>) -> Self {
>> > > + Self { sdev }
>> > > + }
>> > > +
>> > > + fn fill_buf(mut crc: u16, data: &[u8], buf: &mut KVec<u8>) -> Result<u16> {
>> > > + for i in data {
>> > > + crc = crc_ccitt(crc, &[*i]);
>> > > + if *i == HDLC_ESC || *i == HDLC_FRAME {
>> > > + buf.push_within_capacity(HDLC_ESC)?;
>> > > + buf.push_within_capacity(i ^ HDLC_XOR)?;
>> > > + } else {
>> > > + buf.push_within_capacity(*i)?;
>> > > + }
>> > > + }
>> > > +
>> > > + Ok(crc)
>> > > + }
>> > > +}
>> > > +
>> > > +impl gb_softsvc::InterfaceOps for GbNode {
>> > > + fn write(&self, data: &[u8], cport: u16, gfp_mask: Flags) -> Result<()> {
>> > > + // SAFETY: `GbNode` only exists while its serdev driver is bound, so the device is in the
>> > > + // `Bound` state for the duration of this call.
>> > > + let bound: &serdev::Device<Bound> =
>> > > + unsafe { serdev::Device::from_device(self.sdev.as_ref().as_bound()) };
>> > > +
>> > > + let mut buf = KVec::with_capacity(HDLC_MAX_FRAME_LEN, gfp_mask)?;
>> > > +
>> > > + let mut crc = 0xffff;
>> > > +
>> > > + buf.push_within_capacity(HDLC_FRAME)?;
>> > > +
>> > > + crc = Self::fill_buf(crc, &[ADDRESS_GREYBUS, 0x03], &mut buf)?;
>> > > + crc = Self::fill_buf(crc, &cport.to_le_bytes(), &mut buf)?;
>> > > + crc = Self::fill_buf(crc, data, &mut buf)?;
>> > > +
>> > > + crc ^= 0xffff;
>> > > + Self::fill_buf(crc, &crc.to_le_bytes(), &mut buf)?;
>> > > +
>> > > + buf.push_within_capacity(HDLC_FRAME)?;
>> > > +
>> > > + bound.write_all(&buf, 0)?;
>> > > +
>> > > + Ok(())
>> > > + }
>> > > +}
>> > > +
>> > > +#[pin_data]
>> > > +struct GbUartNode {
>> > > + #[pin]
>> > > + rx: SpinLock<Option<HdlcRx>>,
>> > (add me to CC please)
>> >
>> > Instead of using a lock here, it might be a better idea to
>> > synchronize/stop the receive callback before unbind is called in the
>> > serdev rust abstraction. This would allow the abstraction to provide
>> > mutable references to the driver data in `receive` and `unbind`. It
>> > would also remove the Sync requirement.
>> >
>> > I will send a patch soon.
>> >
>> > Thanks
>> > - Markus Probst
>>
>> That sounds great. The lock here was basically only for getting a mut
>> ref. I will base the next version on top of your patches.
>>
>> I have added your email for the next patch version.
> It appears that "would break the driver core's lifetime design", so I
> have to drop the patch.
>
> You will still need the SpinLock.

You can still do sychronization to allow this, just not using things from
drvdata. For example, if you move this "active: Mutex<bool>" to be part of
`serdev_device`, then you don't need to access `drvdata` to set the active
field.

That'll be actually more inline with what other buses are doing, where drvdata
is exclusively for the drivers, and for the bus.

Best,
Gary