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() returns 16 return to caller
Signed-off-by: Ayush Singh ayush@beagleboard.org --- drivers/greybus/operation.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/greybus/operation.c b/drivers/greybus/operation.c index 7e12ffb2dd60..a63bc2eb164e 100644 --- a/drivers/greybus/operation.c +++ b/drivers/greybus/operation.c @@ -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;