Received-SPF: pass (google.com: domain of linux-kernel+bounces-137962-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 9 Apr 2024 23:09:55 -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 14/26] cxl/region: Read existing extents on region
 creation
Message-ID: <66162d33d480c_e9f9f294e5@iweiny-mobl.notmuch>
References: <20240324-dcd-type2-upstream-v1-0-b7b00d623625@intel.com>
 <20240324-dcd-type2-upstream-v1-14-b7b00d623625@intel.com>
 <c2aa9a19-a0ab-46c7-91fc-f3903142097b@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <c2aa9a19-a0ab-46c7-91fc-f3903142097b@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?bbFX5ARbquz/A1O+ImmJ0Zvv1Y4t7WjW991F95V+ePoQG7AClS9Mh/BMj1bc?=
 =?us-ascii?Q?2E79HdOz9k31u+R/VdDN2C7peupOyHL8gjmxk+UsKQxjsbn7xIXFQ6jwGpsF?=
 =?us-ascii?Q?ZjGke77aq9tvM+tMR75OVultg8/XhS9juzxU9RMCasWGOPMKrw8Ja2yuK7ES?=
 =?us-ascii?Q?YjIOsQDQDr7Vg3j5/pEsdQBtOr2i2TymLgnovgPTOWOFL3g/iWPWPp40otPR?=
 =?us-ascii?Q?Ys4Onv4GV8UWXaaL+GQK2ZvKltwt5HRHlNGDYnGRHHPPkAcmAZ34cwe6XSjv?=
 =?us-ascii?Q?2yc+jhTS47fNUJPavLwpBzXbSArcAYzDR4biMbQwipBAjTgie2wDkJb//K93?=
 =?us-ascii?Q?OrAPxsqbRjsM46+AMkZt4KfEYblReTj2TdpOq5o7BmCL8xtHUXKet+Nxf+7B?=
 =?us-ascii?Q?e+kmbJ4XYgqvMSvnLuo40uDLOk3S0jyotZLt+INm9IQDOpVUsDDAh6mZ2Qdv?=
 =?us-ascii?Q?mloU1npoVCIOwETpi9g/ysC/qBTTMtqv6CU8T5uCDXE+B6uFWdmtSHLgKOP7?=
 =?us-ascii?Q?U0cdKENtc+oR1kpWn9Eu/ux/gOah0hSPDYfFIO0oGYOXw49jcVes/qHV+bqF?=
 =?us-ascii?Q?/P2BelRJrosoQdfVgUIw9XCoPwJ+VwOaoJ2McOUMpol3EC5QPsQ6/3PTcfch?=
 =?us-ascii?Q?nNRL6iiS/lkNhUDPbTz4W1PCthrhI+hlDtzLtR+47Fhw2oCxAKHKPo1bUaz4?=
 =?us-ascii?Q?QCqtV1zM6MM39IcIsQAs3BTQXNR+MZ+paqQR0a3uR9d0hke5WikupFqXjFAo?=
 =?us-ascii?Q?tXeeO5Q1TavjQTn4CQfh0vflnfeKdXjUitu8dAsdSPbVpJTdsj8ihlVz8xvq?=
 =?us-ascii?Q?xwgtKxSirWxdG89KsmMTgfaVRI5cSNd+Imz9H17CLdBMwljze5r2xMGT8Of6?=
 =?us-ascii?Q?knidd1ijIlvZ5xoacgWdVjskFVQhRf7KmC+LcBqqEoVKqcyhscQxYw/byw2f?=
 =?us-ascii?Q?yamjjif97/ULHWGS0F1KVkU1VynHvm82EFtmZRg+oJrtk5c+K5TNKdTplnjp?=
 =?us-ascii?Q?udPd2x9RO/wv3fe1+y/FuySw5VUel0eABS65ZURmi/iXqSMMgKp0gPkTJ9dQ?=
 =?us-ascii?Q?BTYlq/Ohh836bPkBl7jvzTxERp/BBGTY/KWgbNMqxPV46iVf/9gpIRSXlOBH?=
 =?us-ascii?Q?9YNffSLzKtf2wIe5qSRZa7bJYuiaWcUub/q/mKZev27qUKXEgiTqoOCv2EdY?=
 =?us-ascii?Q?jamem5zsRTPeHoSDZaXmRppvGcdeBKJ0zmkN+hgWAaINS8SDWy3274g6FgYP?=
 =?us-ascii?Q?Tvm/i16OVLkxT1b0f+09AraLYhl18sXrwZatRScKt0sjggXYkDBpee125UpR?=
 =?us-ascii?Q?R6eowxk8D9vNTCUkN7Ppy5N40J/NeGrXUHDBBGhHHTIDjOIh/no9jXh8mRuC?=
 =?us-ascii?Q?WJ9Nr1qNoxF2Gq2HqTsJDam7jnVhP0Lj0azg9GrTW/WaZR1CcoIJ6YVuabjA?=
 =?us-ascii?Q?bQ83g8LL1PG/X0EfewQgeaB7tIGL64+XMamOKti7j46jJ8HHGK3tFWhX94K4?=
 =?us-ascii?Q?gDGNSm3w4y2P7uoss4bz7Ggm0p5X3RNZ9m9+AT9OnzQ4pauDz5RXTB/spHl+?=
 =?us-ascii?Q?VcvrsOHHP1LdZLeccydmEPUSXWCYKdmQULkkdgxZ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c3a93165-8edb-4e0e-ebb0-08dc5924d9e6
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Apr 2024 06:09:59.0539
 (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: tsT9oqtcpdwWAFMtOo4SRZtUkfX4ELOqQ4ns73IzwWnIMQtMvGe5pFWakRHvRKk/Nd84TFuzLYLEcD0ajq/g4A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4947
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/mbox.c b/drivers/cxl/core/mbox.c
> > index 58b31fa47b93..9e33a0976828 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -870,6 +870,53 @@ int cxl_enumerate_cmds(struct cxl_memdev_state *mds)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_enumerate_cmds, CXL);
> >  
> > +static int cxl_validate_extent(struct cxl_memdev_state *mds,
> > +			       struct cxl_dc_extent *dc_extent)
> > +{
> > +	struct device *dev = mds->cxlds.dev;
> > +	uint64_t start, len;
> u64
> 

Yep

> 
> > +
> > +	start = le64_to_cpu(dc_extent->start_dpa);
> > +	len = le64_to_cpu(dc_extent->length);
> > +
> > +	/* Extents must not cross region boundary's */
> > +	for (int i = 0; i < mds->nr_dc_region; i++) {
> > +		struct cxl_dc_region_info *dcr = &mds->dc_region[i];
> > +
> > +		if (dcr->base <= start &&
> > +		    (start + len) <= (dcr->base + dcr->decode_len)) {
> 
> Can range_contains() be used here as well?

Yep and done.

> 
> > +			dev_dbg(dev, "DC extent DPA %#llx - %#llx (DCR:%d:%#llx)\n",
> > +				start, start + len - 1, i, start - dcr->base);
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	dev_err_ratelimited(dev,
> > +			    "DC extent DPA %#llx - %#llx is not in any DC region\n",
> > +			    start, start + len - 1);
> > +	return -EINVAL;
> > +}
> > +
> > +static bool cxl_dc_extent_in_ed(struct cxl_endpoint_decoder *cxled,
> 
> cxl_dc_extent_in_endpoint_decoder() is more readable

Sure but we are getting awful close to Java naming there...  j/k  ;-)  Changed.

> 
> > +				struct cxl_dc_extent *extent)
> > +{
> > +	uint64_t start = le64_to_cpu(extent->start_dpa);
> > +	uint64_t length = le64_to_cpu(extent->length);
> u64
> 

Yep


[snip]

> >  
> > +static struct cxl_memdev_state *
> > +cxled_to_mds(struct cxl_endpoint_decoder *cxled)
> > +{
> > +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > +
> > +	return container_of(cxlds, struct cxl_memdev_state, cxlds);
> > +}
> > +
> >  static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
> >  				    enum cxl_event_log_type type)
> >  {
> > @@ -1406,6 +1462,142 @@ int cxl_dev_dynamic_capacity_identify(struct cxl_memdev_state *mds)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_dev_dynamic_capacity_identify, CXL);
> >  
> > +static int cxl_dev_get_dc_extent_cnt(struct cxl_memdev_state *mds,
> 
> cxl_dev_get_dc_extent_generation()? or spell out count

I'll spell out count because that is the primary goal.  The generation number
is just to be able to check each query to ensure the list does not change
whilst reading.

Ira