Re: [v5, 1/2] cxl: Add mechanism for delivering AFU driver specific events
From: Vaibhav Jain <hidden>
Date: 2016-05-24 07:01:37
Hi Philippe, Few comments, Philippe Bergheaud [off-list ref] writes:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
+ 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)
quoted hunk ↗ jump to hunk
@@ -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)
quoted hunk ↗ jump to hunk
+ 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