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 X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 65883C433DB for ; Thu, 21 Jan 2021 18:41:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1B41622A83 for ; Thu, 21 Jan 2021 18:41:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726082AbhAUSli (ORCPT ); Thu, 21 Jan 2021 13:41:38 -0500 Received: from mga04.intel.com ([192.55.52.120]:41448 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726467AbhAUSlg (ORCPT ); Thu, 21 Jan 2021 13:41:36 -0500 IronPort-SDR: Gnj00ZfF8g+yyRNBTPSdbKOEzeY6vEYD9lam/VcD4VkpCLqnHsQTb2zq5nJGWLHKOSx+u06W9B YKu1ojPJ14SA== X-IronPort-AV: E=McAfee;i="6000,8403,9871"; a="176747681" X-IronPort-AV: E=Sophos;i="5.79,364,1602572400"; d="scan'208";a="176747681" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jan 2021 10:40:43 -0800 IronPort-SDR: 7zvoVKsgeq7e6EpaNxZ2fCy5jza869ddxJ26eTcXGKm5qrQRN2UecNPmGNGbsAw6hOC+sHspgL +L7o7L5zH5Yw== X-IronPort-AV: E=Sophos;i="5.79,364,1602572400"; d="scan'208";a="385412345" Received: from jonesmel-mobl.amr.corp.intel.com (HELO intel.com) ([10.252.130.74]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jan 2021 10:40:43 -0800 Date: Thu, 21 Jan 2021 10:40:41 -0800 From: Ben Widawsky To: Jonathan Cameron Cc: linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org, Ira Weiny , Dan Williams , Vishal Verma , "Kelley, Sean V" , Rafael Wysocki , Bjorn Helgaas , Jon Masters , Chris Browy , Randy Dunlap , Christoph Hellwig , daniel.lll@alibaba-inc.com Subject: Re: [RFC PATCH v3 13/16] cxl/mem: Create concept of enabled commands Message-ID: <20210121184041.r7x7tofhu2qqneus@intel.com> References: <20210111225121.820014-1-ben.widawsky@intel.com> <20210111225121.820014-15-ben.widawsky@intel.com> <20210114172531.0000347a@Huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210114172531.0000347a@Huawei.com> Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On 21-01-14 17:25:31, Jonathan Cameron wrote: > On Mon, 11 Jan 2021 14:51:18 -0800 > Ben Widawsky wrote: > > > CXL devices must implement the Device Command Interface (described in > > 8.2.9 of the CXL 2.0 spec). While the driver already maintains a list of > > commands it supports, there is still a need to be able to distinguish > > between commands that the driver knows about from commands that may not > > be supported by the hardware. No such commands currently are defined in > > the driver. > > > > The implementation leaves the statically defined table of commands and > > supplements it with a bitmap to determine commands that are enabled. > > > > --- > > > > There are multiple approaches that can be taken, but this is nice for a > > few reasons. > > > > Here are some of the other solutions: > > > > Create a per instance table with only the supported commands. > > 1. Having a fixed command id -> command mapping is much easier to manage > > for development and debugging. > > 2. Dealing with dynamic memory allocation for the table adds unnecessary > > complexity. > > 3. Most tables for device types are likely to be quite similar. > > 4. Makes it difficult to implement helper macros like cxl_for_each_cmd() > > > > If the per instance table did preserve ids, #1 above can be addressed. > > However, as "enable" is currently the only mutable state for the > > commands, it would yield a lot of overhead for not much gain. > > Additionally, the other issues remain. > > > > If "enable" remains the only mutable state, I believe this to be the > > best solution. Once the number of mutable elements in a command grows, > > it probably makes sense to move to per device instance state with a > > fixed command ID mapping. > Agreed with the logic. > > However, patch wise, should either drop the --- above or move this below the > --- after your sign off. Otherwise you run the risk of git dropping your > sign off and resulting complaints from anyone run validation scripts > of the kernel tree that check for that. > > > > > Signed-off-by: Ben Widawsky > > --- > > drivers/cxl/cxl.h | 4 ++++ > > drivers/cxl/mem.c | 38 +++++++++++++++++++++++++++++++++++++- > > 2 files changed, 41 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index 537ac4d8e6bd..963ba30cb200 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > @@ -17,6 +17,9 @@ > > > > #define CXL_GET_FIELD(word, field) FIELD_GET(field##_MASK, word) > > > > +/* XXX: Arbitrary max */ > > +#define CXL_MAX_COMMANDS 32 > > If going this way, probably want a build time check that you don't > go beyond this value for a given command set. I haven't actually > thought about how to construct that but should be easy enough. > I think BUILD_BUG_ON of ARRAY_SIZE mem_commands is sufficient. Let me know if you want anything more. > > + > > /* Device (CXL 2.0 - 8.2.8.3) */ > > #define CXLDEV_CAP_ARRAY_REG 0x0 > > #define CXLDEV_CAP_ARRAY_CAP_ID 0 > > @@ -88,6 +91,7 @@ struct cxl_mem { > > } ram; > > > > char firmware_version[0x10]; > > + DECLARE_BITMAP(enabled_cmds, CXL_MAX_COMMANDS); > > > > /* Cap 0001h - CXL_CAP_CAP_ID_DEVICE_STATUS */ > > struct { > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > index a824cfd4342a..20b26fa2c466 100644 > > --- a/drivers/cxl/mem.c > > +++ b/drivers/cxl/mem.c > > @@ -114,6 +114,8 @@ static struct { > > * would typically be used for deprecated commands. > > * * %CXL_CMD_FLAG_MANDATORY: Hardware must support this command. This flag is > > * only used internally by the driver for sanity checking. > > + * * %CXL_CMD_INTERNAL_FLAG_PSEUDO: This is a pseudo command which doesn't have > > + * a direct mapping to hardware. They are implicitly always enabled. > > * > > * The cxl_mem_command is the driver's internal representation of commands that > > * are supported by the driver. Some of these commands may not be supported by > > @@ -129,6 +131,7 @@ struct cxl_mem_command { > > #define CXL_CMD_INTERNAL_FLAG_NONE 0 > > #define CXL_CMD_INTERNAL_FLAG_HIDDEN BIT(0) > > #define CXL_CMD_INTERNAL_FLAG_MANDATORY BIT(1) > > +#define CXL_CMD_INTERNAL_FLAG_PSEUDO BIT(2) > > }; > > > > /* > > @@ -140,7 +143,7 @@ struct cxl_mem_command { > > static struct cxl_mem_command mem_commands[] = { > > CXL_CMD(INVALID, KERNEL, 0, 0, HIDDEN), > > CXL_CMD(IDENTIFY, NONE, 0, 0x43, MANDATORY), > > - CXL_CMD(RAW, NONE, ~0, ~0, MANDATORY), > > + CXL_CMD(RAW, NONE, ~0, ~0, PSEUDO), > > }; > > > > #define cxl_for_each_cmd(cmd) \ > > @@ -618,6 +621,10 @@ static int cxl_validate_cmd_from_user(struct cxl_mem *cxlm, > > c = &mem_commands[send_cmd->id]; > > info = &c->info; > > > > + /* Check that the command is enabled for hardware */ > > + if (!test_bit(cxl_cmd_index(c), cxlm->enabled_cmds)) > > + return -ENOTTY; > > + > > if (info->flags & CXL_MEM_COMMAND_FLAG_KERNEL) > > return -EPERM; > > > > @@ -1029,6 +1036,31 @@ static int cxl_mem_add_memdev(struct cxl_mem *cxlm) > > return rc; > > } > > > > +/** > > + * cxl_mem_enumerate_cmds() - Enumerate commands for a device. > > + * @cxlm: The device. > > + * > > + * Returns 0 if enumerate completed successfully. > > + * > > + * CXL devices have optional support for certain commands. This function will > > + * determine the set of supported commands for the hardware and update the > > + * enabled_cmds bitmap in the @cxlm. > > + */ > > +static int cxl_mem_enumerate_cmds(struct cxl_mem *cxlm) > > +{ > > + struct cxl_mem_command *c; > > + > > + /* All commands are considered enabled for now (except INVALID). */ > > + cxl_for_each_cmd(c) { > > + if (c->flags & CXL_CMD_INTERNAL_FLAG_HIDDEN) > > + continue; > > + > > + set_bit(cxl_cmd_index(c), cxlm->enabled_cmds); > > + } > > + > > + return 0; > > +} > > + > > /** > > * cxl_mem_identify() - Send the IDENTIFY command to the device. > > * @cxlm: The device to identify. > > @@ -1147,6 +1179,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > if (rc) > > goto err; > > > > + rc = cxl_mem_enumerate_cmds(cxlm); > > + if (rc) > > + return rc; > > + > > rc = cxl_mem_identify(cxlm); > > if (rc) > > goto err; >