On Mon, Jan 31, 2022 at 7:59 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> 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.
Yeah, looks good.
