On Wed, 5 Jun 2024 07:21:44 +0000 "Daisuke Kobayashi (Fujitsu)" <kobayashi.da-06@xxxxxxxxxxx> wrote: > 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). Your description matches my intent. Looking forwards to v8. Jonathan > > > > +} > > > +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, > > > }, >
