Add Greybus Sotfsvc and UART Node drivers
Subsystems (6)
v5 cover Add Greybus Sotfsvc and UART Node drivers
A Greybus network needs 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. On transports that merely carry Greybus messages - a UART, an I2C bus, a network link - nothing on the wire plays that role, so the SVC has to be emulated
somewhere: in a user-space bridge (gbridge), in coprocessor firmware (cc1352p7 in gb-beagleplay), or open-coded inside the host driver itself.
This series moves the emulation into the kernel and makes it shared. This was discussed in a prior RFC as well [2], but to keep things short, removing the need for an external SVC greatly simplifies the greybus network setup when nodes are directly connected over common transports such as UART, I2C etc.
gb-uart-node gb-softsvc greybus core
(transport, serdev) <-> (SVC + host device) <-> (bundles, protocols)
NodeOps module_insert()
submit_message() module_remove()
gb-softsvc registers itself as a Greybus host device and answers the SVC-side operations the core expects during interface bring-up and teardown. A host driver implements the NodeOps trait to push data towards its node, calls module_insert() to announce a new node, submit_message() to hand incoming Greybus messages back to the core, and module_remove() on disconnect.
The first user is gb-uart-node, a driver for Greybus nodes attached over a plain serial port. Framing is HDLC, with a one-byte address (0x01 for Greybus) and control byte, followed by the 16-bit CPort ID and the Greybus message. No SVC firmware is required on the far end, so the node can be a bare microcontroller speaking Greybus - a BeagleConnect Freedom over its serial link, in this case. The testing is performed with greybus-zephyr [0] implementation.
Both drivers are written in Rust, which is why the middle of the series is abstractions rather than drivers. Only the APIs these two drivers need are covered: protocols.rs abstracts the SVC-facing parts of greybus_protocols.h, and types such as Greybus Interface do not implement AlwaysRefCounted yet. The intent is to grow this as more Rust host drivers appear rather than to abstract the whole subsystem up front.
Patches 1 and 2 are small C-side preparations to the Greybus core:
exporting gb_connection_get()/gb_connection_put() and adding gb_connection_hd_find_by_intf(), a lookup by remote interface and CPort id for callers that only know the far end of a connection. Patch 3 adds a CRC-CCITT abstraction, needed for HDLC frame checks. Patch 4 adds the Greybus abstractions, patches 5 and 6 the two drivers, and patch 7 the device tree binding for BeagleConnect Freedom.
Open questions
***************
- gb-uart-node imports types from gb-softsvc, so the series carries the Rust-to-Rust cross-module calling setup from nova-core [1]:
gb_softsvc_exports.c plus the Makefile plumbing that emits crate metadata and generates the export list. This is a workaround for the build system not supporting Rust cross-module dependencies natively, and it should go away once that lands.
- Connection create/destroy and interface activate/resume in gb-softsvc currently just acknowledge the request. Callbacks into NodeOps can be added when a transport actually needs to act on them; I did not want to invent an interface without a user.
- Zerocopy is currently not being used in grebeybus/protocols.rs. They cannot be derived yet since types generated by bindgen do not have them, and it seems explicitly forbindden to manually impl the traits. So using old traits from transmute.
- The bindings are supposed to be created for actual device, but any MCU that supports Zephyr, can run the greybus-zephyr firmware with UART transport. So not sure if adding a beagle,beagleconnect-freedom compatible is the correct choice here.
- The individual patches can be spun off into independent patch series if required. The reason for this single patch series is to provide a complete picture of usage.
- Since gb-softsvc currently is not being used from a C driver, no C API is provided. However, if required, it can be added.
- Writing to UART from gb-uart-node is currently a bit broken. I am not quite sure what the safe way is to go from a non-bound device to a bound device. Any suggestions on this front are welcome.
- I am not sure if Rust abstractions should have a seperate entry in MAINTAINERS with me as the maintainer, or if they should just be added to the respective subsystem entries. As mentioned by Miguel Ojeda [3], this is upto greybus maintainers. So waiting on their inputs. I am available to be maintainer for the Rust abstractions for greybus if required.
[0]: https://github.com/beagleboard/greybus-zephyr
[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/
[3]: https://lore.kernel.org/all/CANiq72mA6pTmPJUk8g7y3Z=wetFMXdzHLiPEPChpY4OTg3OHiw@mail.gmail.com/
---
Changes in v5:
- Remove AlwaysRefCounted impl from Interface. No longer required.
- Fix invalid comments.
- Fix interface drop in spinlock.
- Impl AlwaysRefCounted for hd::Device only in normal context.
- Match the bus device struct defination in device.rs docs.
- Return GB_OP_PROTOCOL_BAD instead of ENOTSUP when encountering an
unknown message.
- Fix possible race condition in SVC module removed. Flagged by
sashiko-bot. Involved, ensuring that operation_cancel cannot drop an
in-flight message.
- Fix timer leak in a greybus operation.
- Check write_all return.
- Return E2BIG when frame lenght is exceeded in uart node driver.
- Link to v4: https://lore.kernel.org/r/[email protected]
Changes in v4:
- Remove gb_uard_node::GbUartNode::unbind. Suggested by Markus Probst,
since write is still valid after unbind.
- Add inline attribute where suggested.
- Remove patch 1. No longer required with modifications to the old patch
2.
- Increse hdlc max buffer size to 512 bytes.
- Send module_removed request before removing from map.
- Also do not hold lock across interface drop.
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- Fix incorrect email in MAINTAINERS.
- Make CONFIG_RUST_GREYBUS_ABSTRACTIONS depend on sysfs like
CONFIG_GREYBUS.
- Fix cleanup in hd::Registration::new in case of errors.
- Fix comments.
- Fix Msg AsBytes impl as suggested by sashiko-bot.
- Use SpinLock for intf_map. This should be fine since we need locking
only for getting the interface. We do not hold locks across operations.
- Properly drop message in case of buffer overflow in gb_uart_node.
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Fix possible null pointer dereference in
gb_connection_hd_find_by_intf. Flagged by sashiko-bot.
- Remove prompt CONFIG_RUST_CRC_CCITT_ABSTRACTIONS Kconfig symbol.
- Add CONFIG_RUST_GREYBUS_ABSTRACTIONS to ensure that greybus is
built-in for rust abstractions to work.
- Use gfp_mask in message_send callback.
- Only provide a Message reference in message_cancel callback.
- Add invariant comment for Registration.
- Call Registration->add directly in new, before constructing
Registration.
- Add Send and Sync bounds to T in Registration<T>
- Set endo_id as 0. Is not used anywhere.
- Fix module_insert intf_count. Was hardcoded to 1 by mistake. Flagged by sashiko-bot.
- Check for empty intfs slice in Module::new.
- Remove module in serdev::Driver::unbind instead of on drop.
- Make gb_uart_node write atomic. Using temp buffer to build frame.
- Fix import style.
- Reorder beagleconnect-freedom dtbinding patch to be before gb_uart_node.
- Add vbat-supply and reg properties to beagleconnect-freedom dtbinding.
- Reference spi-peripheral-props in beagleconnect-freedom dtbinding.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Ayush Singh (9):
greybus: operation: keep request active across message_send
greybus: svc: add gb_svc_flush()
greybus: operation: arm request timeout before sending
greybus: connection: Add greybus_data_rcvd_by_intf()
rust: crc_ccitt: add CRC-CCITT abstraction
rust: kernel: Add greybus abstractions
drivers: greybus: Add software SVC implementation
dt-bindings: beagle: Add BeagleConnect Freedom
greybus: Add Rust UART node driver
.../beagle/beagle,beagleconnect-freedom.yaml | 62 +++ MAINTAINERS | 15 + drivers/greybus/.gitignore | 1 + drivers/greybus/Kconfig | 35 ++ drivers/greybus/Makefile | 50 ++ drivers/greybus/connection.c | 47 ++ drivers/greybus/gb_softsvc.rs | 533 +++++++++++++++++++++ drivers/greybus/gb_softsvc_exports.c | 15 + drivers/greybus/gb_uart_node.rs | 270 +++++++++++ drivers/greybus/operation.c | 29 +- drivers/greybus/svc.c | 14 + include/linux/greybus/connection.h | 2 + include/linux/greybus/svc.h | 1 + lib/crc/Kconfig | 7 + rust/bindings/bindings_helper.h | 2 + rust/kernel/alloc.rs | 6 + rust/kernel/crc_ccitt.rs | 26 + rust/kernel/greybus/hd.rs | 391 +++++++++++++++ rust/kernel/greybus/mod.rs | 200 ++++++++ rust/kernel/greybus/protocols.rs | 445 +++++++++++++++++ rust/kernel/lib.rs | 4 + 21 files changed, 2152 insertions(+), 3 deletions(-)
---
base-commit: becb95e45ef0dda9ffb35156d909d3da7a2789c3
change-id: 20260810-gb-uart-transport-9255d6557c4c
Best regards,
Signature · 1 line
Ayush Singh <[email protected]>
PATCH v5 1/9 greybus: operation: keep request active across message_send
gb_operation_request_send() takes a single active count, and completion drops it. This would be fine normally, but in case of cancellation (in gb_softsvc context, this would happen in module_removed), completion (gb_operation_work), can drop the active count, while message_send is running.
The core does call message_cancel before dropping the active count, but in most normal circumstances, cancelling an operation that is already writing over a protocol (UART) is not really possible. So we are only left with making the core wait for the said write to complete instead of making active count 0 while message_send is in progress.
The patch takes a second reference and active count before the send and drop them once it returns. If the connection starts tearing down between the two calls, return 0 and let the cancel complete the operation.
Since it took at least me a while to truely understand what the initial sashiko-bot report meant, here is an example:
Thread A Thread B + worker active
------------------------ ------------------------------ ------
1 get_active() 1
2 gb_message_send()
3 GbNode::write() ...
4 ... blocked state = DISCONNECTING
5 ... blocked gb_operation_cancel()
6 ... blocked result_set(-ESHUTDOWN)
7 ... blocked queue_work(completion)
8 ... blocked wait_event(!active) [sleeps]
9 ... blocked [worker] gb_operation_work()
10 ... blocked [worker] put_active() [:292] 0
11 ... blocked wait_event wakes, returns <- too early
12 ... blocked gb_module_del() returns
13 ... blocked [softsvc] Module::drop finishes
14 ... blocked serdev port closed, unbind done <- bad
15 write() returns16 return to caller
--- drivers/greybus/operation.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
drivers/greybus/operation.c +18 -0
@@ -766,7 +766,25 @@ int gb_operation_request_send(struct gb_operation *operation,if (ret) goto err_put; +/* +* Completion can drop the reference and active count above while the +* host driver is still sending. Hold another of each across the send, so +* that cancelling the operation waits for the host driver to return. +*/ +gb_operation_get (operation); + ret =gb_operation_get_active (operation); +if (ret) { +/* +* The connection is being torn down, and the operation, which is +* already on its list, will be cancelled and completed. +*/ +gb_operation_put (operation); +return 0 ; + } + ret =gb_message_send (operation->request, gfp); +gb_operation_put_active (operation); +gb_operation_put (operation);if (ret) goto err_put_active;
Signature · 1 line
2.56.0
[email protected]
AI reviewRe: [PATCH v5 1/9] greybus: operation: keep request active across message_send
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] greybus: operation: double put in gb_operation_request_send() on send error - [Critical]…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] greybus: operation: double put in gb_operation_request_send() on send error
- [Critical] greybus: operation: race between gb_operation_get_active() calls on send --
commit 1f5c8fb4470157e5c81c1041163bd49efb2ea63b
Author: Ayush Singh <[email protected]>
greybus: operation: keep request active across message_send
This commit takes an extra reference and active count on the Greybus operation across gb_message_send() in gb_operation_request_send() so that operation cancellation waits for the host driver's send path to return.
Quoted 13 lines
drivers/greybus/operation.c +5
-0@@ -766,7 +766,25 @@ int gb_operation_request_send(struct gb_operation *operation,if (ret) goto err_put; +/* +* Completion can drop the reference and active count above while the +* host driver is still sending. Hold another of each across the send, so +* that cancelling the operation waits for the host driver to return. +*/
drivers/greybus/operation.c int gb_operation_request_send(struct gb_operation *operation, · line 783
Quoted 10 lines
drivers/greybus/operation.c +10
-0+gb_operation_get (operation); + ret =gb_operation_get_active (operation); +if (ret) { +/* +* The connection is being torn down, and the operation, which is +* already on its list, will be cancelled and completed. +*/ +gb_operation_put (operation); +return 0 ; + }
[Severity: Critical]
Because the first gb_operation_get_active(operation) call adds operation->links to connection->operations with operation->active == 1 and then drops connection->lock before the second reference and active count are acquired, can a concurrent teardown race in the window between the two gb_operation_get_active() calls?
1. If gb_connection_disable() or gb_connection_disable_forced() runs right after the first gb_operation_get_active() unlocks connection->lock, gb_connection_cancel_operations() -> gb_operation_cancel() queues gb_operation_work(). If gb_operation_work() runs before this thread reaches the second gb_operation_get(operation), it invokes operation->callback(), decrements operation->active to 0 (waking gb_operation_cancel() before gb_message_send() has even started), and drops the completion reference. For asynchronous callers whose callback drops the caller's own reference (such as gb_loopback_async_operation_callback() in drivers/staging/greybus/loopback.c), operation->kref can reach 0 and free operation before the second gb_operation_get() and gb_operation_get_active() touch it.
2. For GB_OPERATION_FLAG_CORE operations, gb_operation_get_active() permits operations when connection->state is GB_CONNECTION_STATE_DISCONNECTING:
drivers/greybus/operation.c:gb_operation_get_active() {
...
case GB_CONNECTION_STATE_DISCONNECTING:
if (!gb_operation_is_core(operation))
goto err_unlock;
break;
...
}If a core operation passes the first gb_operation_get_active() in GB_CONNECTION_STATE_ENABLED, and gb_connection_disable() transitions to GB_CONNECTION_STATE_DISCONNECTING and cancels/completes the operation in the window before the second gb_operation_get_active(), won't the second gb_operation_get_active() still succeed in DISCONNECTING, re-add the already completed operation to connection->operations, and send it?
Conversely, in drivers/greybus/connection.c, gb_connection_disable() and _gb_connection_enable() call gb_connection_cancel_operations() when entering GB_CONNECTION_STATE_DISCONNECTING, but not when subsequently setting connection->state = GB_CONNECTION_STATE_DISABLED:
drivers/greybus/connection.c:gb_connection_disable() {
...
spin_lock_irq(&connection->lock);
connection->state = GB_CONNECTION_STATE_DISCONNECTING;
gb_connection_cancel_operations(connection, -ESHUTDOWN);
spin_unlock_irq(&connection->lock);
...
connection->state = GB_CONNECTION_STATE_DISABLED;
}If a core operation passes the first gb_operation_get_active() while in GB_CONNECTION_STATE_DISCONNECTING and connection->state transitions to GB_CONNECTION_STATE_DISABLED before the second gb_operation_get_active(), the second call returns -ENOTCONN and this path returns 0 without dropping the first active count or reference and without anyone cancelling the operation, leaking operation and hanging synchronous callers waiting on operation->completion.
drivers/greybus/operation.c int gb_operation_request_send(struct gb_operation *operation, · line 789
Quoted 6 lines
drivers/greybus/operation.c +3
-0+ ret =gb_message_send (operation->request, gfp); +gb_operation_put_active (operation); +gb_operation_put (operation);if (ret) goto err_put_active;
[Severity: Critical]
What happens if gb_operation_cancel() cancels the operation while gb_message_send() is in progress, and gb_message_send() then returns a non-zero error code?
While gb_message_send() is running, operation is on connection->operations with operation->errno set to -EINPROGRESS. If a concurrent teardown calls gb_connection_cancel_operations() -> gb_operation_cancel(operation, -ESHUTDOWN):
drivers/greybus/operation.c:gb_operation_cancel() {
...
if (gb_operation_result_set(operation, errno)) {
gb_message_cancel(operation->request);
queue_work(gb_operation_completion_wq, &operation->work);
}
...
}gb_operation_work() will run operation->callback(operation) and call gb_operation_put_active(operation) and gb_operation_put(operation), consuming the initial completion active count and reference.
If gb_message_send() also returns an error (for example, on disconnect or TX failure), gb_operation_request_send() drops the send active count and reference here, and then jumps to err_put_active:
ret = gb_message_send(operation->request, gfp); gb_operation_put_active(operation); gb_operation_put(operation); if (ret) goto err_put_active; ... err_put_active: gb_operation_put_active(operation); err_put: gb_operation_put(operation);
Wouldn't jumping to err_put_active drop the completion active count and operation->kref a second time (three puts for two gets), underflowing operation->active to -1, underflowing operation->kref into a use-after-free, and running both the asynchronous completion callback and the synchronous error return path?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v5 2/9 greybus: svc: add gb_svc_flush()
In reply to Ayush SinghA host driver that is its own SVC, such as a software SVC, injects requests like MODULE_REMOVED through greybus_data_rcvd(). The core acts on them asynchronously: the request handler runs on the connection workqueue and queues the actual work on svc->wq. The response is sent before that work runs, so it cannot tell the driver that the module is gone.
Add gb_svc_flush() to wait until every request received so far has been fully processed. It flushes the connection workqueue, then svc->wq.
--- drivers/greybus/svc.c | 14 ++++++++++++++ include/linux/greybus/svc.h | 1 + 2 files changed, 15 insertions(+)
drivers/greybus/svc.c +14 -0
@@ -1381,6 +1381,20 @@ static void gb_svc_remove_modules(struct gb_svc *svc) } } +/** +* gb_svc_flush - wait for received SVC requests to be processed +* @svc: the SVC +* +* Requests pass through the connection workqueue on their way to svc->wq, so +* both have to be flushed, in this order. +*/ +void gb_svc_flush (struct gb_svc *svc) +{ +flush_workqueue (svc->connection->wq); +flush_workqueue (svc->wq); +} +EXPORT_SYMBOL_GPL (gb_svc_flush); +void gb_svc_del (struct gb_svc *svc) {gb_connection_disable_rx (svc->connection);
include/linux/greybus/svc.h +1 -0
@@ -63,6 +63,7 @@ struct gb_svc *gb_svc_create(struct gb_host_device *hd);int gb_svc_add (struct gb_svc *svc);void gb_svc_del (struct gb_svc *svc);void gb_svc_put (struct gb_svc *svc); +void gb_svc_flush (struct gb_svc *svc);int gb_svc_pwrmon_intf_sample_get (struct gb_svc *svc,u8 intf_id,u8 measurement_type,u32 *value);
Signature · 1 line
2.56.0
PATCH v5 3/9 greybus: operation: arm request timeout before sending
In reply to Ayush SinghThe host driver can complete a request before add_timer is called. The main case for this is softsvc, although, depending on which transport is used, there is a possibility that response arrives much faster than expected.
By populating timer earilier, we prevent the starting a timer on already completed request (which would then leak an active timer on cleanup).
--- drivers/greybus/operation.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
drivers/greybus/operation.c +10 -5
@@ -782,20 +782,25 @@ int gb_operation_request_send(struct gb_operation *operation,return 0 ; } +/* +* Arm the timeout before sending: the response can arrive, and the +* operation complete, before the host driver returns. +*/ +if (timeout) { + operation->timer.expires = jiffies +msecs_to_jiffies (timeout); +add_timer (&operation->timer); + } + ret =gb_message_send (operation->request, gfp);gb_operation_put_active (operation);gb_operation_put (operation);if (ret) goto err_put_active; -if (timeout) { - operation->timer.expires = jiffies +msecs_to_jiffies (timeout); -add_timer (&operation->timer); - } -return 0 ; err_put_active: +timer_delete_sync (&operation->timer);gb_operation_put_active (operation); err_put:gb_operation_put (operation);
Signature · 1 line
2.56.0
[email protected]
AI reviewRe: [PATCH v5 3/9] greybus: operation: arm request timeout before sending
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] greybus: operation: UAF and ref underflow in gb_operation_request_send() - [Critical] greybus…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] greybus: operation: UAF and ref underflow in gb_operation_request_send()
- [Critical] greybus: operation: UAF from early timeout in gb_operation_request_send()
- [Critical] greybus: operation: timer leak in gb_operation_request_send() on cancel --
commit 7b73a81d11f2d699f2c8ebeafd18ae6352191433
Author: Ayush Singh <[email protected]>
greybus: operation: arm request timeout before sending
This commit arms the operation timeout timer before calling gb_message_send() in gb_operation_request_send() so that a fast response completing before gb_message_send() returns does not leave an armed timer behind, and calls timer_delete_sync() in the err_put_active error path.
Quoted 6 lines
drivers/greybus/operation.c +0
-0@@ -782,20 +782,25 @@ int gb_operation_request_send(struct gb_operation *operation,return 0 ;
drivers/greybus/operation.c int gb_operation_request_send(struct gb_operation *operation, · line 792
Quoted 10 lines
drivers/greybus/operation.c +8
-0} +/* +* Arm the timeout before sending: the response can arrive, and the +* operation complete, before the host driver returns. +*/ +if (timeout) { + operation->timer.expires = jiffies +msecs_to_jiffies (timeout); +add_timer (&operation->timer); + }
[Severity: Critical]
Could an operation still be cancelled between gb_operation_get_active() and add_timer(&operation->timer) in gb_operation_request_send(), leaking an armed timer in operation->timer?
Earlier in gb_operation_request_send(), operation->errno is initialized to -EINPROGRESS and the first gb_operation_get_active() adds operation->links to connection->operations.
If a concurrent thread disables the connection via gb_connection_disable() or gb_connection_disable_forced() after the second gb_operation_get_active() succeeds and before add_timer(&operation->timer) runs:
CPU0 CPU1gb_operation_request_send()
gb_operation_result_set(-EINPROGRESS)
gb_operation_get_active()
gb_operation_get_active()
gb_connection_cancel_operations()
gb_operation_cancel(-ESHUTDOWN)
gb_operation_result_set(-ESHUTDOWN)
queue_work(&operation->work)
gb_operation_work()
timer_delete_sync(&operation->timer)
operation->callback(operation)
gb_operation_put_active(operation)
gb_operation_put(operation) add_timer(&operation->timer)
gb_message_send() -> returns 0
Because gb_operation_work() runs timer_delete_sync(&operation->timer) before CPU0 calls add_timer(&operation->timer), nothing deletes operation->timer afterward. When the caller later drops its reference via gb_operation_put(), _gb_operation_destroy() frees struct gb_operation while operation->timer is still queued in the timer wheel.
drivers/greybus/operation.c int gb_operation_request_send(struct gb_operation *operation, · line 794
Quoted 2 lines
drivers/greybus/operation.c +1
-0+ ret =gb_message_send (operation->request, gfp);
[Severity: Critical]
What happens if operation->timer expires after add_timer(&operation->timer) is called, but before the host driver in gb_message_send() has submitted its transport request?
For an asynchronous host driver such as drivers/greybus/es2.c, if the sending thread is delayed or preempted before message_send() assigns message->hcpriv and calls usb_submit_urb():
drivers/greybus/es2.c:message_send() {
...
spin_lock_irqsave(&es2->cport_out_urb_lock, flags);
message->hcpriv = urb;
spin_unlock_irqrestore(&es2->cport_out_urb_lock, flags);
...
retval = usb_submit_urb(urb, gfp_mask);
}If operation->timer fires first, gb_operation_timeout() sets operation->errno to -ETIMEDOUT and schedules gb_operation_work():
gb_operation_timeout()
gb_operation_result_set(operation, -ETIMEDOUT) queue_work(gb_operation_completion_wq, &operation->work)
gb_operation_work()
timer_delete_sync(&operation->timer)
gb_message_cancel(operation->request)
es2.c:message_cancel() -> no-op because message->hcpriv is not yet active
operation->callback(operation)
gb_operation_put_active(operation)
gb_operation_put(operation)
When the sending thread resumes in es2.c:message_send(), it submits the URB and returns 0 to gb_operation_request_send(). Because operation->callback() has already completed the operation, the caller can drop the final reference to operation via gb_operation_put(), freeing operation->request and operation->request->buffer in _gb_operation_destroy() while the URB is still in flight.
Quoted 7 lines
drivers/greybus/operation.c +0
-2gb_operation_put_active (operation);gb_operation_put (operation);if (ret) goto err_put_active; -if (timeout) { - operation->timer.expires = jiffies +msecs_to_jiffies (timeout);
drivers/greybus/operation.c int gb_operation_request_send(struct gb_operation *operation, · line 806
Quoted 10 lines
drivers/greybus/operation.c +1
-3-add_timer (&operation->timer); - } -return 0 ; err_put_active: +timer_delete_sync (&operation->timer);gb_operation_put_active (operation); err_put:gb_operation_put (operation);
[Severity: Critical]
Can this error path underflow operation->active and operation->kref and trigger a use-after-free on struct gb_operation when gb_message_send() fails after operation->timer has already expired (or after gb_operation_cancel() has been called)?
In gb_operation_request_send(), two pairs of kref and active references are taken before gb_message_send(): one pair for operation completion, and one pair held across gb_message_send().
With operation->timer armed before gb_message_send(), if gb_message_send() sleeps or takes longer than timeout before returning an error (for example, hdlc_tx_frames() in gb-beagleplay.c waiting on a full TX circular buffer before returning -EAGAIN, or write_all() in gb_uart_node.rs returning -EINTR), operation->timer can fire while gb_message_send() is still running:
gb_operation_timeout()
if (gb_operation_result_set(operation, -ETIMEDOUT))
queue_work(gb_operation_completion_wq, &operation->work);
gb_operation_work() then runs on gb_operation_completion_wq, invokes operation->callback(operation), and drops the completion reference pair:
gb_operation_work()
...
operation->callback(operation);
gb_operation_put_active(operation);
gb_operation_put(operation);
When gb_message_send() returns a non-zero ret, gb_operation_request_send() drops the send-guard reference pair and jumps to err_put_active:
ret = gb_message_send(operation->request, gfp);
gb_operation_put_active(operation);
gb_operation_put(operation);
if (ret)
goto err_put_active;In err_put_active, timer_delete_sync(&operation->timer) only waits for the timer callback to finish; it does not cancel the already-queued operation->work, nor does err_put_active check gb_operation_result_set() to see whether completion was already claimed.
Consequently, err_put_active drops gb_operation_put_active(operation) and gb_operation_put(operation) a third time when only two pairs were acquired, underflowing operation->active to -1 (which also causes wait_event() in gb_operation_cancel() to hang) and dropping the caller's own kref on operation (or accessing operation after it was already freed if an asynchronous callback such as gb_loopback_async_operation_callback() dropped the caller's reference before gb_operation_put() prior to err_put_active).
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH v5 4/9 greybus: connection: Add greybus_data_rcvd_by_intf()
In reply to Ayush Singhgreybus_data_rcvd() finds the receiving connection by its host-side CPort id. A host driver such as a software SVC sees only the remote end of a connection, the interface id and the interface CPort id, and cannot deliver data that way.
Add greybus_data_rcvd_by_intf(), which looks the connection up by interface id and interface CPort id and passes the data to it. It returns -EINVAL if no such connection exists. The first user is the gb-softsvc driver, added later in this series.
--- drivers/greybus/connection.c | 47 ++++++++++++++++++++++++++++++++++++++ include/linux/greybus/connection.h | 2 ++ 2 files changed, 49 insertions(+)
drivers/greybus/connection.c +47 -0
@@ -69,6 +69,32 @@ gb_connection_hd_find(struct gb_host_device *hd, u16 cport_id)return connection; } +/* +* Returns a reference-counted pointer to the connection if found. +*/ +static struct gb_connection * +gb_connection_hd_find_by_intf (struct gb_host_device *hd,u8 intf_id, +u16 intf_cport) +{ +struct gb_connection *connection; +unsigned long flags; + +spin_lock_irqsave (&gb_connections_lock, flags); +list_for_each_entry (connection, &hd->connections, hd_links) { +if (connection->intf && + connection->intf->interface_id == intf_id && + connection->intf_cport_id == intf_cport) { +gb_connection_get (connection); + goto found; + } + } + connection =NULL ; +found: +spin_unlock_irqrestore (&gb_connections_lock, flags); + +return connection; +} +/* * Callback from the host driver to let us know that data has been * received on the bundle.
@@ -91,6 +117,27 @@ void greybus_data_rcvd(struct gb_host_device *hd, u16 cport_id, }EXPORT_SYMBOL_GPL (greybus_data_rcvd); +/* +* Callback from a host driver that only knows the remote end of a connection +* (interface id and interface CPort id), such as a software SVC. +*/ +int greybus_data_rcvd_by_intf (struct gb_host_device *hd,u8 intf_id, +u16 intf_cport,u8 *data,size_t length) +{ +struct gb_connection *connection; + +trace_gb_hd_in (hd); + + connection =gb_connection_hd_find_by_intf (hd, intf_id, intf_cport); +if (!connection) +return -EINVAL ; +gb_connection_recv (connection, data, length); +gb_connection_put (connection); + +return 0 ; +} +EXPORT_SYMBOL_GPL (greybus_data_rcvd_by_intf); +static void gb_connection_kref_release (struct kref *kref) {struct gb_connection *connection;
include/linux/greybus/connection.h +2 -0
@@ -92,6 +92,8 @@ void gb_connection_mode_switch_complete(struct gb_connection *connection);void greybus_data_rcvd (struct gb_host_device *hd,u16 cport_id,u8 *data,size_t length); +int greybus_data_rcvd_by_intf (struct gb_host_device *hd,u8 intf_id, +u16 intf_cport,u8 *data,size_t length);void gb_connection_latency_tag_enable (struct gb_connection *connection);void gb_connection_latency_tag_disable (struct gb_connection *connection);
Signature · 1 line
2.56.0
PATCH v5 5/9 rust: crc_ccitt: add CRC-CCITT abstraction
In reply to Ayush SinghThe 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.
--- 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(+)
MAINTAINERS +1 -0
@@ -7036,6 +7036,7 @@ T: git https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git crc-ne F: Documentation/staging/crc* F: include/linux/crc* F: lib/crc/ +F: rust/kernel/crc* F: scripts/gen-crc-consts.py CREATIVE SB0540
lib/crc/Kconfig +7 -0
@@ -32,6 +32,13 @@ config CRC_CCITT The CRC-CCITT library functions. Select this if your module uses any of the functions from <linux/crc-ccitt.h>. +config RUST_CRC_CCITT_ABSTRACTIONS + bool + depends on RUST + select CRC_CCITT + help + This enables the Rust abstraction for the CRC-CCITT API. + config CRC_ITU_T tristate help
rust/bindings/bindings_helper.h +1 -0
@@ -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>
rust/kernel/crc_ccitt.rs new file +26 -0
@@ -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 ()) } +}
rust/kernel/lib.rs +2 -0
@@ -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;
Signature · 1 line
2.56.0
PATCH v5 6/9 rust: kernel: Add greybus abstractions
In reply to Ayush SinghAdd Rust abstractions for the Greybus core, enough to implement a Greybus host driver in Rust.
The abstractions are split into 3 modules:
- Top level mod.rs: Basic abstractions greybus structures.
- hd.rs: Greybus Host Device specific abstractions.
- protocols.rs: Abstractions for greybus_protocols.h, i.e. greybus protocol types and constants.
Only the APIs used by gb-softsvc and gb-uart-node driver are covered. As such, protocols.rs also only contains abstractions for greybus structures that are used by SVC and some types such as Greybus Interface do not implement AlwaysRefCounted at the moment.
HdDriver Note
**************
- In C message_send callback, it is possible for the caller to defer calling gb_message_sent. This is used in es2 driver. However, looking at the internals, it does not seem like the message owenership is actually transferred to the caller in real terms. Instead, it is more like the caller is shared ownership. This is evident by the fact that message_cancel can be called at any point, and depending on wheather the message is request or response, the message can become invalid after the message_cancel callback. Thus the driver is expected to ensure that no dangling references to that message exist.
Since gb_message is not an internally ref counted structure in true terms (it does seem to use the ref counting in the internal operation), and since double calls to gb_message_sent are not really allowed, I have made the decision to call gb_message_sent on a message in the abstractions rather than making the driver call it. This means the driver only gets a reference to the message, and never owns it. This does prevent a driver to use it in the way es2 currently does, but seems much less of a footgun.
Another possibility would be to implement AlwaysRefCounted on Message and pass the calls to gb_operation_get/put, but I have not gone through the internals enough to figure out if this is correct in all cases.
- At the moment, nothing in-tree calls message_send from an atomic context. However, since it does have gfp_mask in param, and an exported function, it is possible to call it from atomic context. So not sure if the doc comment should specific non-sleep requirement.
- message_cancel does not directly provide access to driver data. This was flagged by sashiko-bot. It actually is possible for the Rust abstractions to provide the driver data though. This is because we can do gb_message->operation->connection->host_device->priv_data. However, not sure if we should do this just in Rust abstractions. Maybe it would be preferable to change the C API itself instead.
--- MAINTAINERS | 1 + drivers/greybus/Kconfig | 8 + rust/bindings/bindings_helper.h | 1 + rust/kernel/alloc.rs | 6 + rust/kernel/greybus/hd.rs | 391 ++++++++++++++++++++++++++++++++++ rust/kernel/greybus/mod.rs | 200 ++++++++++++++++++ rust/kernel/greybus/protocols.rs | 445 +++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 2 + 8 files changed, 1054 insertions(+)
MAINTAINERS +1 -0
@@ -11526,6 +11526,7 @@ F: drivers/greybus/ F: drivers/staging/greybus/ F: include/linux/greybus.h F: include/linux/greybus/ +F: rust/kernel/greybus/ GREYBUS UART PROTOCOLS DRIVERS M: David Lin <[email protected]>
drivers/greybus/Kconfig +8 -0
@@ -15,6 +15,14 @@ menuconfig GREYBUS To compile this code as a module, choose M here: the module will be called greybus.ko +config RUST_GREYBUS_ABSTRACTIONS + bool "Rust greybus abstractions" + depends on RUST + depends on SYSFS + select GREYBUS + help + This enables the Rust abstraction for the Greybus API. + if GREYBUS config GREYBUS_BEAGLEPLAY
rust/bindings/bindings_helper.h +1 -0
@@ -64,6 +64,7 @@#include <linux/firmware.h> #include <linux/fwctl.h> #include <linux/fs.h> +#include <linux/greybus.h> #include <linux/i2c.h> #include <linux/interrupt.h> #include <linux/io-pgtable.h>
rust/kernel/alloc.rs +6 -0
@@ -38,6 +38,12 @@pub struct Flags (u32 );impl Flags { +/// Creates [`Flags`] from a raw `gfp_t`. +#[cfg_attr(not( CONFIG_RUST_GREYBUS_ABSTRACTIONS ), expect(dead_code))] +pub (crate )fn new (v: bindings::gfp_t ) ->Self { +Self (v) + } +/// Get the raw representation of this flag. pub (crate )fn as_raw (self) ->u32 { self.0
rust/kernel/greybus/hd.rs new file +391 -0
391 lines not on this page · raw message
rust/kernel/greybus/mod.rs new file +200 -0
200 lines not on this page · raw message
rust/kernel/greybus/protocols.rs new file +445 -0
445 lines not on this page · raw message
rust/kernel/lib.rs +2 -0
@@ -84,6 +84,8 @@pub mod fwctl;#[cfg( CONFIG_GPU_BUDDY = "y" )] pub mod gpu; +#[cfg( CONFIG_RUST_GREYBUS_ABSTRACTIONS )] +pub mod greybus;#[cfg( CONFIG_I2C = "y" )] pub mod i2c;pub mod id_pool;
Signature · 1 line
2.56.0
[email protected]
AI reviewRe: [PATCH v5 6/9] rust: kernel: Add greybus abstractions
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] rust: greybus: blanket Send and Sync impls on hd::Device<Ctx> --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] rust: greybus: blanket Send and Sync impls on hd::Device<Ctx>
--
commit d45a0cafbb6b7cfe1ad47516461c24f42e8c8a34
Author: Ayush Singh <[email protected]>
rust: kernel: Add greybus abstractions
This commit adds Rust abstractions for the Greybus core, including host device registration, driver callbacks, and wire protocol definitions.
Quoted 5 lines
rust/kernel/greybus/hd.rs new file +0
-0
[ ... ]
Quoted 6 lines
rust/kernel/greybus/hd.rs new file +6
-0+/// A Greybus host device. +/// +/// # Invariants +/// +/// The wrapped value is a valid `struct gb_host_device` created by `gb_hd_create()`, and every +/// [`ARef<Device>`](crate::sync::aref::ARef) owns an increment on its reference count.
rust/kernel/greybus/hd.rs line 184
Quoted 10 lines
rust/kernel/greybus/hd.rs new file +10
-0+#[repr(transparent)] +pub struct Device <Ctx = device::Normal >(Opaque <bindings::gb_host_device >,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 > {}
[Severity: Low]
As reported by Sashiko on v4 [1], in rust/kernel/greybus/hd.rs, Device<Ctx> provides blanket Send and Sync implementations for all Ctx and omits the
Ctx: device::DeviceContext bound on the struct definition.
Because Device<Ctx> also implements AsRef<device::Device<Ctx>>, marking Device<device::CoreInternal> as Sync would allow sharing a &Device<device::CoreInternal> reference across threads and obtaining concurrent &device::Device<device::CoreInternal> references, even though device::Device<device::CoreInternal> is intentionally !Sync because set_drvdata() and drvdata_obtain() mutate dev->driver_data without synchronization.
Other bus device wrappers in the kernel crate (such as device::Device, serdev::Device, platform::Device, pci::Device, auxiliary::Device, and usb::Device) bound Ctx: device::DeviceContext on the struct definition and only implement Send and Sync for Device (Device<device::Normal>) and Sync for Device<device::Bound>, keeping Core and CoreInternal contexts !Send and !Sync.
Should Device<Ctx> in rust/kernel/greybus/hd.rs constrain
Ctx: device::DeviceContext and restrict its Send and Sync implementations to match the other device wrappers?
[1] https://lore.kernel.org/r/[email protected]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
PATCH v5 7/9 drivers: greybus: Add software SVC implementation
In reply to Ayush SinghA 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/[email protected]/
--- MAINTAINERS | 6 + drivers/greybus/Kconfig | 12 + drivers/greybus/Makefile | 40 +++ drivers/greybus/gb_softsvc.rs | 533 +++++++++++++++++++++++++++++++++++ drivers/greybus/gb_softsvc_exports.c | 15 + 5 files changed, 606 insertions(+)
MAINTAINERS +6 -0
@@ -11516,6 +11516,12 @@ S: Maintained F: Documentation/devicetree/bindings/net/ti,cc1352p7.yaml F: drivers/greybus/gb-beagleplay.c +GREYBUS SOFTSVC DRIVERS +M: Ayush Singh <[email protected]> +L: [email protected] (moderated for non-subscribers) +S: Maintained +F: drivers/greybus/gb_softsvc.rs + GREYBUS SUBSYSTEM M: Johan Hovold <[email protected]> M: Alex Elder <[email protected]>
drivers/greybus/Kconfig +12 -0
@@ -49,5 +49,17 @@ config GREYBUS_ES2 To compile this code as a module, choose M here: the module will be called gb-es2.ko +config GREYBUS_SOFTSVC + tristate "Greybus software SVC" + depends on RUST_GREYBUS_ABSTRACTIONS + help + Select this option to enable a software implementation of the + Greybus SVC (Supervisory Controller) running in the kernel. It can + be used by host drivers that talk to a Greybus network which does + not provide an SVC of its own. + + To compile this code as a module, choose M here: the module + will be called gb-softsvc.ko + endif # GREYBUS
drivers/greybus/Makefile +40 -0
@@ -25,4 +25,44 @@ gb-es2-y := es2.o obj-$(CONFIG_GREYBUS_ES2) += gb-es2.o +obj-$(CONFIG_GREYBUS_SOFTSVC) += gb-softsvc.o +gb-softsvc-y += gb_softsvc.o gb_softsvc_exports.o +rust_needed_exports = \ + { $(if $(strip $(2)),$(NM) -u $(2);,) echo "__DEFINED_RUST_SYMBOLS__"; \ + $(NM) -p --defined-only $(1); } | \ + awk -v fmt='$(3)' ' \ + /^__DEFINED_RUST_SYMBOLS__$$/ { defs = 1; next } \ + !defs { if ($$NF ~ /^_R/) needed[$$NF] = 1; next } \ + defs && $$2 ~ /(T|R|D|B)/ && $$3 ~ /^_R/ && \ + $$3 !~ /_(init|cleanup)_module$$/ && \ + $$3 !~ /__(pfx|cfi|odr_asan)/ && \ + $$3 in needed { printf fmt, $$3 } \ + ' + +quiet_cmd_exports = EXPORTS $@ + cmd_exports = \ + $(call rust_needed_exports,$<,$(gb-softsvc-export-deps),EXPORT_SYMBOL_RUST_GPL(%s);\n) > $@ + +$(obj)/exports_gb_softsvc_generated.h: $(obj)/gb_softsvc.o $(gb-softsvc-export-deps) FORCE + $(call if_changed,exports) + +targets += exports_gb_softsvc_generated.h + +$(obj)/gb_softsvc_exports.o: $(obj)/exports_gb_softsvc_generated.h +CFLAGS_gb_softsvc_exports.o := -I $(objtree)/$(obj) + +ifdef CONFIG_MODVERSIONS +# The C export shim declares Rust symbols as `extern int`, so reuse its export +# list but generate symbol CRCs from the Rust object instead of the shim's DWARF. +$(obj)/gb_softsvc_exports.o: private cmd_gensymtypes_c = \ + $(call getexportsymbols,\1) | \ + $(objtree)/scripts/gendwarfksyms/gendwarfksyms \ + $(if $(KBUILD_GENDWARFKSYMS_STABLE), --stable) \ + $(if $(KBUILD_SYMTYPES), --symtypes $(@:.o=.symtypes),) \ + $(obj)/gb_softsvc.o +endif + +# Output gb_softsvc's crate metadata for use by gb_uart_node at compile time. +RUSTFLAGS_gb_softsvc.o += \ + --emit=metadata=$(objtree)/$(obj)/libgb_softsvc.rmeta
drivers/greybus/gb_softsvc.rs new file +533 -0
533 lines not on this page · raw message
drivers/greybus/gb_softsvc_exports.c new file +15 -0
@@ -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"
Signature · 1 line
2.56.0
[email protected]
AI reviewRe: [PATCH v5 7/9] drivers: greybus: Add software SVC implementation
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…
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 <[email protected]>
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.
Quoted 25 lines
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, andmodule_remove()on disconnect. Interface IDs are allocated by the SVC and returned frommodule_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.
Quoted 7 lines
gb_softsvc_exports.cexports 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/[email protected]/
Signed-off-by Ayush Singh <[email protected]>
[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.
Quoted 5 lines
drivers/greybus/gb_softsvc.rs new file +0
-0
[ ... ]
drivers/greybus/gb_softsvc.rs line 12
Quoted 13 lines
drivers/greybus/gb_softsvc.rs new file +12
-0@@ -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.
Quoted 15 lines
drivers/greybus/gb_softsvc.rs new file +15
-0+ ( +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 (()) + } +}
[ ... ]
Quoted 19 lines
drivers/greybus/gb_softsvc.rs new file +19
-0+impl kernel::Module for GreybusSoftSvc { +fn init (_module: &'staticThisModule ) ->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,
drivers/greybus/gb_softsvc.rs line 463
Quoted 10 lines
drivers/greybus/gb_softsvc.rs new file +10
-0+ }) + } +} + +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/[email protected]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
PATCH v5 8/9 dt-bindings: beagle: Add BeagleConnect Freedom
In reply to Ayush SinghAdd 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.
--- .../beagle/beagle,beagleconnect-freedom.yaml | 62 ++++++++++++++++++++++ MAINTAINERS | 6 +++ 2 files changed, 68 insertions(+)
Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml new file +62 -0
@@ -0,0 +1,62 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/beagle/beagle,beagleconnect-freedom.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: BeagleBoard.org BeagleConnect Freedom + +description: + The BeagleBoard.org BeagleConnect Freedom board connected over USB, UART, SPI or I2C. + +maintainers: + - Ayush Singh <[email protected]> + +properties: + compatible: + const: beagle,beagleconnect-freedom + + reg: + maxItems: 1 + + vbat-supply: true + +required: + - compatible + +allOf: + - if: + required: + - reg + then: + $ref: /schemas/spi/spi-peripheral-props.yaml# + else: + $ref: /schemas/serial/serial-peripheral-props.yaml# + + properties: + current-speed: + default: 115200 + +unevaluatedProperties: false + +examples: + - | + serial { + mcu { + compatible = "beagle,beagleconnect-freedom"; + vbat-supply = <&vbat>; + }; + }; + + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + + mcu@0 { + compatible = "beagle,beagleconnect-freedom"; + reg = <0>; + spi-max-frequency = <1000000>; + vbat-supply = <&vbat>; + }; + };
MAINTAINERS +6 -0
@@ -11522,6 +11522,12 @@ L: [email protected] (moderated for non-subscribers) S: Maintained F: drivers/greybus/gb_softsvc.rs +GREYBUS UART NODE DRIVERS +M: Ayush Singh <[email protected]> +L: [email protected] (moderated for non-subscribers) +S: Maintained +F: Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml + GREYBUS SUBSYSTEM M: Johan Hovold <[email protected]> M: Alex Elder <[email protected]>
Signature · 1 line
2.56.0
PATCH v5 9/9 greybus: Add Rust UART node driver
In reply to Ayush SinghAdd 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: "current-speed" 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/[email protected]/
--- MAINTAINERS | 1 + drivers/greybus/.gitignore | 1 + drivers/greybus/Kconfig | 15 +++ drivers/greybus/Makefile | 10 ++ drivers/greybus/gb_uart_node.rs | 270 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 297 insertions(+)
MAINTAINERS +1 -0
@@ -11527,6 +11527,7 @@ M: Ayush Singh <[email protected]> L: [email protected] (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 <[email protected]>
drivers/greybus/.gitignore new file +1 -0
@@ -0,0 +1 @@ +exports_gb_softsvc_generated.h
drivers/greybus/Kconfig +15 -0
@@ -61,5 +61,20 @@ config GREYBUS_SOFTSVC To compile this code as a module, choose M here: the module will be called gb-softsvc.ko +config GREYBUS_UART_NODE + tristate "Greybus UART node transport" + depends on GREYBUS_SOFTSVC + depends on RUST_SERIAL_DEV_BUS_ABSTRACTIONS + depends on RUST_GREYBUS_ABSTRACTIONS + select RUST_CRC_CCITT_ABSTRACTIONS + help + Select this option if you have a Greybus node connected over a + serial port. The node is registered with the software SVC, which + handles the SVC protocol on behalf of the AP, so no dedicated + coprocessor running SVC firmware is required. + + To compile this code as a module, choose M here: the module + will be called gb-uart-node.ko + endif # GREYBUS
drivers/greybus/Makefile +10 -0
@@ -28,6 +28,12 @@ obj-$(CONFIG_GREYBUS_ES2) += gb-es2.o obj-$(CONFIG_GREYBUS_SOFTSVC) += gb-softsvc.o gb-softsvc-y += gb_softsvc.o gb_softsvc_exports.o +obj-$(CONFIG_GREYBUS_UART_NODE) += gb-uart-node.o +gb-uart-node-y += gb_uart_node.o + +# Export Rust symbols from gb-softsvc only if gb-uart-node actually references them. +gb-softsvc-export-deps := $(if $(CONFIG_GREYBUS_UART_NODE),$(obj)/gb_uart_node.o) + rust_needed_exports = \ { $(if $(strip $(2)),$(NM) -u $(2);,) echo "__DEFINED_RUST_SYMBOLS__"; \ $(NM) -p --defined-only $(1); } | \
@@ -66,3 +72,7 @@ endif # Output gb_softsvc's crate metadata for use by gb_uart_node at compile time. RUSTFLAGS_gb_softsvc.o += \ --emit=metadata=$(objtree)/$(obj)/libgb_softsvc.rmeta + +# Allow gb_uart_node to import gb_softsvc's types. +$(obj)/gb_uart_node.o: $(obj)/gb_softsvc.o +RUSTFLAGS_gb_uart_node.o := -L $(objtree)/$(obj) --extern gb_softsvc
drivers/greybus/gb_uart_node.rs new file +270 -0
270 lines not on this page · raw message
Signature · 1 line
2.56.0