Re: [PATCH v2 24/28] cxl/region: Program target lists

From: Dan Williams <dan.j.williams@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Dan Williams <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>, <hch@lst.de>,
	<nvdimm@lists.linux.dev>, <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v2 24/28] cxl/region: Program target lists
Date: Thu, 21 Jul 2022 09:56:36 -0700	[thread overview]
Message-ID: <62d985442819d_17f3e8294d8@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <20220720184151.00005b85@Huawei.com>

Jonathan Cameron wrote:
> On Thu, 14 Jul 2022 17:02:58 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Once the region's interleave geometry (ways, granularity, size) is
> > established and all the endpoint decoder targets are assigned, the next
> > phase is to program all the intermediate decoders. Specifically, each
> > CXL switch in the path between the endpoint and its CXL host-bridge
> > (including the logical switch internal to the host-bridge) needs to have
> > its decoders programmed and the target list order assigned.
> > 
> > The difficulty in this implementation lies in determining which endpoint
> > decoder ordering combinations are valid. Consider the cxl_test case of 2
> > host bridges, each of those host-bridges attached to 2 switches, and
> > each of those switches attached to 2 endpoints for a potential 8-way
> > interleave. The x2 interleave at the host-bridge level requires that all
> > even numbered endpoint decoder positions be located on the "left" hand
> > side of the topology tree, and the odd numbered positions on the other.
> > The endpoints that are peers on the same switch need to have a position
> > that can be routed with a dedicated address bit per-endpoint. See
> > check_last_peer() for the details.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> I'm less confident on this one than most the other patches (and I see I skipped
> reviewing it in v1) as I haven't closely checked the verification logic
> but except for one trivial comment inline it looks fine to me.
> I want to hit the whole series with a wide range of test cases (I'm sure you
> already have) to build that confidence, but won't have time to do that till early
> August. However, if there are gremlins hiding, I'd expect them to be minor.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 8ac0c557f6aa..225340529fc3 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -485,6 +485,7 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
> >  		return NULL;
> >  	cxl_rr->port = port;
> >  	cxl_rr->region = cxlr;
> > +	cxl_rr->nr_targets = 1;
> >  	xa_init(&cxl_rr->endpoints);
> >  
> >  	rc = xa_insert(&port->regions, (unsigned long)cxlr, cxl_rr, GFP_KERNEL);
> > @@ -525,10 +526,12 @@ static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr,
> >  	struct cxl_decoder *cxld = cxl_rr->decoder;
> >  	struct cxl_ep *ep = cxl_ep_load(port, cxled_to_memdev(cxled));
> >  
> > -	rc = xa_insert(&cxl_rr->endpoints, (unsigned long)cxled, ep,
> > -			 GFP_KERNEL);
> > -	if (rc)
> > -		return rc;
> > +	if (ep) {
> > +		rc = xa_insert(&cxl_rr->endpoints, (unsigned long)cxled, ep,
> > +			       GFP_KERNEL);
> > +		if (rc)
> > +			return rc;
> > +	}
> >  	cxl_rr->nr_eps++;
> >  
> >  	if (!cxld->region) {
> > @@ -565,7 +568,7 @@ static int cxl_port_attach_region(struct cxl_port *port,
> >  				  struct cxl_endpoint_decoder *cxled, int pos)
> >  {
> >  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > -	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> > +	const struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> 
> Why const now and not previously?

Good question.

> Feels like this should be in an earlier patch.  Maybe I'm missing
> something though.

...or just dropped for now, I can not recall a justification at this
point.