Re: [RFC 08/11] gpiolib: cdev: Add hardware timestamp clock type
From: Kent Gibson <warthog618@gmail.com>
Date: 2021-07-01 14:24:43
Also in:
linux-devicetree, linux-gpio, linux-tegra, lkml
On Fri, Jun 25, 2021 at 04:55:29PM -0700, Dipen Patel wrote:
quoted hunk ↗ jump to hunk
This patch adds new clock type for the GPIO controller which can timestamp gpio lines using hardware means. To expose such functionalities to the userspace, code has been added in this patch where during line create call, it checks for new clock type and if requested, calls hardware timestamp related API from gpiolib.c. During line change event, it retrieves timestamp in nano seconds by calling gpiod_get_hw_timestamp API from gpiolib.c. At the line release, it disables this functionality by calling gpiod_hw_timestamp_control. Signed-off-by: Dipen Patel <dipenp@nvidia.com> --- drivers/gpio/gpiolib-cdev.c | 65 +++++++++++++++++++++++++++++++++++-- include/uapi/linux/gpio.h | 1 + 2 files changed, 64 insertions(+), 2 deletions(-)diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 1631727bf0da..9f98c727e937 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c@@ -518,6 +518,7 @@ struct linereq { GPIO_V2_LINE_DRIVE_FLAGS | \ GPIO_V2_LINE_EDGE_FLAGS | \ GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME | \ + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \ GPIO_V2_LINE_BIAS_FLAGS) static void linereq_put_event(struct linereq *lr,@@ -540,9 +541,20 @@ static void linereq_put_event(struct linereq *lr, static u64 line_event_timestamp(struct line *line) { + bool block; + if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags)) return ktime_get_real_ns(); + if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &line->desc->flags)) { + if (irq_count()) + block = false; + else + block = true; + + return gpiod_get_hw_timestamp(line->desc, block); + } +
Use in_task() instead of block?
quoted hunk ↗ jump to hunk
return ktime_get_ns(); }@@ -828,6 +840,7 @@ static int edge_detector_setup(struct line *line, return ret; line->irq = irq; + return 0; }
Remove gratuitous whitespace changes. If you dislike the formatting then suggest it in a separate patch.
quoted hunk ↗ jump to hunk
@@ -891,7 +904,6 @@ static int gpio_v2_line_flags_validate(u64 flags) /* Return an error if an unknown flag is set */ if (flags & ~GPIO_V2_LINE_VALID_FLAGS) return -EINVAL; - /* * Do not allow both INPUT and OUTPUT flags to be set as they are * contradictory.@@ -900,6 +912,14 @@ static int gpio_v2_line_flags_validate(u64 flags) (flags & GPIO_V2_LINE_FLAG_OUTPUT)) return -EINVAL;
Same here.
+ /* + * Do not mix with any other clocks if hardware assisted timestamp is + * asked. + */ + if ((flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME) && + (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE)) + return -EINVAL; +
The comment is very hw timestamp centric. It should just be something along the lines of "only allow one event clock source".
quoted hunk ↗ jump to hunk
/* Edge detection requires explicit input. */ if ((flags & GPIO_V2_LINE_EDGE_FLAGS) && !(flags & GPIO_V2_LINE_FLAG_INPUT))@@ -992,6 +1012,8 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 flags, assign_bit(FLAG_EVENT_CLOCK_REALTIME, flagsp, flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME); + assign_bit(FLAG_EVENT_CLOCK_HARDWARE, flagsp, + flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE); } static long linereq_get_values(struct linereq *lr, void __user *ip)@@ -1139,6 +1161,18 @@ static long linereq_set_config_unlocked(struct linereq *lr, int val = gpio_v2_line_config_output_value(lc, i); edge_detector_stop(&lr->lines[i]); + + /* + * Assuming line was input before and hardware + * assisted timestamp only timestamps the input + * lines. + */ + if (gpiod_is_hw_timestamp_enabled(desc)) { + ret = gpiod_hw_timestamp_control(desc, false); + if (ret) + return ret; + } +
So if you fail to disable the hw timestamp then you fail the set_config? Does that make sense? It should be impossible to fail, as per the preceding edge_detector_stop(), or any failure in this context is irrelevant and so can be ignored.
quoted hunk ↗ jump to hunk
ret = gpiod_direction_output(desc, val); if (ret) return ret;@@ -1152,6 +1186,13 @@ static long linereq_set_config_unlocked(struct linereq *lr, polarity_change); if (ret) return ret; + + /* Check if new config sets hardware assisted clock */ + if (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE) { + ret = gpiod_hw_timestamp_control(desc, true); + if (ret) + return ret; + } }
The error code here can come from the pinctrl timestamp_control(), so it should be sanitised before being returned to userspace.
quoted hunk ↗ jump to hunk
blocking_notifier_call_chain(&desc->gdev->notifier,@@ -1281,8 +1322,12 @@ static void linereq_free(struct linereq *lr) for (i = 0; i < lr->num_lines; i++) { edge_detector_stop(&lr->lines[i]); - if (lr->lines[i].desc) + if (lr->lines[i].desc) { + if (gpiod_is_hw_timestamp_enabled(lr->lines[i].desc)) + gpiod_hw_timestamp_control(lr->lines[i].desc, + false); gpiod_free(lr->lines[i].desc); + }
Potential race on gpiod_is_hw_timestamp_enabled() and the call to gpiod_hw_timestamp_control()? Why not put the gpiod_is_hw_timestamp_enabled() check inside gpiod_hw_timestamp_control()? And the gpiod_hw_timestamp_control() call should be moved inside gpiod_free(), or more correctly gpiod_free_commit(). i.e. whenever you free the gpio you release any associated hw timestamp.
quoted hunk ↗ jump to hunk
} kfifo_free(&lr->events); kfree(lr->label);@@ -1409,6 +1454,15 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) flags & GPIO_V2_LINE_EDGE_FLAGS); if (ret) goto out_free_linereq; + + /* + * Check if hardware assisted timestamp is requested + */ + if (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE) { + ret = gpiod_hw_timestamp_control(desc, true); + if (ret) + goto out_free_linereq; + } }
Comment can fit on one line, and probably isn't even necessary - the code is clear enough.
quoted hunk ↗ jump to hunk
blocking_notifier_call_chain(&desc->gdev->notifier,@@ -1956,8 +2010,15 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, if (test_bit(FLAG_EDGE_FALLING, &desc->flags)) info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING; + /* + * Practically it is possible that user will want both the real time + * and hardware timestamps on GPIO events, for now however lets just + * work with either clocks + */ if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &desc->flags)) info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME; + else if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &desc->flags)) + info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE;
If there is any need or intent to support multiple clock sources then avoid creeping API changes and add it now. Either way, drop the comment.
quoted hunk ↗ jump to hunk
debounce_period_us = READ_ONCE(desc->debounce_period_us); if (debounce_period_us) {diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h index eaaea3d8e6b4..d360545b4c21 100644 --- a/include/uapi/linux/gpio.h +++ b/include/uapi/linux/gpio.h@@ -80,6 +80,7 @@ enum gpio_v2_line_flag { GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN = _BITULL(9), GPIO_V2_LINE_FLAG_BIAS_DISABLED = _BITULL(10), GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME = _BITULL(11), + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE = _BITULL(12), }; /**-- 2.17.1
Cheers, Kent.