Re: [PATCH v7 2/2] cxl/pci: Add sysfs attribute for CXL 1.1 device link status

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 5 Jun 2024 07:21:44 +0000
"Daisuke Kobayashi (Fujitsu)" <kobayashi.da-06@fujitsu.com> wrote:

> Jonathan Cameron wrote:
> > On Fri, 10 May 2024 16:37:10 +0900
> > "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> 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@huawei.com>
> > > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> > > ---
> > >  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,
> > >  	},  
>