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

Ian Munsie [off-list ref] writes:

No, the kconfig option is there so that cxlflash can add support for
this and not have to worry about breaking any builds if their code is
merged into the scsi tree that doesn't have our code yet.

There is nothing optional about this within our driver, which is why
this is a select and has no configuration choice of it's own.

On a related matter, we should send a patch to remove some of the
leftover config options that were added to smooth the merging of
cxlflash in the first place (CXL_KERNEL_API, CXL_EEH).

Agreed.

quoted

quoted

diff --git a/drivers/misc/cxl/file.c b/drivers/misc/cxl/file.c
index 783337d..d1cc297 100644
--- a/drivers/misc/cxl/file.c
+++ b/drivers/misc/cxl/file.c

@@ -295,6 +295,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)
+        return ctx->afu_driver_ops->event_pending(ctx);

Should also check if ctx->afu_driver_ops->event_pending is NULL before
calling it.

The v1 patch did exactly that and mpe rejected it as it made this code
too ugly - we now check that event_pending field is valid when it is
registered and WARN if it is not.

The driver_ops struct pointer being passed is still owned by the
external module and its free change it even after calling this
function. We can mitigate this to some extent by accepting a const
pointer to the struct or by copying this struct to the context object.

I'm very reluctant to make this kind of change - while nice on paper,
poll() and read() are already very easy calls to screw up, and we have
seen that happen countless times in the past from different drivers that
e.g. and end up in a situation where poll says there is an event but
then read blocks, or poll blocks even though there is an event already
pending.

The API at the moment fits into the poll() / read() model and has
appropriate locking and the correct waiting semantics to avoid those
kind of issues (provided that the afu driver doesn't do something that
violates these semantics like sleep in one of these calls, but the
kernel has debug features to detect that), but any deviation from this
is too risky in my view.

Ian I have responded to Fred with an example implementation of these
calls. Requesting you to please take a look.

Cheers,
~ Vaibhav