Re: [RFC PATCH 04/15] cxl/core: Create distinct decoder structs

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

On Thu, Apr 14, 2022 at 6:45 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Wed, Apr 13, 2022 at 11:38 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > CXL HDM decoders have distinct properties at each level in the
> > hierarchy. Root decoders manage host physical address space. Switch
> > decoders manage demultiplexing of data to downstream targets. Endpoint
> > decoders must be aware of physical media size constraints. To properly
> > support these unique needs, create these unique structures.
> >
> > CXL HDM decoders do have similar architectural properties at all levels:
> > interleave properties, flags, types and consumption of host physical
> > address space. Those are retained and when possible, still utilized.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> >  drivers/cxl/core/hdm.c       |   3 +-
> >  drivers/cxl/core/port.c      | 102 ++++++++++++++++++++++++-----------
> >  drivers/cxl/cxl.h            |  69 +++++++++++++++++++++---
> >  tools/testing/cxl/test/cxl.c |   2 +-
> >  4 files changed, 137 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 3055e246aab9..37c09c77e9a7 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -6,6 +6,7 @@
> >
> >  #include "cxlmem.h"
> >  #include "core.h"
> > +#include "cxl.h"
> >
> >  /**
> >   * DOC: cxl core hdm
> > @@ -242,7 +243,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
> >                 struct cxl_decoder *cxld;
> >
> >                 if (is_cxl_endpoint(port))
> > -                       cxld = cxl_endpoint_decoder_alloc(port);
> > +                       cxld = &cxl_endpoint_decoder_alloc(port)->base;
>
> Please split to:
>
> cxled = cxl_endpoint_decoder_alloc(port);
> cxld = &cxled->base;
>
> >                 else
> >                         cxld = cxl_switch_decoder_alloc(port, target_count);
> >                 if (IS_ERR(cxld)) {
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index 86f451ecb7ed..8dd29c97e318 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -121,18 +121,19 @@ static DEVICE_ATTR_RO(target_type);
> >
> >  static ssize_t emit_target_list(struct cxl_decoder *cxld, char *buf)
> >  {
> > +       struct cxl_decoder_targets *t = cxl_get_decoder_targets(cxld);
> >         ssize_t offset = 0;
> >         int i, rc = 0;
> >
> >         for (i = 0; i < cxld->interleave_ways; i++) {
> > -               struct cxl_dport *dport = cxld->target[i];
> > +               struct cxl_dport *dport = t->target[i];
> >                 struct cxl_dport *next = NULL;
> >
> >                 if (!dport)
> >                         break;
> >
> >                 if (i + 1 < cxld->interleave_ways)
> > -                       next = cxld->target[i + 1];
> > +                       next = t->target[i + 1];
> >                 rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
> >                                    next ? "," : "");
> >                 if (rc < 0)
> > @@ -147,14 +148,15 @@ static ssize_t target_list_show(struct device *dev,
> >                                 struct device_attribute *attr, char *buf)
> >  {
> >         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> > +       struct cxl_decoder_targets *t = cxl_get_decoder_targets(cxld);
> >         ssize_t offset;
> >         unsigned int seq;
> >         int rc;
> >
> >         do {
> > -               seq = read_seqbegin(&cxld->target_lock);
> > +               seq = read_seqbegin(&t->target_lock);
> >                 rc = emit_target_list(cxld, buf);
> > -       } while (read_seqretry(&cxld->target_lock, seq));
> > +       } while (read_seqretry(&t->target_lock, seq));
> >
> >         if (rc < 0)
> >                 return rc;
> > @@ -199,23 +201,6 @@ static const struct attribute_group *cxl_decoder_root_attribute_groups[] = {
> >         NULL,
> >  };
> >
> > -static struct attribute *cxl_decoder_switch_attrs[] = {
> > -       &dev_attr_target_type.attr,
> > -       &dev_attr_target_list.attr,
> > -       NULL,
> > -};
> > -
> > -static struct attribute_group cxl_decoder_switch_attribute_group = {
> > -       .attrs = cxl_decoder_switch_attrs,
> > -};
> > -
> > -static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
> > -       &cxl_decoder_switch_attribute_group,
> > -       &cxl_decoder_base_attribute_group,
> > -       &cxl_base_attribute_group,
> > -       NULL,
> > -};
> > -
> >  static struct attribute *cxl_decoder_endpoint_attrs[] = {
> >         &dev_attr_target_type.attr,
> >         NULL,
> > @@ -232,6 +217,12 @@ static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = {
> >         NULL,
> >  };
> >
> > +static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
> > +       &cxl_decoder_base_attribute_group,
> > +       &cxl_base_attribute_group,
> > +       NULL,
> > +};
> > +
> >  static void cxl_decoder_release(struct device *dev)
> >  {
> >         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> > @@ -264,6 +255,7 @@ bool is_endpoint_decoder(struct device *dev)
> >  {
> >         return dev->type == &cxl_decoder_endpoint_type;
> >  }
> > +EXPORT_SYMBOL_NS_GPL(is_endpoint_decoder, CXL);
> >
> >  bool is_root_decoder(struct device *dev)
> >  {
> > @@ -1136,6 +1128,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_find_dport_by_dev, CXL);
> >  static int decoder_populate_targets(struct cxl_decoder *cxld,
> >                                     struct cxl_port *port, int *target_map)
> >  {
> > +       struct cxl_decoder_targets *t = cxl_get_decoder_targets(cxld);
> >         int i, rc = 0;
> >
> >         if (!target_map)
> > @@ -1146,21 +1139,72 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
> >         if (list_empty(&port->dports))
> >                 return -EINVAL;
> >
> > -       write_seqlock(&cxld->target_lock);
> > -       for (i = 0; i < cxld->nr_targets; i++) {
> > +       write_seqlock(&t->target_lock);
> > +       for (i = 0; i < t->nr_targets; i++) {
> >                 struct cxl_dport *dport = find_dport(port, target_map[i]);
> >
> >                 if (!dport) {
> >                         rc = -ENXIO;
> >                         break;
> >                 }
> > -               cxld->target[i] = dport;
> > +               t->target[i] = dport;
> >         }
> > -       write_sequnlock(&cxld->target_lock);
> > +       write_sequnlock(&t->target_lock);
> >
> >         return rc;
> >  }
> >
> > +static struct cxl_decoder *__cxl_decoder_alloc(struct cxl_port *port,
> > +                                              unsigned int nr_targets)
> > +{
> > +       struct cxl_decoder *cxld;
> > +
> > +       if (is_cxl_endpoint(port)) {
> > +               struct cxl_endpoint_decoder *cxled;
> > +
> > +               cxled = kzalloc(sizeof(*cxled), GFP_KERNEL);
> > +               if (!cxled)
> > +                       return NULL;
> > +               cxld = &cxled->base;
> > +       } else if (is_cxl_root(port)) {
> > +               struct cxl_root_decoder *cxlrd;
> > +
> > +               cxlrd = kzalloc(sizeof(*cxlrd), GFP_KERNEL);
> > +               if (!cxlrd)
> > +                       return NULL;
> > +
> > +               cxlrd->targets =
> > +                       kzalloc(struct_size(cxlrd->targets, target, nr_targets),
> > +                               GFP_KERNEL);
> > +               if (!cxlrd->targets) {
> > +                       kfree(cxlrd);
> > +                       return NULL;
> > +               }
> > +               cxlrd->targets->nr_targets = nr_targets;
> > +               seqlock_init(&cxlrd->targets->target_lock);
> > +               cxld = &cxlrd->base;
> > +       } else {
> > +               struct cxl_switch_decoder *cxlsd;
> > +
> > +               cxlsd = kzalloc(sizeof(*cxlsd), GFP_KERNEL);
> > +               if (!cxlsd)
> > +                       return NULL;
> > +
> > +               cxlsd->targets =
> > +                       kzalloc(struct_size(cxlsd->targets, target, nr_targets),
> > +                               GFP_KERNEL);
> > +               if (!cxlsd->targets) {
> > +                       kfree(cxlsd);
> > +                       return NULL;
> > +               }
> > +               cxlsd->targets->nr_targets = nr_targets;
> > +               seqlock_init(&cxlsd->targets->target_lock);
> > +               cxld = &cxlsd->base;
> > +       }
> > +
> > +       return cxld;
> > +}
> > +
> >  /**
> >   * cxl_decoder_alloc - Allocate a new CXL decoder
> >   * @port: owning port of this decoder
> > @@ -1186,7 +1230,7 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> >         if (nr_targets > CXL_DECODER_MAX_INTERLEAVE)
> >                 return ERR_PTR(-EINVAL);
> >
> > -       cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
> > +       cxld = __cxl_decoder_alloc(port, nr_targets);
> >         if (!cxld)
> >                 return ERR_PTR(-ENOMEM);
> >
> > @@ -1198,8 +1242,6 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> >         get_device(&port->dev);
> >         cxld->id = rc;
> >
> > -       cxld->nr_targets = nr_targets;
> > -       seqlock_init(&cxld->target_lock);
> >         dev = &cxld->dev;
> >         device_initialize(dev);
> >         device_set_pm_not_required(dev);
> > @@ -1274,12 +1316,12 @@ EXPORT_SYMBOL_NS_GPL(cxl_switch_decoder_alloc, CXL);
> >   *
> >   * Return: A new cxl decoder to be registered by cxl_decoder_add()
> >   */
> > -struct cxl_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port)
> > +struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port)
> >  {
> >         if (!is_cxl_endpoint(port))
> >                 return ERR_PTR(-EINVAL);
> >
> > -       return cxl_decoder_alloc(port, 0);
> > +       return to_cxl_endpoint_decoder(cxl_decoder_alloc(port, 0));
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_endpoint_decoder_alloc, CXL);
> >
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 6517d5cdf5ee..85fd5e84f978 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -193,6 +193,18 @@ enum cxl_decoder_type {
> >   */
> >  #define CXL_DECODER_MAX_INTERLEAVE 16
> >
> > +/**
> > + * struct cxl_decoder_targets - Target information for root and switch decoders.
> > + * @target_lock: coordinate coherent reads of the target list
> > + * @nr_targets: number of elements in @target
> > + * @target: active ordered target list in current decoder configuration
> > + */
> > +struct cxl_decoder_targets {
> > +       seqlock_t target_lock;
> > +       int nr_targets;
> > +       struct cxl_dport *target[];
> > +};
> > +
> >  /**
> >   * struct cxl_decoder - CXL address range decode configuration
> >   * @dev: this decoder's device
> > @@ -202,9 +214,6 @@ enum cxl_decoder_type {
> >   * @interleave_granularity: data stride per dport
> >   * @target_type: accelerator vs expander (type2 vs type3) selector
> >   * @flags: memory type capabilities and locking
> > - * @target_lock: coordinate coherent reads of the target list
> > - * @nr_targets: number of elements in @target
> > - * @target: active ordered target list in current decoder configuration
> >   */
> >  struct cxl_decoder {
> >         struct device dev;
> > @@ -214,11 +223,46 @@ struct cxl_decoder {
> >         int interleave_granularity;
> >         enum cxl_decoder_type target_type;
> >         unsigned long flags;
> > -       seqlock_t target_lock;
> > -       int nr_targets;
> > -       struct cxl_dport *target[];
> >  };
> >
> > +/**
> > + * struct cxl_endpoint_decoder - An decoder residing in a CXL endpoint.
> > + * @base: Base class decoder
> > + */
> > +struct cxl_endpoint_decoder {
> > +       struct cxl_decoder base;
> > +};
> > +
> > +/**
> > + * struct cxl_switch_decoder - A decoder in a switch or hostbridge.
> > + * @base: Base class decoder
> > + * @targets: Downstream targets for this switch.
> > + */
> > +struct cxl_switch_decoder {
> > +       struct cxl_decoder base;
> > +       struct cxl_decoder_targets *targets;
>
> Please no double-allocation when not necessary. This can be
>
> struct cxl_switch_decoder {
>        struct cxl_decoder base;
>        struct cxl_decoder_targets targets;
> };
>
> ...and then allocated with a single:
>
> cxlsd = kzalloc(struct_size(cxlsd, targets.target, nr_targets), GFP_KERNEL);
>
> ...or something like that (not compile tested).
>
> > +};
>
> > +};
> > +
> > +/**
> > + * struct cxl_root_decoder - A toplevel/platform decoder
> > + * @base: Base class decoder
> > + * @targets: Downstream targets (ie. hostbridges).
> > + */
> > +struct cxl_root_decoder {
> > +       struct cxl_decoder base;
> > +       struct cxl_decoder_targets *targets;
> > +};
>
> Ditto single allocation feedback...
>
> ...although now that struct cxl_root_decoder is identical to
> cxl_switch_decoder is there any benefit to making them distinct types
> beyond being pedantic? Making them the same type means
> cxl_get_decoder_targets() can go because all callers of that are
> already in known !cxl_endpoint_decoder paths. But of course ignore
> this if cxl_root_decoder is going to game some differentiating
> attributes down the road, I have not looked ahead in this series.

Actually regardless of whether root decoders gain additional
attributes they should simply be a strict super-set of root decoders:

struct cxl_endpoint_decoder {
       /* future endpoint decoder specific attributes go here */
      struct cxl_decoder base;
};

struct cxl_switch_decoder {
       struct cxl_decoder base;
       seqlock_t target_lock;
       int nr_targets;
       struct cxl_dport *target[];
};

struct cxl_root_decoder {
       /* future root decoder specific attributes go here */
       struct cxl_switch_decoder cxlsd;
};

> > +#define _to_cxl_decoder(x)                                                     \
> > +       static inline struct cxl_##x##_decoder *to_cxl_##x##_decoder(          \
> > +               struct cxl_decoder *cxld)                                      \
> > +       {                                                                      \
> > +               return container_of(cxld, struct cxl_##x##_decoder, base);     \
> > +       }
> > +
> > +_to_cxl_decoder(root)
> > +_to_cxl_decoder(switch)
> > +_to_cxl_decoder(endpoint)

Per above root decoders could no longer use this macro which was
already borderline not worth doing in my view.