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

[Dave Jiang <dave.jiang@intel.com>]

On 2/13/25 11:12 AM, Dan Williams wrote:
> 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.

Ok this patch is dropped for next rev.