Re: [PATCH v2 1/2] cxl: Add mechanism for delivering AFU driver specific events

[Matt Ochs]

A couple of minor nits below...

> On Mar 7, 2016, at 12:59 PM, Ian Munsie <imunsie@xxxxxxxxxxx> wrote:
> 
> @@ -346,7 +350,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_adapter_link_ok(ctx->afu->adapter)) {
> @@ -376,7 +380,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 && ctx->afu_driver_ops->event_pending(ctx)) {
> +		pr_devel("afu_read delivering AFU driver specific event\n");
> +		event.header.type = CXL_EVENT_AFU_DRIVER;
> +		ctx->afu_driver_ops->deliver_event(ctx, &event, sizeof(event));
> +		WARN_ON(event.header.size > sizeof(event));
> +
> +	} 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;
> @@ -384,6 +395,7 @@ ssize_t afu_read(struct file *file, char __user *buf, size_t count,
> 		clear_bit(event.irq.irq - 1, ctx->irq_bitmap);
> 		if (bitmap_empty(ctx->irq_bitmap, ctx->irq_count))
> 			ctx->pending_irq = false;
> +
> 	} else if (ctx->pending_fault) {
> 		pr_devel("afu_read delivering data storage fault\n");
> 		event.header.size += sizeof(struct cxl_event_data_storage);
> @@ -391,12 +403,14 @@ ssize_t afu_read(struct file *file, char __user *buf, size_t count,
> 		event.fault.addr = ctx->fault_addr;
> 		event.fault.dsisr = ctx->fault_dsisr;
> 		ctx->pending_fault = false;
> +
> 	} else if (ctx->pending_afu_err) {
> 		pr_devel("afu_read delivering afu error\n");
> 		event.header.size += sizeof(struct cxl_event_afu_error);
> 		event.header.type = CXL_EVENT_AFU_ERROR;
> 		event.afu_error.error = ctx->afu_err;
> 		ctx->pending_afu_err = false;
> +

Any reason for adding these extra lines as part of this commit?

> diff --git a/include/misc/cxl.h b/include/misc/cxl.h
> index f2ffe5b..f198a42 100644
> --- a/include/misc/cxl.h
> +++ b/include/misc/cxl.h
> @@ -210,4 +210,33 @@ ssize_t cxl_fd_read(struct file *file, char __user *buf, size_t count,
> void cxl_perst_reloads_same_image(struct cxl_afu *afu,
> 				  bool perst_reloads_same_image);
> 
> +/*
> + * AFU driver ops allows 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 will be delivered

so _they_ will be delivered

> + * first if multiple types of events are pending.
> + *
> + * even_pending() will be called by the cxl driver to check if an event is
> + * pending (e.g. in select/poll/read calls).

event_pending() <- missing 't'

> + *
> + * deliver_event() will be called to fill out a cxl_event structure with the
> + * driver specific event. The header will already have the type and
> + * process_element fields filled in, and header.size will be set to
> + * sizeof(struct cxl_event_header). The AFU driver can extend that size up to
> + * max_size (if an afu driver requires more space, they should submit a patch
> + * increasing the size in the struct cxl_event_afu_driver_reserved definition).
> + *
> + * Both of these calls are made with a spin lock held, so they must not sleep.
> + */
> +struct cxl_afu_driver_ops {
> +	bool (*event_pending) (struct cxl_context *ctx);
> +	void (*deliver_event) (struct cxl_context *ctx,
> +			struct cxl_event *event, size_t max_size);
> +};
> +
> +/* Associate the above driver ops with a specific context */
> +void cxl_set_driver_ops(struct cxl_context *ctx,
> +			struct cxl_afu_driver_ops *ops);
> +
> #endif /* _MISC_CXL_H */
> diff --git a/include/uapi/misc/cxl.h b/include/uapi/misc/cxl.h
> index 1e889aa..8b097db 100644
> --- a/include/uapi/misc/cxl.h
> +++ b/include/uapi/misc/cxl.h
> @@ -69,6 +69,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 {
> @@ -100,12 +101,33 @@ struct cxl_event_afu_error {
> 	__u64 error;
> };
> 
> +struct cxl_event_afu_driver_reserved {
> +	/*
> +	 * Reserves space for AFU driver specific events. Not actually
> +	 * reserving any more space compared to other events as we can't know
> +	 * how much an AFU driver will need (but it is likely to be small). If
> +	 * your AFU driver needs more than this, please submit a patch
> +	 * increasing it as part of your driver submission.
> +	 *
> +	 * 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 will not increase
> +	 * just because this is, and the user is already required to use a 4K
> +	 * buffer on the read call. This is merely the size of the buffer
> +	 * passed between the cxl and AFU drivers.
> +	 *
> +	 * Of course the contents will be ABI, but that's up the AFU driver.
> +	 */
> +	__u64 reserved[4];
> +};
> +
> 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.4
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@xxxxxxxxxxxxxxxx
> https://lists.ozlabs.org/listinfo/linuxppc-dev