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

[Dan Williams <dan.j.williams@intel.com>]

Verma, Vishal L wrote:
> 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.

"Assembly complete" is determined by "commit" going active. What about
all of the "targetX" attributes printing the decoder-name out with a prefix
like:

    "auto:decoderX.Y"

...that way userspace can both see what candidate decoders the kernel is
considering, and the fact that assembly is still in progress (or
stalled).

The concern thought is breaking legacy that only ever expects to read
decoder names there... guess it depends on how bad the failure mode is.

Instead, maybe a new attribute like "origin" that returns "manual" vs
"auto"?