Re: [PATCH v4 16/40] cxl/core/port: Use dedicated lock for decoder target list

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

On 22-01-25 18:54:36, Dan Williams wrote:
> Lockdep reports:
> 
>  ======================================================
>  WARNING: possible circular locking dependency detected
>  5.16.0-rc1+ #142 Tainted: G           OE
>  ------------------------------------------------------
>  cxl/1220 is trying to acquire lock:
>  ffff979b85475460 (kn->active#144){++++}-{0:0}, at: __kernfs_remove+0x1ab/0x1e0
> 
>  but task is already holding lock:
>  ffff979b87ab38e8 (&dev->lockdep_mutex#2/4){+.+.}-{3:3}, at: cxl_remove_ep+0x50c/0x5c0 [cxl_core]
> 
> ...where cxl_remove_ep() is a helper that wants to delete ports while
> holding a lock on the host device for that port. That sets up a lockdep
> violation whereby target_list_show() can not rely holding the decoder's
> device lock while walking the target_list. Switch to a dedicated seqlock
> for this purpose.
> 
> Reported-by: Ben Widawsky <ben.widawsky@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Changes in v4:
> - Fix missing unlock in error exit case (Ben)

Could you help me understand why we need a lock at all for the target list? I
thought the target list remains static throughout the lifetime of the decoder at
which point, the only issue would be reading the sysfs entries while the decoder
is being destroyed. Is that possible?

> 
>  drivers/cxl/core/port.c |   30 ++++++++++++++++++++++++------
>  drivers/cxl/cxl.h       |    2 ++
>  2 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index f58b2d502ac8..5188d47180f1 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -104,14 +104,11 @@ static ssize_t target_type_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(target_type);
>  
> -static ssize_t target_list_show(struct device *dev,
> -			       struct device_attribute *attr, char *buf)
> +static ssize_t emit_target_list(struct cxl_decoder *cxld, char *buf)
>  {
> -	struct cxl_decoder *cxld = to_cxl_decoder(dev);
>  	ssize_t offset = 0;
>  	int i, rc = 0;
>  
> -	cxl_device_lock(dev);
>  	for (i = 0; i < cxld->interleave_ways; i++) {
>  		struct cxl_dport *dport = cxld->target[i];
>  		struct cxl_dport *next = NULL;
> @@ -127,10 +124,28 @@ static ssize_t target_list_show(struct device *dev,
>  			break;
>  		offset += rc;
>  	}
> -	cxl_device_unlock(dev);
>  
>  	if (rc < 0)
>  		return rc;
> +	return offset;
> +}
> +
> +static ssize_t target_list_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +	ssize_t offset;
> +	unsigned int seq;
> +	int rc;
> +
> +	do {
> +		seq = read_seqbegin(&cxld->target_lock);
> +		rc = emit_target_list(cxld, buf);
> +	} while (read_seqretry(&cxld->target_lock, seq));
> +
> +	if (rc < 0)
> +		return rc;
> +	offset = rc;
>  
>  	rc = sysfs_emit_at(buf, offset, "\n");
>  	if (rc < 0)
> @@ -494,15 +509,17 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
>  		goto out_unlock;
>  	}
>  
> +	write_seqlock(&cxld->target_lock);
>  	for (i = 0; i < cxld->nr_targets; i++) {
>  		struct cxl_dport *dport = find_dport(port, target_map[i]);
>  
>  		if (!dport) {
>  			rc = -ENXIO;
> -			goto out_unlock;
> +			break;
>  		}
>  		cxld->target[i] = dport;
>  	}
> +	write_sequnlock(&cxld->target_lock);
>  
>  out_unlock:
>  	cxl_device_unlock(&port->dev);
> @@ -543,6 +560,7 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
>  
>  	cxld->id = rc;
>  	cxld->nr_targets = nr_targets;
> +	seqlock_init(&cxld->target_lock);
>  	dev = &cxld->dev;
>  	device_initialize(dev);
>  	device_set_pm_not_required(dev);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 569cbe7f23d6..47c256ad105f 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -185,6 +185,7 @@ enum cxl_decoder_type {
>   * @interleave_granularity: data stride per dport
>   * @target_type: accelerator vs expander (type2 vs type3) selector
>   * @flags: memory type capabilities and locking
> + * @target_lock: coordinate coherent reads of the target list
>   * @nr_targets: number of elements in @target
>   * @target: active ordered target list in current decoder configuration
>   */
> @@ -199,6 +200,7 @@ struct cxl_decoder {
>  	int interleave_granularity;
>  	enum cxl_decoder_type target_type;
>  	unsigned long flags;
> +	seqlock_t target_lock;
>  	int nr_targets;
>  	struct cxl_dport *target[];
>  };
>