Jonathan Cameron wrote: > On Fri, 10 May 2024 16:37:10 +0900 > "Kobayashi,Daisuke" <kobayashi.da-06@xxxxxxxxxxx> 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. > > The lspci support raced with this series and has landed. > > https://github.com/pciutils/pciutils/commit/49efa87fcce4f7d5b351238668ae > 1d4491802b88 > > A few follow up comments on suggestion to cache the offset, not the register > value > (which might change over time) > > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@xxxxxxxxxxx> > > --- > > drivers/cxl/pci.c | 101 > ++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 101 insertions(+) > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index 2ff361e756d6..c10797adde2c 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -786,6 +786,106 @@ static int cxl_event_config(struct pci_host_bridge > *host_bridge, > > return 0; > > } > > > > +static ssize_t rcd_link_cap_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + 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; > > + > > + return sysfs_emit(buf, "%x\n", dport->rcrb.rcd_lnkcap); > > If you follow the suggestion of caching the offset, not the register > content then, these are all very similar. Define one common > function which takes a u8 offset and call that from each of the > functions. > > If you stick to separate caches (and ensure they are up to date), > then you can add a utility function to take the dev here and > get you to the rcrb structure. Then you can call that and only > have the final line in each instances of this code. > Thank you for your comment. To confirm my understanding of the proposed implementation approach, I would like to share my interpretation: - The values stored in `cxl_rcrb_info` should be the offsets of the registers, rather than the register values themselves. - Similar to aer, the values should be read from the memory-mapped region using the offset when they are needed. - To achieve this, the offsets (like `cxl_rcrb_pcie_caps`) will be stored in `cxl_rcrb_info` during initialization, and the memory-mapped region will be accessed within the `rcd_*_show()` functions where the values are actually used. - Accessing the memory-mapped region only requires the `struct device *dev`, so a common utility function will be created and used for this purpose. If the cost of accessing the memory-mapped region at runtime is not high, especially considering the avoidance of the walk to find the offset, then I believe it is more reasonable to access the memory-mapped region at runtime. Could you let me know if there are any errors in my understanding. If there are no issues, I will update the implementation with this approach and submit a v8 patch (hopefully tomorrow). > > +} > > +static DEVICE_ATTR_RO(rcd_link_cap); > > + > > +static ssize_t rcd_link_ctrl_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + 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; > > + > > + return sysfs_emit(buf, "%x\n", dport->rcrb.rcd_lnkctrl); > > +} > > +static DEVICE_ATTR_RO(rcd_link_ctrl); > > + > > +static ssize_t rcd_link_status_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + 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; > > + > > + return sysfs_emit(buf, "%x\n", dport->rcrb.rcd_lnkstatus); > > +} > > +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); > > @@ -969,6 +1069,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, > > },
