Re: [PATCH 3/7] Functions to fetch POSIX dynamic clock object

[Thomas Gleixner]

On Thu, Oct 01 2020 at 22:51, Erez Geva wrote:
> Add kernel functions to fetch a pointer to a POSIX dynamic clock
> using a user file description dynamic clock ID.

And how is that supposed to work. What are the lifetime rules?
  
> +struct posix_clock *posix_clock_get_clock(clockid_t id)
> +{
> + int err;
> + struct posix_clock_desc cd;

The core code uses reverse fir tree ordering of variable declaration
based on the length:

struct posix_clock_desc cd;
int err;

> + /* Verify we use posix clock ID */
> + if (!is_clockid_fd_clock(id))
> + return ERR_PTR(-EINVAL);
> +
> + err = get_clock_desc(id, );

So this is a kernel interface and get_clock_desc() does:

struct file *fp = fget(clockid_to_fd(id));

How is that file descriptor valid in random kernel context?

> + if (err)
> + return ERR_PTR(err);
> +
> + get_device(cd.clk->dev);

The purpose of this is? Comments are overrated...

> + put_clock_desc();
> +
> + return cd.clk;
> +}
> +EXPORT_SYMBOL_GPL(posix_clock_get_clock);
> +
> +int posix_clock_put_clock(struct posix_clock *clk)
> +{
> + if (IS_ERR_OR_NULL(clk))
> + return -EINVAL;
> + put_device(clk->dev);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(posix_clock_put_clock);
> +
> +int posix_clock_gettime(struct posix_clock *clk, struct timespec64 *ts)
> +{
> + int err;
> +
> + if (IS_ERR_OR_NULL(clk))
> + return -EINVAL;
> +
> + down_read(>rwsem);

Open coding the logic of get_posix_clock() and having a copy here and
in the next function is really useful.

Thanks,

tglx

[PATCH 3/7] Functions to fetch POSIX dynamic clock object

[Erez Geva]

Add kernel functions to fetch a pointer to a POSIX dynamic clock
using a user file description dynamic clock ID.

Signed-off-by: Erez Geva 
---
 include/linux/posix-clock.h | 39 +++
 kernel/time/posix-clock.c   | 76 +
 2 files changed, 115 insertions(+)

diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h
index 468328b1e1dd..e90bd90d3a01 100644
--- a/include/linux/posix-clock.h
+++ b/include/linux/posix-clock.h
@@ -116,4 +116,43 @@ int posix_clock_register(struct posix_clock *clk, struct 
device *dev);
  */
 void posix_clock_unregister(struct posix_clock *clk);
 
+/**
+ * posix_clock_get_clock() - get reference to a posix clock
+ * @id: A user clockid that uses a posix clock
+ *
+ * Used by kernel code to get a reference to a posix clock.
+ * Increase the reference count, ensure the referece is not removed.
+ */
+struct posix_clock *posix_clock_get_clock(clockid_t id);
+
+/**
+ * posix_clock_put_clock() - release a reference to a posix clock
+ * @clk: The reference to a posix clock to release
+ *
+ * Release a reference to a posix clock.
+ * Reduce the reference count.
+ */
+int posix_clock_put_clock(struct posix_clock *clk);
+
+/**
+ * posix_clock_gettime() - get time from posix clock
+ * @clk: A reference to a posix clock
+ * @ts: pointer to a time structure used to store the time from the posix clock
+ *
+ * Retrieve the time from a posix clock.
+ * In case the clock device was removed, the function return error.
+ */
+int posix_clock_gettime(struct posix_clock *clk, struct timespec64 *ts);
+
+/**
+ * posix_clock_adjtime() - get tune parameters from posix clock
+ * @clk: A reference to a posix clock
+ * @tx: pointer to a kernel timex structure used to store
+ *  the tune parameters from the posix clock
+ *
+ * Retrieve the tune parameters from a posix clock.
+ * In case the clock device was removed, the function return error.
+ */
+int posix_clock_adjtime(struct posix_clock *clk, struct __kernel_timex *tx);
+
 #endif
diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
index 77c0c2370b6d..1e205eea6ebd 100644
--- a/kernel/time/posix-clock.c
+++ b/kernel/time/posix-clock.c
@@ -315,3 +315,79 @@ const struct k_clock clock_posix_dynamic = {
.clock_get_timespec = pc_clock_gettime,
.clock_adj  = pc_clock_adjtime,
 };
+
+struct posix_clock *posix_clock_get_clock(clockid_t id)
+{
+   int err;
+   struct posix_clock_desc cd;
+
+   /* Verify we use posix clock ID */
+   if (!is_clockid_fd_clock(id))
+   return ERR_PTR(-EINVAL);
+
+   err = get_clock_desc(id, );
+   if (err)
+   return ERR_PTR(err);
+
+   get_device(cd.clk->dev);
+
+   put_clock_desc();
+
+   return cd.clk;
+}
+EXPORT_SYMBOL_GPL(posix_clock_get_clock);
+
+int posix_clock_put_clock(struct posix_clock *clk)
+{
+   if (IS_ERR_OR_NULL(clk))
+   return -EINVAL;
+   put_device(clk->dev);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(posix_clock_put_clock);
+
+int posix_clock_gettime(struct posix_clock *clk, struct timespec64 *ts)
+{
+   int err;
+
+   if (IS_ERR_OR_NULL(clk))
+   return -EINVAL;
+
+   down_read(>rwsem);
+
+   if (clk->zombie)
+   err = -ENODEV;
+   else if (clk->ops.clock_gettime)
+   err = clk->ops.clock_gettime(clk, ts);
+   else
+   err = -EOPNOTSUPP;
+
+   up_read(>rwsem);
+   return err;
+}
+EXPORT_SYMBOL_GPL(posix_clock_gettime);
+
+int posix_clock_adjtime(struct posix_clock *clk, struct __kernel_timex *tx)
+{
+   int err;
+
+   /* Allow read only */
+   if (tx->modes != 0)
+   return -EINVAL;
+
+   if (IS_ERR_OR_NULL(clk))
+   return -EINVAL;
+
+   down_read(>rwsem);
+
+   if (clk->zombie)
+   err = -ENODEV;
+   else if (clk->ops.clock_adjtime)
+   err = clk->ops.clock_adjtime(clk, tx);
+   else
+   err = -EOPNOTSUPP;
+
+   up_read(>rwsem);
+   return err;
+}
+EXPORT_SYMBOL_GPL(posix_clock_adjtime);
-- 
2.20.1