Re: [PATCH v5 7/9] 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] greybus: gb_softsvc: unsynchronized async SVC handshake in init() and drop()
- [Medium] greybus: gb_softsvc: unchecked response result in GbHdDriver::svc_handler()
- [Low] greybus: gb_softsvc: commit message describes outdated NodeOps API
--

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

drivers: greybus: Add software SVC implementation

This commit adds gb-softsvc, an in-kernel software implementation of a
Greybus Supervisory Controller (SVC) in Rust for transports that do not
have a hardware SVC on the bus.

>     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.
>     
>     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>

[Severity: Low]
As reported by Sashiko on v4 [1], does the commit message still describe an
older version of the public API?

The changelog states that host drivers implement NodeOps and call
module_insert() (to obtain allocated interface IDs) and module_remove(). In
drivers/greybus/gb_softsvc.rs, the public trait is InterfaceOps,
module_insert() and module_remove() are private methods on Svc, and drivers
use Module::new(), Module::submit_message() with a module-relative interface
index, and Drop for Module.

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

[ ... ]

> @@ -0,0 +1,533 @@
> +            (
> +                false,
> +                protocols::GB_SVC_TYPE_INTF_UNIPRO_ENABLE
> +                | protocols::GB_SVC_TYPE_INTF_UNIPRO_DISABLE,
> +            ) => self.0.intf_unipro_enable_disable(hd, hdr),
> +            (false, protocols::GB_SVC_TYPE_INTF_ACTIVATE) => self.0.intf_activate(hd, msg),
> +            (false, protocols::GB_SVC_TYPE_INTF_RESUME) => self.0.intf_resume(hd, msg),
> +            (true, protocols::GB_SVC_TYPE_PROTOCOL_VERSION) => self.0.send_svc_hello(hd)?,
> +            (true, protocols::GB_SVC_TYPE_SVC_HELLO) => {
> +                let global_state = GlobalState::new(self.0.clone(), hd.into());
> +                let _ = GLOBAL_STATE.lock().replace(global_state);
> +            }

[Severity: Medium]
Should GbHdDriver::svc_handler() verify that the response header result is
GB_OP_SUCCESS before advancing the handshake and populating GLOBAL_STATE?

In drivers/greybus/operation.c, gb_operation_request_handle() always calls
gb_operation_response_send(operation, status) for non-unidirectional requests:

drivers/greybus/operation.c:gb_operation_response_send() {
    ...
    /* Fill in the response header and send it */
    operation->response->header->result = gb_operation_errno_map(errno);

    ret = gb_message_send(operation->response, GFP_KERNEL);
    ...
}

If gb_svc_version_request() or gb_svc_hello() fails in drivers/greybus/svc.c,
the core still sends a response over GB_SVC_CPORT_ID with a non-zero error
code in header->result and leaves svc->state uninitialized. Because
GbHdDriver::svc_handler() only matches on (hdr.is_response(),
hdr.request_type()), it will treat the failed response as a success and
publish an uninitialized SVC in GLOBAL_STATE.

> +            (
> +                true,
> +                protocols::GB_SVC_TYPE_MODULE_INSERTED | protocols::GB_SVC_TYPE_MODULE_REMOVED,
> +            ) => {}
> +            (false, ty) => {
> +                dev_warn!(hd.as_ref(), "unsupported request 0x{:02x}\n", ty);
> +                let rsp = Msg::response(hdr, protocols::GB_OP_PROTOCOL_BAD, ());
> +                hd.data_rcvd(protocols::GB_SVC_CPORT_ID, rsp.as_bytes());
> +            }
> +            (true, ty) => dev_warn!(hd.as_ref(), "unexpected response 0x{:02x}\n", ty),
> +        };
> +
> +        Ok(())
> +    }
> +}

[ ... ]

> +impl kernel::Module for GreybusSoftSvc {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        pr_info!("gb_softsvc (init)\n");
> +
> +        // SAFETY: This runs once at module init, before anything else can reach `GLOBAL_STATE`.
> +        unsafe { GLOBAL_STATE.init() };
> +
> +        let faux = faux::Registration::new(c_str!("gb-softsvc"), None)?;
> +        let svc = Arc::pin_init(Svc::new(), GFP_KERNEL)?;
> +        let data = GbHdDriver(svc.clone());
> +
> +        let dev = faux.as_ref().as_ref();
> +        let hd = hd::Registration::new(dev, GB_OPERATION_SIZE_MAX, CPORT_ID_MAX + 1, Ok(data))?;
> +
> +        svc.send_version(hd.as_ref())?;
> +
> +        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");
> +    }
> +}

[Severity: High]
Because the SVC handshake runs asynchronously on svc->connection->wq, can
GreybusSoftSvc::init() and GreybusSoftSvc::drop() race with the handshake work
items?

In GreybusSoftSvc::init(), svc.send_version() calls hd.data_rcvd() ->
greybus_data_rcvd() -> gb_connection_recv_request(), which only queues the
incoming GB_SVC_TYPE_PROTOCOL_VERSION request onto svc->connection->wq.
GreybusSoftSvc::init() returns before that work item and the subsequent
GB_SVC_TYPE_SVC_HELLO request complete and populate GLOBAL_STATE in
GbHdDriver::svc_handler(), so a consumer calling Module::new() right after
module init will see GLOBAL_STATE as None and fail with EAGAIN:

    pub fn new(intfs: &[Arc<dyn InterfaceOps>]) -> Result<Self> {
        ...
        let guard = GLOBAL_STATE.lock();
        let state = guard.as_ref().ok_or(code::EAGAIN)?;

In addition, when GreybusSoftSvc is dropped, GreybusSoftSvc::drop() runs
GLOBAL_STATE.lock().take() before its _hd field is dropped via
Registration::drop() -> gb_hd_del() -> gb_svc_del() ->
gb_connection_disable_rx(). If the asynchronous GB_SVC_TYPE_SVC_HELLO response
is processed on svc->connection->wq after GreybusSoftSvc::drop() clears
GLOBAL_STATE but before _hd is unregistered, GbHdDriver::svc_handler() will
reinstall GlobalState into GLOBAL_STATE and leak the Arc<Svc> and
ARef<hd::Device> references.

[1] https://lore.kernel.org/r/20261021-gb-uart-transport-v4-4-c308ce821d45@beagleboard.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-gb-uart-transport-v5-0-f8e326d6561f@beagleboard.org?part=7