| Message ID | 20240716042540.89639-3-kobayashi.da-06@fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Export cxl1.1 device link status register value to pci device sysfs. | expand |
Kobayashi,Daisuke wrote: > Add sysfs attribute for CXL 1.1 device link status to the cxl pci device. > > In CXL1.1, the link status of the device is included in the RCRB mapped to > the memory mapped register area. Critically, that arrangement makes the > link status and control registers invisible to existing PCI user tooling. > > Export those registers via sysfs with the expectation that PCI user > tooling will alternatively look for these sysfs files when attempting to > access to these CXL 1.1 endpoints registers. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> > --- > drivers/cxl/pci.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 81 insertions(+) > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 5e7e66f28420..c149c04b029e 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -795,6 +795,86 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge, > return 0; > } > > +static ssize_t rcd_pcie_cap_emit(struct device *dev, u16 offset, char *buf, size_t width) > +{ > + struct cxl_dev_state *cxlds = dev_get_drvdata(dev); > + struct cxl_memdev *cxlmd = cxlds->cxlmd; > + struct device *endpoint_parent; > + struct cxl_dport *dport; > + struct cxl_port *port; > + > + port = cxl_mem_find_port(cxlmd, &dport); In order to avoid a memory leak this needs to be declared as: struct cxl_port *port __free(put_cxl_port) = cxl_mem_find_port(cxlmd, &dport)); > + if (!port) > + return -EINVAL; This return code should be ENXIO to represent that the memdev is disconnected from a CXL port toplogy that connect to the platform root. > + endpoint_parent = port->uport_dev; The @endpoint_parent name only makes sense in cxl_mem_probe() where the device upstream of the endpoint can either be a CXL downstream switch port, a downstream port of a CXL hostbridge, or the CXL root object directly. I would rename "port"=>"root" and "endpoint_parent"=>"root_dev", because this is a known RCD topology where the parent of a root-complex-integrated-endpoint is the ACPI0017 device. > + if (!endpoint_parent) > + return -ENXIO; @endpoint_parent is never NULL, the uport_dev field is always populated in @port instances near where they are allocated so can't be NULL here. > + guard(device)(endpoint_parent); > + if (!endpoint_parent->driver) > + return -ENXIO; > + > + if (dport->regs.rcd_pcie_cap == NULL) > + return -EINVAL; This should never happen because it is mandatory for this rcd_pcie_cap to be populated for the driver to load. So this check can be deleted. > + > + switch (width) { > + case sizeof(u16): That's an odd way to "2", just use "2" and "4" for the case statements. > + return sysfs_emit(buf, "%x\n", > + readw(dport->regs.rcd_pcie_cap + offset)); > + case sizeof(u32): > + return sysfs_emit(buf, "%x\n", > + readl(dport->regs.rcd_pcie_cap + offset)); Make those format strings "%#x\n" so that it is unambiguous that these are hexadecimal values. > + default: > + return -EINVAL; > + } > +} [..] Rest of the patch looks good to me.
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 5e7e66f28420..c149c04b029e 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -795,6 +795,86 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge, return 0; } +static ssize_t rcd_pcie_cap_emit(struct device *dev, u16 offset, char *buf, size_t width) +{ + struct cxl_dev_state *cxlds = dev_get_drvdata(dev); + struct cxl_memdev *cxlmd = cxlds->cxlmd; + struct device *endpoint_parent; + struct cxl_dport *dport; + struct cxl_port *port; + + port = cxl_mem_find_port(cxlmd, &dport); + if (!port) + return -EINVAL; + + endpoint_parent = port->uport_dev; + if (!endpoint_parent) + return -ENXIO; + + guard(device)(endpoint_parent); + if (!endpoint_parent->driver) + return -ENXIO; + + if (dport->regs.rcd_pcie_cap == NULL) + return -EINVAL; + + switch (width) { + case sizeof(u16): + return sysfs_emit(buf, "%x\n", + readw(dport->regs.rcd_pcie_cap + offset)); + case sizeof(u32): + return sysfs_emit(buf, "%x\n", + readl(dport->regs.rcd_pcie_cap + offset)); + default: + return -EINVAL; + } +} + +static ssize_t rcd_link_cap_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return rcd_pcie_cap_emit(dev, PCI_EXP_LNKCAP, buf, sizeof(u32)); +} +static DEVICE_ATTR_RO(rcd_link_cap); + +static ssize_t rcd_link_ctrl_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return rcd_pcie_cap_emit(dev, PCI_EXP_LNKCTL, buf, sizeof(u16)); +} +static DEVICE_ATTR_RO(rcd_link_ctrl); + +static ssize_t rcd_link_status_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return rcd_pcie_cap_emit(dev, PCI_EXP_LNKSTA, buf, sizeof(u16)); +} +static DEVICE_ATTR_RO(rcd_link_status); + +static struct attribute *cxl_rcd_attrs[] = { + &dev_attr_rcd_link_cap.attr, + &dev_attr_rcd_link_ctrl.attr, + &dev_attr_rcd_link_status.attr, + NULL +}; + +static umode_t cxl_rcd_visible(struct kobject *kobj, struct attribute *a, int n) +{ + struct device *dev = kobj_to_dev(kobj); + struct pci_dev *pdev = to_pci_dev(dev); + + if (is_cxl_restricted(pdev)) + return a->mode; + + return 0; +} + +static struct attribute_group cxl_rcd_group = { + .attrs = cxl_rcd_attrs, + .is_visible = cxl_rcd_visible, +}; +__ATTRIBUTE_GROUPS(cxl_rcd); + static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus); @@ -978,6 +1058,7 @@ static struct pci_driver cxl_pci_driver = { .id_table = cxl_mem_pci_tbl, .probe = cxl_pci_probe, .err_handler = &cxl_error_handlers, + .dev_groups = cxl_rcd_groups, .driver = { .probe_type = PROBE_PREFER_ASYNCHRONOUS, },