Re: [PATCH v3 5/7] drivers: greybus: Add software SVC implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] locking: dropping trait object inside SpinLock causes atomic context violation
- [Medium] gb_softsvc: operation_id leak when core drops SVC requests
- [Low] build: missing Make rule for exported header breaks compilation
--

commit 004105b6fb260c2da6da56325f81d73a5407f5e0
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. 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.

[ ... ]

> diff --git a/drivers/greybus/gb_softsvc.rs b/drivers/greybus/gb_softsvc.rs
> new file mode 100644
> index 0000000000000..144e2364aa59c
> --- /dev/null
> +++ b/drivers/greybus/gb_softsvc.rs

[ ... ]

> +    fn map_remove(&self, primary_id: u8, intf_count: u8, dev: &kernel::device::Device) {
> +        let id = match primary_id.checked_sub(INTF_ID_START) {
> +            Some(x) => x,
> +            None => {
> +                dev_warn!(dev, "Invalid interace id");
> +                return;
> +            }
> +        };
> +        let mut guard = self.intf_map.lock();
> +
> +        for i in 0..intf_count {
> +            let _ = guard[usize::from(id + i)].take();

[Severity: High]
Does taking the item out of the SpinLock guard here drop the trait object
while atomic context is still active?

If this is the last reference to the Arc, the Drop implementation of the
underlying dyn InterfaceOps (provided by external host drivers) will be
executed synchronously. If the driver's Drop routine performs any sleepable
operations (like taking a mutex or flushing a workqueue to cleanly tear down
the transport), could this trigger a 'BUG: scheduling while atomic' kernel
panic?

Should the items be moved out of the lock and dropped after the guard is
released?

> +        }
> +    }

[ ... ]

> +    fn send_request<P: AsBytes>(&self, hd: &hd::Device, ty: u8, payload: P) -> Result<()> {
> +        let msg = Msg::request(self.new_operation_id()?, ty, payload);
> +        hd.data_rcvd(protocols::GB_SVC_CPORT_ID, msg.as_bytes());
> +        Ok(())
> +    }

[Severity: Medium]
Is there a mechanism to prevent operation_id leaks if the core drops SVC
requests?

When gb_softsvc sends a request via hd.data_rcvd(), the greybus core might
drop the request early (for instance, if the GFP_ATOMIC memory allocation
fails in gb_operation_create_incoming() or gb_operation_get_active() fails
in gb_connection_recv_request() in drivers/greybus/operation.c).

If the core silently drops the message without calling
gb_operation_request_handle() or sending a response back to the SVC,
could the svc_operation_id IdPool leak IDs until find_unused_id() fails
with EOVERFLOW, preventing the soft SVC from sending further requests?

[ ... ]

> diff --git a/drivers/greybus/gb_softsvc_exports.c b/drivers/greybus/gb_softsvc_exports.c
> new file mode 100644
> index 0000000000000..a0e360c4d137f
> --- /dev/null
> +++ b/drivers/greybus/gb_softsvc_exports.c
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright (c) 2026 BeagleBoard.org. All rights reserved.
> +
> +/*
> + * Exports Rust symbols from the `gb_softsvc` crate for use by dependent modules.
> + *
> + * This is a workaround until the build system supports Rust cross-module
> + * dependencies natively.
> + */
> +
> +#include <linux/export.h>
> +
> +#define EXPORT_SYMBOL_RUST_GPL(sym) extern int sym; EXPORT_SYMBOL_GPL(sym)
> +
> +#include "exports_gb_softsvc_generated.h"

[Severity: Low]
Does the Makefile lack a target rule to generate exports_gb_softsvc_generated.h?

Compiling the kernel with CONFIG_GREYBUS_SOFTSVC enabled seems to
unconditionally fail at build time because the generated header is included
here but never built.

If the generation rule is introduced later in the series, will this break
bisectability?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-gb-uart-transport-v3-0-81d0a8a1acf7@beagleboard.org?part=5