Re: [PATCH v2 13/20] cxl/region: Add region autodiscovery

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Fri, 2023-02-10 at 01:06 -0800, Dan Williams wrote:
> Region autodiscovery is an asynchronous state machine advanced by
> cxl_port_probe(). After the decoders on an endpoint port are enumerated
> they are scanned for actively enabled instances. Each active decoder is
> flagged for auto-assembly CXL_DECODER_F_AUTO and attached to a region.
> If a region does not already exist for the address range setting of the
> decoder one is created. That creation process may race with other
> decoders of the same region being discovered since cxl_port_probe() is
> asynchronous. A new 'struct cxl_root_decoder' lock, @range_lock, is
> introduced to mitigate that race.
> 
> Once all decoders have arrived, "p->nr_targets == p->interleave_ways",
> they are sorted by their relative decode position. The sort algorithm
> involves finding the point in the cxl_port topology where one leg of the
> decode leads to deviceA and the other deviceB. At that point in the
> topology the target order in the 'struct cxl_switch_decoder' indicates
> the relative position of those endpoint decoders in the region.
> 
> > From that point the region goes through the same setup and validation
> steps as user-created regions, but instead of programming the decoders
> it validates that driver would have written the same values to the
> decoders as were already present.
> 
> Tested-by: Fan Ni <fan.ni@samsung.com>
> Link: https://lore.kernel.org/r/167564540972.847146.17096178433176097831.stgit@dwillia2-xfh.jf.intel.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/hdm.c    |   11 +
>  drivers/cxl/core/port.c   |    2 
>  drivers/cxl/core/region.c |  497 ++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/cxl/cxl.h         |   29 +++
>  drivers/cxl/port.c        |   48 ++++
>  5 files changed, 576 insertions(+), 11 deletions(-)
> 
> 
One question below, but otherwise looks good,

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

<..>

>  
> +static int cxl_region_attach_auto(struct cxl_region *cxlr,
> +                                 struct cxl_endpoint_decoder *cxled, int pos)
> +{
> +       struct cxl_region_params *p = &cxlr->params;
> +
> +       if (cxled->state != CXL_DECODER_STATE_AUTO) {
> +               dev_err(&cxlr->dev,
> +                       "%s: unable to add decoder to autodetected region\n",
> +                       dev_name(&cxled->cxld.dev));
> +               return -EINVAL;
> +       }
> +
> +       if (pos >= 0) {
> +               dev_dbg(&cxlr->dev, "%s: expected auto position, not %d\n",
> +                       dev_name(&cxled->cxld.dev), pos);
> +               return -EINVAL;
> +       }
> +
> +       if (p->nr_targets >= p->interleave_ways) {
> +               dev_err(&cxlr->dev, "%s: no more target slots available\n",
> +                       dev_name(&cxled->cxld.dev));
> +               return -ENXIO;
> +       }
> +
> +       /*
> +        * Temporarily record the endpoint decoder into the target array. Yes,
> +        * this means that userspace can view devices in the wrong position
> +        * before the region activates, and must be careful to understand when
> +        * it might be racing region autodiscovery.
> +        */

Would it be worthwhile adding an attribute around this - either to
distinguish an auto-assembled region from a user-created one, or
perhaps better - something to mark the assembly complete? cxl-list
doesn't have to display this attribute as is, but maybe it can make a
decision to mark it as idle while assembly is pending, or maybe even
refuse to add_cxl_region() for it entirely?

This can be a follow-on too.

> +       pos = p->nr_targets;
> +       p->targets[pos] = cxled;
> +       cxled->pos = pos;
> +       p->nr_targets++;
> +
> +       return 0;
> +}
> +
>