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

[Dan Williams <dan.j.williams@intel.com>]

Bjorn Helgaas wrote:
> On Tue, Apr 09, 2024 at 04:35:28PM +0900, 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.
> 
> Idle thought: PCIe does define RCRB, even pre-CXL.  Maybe the PCI core
> should be enhanced to comprehend RCRB directly?

It depends on if this slow drip of features continues, and it seems that
PCIe base RCRB is scoped to a single device/port whereas CXL appears to
extend it to merge the endpoint config space and root-port config space
into a double-sized RCRB area.

I.e. there will continue to be CXL specifics involved.

Also, this is a one-generation-quirk as CXL 2.0+ hosts drop this awkward
RCRB arrangement.

> > +static ssize_t rcd_link_status_show(struct device *dev,
> > +				   struct device_attribute *attr, char *buf)
> > +{
> > +	struct cxl_port *port;
> > +	struct cxl_dport *dport;
> > +	struct device *parent = dev->parent;
> > +	struct pci_dev *parent_pdev = to_pci_dev(parent);
> > +
> > +	port = cxl_pci_find_port(parent_pdev, &dport);
> > +	if (!port)
> > +		return -EINVAL;
> > +
> > +	return sysfs_emit(buf, "%x\n", dport->rcrb.rcd_lnkstatus);
> 
> Is it really what you want to capture PCI_EXP_LNKSTA once at
> enumeration-time and expose that static value forever?  I assume
> status bits can change over time, so I would naively expect that you
> want the *current* value, not just a value from the distant past.

I expect this should copy what is done for aer_cap where that single
RCRB capability block is cached for future access. That said many of the
link status change events would also cause the device to be rescanned
and that value is refreshed once per driver bind event.