Re: [PATCH v5 08/15] cxl: Add support for FWCTL get driver information callback

[Dan Williams <dan.j.williams@intel.com>]

Saeed Mahameed wrote:
> On 11 Feb 11:28, Dave Jiang wrote:
> >Add definition for fwctl_ops->info() to return driver information. The
> >function will return a data structure with a reserved u32. This is setup
> >for future usages.
> >
> >Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> >Reviewed-by: Li Ming <ming.li@zohomail.com>
> >Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >---
> > drivers/cxl/core/features.c | 11 +++++++++--
> > include/cxl/mailbox.h       |  1 +
> > include/uapi/fwctl/cxl.h    | 19 +++++++++++++++++++
> > 3 files changed, 29 insertions(+), 2 deletions(-)
> > create mode 100644 include/uapi/fwctl/cxl.h
> >
> >diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> >index db48a8fbde81..923c054599ad 100644
> >--- a/drivers/cxl/core/features.c
> >+++ b/drivers/cxl/core/features.c
> >@@ -354,8 +354,15 @@ static void cxlctl_close_uctx(struct fwctl_uctx *uctx)
> >
> > static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
> > {
> >-	/* Place holder */
> >-	return ERR_PTR(-EOPNOTSUPP);
> >+	struct fwctl_info_cxl *info;
> >+
> >+	info = kzalloc(sizeof(*info), GFP_KERNEL);
> >+	if (!info)
> >+		return ERR_PTR(-ENOMEM);
> >+
> >+	*length = sizeof(*info);
> >+
> >+	return info;
> > }
> >
> > static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
> >diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
> >index c4e99e2e3a9d..60a26a0d8f1f 100644
> >--- a/include/cxl/mailbox.h
> >+++ b/include/cxl/mailbox.h
> >@@ -4,6 +4,7 @@
> > #define __CXL_MBOX_H__
> > #include <linux/rcuwait.h>
> > #include <cxl/features.h>
> >+#include <uapi/fwctl/cxl.h>
> > #include <uapi/linux/cxl_mem.h>
> >
> > /**
> >diff --git a/include/uapi/fwctl/cxl.h b/include/uapi/fwctl/cxl.h
> >new file mode 100644
> >index 000000000000..d21fd6b80c20
> >--- /dev/null
> >+++ b/include/uapi/fwctl/cxl.h
> >@@ -0,0 +1,19 @@
> >+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >+/*
> >+ * Copyright (c) 2024, Intel Corporation
> >+ *
> >+ * These are definitions for the mailbox command interface of CXL subsystem.
> >+ */
> >+#ifndef _UAPI_FWCTL_CXL_H_
> >+#define _UAPI_FWCTL_CXL_H_
> >+
> >+#include <linux/types.h>
> >+
> >+/**
> >+ * struct fwctl_info_cxl - ioctl(FWCTL_INFO) out_device_data
> >+ * @reserved: zero
> >+ */
> >+struct fwctl_info_cxl {
> >+	__u32 reserved;
> >+};
> 
> I would add some useful info such as: num_user_features, mbox->feature_cap,
> etc ...

I much prefer Jason's suggestion of just allowing the subsystem to
opt-out of the 'info' command.

The Features that are private / exclusive to the kernel are a small
fixed list that is unlikely to grow.

There is no need for an info command when documentation can communicate
what CXL Features are available through the fwctl interface.