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

From: Dan Williams <dan.j.williams@intel.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: linux-cxl@vger.kernel.org,
	Alison Schofield <alison.schofield@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Vishal Verma <vishal.l.verma@intel.com>
Subject: Re: [PATCH 02/13] cxl/core/bus: Add kernel docs for decoder ops
Date: Fri, 10 Sep 2021 11:51:14 -0700	[thread overview]
Message-ID: <CAPcyv4gF12w-rF5Qh0Y8=JxxDEdahnX4KVvy7SwJhY2hLBhChw@mail.gmail.com> (raw)
In-Reply-To: <20210902195017.2516472-3-ben.widawsky@intel.com>

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.

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

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

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