On Tue, 25 Jan 2022 18:54:36 -0800
Dan Williams <dan.j.williams@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
Suggested additional tidy up inline.
Thanks,
Jonathan
> ---
> Changes in v4:
> - Fix missing unlock in error exit case (Ben)
>
> 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;
Now you don't have a lock to unlock above, the only path that can
hit this if (rc < 0) is an if (rc < 0) in the for loop.
Perhaps just return directly there.
> + 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[];
> };
>
