Re: [PATCH v3 09/14] cxl/region: Add infrastructure for decoder programming

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 27 Jan 2022 16:27:02 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> There are 3 steps in handling region programming once it has been
> configured by userspace.
> 1. Sanitize the parameters against the system.
> 2. Collect decoder resources from the topology
> 3. Program decoder resources
> 
> The infrastructure added here addresses #2. Two new APIs are introduced
> to allow collecting and returning decoder resources. Additionally the
> infrastructure includes two lists managed by the region driver, a staged
> list, and a commit list. The staged list contains those collected in
> step #2, and the commit list are all the decoders programmed in step #3.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

Minor comments inline.

>  static void cxld_unregister(void *dev)
>  {
>  	device_unregister(dev);
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 784e4ba25128..a62d48454a56 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -440,6 +440,8 @@ struct cxl_region *cxl_alloc_region(struct cxl_decoder *cxld, int id)
>  	if (!cxlr)
>  		return ERR_PTR(-ENOMEM);
>  
> +	INIT_LIST_HEAD(&cxlr->staged_list);
> +	INIT_LIST_HEAD(&cxlr->commit_list);
>  	cxlr->id = id;
>  
>  	return cxlr;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index ed984465b59c..8ace6cca0776 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -35,6 +35,8 @@
>  #define   CXL_CM_CAP_CAP_ID_HDM 0x5
>  #define   CXL_CM_CAP_CAP_HDM_VERSION 1
>  
> +#define CXL_DECODER_MAX_INSTANCES 10
> +
>  /* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */
>  #define CXL_HDM_DECODER_CAP_OFFSET 0x0
>  #define   CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0)
> @@ -265,6 +267,7 @@ enum cxl_decoder_type {
>   * @target_lock: coordinate coherent reads of the target list
>   * @region_ida: allocator for region ids.
>   * @address_space: Used/free address space for regions.
> + * @region_link: This decoder's place on either the staged, or commit list.
>   * @nr_targets: number of elements in @target
>   * @target: active ordered target list in current decoder configuration
>   */
> @@ -282,6 +285,7 @@ struct cxl_decoder {
>  	seqlock_t target_lock;
>  	struct ida region_ida;
>  	struct gen_pool *address_space;
> +	struct list_head region_link;
>  	int nr_targets;
>  	struct cxl_dport *target[];
>  };
> @@ -326,6 +330,7 @@ struct cxl_nvdimm {
>   * @id: id for port device-name
>   * @dports: cxl_dport instances referenced by decoders
>   * @endpoints: cxl_ep instances, endpoints that are a descendant of this port
> + * @region_link: this port's node on the region's list of ports

docs but no field in the structure...


>   * @decoder_ida: allocator for decoder ids
>   * @component_reg_phys: component register capability base address (optional)
>   * @dead: last ep has been removed, force port re-creation
> @@ -396,6 +401,8 @@ struct cxl_port *find_cxl_root(struct device *dev);
>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
>  int cxl_bus_rescan(void);
>  struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd);
> +struct cxl_decoder *cxl_get_decoder(struct cxl_port *port);
> +void cxl_put_decoder(struct cxl_decoder *cxld);
>  bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
>  
>  struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> @@ -406,6 +413,7 @@ struct cxl_dport *cxl_find_dport_by_dev(struct cxl_port *port,
>  struct cxl_port *ep_find_cxl_port(struct cxl_memdev *cxlmd, unsigned int depth);
>  
>  struct cxl_decoder *to_cxl_decoder(struct device *dev);
> +bool is_cxl_decoder(struct device *dev);

They are multiplying!! (see 2 lines down.)

>  bool is_root_decoder(struct device *dev);
>  bool is_cxl_decoder(struct device *dev);
>  struct cxl_decoder *cxl_root_decoder_alloc(struct cxl_port *port,

> diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> index d2f6c990c8a8..145d7bb02714 100644
> --- a/drivers/cxl/region.c
> +++ b/drivers/cxl/region.c
> @@ -359,21 +359,59 @@ static bool has_switch(const struct cxl_region *cxlr)
>  	return false;
>  }
>

...

>  static int bind_region(const struct cxl_region *cxlr)
> @@ -559,7 +648,7 @@ static int cxl_region_probe(struct device *dev)
>  		return -ENXIO;
>  	}
>  
> -	if (!rootd_valid(cxlr, rootd)) {
> +	if (!rootd_valid(cxlr, rootd, true)) {
>  		dev_err(dev, "Picked invalid rootd\n");
>  		return -ENXIO;
>  	}
> @@ -574,14 +663,18 @@ static int cxl_region_probe(struct device *dev)
>  
>  	ret = collect_ep_decoders(cxlr);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
>  	ret = bind_region(cxlr);
> -	if (!ret) {
> -		cxlr->active = true;
> -		dev_info(dev, "Bound");
> -	}
> +	if (ret)
> +		goto err;
>  
> +	cxlr->active = true;
> +	dev_info(dev, "Bound");

Not keen on this being always printed in the logs.  dev_dbg() perhaps with
some more detail may be 

> +	return 0;
> +
> +err:
> +	cleanup_staged_decoders(cxlr);
>  	return ret;
>  }
>  
> diff --git a/drivers/cxl/region.h b/drivers/cxl/region.h
> index 00a6dc729c26..fc15abaeb638 100644
> --- a/drivers/cxl/region.h
> +++ b/drivers/cxl/region.h
> @@ -14,6 +14,9 @@
>   * @list: Node in decoder's region list.
>   * @res: Resource this region carves out of the platform decode range.
>   * @active: If the region has been activated.
> + * @staged_list: All decoders staged for programming.
> + * @commit_list: All decoders programmed for this region's parameters.
> + *

Why the blank line?  If it makes sense should be in earlier patch
and I'm not sure if kernel-doc allows blank lines in the list.


>   * @config: HDM decoder program config
>   * @config.size: Size of the region determined from LSA or userspace.
>   * @config.uuid: The UUID for this region.
> @@ -27,6 +30,8 @@ struct cxl_region {
>  	struct list_head list;
>  	struct resource *res;
>  	bool active;
> +	struct list_head staged_list;
> +	struct list_head commit_list;
>  
>  	struct {
>  		u64 size;