Re: [PATCH 02/13] cxl/core/bus: Add kernel docs for decoder ops

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

On 21-09-10 11:51:14, Dan Williams wrote:
> On Thu, Sep 2, 2021 at 12:50 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > Since the code to add decoders for switches and endpoints is on the
> > horizon, document the new interfaces that will be consumed by them.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> >  drivers/cxl/core/bus.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > index 3991ac231c3e..9d98dd50d424 100644
> > --- a/drivers/cxl/core/bus.c
> > +++ b/drivers/cxl/core/bus.c
> > @@ -453,6 +453,19 @@ int cxl_add_dport(struct cxl_port *port, struct device *dport_dev, int port_id,
> >  }
> >  EXPORT_SYMBOL_GPL(cxl_add_dport);
> >
> > +/**
> > + * cxl_decoder_alloc - Allocate a new CXL decoder
> > + * @port: owning port of this decoder
> > + * @nr_targets: downstream targets accessible by this decoder
> > + *
> > + * A port should contain one or more decoders. Each of those decoders enable
> > + * some address space for CXL.mem utilization. Therefore, it is logical to
> 
> I think a "therefore it is logical" statement is changelog fodder.
> Once the code is in the kernel it does not need to keep justifying its
> existence.

I agree. This is more appropriate as a commit message.

> 
> > + * allocate decoders while enumerating a port. While >= 1 is defined by the CXL
> > + * specification, due to error conditions it is possible that a port may have 0
> > + * decoders.
> 
> This comment feels out of place. Why does cxl_decoder_alloc() care how
> many decoders a port has? I would expect this comment on a cxl_port
> api that is trying to walk decoders.
> 

I partially agree. The function implementation cares simply for heap allocation
and this detail shouldn't be a part of kdocs. However, as a public API in core,
I think it's warranted to mention cases which might not immediately be obvious.
The main purpose was to change this text when adding endpoints. I didn't
actually end up doing that unfortunately. As such, I think I will move this bit
to the description of nr_targets above.

> > + *
> > + * Return: A new cxl decoder which wants to be added with cxl_decoder_add()
> 
> s/which wants to be added/to be registered by/
> 
> > + */
> >  struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets)
> >  {
> >         struct cxl_decoder *cxld;
> > @@ -491,6 +504,21 @@ 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
> > + * @host: The containing struct device. This is typically the PCI device that is
> > + *        CXL capable
> 
> No, this is the device doing the enumeration. After the devm removal
> for decoder creation it's now only being used to print a debug
> message. Do you have another use for it? Perhaps it should just be
> deleted. The new cxl_decoder_autoremove() handles what @host was used
> for previously.

I have no use for it beyond what's there. Looks like it should be dropped to me
as well. I'll resend with that removal.

> 
> > + * @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.
> > + *
> > + * Return: 0 if decoder was successfully added.
> > + *
> > + * 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).
> > + */
> >  int cxl_decoder_add(struct device *host, struct cxl_decoder *cxld,
> >                     int *target_map)
> >  {
> > --
> > 2.33.0
> >