| Message ID | 162295951736.1109360.12642726343803988356.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | CXL port and decoder enumeration | expand |
On Sat, 5 Jun 2021 23:05:17 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > In preparation for infrastructure that enumerates and configures the CXL > decode mechanism of an upstream port to its downstream ports, add a > representation for a CXL downstream port. > > On ACPI systems the top-most logical downstream ports in the hierarchy > are the host bridges (ACPI0016 devices) that decode the memory windows > described by the CXL Early Discovery Table Fixed Memory Window > Structures (CEDT.CFMWS). > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > Documentation/ABI/testing/sysfs-bus-cxl | 13 ++++ > drivers/cxl/acpi.c | 44 ++++++++++++++ > drivers/cxl/core.c | 95 ++++++++++++++++++++++++++++++- > drivers/cxl/cxl.h | 21 +++++++ > 4 files changed, 169 insertions(+), 4 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > index fb996ced7629..0cb31b7ad17b 100644 > --- a/Documentation/ABI/testing/sysfs-bus-cxl > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > @@ -35,3 +35,16 @@ Description: > CXL component registers. The 'uport' symlink connects the CXL > portX object to the device that published the CXL port > capability. > + > +What: /sys/bus/cxl/devices/portX/dportY > +Date: June, 2021 > +KernelVersion: v5.14 > +Contact: linux-cxl@vger.kernel.org > +Description: > + CXL port objects are enumerated from either a platform firmware > + device (ACPI0017 and ACPI0016) or PCIe switch upstream port with > + CXL component registers. The 'dportY' symlink identifies one or > + more downstream ports that the upstream port may target in its > + decode of CXL memory resources. The 'Y' integer reflects the > + hardware port unique-id used in the hardware decoder target > + list. > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index 556d25ab6966..0ae7464b603d 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -5,19 +5,61 @@ > #include <linux/device.h> > #include <linux/kernel.h> > #include <linux/acpi.h> > +#include <linux/pci.h> Not immediately seeing why this include is added in this patch. > #include "cxl.h" > > +static struct acpi_device *to_cxl_host_bridge(struct device *dev) > +{ > + struct acpi_device *adev = to_acpi_device(dev); > + > + if (strcmp(acpi_device_hid(adev), "ACPI0016") == 0) > + return adev; > + return NULL; > +} > + > +static int add_host_bridge_dport(struct device *match, void *arg) > +{ > + int rc; > + acpi_status status; > + unsigned long long uid; > + struct cxl_port *root_port = arg; > + struct device *host = root_port->dev.parent; > + struct acpi_device *bridge = to_cxl_host_bridge(match); > + > + if (!bridge) > + return 0; > + > + status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL, > + &uid); > + if (status != AE_OK) { > + dev_err(host, "unable to retrieve _UID of %s\n", > + dev_name(match)); > + return -ENODEV; > + } > + > + rc = cxl_add_dport(root_port, match, uid, CXL_RESOURCE_NONE); > + if (rc) { > + dev_err(host, "failed to add downstream port: %s\n", > + dev_name(match)); > + return rc; > + } > + dev_dbg(host, "add dport%llu: %s\n", uid, dev_name(match)); > + return 0; > +} > + > static int cxl_acpi_probe(struct platform_device *pdev) > { > struct cxl_port *root_port; > struct device *host = &pdev->dev; > + struct acpi_device *adev = ACPI_COMPANION(host); > > root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); > if (IS_ERR(root_port)) > return PTR_ERR(root_port); > dev_dbg(host, "add: %s\n", dev_name(&root_port->dev)); > > - return 0; > + return bus_for_each_dev(adev->dev.bus, NULL, root_port, > + add_host_bridge_dport); > } > > static const struct acpi_device_id cxl_acpi_ids[] = { > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c > index dbbb34618d7d..4418b30cce4f 100644 > --- a/drivers/cxl/core.c > +++ b/drivers/cxl/core.c > @@ -33,10 +33,22 @@ static struct attribute_group cxl_base_attribute_group = { > .attrs = cxl_base_attributes, > }; > > +static void cxl_dport_release(struct cxl_dport *dport) > +{ > + put_device(dport->dport); > + list_del(&dport->list); This ordering isn't the reverse of what happens cxl_add_dport() That would be list_del() put_device() kfree() If there is a strong reason for that I'd like to see a comment here. > + kfree(dport); > +} > + > static void cxl_port_release(struct device *dev) > { > struct cxl_port *port = to_cxl_port(dev); > + struct cxl_dport *dport, *_d; > > + device_lock(dev); > + list_for_each_entry_safe(dport, _d, &port->dports, list) > + cxl_dport_release(dport); > + device_unlock(dev); > ida_free(&cxl_port_ida, port->id); > kfree(port); > } > @@ -60,9 +72,22 @@ struct cxl_port *to_cxl_port(struct device *dev) > return container_of(dev, struct cxl_port, dev); > } > > -static void unregister_dev(void *dev) > +static void unregister_port(void *_port) > { > - device_unregister(dev); > + struct cxl_port *port = _port; > + struct cxl_dport *dport; > + > + device_lock(&port->dev); > + list_for_each_entry(dport, &port->dports, list) { > + char link_name[CXL_TARGET_STRLEN]; > + > + if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", > + dport->port_id) >= CXL_TARGET_STRLEN) > + continue; > + sysfs_remove_link(&port->dev.kobj, link_name); > + } > + device_unlock(&port->dev); > + device_unregister(&port->dev); > } > > static void cxl_unlink_uport(void *_port) > @@ -113,6 +138,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport, > > port->uport = uport; > port->component_reg_phys = component_reg_phys; > + INIT_LIST_HEAD(&port->dports); > > device_initialize(dev); > device_set_pm_not_required(dev); > @@ -157,7 +183,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > if (rc) > goto err; > > - rc = devm_add_action_or_reset(host, unregister_dev, dev); > + rc = devm_add_action_or_reset(host, unregister_port, port); > if (rc) > return ERR_PTR(rc); > > @@ -173,6 +199,69 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > } > EXPORT_SYMBOL_GPL(devm_cxl_add_port); > > +static int add_dport(struct cxl_port *port, struct cxl_dport *new) > +{ > + struct cxl_dport *dport, *dup = NULL; > + > + device_lock(&port->dev); > + list_for_each_entry (dport, &port->dports, list) > + if (new->port_id == dport->port_id) { > + dup = dport; > + goto err; > + } > + list_add_tail(&new->list, &port->dports); > + device_unlock(&port->dev); > + > + return 0; > + > +err: > + device_unlock(&port->dev); > + dev_err(&port->dev, > + "unable to add dport%d-%s non-unique port id (%s)\n", > + new->port_id, dev_name(new->dport), dev_name(dup->dport)); As there is potential that you might end up with other errors in here long term, why not move this to the point where the error is detected? I think you are fine doing it under the mutex. Obviously indent will be a deeper than ideal. > + return -EEXIST; > +} > + > +/* This is a bit inconsistent wrt to what functions get full kernel-doc. My personal preference would be all the exported functions + any others where it is particularly useful. > + * Append downstream port data to a cxl_port, note that all allocations > + * and links are undone by cxl_port deletion and release. > + */ > +int cxl_add_dport(struct cxl_port *port, struct device *dport_dev, int port_id, > + resource_size_t component_reg_phys) > +{ > + char link_name[CXL_TARGET_STRLEN]; > + struct cxl_dport *dport; > + int rc; > + > + if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >= > + CXL_TARGET_STRLEN) > + return -EINVAL; > + > + dport = kzalloc(sizeof(*dport), GFP_KERNEL); > + if (!dport) > + return -ENOMEM; > + > + INIT_LIST_HEAD(&dport->list); > + dport->dport = get_device(dport_dev); > + dport->port_id = port_id; > + dport->component_reg_phys = component_reg_phys; > + dport->port = port; > + > + rc = add_dport(port, dport); > + if (rc) If you get an error here, it's not been added to the list, but in the cxl_dport_release() you remove it from the list. I think you just want to put and free the device here. > + goto err; > + > + rc = sysfs_create_link(&port->dev.kobj, &dport_dev->kobj, link_name); > + if (rc) > + goto err; > + > + return 0; > +err: > + cxl_dport_release(dport); > + return rc; > +} > +EXPORT_SYMBOL_GPL(cxl_add_dport); > + > /** > * cxl_probe_component_regs() - Detect CXL Component register blocks > * @dev: Host device of the @base mapping > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 46c81165c210..0b89fcfe728a 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -146,6 +146,7 @@ int cxl_map_device_regs(struct pci_dev *pdev, > struct cxl_register_map *map); > > #define CXL_RESOURCE_NONE ((resource_size_t) -1) > +#define CXL_TARGET_STRLEN 20 > > /** > * struct cxl_port - logical collection of upstream port devices and > @@ -154,19 +155,39 @@ int cxl_map_device_regs(struct pci_dev *pdev, > * @dev: this port's device > * @uport: PCI or platform device implementing the upstream port capability > * @id: id for port device-name > + * @dports: cxl_dport instances referenced by decoders > * @component_regs_phys: component register capability base address (optional) > */ > struct cxl_port { > struct device dev; > struct device *uport; > int id; > + struct list_head dports; > resource_size_t component_reg_phys; > }; > > +/** > + * struct cxl_dport - CXL downstream port > + * @dport: PCI bridge or firmware device representing the downstream link > + * @port_id: unique hardware identifier for dport in decoder target list > + * @component_reg_phys: downstream port component registers > + * @port: reference to cxl_port that contains this downstream port > + * @list: node for a cxl_port's list of cxl_dport instances > + */ > +struct cxl_dport { > + struct device *dport; > + int port_id; > + resource_size_t component_reg_phys; > + struct cxl_port *port; > + struct list_head list; > +}; > + > struct cxl_port *to_cxl_port(struct device *dev); > struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > resource_size_t component_reg_phys, > struct cxl_port *parent_port); > > +int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id, > + resource_size_t component_reg_phys); > extern struct bus_type cxl_bus_type; > #endif /* __CXL_H__ */ >
On Tue, Jun 8, 2021 at 4:49 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Sat, 5 Jun 2021 23:05:17 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > In preparation for infrastructure that enumerates and configures the CXL > > decode mechanism of an upstream port to its downstream ports, add a > > representation for a CXL downstream port. > > > > On ACPI systems the top-most logical downstream ports in the hierarchy > > are the host bridges (ACPI0016 devices) that decode the memory windows > > described by the CXL Early Discovery Table Fixed Memory Window > > Structures (CEDT.CFMWS). > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > Documentation/ABI/testing/sysfs-bus-cxl | 13 ++++ > > drivers/cxl/acpi.c | 44 ++++++++++++++ > > drivers/cxl/core.c | 95 ++++++++++++++++++++++++++++++- > > drivers/cxl/cxl.h | 21 +++++++ > > 4 files changed, 169 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > > index fb996ced7629..0cb31b7ad17b 100644 > > --- a/Documentation/ABI/testing/sysfs-bus-cxl > > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > > @@ -35,3 +35,16 @@ Description: > > CXL component registers. The 'uport' symlink connects the CXL > > portX object to the device that published the CXL port > > capability. > > + > > +What: /sys/bus/cxl/devices/portX/dportY > > +Date: June, 2021 > > +KernelVersion: v5.14 > > +Contact: linux-cxl@vger.kernel.org > > +Description: > > + CXL port objects are enumerated from either a platform firmware > > + device (ACPI0017 and ACPI0016) or PCIe switch upstream port with > > + CXL component registers. The 'dportY' symlink identifies one or > > + more downstream ports that the upstream port may target in its > > + decode of CXL memory resources. The 'Y' integer reflects the > > + hardware port unique-id used in the hardware decoder target > > + list. > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > > index 556d25ab6966..0ae7464b603d 100644 > > --- a/drivers/cxl/acpi.c > > +++ b/drivers/cxl/acpi.c > > @@ -5,19 +5,61 @@ > > #include <linux/device.h> > > #include <linux/kernel.h> > > #include <linux/acpi.h> > > +#include <linux/pci.h> > > Not immediately seeing why this include is added in this patch. My neither, it belongs in the next patch. > > > #include "cxl.h" > > > > +static struct acpi_device *to_cxl_host_bridge(struct device *dev) > > +{ > > + struct acpi_device *adev = to_acpi_device(dev); > > + > > + if (strcmp(acpi_device_hid(adev), "ACPI0016") == 0) > > + return adev; > > + return NULL; > > +} > > + > > +static int add_host_bridge_dport(struct device *match, void *arg) > > +{ > > + int rc; > > + acpi_status status; > > + unsigned long long uid; > > + struct cxl_port *root_port = arg; > > + struct device *host = root_port->dev.parent; > > + struct acpi_device *bridge = to_cxl_host_bridge(match); > > + > > + if (!bridge) > > + return 0; > > + > > + status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL, > > + &uid); > > + if (status != AE_OK) { > > + dev_err(host, "unable to retrieve _UID of %s\n", > > + dev_name(match)); > > + return -ENODEV; > > + } > > + > > + rc = cxl_add_dport(root_port, match, uid, CXL_RESOURCE_NONE); > > + if (rc) { > > + dev_err(host, "failed to add downstream port: %s\n", > > + dev_name(match)); > > + return rc; > > + } > > + dev_dbg(host, "add dport%llu: %s\n", uid, dev_name(match)); > > + return 0; > > +} > > + > > static int cxl_acpi_probe(struct platform_device *pdev) > > { > > struct cxl_port *root_port; > > struct device *host = &pdev->dev; > > + struct acpi_device *adev = ACPI_COMPANION(host); > > > > root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); > > if (IS_ERR(root_port)) > > return PTR_ERR(root_port); > > dev_dbg(host, "add: %s\n", dev_name(&root_port->dev)); > > > > - return 0; > > + return bus_for_each_dev(adev->dev.bus, NULL, root_port, > > + add_host_bridge_dport); > > } > > > > static const struct acpi_device_id cxl_acpi_ids[] = { > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c > > index dbbb34618d7d..4418b30cce4f 100644 > > --- a/drivers/cxl/core.c > > +++ b/drivers/cxl/core.c > > @@ -33,10 +33,22 @@ static struct attribute_group cxl_base_attribute_group = { > > .attrs = cxl_base_attributes, > > }; > > > > +static void cxl_dport_release(struct cxl_dport *dport) > > +{ > > + put_device(dport->dport); > > + list_del(&dport->list); > This ordering isn't the reverse of what happens cxl_add_dport() > That would be > > list_del() > put_device() > kfree() > > If there is a strong reason for that I'd like to see a comment here. No strong reason, I'll reorder. > > > + kfree(dport); > > +} > > + > > static void cxl_port_release(struct device *dev) > > { > > struct cxl_port *port = to_cxl_port(dev); > > + struct cxl_dport *dport, *_d; > > > > + device_lock(dev); > > + list_for_each_entry_safe(dport, _d, &port->dports, list) > > + cxl_dport_release(dport); > > + device_unlock(dev); > > ida_free(&cxl_port_ida, port->id); > > kfree(port); > > } > > @@ -60,9 +72,22 @@ struct cxl_port *to_cxl_port(struct device *dev) > > return container_of(dev, struct cxl_port, dev); > > } > > > > -static void unregister_dev(void *dev) > > +static void unregister_port(void *_port) > > { > > - device_unregister(dev); > > + struct cxl_port *port = _port; > > + struct cxl_dport *dport; > > + > > + device_lock(&port->dev); > > + list_for_each_entry(dport, &port->dports, list) { > > + char link_name[CXL_TARGET_STRLEN]; > > + > > + if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", > > + dport->port_id) >= CXL_TARGET_STRLEN) > > + continue; > > + sysfs_remove_link(&port->dev.kobj, link_name); > > + } > > + device_unlock(&port->dev); > > + device_unregister(&port->dev); > > } > > > > static void cxl_unlink_uport(void *_port) > > @@ -113,6 +138,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport, > > > > port->uport = uport; > > port->component_reg_phys = component_reg_phys; > > + INIT_LIST_HEAD(&port->dports); > > > > device_initialize(dev); > > device_set_pm_not_required(dev); > > @@ -157,7 +183,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > > if (rc) > > goto err; > > > > - rc = devm_add_action_or_reset(host, unregister_dev, dev); > > + rc = devm_add_action_or_reset(host, unregister_port, port); > > if (rc) > > return ERR_PTR(rc); > > > > @@ -173,6 +199,69 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > > } > > EXPORT_SYMBOL_GPL(devm_cxl_add_port); > > > > +static int add_dport(struct cxl_port *port, struct cxl_dport *new) > > +{ > > + struct cxl_dport *dport, *dup = NULL; > > + > > + device_lock(&port->dev); > > + list_for_each_entry (dport, &port->dports, list) > > + if (new->port_id == dport->port_id) { > > + dup = dport; > > + goto err; > > + } > > + list_add_tail(&new->list, &port->dports); > > + device_unlock(&port->dev); > > + > > + return 0; > > + > > +err: > > + device_unlock(&port->dev); > > + dev_err(&port->dev, > > + "unable to add dport%d-%s non-unique port id (%s)\n", > > + new->port_id, dev_name(new->dport), dev_name(dup->dport)); > > As there is potential that you might end up with other errors in here long term, > why not move this to the point where the error is detected? > I think you are fine doing it under the mutex. Obviously indent will be a deeper > than ideal. Hmm, I'll take a look. Might add a dup finder helper to keep the indent under control. > > > + return -EEXIST; > > +} > > + > > +/* > > This is a bit inconsistent wrt to what functions get full kernel-doc. > My personal preference would be all the exported functions + any others > where it is particularly useful. I agree with the sentiment for globally exported symbols. In this case they are in the "CXL" module namespace and privately defined in drivers/cxl/ headers. That said, I did document devm_add_cxl_port(), so there's no good reason to skip the documentation on the other devm_cxl_add_* routines... will fix. > > + * Append downstream port data to a cxl_port, note that all allocations > > + * and links are undone by cxl_port deletion and release. > > + */ > > +int cxl_add_dport(struct cxl_port *port, struct device *dport_dev, int port_id, > > + resource_size_t component_reg_phys) > > +{ > > + char link_name[CXL_TARGET_STRLEN]; > > + struct cxl_dport *dport; > > + int rc; > > + > > + if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >= > > + CXL_TARGET_STRLEN) > > + return -EINVAL; > > + > > + dport = kzalloc(sizeof(*dport), GFP_KERNEL); > > + if (!dport) > > + return -ENOMEM; > > + > > + INIT_LIST_HEAD(&dport->list); > > + dport->dport = get_device(dport_dev); > > + dport->port_id = port_id; > > + dport->component_reg_phys = component_reg_phys; > > + dport->port = port; > > + > > + rc = add_dport(port, dport); > > + if (rc) > > If you get an error here, it's not been added to the list, but > in the cxl_dport_release() you remove it from the list. I think you > just want to put and free the device here. The delete is innocuous because of the INIT_LIST_HEAD() above. So the delete will end up doing the right thing and leaving the entry empty again, and that saves the need for custom code to handle that case.
On Tue, 8 Jun 2021 16:58:19 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > On Tue, Jun 8, 2021 at 4:49 AM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > On Sat, 5 Jun 2021 23:05:17 -0700 > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > In preparation for infrastructure that enumerates and configures the CXL > > > decode mechanism of an upstream port to its downstream ports, add a > > > representation for a CXL downstream port. > > > > > > On ACPI systems the top-most logical downstream ports in the hierarchy > > > are the host bridges (ACPI0016 devices) that decode the memory windows > > > described by the CXL Early Discovery Table Fixed Memory Window > > > Structures (CEDT.CFMWS). > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > --- > > > Documentation/ABI/testing/sysfs-bus-cxl | 13 ++++ > > > drivers/cxl/acpi.c | 44 ++++++++++++++ > > > drivers/cxl/core.c | 95 ++++++++++++++++++++++++++++++- > > > drivers/cxl/cxl.h | 21 +++++++ > > > 4 files changed, 169 insertions(+), 4 deletions(-) > > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > > > index fb996ced7629..0cb31b7ad17b 100644 > > > --- a/Documentation/ABI/testing/sysfs-bus-cxl > > > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > > > @@ -35,3 +35,16 @@ Description: > > > CXL component registers. The 'uport' symlink connects the CXL > > > portX object to the device that published the CXL port > > > capability. > > > + > > > +What: /sys/bus/cxl/devices/portX/dportY > > > +Date: June, 2021 > > > +KernelVersion: v5.14 > > > +Contact: linux-cxl@vger.kernel.org > > > +Description: > > > + CXL port objects are enumerated from either a platform firmware > > > + device (ACPI0017 and ACPI0016) or PCIe switch upstream port with > > > + CXL component registers. The 'dportY' symlink identifies one or > > > + more downstream ports that the upstream port may target in its > > > + decode of CXL memory resources. The 'Y' integer reflects the > > > + hardware port unique-id used in the hardware decoder target > > > + list. > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > > > index 556d25ab6966..0ae7464b603d 100644 > > > --- a/drivers/cxl/acpi.c > > > +++ b/drivers/cxl/acpi.c > > > @@ -5,19 +5,61 @@ > > > #include <linux/device.h> > > > #include <linux/kernel.h> > > > #include <linux/acpi.h> > > > +#include <linux/pci.h> > > > > Not immediately seeing why this include is added in this patch. > > My neither, it belongs in the next patch. > > > > > > #include "cxl.h" > > > > > > +static struct acpi_device *to_cxl_host_bridge(struct device *dev) > > > +{ > > > + struct acpi_device *adev = to_acpi_device(dev); > > > + > > > + if (strcmp(acpi_device_hid(adev), "ACPI0016") == 0) > > > + return adev; > > > + return NULL; > > > +} > > > + > > > +static int add_host_bridge_dport(struct device *match, void *arg) > > > +{ > > > + int rc; > > > + acpi_status status; > > > + unsigned long long uid; > > > + struct cxl_port *root_port = arg; > > > + struct device *host = root_port->dev.parent; > > > + struct acpi_device *bridge = to_cxl_host_bridge(match); > > > + > > > + if (!bridge) > > > + return 0; > > > + > > > + status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL, > > > + &uid); > > > + if (status != AE_OK) { > > > + dev_err(host, "unable to retrieve _UID of %s\n", > > > + dev_name(match)); > > > + return -ENODEV; > > > + } > > > + > > > + rc = cxl_add_dport(root_port, match, uid, CXL_RESOURCE_NONE); > > > + if (rc) { > > > + dev_err(host, "failed to add downstream port: %s\n", > > > + dev_name(match)); > > > + return rc; > > > + } > > > + dev_dbg(host, "add dport%llu: %s\n", uid, dev_name(match)); > > > + return 0; > > > +} > > > + > > > static int cxl_acpi_probe(struct platform_device *pdev) > > > { > > > struct cxl_port *root_port; > > > struct device *host = &pdev->dev; > > > + struct acpi_device *adev = ACPI_COMPANION(host); > > > > > > root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); > > > if (IS_ERR(root_port)) > > > return PTR_ERR(root_port); > > > dev_dbg(host, "add: %s\n", dev_name(&root_port->dev)); > > > > > > - return 0; > > > + return bus_for_each_dev(adev->dev.bus, NULL, root_port, > > > + add_host_bridge_dport); > > > } > > > > > > static const struct acpi_device_id cxl_acpi_ids[] = { > > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c > > > index dbbb34618d7d..4418b30cce4f 100644 > > > --- a/drivers/cxl/core.c > > > +++ b/drivers/cxl/core.c > > > @@ -33,10 +33,22 @@ static struct attribute_group cxl_base_attribute_group = { > > > .attrs = cxl_base_attributes, > > > }; > > > > > > +static void cxl_dport_release(struct cxl_dport *dport) > > > +{ > > > + put_device(dport->dport); > > > + list_del(&dport->list); > > This ordering isn't the reverse of what happens cxl_add_dport() > > That would be > > > > list_del() > > put_device() > > kfree() > > > > If there is a strong reason for that I'd like to see a comment here. > > No strong reason, I'll reorder. > > > > > > + kfree(dport); > > > +} > > > + > > > static void cxl_port_release(struct device *dev) > > > { > > > struct cxl_port *port = to_cxl_port(dev); > > > + struct cxl_dport *dport, *_d; > > > > > > + device_lock(dev); > > > + list_for_each_entry_safe(dport, _d, &port->dports, list) > > > + cxl_dport_release(dport); > > > + device_unlock(dev); > > > ida_free(&cxl_port_ida, port->id); > > > kfree(port); > > > } > > > @@ -60,9 +72,22 @@ struct cxl_port *to_cxl_port(struct device *dev) > > > return container_of(dev, struct cxl_port, dev); > > > } > > > > > > -static void unregister_dev(void *dev) > > > +static void unregister_port(void *_port) > > > { > > > - device_unregister(dev); > > > + struct cxl_port *port = _port; > > > + struct cxl_dport *dport; > > > + > > > + device_lock(&port->dev); > > > + list_for_each_entry(dport, &port->dports, list) { > > > + char link_name[CXL_TARGET_STRLEN]; > > > + > > > + if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", > > > + dport->port_id) >= CXL_TARGET_STRLEN) > > > + continue; > > > + sysfs_remove_link(&port->dev.kobj, link_name); > > > + } > > > + device_unlock(&port->dev); > > > + device_unregister(&port->dev); > > > } > > > > > > static void cxl_unlink_uport(void *_port) > > > @@ -113,6 +138,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport, > > > > > > port->uport = uport; > > > port->component_reg_phys = component_reg_phys; > > > + INIT_LIST_HEAD(&port->dports); > > > > > > device_initialize(dev); > > > device_set_pm_not_required(dev); > > > @@ -157,7 +183,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > > > if (rc) > > > goto err; > > > > > > - rc = devm_add_action_or_reset(host, unregister_dev, dev); > > > + rc = devm_add_action_or_reset(host, unregister_port, port); > > > if (rc) > > > return ERR_PTR(rc); > > > > > > @@ -173,6 +199,69 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > > > } > > > EXPORT_SYMBOL_GPL(devm_cxl_add_port); > > > > > > +static int add_dport(struct cxl_port *port, struct cxl_dport *new) > > > +{ > > > + struct cxl_dport *dport, *dup = NULL; > > > + > > > + device_lock(&port->dev); > > > + list_for_each_entry (dport, &port->dports, list) > > > + if (new->port_id == dport->port_id) { > > > + dup = dport; > > > + goto err; > > > + } > > > + list_add_tail(&new->list, &port->dports); > > > + device_unlock(&port->dev); > > > + > > > + return 0; > > > + > > > +err: > > > + device_unlock(&port->dev); > > > + dev_err(&port->dev, > > > + "unable to add dport%d-%s non-unique port id (%s)\n", > > > + new->port_id, dev_name(new->dport), dev_name(dup->dport)); > > > > As there is potential that you might end up with other errors in here long term, > > why not move this to the point where the error is detected? > > I think you are fine doing it under the mutex. Obviously indent will be a deeper > > than ideal. > > Hmm, I'll take a look. Might add a dup finder helper to keep the > indent under control. > > > > > > + return -EEXIST; > > > +} > > > + > > > +/* > > > > This is a bit inconsistent wrt to what functions get full kernel-doc. > > My personal preference would be all the exported functions + any others > > where it is particularly useful. > > I agree with the sentiment for globally exported symbols. In this case > they are in the "CXL" module namespace and privately defined in > drivers/cxl/ headers. That said, I did document devm_add_cxl_port(), > so there's no good reason to skip the documentation on the other > devm_cxl_add_* routines... will fix. Maybe we should consider using symbol namespaces for CXL? EXPORT_SYMBOL_NS_GPL() etc > > > > + * Append downstream port data to a cxl_port, note that all allocations > > > + * and links are undone by cxl_port deletion and release. > > > + */ > > > +int cxl_add_dport(struct cxl_port *port, struct device *dport_dev, int port_id, > > > + resource_size_t component_reg_phys) > > > +{ > > > + char link_name[CXL_TARGET_STRLEN]; > > > + struct cxl_dport *dport; > > > + int rc; > > > + > > > + if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >= > > > + CXL_TARGET_STRLEN) > > > + return -EINVAL; > > > + > > > + dport = kzalloc(sizeof(*dport), GFP_KERNEL); > > > + if (!dport) > > > + return -ENOMEM; > > > + > > > + INIT_LIST_HEAD(&dport->list); > > > + dport->dport = get_device(dport_dev); > > > + dport->port_id = port_id; > > > + dport->component_reg_phys = component_reg_phys; > > > + dport->port = port; > > > + > > > + rc = add_dport(port, dport); > > > + if (rc) > > > > If you get an error here, it's not been added to the list, but > > in the cxl_dport_release() you remove it from the list. I think you > > just want to put and free the device here. > > The delete is innocuous because of the INIT_LIST_HEAD() above. So the > delete will end up doing the right thing and leaving the entry empty > again, and that saves the need for custom code to handle that case. Ah fair enough. I'd missed that INIT. Not sure I'm keen on that approach as it's not in the 'obviously correct' category but it's your code to maintain so I'm not that fussed. Jonathan
On Wed, Jun 9, 2021 at 4:28 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > > This is a bit inconsistent wrt to what functions get full kernel-doc. > > > My personal preference would be all the exported functions + any others > > > where it is particularly useful. > > > > I agree with the sentiment for globally exported symbols. In this case > > they are in the "CXL" module namespace and privately defined in > > drivers/cxl/ headers. That said, I did document devm_add_cxl_port(), > > so there's no good reason to skip the documentation on the other > > devm_cxl_add_* routines... will fix. > > Maybe we should consider using symbol namespaces for CXL? > EXPORT_SYMBOL_NS_GPL() etc In fact, we already are using that, it's just implicit from the Makefile with this line: ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL > > > > + * Append downstream port data to a cxl_port, note that all allocations > > > > + * and links are undone by cxl_port deletion and release. > > > > + */ > > > > +int cxl_add_dport(struct cxl_port *port, struct device *dport_dev, int port_id, > > > > + resource_size_t component_reg_phys) > > > > +{ > > > > + char link_name[CXL_TARGET_STRLEN]; > > > > + struct cxl_dport *dport; > > > > + int rc; > > > > + > > > > + if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >= > > > > + CXL_TARGET_STRLEN) > > > > + return -EINVAL; > > > > + > > > > + dport = kzalloc(sizeof(*dport), GFP_KERNEL); > > > > + if (!dport) > > > > + return -ENOMEM; > > > > + > > > > + INIT_LIST_HEAD(&dport->list); > > > > + dport->dport = get_device(dport_dev); > > > > + dport->port_id = port_id; > > > > + dport->component_reg_phys = component_reg_phys; > > > > + dport->port = port; > > > > + > > > > + rc = add_dport(port, dport); > > > > + if (rc) > > > > > > If you get an error here, it's not been added to the list, but > > > in the cxl_dport_release() you remove it from the list. I think you > > > just want to put and free the device here. > > > > The delete is innocuous because of the INIT_LIST_HEAD() above. So the > > delete will end up doing the right thing and leaving the entry empty > > again, and that saves the need for custom code to handle that case. > > Ah fair enough. I'd missed that INIT. Not sure I'm keen on that > approach as it's not in the 'obviously correct' category but it's your > code to maintain so I'm not that fussed. I think it's in a similar spirit to devm and the lack of ida_destroy(), try not to write unwind code if at all possible...
diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl index fb996ced7629..0cb31b7ad17b 100644 --- a/Documentation/ABI/testing/sysfs-bus-cxl +++ b/Documentation/ABI/testing/sysfs-bus-cxl @@ -35,3 +35,16 @@ Description: CXL component registers. The 'uport' symlink connects the CXL portX object to the device that published the CXL port capability. + +What: /sys/bus/cxl/devices/portX/dportY +Date: June, 2021 +KernelVersion: v5.14 +Contact: linux-cxl@vger.kernel.org +Description: + CXL port objects are enumerated from either a platform firmware + device (ACPI0017 and ACPI0016) or PCIe switch upstream port with + CXL component registers. The 'dportY' symlink identifies one or + more downstream ports that the upstream port may target in its + decode of CXL memory resources. The 'Y' integer reflects the + hardware port unique-id used in the hardware decoder target + list. diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index 556d25ab6966..0ae7464b603d 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -5,19 +5,61 @@ #include <linux/device.h> #include <linux/kernel.h> #include <linux/acpi.h> +#include <linux/pci.h> #include "cxl.h" +static struct acpi_device *to_cxl_host_bridge(struct device *dev) +{ + struct acpi_device *adev = to_acpi_device(dev); + + if (strcmp(acpi_device_hid(adev), "ACPI0016") == 0) + return adev; + return NULL; +} + +static int add_host_bridge_dport(struct device *match, void *arg) +{ + int rc; + acpi_status status; + unsigned long long uid; + struct cxl_port *root_port = arg; + struct device *host = root_port->dev.parent; + struct acpi_device *bridge = to_cxl_host_bridge(match); + + if (!bridge) + return 0; + + status = acpi_evaluate_integer(bridge->handle, METHOD_NAME__UID, NULL, + &uid); + if (status != AE_OK) { + dev_err(host, "unable to retrieve _UID of %s\n", + dev_name(match)); + return -ENODEV; + } + + rc = cxl_add_dport(root_port, match, uid, CXL_RESOURCE_NONE); + if (rc) { + dev_err(host, "failed to add downstream port: %s\n", + dev_name(match)); + return rc; + } + dev_dbg(host, "add dport%llu: %s\n", uid, dev_name(match)); + return 0; +} + static int cxl_acpi_probe(struct platform_device *pdev) { struct cxl_port *root_port; struct device *host = &pdev->dev; + struct acpi_device *adev = ACPI_COMPANION(host); root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); if (IS_ERR(root_port)) return PTR_ERR(root_port); dev_dbg(host, "add: %s\n", dev_name(&root_port->dev)); - return 0; + return bus_for_each_dev(adev->dev.bus, NULL, root_port, + add_host_bridge_dport); } static const struct acpi_device_id cxl_acpi_ids[] = { diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c index dbbb34618d7d..4418b30cce4f 100644 --- a/drivers/cxl/core.c +++ b/drivers/cxl/core.c @@ -33,10 +33,22 @@ static struct attribute_group cxl_base_attribute_group = { .attrs = cxl_base_attributes, }; +static void cxl_dport_release(struct cxl_dport *dport) +{ + put_device(dport->dport); + list_del(&dport->list); + kfree(dport); +} + static void cxl_port_release(struct device *dev) { struct cxl_port *port = to_cxl_port(dev); + struct cxl_dport *dport, *_d; + device_lock(dev); + list_for_each_entry_safe(dport, _d, &port->dports, list) + cxl_dport_release(dport); + device_unlock(dev); ida_free(&cxl_port_ida, port->id); kfree(port); } @@ -60,9 +72,22 @@ struct cxl_port *to_cxl_port(struct device *dev) return container_of(dev, struct cxl_port, dev); } -static void unregister_dev(void *dev) +static void unregister_port(void *_port) { - device_unregister(dev); + struct cxl_port *port = _port; + struct cxl_dport *dport; + + device_lock(&port->dev); + list_for_each_entry(dport, &port->dports, list) { + char link_name[CXL_TARGET_STRLEN]; + + if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", + dport->port_id) >= CXL_TARGET_STRLEN) + continue; + sysfs_remove_link(&port->dev.kobj, link_name); + } + device_unlock(&port->dev); + device_unregister(&port->dev); } static void cxl_unlink_uport(void *_port) @@ -113,6 +138,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport, port->uport = uport; port->component_reg_phys = component_reg_phys; + INIT_LIST_HEAD(&port->dports); device_initialize(dev); device_set_pm_not_required(dev); @@ -157,7 +183,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, if (rc) goto err; - rc = devm_add_action_or_reset(host, unregister_dev, dev); + rc = devm_add_action_or_reset(host, unregister_port, port); if (rc) return ERR_PTR(rc); @@ -173,6 +199,69 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, } EXPORT_SYMBOL_GPL(devm_cxl_add_port); +static int add_dport(struct cxl_port *port, struct cxl_dport *new) +{ + struct cxl_dport *dport, *dup = NULL; + + device_lock(&port->dev); + list_for_each_entry (dport, &port->dports, list) + if (new->port_id == dport->port_id) { + dup = dport; + goto err; + } + list_add_tail(&new->list, &port->dports); + device_unlock(&port->dev); + + return 0; + +err: + device_unlock(&port->dev); + dev_err(&port->dev, + "unable to add dport%d-%s non-unique port id (%s)\n", + new->port_id, dev_name(new->dport), dev_name(dup->dport)); + return -EEXIST; +} + +/* + * Append downstream port data to a cxl_port, note that all allocations + * and links are undone by cxl_port deletion and release. + */ +int cxl_add_dport(struct cxl_port *port, struct device *dport_dev, int port_id, + resource_size_t component_reg_phys) +{ + char link_name[CXL_TARGET_STRLEN]; + struct cxl_dport *dport; + int rc; + + if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >= + CXL_TARGET_STRLEN) + return -EINVAL; + + dport = kzalloc(sizeof(*dport), GFP_KERNEL); + if (!dport) + return -ENOMEM; + + INIT_LIST_HEAD(&dport->list); + dport->dport = get_device(dport_dev); + dport->port_id = port_id; + dport->component_reg_phys = component_reg_phys; + dport->port = port; + + rc = add_dport(port, dport); + if (rc) + goto err; + + rc = sysfs_create_link(&port->dev.kobj, &dport_dev->kobj, link_name); + if (rc) + goto err; + + return 0; +err: + cxl_dport_release(dport); + return rc; +} +EXPORT_SYMBOL_GPL(cxl_add_dport); + /** * cxl_probe_component_regs() - Detect CXL Component register blocks * @dev: Host device of the @base mapping diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 46c81165c210..0b89fcfe728a 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -146,6 +146,7 @@ int cxl_map_device_regs(struct pci_dev *pdev, struct cxl_register_map *map); #define CXL_RESOURCE_NONE ((resource_size_t) -1) +#define CXL_TARGET_STRLEN 20 /** * struct cxl_port - logical collection of upstream port devices and @@ -154,19 +155,39 @@ int cxl_map_device_regs(struct pci_dev *pdev, * @dev: this port's device * @uport: PCI or platform device implementing the upstream port capability * @id: id for port device-name + * @dports: cxl_dport instances referenced by decoders * @component_regs_phys: component register capability base address (optional) */ struct cxl_port { struct device dev; struct device *uport; int id; + struct list_head dports; resource_size_t component_reg_phys; }; +/** + * struct cxl_dport - CXL downstream port + * @dport: PCI bridge or firmware device representing the downstream link + * @port_id: unique hardware identifier for dport in decoder target list + * @component_reg_phys: downstream port component registers + * @port: reference to cxl_port that contains this downstream port + * @list: node for a cxl_port's list of cxl_dport instances + */ +struct cxl_dport { + struct device *dport; + int port_id; + resource_size_t component_reg_phys; + struct cxl_port *port; + struct list_head list; +}; + struct cxl_port *to_cxl_port(struct device *dev); struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, resource_size_t component_reg_phys, struct cxl_port *parent_port); +int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id, + resource_size_t component_reg_phys); extern struct bus_type cxl_bus_type; #endif /* __CXL_H__ */
In preparation for infrastructure that enumerates and configures the CXL decode mechanism of an upstream port to its downstream ports, add a representation for a CXL downstream port. On ACPI systems the top-most logical downstream ports in the hierarchy are the host bridges (ACPI0016 devices) that decode the memory windows described by the CXL Early Discovery Table Fixed Memory Window Structures (CEDT.CFMWS). Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- Documentation/ABI/testing/sysfs-bus-cxl | 13 ++++ drivers/cxl/acpi.c | 44 ++++++++++++++ drivers/cxl/core.c | 95 ++++++++++++++++++++++++++++++- drivers/cxl/cxl.h | 21 +++++++ 4 files changed, 169 insertions(+), 4 deletions(-)