Jonathan Cameron wrote:
> On Thu, 14 Jul 2022 17:02:58 -0700
> Dan Williams <dan.j.williams@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
> > 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.
