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

[Vaibhav Jain <vaibhav@linux.vnet.ibm.com>]

Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes:

> Hi Vaibhav,
>
> Le 09/03/2016 15:37, Vaibhav Jain a écrit :
>
>> I would propose these two apis.
>>
>> /*
>> *  fetches an event from the driver event queue. NULL means that queue
>> *  is empty. Can sleep if needed. The memory for cxl_event is allocated
>> *  by module being called. Hence it can be potentially be larger then
>> *  sizeof(struct cxl_event). Multiple calls to this should return same
>> *  pointer untill ack_event is called.
>> */
>> struct cxl_event * fetch_event(struct cxl_context * ctx);
>>
>> /*
>> * Returns and acknowledge the struct cxl_event * back to the driver
>> * which can then free it or maybe put it back in a kmem_cache. This
>> * should be called once we have completely returned the current
>> * struct cxl_event from the readcall
>> */
>> void ack_event(struct cxl_context * ctx, struct cxl_event *);
>
>
> How would you implement polling on those APIs?
Hi Fred. I am looking at an implementation similar to this:

static inline bool ctx_event_pending(struct cxl_context *ctx)
{
	typeof (ctx->afu_driver_ops->fetch_event) fn_events =
		(ctx->afu_driver_ops != NULL) ? ctx->afu_driver_ops->fetch_event : NULL;

	if (ctx->pending_irq || ctx->pending_fault || ctx->pending_afu_err)
		return true;

        /*
	 * if fn_event returns a not null then its gauranteed to return
	 * the same pointer on next call
	 */
	if (fn_events)
		return fn_events(ctx) != NULL;

	return false;
}

unsigned int afu_poll(struct file *file, struct poll_table_struct *poll)
{
	struct cxl_context *ctx = file->private_data;
	int mask = 0;
	unsigned long flags;


	poll_wait(file, &ctx->wq, poll);

	pr_devel("afu_poll wait done pe: %i\n", ctx->pe);

	spin_lock_irqsave(&ctx->lock, flags);
	if (ctx_event_pending(ctx))
		mask |= POLLIN | POLLRDNORM;
	else if (ctx->status == CLOSED)
		/* Only error on closed when there are no futher events pending
		 */
		mask |= POLLERR;
	spin_unlock_irqrestore(&ctx->lock, flags);

	pr_devel("afu_poll pe: %i returning %#x\n", ctx->pe, mask);

	return mask;
}

> How would you implement afu_read? There are several sources of events.
Looking at an implementation similar to this:

ssize_t afu_read(struct file *file, char __user *buf, size_t count,
			loff_t *off)
{
	unsigned long flags;
	ssize_t rc = 0;
	struct cxl_context *ctx = file->private_data;
	struct cxl_event *ptr_event, event = {
		.header.process_element = ctx->pe,
		.header.size = sizeof(struct cxl_event_header)
	};
	typeof (ctx->afu_driver_ops->fetch_event) fn_fetch_event =
		(ctx->afu_driver_ops != NULL) ? ctx->afu_driver_ops->fetch_event : NULL;
	typeof (ctx->afu_driver_ops->ack_event) fn_ack_event =
		(ctx->afu_driver_ops != NULL) ? ctx->afu_driver_ops->ack_event : NULL;
 
	if (count < CXL_READ_MIN_SIZE)
		return -EINVAL;

       if (!cxl_adapter_link_ok(ctx->afu->adapter) ||
	   ctx->status == CLOSED)
	       return -EIO;

       if (signal_pending(current))
	       return -ERESTARTSYS;

       /* if no events then wait */
       if (!ctx_event_pending(ctx)) {
	       
	       if ((file->f_flags & O_NONBLOCK))
		       return -EAGAIN;

	       pr_devel("afu_read going to sleep...\n");
	       rc = wait_event_interruptible(ctx->wq,
					     (ctx->status == CLOSED) ||
					     cxl_adapter_link_ok(ctx->afu->adapter) ||
					     ctx_event_pending(ctx));
	       pr_devel("afu_read woken up\n");

       }
       
       /* did we get interrupted during wait sleep */
       if (rc)
	       return rc;

       /* get driver events if any */
       ptr_event = fn_fetch_event ? fn_fetch_event(ctx) : NULL;

       /* In case of error feching driver specific event */
       if (IS_ERR(ptr_event)) {
	       pr_warn("Error fetching driver specific event %ld", PTR_ERR(ptr_event));
	       ptr_event = NULL;
       }

       /* code below manipulates ctx so take a spin lock */
       spin_lock_irqsave(&ctx->lock, flags);

       /* give driver events first priority */
       if (ptr_event) {
		pr_devel("afu_read delivering AFU driver specific event\n");
		/* populate the header type and pe in the event struct */
		ptr_event->header.type = CXL_EVENT_AFU_DRIVER;
		ptr_event->header.process_element = ctx->pe;
		WARN_ON(event.header.size > count);

       } 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;
		event.irq.irq = find_first_bit(ctx->irq_bitmap, ctx->irq_count) + 1;
		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);
		event.header.type = CXL_EVENT_DATA_STORAGE;
		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;

	} else if (ctx->status == CLOSED) {
		pr_devel("afu_read fatal error\n");
		rc = -EIO;
	} else
		WARN(1, "afu_read must be buggy\n");

	spin_unlock_irqrestore(&ctx->lock, flags);

	if (!rc) {
		/* if we dont have a driver event then use 'event' var */
		ptr_event = ptr_event ? ptr_event : &event;
	
		rc = min(((size_t)ptr_event->header.size), count);
	
		if (copy_to_user(buf, ptr_event, rc))
			rc = -EFAULT;
	}
	/* if its a driver event ack it back to the driver */
	if (fn_ack_event && (ptr_event != &event))
		fn_ack_event(ctx, ptr_event);
		
	return rc;
}

Cheers,
~ Vaibhav