Re: [Intel-gfx] [PATCH 1/6] relay: allow the use of const callback structs

2020-11-23 Thread Jani Nikula
On Thu, 19 Nov 2020, Christoph Hellwig  wrote:
> But taking one step back:  All instances implement create_buf_file
> and remove_buf_file, which makes sense as that is the prime aim
> of these methods.  So there is no point in making those optional.
> subbuf_start_callback is overriden by two instances, so making that
> optional totally makes sense.  buf_mapped and buf_unmapped are
> never overriden, so they should be removed entirely.
>
> More importantly there is no case that passes a NULL rchan_callbacks,
> which makes complete sense as it wouldn't even create a file.  So
> remove that case as well and just replace it with a sanity check in
> relay_open().

Many thanks for the feedback; sent v2 [1].

BR,
Jani.


[1] http://lore.kernel.org/r/cover.1606153547.git.jani.nik...@intel.com


-- 
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 1/6] relay: allow the use of const callback structs

2020-11-20 Thread Christoph Hellwig
> +/*
> + * rchan_callback wrappers. Call the callbacks if available, otherwise fall 
> back
> + * to default behaviour.
> + */

This adds an overly long line.  That being said this behavior is pretty
normal for kernel APIs, so I'm not even sure we need it at all.

> +
> +/*
> + * subbuf_start() callback.
> + */

and this one is for sure completley useless.  Same for all the other
similar ones.


But taking one step back:  All instances implement create_buf_file
and remove_buf_file, which makes sense as that is the prime aim
of these methods.  So there is no point in making those optional.
subbuf_start_callback is overriden by two instances, so making that
optional totally makes sense.  buf_mapped and buf_unmapped are
never overriden, so they should be removed entirely.

More importantly there is no case that passes a NULL rchan_callbacks,
which makes complete sense as it wouldn't even create a file.  So
remove that case as well and just replace it with a sanity check in
relay_open().

Please also add a patch to mark all rchan_callbacks instances const
while you're at it.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 1/6] relay: allow the use of const callback structs

2020-11-20 Thread Christoph Hellwig
On Thu, Nov 19, 2020 at 08:11:20AM +, Christoph Hellwig wrote:
> Please also add a patch to mark all rchan_callbacks instances const
> while you're at it.

Oops, I just noticed you actually sent that one.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH 1/6] relay: allow the use of const callback structs

2020-11-18 Thread Jani Nikula
None of the relay users require the use of mutable structs for
callbacks, however the relay code does. Instead of assigning default
callbacks when there is none, add callback wrappers to conditionally
call the client callbacks if available, and fall back to default
behaviour (typically no-op) otherwise.

This lets all relay users make their struct rchan_callbacks const data.

Cc: linux-bl...@vger.kernel.org
Cc: Jens Axboe 
Cc: ath...@lists.infradead.org
Cc: ath...@lists.infradead.org
Cc: Kalle Valo 
Cc: linux-wirel...@vger.kernel.org
Cc: QCA ath9k Development 
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Jani Nikula 
---
 include/linux/relay.h |   4 +-
 kernel/relay.c| 182 +++---
 2 files changed, 86 insertions(+), 100 deletions(-)

diff --git a/include/linux/relay.h b/include/linux/relay.h
index e13a333e7c37..7333909df65a 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -62,7 +62,7 @@ struct rchan
size_t subbuf_size; /* sub-buffer size */
size_t n_subbufs;   /* number of sub-buffers per buffer */
size_t alloc_size;  /* total buffer size allocated */
-   struct rchan_callbacks *cb; /* client callbacks */
+   const struct rchan_callbacks *cb; /* client callbacks, may be NULL */
struct kref kref;   /* channel refcount */
void *private_data; /* for user-defined data */
size_t last_toobig; /* tried to log event > subbuf size */
@@ -170,7 +170,7 @@ struct rchan *relay_open(const char *base_filename,
 struct dentry *parent,
 size_t subbuf_size,
 size_t n_subbufs,
-struct rchan_callbacks *cb,
+const struct rchan_callbacks *cb,
 void *private_data);
 extern int relay_late_setup_files(struct rchan *chan,
  const char *base_filename,
diff --git a/kernel/relay.c b/kernel/relay.c
index b08d936d5fa7..c53676f2d10f 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -27,13 +27,86 @@
 static DEFINE_MUTEX(relay_channels_mutex);
 static LIST_HEAD(relay_channels);
 
+/*
+ * rchan_callback wrappers. Call the callbacks if available, otherwise fall 
back
+ * to default behaviour.
+ */
+
+/*
+ * subbuf_start() callback.
+ */
+static int cb_subbuf_start(const struct rchan_callbacks *cb,
+  struct rchan_buf *buf,
+  void *subbuf,
+  void *prev_subbuf,
+  size_t prev_padding)
+{
+   if (cb && cb->subbuf_start)
+   return cb->subbuf_start(buf, subbuf, prev_subbuf, prev_padding);
+
+   if (relay_buf_full(buf))
+   return 0;
+
+   return 1;
+}
+
+/*
+ * buf_mapped() callback.
+ */
+static void cb_buf_mapped(const struct rchan_callbacks *cb,
+ struct rchan_buf *buf,
+ struct file *filp)
+{
+   if (cb && cb->buf_mapped)
+   cb->buf_mapped(buf, filp);
+}
+
+/*
+ * buf_unmapped() callback.
+ */
+static void cb_buf_unmapped(const struct rchan_callbacks *cb,
+   struct rchan_buf *buf,
+   struct file *filp)
+{
+   if (cb && cb->buf_unmapped)
+   cb->buf_unmapped(buf, filp);
+}
+
+/*
+ * create_buf_file_create() callback.
+ */
+static struct dentry *cb_create_buf_file(const struct rchan_callbacks *cb,
+const char *filename,
+struct dentry *parent,
+umode_t mode,
+struct rchan_buf *buf,
+int *is_global)
+{
+   if (cb && cb->create_buf_file)
+   return cb->create_buf_file(filename, parent, mode, buf, 
is_global);
+
+   return NULL;
+}
+
+/*
+ * remove_buf_file() callback.
+ */
+static int cb_remove_buf_file(const struct rchan_callbacks *cb,
+ struct dentry *dentry)
+{
+   if (cb && cb->remove_buf_file)
+   return cb->remove_buf_file(dentry);
+
+   return -EINVAL;
+}
+
 /*
  * close() vm_op implementation for relay file mapping.
  */
 static void relay_file_mmap_close(struct vm_area_struct *vma)
 {
struct rchan_buf *buf = vma->vm_private_data;
-   buf->chan->cb->buf_unmapped(buf, vma->vm_file);
+   cb_buf_unmapped(buf->chan->cb, buf, vma->vm_file);
 }
 
 /*
@@ -107,7 +180,7 @@ static int relay_mmap_buf(struct rchan_buf *buf, struct 
vm_area_struct *vma)
vma->vm_ops = &relay_file_mmap_ops;
vma->vm_flags |= VM_DONTEXPAND;
vma->vm_private_data = buf;
-   buf->chan->cb->buf_mapped(buf, filp);
+   cb_buf_mapped(buf->chan->cb, buf, filp);
 
return 0;
 }
@@ -264,70 +337,6 @@ EXPORT_SYMBOL_GPL(relay_buf_full);