Re: [PATCH RFC v2 06/18] cxl/port: Add Dynamic Capacity size support to endpoint decoders

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

Jonathan Cameron wrote:
> On Mon, 28 Aug 2023 22:20:57 -0700
> Ira Weiny <ira.weiny@intel.com> wrote:
> 

[snip]

> > 
> > Co-developed-by: Navneet Singh <navneet.singh@intel.com>
> > Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> Various minor things noticed inline.

Thanks!

[snip]

> 
> > +
> > +static int cxl_reserve_dpa_skip(struct cxl_endpoint_decoder *cxled,
> > +				resource_size_t base, resource_size_t skipped)
> > +{
> > +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +	struct cxl_port *port = cxled_to_port(cxled);
> > +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > +	resource_size_t skip_base = base - skipped;
> > +	resource_size_t size, skip_len = 0;
> > +	struct device *dev = &port->dev;
> > +	int rc, index;
> > +
> > +	size = resource_size(&cxlds->ram_res);
> > +	if (size && skip_base <= cxlds->ram_res.end) {
> 
> This size only used in this if statement I'd just put it inline.

And in the pmem case...

>  
> > +		skip_len = cxlds->ram_res.end - skip_base + 1;
> > +		rc = cxl_request_skip(cxled, skip_base, skip_len);
> > +		if (rc)
> > +			return rc;
> > +		skip_base += skip_len;
> > +	}
> > +
> > +	if (skip_base == base) {
> > +		dev_dbg(dev, "skip done!\n");
> 
> Not sure that dbg is much help as other places below where skip also done...

Ok.

> 
> > +		return 0;
> > +	}
> > +
> > +	size = resource_size(&cxlds->pmem_res);
> > +	if (size && skip_base <= cxlds->pmem_res.end) {
> 
> size only used in this if statement. I'd just put
> the resource_size() bit inline.

Ah ok.  I think the line length was the issue here.

I'm ok taking the variable out.

> 
> > +		skip_len = cxlds->pmem_res.end - skip_base + 1;
> > +		rc = cxl_request_skip(cxled, skip_base, skip_len);
> > +		if (rc)
> > +			return rc;
> > +		skip_base += skip_len;
> > +	}
> > +
> > +	index = dc_mode_to_region_index(cxled->mode);
> > +	for (int i = 0; i <= index; i++) {
> > +		struct resource *dcr = &cxlds->dc_res[i];
> > +
> > +		if (skip_base < dcr->start) {
> > +			skip_len = dcr->start - skip_base;
> > +			rc = cxl_request_skip(cxled, skip_base, skip_len);
> > +			if (rc)
> > +				return rc;
> > +			skip_base += skip_len;
> > +		}
> > +
> > +		if (skip_base == base) {
> > +			dev_dbg(dev, "skip done!\n");
> 
> As above - perhaps some more info?

Sure.

> 
> > +			break;
> > +		}
> > +
> > +		if (resource_size(dcr) && skip_base <= dcr->end) {
> > +			if (skip_base > base)
> > +				dev_err(dev, "Skip error\n");
> 
> Not return ?  If there is a reason to carry on, I'd like a comment to say what it is.

Looks like a bug I missed.  thanks!

> 
> > +
> > +			skip_len = dcr->end - skip_base + 1;
> > +			rc = cxl_request_skip(cxled, skip_base, skip_len);
> > +			if (rc)
> > +				return rc;
> > +			skip_base += skip_len;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> 
> > @@ -492,11 +607,13 @@ static resource_size_t cxl_dpa_freespace(struct cxl_endpoint_decoder *cxled,
> >  					 resource_size_t *start_out,
> >  					 resource_size_t *skip_out)
> >  {
> > +	resource_size_t free_ram_start, free_pmem_start, free_dc_start;
> >  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > -	resource_size_t free_ram_start, free_pmem_start;
> >  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > +	struct device *dev = &cxled->cxld.dev;
> 
> There is one existing (I think) call to dev_dbg(cxled_dev(cxled) ...
> in this function.  So both should use that here, and should convert that one
> case to using dev.

I think the type 2 stuff is lower priority than this series.  The main reason I
had this series based on that work was due to the split of the memdev state
from the device state.  Because that patch has landed I've rebased this
series on master in hopes of it landing in 6.7 without the type 2
dependency.

As such this code got moved to __cxl_dpa_reserve().

> 
> >  	resource_size_t start, avail, skip;
> >  	struct resource *p, *last;
> > +	int index;
> >  
> >  	lockdep_assert_held(&cxl_dpa_rwsem);
> >  
> > @@ -514,6 +631,20 @@ static resource_size_t cxl_dpa_freespace(struct cxl_endpoint_decoder *cxled,
> >  	else
> >  		free_pmem_start = cxlds->pmem_res.start;
> >  
> > +	/*
> > +	 * Limit each decoder to a single DC region to map memory with
> > +	 * different DSMAS entry.
> > +	 */
> > +	index = dc_mode_to_region_index(cxled->mode);
> > +	if (index >= 0) {
> > +		if (cxlds->dc_res[index].child) {
> > +			dev_err(dev, "Cannot allocate DPA from DC Region: %d\n",
> > +				index);
> > +			return -EINVAL;
> > +		}
> > +		free_dc_start = cxlds->dc_res[index].start;
> > +	}
> > +
> >  	if (cxled->mode == CXL_DECODER_RAM) {
> >  		start = free_ram_start;
> >  		avail = cxlds->ram_res.end - start + 1;
> > @@ -535,6 +666,29 @@ static resource_size_t cxl_dpa_freespace(struct cxl_endpoint_decoder *cxled,
> >  		else
> >  			skip_end = start - 1;
> >  		skip = skip_end - skip_start + 1;
> > +	} else if (cxl_decoder_mode_is_dc(cxled->mode)) {
> > +		resource_size_t skip_start, skip_end;
> > +
> > +		start = free_dc_start;
> > +		avail = cxlds->dc_res[index].end - start + 1;
> > +		if ((resource_size(&cxlds->pmem_res) == 0) || !cxlds->pmem_res.child)
> 
> Previous patch used !resource_size()
> I prefer compare with 0 like you have here, but which ever is chosen, things should
> be consistent.
> 
> ...
> 

Yea good point.  I audited the series for this and made the change.

Ira