From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8E62AC48BC4 for ; Tue, 20 Feb 2024 11:03:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AD68B6B0072; Tue, 20 Feb 2024 06:03:06 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A851C6B0074; Tue, 20 Feb 2024 06:03:06 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 94DA96B0075; Tue, 20 Feb 2024 06:03:06 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 85BE26B0072 for ; Tue, 20 Feb 2024 06:03:06 -0500 (EST) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 2407580626 for ; Tue, 20 Feb 2024 11:03:06 +0000 (UTC) X-FDA: 81811895172.25.064E9A3 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by imf21.hostedemail.com (Postfix) with ESMTP id 229F51C0008 for ; Tue, 20 Feb 2024 11:03:02 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf21.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708426983; a=rsa-sha256; cv=none; b=figL9aOlDfkWYYijEqBuq4GWoHyCDnqw1D/4XnBPj9fw+fe9zNfTJHJj7ILHwB6FakVeHZ FiRzGEVyzd1B5+wVVpNHD2hvCssCcGEL6YlYGDaLwXkGjDJ3Stbgwp7kBnyluFJZZcqfT8 e8WB/MD260XJrSQahpUBQgYBji9zd1o= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf21.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1708426983; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4Rpwkp7ZZ7ntBVhe5rEDiE3i9s5/hew3NWrc0L+4g3E=; b=m+E8KheP/M+Q5kt9gPt1/5+lNLiQ2Mjx8qA0xO9ykr3G9rXOA2iH7t+31LMXf5MjaHoRcT gd359sQLfjIerzpzSWR8rmqS7ClY3WH7fJAcLdxHVtfdnnuAzKkbqYTTMYyRwtNOa5+rza 5LW1+GnQXmkjV/cVx0rY3JknNJvvjKU= Received: from mail.maildlp.com (unknown [172.18.186.231]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4TfGcM5KXGz6K6QG; Tue, 20 Feb 2024 18:58:59 +0800 (CST) Received: from lhrpeml500005.china.huawei.com (unknown [7.191.163.240]) by mail.maildlp.com (Postfix) with ESMTPS id 18AFD140B54; Tue, 20 Feb 2024 19:02:59 +0800 (CST) Received: from localhost (10.202.227.76) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Tue, 20 Feb 2024 11:02:58 +0000 Date: Tue, 20 Feb 2024 11:02:56 +0000 From: Jonathan Cameron To: CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [RFC PATCH v6 01/12] cxl/mbox: Add GET_SUPPORTED_FEATURES mailbox command Message-ID: <20240220110256.00001586@Huawei.com> In-Reply-To: <20240215111455.1462-2-shiju.jose@huawei.com> References: <20240215111455.1462-1-shiju.jose@huawei.com> <20240215111455.1462-2-shiju.jose@huawei.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: quoted-printable X-Originating-IP: [10.202.227.76] X-ClientProxiedBy: lhrpeml500003.china.huawei.com (7.191.162.67) To lhrpeml500005.china.huawei.com (7.191.163.240) X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 229F51C0008 X-Stat-Signature: x7hgc4qyai7z3ribu1yekmaxk1i9rjdy X-Rspam-User: X-HE-Tag: 1708426982-880329 X-HE-Meta: U2FsdGVkX1/UubJ04tS/6DP0G0N9noV75SApNmnud1E+jBLSLFVNvuWSYY92xhSrPZlxeFrg4S27Kw9qJGeyAbIS1vAA+EGAEj+eh/XGul6jlMKxzrrAPyMb8ojQ9xwSPQLqTcvF3F4n40ZOToLRD1utg873o2acPNicVJjkOlCIZiQdWVgHlpq9+sf9a4u5a1X4T/ezEJrgoSIqQ5QJuJoh8uHbuJj9yAKSI+FPdFDJ+DVmxSpL8jw204+kmmdoLAEp2MnODhpwIWIp9j9oHNLmZWpc5LIFyl32CXrIYFSnag4LjiYPdEcwZGl9qadguflYyU3tJ7yaDcC2NguP+DMc4bCWALXNof/yDM/qK8Mmx2zXUwo/2QqCdIBSUZPm45iuotgW/nShoR53gmdxVQGl5jtG1BBMwOuFgEOSzaNXiin0nAfLIWet+bjnsdpaX76t1K/jIsg6Av/+/10dT/FqI/syzwDPrcTNQWdFirqiEf0F5RPBetlFKku9/rrInH6/+3Asd45UIag4eroYtJY3wH5yMOYkVtKYJ/AwxDfGAi3qA8VER+iUEa2XBSmF7TjOV/PIoiKAVOlt1Cfg9fuqb+Nt+FFPtHX8eThPSUpC6mKpbSFbuTFb/uRb1yiPseflJXlgglQTxNE1KzAbVelp6fV4KuHVajuB7cXnBIWIBTJJbstjS8FROnCoNsexmUkfB+y5l3rKyOFnolcX59zmJsQlUudsR1T/pHt9hjOROP+5WJnS7PcuXSSWREe/CeueyFzoEGCC81hWjqt9z1UFQGfAnIDqJlGDW9i0z2ePmheKih+XKoZBQNoJEDsQgT/5U9geYcK5QFHYyqlyb9Fv7Fjg+xekJyxKmW6EfaZX+gWybr8Xq2c48lYqQVsxQDDmB36MyVyd/cDiGArfoGUkzFpaxaMiB9R7Cr3Df/IMKAntyU1CKjQMof4IAsB2uPrJG0/HI1zXdE63fAe Wsa4ZIkB pzp1/EgHZ6x9c5B6i5zdCPa1VAetobyFYOIpCyTy9Qt/5fBTzcEsUXGZOIoEq91qNcM4dK8R8LZcQBlATtQN5j1d7t029CxAl5NpWdgh5zZVOrOf0wO4Sl+D0Yur8bN8LbDi7X+Cg04mcEGaHQgfkmWOxzsSyuPIYhEZlyX9UhoaCo5ACHYIpvwlTkxUne/gf9kAW6BvPgC8JpoEmu3CRs8vou5jVHWMx2sjWxQcHwFQJ4qyu8l9EwiYdYuLbNaAJSc8rFxUXeaZHFuB7WEOKqg7Ovw== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Thu, 15 Feb 2024 19:14:43 +0800 wrote: > From: Shiju Jose >=20 > Add support for GET_SUPPORTED_FEATURES mailbox command. >=20 > CXL spec 3.1 section 8.2.9.6 describes optional device specific features. > CXL devices supports features with changeable attributes. > Get Supported Features retrieves the list of supported device specific > features. The settings of a feature can be retrieved using Get Feature > and optionally modified using Set Feature. >=20 > Signed-off-by: Shiju Jose Hi Shiju, Some comment inline. Mostly just naming suggestions. Actual functionality looks good to me. > --- > drivers/cxl/core/mbox.c | 23 +++++++++++++++ > drivers/cxl/cxlmem.h | 62 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 85 insertions(+) >=20 > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 27166a411705..191f51f3df0e 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -1290,6 +1290,29 @@ int cxl_set_timestamp(struct cxl_memdev_state *mds) > } > EXPORT_SYMBOL_NS_GPL(cxl_set_timestamp, CXL); > =20 > +int cxl_get_supported_features(struct cxl_memdev_state *mds, > + struct cxl_mbox_get_supp_feats_in *pi, > + void *feats_out) Odd indent. Align the later lines with s of struct Comments on input types in header below. > +{ > + struct cxl_mbox_cmd mbox_cmd; > + int rc; > + > + mbox_cmd =3D (struct cxl_mbox_cmd) { > + .opcode =3D CXL_MBOX_OP_GET_SUPPORTED_FEATURES, > + .size_in =3D sizeof(*pi), > + .payload_in =3D pi, > + .size_out =3D le32_to_cpu(pi->count), > + .payload_out =3D feats_out, > + .min_out =3D sizeof(struct cxl_mbox_get_supp_feats_out), feats_out should be typed, in which case this becomes sizeof(*feats_out) > + }; > + rc =3D cxl_internal_send_cmd(mds, &mbox_cmd); > + if (rc < 0) > + return rc; > + > + return 0; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL); > + > int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len, > struct cxl_region *cxlr) > { > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index 5303d6942b88..23e4d98b9bae 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -529,6 +529,7 @@ enum cxl_opcode { > CXL_MBOX_OP_SET_TIMESTAMP =3D 0x0301, > CXL_MBOX_OP_GET_SUPPORTED_LOGS =3D 0x0400, > CXL_MBOX_OP_GET_LOG =3D 0x0401, > + CXL_MBOX_OP_GET_SUPPORTED_FEATURES =3D 0x0500, > CXL_MBOX_OP_IDENTIFY =3D 0x4000, > CXL_MBOX_OP_GET_PARTITION_INFO =3D 0x4100, > CXL_MBOX_OP_SET_PARTITION_INFO =3D 0x4101, > @@ -698,6 +699,64 @@ struct cxl_mbox_set_timestamp_in { > =20 > } __packed; > =20 > +/* Get Supported Features CXL 3.1 Spec 8.2.9.6.1 */ > +/* > + * Get Supported Features input payload > + * CXL rev 3.1 section 8.2.9.6.1 Table 8-95 > + */ > +struct cxl_mbox_get_supp_feats_in { > + __le32 count; > + __le16 start_index; > + u16 reserved; =46rom a local style point of view using a u16 for reserved is a new style choice - best to avoid that - most common option looks to be u8 rsvd[2]; > +} __packed; > + > +/* > + * Get Supported Features Supported Feature Entry > + * CXL rev 3.1 section 8.2.9.6.1 Table 8-97 > + */ > +/* Supported Feature Entry : Payload out attribute flags */ > +#define CXL_FEAT_ENTRY_FLAG_CHANGABLE BIT(0) > +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_MASK GENMASK(3, 1) > +#define CXL_FEAT_ENTRY_FLAG_PERSIST_ACROSS_FIRMWARE_UPDATE BIT(4) > +#define CXL_FEAT_ENTRY_FLAG_SUPPORT_DEFAULT_SELECTION BIT(5) > +#define CXL_FEAT_ENTRY_FLAG_SUPPORT_SAVED_SELECTION BIT(6) > + > +enum cxl_feat_attr_value_persistence { > + CXL_FEAT_ATTR_VALUE_PERSISTENCE_NONE, > + CXL_FEAT_ATTR_VALUE_PERSISTENCE_CXL_RESET, > + CXL_FEAT_ATTR_VALUE_PERSISTENCE_HOT_RESET, > + CXL_FEAT_ATTR_VALUE_PERSISTENCE_WARM_RESET, > + CXL_FEAT_ATTR_VALUE_PERSISTENCE_COLD_RESET, > + CXL_FEAT_ATTR_VALUE_PERSISTENCE_MAX > +}; > + > +#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_ACROSS_FW_UPDATE_MASK BIT(4) Not sure there is benefit in defining mask for single bit fields. Or if there is don't define the value above. > +#define CXL_FEAT_ENTRY_FLAG_PERSIST_ACROSS_FIRMWARE_UPDATE BIT(4) Either is probably fine, just not both! > +#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_DEFAULT_SEL_SUPPORT_MASK BIT(5) > +#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_SAVED_SEL_SUPPORT_MASK BIT(6) > + > +struct cxl_mbox_supp_feat_entry { > + uuid_t uuid; > + __le16 feat_index; Given it's in a feat entry, could drop 'feat' as redundant info. __le16 index; __le16 get_size; etc > + __le16 get_feat_size; > + __le16 set_feat_size; > + __le32 attr_flags; > + u8 get_feat_version; > + u8 set_feat_version; > + __le16 set_feat_effects; > + u8 rsvd[18]; > +} __packed; > + > +/* > + * Get Supported Features output payload > + * CXL rev 3.1 section 8.2.9.6.1 Table 8-96 > + */ > +struct cxl_mbox_get_supp_feats_out { > + __le16 entries; > + __le16 nsuppfeats_dev; Probably don't need the _dev postfix. Command being sent to a device so that doesn't add much. I looked at naming in similar cases. For mailbox clear we have nr_recs, so perhaps nr_supported ? > + u32 reserved; u8 rsvd[4]; as above - match the local syle. > + struct cxl_mbox_supp_feat_entry feat_entries[]; > +} __packed; > + > /* Get Poison List CXL 3.0 Spec 8.2.9.8.4.1 */ > struct cxl_mbox_poison_in { > __le64 offset; > @@ -829,6 +888,9 @@ void cxl_event_trace_record(const struct cxl_memdev *= cxlmd, > enum cxl_event_type event_type, > const uuid_t *uuid, union cxl_event *evt); > int cxl_set_timestamp(struct cxl_memdev_state *mds); > +int cxl_get_supported_features(struct cxl_memdev_state *mds, > + struct cxl_mbox_get_supp_feats_in *pi, > + void *feats_out); Don't use a void * for that output data. It should be a struct cxl_mbox_get_supp_feats_out * For the input, the other similar functions are providing parameters directly, not wrapped up in a structure. Easy enough to do that here as well as we only need u32 count, u16 start_index instead of pi > int cxl_poison_state_init(struct cxl_memdev_state *mds); > int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len, > struct cxl_region *cxlr);