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

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: <linux-cxl@vger.kernel.org>,
	Alison Schofield <alison.schofield@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	"Ira Weiny" <ira.weiny@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>
Subject: Re: [PATCH 02/13] cxl/core/bus: Add kernel docs for decoder ops
Date: Fri, 3 Sep 2021 15:17:40 +0100	[thread overview]
Message-ID: <20210903151740.00005fcd@Huawei.com> (raw)
In-Reply-To: <20210902195017.2516472-3-ben.widawsky@intel.com>

On Thu, 2 Sep 2021 12:50:06 -0700
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.

Don't look "new" given they are already there...

> 
> 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
number of downstream targets accessible by this decoder

perhaps.  Would be an odd name for a bitmap of them for example but the
help text doesn't rule that out.

> + *
> + * 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
> + * 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.
> + *
> + * Return: A new cxl decoder which wants to be added with cxl_decoder_add()

CXL decoder

Anything else possible?

> + */
>  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

I would drop the 'struct' from that statement and just have device otherwise
it seems to imply something code related rather than physical device related.

> + * @cxld: The cxl decoder allocated by cxl_decoder_alloc()

CXL decoder

> + * @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.

What else is possible?

> + *
> + * 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)
>  {