Received-SPF: pass (google.com: domain of linux-kernel+bounces-128789-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 2 Apr 2024 15:26:44 -0700
From: Ira Weiny <ira.weiny@intel.com>
To: Davidlohr Bueso <dave@stgolabs.net>, <ira.weiny@intel.com>
CC: Dave Jiang <dave.jiang@intel.com>, Fan Ni <fan.ni@samsung.com>, "Jonathan
 Cameron" <Jonathan.Cameron@huawei.com>, Navneet Singh
	<navneet.singh@intel.com>, Dan Williams <dan.j.williams@intel.com>, "Alison
 Schofield" <alison.schofield@intel.com>, Vishal Verma
	<vishal.l.verma@intel.com>, <linux-btrfs@vger.kernel.org>,
	<linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 01/26] cxl/mbox: Flag support for Dynamic Capacity
 Devices (DCD)
Message-ID: <660c8624cb3c9_8a7d029456@iweiny-mobl.notmuch>
References: <20240324-dcd-type2-upstream-v1-0-b7b00d623625@intel.com>
 <20240324-dcd-type2-upstream-v1-1-b7b00d623625@intel.com>
 <aczotz4a2xv6x4cse3hh5vpk57ekuwqii67pu46okdvdciae7i@qsopoigqhvw5>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <aczotz4a2xv6x4cse3hh5vpk57ekuwqii67pu46okdvdciae7i@qsopoigqhvw5>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?zp+I5kYchezsXRpMmZj6QEus1NeJ4Jd7ZxJcBOiQFltczoiutlCPZrhpXF5e?=
 =?us-ascii?Q?XewUxj9F5ieHIHbwgoJrUaSef3dodCVoSx+HadWjeiDa88z3Me0E8gBI6K7+?=
 =?us-ascii?Q?jhbfEsKzTmpRxh1/eMfACqHN+1u/o3/o4hqS4Nf2IXSbrdB/kEqwVie9x/6V?=
 =?us-ascii?Q?mFnjYdoRX9EkJl40OzvVKyEySQniK1VPlq2bTRzHeFujepGKRhH6fuoTigmh?=
 =?us-ascii?Q?B7xwlpb/DtTtQuvDGNjXVswo+9zj7OdkBpQfBDO7csfLYl6Zu/3uexiSErfr?=
 =?us-ascii?Q?Ud9BdVG5m5ObvHd5IgVNaa0Wb43oAfjiRVVPDioOM8siMc0avKOSGZvcAlne?=
 =?us-ascii?Q?hKKKWs8KFNI2L1Ao93/mT3zpLeBw/fSjKnttOp6NlkjozlLDrpsm+HkQWc6y?=
 =?us-ascii?Q?Q8YI9YRdr1759CsXCJUr0wpRzUoqUR7H7HDuLFTTUMh4YiPCb1kDFyvioHt6?=
 =?us-ascii?Q?kmd3TP8mPKJzdCiCGVRr5Zgife2Wpx9BJ4ugCJbUhDsmYaXeYIY76znzYpsd?=
 =?us-ascii?Q?KiICWJTGrnDrBrMMVezRMHwvH/AzQv1H1+Zp8E5Glnsk7djjHwNW0oP0Q+PA?=
 =?us-ascii?Q?cIfLnHBXyJSbbw6jrrnS3/SUVFAwZzRi2hUDZn07UCA8mOcnrCkjX0wVaRQC?=
 =?us-ascii?Q?OGZsl5aU5lK18o3WPFr+iOjIylnCa6qh0NjR3/FCytAvmBBqS1IQIQEFThLF?=
 =?us-ascii?Q?RyNVJRMJeCys30gXPCE6xc99nVA/bIoGnvrN3gQDpc7+KJwFB88DQO6BdBXL?=
 =?us-ascii?Q?Gv+M+B2yN9L4YOBCTYMsxu3uskgJAnjFjFrSs2lLhVPjALbWW1nxe0S5w20O?=
 =?us-ascii?Q?5pEpgxqgwDvUyt4GmBlY09ld+JoUtF6pBO4Ucpjt+w/oYMcK2bzYXn4gE4Uf?=
 =?us-ascii?Q?8/Xdu/TJ1xumPMoBN144t/htln+XtlY+NlG9R9tFkMphh2uDrU68nuRuRXVe?=
 =?us-ascii?Q?rlN3M9AiTmn5jkQ0xHk3QWTAIdHpio1dZrigJpA6bYtJK7pzNguvkxwolJGh?=
 =?us-ascii?Q?gdQyy+aLUZstPW+dslpQzcryPR2vWwg8LbStNZ3VvRXxT7GMi/hs6MI9yd3q?=
 =?us-ascii?Q?tVjE3gExz+dczmGmNqHXj4+tyc6SJwEEZx9pFiriEKFVxX1xNgX06MCzxUAI?=
 =?us-ascii?Q?/ihKkfPgMEqoGZtX/4hdMnbbHhYvxJIwzPMX+jflPZEmhavSnIK+gjOTq2eV?=
 =?us-ascii?Q?cl8VN2tbkCQCTekd9X6AObM0+/7Zd9da2CYSPz6HYSIfFs7pZZ11MCYZ0yKx?=
 =?us-ascii?Q?dHVgXRY8JHiCZtFwL2uHhkzjS67eA837R2aIKhKYXrZOcU/NZinX78r2nhy7?=
 =?us-ascii?Q?55ZxqdfXFiRCFQCkb9QksnVBUwXtUOFBQLg7cZ7Cz34b0QEpsxuYnvlheYip?=
 =?us-ascii?Q?Dbh+btE6dY4qVbvUJyEPG1YuSPLVzUDTqTZbVNYw4VRL04PSUgIHradSwrvM?=
 =?us-ascii?Q?cJtD3S77MvKT/ifwGolfKzFI9bdOi5hPQVsu7/w1I7eAqHgVguFT2e9YAcyG?=
 =?us-ascii?Q?tefAY7Xl1XkDPyQ2uW9nOtxT4oG+O/rTR13PqsBnn8NWD7Nk09QSNbW7EqtS?=
 =?us-ascii?Q?CafwqIHnzeXsAYDte1mwg55vrCx8PQj6jr/C9snS?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1fc74fd1-da33-491f-ae3c-08dc53640037
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Apr 2024 22:26:54.7058
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7g4w6QAelWcGkOnYskenpGjHTYhRPF2gw+TcShcuUVNz5L8WvbCFqVk8X3E5McMgBRk5q9YeTL4CqZ1MGsG20w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4709
X-OriginatorOrg: intel.com

Davidlohr Bueso wrote:
> On Sun, 24 Mar 2024, ira.weiny@intel.com wrote:
> 
> >From: Navneet Singh <navneet.singh@intel.com>
> >
> >Per the CXL 3.1 specification software must check the Command Effects
> >Log (CEL) to know if a device supports dynamic capacity (DC).  If the
> >device does support DC the specifics of the DC Regions (0-7) are read
> >through the mailbox.
> 
> I vote to fold this into patch 3, favoring reduced patch count in the
> series to trvially enlarging that particular patch.

I'll consider it.  I've tried hard to split the original series up into
very easily reviewable chunks.  So I'm inclined to leave this separate for
now.

> 
> >Flag DC Device (DCD) commands in a device if they are supported.
> >Subsequent patches will key off these bits to configure DCD.
> 
> It would be good to mention these here explicitly (if this patch will
> live on). For example, that config will be the driver's way of telling
> if dcd is enabled or disabled - we could have cases of that zeroed bit
> but the rest enabled.

It took me a bit to parse this but I see what you mean.  Yes the
GET_CONFIG command is the one used specifically to determine if DCD is
supported.

I'll clean up the commit message.  In retrospect it was wrong for me to
mention subsequent patches here.  I'm going to move this final detail to
patch 3.

> 
> lgtm otherwise.
> 
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Thanks!  :-D
Ira

> 
> >Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> >Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> >Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> >---
> >Changes for v1
> >[iweiny: update to latest master]
> >[iweiny: update commit message]
> >[iweiny: Based on the fix:
> >	https://lore.kernel.org/all/20230903-cxl-cel-fix-v1-1-e260c9467be3@intel.com/
> >[jonathan: remove unneeded format change]
> >[jonathan: don't split security code in mbox.c]
> >---
> > drivers/cxl/core/mbox.c | 33 +++++++++++++++++++++++++++++++++
> > drivers/cxl/cxlmem.h    | 15 +++++++++++++++
> > 2 files changed, 48 insertions(+)
> >
> >diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> >index 9adda4795eb7..ed4131c6f50b 100644
> >--- a/drivers/cxl/core/mbox.c
> >+++ b/drivers/cxl/core/mbox.c
> >@@ -161,6 +161,34 @@ static void cxl_set_security_cmd_enabled(struct cxl_security_state *security,
> >	}
> > }
> >
> >+static bool cxl_is_dcd_command(u16 opcode)
> >+{
> >+#define CXL_MBOX_OP_DCD_CMDS 0x48
> >+
> >+	return (opcode >> 8) == CXL_MBOX_OP_DCD_CMDS;
> >+}
> >+
> >+static void cxl_set_dcd_cmd_enabled(struct cxl_memdev_state *mds,
> >+					u16 opcode)
> >+{
> >+	switch (opcode) {
> >+	case CXL_MBOX_OP_GET_DC_CONFIG:
> >+		set_bit(CXL_DCD_ENABLED_GET_CONFIG, mds->dcd_cmds);
> >+		break;
> >+	case CXL_MBOX_OP_GET_DC_EXTENT_LIST:
> >+		set_bit(CXL_DCD_ENABLED_GET_EXTENT_LIST, mds->dcd_cmds);
> >+		break;
> >+	case CXL_MBOX_OP_ADD_DC_RESPONSE:
> >+		set_bit(CXL_DCD_ENABLED_ADD_RESPONSE, mds->dcd_cmds);
> >+		break;
> >+	case CXL_MBOX_OP_RELEASE_DC:
> >+		set_bit(CXL_DCD_ENABLED_RELEASE, mds->dcd_cmds);
> >+		break;
> >+	default:
> >+		break;
> >+	}
> >+}
> >+
> > static bool cxl_is_poison_command(u16 opcode)
> > {
> > #define CXL_MBOX_OP_POISON_CMDS 0x43
> >@@ -733,6 +761,11 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
> >			enabled++;
> >		}
> >
> >+		if (cxl_is_dcd_command(opcode)) {
> >+			cxl_set_dcd_cmd_enabled(mds, opcode);
> >+			enabled++;
> >+		}
> >+
> >		dev_dbg(dev, "Opcode 0x%04x %s\n", opcode,
> >			enabled ? "enabled" : "unsupported by driver");
> >	}
> >diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> >index 20fb3b35e89e..79a67cff9143 100644
> >--- a/drivers/cxl/cxlmem.h
> >+++ b/drivers/cxl/cxlmem.h
> >@@ -238,6 +238,15 @@ struct cxl_event_state {
> >	struct mutex log_lock;
> > };
> >
> >+/* Device enabled DCD commands */
> >+enum dcd_cmd_enabled_bits {
> >+	CXL_DCD_ENABLED_GET_CONFIG,
> >+	CXL_DCD_ENABLED_GET_EXTENT_LIST,
> >+	CXL_DCD_ENABLED_ADD_RESPONSE,
> >+	CXL_DCD_ENABLED_RELEASE,
> >+	CXL_DCD_ENABLED_MAX
> >+};
> >+
> > /* Device enabled poison commands */
> > enum poison_cmd_enabled_bits {
> >	CXL_POISON_ENABLED_LIST,
> >@@ -454,6 +463,7 @@ struct cxl_dev_state {
> >  *                (CXL 2.0 8.2.9.5.1.1 Identify Memory Device)
> >  * @mbox_mutex: Mutex to synchronize mailbox access.
> >  * @firmware_version: Firmware version for the memory device.
> >+ * @dcd_cmds: List of DCD commands implemented by memory device
> >  * @enabled_cmds: Hardware commands found enabled in CEL.
> >  * @exclusive_cmds: Commands that are kernel-internal only
> >  * @total_bytes: sum of all possible capacities
> >@@ -481,6 +491,7 @@ struct cxl_memdev_state {
> >	size_t lsa_size;
> >	struct mutex mbox_mutex; /* Protects device mailbox and firmware */
> >	char firmware_version[0x10];
> >+	DECLARE_BITMAP(dcd_cmds, CXL_DCD_ENABLED_MAX);
> >	DECLARE_BITMAP(enabled_cmds, CXL_MEM_COMMAND_ID_MAX);
> >	DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
> >	u64 total_bytes;
> >@@ -551,6 +562,10 @@ enum cxl_opcode {
> >	CXL_MBOX_OP_UNLOCK		= 0x4503,
> >	CXL_MBOX_OP_FREEZE_SECURITY	= 0x4504,
> >	CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE	= 0x4505,
> >+	CXL_MBOX_OP_GET_DC_CONFIG	= 0x4800,
> >+	CXL_MBOX_OP_GET_DC_EXTENT_LIST	= 0x4801,
> >+	CXL_MBOX_OP_ADD_DC_RESPONSE	= 0x4802,
> >+	CXL_MBOX_OP_RELEASE_DC		= 0x4803,
> >	CXL_MBOX_OP_MAX			= 0x10000
> > };
> >
> >
> >--
> >2.44.0
> >