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

From: Dan Williams <dan.j.williams@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: linux-cxl@vger.kernel.org, Ben Widawsky <ben.widawsky@intel.com>,
	Linux PCI <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v4 16/40] cxl/core/port: Use dedicated lock for decoder target list
Date: Mon, 31 Jan 2022 15:31:22 -0800	[thread overview]
Message-ID: <CAPcyv4irL1aT_xEWFaM=0YZz8U99qK6TeyAQxWyKriom4uzCBQ@mail.gmail.com> (raw)
In-Reply-To: <20220131155934.000064ac@Huawei.com>

On Mon, Jan 31, 2022 at 7:59 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 25 Jan 2022 18:54:36 -0800
> Dan Williams <dan.j.williams@intel.com> 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>
> 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.

Yeah, looks good.