Dan Williams wrote:
[snip]
> +
> +static int cmp_decode_pos(const void *a, const void *b)
> +{
> + struct cxl_endpoint_decoder *cxled_a = *(typeof(cxled_a) *)a;
> + struct cxl_endpoint_decoder *cxled_b = *(typeof(cxled_b) *)b;
> + struct cxl_memdev *cxlmd_a = cxled_to_memdev(cxled_a);
> + struct cxl_memdev *cxlmd_b = cxled_to_memdev(cxled_b);
> + struct cxl_port *port_a = cxled_to_port(cxled_a);
> + struct cxl_port *port_b = cxled_to_port(cxled_b);
> + struct cxl_port *iter_a, *iter_b;
> +
> + /* Exit early if any prior sorting failed */
> + if (cxled_a->pos < 0 || cxled_b->pos < 0)
> + return 0;
> +
> + /*
> + * Walk up the hierarchy to find a shared port, find the decoder
> + * that maps the range, compare the relative position of those
> + * dport mappings.
> + */
> + for (iter_a = port_a; iter_a; iter_a = next_port(iter_a)) {
> + struct cxl_port *next_a, *next_b, *port;
> + struct cxl_switch_decoder *cxlsd;
> +
> + next_a = next_port(iter_a);
> + for (iter_b = port_b; iter_b; iter_b = next_port(iter_b)) {
> + int a_pos, b_pos, result;
> + struct device *dev;
> + unsigned int seq;
> +
> + next_b = next_port(iter_b);
> + if (next_a != next_b)
> + continue;
> + if (!next_a)
> + goto out;
To me this check makes more sense before the inner loop.
> + port = next_a;
> + dev = device_find_child(&port->dev, cxled_a,
> + decoder_match_range);
> + if (!dev) {
> + struct range *range = &cxled_a->cxld.hpa_range;
> +
> + dev_err(port->uport,
> + "failed to find decoder that maps %#llx-:%#llx\n",
> + range->start, range->end);
> + cxled_a->pos = -1;
> + return 0;
> + }
> +
> + cxlsd = to_cxl_switch_decoder(dev);
> + do {
> + seq = read_seqbegin(&cxlsd->target_lock);
> + find_positions(cxlsd, iter_a, iter_b, &a_pos,
> + &b_pos);
> + } while (read_seqretry(&cxlsd->target_lock, seq));
> +
> + if (a_pos < 0 || b_pos < 0) {
> + dev_err(port->uport,
> + "failed to find shared decoder for %s and %s\n",
> + dev_name(cxlmd_a->dev.parent),
> + dev_name(cxlmd_b->dev.parent));
> + cxled_a->pos = -1;
> + result = 0;
> + } else {
> + result = a_pos - b_pos;
> + dev_dbg(port->uport, "%s: %s comes %s %s\n",
> + dev_name(&cxlsd->cxld.dev),
> + dev_name(cxlmd_a->dev.parent),
> + result < 0 ? "before" : "after",
> + dev_name(cxlmd_b->dev.parent));
> + }
> +
> + put_device(dev);
> +
> + return result;
> + }
> + }
> +out:
> + dev_err(cxlmd_a->dev.parent, "failed to find shared port with %s\n",
> + dev_name(cxlmd_b->dev.parent));
> + cxled_a->pos = -1;
> + return 0;
> +}
> +
[snip]
> @@ -1500,8 +1766,8 @@ static int detach_target(struct cxl_region *cxlr, int pos)
> return rc;
> }
>
> -static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos,
> - size_t len)
> +static ssize_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos,
> + size_t len)
Is this a separate fix?
> {
> int rc;
>
[snip]
> +
> +/* Establish an empty region covering the given HPA range */
> +static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> + struct cxl_endpoint_decoder *cxled)
Rather than a comment would this be better named construct_empty_region()?
Ira
