* [PATCH v3 1/4] staging: greybus: loopback: Hold per-connection mutex across operations
2017-11-06 1:32 [PATCH v3 0/4] Convert greybus loopback to core async API Bryan O'Donoghue
@ 2017-11-06 1:32 ` Bryan O'Donoghue
2017-11-06 9:17 ` Johan Hovold
2017-11-06 1:32 ` [PATCH v3 2/4] staging: greybus: loopback: Fix iteration count on async path Bryan O'Donoghue
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Bryan O'Donoghue @ 2017-11-06 1:32 UTC (permalink / raw)
To: johan, elder, gregkh, devel, keescook, linux-kernel
Cc: Bryan O'Donoghue, Mitch Tasman, greybus-dev
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 3/4] staging: greybus: operation: add private data with get/set accessors
2017-11-06 1:32 [PATCH v3 0/4] Convert greybus loopback to core async API Bryan O'Donoghue
2017-11-06 1:32 ` [PATCH v3 1/4] staging: greybus: loopback: Hold per-connection mutex across operations Bryan O'Donoghue
2017-11-06 1:32 ` [PATCH v3 2/4] staging: greybus: loopback: Fix iteration count on async path Bryan O'Donoghue
@ 2017-11-06 1:32 ` Bryan O'Donoghue
2017-11-06 1:32 ` [PATCH v3 4/4] staging: greybus: loopback: convert loopback to use generic async operations Bryan O'Donoghue
3 siblings, 0 replies; 8+ messages in thread
From: Bryan O'Donoghue @ 2017-11-06 1:32 UTC (permalink / raw)
To: johan, elder, gregkh, devel, keescook, linux-kernel
Cc: Bryan O'Donoghue, Mitch Tasman, greybus-dev
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 4/4] staging: greybus: loopback: convert loopback to use generic async operations
2017-11-06 1:32 [PATCH v3 0/4] Convert greybus loopback to core async API Bryan O'Donoghue
` (2 preceding siblings ...)
2017-11-06 1:32 ` [PATCH v3 3/4] staging: greybus: operation: add private data with get/set accessors Bryan O'Donoghue
@ 2017-11-06 1:32 ` Bryan O'Donoghue
2017-11-06 9:19 ` Johan Hovold
3 siblings, 1 reply; 8+ messages in thread
From: Bryan O'Donoghue @ 2017-11-06 1:32 UTC (permalink / raw)
To: johan, elder, gregkh, devel, keescook, linux-kernel
Cc: Bryan O'Donoghue, Mitch Tasman, greybus-dev
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
^ permalink raw reply related [flat|nested] 8+ messages in thread