Re: [PATCH v4 19/25] nvdimm/ocxl: Forward events to userspace
On Tue, Mar 31, 2020 at 1:59 AM Alastair D'Silva wrote:
>
> Some of the interrupts that the card generates are better handled
> by the userspace daemon, in particular:
> Controller Hardware/Firmware Fatal
> Controller Dump Available
> Error Log available
>
> This patch allows a userspace application to register an eventfd with
> the driver via SCM_IOCTL_EVENTFD to receive notifications of these
> interrupts.
>
> Userspace can then identify what events have occurred by calling
> SCM_IOCTL_EVENT_CHECK and checking against the SCM_IOCTL_EVENT_FOO
> masks.
The amount new ioctl's in this driver is too high, it seems much of
this data can be exported via sysfs attributes which are more
maintainable that ioctls. Then sysfs also has the ability to signal
events on sysfs attributes, see sys_notify_dirent.
Can you step back and review the ABI exposure of the driver and what
can be moved to sysfs? If you need to have bus specific attributes
ordered underneath the libnvdimm generic attributes you can create a
sysfs attribute subdirectory.
In general a roadmap document of all the proposed ABI is needed to
make sure it is both sufficient and necessary. See the libnvdimm
document that introduced the initial libnvdimm ABI:
https://www.kernel.org/doc/Documentation/nvdimm/nvdimm.txt
>
> Signed-off-by: Alastair D'Silva
> ---
> drivers/nvdimm/ocxl/main.c | 220 +
> drivers/nvdimm/ocxl/ocxlpmem.h | 4 +
> include/uapi/nvdimm/ocxlpmem.h | 12 ++
> 3 files changed, 236 insertions(+)
>
> diff --git a/drivers/nvdimm/ocxl/main.c b/drivers/nvdimm/ocxl/main.c
> index 0040fc09cceb..cb6cdc9eb899 100644
> --- a/drivers/nvdimm/ocxl/main.c
> +++ b/drivers/nvdimm/ocxl/main.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -301,8 +302,19 @@ static void free_ocxlpmem(struct ocxlpmem *ocxlpmem)
> {
> int rc;
>
> + // Disable doorbells
> + (void)ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CHIEC,
> +OCXL_LITTLE_ENDIAN,
> +GLOBAL_MMIO_CHI_ALL);
> +
> free_minor(ocxlpmem);
>
> + if (ocxlpmem->irq_addr[1])
> + iounmap(ocxlpmem->irq_addr[1]);
> +
> + if (ocxlpmem->irq_addr[0])
> + iounmap(ocxlpmem->irq_addr[0]);
> +
> if (ocxlpmem->ocxl_context) {
> rc = ocxl_context_detach(ocxlpmem->ocxl_context);
> if (rc == -EBUSY)
> @@ -398,6 +410,11 @@ static int file_release(struct inode *inode, struct file
> *file)
> {
> struct ocxlpmem *ocxlpmem = file->private_data;
>
> + if (ocxlpmem->ev_ctx) {
> + eventfd_ctx_put(ocxlpmem->ev_ctx);
> + ocxlpmem->ev_ctx = NULL;
> + }
> +
> ocxlpmem_put(ocxlpmem);
> return 0;
> }
> @@ -928,6 +945,52 @@ static int ioctl_controller_stats(struct ocxlpmem
> *ocxlpmem,
> return rc;
> }
>
> +static int ioctl_eventfd(struct ocxlpmem *ocxlpmem,
> +struct ioctl_ocxlpmem_eventfd __user *uarg)
> +{
> + struct ioctl_ocxlpmem_eventfd args;
> +
> + if (copy_from_user(&args, uarg, sizeof(args)))
> + return -EFAULT;
> +
> + if (ocxlpmem->ev_ctx)
> + return -EBUSY;
> +
> + ocxlpmem->ev_ctx = eventfd_ctx_fdget(args.eventfd);
> + if (IS_ERR(ocxlpmem->ev_ctx))
> + return PTR_ERR(ocxlpmem->ev_ctx);
> +
> + return 0;
> +}
> +
> +static int ioctl_event_check(struct ocxlpmem *ocxlpmem, u64 __user *uarg)
> +{
> + u64 val = 0;
> + int rc;
> + u64 chi = 0;
> +
> + rc = ocxlpmem_chi(ocxlpmem, &chi);
> + if (rc < 0)
> + return rc;
> +
> + if (chi & GLOBAL_MMIO_CHI_ELA)
> + val |= IOCTL_OCXLPMEM_EVENT_ERROR_LOG_AVAILABLE;
> +
> + if (chi & GLOBAL_MMIO_CHI_CDA)
> + val |= IOCTL_OCXLPMEM_EVENT_CONTROLLER_DUMP_AVAILABLE;
> +
> + if (chi & GLOBAL_MMIO_CHI_CFFS)
> + val |= IOCTL_OCXLPMEM_EVENT_FIRMWARE_FATAL;
> +
> + if (chi & GLOBAL_MMIO_CHI_CHFS)
> + val |= IOCTL_OCXLPMEM_EVENT_HARDWARE_FATAL;
> +
> + if (copy_to_user((u64 __user *)uarg, &val, sizeof(val)))
> + return -EFAULT;
> +
> + return rc;
> +}
> +
> static long file_ioctl(struct file *file, unsigned int cmd, unsigned long
> args)
> {
> struct ocxlpmem *ocxlpmem = file->private_data;
> @@ -956,6 +1019,15 @@ static long file_ioctl(struct file *file, unsigned int
> cmd, unsigned long args)
> rc = ioctl_controller_stats(ocxlpmem,
> (struct
> ioctl_ocxlpmem_controller_stats __user *)args);
> break;
> +
> + case IOCTL_OCXLPMEM_EVENTFD:
> + rc = ioctl_eventfd(ocxlpmem,
> + (struct ioctl_ocxlpmem_eventfd __user
> *)args);
> + br
[PATCH v4 19/25] nvdimm/ocxl: Forward events to userspace
Some of the interrupts that the card generates are better handled
by the userspace daemon, in particular:
Controller Hardware/Firmware Fatal
Controller Dump Available
Error Log available
This patch allows a userspace application to register an eventfd with
the driver via SCM_IOCTL_EVENTFD to receive notifications of these
interrupts.
Userspace can then identify what events have occurred by calling
SCM_IOCTL_EVENT_CHECK and checking against the SCM_IOCTL_EVENT_FOO
masks.
Signed-off-by: Alastair D'Silva
---
drivers/nvdimm/ocxl/main.c | 220 +
drivers/nvdimm/ocxl/ocxlpmem.h | 4 +
include/uapi/nvdimm/ocxlpmem.h | 12 ++
3 files changed, 236 insertions(+)
diff --git a/drivers/nvdimm/ocxl/main.c b/drivers/nvdimm/ocxl/main.c
index 0040fc09cceb..cb6cdc9eb899 100644
--- a/drivers/nvdimm/ocxl/main.c
+++ b/drivers/nvdimm/ocxl/main.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -301,8 +302,19 @@ static void free_ocxlpmem(struct ocxlpmem *ocxlpmem)
{
int rc;
+ // Disable doorbells
+ (void)ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CHIEC,
+OCXL_LITTLE_ENDIAN,
+GLOBAL_MMIO_CHI_ALL);
+
free_minor(ocxlpmem);
+ if (ocxlpmem->irq_addr[1])
+ iounmap(ocxlpmem->irq_addr[1]);
+
+ if (ocxlpmem->irq_addr[0])
+ iounmap(ocxlpmem->irq_addr[0]);
+
if (ocxlpmem->ocxl_context) {
rc = ocxl_context_detach(ocxlpmem->ocxl_context);
if (rc == -EBUSY)
@@ -398,6 +410,11 @@ static int file_release(struct inode *inode, struct file
*file)
{
struct ocxlpmem *ocxlpmem = file->private_data;
+ if (ocxlpmem->ev_ctx) {
+ eventfd_ctx_put(ocxlpmem->ev_ctx);
+ ocxlpmem->ev_ctx = NULL;
+ }
+
ocxlpmem_put(ocxlpmem);
return 0;
}
@@ -928,6 +945,52 @@ static int ioctl_controller_stats(struct ocxlpmem
*ocxlpmem,
return rc;
}
+static int ioctl_eventfd(struct ocxlpmem *ocxlpmem,
+struct ioctl_ocxlpmem_eventfd __user *uarg)
+{
+ struct ioctl_ocxlpmem_eventfd args;
+
+ if (copy_from_user(&args, uarg, sizeof(args)))
+ return -EFAULT;
+
+ if (ocxlpmem->ev_ctx)
+ return -EBUSY;
+
+ ocxlpmem->ev_ctx = eventfd_ctx_fdget(args.eventfd);
+ if (IS_ERR(ocxlpmem->ev_ctx))
+ return PTR_ERR(ocxlpmem->ev_ctx);
+
+ return 0;
+}
+
+static int ioctl_event_check(struct ocxlpmem *ocxlpmem, u64 __user *uarg)
+{
+ u64 val = 0;
+ int rc;
+ u64 chi = 0;
+
+ rc = ocxlpmem_chi(ocxlpmem, &chi);
+ if (rc < 0)
+ return rc;
+
+ if (chi & GLOBAL_MMIO_CHI_ELA)
+ val |= IOCTL_OCXLPMEM_EVENT_ERROR_LOG_AVAILABLE;
+
+ if (chi & GLOBAL_MMIO_CHI_CDA)
+ val |= IOCTL_OCXLPMEM_EVENT_CONTROLLER_DUMP_AVAILABLE;
+
+ if (chi & GLOBAL_MMIO_CHI_CFFS)
+ val |= IOCTL_OCXLPMEM_EVENT_FIRMWARE_FATAL;
+
+ if (chi & GLOBAL_MMIO_CHI_CHFS)
+ val |= IOCTL_OCXLPMEM_EVENT_HARDWARE_FATAL;
+
+ if (copy_to_user((u64 __user *)uarg, &val, sizeof(val)))
+ return -EFAULT;
+
+ return rc;
+}
+
static long file_ioctl(struct file *file, unsigned int cmd, unsigned long args)
{
struct ocxlpmem *ocxlpmem = file->private_data;
@@ -956,6 +1019,15 @@ static long file_ioctl(struct file *file, unsigned int
cmd, unsigned long args)
rc = ioctl_controller_stats(ocxlpmem,
(struct
ioctl_ocxlpmem_controller_stats __user *)args);
break;
+
+ case IOCTL_OCXLPMEM_EVENTFD:
+ rc = ioctl_eventfd(ocxlpmem,
+ (struct ioctl_ocxlpmem_eventfd __user
*)args);
+ break;
+
+ case IOCTL_OCXLPMEM_EVENT_CHECK:
+ rc = ioctl_event_check(ocxlpmem, (u64 __user *)args);
+ break;
}
return rc;
@@ -1109,6 +1181,148 @@ static void dump_error_log(struct ocxlpmem *ocxlpmem)
kfree(buf);
}
+static irqreturn_t imn0_handler(void *private)
+{
+ struct ocxlpmem *ocxlpmem = private;
+ u64 chi = 0;
+
+ (void)ocxlpmem_chi(ocxlpmem, &chi);
+
+ if (chi & GLOBAL_MMIO_CHI_ELA) {
+ dev_warn(&ocxlpmem->dev, "Error log is available\n");
+
+ if (ocxlpmem->ev_ctx)
+ eventfd_signal(ocxlpmem->ev_ctx, 1);
+ }
+
+ if (chi & GLOBAL_MMIO_CHI_CDA) {
+ dev_warn(&ocxlpmem->dev, "Controller dump is available\n");
+
+ if (ocxlpmem->ev_ctx)
+ eventfd_signal(ocxlpmem->ev_ctx, 1);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t imn1_handler(void *private)
+{
+ struct ocxlpmem *ocxlpmem = private;
+ u64 chi = 0;
+
