Hi Philippe,
Few comments,
Philippe Bergheaud <fe...@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
