greybus-dev November 2017

greybus-dev@lists.linaro.org

7 participants
14 discussions

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

by 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(a)nexus-software.ie> Cc: Johan Hovold <johan(a)kernel.org> Cc: Alex Elder <elder(a)kernel.org> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> Cc: Kees Cook <keescook(a)chromium.org> Cc: greybus-dev(a)lists.linaro.org Cc: devel(a)driverdev.osuosl.org Cc: linux-kernel(a)vger.kernel.org --- drivers/staging/greybus/loopback.c | 165 +++++++------------------------------ 1 file changed, 31 insertions(+), 134 deletions(-) diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 3d92638..48599ed 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,29 @@ 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); + mutex_lock(&gb->mutex); 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); + atomic_dec(&gb->outstanding_operations); + gb_operation_put(operation); + kfree(op_async); done: mutex_unlock(&gb->mutex); return ret; @@ -1024,8 +919,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

6 years, 8 months

[PATCH 1/2] staging: greybus: operation: add private data with get/set accessors

by 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 operation.h and get/set methods to access that pointer. Signed-off-by: Bryan O'Donoghue <pure.logic(a)nexus-software.ie> Cc: Johan Hovold <johan(a)kernel.org> Cc: Alex Elder <elder(a)kernel.org> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> Cc: greybus-dev(a)lists.linaro.org Cc: devel(a)driverdev.osuosl.org Cc: linux-kernel(a)vger.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

6 years, 8 months

[PATCH 1/2] staging: greybus: remove unused kfifo_ts

by Arnd Bergmann

As of commit 8e1d6c336d74 ("greybus: loopback: drop bus aggregate calculation"), nothing ever reads from kfifo_ts, so there is no reason to write to it or even allocate it any more. Signed-off-by: Arnd Bergmann <arnd(a)arndb.de> --- drivers/staging/greybus/loopback.c | 27 +++------------------------ 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 08e255884206..85046fb16aad 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -72,7 +72,6 @@ struct gb_loopback { struct dentry *file; struct kfifo kfifo_lat; - struct kfifo kfifo_ts; struct mutex mutex; struct task_struct *task; struct list_head entry; @@ -262,7 +261,6 @@ static void gb_loopback_check_attr(struct gb_loopback *gb) gb->iteration_max, kfifo_depth); } kfifo_reset_out(&gb->kfifo_lat); - kfifo_reset_out(&gb->kfifo_ts); switch (gb->type) { case GB_LOOPBACK_TYPE_PING: @@ -387,13 +385,6 @@ static u64 gb_loopback_calc_latency(struct timeval *ts, struct timeval *te) return __gb_loopback_calc_latency(t1, t2); } -static void gb_loopback_push_latency_ts(struct gb_loopback *gb, - struct timeval *ts, struct timeval *te) -{ - kfifo_in(&gb->kfifo_ts, (unsigned char *)ts, sizeof(*ts)); - kfifo_in(&gb->kfifo_ts, (unsigned char *)te, sizeof(*te)); -} - static int gb_loopback_operation_sync(struct gb_loopback *gb, int type, void *request, int request_size, void *response, int response_size) @@ -433,7 +424,6 @@ static int gb_loopback_operation_sync(struct gb_loopback *gb, int type, do_gettimeofday(&te); /* Calculate the total time the message took */ - gb_loopback_push_latency_ts(gb, &ts, &te); gb->elapsed_nsecs = gb_loopback_calc_latency(&ts, &te); out_put_operation: @@ -521,11 +511,9 @@ static void gb_loopback_async_operation_callback(struct gb_operation *operation) err = true; } - if (!err) { - gb_loopback_push_latency_ts(gb, &op_async->ts, &te); + if (!err) gb->elapsed_nsecs = gb_loopback_calc_latency(&op_async->ts, &te); - } if (op_async->pending) { if (err) @@ -1241,18 +1229,12 @@ static int gb_loopback_probe(struct gb_bundle *bundle, retval = -ENOMEM; goto out_conn; } - if (kfifo_alloc(&gb->kfifo_ts, kfifo_depth * sizeof(struct timeval) * 2, - GFP_KERNEL)) { - retval = -ENOMEM; - goto out_kfifo0; - } - /* Fork worker thread */ mutex_init(&gb->mutex); gb->task = kthread_run(gb_loopback_fn, gb, "gb_loopback"); if (IS_ERR(gb->task)) { retval = PTR_ERR(gb->task); - goto out_kfifo1; + goto out_kfifo; } spin_lock_irqsave(&gb_dev.lock, flags); @@ -1266,9 +1248,7 @@ static int gb_loopback_probe(struct gb_bundle *bundle, return 0; -out_kfifo1: - kfifo_free(&gb->kfifo_ts); -out_kfifo0: +out_kfifo: kfifo_free(&gb->kfifo_lat); out_conn: device_unregister(dev); @@ -1302,7 +1282,6 @@ static void gb_loopback_disconnect(struct gb_bundle *bundle) kthread_stop(gb->task); kfifo_free(&gb->kfifo_lat); - kfifo_free(&gb->kfifo_ts); gb_connection_latency_tag_disable(gb->connection); debugfs_remove(gb->file); -- 2.9.0

6 years, 8 months

[PATCH v2] staging: greybus: Convert timers to use timer_setup()

by Kees Cook

In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly. Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> Cc: "Bryan O'Donoghue" <pure.logic(a)nexus-software.ie> Cc: Johan Hovold <johan(a)kernel.org> Cc: Alex Elder <elder(a)kernel.org> Cc: greybus-dev(a)lists.linaro.org Cc: devel(a)driverdev.osuosl.org Signed-off-by: Kees Cook <keescook(a)chromium.org> --- v2: Added back "get" in timer code, thanks to Bryan. :) --- drivers/staging/greybus/loopback.c | 19 +++++++++---------- drivers/staging/greybus/operation.c | 7 +++---- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 08e255884206..1c0bafeb7ea5 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -572,16 +572,16 @@ static void gb_loopback_async_operation_work(struct work_struct *work) gb_loopback_async_operation_put(op_async); } -static void gb_loopback_async_operation_timeout(unsigned long data) +static void gb_loopback_async_operation_timeout(struct timer_list *t) { - struct gb_loopback_async_operation *op_async; - u16 id = data; + struct gb_loopback_async_operation *op_async = + from_timer(op_async, t, timer); + unsigned long flags; + + spin_lock_irqsave(&gb_dev.lock, flags); + gb_loopback_async_operation_get(op_async); + spin_unlock_irqrestore(&gb_dev.lock, flags); - 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); } @@ -631,8 +631,7 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type, if (ret) goto error; - setup_timer(&op_async->timer, gb_loopback_async_operation_timeout, - (unsigned long)operation->id); + timer_setup(&op_async->timer, gb_loopback_async_operation_timeout, 0); op_async->timer.expires = jiffies + gb->jiffy_timeout; add_timer(&op_async->timer); diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 3023012808d9..ee4ba3f23bef 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -294,9 +294,9 @@ static void gb_operation_work(struct work_struct *work) gb_operation_put(operation); } -static void gb_operation_timeout(unsigned long arg) +static void gb_operation_timeout(struct timer_list *t) { - struct gb_operation *operation = (void *)arg; + struct gb_operation *operation = from_timer(operation, t, timer); if (gb_operation_result_set(operation, -ETIMEDOUT)) { /* @@ -541,8 +541,7 @@ gb_operation_create_common(struct gb_connection *connection, u8 type, goto err_request; } - setup_timer(&operation->timer, gb_operation_timeout, - (unsigned long)operation); + timer_setup(&operation->timer, gb_operation_timeout, 0); } operation->flags = op_flags; -- 2.7.4 -- Kees Cook Pixel Security

6 years, 8 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

greybus-dev November 2017