All of lore.kernel.org
 help / color / mirror / Atom feed
* [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events
@ 2016-05-23 12:49 Philippe Bergheaud
  2016-05-23 12:49 ` [v4,2/2] cxl: Add set and get private data to context struct Philippe Bergheaud
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Philippe Bergheaud @ 2016-05-23 12:49 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: mrochs, manoj, mpe, imunsie, mikey, Philippe Bergheaud

This adds an afu_driver_ops structure with deliver_event() and
event_delivered() callbacks. An AFU driver such as cxlflash can fill
this out and associate it with a context to enable passing custom
AFU specific events to userspace.

This also adds a new kernel API function cxl_context_pending_events(),
that the AFU driver can use to notify the cxl driver that new specific
events are ready to be delivered, and wake up anyone waiting on the
context wait queue.

The current count of AFU driver specific events is stored in the field
afu_driver_events of the context structure.

The cxl driver checks the afu_driver_events count during poll, select,
read, etc. calls to check if an AFU driver specific event is pending,
and calls deliver_event() to obtain and deliver that event. This way,
the cxl driver takes care of all the usual locking semantics around these
calls and handles all the generic cxl events, so that the AFU driver only
needs to worry about it's own events.

deliver_event() return a struct cxl_event_afu_driver_reserved, allocated
by the AFU driver, and filled in with the specific event information and
size. Data size should not be greater than CXL_MAX_EVENT_DATA_SIZE.

Th cxl driver prepends an appropriate cxl event header, copies the event
to userspace, and finally calls event_delivered() to return the status of
the operation to the AFU driver. The event is identified by the context
and cxl_event_afu_driver_reserved pointers.

Since AFU drivers provide their own means for userspace to obtain the
AFU file descriptor (i.e. cxlflash uses an ioctl on their scsi file
descriptor to obtain the AFU file descriptor) and the generic cxl driver
will never use this event, the ABI of the event is up to each individual
AFU driver.

Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
---
Changes since v4:
- Addressed comments from Vaibhav:
  - Changed struct cxl_event_afu_driver_reserved from
    { __u64 reserved[4]; } to { size_t data_size; u8 data[]; }
  - Modified deliver_event to return a struct cxl_event_afu_driver_reserved
  - Added new callback event_delivered
  - Added static function afu_driver_event_copy

Changes since v3:
- Removed driver ops callback ctx_event_pending
- Created cxl function cxl_context_pending_events
- Created cxl function cxl_unset_driver_ops
- Added atomic event counter afu_driver_events

Changes since v2:
- Fixed some typos spotted by Matt Ochs

Changes since v1:
- Rebased on upstream
- Bumped cxl api version to 3
- Addressed comments from mpe:
  - Clarified commit message & some comments
  - Mentioned 'cxlflash' as a possible user of this event
  - Check driver ops on registration and warn if missing calls
  - Remove redundant checks where driver ops is used
  - Simplified ctx_event_pending and removed underscore version
  - Changed deliver_event to take the context as the first argument

 drivers/misc/cxl/Kconfig |  5 +++++
 drivers/misc/cxl/api.c   | 27 +++++++++++++++++++++++++
 drivers/misc/cxl/cxl.h   |  7 ++++++-
 drivers/misc/cxl/file.c  | 52 ++++++++++++++++++++++++++++++++++++++++--------
 include/misc/cxl.h       | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/misc/cxl.h  | 21 +++++++++++++++++++
 6 files changed, 153 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/cxl/Kconfig b/drivers/misc/cxl/Kconfig
index 8756d06..560412c 100644
--- a/drivers/misc/cxl/Kconfig
+++ b/drivers/misc/cxl/Kconfig
@@ -15,12 +15,17 @@ config CXL_EEH
 	bool
 	default n
 
+config CXL_AFU_DRIVER_OPS
+	bool
+	default n
+
 config CXL
 	tristate "Support for IBM Coherent Accelerators (CXL)"
 	depends on PPC_POWERNV && PCI_MSI && EEH
 	select CXL_BASE
 	select CXL_KERNEL_API
 	select CXL_EEH
+	select CXL_AFU_DRIVER_OPS
 	default m
 	help
 	  Select this option to enable driver support for IBM Coherent
diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
index 6d228cc..71673cb 100644
--- a/drivers/misc/cxl/api.c
+++ b/drivers/misc/cxl/api.c
@@ -323,6 +323,33 @@ struct cxl_context *cxl_fops_get_context(struct file *file)
 }
 EXPORT_SYMBOL_GPL(cxl_fops_get_context);
 
+void cxl_set_driver_ops(struct cxl_context *ctx,
+			struct cxl_afu_driver_ops *ops)
+{
+	WARN_ON(!ops->deliver_event || !ops->event_delivered);
+	atomic_set(&ctx->afu_driver_events, 0);
+	ctx->afu_driver_ops = ops;
+}
+EXPORT_SYMBOL_GPL(cxl_set_driver_ops);
+
+int cxl_unset_driver_ops(struct cxl_context *ctx)
+{
+	if (atomic_read(&ctx->afu_driver_events))
+		return -EBUSY;
+
+	ctx->afu_driver_ops = NULL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cxl_unset_driver_ops);
+
+void cxl_context_events_pending(struct cxl_context *ctx,
+				unsigned int new_events)
+{
+	atomic_add(new_events, &ctx->afu_driver_events);
+	wake_up_all(&ctx->wq);
+}
+EXPORT_SYMBOL_GPL(cxl_context_events_pending);
+
 int cxl_start_work(struct cxl_context *ctx,
 		   struct cxl_ioctl_start_work *work)
 {
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 4fe5078..b0027e6 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -24,6 +24,7 @@
 #include <asm/reg.h>
 #include <misc/cxl-base.h>
 
+#include <misc/cxl.h>
 #include <uapi/misc/cxl.h>
 
 extern uint cxl_verbose;
@@ -34,7 +35,7 @@ extern uint cxl_verbose;
  * Bump version each time a user API change is made, whether it is
  * backwards compatible ot not.
  */
-#define CXL_API_VERSION 2
+#define CXL_API_VERSION 3
 #define CXL_API_VERSION_COMPATIBLE 1
 
 /*
@@ -522,6 +523,10 @@ struct cxl_context {
 	bool pending_fault;
 	bool pending_afu_err;
 
+	/* Used by AFU drivers for driver specific event delivery */
+	struct cxl_afu_driver_ops *afu_driver_ops;
+	atomic_t afu_driver_events;
+
 	struct rcu_head rcu;
 };
 
diff --git a/drivers/misc/cxl/file.c b/drivers/misc/cxl/file.c
index eec468f..6aebd0d 100644
--- a/drivers/misc/cxl/file.c
+++ b/drivers/misc/cxl/file.c
@@ -293,6 +293,17 @@ int afu_mmap(struct file *file, struct vm_area_struct *vm)
 	return cxl_context_iomap(ctx, vm);
 }
 
+static inline bool ctx_event_pending(struct cxl_context *ctx)
+{
+	if (ctx->pending_irq || ctx->pending_fault || ctx->pending_afu_err)
+		return true;
+
+	if (ctx->afu_driver_ops && atomic_read(&ctx->afu_driver_events))
+		return true;
+
+	return false;
+}
+
 unsigned int afu_poll(struct file *file, struct poll_table_struct *poll)
 {
 	struct cxl_context *ctx = file->private_data;
@@ -305,8 +316,7 @@ unsigned int afu_poll(struct file *file, struct poll_table_struct *poll)
 	pr_devel("afu_poll wait done pe: %i\n", ctx->pe);
 
 	spin_lock_irqsave(&ctx->lock, flags);
-	if (ctx->pending_irq || ctx->pending_fault ||
-	    ctx->pending_afu_err)
+	if (ctx_event_pending(ctx))
 		mask |= POLLIN | POLLRDNORM;
 	else if (ctx->status == CLOSED)
 		/* Only error on closed when there are no futher events pending
@@ -319,16 +329,32 @@ unsigned int afu_poll(struct file *file, struct poll_table_struct *poll)
 	return mask;
 }
 
-static inline int ctx_event_pending(struct cxl_context *ctx)
+static ssize_t afu_driver_event_copy(struct cxl_context *ctx,
+				     char __user *buf,
+				     struct cxl_event *event,
+				     struct cxl_event_afu_driver_reserved *pl)
 {
-	return (ctx->pending_irq || ctx->pending_fault ||
-	    ctx->pending_afu_err || (ctx->status == CLOSED));
+	int header_size = sizeof(struct cxl_event_header);
+
+	/* Copy the event header */
+	if (copy_to_user(buf, event, header_size)) {
+		ctx->afu_driver_ops->event_delivered(ctx, pl, -EFAULT);
+		return -EFAULT;
+	}
+	/* Copy the event data */
+	if (copy_to_user(buf + header_size, &pl->data, pl->data_size)) {
+		ctx->afu_driver_ops->event_delivered(ctx, pl, -EFAULT);
+		return -EFAULT;
+	}
+	ctx->afu_driver_ops->event_delivered(ctx, pl, 0);
+	return event->header.size;
 }
 
 ssize_t afu_read(struct file *file, char __user *buf, size_t count,
 			loff_t *off)
 {
 	struct cxl_context *ctx = file->private_data;
+	struct cxl_event_afu_driver_reserved *pl = NULL;
 	struct cxl_event event;
 	unsigned long flags;
 	int rc;
@@ -344,7 +370,7 @@ ssize_t afu_read(struct file *file, char __user *buf, size_t count,
 
 	for (;;) {
 		prepare_to_wait(&ctx->wq, &wait, TASK_INTERRUPTIBLE);
-		if (ctx_event_pending(ctx))
+		if (ctx_event_pending(ctx) || (ctx->status == CLOSED))
 			break;
 
 		if (!cxl_ops->link_ok(ctx->afu->adapter, ctx->afu)) {
@@ -374,7 +400,14 @@ ssize_t afu_read(struct file *file, char __user *buf, size_t count,
 	memset(&event, 0, sizeof(event));
 	event.header.process_element = ctx->pe;
 	event.header.size = sizeof(struct cxl_event_header);
-	if (ctx->pending_irq) {
+	if (ctx->afu_driver_ops && atomic_read(&ctx->afu_driver_events)) {
+		pr_devel("afu_read delivering AFU driver specific event\n");
+		pl = ctx->afu_driver_ops->deliver_event(ctx);
+		atomic_dec(&ctx->afu_driver_events);
+		WARN_ON(!pl || (pl->data_size > CXL_MAX_EVENT_DATA_SIZE));
+		event.header.size += pl->data_size;
+		event.header.type = CXL_EVENT_AFU_DRIVER;
+	} else if (ctx->pending_irq) {
 		pr_devel("afu_read delivering AFU interrupt\n");
 		event.header.size += sizeof(struct cxl_event_afu_interrupt);
 		event.header.type = CXL_EVENT_AFU_INTERRUPT;
@@ -404,6 +437,9 @@ ssize_t afu_read(struct file *file, char __user *buf, size_t count,
 
 	spin_unlock_irqrestore(&ctx->lock, flags);
 
+	if (event.header.type == CXL_EVENT_AFU_DRIVER)
+		return afu_driver_event_copy(ctx, buf, &event, pl);
+
 	if (copy_to_user(buf, &event, event.header.size))
 		return -EFAULT;
 	return event.header.size;
@@ -558,7 +594,7 @@ int __init cxl_file_init(void)
 	 * If these change we really need to update API.  Either change some
 	 * flags or update API version number CXL_API_VERSION.
 	 */
-	BUILD_BUG_ON(CXL_API_VERSION != 2);
+	BUILD_BUG_ON(CXL_API_VERSION != 3);
 	BUILD_BUG_ON(sizeof(struct cxl_ioctl_start_work) != 64);
 	BUILD_BUG_ON(sizeof(struct cxl_event_header) != 8);
 	BUILD_BUG_ON(sizeof(struct cxl_event_afu_interrupt) != 8);
diff --git a/include/misc/cxl.h b/include/misc/cxl.h
index 56560c5..ef3115e 100644
--- a/include/misc/cxl.h
+++ b/include/misc/cxl.h
@@ -220,4 +220,54 @@ void cxl_perst_reloads_same_image(struct cxl_afu *afu,
  */
 ssize_t cxl_read_adapter_vpd(struct pci_dev *dev, void *buf, size_t count);
 
+/*
+ * AFU driver ops allow an AFU driver to create their own events to pass to
+ * userspace through the file descriptor as a simpler alternative to overriding
+ * the read() and poll() calls that works with the generic cxl events. These
+ * events are given priority over the generic cxl events, so they will be
+ * delivered first if multiple types of events are pending.
+ *
+ * The AFU driver must call cxl_context_events_pending() to notify the cxl
+ * driver that new events are ready to be delivered for a specific context.
+ * cxl_context_events_pending() will adjust the current count of AFU driver
+ * events for this context, and wake up anyone waiting on the context wait
+ * queue.
+ *
+ * The cxl driver will then call deliver_event() to get a structure defining
+ * the size and address of the driver specific event data. Data size cannot
+ * be greater than CXL_MAX_EVENT_DATA_SIZE. The cxl driver will build a cxl
+ * header with type and process_element fields filled in, and header.size set
+ * to sizeof(struct cxl_event_header) + data_size.
+ *
+ * deliver_event() is called with a spin lock held, so it must not sleep.
+ *
+ * The cxl driver will finally call event_delivered() to return the status
+ * of the operation, identified by its context and AFU driver event data
+ * pointers. If the copy to user has failed, it is up to the AFU driver to
+ * retry and submit the event again with cxl_context_events_pending().
+ */
+struct cxl_afu_driver_ops {
+	struct cxl_event_afu_driver_reserved *(*deliver_event) (
+						struct cxl_context *ctx);
+	void (*event_delivered) (struct cxl_context *ctx,
+				 struct cxl_event_afu_driver_reserved *event,
+				 int rc);
+};
+
+/*
+ * Associate the above driver ops with a specific context.
+ * Reset the current count of AFU driver events.
+ */
+void cxl_set_driver_ops(struct cxl_context *ctx,
+			struct cxl_afu_driver_ops *ops);
+/*
+ * Remove the driver ops from a specific context.
+ * The current count of AFU driver events must be 0.
+ */
+int cxl_unset_driver_ops(struct cxl_context *ctx);
+
+/* Notify cxl driver that new events are ready to be delivered for context */
+void cxl_context_events_pending(struct cxl_context *ctx,
+				unsigned int new_events);
+
 #endif /* _MISC_CXL_H */
diff --git a/include/uapi/misc/cxl.h b/include/uapi/misc/cxl.h
index 8cd334f..4fa36e4 100644
--- a/include/uapi/misc/cxl.h
+++ b/include/uapi/misc/cxl.h
@@ -93,6 +93,7 @@ enum cxl_event_type {
 	CXL_EVENT_AFU_INTERRUPT = 1,
 	CXL_EVENT_DATA_STORAGE  = 2,
 	CXL_EVENT_AFU_ERROR     = 3,
+	CXL_EVENT_AFU_DRIVER    = 4,
 };
 
 struct cxl_event_header {
@@ -124,12 +125,32 @@ struct cxl_event_afu_error {
 	__u64 error;
 };
 
+#define CXL_MAX_EVENT_DATA_SIZE 128
+
+struct cxl_event_afu_driver_reserved {
+	/*
+	 * Defines the buffer passed to the cxl driver by the AFU driver.
+	 *
+	 * This is not ABI since the event header.size passed to the user for
+	 * existing events is set in the read call to sizeof(cxl_event_header)
+	 * + sizeof(whatever event is being dispatched) and the user is already
+	 * required to use a 4K buffer on the read call.
+	 *
+	 * Of course the contents will be ABI, but that's up the AFU driver.
+	 *
+	 * data_size is limited to MAX_EVENT_DATA_SIZE to prevent abuse.
+	 */
+	size_t data_size;
+	u8 data[];
+};
+
 struct cxl_event {
 	struct cxl_event_header header;
 	union {
 		struct cxl_event_afu_interrupt irq;
 		struct cxl_event_data_storage fault;
 		struct cxl_event_afu_error afu_error;
+		struct cxl_event_afu_driver_reserved afu_driver_event;
 	};
 };
 
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [v4,2/2] cxl: Add set and get private data to context struct
  2016-05-23 12:49 [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events Philippe Bergheaud
@ 2016-05-23 12:49 ` Philippe Bergheaud
  2016-05-24  6:59 ` [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events Vaibhav Jain
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Philippe Bergheaud @ 2016-05-23 12:49 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: mrochs, manoj, mpe, imunsie, mikey, Philippe Bergheaud

From: Michael Neuling <mikey@neuling.org>

This provides AFU drivers a means to associate private data with a cxl
context. This is particularly intended for make the new callbacks for
driver specific events easier for AFU drivers to use, as they can easily
get back to any private data structures they may use.

Signed-off-by: Michael Neuling <mikey@neuling.org>
Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com
Reviewed-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
No changes since v1, added Matt Ochs reviewed-by tag.

 drivers/misc/cxl/api.c | 21 +++++++++++++++++++++
 drivers/misc/cxl/cxl.h |  3 +++
 include/misc/cxl.h     |  7 +++++++
 3 files changed, 31 insertions(+)

diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
index 71673cb..efeaa6e 100644
--- a/drivers/misc/cxl/api.c
+++ b/drivers/misc/cxl/api.c
@@ -94,6 +94,27 @@ static irq_hw_number_t cxl_find_afu_irq(struct cxl_context *ctx, int num)
 	return 0;
 }
 
+
+int cxl_set_priv(struct cxl_context *ctx, void *priv)
+{
+	if (!ctx)
+		return -EINVAL;
+
+	ctx->priv = priv;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cxl_set_priv);
+
+void *cxl_get_priv(struct cxl_context *ctx)
+{
+	if (!ctx)
+		return ERR_PTR(-EINVAL);
+
+	return ctx->priv;
+}
+EXPORT_SYMBOL_GPL(cxl_get_priv);
+
 int cxl_allocate_afu_irqs(struct cxl_context *ctx, int num)
 {
 	int res;
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index b0027e6..1e56304 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -478,6 +478,9 @@ struct cxl_context {
 	/* Only used in PR mode */
 	u64 process_token;
 
+	/* driver private data */
+	void *priv;
+
 	unsigned long *irq_bitmap; /* Accessed from IRQ context */
 	struct cxl_irq_ranges irqs;
 	struct list_head irq_names;
diff --git a/include/misc/cxl.h b/include/misc/cxl.h
index ef3115e..667f99d 100644
--- a/include/misc/cxl.h
+++ b/include/misc/cxl.h
@@ -86,6 +86,13 @@ struct cxl_context *cxl_dev_context_init(struct pci_dev *dev);
 int cxl_release_context(struct cxl_context *ctx);
 
 /*
+ * Set and get private data associated with a context. Allows drivers to have a
+ * back pointer to some useful structure.
+ */
+int cxl_set_priv(struct cxl_context *ctx, void *priv);
+void *cxl_get_priv(struct cxl_context *ctx);
+
+/*
  * Allocate AFU interrupts for this context. num=0 will allocate the default
  * for this AFU as given in the AFU descriptor. This number doesn't include the
  * interrupt 0 (CAIA defines AFU IRQ 0 for page faults). Each interrupt to be
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events
  2016-05-23 12:49 [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events Philippe Bergheaud
  2016-05-23 12:49 ` [v4,2/2] cxl: Add set and get private data to context struct Philippe Bergheaud
@ 2016-05-24  6:59 ` Vaibhav Jain
  2016-05-24 14:44   ` Matthew R. Ochs
  2016-06-14 15:21 ` Matthew R. Ochs
  2016-06-16 13:05 ` Michael Ellerman
  3 siblings, 1 reply; 9+ messages in thread
From: Vaibhav Jain @ 2016-05-24  6:59 UTC (permalink / raw)
  To: Philippe Bergheaud, linuxppc-dev
  Cc: mikey, mrochs, manoj, imunsie, Philippe Bergheaud

Hi Philippe,

Few comments,

Philippe Bergheaud <felix@linux.vnet.ibm.com> writes:

> diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
> index 4fe5078..b0027e6 100644
> --- a/drivers/misc/cxl/cxl.h
> +++ b/drivers/misc/cxl/cxl.h
> @@ -24,6 +24,7 @@
>  #include <asm/reg.h>
>  #include <misc/cxl-base.h>
>  
> +#include <misc/cxl.h>
>  #include <uapi/misc/cxl.h>
>  
>  extern uint cxl_verbose;
> @@ -34,7 +35,7 @@ extern uint cxl_verbose;
>   * Bump version each time a user API change is made, whether it is
>   * backwards compatible ot not.
>   */
> -#define CXL_API_VERSION 2
> +#define CXL_API_VERSION 3
>  #define CXL_API_VERSION_COMPATIBLE 1
>  
>  /*
> @@ -522,6 +523,10 @@ struct cxl_context {
>  	bool pending_fault;
>  	bool pending_afu_err;
>  
> +	/* Used by AFU drivers for driver specific event delivery */
> +	struct cxl_afu_driver_ops *afu_driver_ops;
> +	atomic_t afu_driver_events;
If the afu_driver_ops.deliver_event is idempotent then instead of using
a refcount we can simply call afu_driver_ops.deliver_event to see if
there are any driver events queued. Since callback deliver_event is
called in context of a spinlock and cannot sleep hence requirement of
idempotency seems reasonable.

> +
>  	struct rcu_head rcu;
>  };
>  
> diff --git a/drivers/misc/cxl/file.c b/drivers/misc/cxl/file.c
> index eec468f..6aebd0d 100644
> --- a/drivers/misc/cxl/file.c
> +++ b/drivers/misc/cxl/file.c
> @@ -293,6 +293,17 @@ int afu_mmap(struct file *file, struct vm_area_struct *vm)
>  	return cxl_context_iomap(ctx, vm);
>  }
>  
> +static inline bool ctx_event_pending(struct cxl_context *ctx)
> +{
> +	if (ctx->pending_irq || ctx->pending_fault || ctx->pending_afu_err)
> +		return true;
> +
> +	if (ctx->afu_driver_ops && atomic_read(&ctx->afu_driver_events))
> +		return true;
> +
> +	return false;
> +}
> +
>  unsigned int afu_poll(struct file *file, struct poll_table_struct *poll)
>  {
>  	struct cxl_context *ctx = file->private_data;
> @@ -305,8 +316,7 @@ unsigned int afu_poll(struct file *file, struct poll_table_struct *poll)
>  	pr_devel("afu_poll wait done pe: %i\n", ctx->pe);
>  
>  	spin_lock_irqsave(&ctx->lock, flags);
> -	if (ctx->pending_irq || ctx->pending_fault ||
> -	    ctx->pending_afu_err)
> +	if (ctx_event_pending(ctx))
>  		mask |= POLLIN | POLLRDNORM;
>  	else if (ctx->status == CLOSED)
>  		/* Only error on closed when there are no futher events pending
> @@ -319,16 +329,32 @@ unsigned int afu_poll(struct file *file, struct poll_table_struct *poll)
>  	return mask;
>  }
>  
> -static inline int ctx_event_pending(struct cxl_context *ctx)
> +static ssize_t afu_driver_event_copy(struct cxl_context *ctx,
> +				     char __user *buf,
Instead of using (char __user *buf) as an argument it would be better to use
(struct cxl_event __user *buf) so that we dont have to use pointer
arithmetic in this function body; which may break with future iterations
of this struct. The struct cxl_event_afu_driver_reserved may simply be
referred to as &(buf.afu_driver_event)

> @@ -374,7 +400,14 @@ ssize_t afu_read(struct file *file, char __user *buf, size_t count,
>  	memset(&event, 0, sizeof(event));
>  	event.header.process_element = ctx->pe;
>  	event.header.size = sizeof(struct cxl_event_header);
> -	if (ctx->pending_irq) {
> +	if (ctx->afu_driver_ops && atomic_read(&ctx->afu_driver_events)) {
> +		pr_devel("afu_read delivering AFU driver specific event\n");
> +		pl = ctx->afu_driver_ops->deliver_event(ctx);
> +		atomic_dec(&ctx->afu_driver_events);
> +		WARN_ON(!pl || (pl->data_size >
> CXL_MAX_EVENT_DATA_SIZE));
Should return an error to the driver via
event_delivered(..,..,EOVERFLOW)

> +		event.header.size += pl->data_size;
> +		event.header.type = CXL_EVENT_AFU_DRIVER;
> +	} else if (ctx->pending_irq) {
>  		pr_devel("afu_read delivering AFU interrupt\n");
>  		event.header.size += sizeof(struct cxl_event_afu_interrupt);
>  		event.header.type = CXL_EVENT_AFU_INTERRUPT;
> @@ -404,6 +437,9 @@ ssize_t afu_read(struct file *file, char __user *buf, size_t count,
>  
>  	spin_unlock_irqrestore(&ctx->lock, flags);
>  
> +	if (event.header.type == CXL_EVENT_AFU_DRIVER)
> +		return afu_driver_event_copy(ctx, buf, &event, pl);
there should be check againt the count to see if driver event can fit
into the buffer provided. If not then the driver/userspace should be
notified. Otherwise it can result in corruption of userspace memory.

Cheers,
~ Vaibhav

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events
  2016-05-24  6:59 ` [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events Vaibhav Jain
@ 2016-05-24 14:44   ` Matthew R. Ochs
  2016-05-25  7:22     ` Vaibhav Jain
  0 siblings, 1 reply; 9+ messages in thread
From: Matthew R. Ochs @ 2016-05-24 14:44 UTC (permalink / raw)
  To: Vaibhav Jain; +Cc: Philippe Bergheaud, linuxppc-dev, mikey, manoj, imunsie

> On May 24, 2016, at 1:59 AM, Vaibhav Jain <vaibhav@linux.vnet.ibm.com> =
wrote:
>=20
> Hi Philippe,
>=20
> Few comments,
>=20
> Philippe Bergheaud <felix@linux.vnet.ibm.com> writes:
>=20
>> diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
>> index 4fe5078..b0027e6 100644
>> --- a/drivers/misc/cxl/cxl.h
>> +++ b/drivers/misc/cxl/cxl.h
>> @@ -24,6 +24,7 @@
>> #include <asm/reg.h>
>> #include <misc/cxl-base.h>
>>=20
>> +#include <misc/cxl.h>
>> #include <uapi/misc/cxl.h>
>>=20
>> extern uint cxl_verbose;
>> @@ -34,7 +35,7 @@ extern uint cxl_verbose;
>>  * Bump version each time a user API change is made, whether it is
>>  * backwards compatible ot not.
>>  */
>> -#define CXL_API_VERSION 2
>> +#define CXL_API_VERSION 3
>> #define CXL_API_VERSION_COMPATIBLE 1
>>=20
>> /*
>> @@ -522,6 +523,10 @@ struct cxl_context {
>> 	bool pending_fault;
>> 	bool pending_afu_err;
>>=20
>> +	/* Used by AFU drivers for driver specific event delivery */
>> +	struct cxl_afu_driver_ops *afu_driver_ops;
>> +	atomic_t afu_driver_events;
> If the afu_driver_ops.deliver_event is idempotent then instead of =
using
> a refcount we can simply call afu_driver_ops.deliver_event to see if
> there are any driver events queued. Since callback deliver_event is
> called in context of a spinlock and cannot sleep hence requirement of
> idempotency seems reasonable.

The purpose for the count is so the AFU driver is only called when it
has something to send. Otherwise we don't want to be called.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events
  2016-05-24 14:44   ` Matthew R. Ochs
@ 2016-05-25  7:22     ` Vaibhav Jain
  2016-06-14 14:47       ` Matthew R. Ochs
  0 siblings, 1 reply; 9+ messages in thread
From: Vaibhav Jain @ 2016-05-25  7:22 UTC (permalink / raw)
  To: Matthew R. Ochs; +Cc: Philippe Bergheaud, linuxppc-dev, mikey, manoj, imunsie

Hi Matt,

"Matthew R. Ochs" <mrochs@linux.vnet.ibm.com> writes:
> The purpose for the count is so the AFU driver is only called when it
> has something to send. Otherwise we don't want to be called.

Agreed, but this opens up a possible boundary condition where in we have
non-zero event count and deliver_event callback returns a NULL/empty
struct (This isn't handled correctly at the moment). Imo the condition
event-count == number-of-calls-to-deliver_events is bit too rigid.

Instead a more relaxed condition can be number-of-calls-to-deliver_event ==
count-until-deliver_event-returns-NULL. This could be implemented as
boolean flag inside the context to indicate that afu-driver has some
events queued. This flag can be set when cxl_context_events_pending gets
called. The cxl code can simply call deliver_event on each read call
untill it returns NULL in which case this boolean flag is reset.

This should slightly simplify the code flow at the afu-driver end as
enquing to an event need not be paired by a call to
cxl_context_events_pending. It can quite possibly enqueue bunch of
events and then do a single call to cxl_context_events_pending. In this
case the function cxl_context_events_pending essentially works more like
a function named as cxl_context_events_flush.

~ Vaibhav

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events
  2016-05-25  7:22     ` Vaibhav Jain
@ 2016-06-14 14:47       ` Matthew R. Ochs
  0 siblings, 0 replies; 9+ messages in thread
From: Matthew R. Ochs @ 2016-06-14 14:47 UTC (permalink / raw)
  To: Vaibhav Jain
  Cc: Philippe Bergheaud, Michael Neuling, Manoj N. Kumar,
	linuxppc-dev, Ian Munsie, Uma Krishnan

Vaibhav/Philippe,

Finally getting back around to looking at this.


-matt

> On May 25, 2016, at 2:22 AM, Vaibhav Jain <vaibhav@linux.vnet.ibm.com> =
wrote:
>=20
> Hi Matt,
>=20
> "Matthew R. Ochs" <mrochs@linux.vnet.ibm.com> writes:
>> The purpose for the count is so the AFU driver is only called when it
>> has something to send. Otherwise we don't want to be called.
>=20
> Agreed, but this opens up a possible boundary condition where in we =
have
> non-zero event count and deliver_event callback returns a NULL/empty
> struct (This isn't handled correctly at the moment). Imo the condition
> event-count =3D=3D number-of-calls-to-deliver_events is bit too rigid.
>=20
> Instead a more relaxed condition can be =
number-of-calls-to-deliver_event =3D=3D
> count-until-deliver_event-returns-NULL. This could be implemented as
> boolean flag inside the context to indicate that afu-driver has some
> events queued. This flag can be set when cxl_context_events_pending =
gets
> called. The cxl code can simply call deliver_event on each read call
> until it returns NULL in which case this boolean flag is reset.

We're fine with being called until we return NULL. We just don't want to
always be called. =3D)

I believe the earlier discussions we had with Ian indicated that us =
returning
NULL (effectively 'failing') could be problematic for the read handler. =
Perhaps
this is no longer the case with the updated patch.

Regardless of your internal implementation, we would still like for the =
API
we call to indicate the number of events we've enqueued and desire to =
send
to the user. This will allow for flexibility in the future should your =
internal
implementation change.

>=20
> This should slightly simplify the code flow at the afu-driver end as
> enquing to an event need not be paired by a call to
> cxl_context_events_pending. It can quite possibly enqueue bunch of
> events and then do a single call to cxl_context_events_pending. In =
this
> case the function cxl_context_events_pending essentially works more =
like
> a function named as cxl_context_events_flush.
>=20
> ~ Vaibhav
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events
  2016-05-23 12:49 [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events Philippe Bergheaud
  2016-05-23 12:49 ` [v4,2/2] cxl: Add set and get private data to context struct Philippe Bergheaud
  2016-05-24  6:59 ` [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events Vaibhav Jain
@ 2016-06-14 15:21 ` Matthew R. Ochs
  2016-06-16 13:05 ` Michael Ellerman
  3 siblings, 0 replies; 9+ messages in thread
From: Matthew R. Ochs @ 2016-06-14 15:21 UTC (permalink / raw)
  To: Philippe Bergheaud
  Cc: linuxppc-dev, Michael Neuling, Manoj N. Kumar, Ian Munsie, Uma Krishnan

Hi Philippe,

So I think we're largely okay with this revision. I made one comment
below with respect to the name of the event retrieval callback.


-matt

> On May 23, 2016, at 7:49 AM, Philippe Bergheaud =
<felix@linux.vnet.ibm.com> wrote:
>=20
> This adds an afu_driver_ops structure with deliver_event() and
> event_delivered() callbacks. An AFU driver such as cxlflash can fill
> this out and associate it with a context to enable passing custom
> AFU specific events to userspace.
>=20
> This also adds a new kernel API function cxl_context_pending_events(),
> that the AFU driver can use to notify the cxl driver that new specific
> events are ready to be delivered, and wake up anyone waiting on the
> context wait queue.
>=20
> The current count of AFU driver specific events is stored in the field
> afu_driver_events of the context structure.
>=20
> The cxl driver checks the afu_driver_events count during poll, select,
> read, etc. calls to check if an AFU driver specific event is pending,
> and calls deliver_event() to obtain and deliver that event. This way,
> the cxl driver takes care of all the usual locking semantics around =
these
> calls and handles all the generic cxl events, so that the AFU driver =
only
> needs to worry about it's own events.
>=20
> deliver_event() return a struct cxl_event_afu_driver_reserved, =
allocated
> by the AFU driver, and filled in with the specific event information =
and
> size. Data size should not be greater than CXL_MAX_EVENT_DATA_SIZE.
>=20
> Th cxl driver prepends an appropriate cxl event header, copies the =
event
> to userspace, and finally calls event_delivered() to return the status =
of
> the operation to the AFU driver. The event is identified by the =
context
> and cxl_event_afu_driver_reserved pointers.
>=20
> Since AFU drivers provide their own means for userspace to obtain the
> AFU file descriptor (i.e. cxlflash uses an ioctl on their scsi file
> descriptor to obtain the AFU file descriptor) and the generic cxl =
driver
> will never use this event, the ABI of the event is up to each =
individual
> AFU driver.
>=20
> Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
> ---
> Changes since v4:
> - Addressed comments from Vaibhav:
>  - Changed struct cxl_event_afu_driver_reserved from
>    { __u64 reserved[4]; } to { size_t data_size; u8 data[]; }
>  - Modified deliver_event to return a struct =
cxl_event_afu_driver_reserved
>  - Added new callback event_delivered
>  - Added static function afu_driver_event_copy
>=20
> Changes since v3:
> - Removed driver ops callback ctx_event_pending
> - Created cxl function cxl_context_pending_events
> - Created cxl function cxl_unset_driver_ops
> - Added atomic event counter afu_driver_events
>=20
> Changes since v2:
> - Fixed some typos spotted by Matt Ochs
>=20
> Changes since v1:
> - Rebased on upstream
> - Bumped cxl api version to 3
> - Addressed comments from mpe:
>  - Clarified commit message & some comments
>  - Mentioned 'cxlflash' as a possible user of this event
>  - Check driver ops on registration and warn if missing calls
>  - Remove redundant checks where driver ops is used
>  - Simplified ctx_event_pending and removed underscore version
>  - Changed deliver_event to take the context as the first argument
>=20
> drivers/misc/cxl/Kconfig |  5 +++++
> drivers/misc/cxl/api.c   | 27 +++++++++++++++++++++++++
> drivers/misc/cxl/cxl.h   |  7 ++++++-
> drivers/misc/cxl/file.c  | 52 =
++++++++++++++++++++++++++++++++++++++++--------
> include/misc/cxl.h       | 50 =
++++++++++++++++++++++++++++++++++++++++++++++
> include/uapi/misc/cxl.h  | 21 +++++++++++++++++++
> 6 files changed, 153 insertions(+), 9 deletions(-)
>=20
> diff --git a/drivers/misc/cxl/Kconfig b/drivers/misc/cxl/Kconfig
> index 8756d06..560412c 100644
> --- a/drivers/misc/cxl/Kconfig
> +++ b/drivers/misc/cxl/Kconfig
> @@ -15,12 +15,17 @@ config CXL_EEH
> 	bool
> 	default n
>=20
> +config CXL_AFU_DRIVER_OPS
> +	bool
> +	default n
> +
> config CXL
> 	tristate "Support for IBM Coherent Accelerators (CXL)"
> 	depends on PPC_POWERNV && PCI_MSI && EEH
> 	select CXL_BASE
> 	select CXL_KERNEL_API
> 	select CXL_EEH
> +	select CXL_AFU_DRIVER_OPS
> 	default m
> 	help
> 	  Select this option to enable driver support for IBM Coherent
> diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
> index 6d228cc..71673cb 100644
> --- a/drivers/misc/cxl/api.c
> +++ b/drivers/misc/cxl/api.c
> @@ -323,6 +323,33 @@ struct cxl_context *cxl_fops_get_context(struct =
file *file)
> }
> EXPORT_SYMBOL_GPL(cxl_fops_get_context);
>=20
> +void cxl_set_driver_ops(struct cxl_context *ctx,
> +			struct cxl_afu_driver_ops *ops)
> +{
> +	WARN_ON(!ops->deliver_event || !ops->event_delivered);
> +	atomic_set(&ctx->afu_driver_events, 0);
> +	ctx->afu_driver_ops =3D ops;
> +}
> +EXPORT_SYMBOL_GPL(cxl_set_driver_ops);
> +
> +int cxl_unset_driver_ops(struct cxl_context *ctx)
> +{
> +	if (atomic_read(&ctx->afu_driver_events))
> +		return -EBUSY;
> +
> +	ctx->afu_driver_ops =3D NULL;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cxl_unset_driver_ops);
> +
> +void cxl_context_events_pending(struct cxl_context *ctx,
> +				unsigned int new_events)
> +{
> +	atomic_add(new_events, &ctx->afu_driver_events);
> +	wake_up_all(&ctx->wq);
> +}
> +EXPORT_SYMBOL_GPL(cxl_context_events_pending);
> +
> int cxl_start_work(struct cxl_context *ctx,
> 		   struct cxl_ioctl_start_work *work)
> {
> diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
> index 4fe5078..b0027e6 100644
> --- a/drivers/misc/cxl/cxl.h
> +++ b/drivers/misc/cxl/cxl.h
> @@ -24,6 +24,7 @@
> #include <asm/reg.h>
> #include <misc/cxl-base.h>
>=20
> +#include <misc/cxl.h>
> #include <uapi/misc/cxl.h>
>=20
> extern uint cxl_verbose;
> @@ -34,7 +35,7 @@ extern uint cxl_verbose;
>  * Bump version each time a user API change is made, whether it is
>  * backwards compatible ot not.
>  */
> -#define CXL_API_VERSION 2
> +#define CXL_API_VERSION 3
> #define CXL_API_VERSION_COMPATIBLE 1
>=20
> /*
> @@ -522,6 +523,10 @@ struct cxl_context {
> 	bool pending_fault;
> 	bool pending_afu_err;
>=20
> +	/* Used by AFU drivers for driver specific event delivery */
> +	struct cxl_afu_driver_ops *afu_driver_ops;
> +	atomic_t afu_driver_events;
> +
> 	struct rcu_head rcu;
> };
>=20
> diff --git a/drivers/misc/cxl/file.c b/drivers/misc/cxl/file.c
> index eec468f..6aebd0d 100644
> --- a/drivers/misc/cxl/file.c
> +++ b/drivers/misc/cxl/file.c
> @@ -293,6 +293,17 @@ int afu_mmap(struct file *file, struct =
vm_area_struct *vm)
> 	return cxl_context_iomap(ctx, vm);
> }
>=20
> +static inline bool ctx_event_pending(struct cxl_context *ctx)
> +{
> +	if (ctx->pending_irq || ctx->pending_fault || =
ctx->pending_afu_err)
> +		return true;
> +
> +	if (ctx->afu_driver_ops && atomic_read(&ctx->afu_driver_events))
> +		return true;
> +
> +	return false;
> +}
> +
> unsigned int afu_poll(struct file *file, struct poll_table_struct =
*poll)
> {
> 	struct cxl_context *ctx =3D file->private_data;
> @@ -305,8 +316,7 @@ unsigned int afu_poll(struct file *file, struct =
poll_table_struct *poll)
> 	pr_devel("afu_poll wait done pe: %i\n", ctx->pe);
>=20
> 	spin_lock_irqsave(&ctx->lock, flags);
> -	if (ctx->pending_irq || ctx->pending_fault ||
> -	    ctx->pending_afu_err)
> +	if (ctx_event_pending(ctx))
> 		mask |=3D POLLIN | POLLRDNORM;
> 	else if (ctx->status =3D=3D CLOSED)
> 		/* Only error on closed when there are no futher events =
pending
> @@ -319,16 +329,32 @@ unsigned int afu_poll(struct file *file, struct =
poll_table_struct *poll)
> 	return mask;
> }
>=20
> -static inline int ctx_event_pending(struct cxl_context *ctx)
> +static ssize_t afu_driver_event_copy(struct cxl_context *ctx,
> +				     char __user *buf,
> +				     struct cxl_event *event,
> +				     struct =
cxl_event_afu_driver_reserved *pl)
> {
> -	return (ctx->pending_irq || ctx->pending_fault ||
> -	    ctx->pending_afu_err || (ctx->status =3D=3D CLOSED));
> +	int header_size =3D sizeof(struct cxl_event_header);
> +
> +	/* Copy the event header */
> +	if (copy_to_user(buf, event, header_size)) {
> +		ctx->afu_driver_ops->event_delivered(ctx, pl, -EFAULT);
> +		return -EFAULT;
> +	}
> +	/* Copy the event data */
> +	if (copy_to_user(buf + header_size, &pl->data, pl->data_size)) {
> +		ctx->afu_driver_ops->event_delivered(ctx, pl, -EFAULT);
> +		return -EFAULT;
> +	}
> +	ctx->afu_driver_ops->event_delivered(ctx, pl, 0);
> +	return event->header.size;
> }
>=20
> ssize_t afu_read(struct file *file, char __user *buf, size_t count,
> 			loff_t *off)
> {
> 	struct cxl_context *ctx =3D file->private_data;
> +	struct cxl_event_afu_driver_reserved *pl =3D NULL;
> 	struct cxl_event event;
> 	unsigned long flags;
> 	int rc;
> @@ -344,7 +370,7 @@ ssize_t afu_read(struct file *file, char __user =
*buf, size_t count,
>=20
> 	for (;;) {
> 		prepare_to_wait(&ctx->wq, &wait, TASK_INTERRUPTIBLE);
> -		if (ctx_event_pending(ctx))
> +		if (ctx_event_pending(ctx) || (ctx->status =3D=3D =
CLOSED))
> 			break;
>=20
> 		if (!cxl_ops->link_ok(ctx->afu->adapter, ctx->afu)) {
> @@ -374,7 +400,14 @@ ssize_t afu_read(struct file *file, char __user =
*buf, size_t count,
> 	memset(&event, 0, sizeof(event));
> 	event.header.process_element =3D ctx->pe;
> 	event.header.size =3D sizeof(struct cxl_event_header);
> -	if (ctx->pending_irq) {
> +	if (ctx->afu_driver_ops && atomic_read(&ctx->afu_driver_events)) =
{
> +		pr_devel("afu_read delivering AFU driver specific =
event\n");
> +		pl =3D ctx->afu_driver_ops->deliver_event(ctx);
> +		atomic_dec(&ctx->afu_driver_events);
> +		WARN_ON(!pl || (pl->data_size > =
CXL_MAX_EVENT_DATA_SIZE));
> +		event.header.size +=3D pl->data_size;
> +		event.header.type =3D CXL_EVENT_AFU_DRIVER;
> +	} else if (ctx->pending_irq) {
> 		pr_devel("afu_read delivering AFU interrupt\n");
> 		event.header.size +=3D sizeof(struct =
cxl_event_afu_interrupt);
> 		event.header.type =3D CXL_EVENT_AFU_INTERRUPT;
> @@ -404,6 +437,9 @@ ssize_t afu_read(struct file *file, char __user =
*buf, size_t count,
>=20
> 	spin_unlock_irqrestore(&ctx->lock, flags);
>=20
> +	if (event.header.type =3D=3D CXL_EVENT_AFU_DRIVER)
> +		return afu_driver_event_copy(ctx, buf, &event, pl);
> +
> 	if (copy_to_user(buf, &event, event.header.size))
> 		return -EFAULT;
> 	return event.header.size;
> @@ -558,7 +594,7 @@ int __init cxl_file_init(void)
> 	 * If these change we really need to update API.  Either change =
some
> 	 * flags or update API version number CXL_API_VERSION.
> 	 */
> -	BUILD_BUG_ON(CXL_API_VERSION !=3D 2);
> +	BUILD_BUG_ON(CXL_API_VERSION !=3D 3);
> 	BUILD_BUG_ON(sizeof(struct cxl_ioctl_start_work) !=3D 64);
> 	BUILD_BUG_ON(sizeof(struct cxl_event_header) !=3D 8);
> 	BUILD_BUG_ON(sizeof(struct cxl_event_afu_interrupt) !=3D 8);
> diff --git a/include/misc/cxl.h b/include/misc/cxl.h
> index 56560c5..ef3115e 100644
> --- a/include/misc/cxl.h
> +++ b/include/misc/cxl.h
> @@ -220,4 +220,54 @@ void cxl_perst_reloads_same_image(struct cxl_afu =
*afu,
>  */
> ssize_t cxl_read_adapter_vpd(struct pci_dev *dev, void *buf, size_t =
count);
>=20
> +/*
> + * AFU driver ops allow an AFU driver to create their own events to =
pass to
> + * userspace through the file descriptor as a simpler alternative to =
overriding
> + * the read() and poll() calls that works with the generic cxl =
events. These
> + * events are given priority over the generic cxl events, so they =
will be
> + * delivered first if multiple types of events are pending.
> + *
> + * The AFU driver must call cxl_context_events_pending() to notify =
the cxl
> + * driver that new events are ready to be delivered for a specific =
context.
> + * cxl_context_events_pending() will adjust the current count of AFU =
driver
> + * events for this context, and wake up anyone waiting on the context =
wait
> + * queue.
> + *
> + * The cxl driver will then call deliver_event() to get a structure =
defining
> + * the size and address of the driver specific event data. Data size =
cannot
> + * be greater than CXL_MAX_EVENT_DATA_SIZE. The cxl driver will build =
a cxl
> + * header with type and process_element fields filled in, and =
header.size set
> + * to sizeof(struct cxl_event_header) + data_size.
> + *
> + * deliver_event() is called with a spin lock held, so it must not =
sleep.
> + *
> + * The cxl driver will finally call event_delivered() to return the =
status
> + * of the operation, identified by its context and AFU driver event =
data
> + * pointers. If the copy to user has failed, it is up to the AFU =
driver to
> + * retry and submit the event again with =
cxl_context_events_pending().
> + */
> +struct cxl_afu_driver_ops {
> +	struct cxl_event_afu_driver_reserved *(*deliver_event) (
> +						struct cxl_context =
*ctx);

I really find this name confusing. How about something more indicative
that you're obtaining something from the driver?

fetch_event()
get_event()
retrieve_event()
etc...

> +	void (*event_delivered) (struct cxl_context *ctx,
> +				 struct cxl_event_afu_driver_reserved =
*event,
> +				 int rc);
> +};
> +
> +/*
> + * Associate the above driver ops with a specific context.
> + * Reset the current count of AFU driver events.
> + */
> +void cxl_set_driver_ops(struct cxl_context *ctx,
> +			struct cxl_afu_driver_ops *ops);
> +/*
> + * Remove the driver ops from a specific context.
> + * The current count of AFU driver events must be 0.
> + */
> +int cxl_unset_driver_ops(struct cxl_context *ctx);
> +
> +/* Notify cxl driver that new events are ready to be delivered for =
context */
> +void cxl_context_events_pending(struct cxl_context *ctx,
> +				unsigned int new_events);
> +
> #endif /* _MISC_CXL_H */
> diff --git a/include/uapi/misc/cxl.h b/include/uapi/misc/cxl.h
> index 8cd334f..4fa36e4 100644
> --- a/include/uapi/misc/cxl.h
> +++ b/include/uapi/misc/cxl.h
> @@ -93,6 +93,7 @@ enum cxl_event_type {
> 	CXL_EVENT_AFU_INTERRUPT =3D 1,
> 	CXL_EVENT_DATA_STORAGE  =3D 2,
> 	CXL_EVENT_AFU_ERROR     =3D 3,
> +	CXL_EVENT_AFU_DRIVER    =3D 4,
> };
>=20
> struct cxl_event_header {
> @@ -124,12 +125,32 @@ struct cxl_event_afu_error {
> 	__u64 error;
> };
>=20
> +#define CXL_MAX_EVENT_DATA_SIZE 128
> +
> +struct cxl_event_afu_driver_reserved {
> +	/*
> +	 * Defines the buffer passed to the cxl driver by the AFU =
driver.
> +	 *
> +	 * This is not ABI since the event header.size passed to the =
user for
> +	 * existing events is set in the read call to =
sizeof(cxl_event_header)
> +	 * + sizeof(whatever event is being dispatched) and the user is =
already
> +	 * required to use a 4K buffer on the read call.
> +	 *
> +	 * Of course the contents will be ABI, but that's up the AFU =
driver.
> +	 *
> +	 * data_size is limited to MAX_EVENT_DATA_SIZE to prevent abuse.
> +	 */
> +	size_t data_size;
> +	u8 data[];
> +};
> +
> struct cxl_event {
> 	struct cxl_event_header header;
> 	union {
> 		struct cxl_event_afu_interrupt irq;
> 		struct cxl_event_data_storage fault;
> 		struct cxl_event_afu_error afu_error;
> +		struct cxl_event_afu_driver_reserved afu_driver_event;
> 	};
> };
>=20
> --=20
> 2.1.0
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events
  2016-05-23 12:49 [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events Philippe Bergheaud
                   ` (2 preceding siblings ...)
  2016-06-14 15:21 ` Matthew R. Ochs
@ 2016-06-16 13:05 ` Michael Ellerman
  2016-06-16 13:59   ` Philippe Bergheaud
  3 siblings, 1 reply; 9+ messages in thread
From: Michael Ellerman @ 2016-06-16 13:05 UTC (permalink / raw)
  To: Philippe Bergheaud, linuxppc-dev
  Cc: mikey, mrochs, manoj, imunsie, Philippe Bergheaud

On Mon, 2016-23-05 at 12:49:01 UTC, Philippe Bergheaud wrote:
> This adds an afu_driver_ops structure with deliver_event() and
> event_delivered() callbacks. An AFU driver such as cxlflash can fill
> this out and associate it with a context to enable passing custom
> AFU specific events to userspace.
> 
> This also adds a new kernel API function cxl_context_pending_events(),
> that the AFU driver can use to notify the cxl driver that new specific
> events are ready to be delivered, and wake up anyone waiting on the
> context wait queue.
...
> 
> Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>

It looks like the discussion has settled down on this one, and everyone was OK
with it, except maybe the naming?

So can we either get some ACKs, or a v6 with new naming?

cheers

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events
  2016-06-16 13:05 ` Michael Ellerman
@ 2016-06-16 13:59   ` Philippe Bergheaud
  0 siblings, 0 replies; 9+ messages in thread
From: Philippe Bergheaud @ 2016-06-16 13:59 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, mikey, mrochs, manoj, imunsie

Michael Ellerman wrote:
> On Mon, 2016-23-05 at 12:49:01 UTC, Philippe Bergheaud wrote:
> 
>>This adds an afu_driver_ops structure with deliver_event() and
>>event_delivered() callbacks. An AFU driver such as cxlflash can fill
>>this out and associate it with a context to enable passing custom
>>AFU specific events to userspace.
>>
>>This also adds a new kernel API function cxl_context_pending_events(),
>>that the AFU driver can use to notify the cxl driver that new specific
>>events are ready to be delivered, and wake up anyone waiting on the
>>context wait queue.
> 
> ...
> 
>>Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
> 
> 
> It looks like the discussion has settled down on this one, and everyone was OK
> with it, except maybe the naming?
> 
> So can we either get some ACKs, or a v6 with new naming?
> 
I am about to send a v6.

Philippe

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-06-16 13:58 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-23 12:49 [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events Philippe Bergheaud
2016-05-23 12:49 ` [v4,2/2] cxl: Add set and get private data to context struct Philippe Bergheaud
2016-05-24  6:59 ` [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events Vaibhav Jain
2016-05-24 14:44   ` Matthew R. Ochs
2016-05-25  7:22     ` Vaibhav Jain
2016-06-14 14:47       ` Matthew R. Ochs
2016-06-14 15:21 ` Matthew R. Ochs
2016-06-16 13:05 ` Michael Ellerman
2016-06-16 13:59   ` Philippe Bergheaud

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.