Re: [PATCH v2] cxl/core/bus: Document and tighten up decoder APIs

[Ben Widawsky <ben.widawsky@intel.com>]

On 21-09-21 14:41:20, Dan Williams wrote:
> On Wed, Sep 15, 2021 at 9:00 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > Since the code to add decoders for switches and endpoints is on the
> > horizon, document the recently added interfaces that will be consumed by
> > them.
> >
> > Part of the original version of this patch was subsumed by
> > f5786a5aedfc ("cxl/core: Split decoder setup into alloc + add")
> 
> It's dangerous to quote 'pending' commits. The stable commit id for this is now:
> 
> 48667f676189 ("cxl/core: Split decoder setup into alloc + add")

Yes, sorry. At the time I thought pending was going to become next but that
didn't happen and thus the danger.

> 
> ...I would have just fixed this up on applying, but some notes below.
> 
> This overall feels like a patch that should wait to go in with first
> introduction of endpoint decoders.

It's fine with me to wait for intro of endpoint decoders. I probably should have
requested you just take the kdoc stuff with your last series, but not worth it
now.

> 
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> > v2:
> > - Dropped removal of host from cxl_decoder_add (Ben)
> > - Change nr_targets to unsigned int (Jonathan)
> > - Move description of 0 special case to param kdoc (Dan, Ben)
> > - Reword kdocs to be more accurate (Jonathan, Ben)
> > - Add back debug message to decoder_populate_targets (Ben)
> > ---
> >
> >  drivers/cxl/core/bus.c | 34 ++++++++++++++++++++++++++++++++--
> >  drivers/cxl/cxl.h      |  2 ++
> >  2 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > index 396252749477..d242afe08402 100644
> > --- a/drivers/cxl/core/bus.c
> > +++ b/drivers/cxl/core/bus.c
> > @@ -474,6 +474,7 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
> >                         rc = -ENXIO;
> >                         goto out_unlock;
> >                 }
> > +               dev_dbg(&cxld->dev, "%s: target: %d\n", dev_name(dport->dport), i);
> 
> Seems like this deserves its own patch since it's not related to $subject.

I don't remember why I put this in this patch....

> 
> >                 cxld->target[i] = dport;
> >         }
> >
> > @@ -483,13 +484,28 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
> >         return rc;
> >  }
> >
> > -struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets)
> > +/**
> > + * cxl_decoder_alloc - Allocate a new CXL decoder
> > + * @port: owning port of this decoder
> > + * @nr_targets: downstream targets accessible by this decoder. All upstream
> > + *             ports and root ports must have at least 1 target. Endpoint
> > + *             devices will have 0 targets. Callers wishing to register an
> > + *             endpoint device should specify 0.
> > + *
> > + * A port should contain one or more decoders. Each of those decoders enable
> > + * some address space for CXL.mem utilization. A decoder is expected to be
> > + * configured by the caller before registering.
> > + *
> > + * Return: A new cxl decoder to be registered by cxl_decoder_add()
> > + */
> > +struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> > +                                     unsigned int nr_targets)
> >  {
> >         struct cxl_decoder *cxld;
> >         struct device *dev;
> >         int rc = 0;
> >
> > -       if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets < 1)
> > +       if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets == 0)
> 
> This looks broken as the above text says that 0 is now an allowed
> value. Shouldn't this be:
> 
> -    if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets < 1)
> +    if (nr_targets > CXL_DECODER_MAX_INTERLEAVE)
> 
> ...?
> 

Yes. This got messed up. I actually originally intended this change as part of
the endpoint decoder series. I'd rather drop this and just change to unsigned
int.

> >                 return ERR_PTR(-EINVAL);
> >
> >         cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
> > @@ -521,6 +537,20 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets)
> >  }
> >  EXPORT_SYMBOL_GPL(cxl_decoder_alloc);
> >
> > +/**
> > + * cxl_decoder_add - Add a decoder with targets
> > + * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
> > + * @target_map: A list of downstream ports that this decoder can direct memory
> > + *              traffic to. These numbers should correspond with the port number
> > + *              in the PCIe Link Capabilities structure.
> > + *
> > + * Certain types of decoders may not have any targets. The main example of this
> > + * is an endpoint device. A more awkward example is a hostbridge whose root
> > + * ports get hot added (technically possible, though unlikely).
> 
> I don't think this is possible, certainly not worth calling out unless
> you want to prompt someone to go audit the PCI side for proper
> operation in this case.
> 
> The endpoint note is enough.

I ran it by Chet as to whether or not it's technically possible, which it is
according to him. I'm fine to drop it though.

> 
> > + *
> > + * Return: Negative error code if the decoder wasn't properly configured; else
> > + *        returns 0.
> > + */
> >  int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> >  {
> >         struct cxl_port *port;
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 7d6b011dd963..e632cc8da091 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -289,6 +289,8 @@ int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
> >  struct cxl_decoder *to_cxl_decoder(struct device *dev);
> >  bool is_root_decoder(struct device *dev);
> >  struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets);
> > +struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> > +                                     unsigned int nr_targets);
> 
> Why are there now 2 different cxl_decoder_alloc() function signatures?

This was unintentional.

My apologies for the sloppy patch.