Re: [PATCH v15 2/8] cxl/mem: Read dynamic capacity configuration from the device

[Jonathan Cameron <jic23@kernel.org>]

Please remember to crop down to only the context you need!
(grumpy as on a train and the pagedown key on my laptop requires pressing far too
many things at once!)

> > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > > index b3419df586b9..def26c490dfe 100644
> > > --- a/drivers/cxl/core/memdev.c
> > > +++ b/drivers/cxl/core/memdev.c
> > > @@ -643,6 +643,53 @@ int cxl_mem_dpa_fetch(struct cxl_memdev_state *mds, struct cxl_dpa_info *info)
> > >  }
> > >  EXPORT_SYMBOL_NS_GPL(cxl_mem_dpa_fetch, "CXL");
> > >  
> > > +int cxl_configure_dcd(struct cxl_memdev_state *mds, struct cxl_dpa_info *info)
> > > +{
> > > +	struct cxl_dc_partition_info dc_info = { };
> > > +	struct device *dev = mds->cxlds.dev;
> > > +	u64 total;
> > > +	int rc;
> > > +
> > > +	if (!mds->cxlds.media_ready)
> > > +		return 0;
> > > +
> > > +	rc = cxl_dev_dc_identify(&mds->cxlds.cxl_mbox, &dc_info);
> > > +	if (rc) {
> > > +		dev_warn(dev,
> > > +			 "Failed to read Dynamic Capacity config: %d\n", rc);
> > > +		return rc;
> > > +	}
> > > +
> > > +	if (dc_info.start < info->size) {
> > > +		dev_err(dev,
> > > +			"DC partition 0 base %#llx overlaps static capacity ending at %#llx\n",
> > > +			dc_info.start, info->size);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* A gap between static capacity and the DC partition is not supported */
> > > +	if (dc_info.start > info->size) {
> > > +		dev_warn(dev,
> > > +			 "DC partition 0 base %#llx leaves a gap from static capacity ending at %#llx\n",
> > > +			 dc_info.start, info->size);
> > > +		return -EOPNOTSUPP;
> > > +	}
> > > +
> > > +	if (check_add_overflow(info->size, dc_info.size, &total)) {
> > > +		dev_err(dev,
> > > +			"DC partition 0 size %#llx overflows DPA space ending at %#llx\n",
> > > +			dc_info.size, info->size);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	info->size = total;
> > > +	dev_dbg(dev, "Adding dynamic ram partition; %#llx size %#llx\n",
> > > +		dc_info.start, dc_info.size);
> > > +	add_part(info, dc_info.start, dc_info.size, CXL_PARTMODE_DYNAMIC_RAM);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(cxl_configure_dcd, "CXL");
> > >   
> > 
> > Hi Anisa,
> > 
> > This function uses info->size, which is Total Capacity, as the end of static
> > capacity, but now we require DC partition to start at the end of the last
> > static partition, which is RAM + PMEM. CXL r4.0 8.2.10.9.1.1 "Identify Memory
> > Device" only requires Total Capacity to be >= the sum of RAM + PMEM.
> > If they differ and device place DCD region base at Total, we can pass the check
> > here but cxl_dpa_setup() fails probe with -EINVAL, so a DCD config
> > quirk costs the whole memdev instead of just DCD.
> > 
> > Think of an example, Total capacity 12G, RAM has 8G, PMEM has 2G, now if
> > DC starts at 12G, this function check will be passed, but cxl_dpa_setup()
> > will see PMEM end at 10G, DC start at 12G, think of it as a hole.
> > If the DC start at 10G, we can pass cxl_dpa_setup(), but the test here will see
> > 10G < 12 G and think DC region start inside the device memory and print
> > "overlaps"
> > 
> > Maybe derive the static end from info->part[nr_partitions - 1] and size
> > info->size from DC partition end ?
> >   
> Oh you're right.... fixed.

For this sort of thing where you are fully agreeing. 
Just fix it and acknowledge in change log!  That way there is no email
for others to get grumpy scrolling through :)

Jonathan