Re: [PATCH RFC v2 09/18] cxl/mem: Read extents on memory device discovery

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

Jonathan Cameron wrote:
> On Mon, 28 Aug 2023 22:21:00 -0700
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > When a Dynamic Capacity Device (DCD) is realized some extents may
> > already be available within the DC Regions.  This can happen if the host
> > has accepted extents and been rebooted or any other time the host driver
> > software has become out of sync with the device hardware.
> > 
> > Read the available extents during probe and store them for later
> > use.
> > 
> > Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> > Co-developed-by: Navneet Singh <navneet.singh@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> A few minor comments inline.
> 
> Thanks,
> 
> Jonathan
> 

[snip]

> 
> > +static int cxl_dev_get_dc_extent_cnt(struct cxl_memdev_state *mds,
> > +				     unsigned int *extent_gen_num)
> > +{
> > +	struct cxl_mbox_get_dc_extent get_dc_extent;
> > +	struct cxl_mbox_dc_extents dc_extents;
> > +	struct device *dev = mds->cxlds.dev;
> > +	struct cxl_mbox_cmd mbox_cmd;
> > +	unsigned int count;
> > +	int rc;
> > +
> > +	/* Check GET_DC_EXTENT_LIST is supported by device */
> > +	if (!test_bit(CXL_DCD_ENABLED_GET_EXTENT_LIST, mds->dcd_cmds)) {
> > +		dev_dbg(dev, "unsupported cmd : get dyn cap extent list\n");
> > +		return 0;
> > +	}
> > +
> > +	get_dc_extent = (struct cxl_mbox_get_dc_extent) {
> > +		.extent_cnt = cpu_to_le32(0),
> > +		.start_extent_index = cpu_to_le32(0),
> > +	};
> > +
> > +	mbox_cmd = (struct cxl_mbox_cmd) {
> > +		.opcode = CXL_MBOX_OP_GET_DC_EXTENT_LIST,
> > +		.payload_in = &get_dc_extent,
> > +		.size_in = sizeof(get_dc_extent),
> > +		.size_out = mds->payload_size,
> 
> If all you are after is the count, then size_out can be a lot smaller than that
> I think as we know it can't return any extents.

Done.

> 
> > +		.payload_out = &dc_extents,
> > +		.min_out = 1,
> > +	};
> > +
> > +	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	count = le32_to_cpu(dc_extents.total_extent_cnt);
> > +	*extent_gen_num = le32_to_cpu(dc_extents.extent_list_num);
> > +
> > +	return count;
> > +}
> > +
> > +static int cxl_dev_get_dc_extents(struct cxl_memdev_state *mds,
> > +				  unsigned int start_gen_num,
> > +				  unsigned int exp_cnt)
> > +{
> > +	struct cxl_mbox_dc_extents *dc_extents;
> > +	unsigned int start_index, total_read;
> > +	struct device *dev = mds->cxlds.dev;
> > +	struct cxl_mbox_cmd mbox_cmd;
> > +	int retry = 3;
> 
> Why 3?

Removed.

> 
> > +	int rc;
> > +
> > +	/* Check GET_DC_EXTENT_LIST is supported by device */
> > +	if (!test_bit(CXL_DCD_ENABLED_GET_EXTENT_LIST, mds->dcd_cmds)) {
> > +		dev_dbg(dev, "unsupported cmd : get dyn cap extent list\n");
> > +		return 0;
> > +	}
> > +
> > +	dc_extents = kvmalloc(mds->payload_size, GFP_KERNEL);
> 
> Maybe __free magic would simplify this enough to be useful.

Yes.  I'd not wrapped my head around the __free magic until you mentioned in
in the other patch.  It is pretty easy to use.  But I'm worried because it
seems 'too easy'...  ;-)

I'll convert this one too.  So far the other one seems good.  So dare I
say "I know what I'm doing now"...  :-D

> 
> > +	if (!dc_extents)
> > +		return -ENOMEM;
> > +
> > +reset:
> > +	total_read = 0;
> > +	start_index = 0;
> > +	do {
> > +		unsigned int nr_ext, total_extent_cnt, gen_num;
> > +		struct cxl_mbox_get_dc_extent get_dc_extent;
> > +
> > +		get_dc_extent = (struct cxl_mbox_get_dc_extent) {
> > +			.extent_cnt = exp_cnt - start_index,
> > +			.start_extent_index = start_index,
> > +		};
> > +		
> > +		mbox_cmd = (struct cxl_mbox_cmd) {
> > +			.opcode = CXL_MBOX_OP_GET_DC_EXTENT_LIST,
> > +			.payload_in = &get_dc_extent,
> > +			.size_in = sizeof(get_dc_extent),
> > +			.size_out = mds->payload_size,
> > +			.payload_out = dc_extents,
> > +			.min_out = 1,
> > +		};
> > +		
> > +		rc = cxl_internal_send_cmd(mds, &mbox_cmd);
> > +		if (rc < 0)
> > +			goto out;
> > +		
> > +		nr_ext = le32_to_cpu(dc_extents->ret_extent_cnt);
> > +		total_read += nr_ext;
> > +		total_extent_cnt = le32_to_cpu(dc_extents->total_extent_cnt);
> > +		gen_num = le32_to_cpu(dc_extents->extent_list_num);
> > +
> > +		dev_dbg(dev, "Get extent list count:%d generation Num:%d\n",
> > +			total_extent_cnt, gen_num);
> > +
> > +		if (gen_num != start_gen_num || exp_cnt != total_extent_cnt) {
> > +			dev_err(dev, "Extent list changed while reading; %u != %u : %u != %u\n",
> > +				gen_num, start_gen_num, exp_cnt, total_extent_cnt);
> > +			if (retry--)
> > +				goto reset;
> > +			return -EIO;

And this was a bug too :-(  ...  Fixed with the __free() magic.

Thanks for the review,
Ira

> > +		}
> > +		
> > +		for (int i = 0; i < nr_ext ; i++) {
> > +			dev_dbg(dev, "Storing extent %d/%d\n",
> > +				start_index + i, exp_cnt);
> > +			rc = cxl_store_dc_extent(mds, &dc_extents->extent[i]);
> > +			if (rc)
> > +				goto out;
> > +		}
> > +
> > +		start_index += nr_ext;
> > +	} while (exp_cnt > total_read);
> > +
> > +out:
> > +	kvfree(dc_extents);
> > +	return rc;
> > +}
> 
>