Re: [PATCH RFC v2 11/18] cxl/region: Expose DC extents on region driver load

[Ira Weiny <ira.weiny@intel.com>]

Dave Jiang wrote:
> 
> 
> On 8/28/23 22:21, Ira Weiny wrote:
> >   

[snip]

> > +static bool cxl_dc_extent_in_ed(struct cxl_endpoint_decoder *cxled,
> > +				struct cxl_dc_extent_data *extent)
> > +{
> > +	struct range dpa_range = (struct range){
> > +		.start = extent->dpa_start,
> > +		.end = extent->dpa_start + extent->length - 1,
> > +	};
> > +	struct device *dev = &cxled->cxld.dev;
> > +
> > +	dev_dbg(dev, "Checking extent DPA:%llx LEN:%llx\n",
> > +		extent->dpa_start, extent->length);
> > +
> > +	if (!cxled->cxld.region || !cxled->dpa_res)
> > +		return false;
> > +
> > +	dev_dbg(dev, "Cxled start:%llx end:%llx\n",
> > +		cxled->dpa_res->start, cxled->dpa_res->end);
> 
> Just use %pr?

Yep!

> 
> > +	return (cxled->dpa_res->start <= dpa_range.start &&
> > +		dpa_range.end <= cxled->dpa_res->end);
> 
> I may be easier to read for some if you have (dpa_range.start > 
> cxled->dpa_res->start && ...) instead.

<sigh>  I think about checks like this visually.

Resource
	As                        Ae
	|-------------------------|
Check
	   Bs                   Be
	   |--------------------|

	As <= Bs && Be <= Ae

I know this is odd for some but I like seeing B 'inside' A.

If others feel strongly like you I can change it but I'm inclined to leave
it.

[snip]

> > +
> > +#define DAX_EXTENT_LABEL_LEN 64
> > +/**
> > + * struct dax_reg_ext_dev - Device object to expose extent information
> > + * @dev: device representing this extent
> > + * @dr_extent: reference back to private extent data
> > + * @offset: offset of this extent
> > + * @length: size of this extent
> > + * @label: identifier to group extents
> > + */
> > +struct dax_reg_ext_dev {
> > +	struct device dev;
> > +	struct dax_region_extent *dr_extent;
> > +	resource_size_t offset;
> > +	resource_size_t length;
> > +	char label[DAX_EXTENT_LABEL_LEN];
> > +};
> > +
> > +int dax_region_ext_create_dev(struct dax_region *dax_region,
> > +			      struct dax_region_extent *dr_extent,
> > +			      resource_size_t offset,
> > +			      resource_size_t length,
> > +			      const char *label);
> > +#define to_dr_ext_dev(dev)	\
> > +	container_of(dev, struct dax_reg_ext_dev, dev)
> > +
> >   struct dax_mapping {
> >   	struct device dev;
> >   	int range_id;
> 
> 
> This is a rather large patch. Can the code below be broken out to a 
> separate patch?

Possibly.  The issue was that the natural split was to implement extents
at the CXL region level.  Then implement the dax region extents.  But
without the 2nd patch the CXL region code does not do anything.  This is
because the CXL region driver load triggers this patch to do something.
It made more sense to have the code which triggers the extent processing
bundled with the extent processing.

To split it as you suggest would still be a very large patch with this new
extent file being pretty small.  So I just combined them.

Ira