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

[Vaibhav Jain]

Ian Munsie <imunsie@xxxxxxxxxxx> 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.

>> > 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