Received-SPF: pass (google.com: domain of linux-kernel+bounces-133565-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 5 Apr 2024 13:34:24 -0700
From: Ira Weiny <ira.weiny@intel.com>
To: Dave Jiang <dave.jiang@intel.com>, <ira.weiny@intel.com>, Fan Ni
	<fan.ni@samsung.com>, Jonathan Cameron <Jonathan.Cameron@huawei.com>, Navneet
 Singh <navneet.singh@intel.com>
CC: Dan Williams <dan.j.williams@intel.com>, Davidlohr Bueso
	<dave@stgolabs.net>, 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 06/26] cxl/port: Add Dynamic Capacity mode support to
 endpoint decoders
Message-ID: <6610605049149_e9f9f29439@iweiny-mobl.notmuch>
References: <20240324-dcd-type2-upstream-v1-0-b7b00d623625@intel.com>
 <20240324-dcd-type2-upstream-v1-6-b7b00d623625@intel.com>
 <891efc78-4f4b-494b-8c51-4f15db68ef23@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <891efc78-4f4b-494b-8c51-4f15db68ef23@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?+5k6f5yF7vaGRQJlCkrADSQk3POYqmIjN9P2j1yUTuK9C0YCk98V8fg5PfYY?=
 =?us-ascii?Q?SVQWojRx1wWqxS26yTKwf945Fh63vRZ8Hi7dCsfkXkv8fI1LTGcWJcQxAJMA?=
 =?us-ascii?Q?nzeT/LCo8rNGqDepSDh1y7ocwa4Z7mvxBrTZCy1MjWoEXi5j6G2FJVYprGxh?=
 =?us-ascii?Q?AVucChMt7mqWHJW0lyofJ5Qhcgolg5cI7mTXBkfEO2nxUMSyGFzv+Y2KpLwH?=
 =?us-ascii?Q?gpVLbGbPTrnII74mCLIlZhHju/u7nyE7fobcRfLd9qPvkv/N27durOGhfbjF?=
 =?us-ascii?Q?S1B3vFqgWKvQQjbWQxFaU+ZS+xtEVOgRmApxdhgs0k8rwkQiW+NVRpDqw4OM?=
 =?us-ascii?Q?9gCnfIqFVHzqLWaPQFKt+1Fiye+ALIVWf2UmZVATnNVAtcp5OtvQJCdjCUQd?=
 =?us-ascii?Q?WgBv8d06uVhiYjM/n+5m3+9pn1UMLrQ4dUwREef8I0j3oMvPFeR4aHGTNGvV?=
 =?us-ascii?Q?Gik/4V1IHIPTMe5w0XpU3NOwfg5NFACZlc5nnG/EGFCJXjq7qprA4hHhoOPF?=
 =?us-ascii?Q?Eib4WRCsLSILNCAZACBROdNnGx/OX4ceMrbo/e0QMuDOP6IMT52WAn+Vf6fX?=
 =?us-ascii?Q?yjf3rkz63Ag/HKVH38TKA9Ugfqo4w1RkHWRD0ztHhiHyg3F7MicdMrFym/hE?=
 =?us-ascii?Q?wyYFjan7H77bz/uym4AlGC9jcweUhFa9HjOUDBk9u9CkE+HllOAhnl5JH17z?=
 =?us-ascii?Q?KfRkbJDJtBPCkKVRgWUv0Pn+uXQV8EF2TEIpRcxwD2/CmqmzzKPeYp4Cq0RT?=
 =?us-ascii?Q?Oa2CqfEuJMGp1x3s0VYq0S5crzp8Tg9Z5lTKAzz6hjfB1sHOWruD62Goaiyd?=
 =?us-ascii?Q?pAVZGYOJmGwB8iENZMZTvfk3nC7WSwyv+kSW3QPIzr6nfCd13TwHXzjPiuOK?=
 =?us-ascii?Q?3OlMrKS3ZnJcDdTxVHQy7HOuCFVHmdmuTgOkd6Smjk51aIoynX0XS8F4Lrx1?=
 =?us-ascii?Q?HWGEnaMJueh0mEtjZC6GWcJh4kMA2bfGspVVPnlwB7o7YI2YUmx4e9l18VuN?=
 =?us-ascii?Q?TiPBqniOYdPgWww5o4KtGYFRsx6qO1wL9rvKNm/jgcaLGvXKVRi3mTZOT1hl?=
 =?us-ascii?Q?Jl3MRtwjLwfo+ARpy64vca4pmQAVfoTdUcjZIBiNaDTf3c/tAVcx75bvPelM?=
 =?us-ascii?Q?u90we18ugyRyFH92gyiRb3fMLAG0MWNgzRYlCNJPF1cF0hsABUtqpDamxkaV?=
 =?us-ascii?Q?60Coyt/qY2EFrj+cqITLn/zrY4rUB2AdxrF8hd1UvDuIld3NrQdzUfPrw5Va?=
 =?us-ascii?Q?iYDpSJX6zZSUIyHeZkNHbSydURX6IU0WEYGyqTKHSV1Hu748LFqZKOCI/nX2?=
 =?us-ascii?Q?kfc6jTVs2mvClDCtqVSCaE/Ofq2DKDMH1eYNFYUjohHLUc/UcauBOngy0NOC?=
 =?us-ascii?Q?Ifqg/13OZZ6/qSyuEEO3B1y9LacAL2T9i0caz3DDmJisQRw56k+GEl+jrQp2?=
 =?us-ascii?Q?sxIrsk1Q1jit7QvYdgPF/Go/GRyG4KX9JQbzzRP9vQi915G45M+tjirYZt2i?=
 =?us-ascii?Q?sPBuHFs/NL+QyD0A3eNeLQIr4/E/LlaycBgzSufXqW2cBwdL2aqd6P/ZpqU3?=
 =?us-ascii?Q?qjZX9kaxNqg4rj5hUzJl5g7d9wwHrQBB2oZbYmjy?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 023a8074-c0df-4b68-b10e-08dc55afcbef
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Apr 2024 20:34:31.1704
 (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: lI2hj6rU0jqiGSYu3Oto9iNOjjv0wfuq52f60BMoscQRrC3q0PNzA3CDCFfud5zpjl04svV+BITTSrXU/liitw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5305
X-OriginatorOrg: intel.com

Dave Jiang wrote:
> 
> 
> On 3/24/24 4:18 PM, ira.weiny@intel.com wrote:
> > From: Navneet Singh <navneet.singh@intel.com>
> > 

[snip]

> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 66b8419fd0c3..e22b6f4f7145 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -255,6 +255,14 @@ static void devm_cxl_dpa_release(struct cxl_endpoint_decoder *cxled)
> >  	__cxl_dpa_release(cxled);
> >  }
> >  
> > +static int dc_mode_to_region_index(enum cxl_decoder_mode mode)
> > +{
> > +	if (mode < CXL_DECODER_DC0 || CXL_DECODER_DC7 < mode)
> 
> I second what Fan said about readability here if you do (mode > CXL_DECODER_DC7) for upper bound check instead.
> 

Sure...


[snip]

> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index e59d9d37aa65..80c0651794eb 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -208,6 +208,22 @@ static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
> >  		mode = CXL_DECODER_PMEM;
> >  	else if (sysfs_streq(buf, "ram"))
> >  		mode = CXL_DECODER_RAM;
> > +	else if (sysfs_streq(buf, "dc0"))
> > +		mode = CXL_DECODER_DC0;
> > +	else if (sysfs_streq(buf, "dc1"))
> > +		mode = CXL_DECODER_DC1;
> > +	else if (sysfs_streq(buf, "dc2"))
> > +		mode = CXL_DECODER_DC2;
> > +	else if (sysfs_streq(buf, "dc3"))
> > +		mode = CXL_DECODER_DC3;
> > +	else if (sysfs_streq(buf, "dc4"))
> > +		mode = CXL_DECODER_DC4;
> > +	else if (sysfs_streq(buf, "dc5"))
> > +		mode = CXL_DECODER_DC5;
> > +	else if (sysfs_streq(buf, "dc6"))
> > +		mode = CXL_DECODER_DC6;
> > +	else if (sysfs_streq(buf, "dc7"))
> > +		mode = CXL_DECODER_DC7;
> 
> I think maybe create a static string table that correlates cxl_decoder_mode
> to string. Then you can simplify cxl_decoder_mode_name() and as well as here.
> And here I think you can just do a for loop and go through the entire static
> table. 

Ok...  Sure.

Ira