January 2017 - greybus-dev - lists.linaro.org

[PATCH] staging: greybus: operation: add generic timeout support

by Johan Hovold

Add a struct timer_list to struct gb_operation and use that to implement generic operation timeouts. This simplifies the synchronous operation handling somewhat while also providing a generic timeout mechanism that drivers can use for asynchronous operations. Signed-off-by: Johan Hovold <johan(a)kernel.org> --- drivers/staging/greybus/loopback.c | 1 + drivers/staging/greybus/operation.c | 50 ++++++++++++++++++++++++++----------- drivers/staging/greybus/operation.h | 2 ++ 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 6c2a41c638c3..4bee33f62fd4 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -629,6 +629,7 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type, mutex_lock(&gb->mutex); ret = gb_operation_request_send(operation, gb_loopback_async_operation_callback, + 0, GFP_KERNEL); if (ret) goto error; diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 0123109a1070..3023012808d9 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -273,18 +273,40 @@ static void gb_operation_request_handle(struct gb_operation *operation) static void gb_operation_work(struct work_struct *work) { struct gb_operation *operation; + int ret; operation = container_of(work, struct gb_operation, work); - if (gb_operation_is_incoming(operation)) + if (gb_operation_is_incoming(operation)) { gb_operation_request_handle(operation); - else + } else { + ret = del_timer_sync(&operation->timer); + if (!ret) { + /* Cancel request message if scheduled by timeout. */ + if (gb_operation_result(operation) == -ETIMEDOUT) + gb_message_cancel(operation->request); + } + operation->callback(operation); + } gb_operation_put_active(operation); gb_operation_put(operation); } +static void gb_operation_timeout(unsigned long arg) +{ + struct gb_operation *operation = (void *)arg; + + if (gb_operation_result_set(operation, -ETIMEDOUT)) { + /* + * A stuck request message will be cancelled from the + * workqueue. + */ + queue_work(gb_operation_completion_wq, &operation->work); + } +} + static void gb_operation_message_init(struct gb_host_device *hd, struct gb_message *message, u16 operation_id, size_t payload_size, u8 type) @@ -518,6 +540,9 @@ gb_operation_create_common(struct gb_connection *connection, u8 type, gfp_flags)) { goto err_request; } + + setup_timer(&operation->timer, gb_operation_timeout, + (unsigned long)operation); } operation->flags = op_flags; @@ -679,6 +704,7 @@ static void gb_operation_sync_callback(struct gb_operation *operation) * gb_operation_request_send() - send an operation request message * @operation: the operation to initiate * @callback: the operation completion callback + * @timeout: operation timeout in milliseconds, or zero for no timeout * @gfp: the memory flags to use for any allocations * * The caller has filled in any payload so the request message is ready to go. @@ -693,6 +719,7 @@ static void gb_operation_sync_callback(struct gb_operation *operation) */ int gb_operation_request_send(struct gb_operation *operation, gb_operation_callback callback, + unsigned int timeout, gfp_t gfp) { struct gb_connection *connection = operation->connection; @@ -742,6 +769,11 @@ int gb_operation_request_send(struct gb_operation *operation, if (ret) goto err_put_active; + if (timeout) { + operation->timer.expires = jiffies + msecs_to_jiffies(timeout); + add_timer(&operation->timer); + } + return 0; err_put_active: @@ -763,26 +795,16 @@ int gb_operation_request_send_sync_timeout(struct gb_operation *operation, unsigned int timeout) { int ret; - unsigned long timeout_jiffies; ret = gb_operation_request_send(operation, gb_operation_sync_callback, - GFP_KERNEL); + timeout, GFP_KERNEL); if (ret) return ret; - if (timeout) - timeout_jiffies = msecs_to_jiffies(timeout); - else - timeout_jiffies = MAX_SCHEDULE_TIMEOUT; - - ret = wait_for_completion_interruptible_timeout(&operation->completion, - timeout_jiffies); + ret = wait_for_completion_interruptible(&operation->completion); if (ret < 0) { /* Cancel the operation if interrupted */ gb_operation_cancel(operation, -ECANCELED); - } else if (ret == 0) { - /* Cancel the operation if op timed out */ - gb_operation_cancel(operation, -ETIMEDOUT); } return gb_operation_result(operation); diff --git a/drivers/staging/greybus/operation.h b/drivers/staging/greybus/operation.h index de09a2c7de54..7529f01b2529 100644 --- a/drivers/staging/greybus/operation.h +++ b/drivers/staging/greybus/operation.h @@ -98,6 +98,7 @@ struct gb_operation { struct work_struct work; gb_operation_callback callback; struct completion completion; + struct timer_list timer; struct kref kref; atomic_t waiters; @@ -164,6 +165,7 @@ bool gb_operation_response_alloc(struct gb_operation *operation, int gb_operation_request_send(struct gb_operation *operation, gb_operation_callback callback, + unsigned int timeout, gfp_t gfp); int gb_operation_request_send_sync_timeout(struct gb_operation *operation, unsigned int timeout); -- 2.10.2

7 years, 10 months

3
16
0 0

[PATCH] staging: greybus: loopback: fix broken udelay

by Johan Hovold

The loopback driver allows the user to set a minimum delay of up to one second to be inserted between test iterations (i.e. request submissions). The delay is currently specified in microseconds and is implemented using udelay. Busy looping for long periods is not just anti-social; udelay must not be used for delays longer than a few milliseconds due to the risk of integer overflow. Replace the broken udelay with a usleep_range with a 100 us range for short delays (< 20 ms) and otherwise revert to using msleep. Fixes: b36f04fa9417 ("greybus: loopback: Convert thread delay to microseconds") Signed-off-by: Johan Hovold <johan(a)kernel.org> --- drivers/staging/greybus/loopback.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 6c2a41c638c3..a8329daf1e57 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -1061,8 +1061,13 @@ static int gb_loopback_fn(void *data) gb_loopback_calculate_stats(gb, !!error); } gb->send_count++; - if (us_wait) - udelay(us_wait); + + if (us_wait) { + if (us_wait < 20000) + usleep_range(us_wait, us_wait + 100); + else + msleep(us_wait / 1000); + } } gb_pm_runtime_put_autosuspend(bundle); -- 2.10.2

7 years, 10 months

2
1
0 0

[PATCH] greybus: Fix coding stye error

by mrossibellom＠gmail.com

From: Maxime Rossi Bellom <mrossibellom(a)gmail.com> Split line before funtion argument. Error was reported by checkpatch.pl as WARNING: line over 80 characters Signed-off-by: Maxime Rossi Bellom <mrossibellom(a)gmail.com> --- drivers/staging/greybus/fw-management.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c index 3cd6cf0..d88117e 100644 --- a/drivers/staging/greybus/fw-management.c +++ b/drivers/staging/greybus/fw-management.c @@ -516,7 +516,8 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd, */ fw_mgmt->mode_switch_started = true; - ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf); + ret = gb_interface_request_mode_switch( + fw_mgmt->connection->intf); if (ret) { dev_err(fw_mgmt->parent, "Mode-switch failed: %d\n", ret); -- 2.7.4

7 years, 10 months

5
11
0 0

[PATCH] staging: greybus: sdio: Prefer u32 over uint32_t

by Franck Demathieu

It fixes the following issue reported by checkpatch.pl: Prefer kernel type 'u32' over 'uint32_t' Signed-off-by: Franck Demathieu <fdemathieu(a)gmail.com> --- drivers/staging/greybus/sdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/sdio.c b/drivers/staging/greybus/sdio.c index 66b37ea..101ca50 100644 --- a/drivers/staging/greybus/sdio.c +++ b/drivers/staging/greybus/sdio.c @@ -52,7 +52,7 @@ struct gb_sdio_host { static inline bool single_op(struct mmc_command *cmd) { - uint32_t opcode = cmd->opcode; + u32 opcode = cmd->opcode; return opcode == MMC_WRITE_BLOCK || opcode == MMC_READ_SINGLE_BLOCK; -- 2.10.1

7 years, 11 months

3
2
0 0

[PATCH] staging: greybus: uart: fix TIOCGSERIAL flags

by Johan Hovold

This driver does not have a low-latency mode and should not report anything else. Also drop the skip-test flag which isn't used either. Signed-off-by: Johan Hovold <johan(a)kernel.org> --- drivers/staging/greybus/uart.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 6d39f4a04754..248ad6661a02 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -624,7 +624,6 @@ static int get_serial_info(struct gb_tty *gb_tty, struct serial_struct tmp; memset(&tmp, 0, sizeof(tmp)); - tmp.flags = ASYNC_LOW_LATENCY | ASYNC_SKIP_TEST; tmp.type = PORT_16550A; tmp.line = gb_tty->minor; tmp.xmit_fifo_size = 16; -- 2.10.2

7 years, 11 months

2
1
0 0

[PATCH] staging: greybus: sdio: Prefer u32 over uint32_t

by Franck Demathieu

It fixes the following issue reported by checkpatch.pl: Prefer kernel type 'u32' over 'uint32_t' Signed-off-by: Franck Demathieu <fdemathieu(a)gmail.com> --- drivers/staging/greybus/sdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/sdio.c b/drivers/staging/greybus/sdio.c index 66b37ea..101ca50 100644 --- a/drivers/staging/greybus/sdio.c +++ b/drivers/staging/greybus/sdio.c @@ -52,7 +52,7 @@ struct gb_sdio_host { static inline bool single_op(struct mmc_command *cmd) { - uint32_t opcode = cmd->opcode; + u32 opcode = cmd->opcode; return opcode == MMC_WRITE_BLOCK || opcode == MMC_READ_SINGLE_BLOCK; -- 2.10.1

7 years, 11 months

2
1
0 0

[PATCH v2] staging: greybus: timesync: validate platform state callback

by Rui Miguel Silva

When tearingdown timesync, and not in arche platform, the state platform callback is not initialized. That will trigger the following NULL dereferencing. CallTrace: ? gb_timesync_platform_unlock_bus+0x11/0x20 [greybus] gb_timesync_teardown+0x85/0xc0 [greybus] gb_timesync_svc_remove+0xab/0x190 [greybus] gb_svc_del+0x29/0x110 [greybus] gb_hd_del+0x14/0x20 [greybus] ap_disconnect+0x24/0x60 [gb_es2] usb_unbind_interface+0x7a/0x2c0 __device_release_driver+0x96/0x150 device_release_driver+0x1e/0x30 bus_remove_device+0xe7/0x130 device_del+0x116/0x230 usb_disable_device+0x97/0x1f0 usb_disconnect+0x80/0x260 hub_event+0x5ca/0x10e0 process_one_work+0x126/0x3b0 worker_thread+0x55/0x4c0 ? process_one_work+0x3b0/0x3b0 kthread+0xc4/0xe0 ? kthread_park+0xb0/0xb0 ret_from_fork+0x22/0x30 So, fix that by adding checks before use the callback. Fixes: 970dc85bd95d ("greybus: timesync: Add timesync core driver") Cc: <stable(a)vger.kernel.org> # 4.9.x Signed-off-by: Rui Miguel Silva <rmfrfs(a)gmail.com> --- Greg, This patch does not contain the "commit <sha> upstream" snippet since you already removed the timesync code from the -next tree, even though it is still in 4.10-rc. However this will affect the greybus users of 4.9.x. If you meanwhile pull that remove to rc, this patch is only necessary in stable 4.9.x. v1->v2: - add Fixes tag to changelog Johan Hovold: - for symmetry with the lock, adjust the unlock_bus function accordantly. drivers/staging/greybus/timesync_platform.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/staging/greybus/timesync_platform.c b/drivers/staging/greybus/timesync_platform.c index 113f3d6c4b3a..27f75b17679b 100644 --- a/drivers/staging/greybus/timesync_platform.c +++ b/drivers/staging/greybus/timesync_platform.c @@ -45,12 +45,18 @@ u32 gb_timesync_platform_get_clock_rate(void) int gb_timesync_platform_lock_bus(struct gb_timesync_svc *pdata) { + if (!arche_platform_change_state_cb) + return 0; + return arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_TIME_SYNC, pdata); } void gb_timesync_platform_unlock_bus(void) { + if (!arche_platform_change_state_cb) + return; + arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE, NULL); } -- 2.11.0

7 years, 11 months

3
2
0 0

[PATCH] staging: greybus: timesync: validate platform state callback

by Rui Miguel Silva

When tearingdown timesync, and not in arche platform, the state platform callback is not initialized. That will trigger the following NULL dereferencing. CallTrace: ? gb_timesync_platform_unlock_bus+0x11/0x20 [greybus] gb_timesync_teardown+0x85/0xc0 [greybus] gb_timesync_svc_remove+0xab/0x190 [greybus] gb_svc_del+0x29/0x110 [greybus] gb_hd_del+0x14/0x20 [greybus] ap_disconnect+0x24/0x60 [gb_es2] usb_unbind_interface+0x7a/0x2c0 __device_release_driver+0x96/0x150 device_release_driver+0x1e/0x30 bus_remove_device+0xe7/0x130 device_del+0x116/0x230 usb_disable_device+0x97/0x1f0 usb_disconnect+0x80/0x260 hub_event+0x5ca/0x10e0 process_one_work+0x126/0x3b0 worker_thread+0x55/0x4c0 ? process_one_work+0x3b0/0x3b0 kthread+0xc4/0xe0 ? kthread_park+0xb0/0xb0 ret_from_fork+0x22/0x30 So, fix that by adding checks before use the callback. Cc: <stable(a)vger.kernel.org> # 4.9.x Signed-off-by: Rui Miguel Silva <rmfrfs(a)gmail.com> --- Greg, This patch does not contain the "commit <sha> upstream" snippet since you already removed the timesync code from the -next tree. So, It would never get there. However this will affect the greybus users of 4.9.x. drivers/staging/greybus/timesync_platform.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/staging/greybus/timesync_platform.c b/drivers/staging/greybus/timesync_platform.c index 113f3d6c4b3a..7d3ccc2bc98c 100644 --- a/drivers/staging/greybus/timesync_platform.c +++ b/drivers/staging/greybus/timesync_platform.c @@ -45,13 +45,18 @@ u32 gb_timesync_platform_get_clock_rate(void) int gb_timesync_platform_lock_bus(struct gb_timesync_svc *pdata) { + if (!arche_platform_change_state_cb) + return 0; + return arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_TIME_SYNC, pdata); } void gb_timesync_platform_unlock_bus(void) { - arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE, NULL); + if (arche_platform_change_state_cb) + arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE, + NULL); } static const struct of_device_id arch_timer_of_match[] = { -- 2.11.0

7 years, 11 months

2
2
0 0

[PATCH v3 0/4] drivers/staging/greybus: add async operations

by Bryan O'Donoghue

V3: Picard: "Let's make sure history never forgets the name ... Enterprise." Drop gb_operation_cancel_async. The sync version works just as well - Johan V2: Scotty : "What are you standing around for - do you not know a jailbreak when you see one" ? Making sure to run format-patch against the right tree this time.. Updated meta-comments to highlight adding a generic async timeout - Johan Captured the result code of schedule_work/queue_work - Bryan Drop reference count on failure to submit schedule_work/queue_work - Bryan Added timeout suffix back to new gb_operation_request_send - Johan/Greg Updated Signed-off-by to my nexus-software.ie address - Bryan Iterated requisite Star Trek Quote - Bryan/Johan V4-private -> V1-public: McCOY: You've got him, Jim! You've got him where you want him. This patchset adds async operations to greybus-core. Rather than have different drivers do variations on the same theme of launching and timing out asynchronous operations, it makes sense instead to provide this functionality via greybus-core. This patchset makes it possible to launch asynchronous operations and have the completion callback for an operation indicate -ETIMEDOUT for timeouts without drivers having to manage that locally. This set doesn't convert the existing synchronous operations away from wait_for_completion_interruptible_timeout() since that would involve adding an extra work-queue item to each synchronous call, with no added benefit for synchronous operations. Synchronous operations can already detect -ETIMEDOUT by blocking on the synchronous send operations, asynchronous operations and the driver associated with those operations OTOH must implement a completion handler. The main improvement this patchset makes is to pass the -ETIMEDOUT completion status into that completion handler - hiding the setup/timeout of operations away from a driver-level and into a core-level set of logic. Loopback as an example can have lots and lots of redundant code removed and given we previously had some in-flight effort to add asynchronous operations to SDIO it makes sense to move the generic types of things we need to do on the asynchronous path into greybus-core so that SDIO and other bundle drivers can reuse instead of reimplement asynchronous operations. Note: we had approximately three internal versions of this on Project-Ara. Here's the log for completeness. V3-private -> V4-private: Fix bug in loopback conversion - Mitch Tasman V2-private -> V3-private: remotes/ara-main/sandbox/bodonoghue/ara-main-current+async_op3 Drop patch #6 converting sync operations to new t/o structure. Johan had a concern about doing an in-depth analysis on that change and considering our compressed timelines now, this analysis won't happen. OTOH the new async API is the right thing for loopback and for potential later greybus users/implementers to reuse. Although it wasn't part of the motive for this change - I observe slightly better performance than baseline with loopback tests with this change in place - which shouldn't be surprising since we have less aggregate context switching for operation timeouts. V1-private -> V2-private: Rename async_private => private - Greg gb_operation_request_send_async_timeout -> gb_operation_request_send_async() - Greg Using existing gb_operation_completion_wq - Viresh Split out gb_operation_cancel() - timeout need not wait synchronously on cancellation queue - Bryan Move timeout to work-queue - Greg/Viresh cancel timeout workqueue @ operation completion point - Viresh/Bryan Bryan O'Donoghue (4): staging: greybus: operation: add generic asynchronous timeout operation support staging: greybus: operation: add private data with get/set accessors staging: greybus: loopback: convert loopback to use generic async operations staging: greybus: loopback: convert to use msecs internally drivers/staging/greybus/loopback.c | 181 ++++++++---------------------------- drivers/staging/greybus/operation.c | 66 +++++++++++++ drivers/staging/greybus/operation.h | 18 ++++ 3 files changed, 123 insertions(+), 142 deletions(-) -- 2.7.4

7 years, 11 months

3
10
0 0

[PATCH] Staging: greybus: gpio.c - style fix

by Derek Robson

Fixed bare use of 'unsigned', Found using checkpatch Signed-off-by: Derek Robson <robsonde(a)gmail.com> --- drivers/staging/greybus/gpio.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c index 250caa00de5e..e937666610e6 100644 --- a/drivers/staging/greybus/gpio.c +++ b/drivers/staging/greybus/gpio.c @@ -410,21 +410,21 @@ static int gb_gpio_request_handler(struct gb_operation *op) return 0; } -static int gb_gpio_request(struct gpio_chip *chip, unsigned offset) +static int gb_gpio_request(struct gpio_chip *chip, unsigned int offset) { struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip); return gb_gpio_activate_operation(ggc, (u8)offset); } -static void gb_gpio_free(struct gpio_chip *chip, unsigned offset) +static void gb_gpio_free(struct gpio_chip *chip, unsigned int offset) { struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip); gb_gpio_deactivate_operation(ggc, (u8)offset); } -static int gb_gpio_get_direction(struct gpio_chip *chip, unsigned offset) +static int gb_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) { struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip); u8 which; @@ -438,14 +438,14 @@ static int gb_gpio_get_direction(struct gpio_chip *chip, unsigned offset) return ggc->lines[which].direction ? 1 : 0; } -static int gb_gpio_direction_input(struct gpio_chip *chip, unsigned offset) +static int gb_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) { struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip); return gb_gpio_direction_in_operation(ggc, (u8)offset); } -static int gb_gpio_direction_output(struct gpio_chip *chip, unsigned offset, +static int gb_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value) { struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip); @@ -453,7 +453,7 @@ static int gb_gpio_direction_output(struct gpio_chip *chip, unsigned offset, return gb_gpio_direction_out_operation(ggc, (u8)offset, !!value); } -static int gb_gpio_get(struct gpio_chip *chip, unsigned offset) +static int gb_gpio_get(struct gpio_chip *chip, unsigned int offset) { struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip); u8 which; @@ -467,15 +467,15 @@ static int gb_gpio_get(struct gpio_chip *chip, unsigned offset) return ggc->lines[which].value; } -static void gb_gpio_set(struct gpio_chip *chip, unsigned offset, int value) +static void gb_gpio_set(struct gpio_chip *chip, unsigned int offset, int value) { struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip); gb_gpio_set_value_operation(ggc, (u8)offset, !!value); } -static int gb_gpio_set_debounce(struct gpio_chip *chip, unsigned offset, - unsigned debounce) +static int gb_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset, + unsigned int debounce) { struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip); u16 usec; @@ -593,7 +593,7 @@ static int gb_gpio_irqchip_add(struct gpio_chip *chip, { struct gb_gpio_controller *ggc; unsigned int offset; - unsigned irq_base; + unsigned int irq_base; if (!chip || !irqchip) return -EINVAL; @@ -625,7 +625,7 @@ static int gb_gpio_irqchip_add(struct gpio_chip *chip, return 0; } -static int gb_gpio_to_irq(struct gpio_chip *chip, unsigned offset) +static int gb_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) { struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip); -- 2.11.0

7 years, 11 months

2
1
0 0

2024

2023

2022

2021

2020

2019

2018

2017

2016

greybus-dev January 2017