| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <52ecf0a6-07a6-ec43-4b1e-fb341ad969b6@nvidia.com>
Date: Tue, 27 Jul 2021 21:38:45 -0700
From: Dipen Patel <dipenp@...dia.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: <thierry.reding@...il.com>, <jonathanh@...dia.com>,
<linux-kernel@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
<linux-gpio@...r.kernel.org>, <linus.walleij@...aro.org>,
<bgolaszewski@...libre.com>, <warthog618@...il.com>,
<devicetree@...r.kernel.org>, <linux-doc@...r.kernel.org>,
<robh+dt@...nel.org>
Subject: Re: [RFC 02/11] drivers: Add HTE subsystem
On 7/4/21 1:15 PM, Jonathan Cameron wrote:
> On Fri, 25 Jun 2021 16:55:23 -0700
> Dipen Patel <dipenp@...dia.com> wrote:
>
>> Some devices can timestamp system lines/signals/Buses in real-time
>> using the hardware counter or other hardware means which can give
>> finer granularity and help avoid jitter introduced by software means
>> of timestamping. To utilize such functionality there has to be
>> framework where such devices can register themselves as producers or
>> providers so that the consumers or clients devices can request specific
>> line from the providers. This patch introduces such subsystem as
>> hardware timestamping engine (HTE).
>>
>> It provides below APIs for the provider:
>> - hte_register_chip() -- To register the HTE chip.
>> - hte_unregister_chip() -- To unregister the HTE chip.
>> - hte_push_ts_ns_atomic() -- To push timestamp data into HTE subsystem.
>>
>> It provides below APIs for the consumer:
>> - of_hte_request_ts() -- To request timestamp functionality.
>> - devm_of_hte_request_ts() -- Managed version of the above.
>> - hte_req_ts_by_dt_node() -- To request timestamp functionality by
>> using HTE provider dt node.
>> - devm_hte_release_ts() -- The managed version to release timestamp
>> functionality and associated resources.
>> - hte_retrieve_ts_ns() -- To retrieve timestamps.
>> - hte_retrieve_ts_ns_wait() -- Same as above but blocking version.
>> - hte_enable_ts() -- To disable timestamp functionality.
>> - hte_disable_ts() -- To enable timestamp functionality.
>> - hte_available_ts() -- To query available timestamp data.
>> - hte_release_ts() -- To release timestamp functionality and its
>> associated resources.
>> - hte_get_clk_src_info() -- To query clock source information from
>> the provider
>>
>> It provides centralized software buffer management per requested id to
>> store the timestamp data for the consumers as below:
>> - hte_set_buf_len() -- To set the buffer length.
>> - hte_get_buf_len() -- To get the buffer length.
>> - hte_set_buf_watermark() -- To set the software threshold/watermark.
>> - hte_get_buf_watermark() -- To get the software threshold/watermark.
>>
>> The detail about parameters and API usage are described in each
>> functions definitions in drivers/hte/hte.c file.
>>
>> The patch adds compilation support in Makefile and menu options in
>> Kconfig.
>>
>> Signed-off-by: Dipen Patel <dipenp@...dia.com>
> Hi Dipen, this isn't a particularly thorough review as I'm still getting my head
> around what this is doing + it is an RFC :)
Thanks for the review comments. My responses inline.
>
>> ---
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/hte/Kconfig | 22 +
>> drivers/hte/Makefile | 1 +
>> drivers/hte/hte.c | 1368 ++++++++++++++++++++++++++++++++++++++++++
>> include/linux/hte.h | 278 +++++++++
>> 6 files changed, 1672 insertions(+)
>> create mode 100644 drivers/hte/Kconfig
>> create mode 100644 drivers/hte/Makefile
>> create mode 100644 drivers/hte/hte.c
>> create mode 100644 include/linux/hte.h
>>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 47980c6b1945..9b078964974b 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -238,4 +238,6 @@ source "drivers/interconnect/Kconfig"
>> source "drivers/counter/Kconfig"
>>
>> source "drivers/most/Kconfig"
>> +
>> +source "drivers/hte/Kconfig"
>> endmenu
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 5a6d613e868d..0a996a698e4c 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -190,3 +190,4 @@ obj-$(CONFIG_GNSS) += gnss/
>> obj-$(CONFIG_INTERCONNECT) += interconnect/
>> obj-$(CONFIG_COUNTER) += counter/
>> obj-$(CONFIG_MOST) += most/
>> +obj-$(CONFIG_HTE) += hte/
>> diff --git a/drivers/hte/Kconfig b/drivers/hte/Kconfig
>> new file mode 100644
>> index 000000000000..394e112f7dfb
>> --- /dev/null
>> +++ b/drivers/hte/Kconfig
>> @@ -0,0 +1,22 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +menuconfig HTE
>> + bool "Hardware Timestamping Engine (HTE) Support"
>> + help
>> + Hardware Timestamping Engine (HTE) Support.
> Tidy this up, but think that's already been commented on.
Will do, not sure why my editor did not show this issue.
>
>> +
>> + Some devices provide hardware timestamping engine which can timestamp
>> + certain device lines/signals in realtime. This way to provide
>> + hardware assisted timestamp to generic signals like GPIOs, IRQs lines
>> + comes with benefit for the applications like autonomous machines
>> + needing accurate timestamping event with less jitter.
>> +
>> + This framework provides a generic interface to such HTE devices
>> + within the Linux kernel. It provides an API to register and
>> + unregister a HTE provider chip, configurable sw buffer to
>> + store the timestamps, push the timestamp from the HTE providers and
>> + retrieve timestamps for the consumers. It also provides means for the
>> + consumers to request signals it wishes to hardware timestamp and
>> + release them if not required.
>> +
>> + If unsure, say no.
>> +
>> diff --git a/drivers/hte/Makefile b/drivers/hte/Makefile
>> new file mode 100644
>> index 000000000000..9899dbe516f7
>> --- /dev/null
>> +++ b/drivers/hte/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_HTE) += hte.o
>> diff --git a/drivers/hte/hte.c b/drivers/hte/hte.c
>> new file mode 100644
>> index 000000000000..c53260d1e250
>> --- /dev/null
>> +++ b/drivers/hte/hte.c
>> @@ -0,0 +1,1368 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2021 NVIDIA Corporation
>> + *
>> + * Author: Dipen Patel <dipenp@...dia.com>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/kfifo.h>
>> +#include <linux/mutex.h>
>> +#include <linux/sched.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/hte.h>
>> +#include <linux/delay.h>
>> +#include <linux/debugfs.h>
>> +
>> +/* Global list of the HTE devices */
>> +static DEFINE_SPINLOCK(hte_lock);
>> +static LIST_HEAD(hte_devices);
>> +
>> +enum {
>> + HTE_TS_REGISTERED,
>> + HTE_TS_DISABLE,
>> +};
>> +
>> +/* Default FIFO depth */
>> +#define HTE_EV_FIFO_EL 32
>> +
>> +#define HTE_TS_NAME_LEN 10
>> +
>> +struct hte_ts_buf;
>> +
>> +/**
>> + * struct hte_ts_buf_acc_func - Software buffer management functions.
>> + * @store: Store timestamp from atomic context as providers most likely
>> + * be pushing timestamps from their interrupt handlers.
>> + * @read: Read timestamps from the buffer.
>> + * @el_available: Available timestamps to retrieve. The client can use this to
>> + * query available elements so that it can pre-allocate internal buffer to send
>> + * to during hte_retrieve_ts_ns API.
>> + * @set_length: Set length/depth of the buffer.
>> + * @get_length: Get length/depth of the buffer.
>> + * @set_watermark: Set software threshold of the buffer.
>> + * @get_watermark: Get software threshold of the buffer.
>> + * @release: Release/free buffer.
>> + * @reset: Reset the buffer.
>> + */
>> +struct hte_ts_buf_acc_func {
>> + unsigned int (*store)(struct hte_ts_buf *buf, void *data, size_t n);
>> + int (*read)(struct hte_ts_buf *buf, unsigned char *data, size_t n,
>> + size_t *copied);
>> + size_t (*el_available)(struct hte_ts_buf *buf);
>> + int (*set_length)(struct hte_ts_buf *buf,
>> + size_t length, size_t bpd);
>> + size_t (*get_length)(struct hte_ts_buf *buf);
>> + int (*set_watermark)(struct hte_ts_buf *buf,
>> + size_t val);
>> + size_t (*get_watermark)(struct hte_ts_buf *buf);
>> + void (*release)(struct hte_ts_buf *buf);
>> + void (*reset)(struct hte_ts_buf *buf);
>> +};
>> +
>> +/**
>> + * struct hte_ts_buf - Software buffer per requested id or entity to store
>> + * timestamps.
>> + *
>> + * @datum_len: Buffer depth or number of elements.
>> + * @bytes_per_datum: Element size in bytes.
>> + * @watermark: Software threshold at which client will be notified.
>> + * @valid: Validity of the buffer.
>> + * @pollq: Waitqueue for the blocking clients.
>> + * @access: Various buffer management functions.
>> + */
>> +struct hte_ts_buf {
>> + size_t datum_len;
>> + size_t bytes_per_datum;
>> + size_t watermark;
>> + bool valid;
>> + wait_queue_head_t pollq;
>> + const struct hte_ts_buf_acc_func *access;
>> +};
>> +
>> +/**
>> + * struct hte_ts_info - Information related to requested timestamp.
>> + *
>> + * @xlated_id: Timestamp ID as understood between HTE subsys and HTE provider,
>> + * See xlate callback API.
>> + * @flags: Flags holding state informations.
>> + * @seq: Timestamp sequence counter.
>> + * @dropped_ts: Dropped timestamps.
>> + * @cb: Callback to notify clients.
>> + * @mlock: Lock during timestamp request/release APIs.
>> + * @ts_dbg_root: Root for the debug fs.
>> + * @gdev: HTE abstract device that this timestamp belongs to.
>> + * @buf: Per requested timestamp software buffer.
>> + * @desc: Timestamp descriptor understood between clients and HTE subsystem.
>> + */
>> +struct hte_ts_info {
>> + u32 xlated_id;
>> + unsigned long flags;
>> + u64 seq;
>> + atomic_t dropped_ts;
>> + void (*cb)(enum hte_notify n);
>> + struct mutex mlock;
>> + struct dentry *ts_dbg_root;
>> + struct hte_device *gdev;
>> + struct hte_ts_buf *buf;
>> + struct hte_ts_desc *desc;
>> +};
>> +
>> +/**
>> + * struct hte_device - HTE abstract device
>> + * @nlines: Number of entities this device supports.
>> + * @ts_req: Total number of entities requested.
>> + * @ei: Timestamp information.
>> + * @sdev: Device used at various debug prints.
>> + * @dbg_root: Root directory for debug fs.
>> + * @list: List node for internal use.
> Be more specific of what sort of internal use.
Sure..
>
>> + * @chip: HTE chip providing this HTE device.
>> + * @owner: helps prevent removal of modules when in use.
>> + */
>> +struct hte_device {
>> + u32 nlines;
>> + atomic_t ts_req;
>> + struct hte_ts_info *ei;
>> + struct device *sdev;
>> + struct dentry *dbg_root;
>> + struct list_head list;
>> + struct hte_chip *chip;
>> + struct module *owner;
>> +};
>> +
>> +/* Buffer management functions */
>> +
>> +/**
>> + * struct hte_kfifo - Software buffer wrapper.
>> + * @buffer: Abstract buffer device.
>> + * @gkf: Actual software buffer type, this case its FIFO.
>> + */
>> +struct hte_kfifo {
>> + struct hte_ts_buf buffer;
>> + struct kfifo gkf;
>> +};
>> +
>> +#define buf_to_kfifo(r) container_of(r, struct hte_kfifo, buffer)
>> +
>> +static unsigned int hte_ts_store_to_buf(struct hte_ts_buf *r, void *data,
>> + size_t n)
>> +{
>> + struct hte_kfifo *kf = buf_to_kfifo(r);
>> +
>> + if (unlikely(!r->valid))
>> + return 0;
>> +
>> + return kfifo_in(&kf->gkf, (unsigned char *)data, n);
>> +}
>> +
>> +static inline int hte_ts_buf_read(struct hte_ts_buf *r,
>> + unsigned char *buf, size_t n,
>> + size_t *copied)
>> +{
>> + struct hte_kfifo *kf = buf_to_kfifo(r);
>> +
>> + if ((!r->valid) || (n < kfifo_esize(&kf->gkf)))
>> + return -EINVAL;
>> +
>> + *copied = kfifo_out(&kf->gkf, buf, n);
>> +
>> + return 0;
>> +}
>> +
>> +static size_t hte_ts_buf_el_available(struct hte_ts_buf *r)
>> +{
>> + struct hte_kfifo *kf = buf_to_kfifo(r);
>> +
>> + if (!r->valid)
>> + return 0;
>> +
>> + return (kfifo_len(&kf->gkf) / r->bytes_per_datum);
>> +}
>> +
>> +static int hte_ts_buf_set_length(struct hte_ts_buf *r,
>> + size_t length, size_t bpd)
>> +{
>> + int ret = 0;
>> + struct hte_kfifo *buf;
>> +
>> + if ((length == 0) || (bpd == 0) || !r)
>> + return -EINVAL;
>> +
>> + buf = buf_to_kfifo(r);
>> +
>> + if (r->datum_len != length) {
>> + if (r->valid)
>> + kfifo_free(&buf->gkf);
>> + r->valid = false;
>> + r->datum_len = length;
>> + r->bytes_per_datum = bpd;
>> + ret = kfifo_alloc(&buf->gkf, length * bpd, GFP_KERNEL);
>> + if (!ret)
>> + r->valid = true;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static inline size_t hte_ts_buf_get_length(struct hte_ts_buf *r)
>> +{
>> + if ((!r->valid) || !r->datum_len)
>> + return 0;
>> +
>> + return r->datum_len;
>> +}
>> +
>> +static inline int hte_ts_buf_set_watermark(struct hte_ts_buf *r, size_t val)
>> +{
>> + if ((!r->valid) || (val > r->datum_len))
>> + return -EINVAL;
>> +
>> + r->watermark = val;
>> +
>> + return 0;
>> +}
>> +
>> +static inline size_t hte_ts_buf_get_watermark(struct hte_ts_buf *r)
>> +{
>> + if (!r->valid)
>> + return 0;
>> +
>> + return r->watermark;
>> +}
>> +
>> +static inline void hte_ts_buf_release(struct hte_ts_buf *r)
>> +{
>> + struct hte_kfifo *kf = buf_to_kfifo(r);
>> +
>> + r->valid = false;
>> + kfifo_free(&kf->gkf);
>> + kfree(kf);
>> +}
>> +
>> +static inline void hte_ts_buf_reset(struct hte_ts_buf *r)
>> +{
>> + struct hte_kfifo *kf = buf_to_kfifo(r);
>> +
>> + if (!r->valid)
>> + return;
>> +
>> + kfifo_reset(&kf->gkf);
>> +}
>> +
>> +static const struct hte_ts_buf_acc_func kfifo_access_funcs = {
>> + .store = &hte_ts_store_to_buf,
>> + .read = &hte_ts_buf_read,
>> + .el_available = &hte_ts_buf_el_available,
>> + .set_length = &hte_ts_buf_set_length,
>> + .get_length = &hte_ts_buf_get_length,
>> + .set_watermark = &hte_ts_buf_set_watermark,
>> + .get_watermark = &hte_ts_buf_get_watermark,
>> + .release = &hte_ts_buf_release,
>> + .reset = &hte_ts_buf_reset,
>> +};
>> +
>> +static struct hte_ts_buf *hte_ts_buf_allocate(void)
>> +{
>> + struct hte_kfifo *kf;
>> +
>> + kf = kzalloc(sizeof(*kf), GFP_KERNEL);
>> + if (!kf)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + init_waitqueue_head(&kf->buffer.pollq);
>> + kf->buffer.watermark = 1;
>> + kf->buffer.datum_len = 0;
>> + kf->buffer.valid = false;
>> + kf->buffer.access = &kfifo_access_funcs;
>> +
>> + return &kf->buffer;
>> +}
>> +/* End of buffer management */
>> +
>> +/* Debugfs management */
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> +
>> +static struct dentry *hte_root;
>> +
>> +static void __init hte_subsys_dbgfs_init(void)
>> +{
>> + /* creates /sys/kernel/debug/hte/ */
>> + hte_root = debugfs_create_dir("hte", NULL);
>> +}
>> +subsys_initcall(hte_subsys_dbgfs_init);
>> +
>> +static void hte_chip_dbgfs_init(struct hte_device *gdev)
>> +{
>> + const struct hte_chip *chip = gdev->chip;
>> + const char *name = chip->name ? chip->name : dev_name(chip->dev);
>> +
>> + gdev->dbg_root = debugfs_create_dir(name, hte_root);
>> + if (!gdev->dbg_root)
>> + return;
>> +
>> + debugfs_create_atomic_t("ts_requested", 0444, gdev->dbg_root,
>> + &gdev->ts_req);
>> + debugfs_create_u32("total_ts", 0444, gdev->dbg_root,
>> + &gdev->nlines);
>> +}
>> +
>> +static void hte_ts_dbgfs_init(const char *name, struct hte_ts_info *ei)
>> +{
>> + if (!ei->gdev->dbg_root || !name)
>> + return;
>> +
>> + ei->ts_dbg_root = debugfs_create_dir(name, ei->gdev->dbg_root);
>> + if (!ei->ts_dbg_root)
>> + return;
>> +
>> + debugfs_create_size_t("ts_buffer_depth", 0444, ei->ts_dbg_root,
>> + &ei->buf->datum_len);
>> + debugfs_create_size_t("ts_buffer_watermark", 0444, ei->ts_dbg_root,
>> + &ei->buf->watermark);
>> + debugfs_create_atomic_t("dropped_timestamps", 0444, ei->ts_dbg_root,
>> + &ei->dropped_ts);
>> +}
>> +
>> +static inline void hte_dbgfs_deinit(struct dentry *root)
>> +{
>> + if (!root)
>> + return;
>> +
>> + debugfs_remove_recursive(root);
>> +}
>> +
>> +#else
>> +
>> +static void hte_chip_dbgfs_init(struct hte_device *gdev)
>> +{
>> +}
>> +
>> +static void hte_ts_dbgfs_init(const char *name, struct hte_ts_info *ei)
>> +{
>> +}
>> +
>> +static inline void hte_dbgfs_deinit(struct dentry *root)
>> +{
>> +}
>> +
>> +#endif
>> +/* end of debugfs management*/
>> +
>> +/* Driver APIs */
>> +
>> +/**
>> + * hte_release_ts() - Consumer calls this API to release the entity, where
>> + * entity could be anything providers support, like lines, signals, buses,
>> + * etc...
>> + *
>> + * The correct sequence to call this API is as below:
>> + * 1) Call hte_disable_ts, this stops the timestamp push from the provider.
>> + * 2) Retrieve timestamps by calling non blocking hte_retrieve_ts_ns API if you
>> + * still care about the data.
>> + * 3) Call this API.
>> + * Above sequence makes sure that entity gets released race free.
>> + *
>> + * @desc: timestamp descriptor, this is the same as returned by the request API.
>> + *
>> + * Context: hte_dbgfs_deinit() function call may use sleeping locks,
>> + * not suitable from atomic context in that case.
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int hte_release_ts(struct hte_ts_desc *desc)
>> +{
>> + u32 id;
>> + int ret = 0;
>> + struct hte_device *gdev;
>> + struct hte_ts_info *ei;
>> + struct hte_ts_buf *buf;
>> +
>> + if (!desc)
>> + return -EINVAL;
>> +
>> + ei = (struct hte_ts_info *)desc->data_subsys;
> As data_subsys is void * you don't need to explicitly cast it to another pointer type.
Sure..
>
>> +
>> + if (!ei || !ei->gdev || !ei->buf)
>> + return -EINVAL;
>> +
>> + gdev = ei->gdev;
>> + buf = ei->buf;
>> + id = desc->con_id;
>> +
>> + if (!test_bit(HTE_TS_REGISTERED, &ei->flags)) {
>> + dev_info(gdev->sdev, "id:%d is not registered", id);
>> + return -EUSERS;
>> + }
>> +
>> + ret = gdev->chip->ops->release(gdev->chip, ei->xlated_id);
>> + if (ret) {
>> + dev_err(gdev->sdev, "id: %d free failed\n", id);
>> + goto out;
>> + }
>> +
>> + atomic_dec(&gdev->ts_req);
>> + atomic_set(&ei->dropped_ts, 0);
>> +
>> + kfree(desc->name);
>> + kfree(desc);
>> + ei->desc = NULL;
>> + ei->seq = 0;
>> + buf->access->release(buf);
>> +
>> + hte_dbgfs_deinit(ei->ts_dbg_root);
>> + module_put(gdev->owner);
>> +
>> + clear_bit(HTE_TS_REGISTERED, &ei->flags);
>> +
>> +out:
>> + dev_dbg(gdev->sdev, "%s: id: %d\n", __func__, id);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(hte_release_ts);
>> +
>> +static int hte_ts_dis_en_common(struct hte_ts_desc *desc, bool en)
>> +{
>> + u32 ts_id;
>> + struct hte_device *gdev;
>> + struct hte_ts_info *ei;
>> + int ret;
>> +
>> + if (!desc)
>> + return -EINVAL;
>> +
>> + ei = (struct hte_ts_info *)desc->data_subsys;
> As above, no need to cast - though it rather implies the type of data_subsys
> should not be void *.
desc is public facing structure, I wanted to make subsystem related
information opaque that is why I had it void *.
>
>> +
>> + if (!ei || !ei->gdev)
>> + return -EINVAL;
>> +
>> + gdev = ei->gdev;
>> + ts_id = desc->con_id;
>> +
>> + mutex_lock(&ei->mlock);
>> +
>> + if (!test_bit(HTE_TS_REGISTERED, &ei->flags)) {
>> + dev_dbg(gdev->sdev, "id:%d is not registered", ts_id);
>> + ret = -EUSERS;
>> + goto out;
>> + }
>> +
>> + if (en) {
>> + if (!test_bit(HTE_TS_DISABLE, &ei->flags)) {
>> + ret = 0;
>> + goto out;
>> + }
>> + ret = gdev->chip->ops->enable(gdev->chip, ei->xlated_id);
>> + if (ret) {
>> + dev_warn(gdev->sdev, "id: %d enable failed\n",
>> + ts_id);
>> + goto out;
>> + }
>> +
>> + clear_bit(HTE_TS_DISABLE, &ei->flags);
>> + ret = 0;
> ret is already 0 so no point in setting it again.
Yes, will clean up.
>
>> + } else {
>> + if (test_bit(HTE_TS_DISABLE, &ei->flags)) {
>> + ret = 0;
>> + goto out;
>> + }
>> + ret = gdev->chip->ops->disable(gdev->chip, ei->xlated_id);
>> + if (ret) {
>> + dev_warn(gdev->sdev, "id: %d disable failed\n",
>> + ts_id);
>> + goto out;
>> + }
>> +
>> + set_bit(HTE_TS_DISABLE, &ei->flags);
>> + ret = 0;
>> + }
>> +
>> +out:
>> + mutex_unlock(&ei->mlock);
>> + return ret;
>> +}
>> +
>> +/**
>> + * hte_disable_ts() - Disable timestamp on given descriptor.
>> + *
>> + * @desc: ts descriptor, this is the same as returned by the request API.
>> + *
>> + * Context: Holds mutex lock, not suitable from atomic context.
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int hte_disable_ts(struct hte_ts_desc *desc)
>> +{
>> + return hte_ts_dis_en_common(desc, false);
>> +}
>> +EXPORT_SYMBOL_GPL(hte_disable_ts);
>> +
>> +/**
>> + * hte_enable_ts() - Enable timestamp on given descriptor.
>> + *
>> + * @desc: ts descriptor, this is the same as returned by the request API.
>> + *
>> + * Context: Holds mutex lock, not suitable from atomic context.
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int hte_enable_ts(struct hte_ts_desc *desc)
>> +{
>> + return hte_ts_dis_en_common(desc, true);
>> +}
>> +EXPORT_SYMBOL_GPL(hte_enable_ts);
>> +
>> +static int hte_simple_xlate(struct hte_chip *gc,
>> + const struct of_phandle_args *args,
>> + struct hte_ts_desc *desc,
>> + u32 *id)
>> +{
>> + if (!id || !desc || !gc)
>> + return -EINVAL;
>> +
>> + /*
>> + * For the providers which do not have any internal mappings between
>> + * logically exposed ids and actual ids, will set both
>> + * the same.
>> + *
>> + * In case there is a internal mapping needed, providers will need to
>> + * provide its own xlate function where con_id will be sent as
>> + * args[0] and it will return xlated id. Later xlated id will be
>> + * used for any future exchanges between provider and subsystems.
>> + */
>> +
>> + if (args) {
>> + if (gc->of_hte_n_cells < 1)
>> + return -EINVAL;
>> +
>> + if (args->args_count != gc->of_hte_n_cells)
>> + return -EINVAL;
>> +
>> + *id = args->args[0];
>> + desc->con_id = *id;
>> + } else {
>> + *id = desc->con_id;
>> + }
>> +
>> + if (desc->con_id > gc->nlines)
>> + return -EINVAL;
>> +
>> + desc->data_subsys = NULL;
>> +
>> + return 0;
>> +}
>> +
>> +static struct hte_device *of_node_to_htedevice(struct device_node *np)
>> +{
>> + struct hte_device *gdev;
>> +
>> + spin_lock(&hte_lock);
>> +
>> + list_for_each_entry(gdev, &hte_devices, list)
>> + if (gdev->chip && gdev->chip->dev &&
>> + gdev->chip->dev->of_node == np) {
>> + spin_unlock(&hte_lock);
>> + return gdev;
>> + }
>> +
>> + spin_unlock(&hte_lock);
>> +
>> + return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static int ___hte_req_ts(struct hte_device *gdev, struct hte_ts_desc *desc,
>> + u32 xlated_id, void (*cb)(enum hte_notify n))
>> +{
>> + struct hte_ts_info *ei;
>> + struct hte_ts_buf *buf;
>> + int ret;
>> + u32 con_id = desc->con_id;
>> +
>> + if (!try_module_get(gdev->owner))
>> + return -ENODEV;
>> +
>> + ei = &gdev->ei[xlated_id];
>> + ei->xlated_id = xlated_id;
>> +
>> + /*
>> + * There a chance that multiple consumers requesting same entity,
>> + * lock here.
>> + */
>> + mutex_lock(&ei->mlock);
>> +
>> + if (test_bit(HTE_TS_REGISTERED, &ei->flags)) {
>> + dev_dbg(gdev->chip->dev, "id:%u is already registered",
>> + xlated_id);
>> + ret = -EUSERS;
>> + goto unlock;
>> + }
>> +
>> + buf = hte_ts_buf_allocate();
>> + if (IS_ERR(buf)) {
>> + dev_err(gdev->chip->dev, "Buffer allocation failed");
>> + ret = PTR_ERR(buf);
>> + goto unlock;
>> + }
>> +
>> + /* Set default here, let consumer decide how much to set later */
>> + ret = buf->access->set_length(buf, HTE_EV_FIFO_EL,
>> + sizeof(struct hte_ts_data));
>> +
> It's good to keep to consistent style of no line break between a statement
> and it's error check.
Sure...
>
>> + if (ret) {
>> + dev_err(gdev->chip->dev, "Fifo set length failed");
>> + goto buf_rel;
>> + }
>> +
>> + buf->access->reset(buf);
>> + buf->valid = true;
>> +
>> + ei->buf = buf;
>> + ei->cb = cb;
>> +
>> + ret = gdev->chip->ops->request(gdev->chip, xlated_id);
>> + if (ret < 0) {
>> + dev_err(gdev->chip->dev, "ts request failed\n");
>> + goto buf_rel;
>> + }
>> +
>> + desc->data_subsys = ei;
>> + ei->desc = desc;
>> +
>> + atomic_inc(&gdev->ts_req);
>> + set_bit(HTE_TS_REGISTERED, &ei->flags);
>> + mutex_unlock(&ei->mlock);
>> +
>> + if (!desc->name) {
>> + desc->name = kzalloc(HTE_TS_NAME_LEN, GFP_KERNEL);
>> + if (desc->name)
>> + scnprintf(desc->name, HTE_TS_NAME_LEN, "ts_%u",
>> + con_id);
>> + }
>> +
>> + hte_ts_dbgfs_init(desc->name, ei);
>> +
>> + dev_dbg(gdev->chip->dev, "%s: id: %u, xlated id:%u",
>> + __func__, con_id, xlated_id);
>> +
>> + return 0;
>> +
>> +buf_rel:
>> + buf->access->release(buf);
>> +unlock:
>> + module_put(gdev->owner);
>> + mutex_unlock(&ei->mlock);
>> +
>> + return ret;
>> +}
>> +
>> +static struct hte_device *of_hte_dev_get(struct device *dev,
>> + struct device_node *np,
>> + const char *label,
>> + struct of_phandle_args *args)
>> +{
>> + struct hte_device *gdev = NULL;
>> + int index = 0;
>> + int err;
>> +
>> + if (label) {
>> + index = of_property_match_string(np, "hte-names", label);
>> + if (index < 0)
>> + return ERR_PTR(index);
>> + }
>> +
>> + err = of_parse_phandle_with_args(np, "htes", "#hte-cells", index,
>> + args);
>> + if (err) {
>> + pr_err("%s(): can't parse \"htes\" property\n", __func__);
>> + return ERR_PTR(err);
>> + }
>> +
>> + gdev = of_node_to_htedevice(args->np);
>> + if (IS_ERR(gdev)) {
>> + pr_err("%s(): HTE chip not found\n", __func__);
>> + of_node_put(args->np);
>> + return gdev;
>> + }
>> +
>> + return gdev;
>> +}
>> +
>> +static struct hte_ts_desc *__hte_req_ts(struct device *dev,
>> + struct device_node *np,
>> + const char *label,
>> + void (*cb)(enum hte_notify n))
>> +{
>> + struct hte_device *gdev = NULL;
>> + struct hte_ts_desc *desc;
>> + struct of_phandle_args args;
>> + int ret;
>> + u32 xlated_id;
>> +
>> + gdev = of_hte_dev_get(dev, np, label, &args);
>> + if (IS_ERR(gdev))
>> + return ERR_CAST(gdev);
>> +
>> + if (!gdev->chip) {
>> + pr_debug("requested id does not have provider\n");
>> + return ERR_PTR(-ENODEV);
>> + }
>> +
>> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>> + if (!desc)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + ret = gdev->chip->xlate(gdev->chip, &args, desc, &xlated_id);
>> + if (ret < 0)
>> + goto put;
>> +
>> + desc->name = NULL;
>> + if (label)
>> + desc->name = kstrdup(label, GFP_KERNEL);
>> +
>> + ret = ___hte_req_ts(gdev, desc, xlated_id, cb);
>> + if (ret < 0)
>> + goto put;
>> +
>> + return desc;
>> +
>> +put:
>> + of_node_put(args.np);
>> + kfree(desc);
>> +
>> + return ERR_PTR(ret);
>> +}
>> +
>> +/**
>> + * of_hte_request_ts() - Consumer calls this API to request the HTE facility
>> + * on the specified entity, where entity is provider specific for example,
>> + * GPIO lines, signals, buses etc...
>> + *
>> + * @dev: Consumer device.
>> + * @label: Optional label.
>> + * @cb: Optional notify callback to consumer when data is pushed by the
>> + * provider.
>> + *
>> + * Context: Holds mutex lock, not suitable from atomic context.
>> + * Returns: Timestamp descriptor on success or error ptr on failure.
>> + */
>> +struct hte_ts_desc *of_hte_request_ts(struct device *dev,
>> + const char *label,
>> + void (*cb)(enum hte_notify n))
>> +{
>> +
>> + if (dev && dev->of_node)
>> + return __hte_req_ts(dev, dev->of_node, label, cb);
>> + else
>> + return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +EXPORT_SYMBOL_GPL(of_hte_request_ts);
>> +
>> +static int devm_hte_ts_match_desc(struct device *dev, void *res, void *data)
> I'm not seeing what is devm about this.
It was part of devm release. However I am planing to remove devm release
function, in turns this will be deleted as well.
>
>> +{
>> + struct hte_ts_desc **p = res;
>> +
>> + if (WARN_ON(!p || !*p))
>> + return 0;
>> +
>> + return *p == data;
>> +}
>> +
>> +static void __devm_hte_release_ts(struct device *dev, void *res)
>> +{
>> + hte_release_ts(*(struct hte_ts_desc **)res);
>> +}
>> +
>> +/**
>> + * devm_hte_release_ts() - Resource managed hte_release_ts().
> I'd not introduce this until you have a user. It very rarely actually makes
> sense to call a devm release manually. Not having one makes people think harder
> about it.
Will remove.
>
>> + * @dev: HTE consumer/client device.
>> + * @desc: HTE ts descriptor.
>> + *
>> + * Release timestamp functionality and its resources previously allocated using
>> + * of_hte_request_ts(). Calling this function is usually not needed because
>> + * devm-allocated resources are automatically released on driver detach.
>> + *
>> + * Context: Same as hte_release_ts() function.
>> + * Returns: 0 on success otherwise negative error code.
>> + */
>> +int devm_hte_release_ts(struct device *dev, struct hte_ts_desc *desc)
>> +{
>> + return devres_release(dev, __devm_hte_release_ts,
>> + devm_hte_ts_match_desc, desc);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_hte_release_ts);
>> +
>> +/**
>> + * devm_of_hte_request_ts() - Resource managed of_hte_request_ts().
> If it's kernel-doc it needs to give no warnings when you point the kernel-doc
> scripts at it. They insist on full parameter documentation.
>
>> + */
>> +struct hte_ts_desc *devm_of_hte_request_ts(struct device *dev,
>> + const char *label,
>> + void (*cb)(enum hte_notify n))
>> +{
>> +
>> + struct hte_ts_desc **ptr, *desc;
>> +
>> + ptr = devres_alloc(__devm_hte_release_ts, sizeof(*ptr), GFP_KERNEL);
> Superficially looks like you might get way with just calling dev_add_action_or_reset() in here
> and avoid this boilerplate. A lot of cases that looked like this got cleaned up in the
> last kernel cycle.
I based my patches from linux-next/master. Not sure if that has
dev_add_action_or_reset
>
>
>> + if (!ptr)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + desc = of_hte_request_ts(dev, label, cb);
>> + if (!IS_ERR(desc)) {
>> + *ptr = desc;
>> + devres_add(dev, ptr);
>> + } else {
>> + devres_free(ptr);
>> + }
>> + return desc;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_of_hte_request_ts);
>> +
>> +static struct hte_ts_info *hte_para_check(const struct hte_ts_desc *desc,
>> + size_t val)
> Not a good name or indeed combination of different things.
> hte_desc_to_info() and some separate check on val would be better.
Good suggestion, will follow new name.
>
>> +{
>> + struct hte_ts_info *ei;
>> +
>> + if (!desc || !desc->data_subsys || !val) {
>> + pr_debug("%s:%d: val :%lu\n", __func__, __LINE__, val);
>> + return NULL;
>> + }
>> +
>> + ei = desc->data_subsys;
>> + if (!ei || !ei->buf) {
>> + pr_debug("%s:%d\n", __func__, __LINE__);
>> + return NULL;
>> + }
>> +
>> + return ei;
>> +}
>> +
>> +static inline bool hte_ts_buf_wait(struct hte_ts_buf *buffer, size_t to_read)
>> +{
>> + size_t el_avail;
>> +
>> + el_avail = buffer->access->el_available(buffer);
>> +
>> + return (el_avail >= to_read) ? false : true;
> return el_avail < to_read;
Sure...
>
>> +}
>> +
>> +static int _hte_retrieve_ts_ns(const struct hte_ts_desc *desc,
>> + struct hte_ts_data *el, size_t n, bool block)
>> +{
>> + struct hte_ts_buf *buffer;
>> + struct hte_ts_info *ei;
>> + int ret;
>> + size_t to_read, copied;
>> +
>> + ei = hte_para_check(desc, n);
>> + if (!ei)
>> + return -EINVAL;
>> +
>> + buffer = ei->buf;
>> +
>> + to_read = min_t(size_t, n, buffer->watermark);
> Needs a comment as not obvious why you'd read the min of that requested or
> the watermark if there might be more available.
I will add detailed comment in next version of the RFC.
>
>> +
>> + do {
>> + if (hte_ts_buf_wait(buffer, to_read)) {
>> + if (!block) {
>> + /* Possibly early here to retrieve, try again */
>> + dev_dbg(ei->gdev->chip->dev, "%s: %d\n",
>> + __func__, ret);
>> + return -EAGAIN;
>> + }
>> + ret = wait_event_interruptible(buffer->pollq,
>> + !hte_ts_buf_wait(buffer, to_read));
>> + if (ret)
>> + return ret;
>> + }
>> + ret = buffer->access->read(buffer, (void *)el,
> If you have to cast to a void * that usually means something is wrong in your definitions.
> Why is it needed here? Looks like read has an inappropriate definition.
Will change to void* in read definitions to remove this cast.
>
>> + n * buffer->bytes_per_datum,
>> + &copied);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (copied > 0)
>> + return 0;
>> + else if (copied == 0 && !block)
>> + return -EAGAIN;
>> + } while (copied == 0);
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * hte_retrieve_ts_ns() - Consumer calls this API to retrieve timestamp in
>> + * nano seconds i.e. el->tsc will be in ns.
>> + *
>> + * @desc: ts descriptor, same as returned from request API.
>> + * @el: buffer to store the timestamp details.
>> + * @n: Number of struct hte_timestamp_el elements.
>> + *
>> + * Context: Can be called from the atomic context.
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int hte_retrieve_ts_ns(const struct hte_ts_desc *desc,
>> + struct hte_ts_data *el, size_t n)
>> +{
>> + return _hte_retrieve_ts_ns(desc, el, n, false);
>> +}
>> +EXPORT_SYMBOL_GPL(hte_retrieve_ts_ns);
>> +
>> +/**
>> + * hte_retrieve_ts_ns_wait() - Blocking version of the hte_retrieve_ts_ns.
>> + * @desc: ts descriptor, same as returned from request API.
>> + * @el: buffer to store the timestamp data.
>> + * @n: Number of struct hte_ts_data data.
>> + *
>> + * Context: Can not be called from the atomic context.
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int hte_retrieve_ts_ns_wait(const struct hte_ts_desc *desc,
>> + struct hte_ts_data *el, size_t n)
>> +{
>> + return _hte_retrieve_ts_ns(desc, el, n, true);
>> +}
>> +EXPORT_SYMBOL_GPL(hte_retrieve_ts_ns_wait);
>> +
>> +/**
>> + * hte_set_buf_len() - Consumer calls this API to set timestamp software buffer
>> + * depth.
>> + *
>> + * @desc: ts descriptor, same as returned from request API.
>> + * @len: New length/depth.
>> + *
>> + * The correct sequence to set buffer length is as below:
>> + * 1) Disable timestamp by calling hte_disable_ts API.
>> + * 2) Optionally retrieve all the timestamps by calling non blocking
>> + * hte_retrieve_ts_ns() API. This step only needed if you still care about
>> + * the data.
>> + * 3) Call this API.
>> + * 4) Enable timestamp by calling hte_enable_ts API.
>> + *
>> + * This API destroys previously allocated buffer and creates new one, because
>> + * of that, it is mandatory to follow above sequence to make sure there is no
>> + * race between various other APIs in the subsystem.
> Good docs. This is why I mentioned in review of docs patch that it is better
> to just have that refer to the kernel-doc in these files. Keep all this good
> information in one place.
Agree..
>
>> + *
>> + * By default during the request API call, HTE subsystem allocates software
>> + * buffer with predefined length, this API gives flexibility to adjust the
>> + * length according to consumer's need.
>> + *
>> + * Context: Can not be called from atomic context.
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int hte_set_buf_len(const struct hte_ts_desc *desc, size_t len)
>> +{
>> + struct hte_ts_buf *buffer;
>> + struct hte_ts_info *ei;
>> + int ret;
>> +
>> + ei = hte_para_check(desc, len);
>> + if (!ei)
>> + return -EINVAL;
>> +
>> + buffer = ei->buf;
>> + ret = buffer->access->set_length(buffer, len,
>> + sizeof(struct hte_ts_data));
>> + if (ret)
>> + dev_err(ei->gdev->chip->dev, "%s: ret:%d\n", __func__, ret);
> Not point in printing things line __func__ manually in dev_err() etc.
> Dynamic debug includes that and gives far more information + control of this.
yes, will remove.
>
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(hte_set_buf_len);
>> +
>> +/**
>> + * hte_get_buf_len() - Consumer calls this API to get timestamp software buffer
>> + * depth or length.
>> + *
>> + * @desc: ts descriptor, same as returned from request API.
>> + *
>> + * Context: Any context.
>> + * Returns: Positive length on success or 0 on failure.
>> + */
>> +size_t hte_get_buf_len(const struct hte_ts_desc *desc)
>> +{
>> + struct hte_ts_buf *buffer;
>> + struct hte_ts_info *ei;
>> +
>> + ei = hte_para_check(desc, 1);
>> + if (!ei)
>> + return 0;
>> +
>> + buffer = ei->buf;
>> +
>> + return buffer->access->get_length(buffer);
>> +}
>> +EXPORT_SYMBOL_GPL(hte_get_buf_len);
>> +
>> +/**
>> + * hte_available_ts() - Returns total available timestamps.
>> + *
>> + * @desc: ts descriptor, same as returned from request API.
>> + *
>> + * The API helps consumers to pre-allocate its internal buffer required
>> + * during hte_retrieve_ts_ns call.
>> + *
>> + * Context: Any context.
>> + * Returns: Positive value if elements are available else 0. The value is
>> + * number of total available struct hte_timestamp_el elements available not
>> + * the size in bytes.
>> + */
>> +size_t hte_available_ts(const struct hte_ts_desc *desc)
>> +{
>> + struct hte_ts_buf *buffer;
>> + struct hte_ts_info *ei;
>> +
>> + ei = hte_para_check(desc, 1);
>> + if (!ei)
>> + return 0;
>> +
>> + buffer = ei->buf;
>> +
>> + return buffer->access->el_available(buffer);
>> +}
>> +EXPORT_SYMBOL_GPL(hte_available_ts);
>> +
>> +/**
>> + * hte_set_buf_watermark() - Consumer calls this API to set timestamp software
>> + * buffer watermark. The correct sequence to call this API is as below:
>> + * 1) Disable timestamp by calling hte_disable_ts API.
>> + * 2) Call this API.
>> + * 3) Enable timestamp by calling hte_enable_ts API.
>> + *
>> + * @desc: ts descriptor, same as returned from request API.
>> + * @val: New watermark.
>> + *
>> + * By default during the request API call, HTE subsystem sets watermark as 1,
>> + * this API gives flexibility to adjust the watermark according to consumer's
>> + * need. The consumers will get notification through callback registered during
>> + * request API either when timestamp is dropped or watermark is reached or will
>> + * wait till watermark is reached. Refer hte_retrieve_ts_ns() and
>> + * hte_push_ts_ns_atomic() APIs to understand how watermark is used.
>> + *
>> + * Context: Any context.
> You have no way of knowing that as will depend on the driver - I'd definitely
> suggest not from atomic context, but then that would be crazy so you are better
> off not documenting any specific requirement at all.
set watermark does not talk to driver, at least for now as it is totally software
managed buffer from the hte core. I will remove context.
>
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int hte_set_buf_watermark(const struct hte_ts_desc *desc, size_t val)
>> +{
>> + struct hte_ts_buf *buffer;
>> + struct hte_ts_info *ei;
>> + int ret;
>> +
>> + ei = hte_para_check(desc, val);
>> + if (!ei)
>> + return -EINVAL;
>> +
>> + buffer = ei->buf;
>> + ret = buffer->access->set_watermark(buffer, val);
>> + if (ret)
>> + dev_dbg(ei->gdev->chip->dev, "%s: ret:%d\n", __func__, ret);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(hte_set_buf_watermark);
>> +
>> +/**
>> + * hte_get_buf_watermark() - Consumer calls this API to get software
>> + * buffer watermark.
>> + * @desc: ts descriptor, same as returned from request API.
>> + *
>> + * Context: Any context.
>> + * Returns: Positive current watermark on success or 0 on failure.
>> + */
>> +size_t hte_get_buf_watermark(const struct hte_ts_desc *desc)
>> +{
>> + struct hte_ts_buf *buffer;
>> + struct hte_ts_info *ei;
>> +
>> + ei = hte_para_check(desc, 1);
>> + if (!ei)
>> + return 0;
>> +
>> + buffer = ei->buf;
>> +
>> + return buffer->access->get_watermark(buffer);
>> +}
>> +EXPORT_SYMBOL_GPL(hte_get_buf_watermark);
>> +
>> +/**
>> + * hte_req_ts_by_dt_node() - Request entity to monitor by passing HTE device
>> + * node directly, where meaning of the entity is provider specific, for example
>> + * lines, signals, GPIOs, buses etc...
>> + *
>> + * @of_node: HTE provider device node.
>> + * @id: entity id to monitor, this id belongs to HTE provider of_node.
>> + * @cb: Optional callback to notify.
>> + *
>> + * Context: Holds mutex lock, can not be called from atomic context.
> What mutex and why? If it is one you can check is held even better.
___hte_req_ts holds the mutex lock to serialize multiple consumers
requesting same entity.
>
>> + * Returns: ts descriptor on success or error pointers.
>> + */
>> +struct hte_ts_desc *hte_req_ts_by_dt_node(struct device_node *of_node,
>> + unsigned int id,
>> + void (*cb)(enum hte_notify n))
>> +{
>> + struct hte_device *gdev;
>> + struct hte_ts_desc *desc;
>> + int ret;
>> + u32 xlated_id;
>> +
>> + gdev = of_node_to_htedevice(of_node);
>> + if (IS_ERR(gdev))
>> + return ERR_PTR(-ENOTSUPP);
>> +
>> + if (!gdev->chip || !gdev->chip->ops)
>> + return ERR_PTR(-ENOTSUPP);
>> +
>> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>> + if (!desc) {
>> + ret = -ENOMEM;
>> + goto out_put_device;
>> + }
> Pass a desc pointer into this function rather than allocating the structure
> in here. That lets the caller embed that structure inside one of it's own
> structures if it wants to, resulting in fewer small allocations which is always good.
>
> It's far from obvious that the caller needs to free desc.
Are you suggesting to shift burden of allocation/deallocation (static or dynamic)
at client/consumer side?
>
>> +
>> + desc->con_id = id;
>> + ret = gdev->chip->xlate(gdev->chip, NULL, desc, &xlated_id);
>> + if (ret < 0) {
>> + dev_err(gdev->chip->dev,
>> + "failed to xlate id: %d\n", id);
>> + goto out_free_desc;
>> + }
>> +
>> + ret = ___hte_req_ts(gdev, desc, xlated_id, cb);
>> + if (ret < 0) {
>> + dev_err(gdev->chip->dev,
>> + "failed to request id: %d\n", id);
>> + goto out_free_desc;
>> + }
>> +
>> + return desc;
>> +
>> +out_free_desc:
>> + kfree(desc);
>> +
>> +out_put_device:
>> + return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(hte_req_ts_by_dt_node);
>> +
>> +/**
>> + * hte_get_clk_src_info() - Consumer calls this API to query clock source
>> + * information of the desc.
>> + *
>> + * @desc: ts descriptor, same as returned from request API.
>> + *
>> + * Context: Any context.
>> + * Returns: 0 on success else negative error code on failure.
>> + */
>> +int hte_get_clk_src_info(const struct hte_ts_desc *desc,
>> + struct hte_clk_info *ci)
>> +{
>> + struct hte_chip *chip;
>> + struct hte_ts_info *ei;
>> +
>> + if (!desc || !desc->data_subsys || !ci) {
>> + pr_debug("%s:%d\n", __func__, __LINE__);
>> + return -EINVAL;
>> + }
>> +
>> + ei = desc->data_subsys;
>> + if (!ei || !ei->gdev || !ei->gdev->chip)
>> + return -EINVAL;
>> +
>> + chip = ei->gdev->chip;
>> + if (!chip->ops->get_clk_src_info)
>> + return -ENOTSUPP;
>> +
>> + return chip->ops->get_clk_src_info(chip, ci);
>> +}
>> +EXPORT_SYMBOL_GPL(hte_get_clk_src_info);
>> +
>> +static inline void hte_add_to_device_list(struct hte_device *gdev)
>> +{
>> + struct hte_device *prev;
> Needs to take an appropriate lock as you may have concurrent calls.
There is spin_lock held from register API from where this gets
called.
>
>> +
>> + if (list_empty(&hte_devices)) {
>> + list_add_tail(&gdev->list, &hte_devices);
> Needs a comment. I've no idea why you might want to only add it if there were
> no other hte_devices already there.
>
>> + return;
>> + }
>> +
>> + prev = list_last_entry(&hte_devices, struct hte_device, list);
> Why woud you do this?
Thanks for pointing out. I definitely missed cleaning this up. Now, I will
remove this function in next RFC version as one line can be added directly
in register API.
>
>> + list_add_tail(&gdev->list, &hte_devices);
>> +}
>> +
>> +/**
>> + * hte_push_ts_ns_atomic() - Used by the provider to push timestamp in nano
>> + * seconds i.e data->tsc will be in ns, it is assumed that provider will be
>> + * using this API from its ISR or atomic context.
>> + *
>> + * @chip: The HTE chip, used during the registration.
>> + * @xlated_id: entity id understood by both subsystem and provider, usually this
>> + * is obtained from xlate callback during request API.
>> + * @data: timestamp data.
>> + * @n: Size of the data.
>> + *
>> + * Context: Atomic.
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int hte_push_ts_ns_atomic(const struct hte_chip *chip, u32 xlated_id,
>> + struct hte_ts_data *data, size_t n)
>> +{
>> + unsigned int ret;
>> + bool notify;
>> + size_t el_avail;
>> + struct hte_ts_buf *buffer;
>> + struct hte_ts_info *ei;
>> +
>> + if (!chip || !data || !chip->gdev)
>> + return -EINVAL;
>> +
>> + if (xlated_id > chip->nlines)
>> + return -EINVAL;
>> +
>> + ei = &chip->gdev->ei[xlated_id];
>> +
>> + if (!test_bit(HTE_TS_REGISTERED, &ei->flags) ||
>> + test_bit(HTE_TS_DISABLE, &ei->flags)) {
>> + dev_dbg(chip->dev, "Unknown timestamp push\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* timestamp sequence counter, start from 0 */
>> + data->seq = ei->seq++;
>> +
>> + buffer = ei->buf;
>> + el_avail = buffer->access->el_available(buffer);
>> + ret = buffer->access->store(buffer, data, n);
> If we are doing this from the hte core, why is buffer definition in the scope of the
> drivers rather than the core? That seems backwards to me.
I do not understand this comment. The buffer definition is in scope of hte core
as it is the only entity that manages it.
>
>> + if (ret != n) {
>> + atomic_inc(&ei->dropped_ts);
>> + if (ei->cb)
>> + ei->cb(HTE_TS_DROPPED);
>> + return -ENOMEM;
>> + }
>> +
>> + notify = ((el_avail + 1) >= buffer->watermark) ? true : false;
> You push n but only check on el_avail + 1 here.
> Also, this is the same as
Good catch, will correct that.
>
> notify = ((el_avail + 1) >= buffer->watermark;
right, not sure why do I have obsession with ternary operators :)
>
>
>> +
>> + /*
>> + * If there is a callback, its consumer's job to retrieve the timestamp.
>> + * For the rest, wake up the process.
>> + */
>> + if (notify && ei->cb) {
>> + ei->cb(HTE_TS_AVAIL);
>> + return 0;
> Given you return 0 anyway, might as well not have this line.
True, will remove.
>
>> + } else if (notify) {
>> + wake_up_interruptible(&buffer->pollq);
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(hte_push_ts_ns_atomic);
>> +
>> +/**
>> + * hte_register_chip() - Used by provider to register a HTE chip.
>> + * @chip: the HTE chip to add to subsystem.
>> + *
>> + * Context: Can not be called from atomic context.
> Whilst true, I'd think that was common sense as it would be insane
> to register something like this from atomic context. So I'd say no
> need to comment on it! Keep those comments for things that
> might be used like that.
Will remove...
>
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int hte_register_chip(struct hte_chip *chip)
>> +{
>> + struct hte_device *gdev;
>> + int ret;
>> + u32 i;
>> +
>> + if (!chip || !chip->dev || !chip->dev->of_node)
>> + return -EINVAL;
>> +
>> + if (!chip->ops || !chip->ops->request || !chip->ops->release) {
>> + dev_err(chip->dev, "Driver needs to provide ops\n");
>> + return -EINVAL;
>> + }
>> +
>> + gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
>> + if (!gdev)
>> + return -ENOMEM;
>> +
>> + gdev->chip = chip;
>> + chip->gdev = gdev;
>> + gdev->nlines = chip->nlines;
>> + gdev->sdev = chip->dev;
>> +
>> + /*
>> + * Allocate all the supported entities here at once, this will have
>> + * following advantages:
>> + * When provider pushes timestamp, it can then just send the
>> + * xlated_id, subsystem will use it as an index which
>> + * gives us the constant time access; this is important as mostly
>> + * providers will be pushing the timestamps from their ISR.
>> + */
>> + gdev->ei = kcalloc(chip->nlines, sizeof(struct hte_ts_info),
>> + GFP_KERNEL);
> I'd be tempted to do this as a 0 length element at the end of gdev
> then do the allocation in one go use struct_size() etc to work out
> how long it is. Cuts down on allocations + error paths to deal with
> for no obvious disadvantage.
Sure...
>
>> + if (!gdev->ei) {
>> + ret = -ENOMEM;
>> + goto err_free_gdev;
>> + }
>> +
>> + for (i = 0; i < chip->nlines; i++) {
>> + gdev->ei[i].flags = 0;
> zero allocated, so don't bother setting things to 0 where it's a fairly obvious
> base state. If you set something to 0 to act as some form of documentation then
> that's fine, but I don't think that's true here.
True, will remove.
>
>> + gdev->ei[i].gdev = gdev;
>> + gdev->ei[i].seq = 0;
>> + mutex_init(&gdev->ei[i].mlock);
>> + }
>> +
>> + if (chip->dev->driver)
>> + gdev->owner = chip->dev->driver->owner;
>> + else
>> + gdev->owner = THIS_MODULE;
>> +
>> + if (!chip->xlate) {
>> + chip->xlate = hte_simple_xlate;
>> + /* Just a id number to monitor */
>> + chip->of_hte_n_cells = 1;
>> + }
>> +
>> + of_node_get(chip->dev->of_node);
>> +
>> + INIT_LIST_HEAD(&gdev->list);
>> +
>> + spin_lock(&hte_lock);
>> + hte_add_to_device_list(gdev);
>> + spin_unlock(&hte_lock);
>> +
>> + hte_chip_dbgfs_init(gdev);
>> +
>> + dev_dbg(chip->dev, "Added hte chip\n");
>> + return 0;
>> +
>> +err_free_gdev:
>> + kfree(gdev);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(hte_register_chip);
>> +
>> +/**
>> + * hte_unregister_chip() - Used by the provider to remove a HTE chip.
>> + * @chip: the HTE chip to remove.
>> + *
>> + * Context: Can not be called from atomic context.
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int hte_unregister_chip(struct hte_chip *chip)
>> +{
>> + struct hte_device *gdev = chip->gdev;
>> +
>> + spin_lock(&hte_lock);
>> + list_del(&gdev->list);
>> + spin_unlock(&hte_lock);
>> +
>> + gdev->chip = NULL;
>> +
>> + of_node_put(chip->dev->of_node);
>> + hte_dbgfs_deinit(gdev->dbg_root);
>> + kfree(gdev->ei);
>> + kfree(gdev);
>> +
>> + dev_dbg(chip->dev, "Removed hte chip\n");
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(hte_unregister_chip);
>> +
>> +/* Driver APIs ends */
> Don't bother with file layout type comments. They don't add that much and tend
> to rot horribly over time as people move code around in files.
Sure..
>
>> diff --git a/include/linux/hte.h b/include/linux/hte.h
>> new file mode 100644
>> index 000000000000..e1737579d4c4
>> --- /dev/null
>> +++ b/include/linux/hte.h
>> @@ -0,0 +1,278 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2021 NVIDIA Corporation
>> + *
>> + * Author: Dipen Patel <dipenp@...dia.com>
>> + */
>> +
>> +#ifndef __LINUX_HTE_H
>> +#define __LINUX_HTE_H
>> +
>> +struct hte_chip;
>> +struct hte_device;
>> +struct of_phandle_args;
>> +
>> +/**
>> + * Used by providers to indicate the direction of the timestamp.
>> + */
>> +#define HTE_EVENT_RISING_EDGE 0x1
>> +#define HTE_EVENT_FALLING_EDGE 0x2
> Use an enum rather than a define for this as it's a value that can take a
> set of distinct values. Also, provide a name for 'I've no idea' which
> I'm guessing is 0 currently.
I will change to enum.
>
>> +
>> +/**
>> + * struct hte_ts_data - HTE timestamp data.
>> + * The provider uses and fills timestamp related details during push_timestamp
>> + * API call. The consumer uses during retrieve_timestamp API call.
>> + *
>> + * @tsc: Timestamp value.
>> + * @seq: Sequence counter of the timestamps.
>> + * @dir: Direction of the event at the time of timestamp.
>> + */
>> +struct hte_ts_data {
>> + u64 tsc;
>> + u64 seq;
>> + int dir;
>> +};
>> +
>> +/**
>> + * struct hte_clk_info - Clock source info that HTE provider uses.
>> + * The provider uses hardware clock as a source to timestamp real time. This
>> + * structure presents the clock information to consumers.
>> + *
>> + * @hz: Clock rate in HZ, for example 1KHz clock = 1000.
>> + * @type: Clock type. CLOCK_* types.
> So this is something we got a it wrong in IIO. It's much better to define
> a subset of clocks that can be potentially used. There are some that make
> absolutely no sense and consumers really don't want to have to deal with them.
Is there anything I have to change here?
>
>> + */
>> +struct hte_clk_info {
>> + u64 hz;
>> + clockid_t type;
>> +};
>> +
>> +/**
>> + * HTE subsystem notifications for the consumers.
>> + *
>> + * @HTE_TS_AVAIL: Timestamps available notification.
>> + * @HTE_TS_DROPPED: Timestamps dropped notification.
> Something I've missed so far is whether drops are in a kfifo or a ring
> fashion. I'm guess that's stated somewhere, but it might be useful to have
> it here.
Dropped are from kfifo if kfifo does not have space.
>
>> + */
>> +enum hte_notify {
>> + HTE_TS_AVAIL = 1,
>> + HTE_TS_DROPPED,
>> + HTE_NUM_NOTIFIER,
>> +};
>> +
>> +/**
>> + * struct hte_ts_desc - HTE timestamp descriptor, this structure will be
>> + * communication token between consumers to subsystem and subsystem to
>> + * providers.
>> + *
>> + * @con_id: This is the same id sent in request APIs.
>> + * @name: Descriptive name of the entity that is being monitored for the
>> + * realtime timestamping.
>> + * @data_subsys: Subsystem's private data relate to requested con_id.
>> + */
>> +struct hte_ts_desc {
>> + u32 con_id;
>> + char *name;
>> + void *data_subsys;
>> +};
>> +
>> +/**
>> + * struct hte_ops - HTE operations set by providers.
>> + *
>> + * @request: Hook for requesting a HTE timestamp. Returns 0 on success,
>> + * non-zero for failures.
>> + * @release: Hook for releasing a HTE timestamp. Returns 0 on success,
>> + * non-zero for failures.
>> + * @enable: Hook to enable the specified timestamp. Returns 0 on success,
>> + * non-zero for failures.
>> + * @disable: Hook to disable specified timestamp. Returns 0 on success,
>> + * non-zero for failures.
>> + * @get_clk_src_info: Optional hook to get the clock information provider uses
>> + * to timestamp. Returns 0 for success and negative error code for failure. On
>> + * success HTE subsystem fills up provided struct hte_clk_info.
> Why optional? Consumers will probably need that information.
Sure, will remove optional. But you wrote "probably", that would make it
optional :).
>
>> + *
>> + * xlated_id parameter is used to communicate between HTE subsystem and the
>> + * providers. It is the same id returned during xlate API call and translated
>> + * by the provider. This may be helpful as both subsystem and provider locate
>> + * the requested entity in constant time, where entity could be anything from
>> + * lines, signals, events, buses etc.. that providers support.
>> + */
>> +struct hte_ops {
>> + int (*request)(struct hte_chip *chip, u32 xlated_id);
>> + int (*release)(struct hte_chip *chip, u32 xlated_id);
>> + int (*enable)(struct hte_chip *chip, u32 xlated_id);
>> + int (*disable)(struct hte_chip *chip, u32 xlated_id);
>> + int (*get_clk_src_info)(struct hte_chip *chip,
>> + struct hte_clk_info *ci);
>> +};
>> +
>> +/**
>> + * struct hte_chip - Abstract HTE chip structure.
>> + * @name: functional name of the HTE IP block.
>> + * @dev: device providing the HTE.
> Unclear naming. Is this the parent device, or one associated with the HTE itself?
> I'm guessing today you don't have one associated with the HTE, but it is plausible you
> might gain on in future to make it fit nicely in the device model as a function of another
> device.
This is provider's device, could be &pdev->dev or any dev provider deems fit hence the
generic name.
>
>> + * @ops: callbacks for this HTE.
>> + * @nlines: number of lines/signals supported by this chip.
>> + * @xlate: Callback which translates consumer supplied logical ids to
>> + * physical ids, return from 0 for the success and negative for the
>> + * failures. It stores (0 to @nlines) in xlated_id parameter for the success.
>> + * @of_hte_n_cells: Number of cells used to form the HTE specifier.
>> + * @gdev: HTE subsystem abstract device, internal to the HTE subsystem.
>> + * @data: chip specific private data.
>> + */
>> +struct hte_chip {
>> + const char *name;
>> + struct device *dev;
>> + const struct hte_ops *ops;
>> + u32 nlines;
>> + int (*xlate)(struct hte_chip *gc,
>> + const struct of_phandle_args *args,
>> + struct hte_ts_desc *desc, u32 *xlated_id);
>> + u8 of_hte_n_cells;
>> +
>> + /* only used internally by the HTE framework */
>> + struct hte_device *gdev;
>> + void *data;
>> +};
>> +
>> +#if IS_ENABLED(CONFIG_HTE)
>> +/* HTE APIs for the providers */
>> +int hte_register_chip(struct hte_chip *chip);
>> +int hte_unregister_chip(struct hte_chip *chip);
>> +int hte_push_ts_ns_atomic(const struct hte_chip *chip, u32 xlated_id,
>> + struct hte_ts_data *data, size_t n);
>> +
>> +/* HTE APIs for the consumers */
>> +
>> +int hte_release_ts(struct hte_ts_desc *desc);
>> +struct hte_ts_desc *of_hte_request_ts(struct device *dev, const char *label,
>> + void (*cb)(enum hte_notify n));
>> +
>> +struct hte_ts_desc *devm_of_hte_request_ts(struct device *dev,
>> + const char *label,
>> + void (*cb)(enum hte_notify n));
>> +struct hte_ts_desc *hte_req_ts_by_dt_node(struct device_node *of_node,
>> + unsigned int id,
>> + void (*cb)(enum hte_notify n));
>> +int devm_hte_release_ts(struct device *dev, struct hte_ts_desc *desc);
>> +int hte_retrieve_ts_ns(const struct hte_ts_desc *desc, struct hte_ts_data *el,
>> + size_t n);
>> +int hte_retrieve_ts_ns_wait(const struct hte_ts_desc *desc,
>> + struct hte_ts_data *el, size_t n);
>> +int hte_set_buf_len(const struct hte_ts_desc *desc, size_t len);
>> +size_t hte_get_buf_len(const struct hte_ts_desc *desc);
>> +int hte_set_buf_watermark(const struct hte_ts_desc *desc, size_t val);
>> +size_t hte_get_buf_watermark(const struct hte_ts_desc *desc);
>> +size_t hte_available_ts(const struct hte_ts_desc *desc);
>> +int hte_enable_ts(struct hte_ts_desc *desc);
>> +int hte_disable_ts(struct hte_ts_desc *desc);
>> +int hte_get_clk_src_info(const struct hte_ts_desc *desc,
>> + struct hte_clk_info *ci);
>> +
>>
Powered by blists - more mailing lists