| Message ID | 5a7133e0ab7eb11f96daa73d88ec765f3536634a.1696550786.git.alison.schofield@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/region: Autodiscovery position repair | expand |
On 10/5/23 17:43, alison.schofield@intel.com wrote: > From: Alison Schofield <alison.schofield@intel.com> > > Introduce a calculation that determines a targets position in a region > interleave. Perform a selftest of the calculation on user-defined > regions. > > The region driver users the kernel sort() function to put region s/users/uses/ > targets in relative order. Positions are assigned based on each > targets index in that sorted list. That relative sort doesn't s/targets/target/ > consider the offset of a port into its parent port causing some s/causing/which causes/ > autodiscovered regions to fail creation. In one failure case, s/autodiscovered/auto-discovered/ > a 2 + 2 config (2 host bridges each with 2 endpoints), the sort > put all targets of one port ahead of another port, when they were s/put/puts/ s/targets/the targets/ comma not needed > expected to be interleaved. > > In preparation for repairing the autodiscovery region assembly, s/autodiscovery/auto-discovery/ > introduce a new method for discovering a target position in the > region interleave. > > cxl_interleave_pos() offers a method to determine a targets position > by ascending from an endpoint to a root decoder. The calculation starts > with the endpoints local position and its position in its parents port. > Traversing towards the root decoder and examining position and ways, > allows the position to be refined all the way to the root decoder. Please consider: It transverses towards the root decoder and examines both position and ways in order to allow the position to be refined all the way to the root decoder. > > This calculation, applied iteratively, yields the correct position: > > position = position * parent_ways + parent_pos; > > ...when you follow these rules: > > Rule #1 - When (parent_ways == region_ways), abort. > position = parent_position; > This rule is applied in calc_interleave_pos() > > Rule #2 - Use an index into the target list when finding pos. > This rule is applied in the helper find_pos_and_ways(). > > Include a selftest that exercises this new position calculation against > every successfully configured user-defined region. > > Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery") > Reported-by: Dmytro Adamenko <dmytro.adamenko@intel.com> > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > drivers/cxl/core/region.c | 100 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 100 insertions(+) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 64206fc4d99b..297b9132d5b3 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1500,6 +1500,91 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) > return range_contains(r1, r2); > } > > +/* Find the position of a port in it's parent and the parents ways */ > +static int find_pos_and_ways(struct cxl_port *port, struct range *range, > + int *pos, int *ways) > +{ > + struct cxl_switch_decoder *cxlsd; > + struct cxl_port *parent; > + int child_ways = *ways; > + int child_pos = *pos; > + struct device *dev; > + int index = 0; > + int rc = -1; > + > + parent = next_port(port); > + if (!parent) > + return rc; > + > + dev = device_find_child(&parent->dev, range, > + match_switch_decoder_by_range); > + if (!dev) { > + dev_err(port->uport_dev, > + "failed to find decoder mapping %#llx-%#llx\n", > + range->start, range->end); > + return rc; > + } > + cxlsd = to_cxl_switch_decoder(dev); > + *ways = cxlsd->cxld.interleave_ways; > + > + /* Use the child ways/pos as index to target list */ > + if (cxlsd->nr_targets > child_ways) > + index = child_pos * child_ways; > + > + for (int i = index; i < *ways; i++) { > + if (cxlsd->target[i] == port->parent_dport) { > + *pos = i; > + rc = 0; > + break; > + } > + } > + put_device(dev); > + > + return rc; > +} > + > +static int calc_interleave_pos(struct cxl_endpoint_decoder *cxled, > + int region_ways) > +{ > + struct cxl_port *iter, *port = cxled_to_port(cxled); > + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > + struct range *range = &cxled->cxld.hpa_range; > + int parent_ways = 0; > + int parent_pos = 0; > + int rc, pos; > + > + /* Initialize pos to its local position */ > + rc = find_pos_and_ways(port, range, &parent_pos, &parent_ways); > + if (rc) > + return -ENXIO; > + > + pos = parent_pos; > + > + if (parent_ways == region_ways) > + goto out; > + > + /* Iterate up the ancestral tree refining the position */ > + for (iter = next_port(port); iter; iter = next_port(iter)) { > + if (is_cxl_root(iter)) > + break; > + > + rc = find_pos_and_ways(iter, range, &parent_pos, &parent_ways); > + if (rc) > + return -ENXIO; > + > + if (parent_ways == region_ways) { > + pos = parent_pos; > + break; > + } > + pos = pos * parent_ways + parent_pos; > + } > +out: > + dev_dbg(&cxlmd->dev, > + "decoder:%s parent:%s port:%s range:%#llx-%#llx pos:%d\n", > + dev_name(&cxled->cxld.dev), dev_name(cxlmd->dev.parent), > + dev_name(&port->dev), range->start, range->end, pos); > + > + return pos; > } > > static void find_positions(const struct cxl_switch_decoder *cxlsd, > @@ -1765,6 +1850,21 @@ static int cxl_region_attach(struct cxl_region *cxlr, > .end = p->res->end, > }; > > + if (p->nr_targets != p->interleave_ways) > + return 0; > + > + /* Exercise position calculator on user-defined regions */ > + for (int i = 0; i < p->nr_targets; i++) { > + struct cxl_endpoint_decoder *cxled = p->targets[i]; > + int test_pos; > + > + test_pos = calc_interleave_pos(cxled, p->interleave_ways); > + dev_dbg(&cxled->cxld.dev, > + "Interleave calc match %s test_pos:%d cxled->pos:%d\n", > + (test_pos == cxled->pos) ? "Success" : "Fail", > + test_pos, cxled->pos); Mismatch from selftest does not cause error? > + } > + > return 0; > > err_decrement:
On Thu, Oct 12, 2023 at 04:49:15PM -0700, Dave Jiang wrote: > Thanks for the review Dave - I took all the commit msg updates w one exception noted below. > > On 10/5/23 17:43, alison.schofield@intel.com wrote: > > From: Alison Schofield <alison.schofield@intel.com> > > > > Introduce a calculation that determines a targets position in a region > > interleave. Perform a selftest of the calculation on user-defined > > regions. > > > > The region driver users the kernel sort() function to put region > > s/users/uses/ > > > targets in relative order. Positions are assigned based on each > > targets index in that sorted list. That relative sort doesn't > > s/targets/target/ "targets" was wrong because it is the plural noun, but I actually want the possessive noun "target's", not the singular "target". > > > consider the offset of a port into its parent port causing some > > s/causing/which causes/ > > > autodiscovered regions to fail creation. In one failure case, > > s/autodiscovered/auto-discovered/ > > > a 2 + 2 config (2 host bridges each with 2 endpoints), the sort > > put all targets of one port ahead of another port, when they were > > s/put/puts/ > s/targets/the targets/ > comma not needed > > > expected to be interleaved. > > > > In preparation for repairing the autodiscovery region assembly, > > s/autodiscovery/auto-discovery/ > > > introduce a new method for discovering a target position in the > > region interleave. > > > > cxl_interleave_pos() offers a method to determine a targets position > > by ascending from an endpoint to a root decoder. The calculation starts > > with the endpoints local position and its position in its parents port. > > Traversing towards the root decoder and examining position and ways, > > allows the position to be refined all the way to the root decoder. > > Please consider: > It transverses towards the root decoder and examines both position and ways in order to allow the position to be refined all the way to the root decoder. > > > > > This calculation, applied iteratively, yields the correct position: ..skip.. > > + > > + /* Exercise position calculator on user-defined regions */ > > + for (int i = 0; i < p->nr_targets; i++) { > > + struct cxl_endpoint_decoder *cxled = p->targets[i]; > > + int test_pos; > > + > > + test_pos = calc_interleave_pos(cxled, p->interleave_ways); > > + dev_dbg(&cxled->cxld.dev, > > + "Interleave calc match %s test_pos:%d cxled->pos:%d\n", > > + (test_pos == cxled->pos) ? "Success" : "Fail", > > + test_pos, cxled->pos); > > Mismatch from selftest does not cause error? So I'm doing something weird here and am looking for input on how and where this check might be inserted, if at all. Here I have inserted the check at the end of the successful creation of a user-defined region. This tells us if the driver would have been able to create the same region in the auto-discovery path. So for the region created at this moment, everything is fine. This is how I've been testing the various interleave flavors of auto-discovered regions for this patchset. I create the regions as a user and check that the new calculation finds the interleave positions. I am imagining a less than finite set of interleave configs and thinking this may give early warning to configs where the interleave calc fails. I'm going to post a v2 without any change here and we can continue this discussion in v2. Alison > > > + } > > + > > return 0; > > > > err_decrement:
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 64206fc4d99b..297b9132d5b3 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1500,6 +1500,91 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) return range_contains(r1, r2); } +/* Find the position of a port in it's parent and the parents ways */ +static int find_pos_and_ways(struct cxl_port *port, struct range *range, + int *pos, int *ways) +{ + struct cxl_switch_decoder *cxlsd; + struct cxl_port *parent; + int child_ways = *ways; + int child_pos = *pos; + struct device *dev; + int index = 0; + int rc = -1; + + parent = next_port(port); + if (!parent) + return rc; + + dev = device_find_child(&parent->dev, range, + match_switch_decoder_by_range); + if (!dev) { + dev_err(port->uport_dev, + "failed to find decoder mapping %#llx-%#llx\n", + range->start, range->end); + return rc; + } + cxlsd = to_cxl_switch_decoder(dev); + *ways = cxlsd->cxld.interleave_ways; + + /* Use the child ways/pos as index to target list */ + if (cxlsd->nr_targets > child_ways) + index = child_pos * child_ways; + + for (int i = index; i < *ways; i++) { + if (cxlsd->target[i] == port->parent_dport) { + *pos = i; + rc = 0; + break; + } + } + put_device(dev); + + return rc; +} + +static int calc_interleave_pos(struct cxl_endpoint_decoder *cxled, + int region_ways) +{ + struct cxl_port *iter, *port = cxled_to_port(cxled); + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); + struct range *range = &cxled->cxld.hpa_range; + int parent_ways = 0; + int parent_pos = 0; + int rc, pos; + + /* Initialize pos to its local position */ + rc = find_pos_and_ways(port, range, &parent_pos, &parent_ways); + if (rc) + return -ENXIO; + + pos = parent_pos; + + if (parent_ways == region_ways) + goto out; + + /* Iterate up the ancestral tree refining the position */ + for (iter = next_port(port); iter; iter = next_port(iter)) { + if (is_cxl_root(iter)) + break; + + rc = find_pos_and_ways(iter, range, &parent_pos, &parent_ways); + if (rc) + return -ENXIO; + + if (parent_ways == region_ways) { + pos = parent_pos; + break; + } + pos = pos * parent_ways + parent_pos; + } +out: + dev_dbg(&cxlmd->dev, + "decoder:%s parent:%s port:%s range:%#llx-%#llx pos:%d\n", + dev_name(&cxled->cxld.dev), dev_name(cxlmd->dev.parent), + dev_name(&port->dev), range->start, range->end, pos); + + return pos; } static void find_positions(const struct cxl_switch_decoder *cxlsd, @@ -1765,6 +1850,21 @@ static int cxl_region_attach(struct cxl_region *cxlr, .end = p->res->end, }; + if (p->nr_targets != p->interleave_ways) + return 0; + + /* Exercise position calculator on user-defined regions */ + for (int i = 0; i < p->nr_targets; i++) { + struct cxl_endpoint_decoder *cxled = p->targets[i]; + int test_pos; + + test_pos = calc_interleave_pos(cxled, p->interleave_ways); + dev_dbg(&cxled->cxld.dev, + "Interleave calc match %s test_pos:%d cxled->pos:%d\n", + (test_pos == cxled->pos) ? "Success" : "Fail", + test_pos, cxled->pos); + } + return 0; err_decrement: