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

From: Ira Weiny <ira.weiny@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Ira Weiny <ira.weiny@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>,
	Navneet Singh <navneet.singh@intel.com>,
	Fan Ni <fan.ni@samsung.com>, Davidlohr Bueso <dave@stgolabs.net>,
	Dave Jiang <dave.jiang@intel.com>,
	Alison Schofield <alison.schofield@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	<linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC v2 09/18] cxl/mem: Read extents on memory device discovery
Date: Tue, 29 Aug 2023 17:16:59 -0700	[thread overview]
Message-ID: <64ee8a7b2c69a_18d28a294ad@iweiny-mobl.notmuch> (raw)
In-Reply-To: <20230829162600.00004ac2@Huawei.com>

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

[snip]

I'll go through each review but I had to respond to this one...

> 
> > +		.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?
> 

Then shall thou count to 3, no more, no less...
4 shall thou not count...
5 is right out...

;-)

Seriously, it seemed like a decent number to try.  I would hope that the
extents are not changing much as the host is booting or the device drivers
are loading.  But since the generation number is there I figured it was
fine to try again.

However, its been a while since I focused on this patch and as I look at
it now I realize that retrying is going to be a problem anyway.  Some of
the old extents from the previous generation may have been stored and the
new list is likely to have the same extents.  Which would result in errors
later.

I think it is best to remove the retry and just throw an error.

Thanks for catching,
Ira