Re: [PATCH v3 01/14] cxl/region: Add region creation ABI

From: Ben Widawsky <ben.widawsky@intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: linux-cxl@vger.kernel.org, patches@lists.linux.dev,
	Alison Schofield <alison.schofield@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Bjorn Helgaas <helgaas@kernel.org>,
	Linux NVDIMM <nvdimm@lists.linux.dev>,
	Linux PCI <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v3 01/14] cxl/region: Add region creation ABI
Date: Wed, 2 Feb 2022 10:48:13 -0800	[thread overview]
Message-ID: <20220202184813.euepn3m2twpybpoc@intel.com> (raw)
In-Reply-To: <20220202182811.ivupsaeogyiwl5so@intel.com>

On 22-02-02 10:28:11, Ben Widawsky wrote:
> On 22-02-02 10:26:06, Ben Widawsky wrote:
> > On 22-01-28 10:59:26, Dan Williams wrote:
> > > On Fri, Jan 28, 2022 at 10:14 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > > [..]
> > > > Here is that put_device() I was expecting, that kfree() earlier was a
> > > > double-free it seems.
> > > >
> > > > Also, I would have expected a devm action to remove this. Something like:
> > > >
> > > > struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > > >
> > > > cxl_device_lock(&port->dev);
> > > > if (port->dev.driver)
> > > >     devm_cxl_add_region(port->uport, cxld, id);
> > 
> > I assume you mean devm_cxl_delete_region(), yes?
> > 
> > > > else
> > > >     rc = -ENXIO;
> > > > cxl_device_unlock(&port->dev);
> > > >
> > > > ...then no matter what you know the region will be unregistered when
> > > > the root port goes away.
> > > 
> > > ...actually, the lock and ->dev.driver check here are not needed
> > > because this attribute is only registered while the cxl_acpi driver is
> > > bound. So, it is safe to assume this is protected as decoder remove
> > > synchronizes against active sysfs users.
> > 
> > I'm somewhat confused when you say devm action to remove this. The current auto
> > region deletion happens when the ->release() is called. Are you suggesting when
> > the root decoder is removed I delete the regions at that point?
> 
> Hmm. I went back and looked and I had changed this functionality at some
> point... So forget I said that, it isn't how it's working currently. But the
> question remains, are you suggesting I delete in the root decoder
> unregistration?

I think it's easier if I write what I think you mean.... Here are the relevant
parts:

devm_cxl_region_delete() is removed entirely.

static void unregister_region(void *_cxlr)
{
        struct cxl_region *cxlr = _cxlr;

        device_unregister(&cxlr->dev);
}

static int devm_cxl_region_add(struct cxl_decoder *cxld, struct cxl_region *cxlr)
{
        struct cxl_port *port = to_cxl_port(cxld->dev.parent);
        struct device *dev = &cxlr->dev;
        int rc;

        rc = dev_set_name(dev, "region%d.%d:%d", port->id, cxld->id, cxlr->id);
        if (rc)
                return rc;

        rc = device_add(dev);
        if (rc)
                return rc;

        return devm_add_action_or_reset(&cxld->dev, unregister_region, cxlr);
}

static ssize_t delete_region_store(struct device *dev,
                                   struct device_attribute *attr,
                                   const char *buf, size_t len)
{
        struct cxl_decoder *cxld = to_cxl_decoder(dev);
        struct cxl_region *cxlr;

        cxlr = cxl_find_region_by_name(cxld, buf);
        if (IS_ERR(cxlr))
                return PTR_ERR(cxlr);

        devm_release_action(dev, unregister_region, cxlr);

        return len;
}
DEVICE_ATTR_WO(delete_region);