> -----Original Message-----
> From: Dan Williams <dan.j.williams@xxxxxxxxx>
> Sent: Wednesday, January 17, 2024 6:29 AM
> To: Kobayashi, Daisuke/小林 大介 <kobayashi.da-06@xxxxxxxxxxx>;
> 'Jonathan Cameron' <Jonathan.Cameron@xxxxxxxxxx>; Dan Williams
> <dan.j.williams@xxxxxxxxx>
> Cc: linux-pci@xxxxxxxxxxxxxxx; linux-cxl@xxxxxxxxxxxxxxx; Gotou, Yasunori/五島
> 康文 <y-goto@xxxxxxxxxxx>
> Subject: RE: [RFC PATCH 0/3] lspci: Display cxl1.1 device link status
>
> 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:
>
Thank you, it seems that my understanding was incorrect.
I will try to consider the implementation by dividing it into parts:
the hook on the pci driver, the RCRB access in the cxl driver,
and the sysfs reading in lspci.
> 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.
