[PATCH v5 1/9] greybus: operation: keep request active across message_send

[Ayush Singh]

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@xxxxxxxxxxxxxxx>
---
 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;
 

-- 
2.56.0