Linux-CXL Archive on lore.kernel.org
 help / color / Atom feed
From: Dan Williams <dan.j.williams@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: linux-cxl@vger.kernel.org, Linux PCI <linux-pci@vger.kernel.org>,
	Linux ACPI <linux-acpi@vger.kernel.org>,
	"Weiny, Ira" <ira.weiny@intel.com>,
	Vishal L Verma <vishal.l.verma@intel.com>,
	"Schofield, Alison" <alison.schofield@intel.com>,
	Ben Widawsky <ben.widawsky@intel.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 5/8] cxl/acpi: Introduce ACPI0017 driver and cxl_root
Date: Thu, 15 Apr 2021 08:00:00 -0700
Message-ID: <CAPcyv4jvhc6wgne4r_zgJzB3PgSHgMtHrnsRCvx=F8JxPKfBCw@mail.gmail.com> (raw)
In-Reply-To: <20210406183232.00003189@Huawei.com>

On Tue, Apr 6, 2021 at 10:47 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 1 Apr 2021 07:31:09 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > While CXL builds upon the PCI software model for dynamic enumeration and
> > control, a static platform component is required to bootstrap the CXL
> > memory layout. In addition to identifying the host bridges ACPI is
> > responsible for enumerating the CXL memory space that can be addressed
> > by decoders. This is similar to the requirement for ACPI to publish
> > resources reported by _CRS for PCI host bridges.
> >
> > Introduce the cxl_root object as an abstract "port" into the CXL.mem
> > address space described by HDM decoders identified by the ACPI
> > CEDT.CHBS.
> >
> > For now just establish the initial boilerplate and sysfs attributes, to
> > be followed by enumeration of the ports within the host bridge.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> A few minor comments inline.
>
> > ---
> >  drivers/cxl/Kconfig  |   14 ++
> >  drivers/cxl/Makefile |    2
> >  drivers/cxl/acpi.c   |   39 ++++++
> >  drivers/cxl/core.c   |  349 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/cxl/cxl.h    |   64 +++++++++
> >  5 files changed, 468 insertions(+)
> >  create mode 100644 drivers/cxl/acpi.c
> >
> > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> > index 97dc4d751651..fb282af84afd 100644
> > --- a/drivers/cxl/Kconfig
> > +++ b/drivers/cxl/Kconfig
> > @@ -50,4 +50,18 @@ config CXL_MEM_RAW_COMMANDS
> >         potential impact to memory currently in use by the kernel.
> >
> >         If developing CXL hardware or the driver say Y, otherwise say N.
> > +
> > +config CXL_ACPI
> > +     tristate "CXL ACPI: Platform Support"
> > +     depends on ACPI
> > +     help
> > +       Enable support for host managed device memory (HDM) resources
> > +       published by a platform's ACPI CXL memory layout description.
> > +       See Chapter 9.14.1 CXL Early Discovery Table (CEDT) in the CXL
> > +       2.0 specification. The CXL core consumes these resource to
> > +       publish port and address_space objects used to map regions
> > +       that represent System RAM, or Persistent Memory regions to be
> > +       managed by LIBNVDIMM.
> > +
> > +       If unsure say 'm'.
> >  endif
> > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> > index 3808e39dd31f..f429ca6b59d9 100644
> > --- a/drivers/cxl/Makefile
> > +++ b/drivers/cxl/Makefile
> > @@ -1,7 +1,9 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_CXL_BUS) += cxl_core.o
> >  obj-$(CONFIG_CXL_MEM) += cxl_mem.o
> > +obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
> >
> >  ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
> >  cxl_core-y := core.o
> >  cxl_mem-y := mem.o
> > +cxl_acpi-y := acpi.o
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > new file mode 100644
> > index 000000000000..d54c2d5de730
> > --- /dev/null
> > +++ b/drivers/cxl/acpi.c
> > @@ -0,0 +1,39 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/acpi.h>
>
> swap acpi.h that for mod_devicetable.h unless this is going to
> need acpi.h later for something else.

It will need it after patch7, so I'll just leave it as is for now.

[..]
> > +static struct cxl_root *cxl_root_alloc(struct device *parent,
> > +                                    struct cxl_address_space *cxl_space,
> > +                                    int nr_spaces)
> > +{
> > +     struct cxl_root *cxl_root;
> > +     struct cxl_port *port;
> > +     struct device *dev;
> > +     int rc;
> > +
> > +     cxl_root = kzalloc(struct_size(cxl_root, address_space, nr_spaces),
> > +                        GFP_KERNEL);
> > +     if (!cxl_root)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     memcpy(cxl_root->address_space, cxl_space,
> > +            flex_array_size(cxl_root, address_space, nr_spaces));
> > +     cxl_root->nr_spaces = nr_spaces;
> > +
> > +     rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
> > +     if (rc < 0)
> > +             goto err;
> > +     port = &cxl_root->port;
> > +     port->id = rc;
> > +
> > +     /*
> > +      * Root does not have a cxl_port as its parent and it does not
> > +      * have any corresponding component registers it is only a
>
> have any corresponding component registers; it is only a
> .. or you could use two sentences

Sure.

>
> > +      * logical anchor to the first level of actual ports that decode
> > +      * the root address spaces.
> > +      */
> > +     port->port_host = parent;
> > +     port->target_id = -1;
> > +     port->component_regs_phys = -1;
> > +
> > +     dev = &port->dev;
> > +     device_initialize(dev);
> > +     device_set_pm_not_required(dev);
> > +     dev->parent = parent;
> > +     dev->bus = &cxl_bus_type;
> > +     dev->type = &cxl_root_type;
> > +
> > +     return cxl_root;
> > +
> > +err:
> > +     kfree(cxl_root);
> > +     return ERR_PTR(rc);
> > +}
> > +
> > +static struct cxl_address_space_dev *
> > +cxl_address_space_dev_alloc(struct device *parent,
> > +                         struct cxl_address_space *space)
> > +{
> > +     struct cxl_address_space_dev *cxl_asd;
> > +     struct resource *res;
> > +     struct device *dev;
> > +     int rc;
> > +
> > +     cxl_asd = kzalloc(sizeof(*cxl_asd), GFP_KERNEL);
> > +     if (!cxl_asd)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     res = &cxl_asd->res;
> > +     res->name = "CXL Address Space";
> > +     res->start = space->range.start;
> > +     res->end = space->range.end;
> > +     res->flags = IORESOURCE_MEM;
> > +
> > +     rc = insert_resource(&iomem_resource, res);
> > +     if (rc)
> > +             goto err;
> > +
> > +     cxl_asd->address_space = space;
> > +     dev = &cxl_asd->dev;
> > +     device_initialize(dev);
> > +     device_set_pm_not_required(dev);
> > +     dev->parent = parent;
> > +     dev->type = &cxl_address_space_type;
> > +
> > +     return cxl_asd;
> > +
> > +err:
> > +     kfree(cxl_asd);
> > +     return ERR_PTR(rc);
> > +}
> > +
> > +static int cxl_address_space_dev_add(struct device *host,
> > +                                  struct cxl_address_space_dev *cxl_asd,
> > +                                  int id)
> > +{
> > +     struct device *dev = &cxl_asd->dev;
> > +     int rc;
> > +
> > +     rc = dev_set_name(dev, "address_space%d", id);
> > +     if (rc)
> > +             goto err;
> > +
> > +     rc = device_add(dev);
> > +     if (rc)
> > +             goto err;
> > +
> > +     dev_dbg(host, "%s: register %s\n", dev_name(dev->parent),
> > +             dev_name(dev));
> > +
> > +     return devm_add_action_or_reset(host, unregister_dev, dev);
> > +
> > +err:
> > +     put_device(dev);
> This is unusual. The error handling is undoing something this function
> wasn't responsible for.  See below for suggested resolution.
>
> > +     return rc;
> > +}
> > +
> > +struct cxl_root *devm_cxl_add_root(struct device *host,
> > +                                struct cxl_address_space *cxl_space,
> > +                                int nr_spaces)
> > +{
> > +     struct cxl_root *cxl_root;
> > +     struct cxl_port *port;
> > +     struct device *dev;
> > +     int i, rc;
> > +
> > +     cxl_root = cxl_root_alloc(host, cxl_space, nr_spaces);
> > +     if (IS_ERR(cxl_root))
> > +             return cxl_root;
> > +
> > +     port = &cxl_root->port;
> > +     dev = &port->dev;
> > +     rc = dev_set_name(dev, "root%d", port->id);
> > +     if (rc)
> > +             goto err;
> > +
> > +     rc = device_add(dev);
> > +     if (rc)
> > +             goto err;
> > +
> > +     rc = devm_add_action_or_reset(host, unregister_dev, dev);
> > +     if (rc)
> > +             return ERR_PTR(rc);
> > +
> > +     for (i = 0; i < nr_spaces; i++) {
> > +             struct cxl_address_space *space = &cxl_root->address_space[i];
> > +             struct cxl_address_space_dev *cxl_asd;
> > +
> > +             if (!range_len(&space->range))
> > +                     continue;
> > +
> > +             cxl_asd = cxl_address_space_dev_alloc(dev, space);
> > +             if (IS_ERR(cxl_asd))
> > +                     return ERR_CAST(cxl_asd);
> > +
>
> Nothing is done between the dev_alloc() and the dev_add()
> and this is currently in the odd position of doing put_device() in the
> error path of *dev_add() when it wasn't responsible for getting the
> reference it is putting, dev_alloc() did that.

No, you missed the back and forth that Jason and I had about proper
device initialization flows:

https://lore.kernel.org/linux-cxl/161714738634.2168142.10860201861152789544.stgit@dwillia2-desk3.amr.corp.intel.com/

The put_device() is not undoing the dev_alloc(), it is undoing the
dev_alloc() + follow on allocations. Specifically it undoes the
dev_set_name() allocation. That is why the alloc and the add are split
into explicit code paths where the recovery shifts from alloc-unwind
to put_device().

> That suggests to me that we can clean up the oddity by just combining
> cxl_address_space_dev_alloc() and cxl_adress_space_dev_add() into one
>
> alloc_and_add() function (with a better name)

It's not an oddity and alloc_and_add() is an anti-pattern that leads to bugs.

>
> > +             rc = cxl_address_space_dev_add(host, cxl_asd, i);
>
> Lifetime management here seems overly complex.   Why not use host for both
> the alloc and add() devm calls?  I guess there is a good reason
> though so good to have a comment here saying what it is.

I'll add a kernel-doc to cxl_address_space_dev_add() to clarify what
is happening here.

  reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-01 14:30 [PATCH v2 0/8] CXL Port Enumeration Dan Williams
2021-04-01 14:30 ` [PATCH v2 1/8] cxl/mem: Move some definitions to mem.h Dan Williams
2021-04-06 16:38   ` Jonathan Cameron
2021-04-14  0:18     ` Dan Williams
2021-04-14  0:42       ` Dan Williams
2021-04-14  9:21         ` Jonathan Cameron
2021-04-01 14:30 ` [PATCH v2 2/8] cxl/mem: Introduce 'struct cxl_regs' for "composable" CXL devices Dan Williams
2021-04-06 17:00   ` Jonathan Cameron
2021-04-14  0:40     ` Dan Williams
2021-04-01 14:30 ` [PATCH v2 3/8] cxl/core: Rename bus.c to core.c Dan Williams
2021-04-01 14:31 ` [PATCH v2 4/8] cxl/core: Refactor CXL register lookup for bridge reuse Dan Williams
2021-04-06 17:00   ` Jonathan Cameron
2021-04-15 20:53     ` Dan Williams
2021-04-01 14:31 ` [PATCH v2 5/8] cxl/acpi: Introduce ACPI0017 driver and cxl_root Dan Williams
2021-04-01 21:34   ` kernel test robot
2021-04-06 17:32   ` Jonathan Cameron
2021-04-15 15:00     ` Dan Williams [this message]
2021-04-01 14:31 ` [PATCH v2 6/8] cxl/Kconfig: Default drivers to CONFIG_CXL_BUS Dan Williams
2021-04-01 14:31 ` [PATCH v2 7/8] cxl/port: Introduce cxl_port objects Dan Williams
2021-04-06 17:44   ` Jonathan Cameron
2021-04-08 22:42   ` Bjorn Helgaas
2021-04-09  2:13     ` Dan Williams
2021-04-13 17:18       ` Dan Williams
2021-04-14  1:14       ` Bjorn Helgaas
2021-04-15  5:21         ` Dan Williams
2021-04-01 14:31 ` [PATCH v2 8/8] cxl/acpi: Add module parameters to stand in for ACPI tables Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPcyv4jvhc6wgne4r_zgJzB3PgSHgMtHrnsRCvx=F8JxPKfBCw@mail.gmail.com' \
    --to=dan.j.williams@intel.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=alison.schofield@intel.com \
    --cc=ben.widawsky@intel.com \
    --cc=ira.weiny@intel.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=vishal.l.verma@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-CXL Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cxl/0 linux-cxl/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cxl linux-cxl/ https://lore.kernel.org/linux-cxl \
		linux-cxl@vger.kernel.org
	public-inbox-index linux-cxl

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cxl


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git