From: Dan Williams <dan.j.williams@intel.com>
To: Dave Jiang <dave.jiang@intel.com>, "linux-cxl@vger.kernel.org"
	<linux-cxl@vger.kernel.org>
CC: "dan.j.williams@intel.com" <dan.j.williams@intel.com>,
	"ira.weiny@intel.com" <ira.weiny@intel.com>, "vishal.l.verma@intel.com"
	<vishal.l.verma@intel.com>, "alison.schofield@intel.com"
	<alison.schofield@intel.com>, Jonathan Cameron <jonathan.cameron@huawei.com>,
	"dave@stgolabs.net" <dave@stgolabs.net>, "jgg@nvidia.com" <jgg@nvidia.com>,
	Shiju Jose <shiju.jose@huawei.com>
Subject: Re: [RFC PATCH v2 04/20] cxl/mbox: Add GET_FEATURE mailbox command
Thread-Topic: [RFC PATCH v2 04/20] cxl/mbox: Add GET_FEATURE mailbox command
Thread-Index: AQHbN6U//ktgkagc00iV+8Wv/2ktBLLYb+oA
Date: Fri, 6 Dec 2024 00:44:20 +0000
Message-ID: <675248e44c037_25073294f4@dwillia2-xfh.jf.intel.com.notmuch>
References: <20241115212745.869552-1-dave.jiang@intel.com>
 <20241115212745.869552-5-dave.jiang@intel.com>
In-Reply-To: <20241115212745.869552-5-dave.jiang@intel.com>
Content-Language: en-GB
X-MS-Exchange-Organization-Network-Message-Id: 036af65b-d274-4f6e-66a8-08dd158f2689
X-MS-TNEF-Correlator:
X-MS-Exchange-Organization-RecordReviewCfmType: 0
x-ms-exchange-organization-originalclientipaddress: 172.19.92.136
x-ms-exchange-organization-originalserveripaddress: 10.3.19.182
x-ms-publictraffictype: Email
Content-Type: text/plain; charset="us-ascii"
Content-ID: <8F8C68F4B1594B47971144A863BFB535@huawei.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Dave Jiang wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
>
> Add support for GET_FEATURE mailbox command.
>
> CXL spec 3.1 section 8.2.9.6 describes optional device specific features.
> The settings of a feature can be retrieved using Get Feature command.
> CXL spec 3.1 section 8.2.9.6.2 describes Get Feature command.
>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> This patch is not needed by this series except the opcode enumeration.
> ---

Wait, then why is it included?

I would say fold the small bit that you need into the previous patch,
especially if we think that fwctl can cover all the "get feature" use
cases.

>  drivers/cxl/core/mbox.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h    | 26 ++++++++++++++++++++++++++
>  2 files changed, 67 insertions(+)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 4ba56f3d5a65..f70bd7d28ff8 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -968,6 +968,47 @@ int cxl_get_supported_feature_entry(struct cxl_dev_s=
tate *cxlds, const uuid_t *f
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_get_supported_feature_entry, CXL);
>
> +size_t cxl_get_feature(struct cxl_dev_state *cxlds, const uuid_t feat_uu=
id,
> +                    enum cxl_get_feat_selection selection,
> +                    void *feat_out, size_t feat_out_size)
> +{
> +     struct cxl_mailbox *cxl_mbox =3D &cxlds->cxl_mbox;

cxlds is otherwise unused in this function, just pass in @cxl_mbox
directly.