On Fri, 2022-11-04 at 11:59 -0700, Dan Williams wrote: > Verma, Vishal L wrote: > > On Thu, 2022-11-03 at 17:30 -0700, Dan Williams wrote: > > > When programming port decode targets, the algorithm wants to ensure that > > > two devices are compatible to be programmed as peers beneath a given > > > port. A compatible peer is a target that shares the same dport, and > > > where that target's interleave position also routes it to the same > > > dport. Compatibility is determined by the device's interleave position > > > being >= to distance. For example, if a given dport can only map every > > > Nth position then positions less than N away from the last target > > > programmed are incompatible. > > > > > > The @distance for the host-bridge-cxl_port a simple dual-ported > > ^ > > Is this meant to be "the distance for the host-bridge to a cxl_port"? > > No, but I will preface this explanation by admitting "distance" may not > be the best term for what this value is describing. CXL decode routes to > targets in a round robin fashion per-port. Take the diagram below: > > ┌───────────────────────────────────┬──┐ > │WINDOW0 │x2│ > └─────────┬─────────────────┬───────┴──┘ > │ │ > ┌─────────▼────┬──┐ ┌──────▼───────┬──┐ > │HB0 │x2│ │HB1 │x2│ > └──┬────────┬──┴──┘ └─┬─────────┬──┴──┘ > │ │ │ │ > ┌──▼─┬──┐ ┌─▼──┬──┐ ┌─▼──┬──┐ ┌─▼──┬──┐ > │DEV0│x4│ │DEV1│x4│ │DEV2│x4│ │DEV3│x4│ > └────┴──┘ └────┴──┘ └────┴──┘ └────┴──┘ > 0 2 1 3 > > ...where an x4 region is being established, and all the xN values are > the interleave-ways settings for those ports/devices. The @distance > value for that "HB0" port is 2. I.e. in order for 2 devices in that > region to be mapped under HB0 they must be at position X and X+2 in the > region. The algorithm needs to be flexible to also allow this > configuration: > > ┌───────────────────────────────────┬──┐ > │WINDOW0 │x2│ > └─────────┬─────────────────┬───────┴──┘ > │ │ > ┌─────────▼────┬──┐ ┌──────▼───────┬──┐ > │HB0 │x2│ │HB1 │x2│ > └──┬────────┬──┴──┘ └─┬─────────┬──┴──┘ > │ │ │ │ > ┌──▼─┬──┐ ┌─▼──┬──┐ ┌─▼──┬──┐ ┌─▼──┬──┐ > │DEV3│x4│ │DEV2│x4│ │DEV1│x4│ │DEV0│x4│ > └────┴──┘ └────┴──┘ └────┴──┘ └────┴──┘ > 3 1 2 0 > > ...and the algorithm can not know that a device is in the wrong position > until trying to map the peer (like DEV0 and DEV1 are peers) into the > decode. So "The @distance for the host-bridge-cxl_port" is referring to > the value "2" for host-bridge-cxl_port:HB0 and host-bridge-cxl_port:HB1 > in the diagram. Ah I understand now. The host-bridge-cxl_port term confused me slightly - I wonder if it is better to say "the host-bridge's cxl_port" or something? Regardless, thanks for the explanation! > > > Also missing '/in/ a simple dual-ported..'? > > Yes to this fixup though. > > > > > > host-bridge configuration with 2 direct-attached devices is 1. An x2 > > > region divded by 2 dports to reach 2 region targets. > > > > The second sentence seems slightly incomprehensible too. > > Oh, I think I meant that to be s/. An/, i.e. an/: > > "...host-bridge configuration with 2 direct-attached devices is 1, i.e. > an x2 region divded by 2 dports to reach 2 region positions" Ah ok that makes more sense! > > > > > > > An x4 region under an x2 host-bridge would need 2 intervening switches > > > where the @distance at the host bridge level is 2 (x4 region divided by > > > 2 switches to reach 4 devices). > > > > > > However, the distance between peers underneath a single ported > > > host-bridge is always zero because there is no limit to the number of > > > devices that can be mapped. In other words, there are no decoders to > > > program in a passthrough, all descendants are mapped and distance only > > > starts matters for the intervening descendant ports of the passthrough > > > > starts to matter? > > s/starts matters/matters/ > > > > > > port. > > > > > > Add tracking for the number of dports mapped to a port, and use that to > > > detect the passthrough case for calculating @distance. > > > > > > Cc: <stable@xxxxxxxxxxxxxxx> > > > Reported-by: Bobo WL <lmw.bobo@xxxxxxxxx> > > > Reported-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > > Link: http://lore.kernel.org/r/20221010172057.00001559@xxxxxxxxxx > > > Fixes: 27b3f8d13830 ("cxl/region: Program target lists") > > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> > > > --- > > > drivers/cxl/core/port.c | 11 +++++++++-- > > > drivers/cxl/core/region.c | 9 ++++++++- > > > drivers/cxl/cxl.h | 2 ++ > > > 3 files changed, 19 insertions(+), 3 deletions(-) > > > > Other than the above, looks good, > > > > Reviewed-by: Vishal Verma <vishal.l.verma@xxxxxxxxx> > > > > > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > > index bffde862de0b..e7556864ea80 100644 > > > --- a/drivers/cxl/core/port.c > > > +++ b/drivers/cxl/core/port.c > > > @@ -811,6 +811,7 @@ static struct cxl_dport *find_dport(struct cxl_port *port, int id) > > > static int add_dport(struct cxl_port *port, struct cxl_dport *new) > > > { > > > struct cxl_dport *dup; > > > + int rc; > > > > > > device_lock_assert(&port->dev); > > > dup = find_dport(port, new->port_id); > > > @@ -821,8 +822,14 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *new) > > > dev_name(dup->dport)); > > > return -EBUSY; > > > } > > > - return xa_insert(&port->dports, (unsigned long)new->dport, new, > > > - GFP_KERNEL); > > > + > > > + rc = xa_insert(&port->dports, (unsigned long)new->dport, new, > > > + GFP_KERNEL); > > > + if (rc) > > > + return rc; > > > + > > > + port->nr_dports++; > > > + return 0; > > > } > > > > > > /* > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > > index c52465e09f26..c0253de74945 100644 > > > --- a/drivers/cxl/core/region.c > > > +++ b/drivers/cxl/core/region.c > > > @@ -990,7 +990,14 @@ static int cxl_port_setup_targets(struct cxl_port *port, > > > if (cxl_rr->nr_targets_set) { > > > int i, distance; > > > > > > - distance = p->nr_targets / cxl_rr->nr_targets; > > > + /* > > > + * Passthrough ports impose no distance requirements between > > > + * peers > > > + */ > > > + if (port->nr_dports == 1) > > > + distance = 0; > > > + else > > > + distance = p->nr_targets / cxl_rr->nr_targets; > > > for (i = 0; i < cxl_rr->nr_targets_set; i++) > > > if (ep->dport == cxlsd->target[i]) { > > > rc = check_last_peer(cxled, ep, cxl_rr, > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > > index 1164ad49f3d3..ac75554b5d76 100644 > > > --- a/drivers/cxl/cxl.h > > > +++ b/drivers/cxl/cxl.h > > > @@ -457,6 +457,7 @@ struct cxl_pmem_region { > > > * @regions: cxl_region_ref instances, regions mapped by this port > > > * @parent_dport: dport that points to this port in the parent > > > * @decoder_ida: allocator for decoder ids > > > + * @nr_dports: number of entries in @dports > > > * @hdm_end: track last allocated HDM decoder instance for allocation ordering > > > * @commit_end: cursor to track highest committed decoder for commit ordering > > > * @component_reg_phys: component register capability base address (optional) > > > @@ -475,6 +476,7 @@ struct cxl_port { > > > struct xarray regions; > > > struct cxl_dport *parent_dport; > > > struct ida decoder_ida; > > > + int nr_dports; > > > int hdm_end; > > > int commit_end; > > > resource_size_t component_reg_phys; > > > > > > > >
