On Thu, Jul 29, 2021 at 07:25:36PM -0700, Dipen Patel wrote:

On 7/1/21 7:24 AM, Kent Gibson wrote:

quoted

On Fri, Jun 25, 2021 at 04:55:27PM -0700, Dipen Patel wrote:

quoted

Some GPIO chip can provide hardware timestamp support on its GPIO lines
, in order to support that additional functions needs to be added which
can talk to both GPIO chip and HTE (hardware timestamping engine)
subsystem. This patch introduces functions which gpio consumer can use
to request hardware assisted timestamping. Below is the list of the APIs
that are added in gpiolib subsystem.

- gpiod_hw_timestamp_control - to enable/disable HTE on specified GPIO
line. This API will return HTE specific descriptor for the specified
GPIO line during the enable call, it will be stored as pointer in the
gpio_desc structure as hw_ts_data.
- gpiod_is_hw_timestamp_enabled - to query if HTE is enabled on
specified GPIO line.
- gpiod_get_hw_timestamp - to retrieve hardware timestamps.

Signed-off-by: Dipen Patel <dipenp@nvidia.com>
---
 drivers/gpio/gpiolib.c        | 92 +++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.h        | 11 +++++
 include/linux/gpio/consumer.h | 21 +++++++-
 include/linux/gpio/driver.h   | 13 +++++
 4 files changed, 135 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 220a9d8dd4e3..335eaddfde98 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c

@@ -2361,6 +2361,98 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_output);
 
+/**
+ * gpiod_hw_timestamp_control - set the hardware assisted timestamp control.
+ * @desc:	GPIO to set
+ * @enable:	Set true to enable the hardware timestamp, false otherwise.
+ *
+ * Certain GPIO chip can rely on hardware assisted timestamp engines which can
+ * record timestamp at the occurance of the configured events on selected GPIO
+ * lines. This is helper API to control such engine.
+ *
+ * Return 0 in case of success, else an error code.
+ */
+int gpiod_hw_timestamp_control(struct gpio_desc *desc, bool enable)
+{
+	struct gpio_chip	*gc;
+	int			ret = 0;
+
+	VALIDATE_DESC(desc);
+	gc = desc->gdev->chip;
+
+	if (!gc->timestamp_control) {
+		gpiod_warn(desc,
+			   "%s: Hardware assisted ts not supported\n",
+			   __func__);
+		return -ENOTSUPP;
+	}
+
+	ret = gc->timestamp_control(gc, gpio_chip_hwgpio(desc),
+				    &desc->hdesc, enable);
+
+	if (ret) {
+		gpiod_warn(desc,
+			   "%s: ts control operation failed\n", __func__);
+		return ret;
+	}
+
+	if (!enable)
+		desc->hdesc = NULL;
+
+	return ret;
+}

Last I checked, pointer accesses are not guaranteed atomic, so how is
hdesc protected from concurrent access?
Here is it modified unprotected.
Below it is read unprotected.

The assumption I made here was, gpiod_hw_timestamp_control will be

called after client has made at least gpdio_request call. With that assumption,

how two or more client/consumers call gpiod_hw_timestamp_control API

with the same gpio_desc? I believe its not allowed as gpiod_request call will

fail for the looser if there is a race and hence there will not be any race here

in this API. Let me know your thoughts.

My assumptions are that the userspace client is multi-threaded and that
there is nothing preventing concurrent uAPI calls, including closing the
line request fd.

The specific case I had in mind is one thread closing the req fd while
another is using set_config to switch to the hardware event clock.
In that race, the close be calling linereq_free() at the same time the
linereq_set_config_unlocked() is being called.  Both of those functions
make calls to the functions here that read and write the hdesc.

There may be others, e.g. line_event_timestamp() running in the
irq_thread at the same time a set_config call switches the event clock
away from the hardware clock.

So assume concurrent access unless you can prove otherwise.

Cheers,
Kent.