Daisuke Kobayashi (Fujitsu) wrote:
> > > I am wondering about an approach like below is sufficient for lspci.
> > >
> > > The idea here is that cxl_pci (or other PCI driver for Type-2 RCDs) can
> > > opt-in to publishing these hidden registers.
> > >
> > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > index 4fd1f207c84e..ee63dff63b68 100644
> > > --- a/drivers/cxl/pci.c
> > > +++ b/drivers/cxl/pci.c
> > > @@ -960,6 +960,19 @@ static const struct pci_error_handlers
> > cxl_error_handlers = {
> > > .cor_error_detected = cxl_cor_error_detected,
> > > };
> > >
> > > +static struct attribute *cxl_rcd_attrs[] = {
> > > + &dev_attr_rcd_lnkcp.attr,
> > > + &dev_attr_rcd_lnkctl.attr,
> > > + NULL
> > > +};
> > > +
> > > +static struct attribute_group cxl_rcd_group = {
> > > + .attrs = cxl_rcd_attrs,
> > > + .is_visible = cxl_rcd_visible,
> > > +};
> > > +
> > > +__ATTRIBUTE_GROUPS(cxl_pci);
> > > +
> > > static struct pci_driver cxl_pci_driver = {
> > > .name = KBUILD_MODNAME,
> > > .id_table = cxl_mem_pci_tbl,
> > > @@ -967,6 +980,7 @@ static struct pci_driver cxl_pci_driver = {
> > > .err_handler = &cxl_error_handlers,
> > > .driver = {
> > > .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > > + .dev_groups = cxl_rcd_groups,
> > > },
> > > };
> > >
> > >
> > > However, the problem I believe is this will end up with:
> > >
> > > /sys/bus/pci/devices/$pdev/rcd_lnkcap
> > > /sys/bus/pci/devices/$pdev/rcd_lnkctl
> > >
> > > ...with valid values, but attributes like:
> > >
> > > /sys/bus/pci/devices/$pdev/current_link_speed
> > >
> > > ...returning -EINVAL.
> > >
> > > So I think the options are:
> > >
> > > 1/ Keep the status quo of RCRB knowledge only lives in drivers/cxl/ and
> > > piecemeal enable specific lspci needs with RCD-specific attributes
> >
> > This one gets my vote.
>
> Thank you for your feedback.
> Like Dan, I also believe that implementing this feature in the kernel may
> not be appropriate, as it is specifically needed for CXL1.1 devices.
> Therefore, I understand that it would be better to implement
> the link status of CXL1.1 devices directly in lspci.
> Please tell me if my understanding is wrong.
The proposal is to do a hybrid approach. The drivers/cxl/ subsystem
already handles RCRB register access internally, so it can go further
and expose a couple attributes ("rcd_lnkcap" and "rcd_lnkctl") that
lspci can go read. In other words, "/dev/mem" is not a reliable way to
access the RCRB, and it is too much work to make the existing sysfs
config-space access ABI understand the RCRB layout since that
complication would only be useful for one hardware generation.
An additional idea here is to allow for the CXL subsystem to takeover
publishing PCIe attributes like "current_link_speed", that are currently
broken by the RCRB configuration, with a change like this:
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 2321fdfefd7d..982bbec721fd 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1613,7 +1613,7 @@ static umode_t pcie_dev_attrs_are_visible(struct kobject *kobj,
struct device *dev = kobj_to_dev(kobj);
struct pci_dev *pdev = to_pci_dev(dev);
- if (pci_is_pcie(pdev))
+ if (pci_is_pcie(pdev) && !is_cxl_rcd(pdev))
return a->mode;
return 0;
...then the CXL subsystem can produce its own attributes with the same
name, but backed by the RCRB lookup mechanism.
