Re: [PATCH v3 1/2] cxl: Add mechanism for delivering AFU driver specific events
From: Vaibhav Jain <hidden>
Date: 2016-03-09 14:38:21
Also in:
lkml
Hi Ian, Sorry for getting into this discussion late. I have few suggestions. Ian Munsie [off-list ref] writes:
quoted hunk ↗ jump to hunk
diff --git a/drivers/misc/cxl/Kconfig b/drivers/misc/cxl/Kconfig index 8756d06..560412c 100644 --- a/drivers/misc/cxl/Kconfig +++ b/drivers/misc/cxl/Kconfig@@ -15,12 +15,17 @@ config CXL_EEH bool default n +config CXL_AFU_DRIVER_OPS + bool + default n + config CXL tristate "Support for IBM Coherent Accelerators (CXL)" depends on PPC_POWERNV && PCI_MSI && EEH select CXL_BASE select CXL_KERNEL_API select CXL_EEH + select CXL_AFU_DRIVER_OPS
I suggest wrapping the driver_ops struct definition and other related functions inside a #ifdef CONFIG_CXL_AFU_DRIVER_OPS.
quoted hunk ↗ jump to hunk
default m help Select this option to enable driver support for IBM Coherentdiff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c index ea3eeb7..eebc9c3 100644 --- a/drivers/misc/cxl/api.c +++ b/drivers/misc/cxl/api.c@@ -296,6 +296,14 @@ struct cxl_context *cxl_fops_get_context(struct file *file) } EXPORT_SYMBOL_GPL(cxl_fops_get_context); +void cxl_set_driver_ops(struct cxl_context *ctx, + struct cxl_afu_driver_ops *ops) +{ + WARN_ON(!ops->event_pending || !ops->deliver_event); + ctx->afu_driver_ops = ops; +}
I would recommend adding a "struct module *" member to afu_driver_ops and doing a __module_get on to it here and module_put when we destroy the context. Since these callbacks will be residing within an external module .text region hence it should stay in the memory until the context is alive.
quoted hunk ↗ jump to hunk
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.
+/*
+ * 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 they will be
+ * delivered first if multiple types of events are pending.
+ *
+ * event_pending() will be called by the cxl driver to check if an event is
+ * pending (e.g. in select/poll/read calls).
+ *
+ * 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);
+};
+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 *); I think above apis would give us more flexbility in the future when drivers would want to send larger events without breaking the abi. Cheers, ~ Vaibhav