[PATCH v3 0/4] Convert greybus loopback to core async API

[PATCH v3 0/4] Convert greybus loopback to core async API

[Bryan O'Donoghue]

v3:
- Patch #1 Cc: linux-kernel@vger.kernel.or -> Cc:
  linux-kernel@vger.kernel.org

v2:
- Added Reviewed-by Johan for patch #3
- Added fix for mutex hold duration.
  Johan mentioned the holding of this across a gb_operation_send() call.
  Looking at this it shows a bug that crept in between two commits detailed
  in patch#1 here.
- Split a separate bugfix from Mitch which had been squashed into an old
  patch from nearly a year ago
- Added Mitch to the cc list
- Depends on Arnd Bermann's patch
  "staging: greybus/loopback: use ktime_get() for time intervals"

v1:
dbec27298b0d ('staging: greybus: operation: add generic timeout support')
gives the ability to remove lots of the asynchronous operation code in
loopback.

Kees is also doing a cleanup of timer code which for loopback will go away
when converting to the core API.

These two patches kill two birds with err, two stones (no aggression to
birds intended) namely:

- Converting over to the core asynchronous API
- Getting rid of the timer code in loopback which will unblock what Kees is
  doing.

Bryan O'Donoghue (4):
  staging: greybus: loopback: Hold per-connection mutex across
    operations
  staging: greybus: loopback: Fix iteration count on async path
  staging: greybus: operation: add private data with get/set accessors
  staging: greybus: loopback: convert loopback to use generic async
    operations

 drivers/staging/greybus/loopback.c  | 176 +++++++-----------------------------
 drivers/staging/greybus/operation.h |  13 +++
 2 files changed, 46 insertions(+), 143 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] staging: greybus: loopback: Hold per-connection mutex across operations

[Bryan O'Donoghue]

Commit d9fb3754ecf8 ("greybus: loopback: Relax locking during loopback
operations") changes the holding of the per-connection mutex to be less
restrictive because at the time of that commit per-connection mutexes were
encapsulated by a per-driver level gb_dev.mutex.

Commit 8e1d6c336d74 ("greybus: loopback: drop bus aggregate calculation")
on the other hand subtracts the driver level gb_dev.mutex but neglects to
move the mutex back to the place it was prior to commit d9fb3754ecf8
("greybus: loopback: Relax locking during loopback operations"), as a
result several members of the per connection struct gb_loopback are racy.

The solution is restoring the old location of mutex_unlock(&gb->mutex) as
it was in commit d9fb3754ecf8 ("greybus: loopback: Relax locking during
loopback operations").

Fixes: 8e1d6c336d74 ("greybus: loopback: drop bus aggregate calculation")

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Cc: Johan Hovold <johan@kernel.org>
Cc: Alex Elder <elder@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Mitch Tasman <tasman@leaflabs.com>
Cc: greybus-dev@lists.linaro.org
Cc: devel@driverdev.osuosl.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/staging/greybus/loopback.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
index 3d92638..20d1b45 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -605,7 +605,6 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type,
 	op_async->ts = ktime_get();
 	op_async->pending = true;
 	atomic_inc(&gb->outstanding_operations);
-	mutex_lock(&gb->mutex);
 	ret = gb_operation_request_send(operation,
 					gb_loopback_async_operation_callback,
 					0,
@@ -622,7 +621,6 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type,
 error:
 	gb_loopback_async_operation_put(op_async);
 done:
-	mutex_unlock(&gb->mutex);
 	return ret;
 }
 
@@ -1013,7 +1011,6 @@ static int gb_loopback_fn(void *data)
 		type = gb->type;
 		if (ktime_to_ns(gb->ts) == 0)
 			gb->ts = ktime_get();
-		mutex_unlock(&gb->mutex);
 
 		/* Else operations to perform */
 		if (gb->async) {
@@ -1041,6 +1038,7 @@ static int gb_loopback_fn(void *data)
 			gb_loopback_calculate_stats(gb, !!error);
 		}
 		gb->send_count++;
+		mutex_unlock(&gb->mutex);
 
 		if (us_wait) {
 			if (us_wait < 20000)
-- 
2.7.4

[PATCH v3 2/4] staging: greybus: loopback: Fix iteration count on async path

[Bryan O'Donoghue]

Commit 12927835d211 ("greybus: loopback: Add asynchronous bi-directional
support") does what it says on the tin - namely, adds support for
asynchronous bi-directional loopback operations.

What it neglects to do though is increment the per-connection
gb->iteration_count on an asynchronous operation error. This patch fixes
that omission.

Fixes: 12927835d211 ("greybus: loopback: Add asynchronous bi-directional support")

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Reported-by: Mitch Tasman <tasman@leaflabs.com>
Cc: Johan Hovold <johan@kernel.org>
Cc: Alex Elder <elder@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Mitch Tasman <tasman@leaflabs.com>
Cc: greybus-dev@lists.linaro.org
Cc: devel@driverdev.osuosl.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/staging/greybus/loopback.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
index 20d1b45..9c5980c 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -1021,8 +1021,10 @@ static int gb_loopback_fn(void *data)
 			else if (type == GB_LOOPBACK_TYPE_SINK)
 				error = gb_loopback_async_sink(gb, size);
 
-			if (error)
+			if (error) {
 				gb->error++;
+				gb->iteration_count++;
+			}
 		} else {
 			/* We are effectively single threaded here */
 			if (type == GB_LOOPBACK_TYPE_PING)
-- 
2.7.4

[PATCH v3 3/4] staging: greybus: operation: add private data with get/set accessors

[Bryan O'Donoghue]

Asynchronous operation completion handler's lives are made easier if there
is a generic pointer that can store private data associated with the
operation. This patch adds a pointer field to struct gb_operation and
get/set methods to access that pointer.

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Cc: Johan Hovold <johan@kernel.org>
Cc: Alex Elder <elder@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Mitch Tasman <tasman@leaflabs.com>
Cc: greybus-dev@lists.linaro.org
Cc: devel@driverdev.osuosl.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Johan Hovold <johan@kernel.org>
---
 drivers/staging/greybus/operation.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/greybus/operation.h b/drivers/staging/greybus/operation.h
index 7529f01..bfec1e9 100644
--- a/drivers/staging/greybus/operation.h
+++ b/drivers/staging/greybus/operation.h
@@ -105,6 +105,8 @@ struct gb_operation {
 
 	int			active;
 	struct list_head	links;		/* connection->operations */
+
+	void			*private;
 };
 
 static inline bool
@@ -206,6 +208,17 @@ static inline int gb_operation_unidirectional(struct gb_connection *connection,
 			request, request_size, GB_OPERATION_TIMEOUT_DEFAULT);
 }
 
+static inline void *gb_operation_get_data(struct gb_operation *operation)
+{
+	return operation->private;
+}
+
+static inline void gb_operation_set_data(struct gb_operation *operation,
+					 void *data)
+{
+	operation->private = data;
+}
+
 int gb_operation_init(void);
 void gb_operation_exit(void);
 
-- 
2.7.4

[PATCH v3 4/4] staging: greybus: loopback: convert loopback to use generic async operations

[Bryan O'Donoghue]

Loopback has its own internal method for tracking and timing out
asynchronous operations however previous patches make it possible to use
functionality provided by operation.c to do this instead. Using the code in
operation.c means we can completely subtract the timer, the work-queue, the
kref and the cringe-worthy 'pending' flag. The completion callback
triggered by operation.c will provide an authoritative result code -
including -ETIMEDOUT for asynchronous operations.

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Cc: Johan Hovold <johan@kernel.org>
Cc: Alex Elder <elder@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mitch Tasman <tasman@leaflabs.com>
Cc: greybus-dev@lists.linaro.org
Cc: devel@driverdev.osuosl.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/staging/greybus/loopback.c | 168 +++++++------------------------------
 1 file changed, 29 insertions(+), 139 deletions(-)

diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
index 9c5980c..6d51998 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -59,11 +59,6 @@ struct gb_loopback_async_operation {
 	struct gb_loopback *gb;
 	struct gb_operation *operation;
 	ktime_t ts;
-	struct timer_list timer;
-	struct list_head entry;
-	struct work_struct work;
-	struct kref kref;
-	bool pending;
 	int (*completion)(struct gb_loopback_async_operation *op_async);
 };
 
@@ -427,56 +422,6 @@ static int gb_loopback_operation_sync(struct gb_loopback *gb, int type,
 	return ret;
 }
 
-static void __gb_loopback_async_operation_destroy(struct kref *kref)
-{
-	struct gb_loopback_async_operation *op_async;
-
-	op_async = container_of(kref, struct gb_loopback_async_operation, kref);
-
-	list_del(&op_async->entry);
-	if (op_async->operation)
-		gb_operation_put(op_async->operation);
-	atomic_dec(&op_async->gb->outstanding_operations);
-	wake_up(&op_async->gb->wq_completion);
-	kfree(op_async);
-}
-
-static void gb_loopback_async_operation_get(struct gb_loopback_async_operation
-					    *op_async)
-{
-	kref_get(&op_async->kref);
-}
-
-static void gb_loopback_async_operation_put(struct gb_loopback_async_operation
-					    *op_async)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&gb_dev.lock, flags);
-	kref_put(&op_async->kref, __gb_loopback_async_operation_destroy);
-	spin_unlock_irqrestore(&gb_dev.lock, flags);
-}
-
-static struct gb_loopback_async_operation *
-	gb_loopback_operation_find(u16 id)
-{
-	struct gb_loopback_async_operation *op_async;
-	bool found = false;
-	unsigned long flags;
-
-	spin_lock_irqsave(&gb_dev.lock, flags);
-	list_for_each_entry(op_async, &gb_dev.list_op_async, entry) {
-		if (op_async->operation->id == id) {
-			gb_loopback_async_operation_get(op_async);
-			found = true;
-			break;
-		}
-	}
-	spin_unlock_irqrestore(&gb_dev.lock, flags);
-
-	return found ? op_async : NULL;
-}
-
 static void gb_loopback_async_wait_all(struct gb_loopback *gb)
 {
 	wait_event(gb->wq_completion,
@@ -488,83 +433,41 @@ static void gb_loopback_async_operation_callback(struct gb_operation *operation)
 	struct gb_loopback_async_operation *op_async;
 	struct gb_loopback *gb;
 	ktime_t te;
-	bool err = false;
+	int result;
 
 	te = ktime_get();
-	op_async = gb_loopback_operation_find(operation->id);
-	if (!op_async)
-		return;
-
+	result = gb_operation_result(operation);
+	op_async = gb_operation_get_data(operation);
 	gb = op_async->gb;
+
 	mutex_lock(&gb->mutex);
 
-	if (!op_async->pending || gb_operation_result(operation)) {
-		err = true;
-	} else {
-		if (op_async->completion)
-			if (op_async->completion(op_async))
-				err = true;
-	}
+	if (!result && op_async->completion)
+		result = op_async->completion(op_async);
 
-	if (!err)
+	if (!result) {
 		gb->elapsed_nsecs = gb_loopback_calc_latency(op_async->ts, te);
-
-	if (op_async->pending) {
-		if (err)
-			gb->error++;
-		gb->iteration_count++;
-		op_async->pending = false;
-		del_timer_sync(&op_async->timer);
-		gb_loopback_async_operation_put(op_async);
-		gb_loopback_calculate_stats(gb, err);
+	} else {
+		gb->error++;
+		if (result == -ETIMEDOUT)
+			gb->requests_timedout++;
 	}
-	mutex_unlock(&gb->mutex);
-
-	dev_dbg(&gb->connection->bundle->dev, "complete operation %d\n",
-		operation->id);
-
-	gb_loopback_async_operation_put(op_async);
-}
-
-static void gb_loopback_async_operation_work(struct work_struct *work)
-{
-	struct gb_loopback *gb;
-	struct gb_operation *operation;
-	struct gb_loopback_async_operation *op_async;
 
-	op_async = container_of(work, struct gb_loopback_async_operation, work);
-	gb = op_async->gb;
-	operation = op_async->operation;
+	gb->iteration_count++;
+	gb_loopback_calculate_stats(gb, result);
 
-	mutex_lock(&gb->mutex);
-	if (op_async->pending) {
-		gb->requests_timedout++;
-		gb->error++;
-		gb->iteration_count++;
-		op_async->pending = false;
-		gb_loopback_async_operation_put(op_async);
-		gb_loopback_calculate_stats(gb, true);
-	}
 	mutex_unlock(&gb->mutex);
 
-	dev_dbg(&gb->connection->bundle->dev, "timeout operation %d\n",
+	dev_dbg(&gb->connection->bundle->dev, "complete operation %d\n",
 		operation->id);
 
-	gb_operation_cancel(operation, -ETIMEDOUT);
-	gb_loopback_async_operation_put(op_async);
-}
-
-static void gb_loopback_async_operation_timeout(unsigned long data)
-{
-	struct gb_loopback_async_operation *op_async;
-	u16 id = data;
+	/* Wake up waiters */
+	atomic_dec(&op_async->gb->outstanding_operations);
+	wake_up(&gb->wq_completion);
 
-	op_async = gb_loopback_operation_find(id);
-	if (!op_async) {
-		pr_err("operation %d not found - time out ?\n", id);
-		return;
-	}
-	schedule_work(&op_async->work);
+	/* Release resources */
+	gb_operation_put(operation);
+	kfree(op_async);
 }
 
 static int gb_loopback_async_operation(struct gb_loopback *gb, int type,
@@ -575,15 +478,11 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type,
 	struct gb_loopback_async_operation *op_async;
 	struct gb_operation *operation;
 	int ret;
-	unsigned long flags;
 
 	op_async = kzalloc(sizeof(*op_async), GFP_KERNEL);
 	if (!op_async)
 		return -ENOMEM;
 
-	INIT_WORK(&op_async->work, gb_loopback_async_operation_work);
-	kref_init(&op_async->kref);
-
 	operation = gb_operation_create(gb->connection, type, request_size,
 					response_size, GFP_KERNEL);
 	if (!operation) {
@@ -594,33 +493,24 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type,
 	if (request_size)
 		memcpy(operation->request->payload, request, request_size);
 
+	gb_operation_set_data(operation, op_async);
+
 	op_async->gb = gb;
 	op_async->operation = operation;
 	op_async->completion = completion;
 
-	spin_lock_irqsave(&gb_dev.lock, flags);
-	list_add_tail(&op_async->entry, &gb_dev.list_op_async);
-	spin_unlock_irqrestore(&gb_dev.lock, flags);
-
 	op_async->ts = ktime_get();
-	op_async->pending = true;
+
 	atomic_inc(&gb->outstanding_operations);
 	ret = gb_operation_request_send(operation,
 					gb_loopback_async_operation_callback,
-					0,
+					jiffies_to_msecs(gb->jiffy_timeout),
 					GFP_KERNEL);
-	if (ret)
-		goto error;
-
-	setup_timer(&op_async->timer, gb_loopback_async_operation_timeout,
-			(unsigned long)operation->id);
-	op_async->timer.expires = jiffies + gb->jiffy_timeout;
-	add_timer(&op_async->timer);
-
-	goto done;
-error:
-	gb_loopback_async_operation_put(op_async);
-done:
+	if (ret) {
+		atomic_dec(&gb->outstanding_operations);
+		gb_operation_put(operation);
+		kfree(op_async);
+	}
 	return ret;
 }
 
-- 
2.7.4

Re: [PATCH v3 1/4] staging: greybus: loopback: Hold per-connection mutex across operations

[Johan Hovold]

On Mon, Nov 06, 2017 at 01:32:19AM +0000, Bryan O'Donoghue wrote:
> Commit d9fb3754ecf8 ("greybus: loopback: Relax locking during loopback
> operations") changes the holding of the per-connection mutex to be less
> restrictive because at the time of that commit per-connection mutexes were
> encapsulated by a per-driver level gb_dev.mutex.
> 
> Commit 8e1d6c336d74 ("greybus: loopback: drop bus aggregate calculation")
> on the other hand subtracts the driver level gb_dev.mutex but neglects to
> move the mutex back to the place it was prior to commit d9fb3754ecf8
> ("greybus: loopback: Relax locking during loopback operations"), as a
> result several members of the per connection struct gb_loopback are racy.
> 
> The solution is restoring the old location of mutex_unlock(&gb->mutex) as
> it was in commit d9fb3754ecf8 ("greybus: loopback: Relax locking during
> loopback operations").
> 
> Fixes: 8e1d6c336d74 ("greybus: loopback: drop bus aggregate calculation")
> 
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Alex Elder <elder@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Mitch Tasman <tasman@leaflabs.com>
> Cc: greybus-dev@lists.linaro.org
> Cc: devel@driverdev.osuosl.org
> Cc: linux-kernel@vger.kernel.org
> ---

This looks like it addresses both my comments on v1 (mutex and error
stats). Thanks for tracking it down.

Reviewed-by: Johan Hovold <johan@kernel.org>

Johan

Re: [PATCH v3 2/4] staging: greybus: loopback: Fix iteration count on async path

[Johan Hovold]

On Mon, Nov 06, 2017 at 01:32:20AM +0000, Bryan O'Donoghue wrote:
> Commit 12927835d211 ("greybus: loopback: Add asynchronous bi-directional
> support") does what it says on the tin - namely, adds support for
> asynchronous bi-directional loopback operations.
> 
> What it neglects to do though is increment the per-connection
> gb->iteration_count on an asynchronous operation error. This patch fixes
> that omission.
> 
> Fixes: 12927835d211 ("greybus: loopback: Add asynchronous bi-directional support")
> 
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> Reported-by: Mitch Tasman <tasman@leaflabs.com>

Reviewed-by: Johan Hovold <johan@kernel.org>

Re: [PATCH v3 4/4] staging: greybus: loopback: convert loopback to use generic async operations

[Johan Hovold]

On Mon, Nov 06, 2017 at 01:32:22AM +0000, Bryan O'Donoghue wrote:
> Loopback has its own internal method for tracking and timing out
> asynchronous operations however previous patches make it possible to use
> functionality provided by operation.c to do this instead. Using the code in
> operation.c means we can completely subtract the timer, the work-queue, the
> kref and the cringe-worthy 'pending' flag. The completion callback
> triggered by operation.c will provide an authoritative result code -
> including -ETIMEDOUT for asynchronous operations.
> 
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>

Reviewed-by: Johan Hovold <johan@kernel.org>