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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E8662C433F5 for ; Thu, 27 Jan 2022 20:46:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235230AbiA0Uqz (ORCPT ); Thu, 27 Jan 2022 15:46:55 -0500 Received: from mga12.intel.com ([192.55.52.136]:17688 "EHLO mga12.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235860AbiA0Uqz (ORCPT ); Thu, 27 Jan 2022 15:46:55 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643316415; x=1674852415; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=iW3AE0TXTvU4wk8QfsMipZ1jcNRQkRmBhEPy1lUEpyQ=; b=ifnXkgF+FxHigymZeCwXA09WuKE5C5WCQ8Ra1PbkPQwe6tVk7HZaACmX vNME2uRFs9e5WsFbMo2T2Vcy+nzRFZMRZwlCUvQFX9aPZm50cgTRoAjM4 8xqZG2TUNSoFgJP5IHWhOqodVJaZRzvduh2x5FqGl/VUrbawjZENz7F61 EzGYfJQhDJK7BrOgwUnhJhZKIp08Y1cmxvSCR11CTwjaDYnW1VM8MEJIU DruVFV2HXyWShXa0SYcw8xeKLaiQzRgNwMxt8V5HMf7H5Wb916tn3mEWa WYtR6/Jxr3toE/WWWh7RhV8KhEuHxIlsE8ZiFnnIxlSckSJ3z3QvFKwSN w==; X-IronPort-AV: E=McAfee;i="6200,9189,10239"; a="226935793" X-IronPort-AV: E=Sophos;i="5.88,321,1635231600"; d="scan'208";a="226935793" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Jan 2022 12:45:40 -0800 X-IronPort-AV: E=Sophos;i="5.88,321,1635231600"; d="scan'208";a="618467759" Received: from alison-desk.jf.intel.com (HELO alison-desk) ([10.54.74.41]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Jan 2022 12:45:39 -0800 Date: Thu, 27 Jan 2022 12:50:09 -0800 From: Alison Schofield To: Dan Williams Cc: Ben Widawsky , Ira Weiny , Vishal Verma , Linux NVDIMM , linux-cxl@vger.kernel.org Subject: Re: [ndctl PATCH v3 5/6] libcxl: add interfaces for SET_PARTITION_INFO mailbox command Message-ID: <20220127205009.GA894403@alison-desk> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-cxl@vger.kernel.org Hi Dan, Thanks for the review. I'm still working thru this, but a clarifying question below... On Wed, Jan 26, 2022 at 03:41:14PM -0800, Dan Williams wrote: > On Tue, Jan 18, 2022 at 12:20 PM wrote: > > > > From: Alison Schofield > > > > Users may want the ability to change the partition layout of a CXL > > memory device. > > > > Add interfaces to libcxl to allocate and send a SET_PARTITION_INFO > > mailbox as defined in the CXL 2.0 specification. > > > > Signed-off-by: Alison Schofield > > --- > > cxl/lib/libcxl.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++ > > cxl/lib/libcxl.sym | 5 +++++ > > cxl/lib/private.h | 8 ++++++++ > > cxl/libcxl.h | 5 +++++ > > 4 files changed, 68 insertions(+) > > snip > > > I don't understand what this is for? > > Let's back up. In order to future proof against spec changes, and > endianness, struct packing and all other weird things that make struct > ABIs hard to maintain compatibility the ndctl project adopts the > libabc template of just not letting library consumers see any raw data > structures or bit fields by default [1]. For a situation like this > since the command only has one flag that affects the mode of operation > I would just go ahead and define an enum for that explicitly. > > enum cxl_setpartition_mode { > CXL_SETPART_NONE, > CXL_SETPART_NEXTBOOT, > CXL_SETPART_IMMEDIATE, > }; > > Then the main function prototype becomes: > > int cxl_cmd_new_setpartition(struct cxl_memdev *memdev, unsigned long > long volatile_capacity); > > ...with a new: > > int cxl_cmd_setpartition_set_mode(struct cxl_cmd *cmd, enum > cxl_setpartition_mode mode); > I don't understand setting of the mode separately. Can it be: int cxl_cmd_new_setpartition(struct cxl_memdev *memdev, unsigned long long volatile_capacity, enum cxl_setpartition_mode mode); > ...and it becomes impossible for users to pass unsupported flag > values. If the specification later on adds more flags then we can add > more: > > int cxl_cmd_setpartition_set_(struct cxl_cmd *cmd, enum > cxl_setpartition_X x); > > ...style helpers. > > Note, I was thinking CXL_SETPART_NONE is there to catch API users that > forget to set a mode, but I also don't mind skipping that and just > defaulting cxl_cmd_new_setpartition() to CXL_SETPART_NEXTBOOT, up to > you. > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/kay/libabc.git/tree/README#n99