Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Date:   Wed, 13 Sep 2023 11:17:40 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Dan Williams <dan.j.williams@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Jonathan Cameron <jonathan.cameron@huawei.com>,
        Dave Jiang <dave.jiang@intel.com>,
        "Alison Schofield" <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>
CC:     <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Ira Weiny <ira.weiny@intel.com>
Subject: RE: [PATCH] cxl/mbox: Fix CEL logic for poison and security commands
Message-ID: <6501fcc4e2c9_31df4629421@iweiny-mobl.notmuch>
References: <20230903-cxl-cel-fix-v1-1-e260c9467be3@intel.com>
 <6500e8a179440_12747294a3@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6500e8a179440_12747294a3@dwillia2-xfh.jf.intel.com.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?YD8WVNNTCIkXE6nrwChJD+jPYaYxja13ZJ059UL5ZB9/wD/ufMhsv2Cc9qY2?=
 =?us-ascii?Q?aZ9v45qi0OwqlyFz6jEy4P31tGDnzOHSzV+4W2p8DuQ1RUlii/euHE5NoX8c?=
 =?us-ascii?Q?Jpfl9kSQooMwuD2/fLFUKrvYOMCsLS+Pidz06Jtsv8iN6yVj7rlcN0NWAEfv?=
 =?us-ascii?Q?hK7V6cx95XODWXjzjZ+0DSn+Mpk+OGiAxR11FxLSGcX0jmCbY4+TEoMJQU5n?=
 =?us-ascii?Q?AdZqDLQtSiF050He/Lks2QruVN6cl7tAPSExbPO+Admozqcqf/8aAcKt0ZW/?=
 =?us-ascii?Q?R/UfBaTFKX52DQxSpSaFmr20uo4DWMCQRT0pFxnm2Pm8jsxUPTRD5tpI0nzT?=
 =?us-ascii?Q?OTsnhPkCLimaMmLMGJODkSkSyPpPeChvXX9DpaWBosYaxt70/g7dSbOhvEUY?=
 =?us-ascii?Q?B0/FuLanEczNvgHliZamhtt9eTaVogwqjZsPjr7xgcfAT966ALt/GAynsuoz?=
 =?us-ascii?Q?hqOruKRzcDFKDPbLMC7psiBh49uuTQy9EnyOgGHqnfUWb3bGLrWzPGAB+ZLh?=
 =?us-ascii?Q?Es9ThUV1dQIeE7LB7TqqX7cgPJd1rkBIdtl5gASpRf0Xh+toa196YrFs5qx8?=
 =?us-ascii?Q?kidVEOdjgYRXKWRz5y/pMZt8xY6A8O2Hqkl2CW/GXqMWt8BTKXqSdlnjGPbr?=
 =?us-ascii?Q?mv97FxzIK1vtvNOjnDn3+fGNGI4J6LzyXfXOYPleZUra5BBrs7UGMREbMxGp?=
 =?us-ascii?Q?dQvtHAZsZkSXvxkZH3DwViUm+UEvybTOhxpo4FANURD5mMB9tzDvQZFa2S9I?=
 =?us-ascii?Q?qa4E3oghQV1uNFuHazBVmQo2lGMvL+aiusDnw5S93hmepZI80WR2zgQ3cz2a?=
 =?us-ascii?Q?1124EbjNLmyBB/Y2+i+bA0PkUALXKS1UOQEGk55UQ0GkDbnnqTI9TRPbRlOQ?=
 =?us-ascii?Q?u7XM80bTe0YcMMmmQYcsxsMo3G1/oTvGI8SvUUdoUSj7WQaRUM8W/FHGfOYq?=
 =?us-ascii?Q?LvujylwYwznDfVZGP6dJK5Nl/HffRU34uTLgdp6eLvDXcvaUOlI5WBkebgmh?=
 =?us-ascii?Q?q7t1NnwbjDNGxF+OMde9exhtTAAb8K7BEt1zJsu4YPOGsyWHnGvReerI1Euo?=
 =?us-ascii?Q?lifswS8Vgvk5DbbZSUc+T/+7VDKcfmW8UwO5oWQmm5mipzIfV9BYiSTAZw4a?=
 =?us-ascii?Q?SAiOkK0/JP7iZGFMpaDtCSK3Y7hrgRm//0+154v0k2k7nA8RMsrQw2aawdgy?=
 =?us-ascii?Q?wPVPjiIga6UEygCIBullV/TXz3jvIBc9wmnc0FZBu3cQJcCmAGTwRRlcMiZI?=
 =?us-ascii?Q?HU3Xg/QWUlldx7MoZdzXAEx+3dv6mO06OLQ5RQFweIzPZ86sV9sHtyu1pGfy?=
 =?us-ascii?Q?+6gQF+506IKSnhopLNQuEkQGJ4Q3H0YgJ7YUymbPN2QbZ/BFPWMOp7Le9nMJ?=
 =?us-ascii?Q?8NZ1+k0SwqDN2rSyzxVYeqYhiLBHrQjKatlHadZaJVyWrOQtCw+m4doDKQtk?=
 =?us-ascii?Q?GegjdpBkaY12truZqjR61t5y7VACZOnhSA6Z/rg92+q+WtLWTnhHLJVEDOth?=
 =?us-ascii?Q?3o+5JalItadFsZlvStGJxU2qqM3h5Q/QbRlxt2vYcoLbv2nd07m06a07uEWv?=
 =?us-ascii?Q?71V+B16GJm8kAY9Ww/TVWbawW+G3Bf+VFWTM97rJ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ba6b17fa-2663-4e60-1f78-08dbb485b973
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Sep 2023 18:17:43.9775
 (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: Iv1kSy4YUVYrECqmasMXx77SKuAXaOVl5430fxEkCRcuQeTNecy+4IwEJIGZ8d3e/n6oFIDzzoaVaJpnJvqP6w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7677
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Wed, 13 Sep 2023 11:18:45 -0700 (PDT)
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email

Dan Williams wrote:
> Ira Weiny wrote:
> > The following debug output was observed while testing CXL
> > 
> > cxl_core:cxl_walk_cel:721: cxl_mock_mem cxl_mem.0: Opcode 0x4300 unsupported by driver
> > 
> > opcode 0x4300 (Get Poison) is supported by the driver and the mock
> > device supports it.  The logic should be checking that the opcode is
> > both not poison and not security.
> > 
> > Fix the logic to allow poison and security commands.
> > 
> > Fixes: ad64f5952ce3 ("cxl/memdev: Only show sanitize sysfs files when supported")
> > Cc: Davidlohr Bueso <dave@stgolabs.net>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  drivers/cxl/core/mbox.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index ca60bb8114f2..b315bdab9197 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -716,8 +716,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
> >  		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
> >  		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
> >  
> > -		if (!cmd && (!cxl_is_poison_command(opcode) ||
> > -			     !cxl_is_security_command(opcode))) {
> > +		if (!cmd && !cxl_is_poison_command(opcode) &&
> > +		    !cxl_is_security_command(opcode)) {
> 
> Given that this is going to be a recurring pattern to add optional
> command support, I am not a fan of continuing to expand this boolean
> algebra eye exam.
> 
> How about the following to only do the validation check and enabling in
> one place:

I like it.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> 
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index ca60bb8114f2..4df4f614f490 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -715,24 +715,25 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  	for (i = 0; i < cel_entries; i++) {
>  		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
>  		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
> +		int enabled = 0;
>  
> -		if (!cmd && (!cxl_is_poison_command(opcode) ||
> -			     !cxl_is_security_command(opcode))) {
> -			dev_dbg(dev,
> -				"Opcode 0x%04x unsupported by driver\n", opcode);
> -			continue;
> -		}
> -
> -		if (cmd)
> +		if (cmd) {
>  			set_bit(cmd->info.id, mds->enabled_cmds);
> +			enabled++;
> +		}
>  
> -		if (cxl_is_poison_command(opcode))
> +		if (cxl_is_poison_command(opcode)) {
>  			cxl_set_poison_cmd_enabled(&mds->poison, opcode);
> +			enabled++;
> +		}
>  
> -		if (cxl_is_security_command(opcode))
> +		if (cxl_is_security_command(opcode)) {
>  			cxl_set_security_cmd_enabled(&mds->security, opcode);
> +			enabled++;
> +		}
>  
> -		dev_dbg(dev, "Opcode 0x%04x enabled\n", opcode);
> +		dev_dbg(dev, "Opcode 0x%04x %s\n", opcode,
> +			enabled ? "enabled" : "unsupported by driver");
>  	}
>  }
>