Re: [PATCH v2 13/20] cxl/region: Add region autodiscovery

[Dan Williams <dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Fri, 10 Feb 2023 01:06:39 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Region autodiscovery is an asynchronous state machine advanced by
> > cxl_port_probe(). After the decoders on an endpoint port are enumerated
> > they are scanned for actively enabled instances. Each active decoder is
> > flagged for auto-assembly CXL_DECODER_F_AUTO and attached to a region.
> > If a region does not already exist for the address range setting of the
> > decoder one is created. That creation process may race with other
> > decoders of the same region being discovered since cxl_port_probe() is
> > asynchronous. A new 'struct cxl_root_decoder' lock, @range_lock, is
> > introduced to mitigate that race.
> > 
> > Once all decoders have arrived, "p->nr_targets == p->interleave_ways",
> > they are sorted by their relative decode position. The sort algorithm
> > involves finding the point in the cxl_port topology where one leg of the
> > decode leads to deviceA and the other deviceB. At that point in the
> > topology the target order in the 'struct cxl_switch_decoder' indicates
> > the relative position of those endpoint decoders in the region.
> > 
> > >From that point the region goes through the same setup and validation 
> Why the >? 

I believe this is auto-added by git send-email or public-inbox to make
sure that a sentence that begins with "From" is not misinterpreted as a
"From:" header. You can see this throughout the kernel commit history.
In this case I pulled the patches back down from lore before editing
them to collect review tags.

> > steps as user-created regions, but instead of programming the decoders
> > it validates that driver would have written the same values to the
> > decoders as were already present.
> > 
> > Tested-by: Fan Ni <fan.ni@samsung.com>
> > Link: https://lore.kernel.org/r/167564540972.847146.17096178433176097831.stgit@dwillia2-xfh.jf.intel.com
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> A few trivial things inline and this being complex code I'm not
> as confident about it as the rest of the series but with that in mind
> and the fact I didn't find anything that looked broken...
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ...
> 
> 
> 
> > +
> > +static int cxl_region_sort_targets(struct cxl_region *cxlr)
> > +{
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	int i, rc = 0;
> > +
> > +	sort(p->targets, p->nr_targets, sizeof(p->targets[0]), cmp_decode_pos,
> > +	     NULL);
> > +
> > +	for (i = 0; i < p->nr_targets; i++) {
> > +		struct cxl_endpoint_decoder *cxled = p->targets[i];
> > +
> > +		if (cxled->pos < 0)
> > +			rc = -ENXIO;
> 
> If it makes sense to carry on after pos < 0 I'd like to see a comment here
> on why.  If not, nicer to have a separate dev_dbg() for failed case nad
> direct return here.

Ok, I'll add:

/*
 * Record that sorting failed, but still continue to restore cxled->pos
 * with its ->targets[] position so that follow-on code paths can reliably
 * do p->targets[cxled->pos] to self-reference their entry.
 */

> 
> > +		cxled->pos = i;
> > +	}
> > +
> > +	dev_dbg(&cxlr->dev, "region sort %s\n", rc ? "failed" : "successful");
> > +	return rc;
> > +}
> > +
> 
> > +
> > +int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
> > +{
> > +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +	struct range *hpa = &cxled->cxld.hpa_range;
> > +	struct cxl_decoder *cxld = &cxled->cxld;
> > +	struct cxl_root_decoder *cxlrd;
> > +	struct cxl_region_params *p;
> > +	struct cxl_region *cxlr;
> > +	bool attach = false;
> > +	struct device *dev;
> > +	int rc;
> > +
> > +	dev = device_find_child(&root->dev, &cxld->hpa_range,
> > +				match_decoder_by_range);
> > +	if (!dev) {
> > +		dev_err(cxlmd->dev.parent,
> > +			"%s:%s no CXL window for range %#llx:%#llx\n",
> > +			dev_name(&cxlmd->dev), dev_name(&cxld->dev),
> > +			cxld->hpa_range.start, cxld->hpa_range.end);
> > +		return -ENXIO;
> > +	}
> > +
> > +	cxlrd = to_cxl_root_decoder(dev);
> > +
> > +	/*
> > +	 * Ensure that if multiple threads race to construct_region() for @hpa
> > +	 * one does the construction and the others add to that.
> > +	 */
> > +	mutex_lock(&cxlrd->range_lock);
> > +	dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
> > +				match_region_by_range);
> > +	if (!dev)
> > +		cxlr = construct_region(cxlrd, cxled);
> > +	else
> > +		cxlr = to_cxl_region(dev);
> > +	mutex_unlock(&cxlrd->range_lock);
> > +
> > +	if (IS_ERR(cxlr)) {
> > +		rc = PTR_ERR(cxlr);
> > +		goto out;
> > +	}
> > +
> > +	attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
> > +
> > +	down_read(&cxl_region_rwsem);
> > +	p = &cxlr->params;
> > +	attach = p->state == CXL_CONFIG_COMMIT;
> > +	up_read(&cxl_region_rwsem);
> > +
> > +	if (attach) {
> > +		int rc = device_attach(&cxlr->dev);
> 
> Shadowing int rc isn't great for readability. Just call it rc2 or something :)
> Or given you don't make use of the value...

0day did not like this either...

> 
> 		/*
> 		 * If device_attach() fails the range may still be active via
> 		 * the platform-firmware memory map, otherwise the driver for
> 		 * regions is local to this file, so driver matching can't fail
> +                * and hence device_attach() cannot return 1.
> 
> //very much not obvious otherwise to anyone who isn't far too familiar with device_attach()

Hence the comment? Not sure what else can be said here about why
device_attach() < 0 is a sufficient check.

> 
> 		 */
> 		if (device_attach(&cxlr->dev) < 0)
> 			dev_err()
> > +
> > +		/*
> > +		 * If device_attach() fails the range may still be active via
> > +		 * the platform-firmware memory map, otherwise the driver for
> > +		 * regions is local to this file, so driver matching can't fail.
> > +		 */
> > +		if (rc < 0)
> > +			dev_err(&cxlr->dev, "failed to enable, range: %pr\n",
> > +				p->res);
> > +	}
> > +
> > +	put_device(&cxlr->dev);
> > +out:
> > +	put_device(&cxlrd->cxlsd.cxld.dev);
> 
> Moderately horrible.  Maybe just keep an extra local variable around for the first
> use of struct device *dev?  or maybe add a put_cxl_root_decoder() helper?
> 
> There are lots of other deep structure access like this I guess, so I don't mind
> if you just leave this as yet another one.

Yeah, it's difficult to have symmetry here, but I think I'll switch to
using an @cxlrd_dev variable so better match the get with the put.

> 
> 
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, CXL);
> 
> ...
> 
> > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> > index a8d46a67b45e..d88518836c2d 100644
> > --- a/drivers/cxl/port.c
> > +++ b/drivers/cxl/port.c
> > @@ -30,6 +30,34 @@ static void schedule_detach(void *cxlmd)
> >  	schedule_cxl_memdev_detach(cxlmd);
> >  }
> >  
> > +static int discover_region(struct device *dev, void *root)
> > +{
> > +	struct cxl_endpoint_decoder *cxled;
> > +	int rc;
> > +
> > +	if (!is_endpoint_decoder(dev))
> > +		return 0;
> > +
> > +	cxled = to_cxl_endpoint_decoder(dev);
> > +	if ((cxled->cxld.flags & CXL_DECODER_F_ENABLE) == 0)
> > +		return 0;
> > +
> > +	if (cxled->state != CXL_DECODER_STATE_AUTO)
> > +		return 0;
> > +
> > +	/*
> > +	 * Region enumeration is opportunistic, if this add-event fails,
> > +	 * continue to the next endpoint decoder.
> > +	 */
> > +	rc = cxl_add_to_region(root, cxled);
> > +	if (rc)
> > +		dev_dbg(dev, "failed to add to region: %#llx-%#llx\n",
> > +			cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end);
> > +
> > +	return 0;
> > +}
> > +
> > +
> 
> Two blank lines?

Just stashing this here so I can introduce a spurious whitespace removal
in the next patch. Will clean up.