Dan Williams wrote:
> Kobayashi,Daisuke wrote:
> > This patch implements a process to output the link status information
> > of the CXL1.1 device to sysfs. The values of the registers related to
> > the link status are outputted into three separate files.
> >
> > In CXL1.1, the link status of the device is included in the RCRB
> > mapped to the memory mapped register area. This function accesses the
> > address where the device's RCRB is mapped.
>
> Per the comments on the cover letter I would rewrite this as:
>
> ---
> 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 these
> registers on CXL 1.1 endpoints.
> ---
>
This message will be updated in the next patch.
Thank you for your helpful feedback.
> > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@xxxxxxxxxxx>
> > ---
> > drivers/cxl/pci.c | 193
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 193 insertions(+)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index
> > 4fd1f207c84e..8f66f80a7bdc 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -781,6 +781,195 @@ static int cxl_event_config(struct pci_host_bridge
> *host_bridge,
> > return 0;
> > }
> >
> > +static u8 cxl_rcrb_get_pcie_cap_offset(void __iomem *addr){
> > + u8 offset;
> > + u32 cap_hdr;
> > +
> > + offset = readb(addr + PCI_CAPABILITY_LIST);
> > + cap_hdr = readl(addr + offset);
> > + while ((cap_hdr & 0x000000ff) != PCI_CAP_ID_EXP) {
> > + offset = (cap_hdr >> 8) & 0x000000ff;
> > + if (offset == 0) // End of capability list
> > + return 0;
> > + cap_hdr = readl(addr + offset);
> > + }
> > + return offset;
>
> The location is static, so there should be no need to lookup the location every
> time the sysfs attribute is accessed. I also think the values are static unless the
> link is reset. So my expectation is that these register values can just be read
> once and cached.
>
> Likely the best place to do this is inside __rcrb_to_component(). That routine
> already has the RCRB mapped and can be refactored to collect the the link
> status registers. Something like, rename __rcrb_to_component() to
> __rcrb_to_regs() and then have it fill in an updated cxl_rcrb_info():
>
Add processing to__rcrb_to_component() to change the implementation
to cache these values. As you say, I think these values are static,
so it's not efficient to access the RCRB every time you access the sysfs attribute.
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index
> 534e25e2f0a4..16c7472877b7 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -651,7 +651,12 @@ cxl_find_dport_by_dev(struct cxl_port *port, const
> struct device *dport_dev)
>
> struct cxl_rcrb_info {
> resource_size_t base;
> + resource_size_t component_reg;
> + resource_size_t rcd_component_reg;
> u16 aer_cap;
> + u16 rcd_lnkctrl;
> + u16 rcd_lnkstatus;
> + u32 rcd_lnkcap;
> };
>
> /**
>
> > +
> > +}
> > +
> > +static u32 cxl_rcrb_to_linkcap(struct device *dev, resource_size_t
> > +rcrb) {
> > + void __iomem *addr;
> > + u8 offset;
> > + u32 linkcap;
> > +
> > + if (WARN_ON_ONCE(rcrb == CXL_RESOURCE_NONE))
> > + return 0;
>
> Why is this a WARN_ON_ONCE()? In other words the caller should know ahead
> of time whether it has a valid RCRB base or not.
>
> ...oh, I see this is copying cxl_rcrb_to_aer(). I think that
> WARN_ON_ONCE() in that function is bogus as well.
>
>
Yes, as you mentioned, I have copied cxl_rcrb_to_aer().
However, it seems to be an improper implementation.
In the next patch, I will modify the implementation to cache the value,
and consequently, this part of the code will be removed.
> > + if (!request_mem_region(rcrb, SZ_4K, dev_name(dev)))
> > + return 0;
>
> This is awkward because it may collide with usages of the RCRB, so that is
> another reason to cache the values.
>
> > +
> > + addr = ioremap(rcrb, SZ_4K);
> > + if (!addr)
> > + goto out;
> > +
> > + offset = cxl_rcrb_get_pcie_cap_offset(addr);
> > + if (offset)
> > + dev_dbg(dev, "found PCIe capability (0x%x)\n", offset);
> > + else
> > + goto out;
> > +
> > + linkcap = readl(addr + offset + PCI_EXP_LNKCAP);
> > + iounmap(addr);
> > +out:
> > + release_mem_region(rcrb, SZ_4K);
> > +
> > + return linkcap;
> > +}
> > +
> > +static ssize_t rcd_link_cap_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);
> > + u32 linkcap;
> > +
> > + port = cxl_pci_find_port(parent_pdev, &dport);
> > + if (!port)
> > + return -EINVAL;
> > +
> > + linkcap = cxl_rcrb_to_linkcap(dev, dport->rcrb.base + SZ_4K);
> > + return sysfs_emit(buf, "%x\n", linkcap);
>
> This and the other ones should be using "%#x\n" so that the format of the
> number base is included.
>
I will fix them. Thank you.
> > +}
> > +static DEVICE_ATTR_RO(rcd_link_cap);
> > +
> > +static u16 cxl_rcrb_to_linkctr(struct device *dev, resource_size_t
> > +rcrb) {
> > + void __iomem *addr;
> > + u8 offset;
> > + u16 linkctrl;
> > +
> > + if (WARN_ON_ONCE(rcrb == CXL_RESOURCE_NONE))
> > + return 0;
> > +
> > + if (!request_mem_region(rcrb, SZ_4K, dev_name(dev)))
> > + return 0;
>
> ...the other benefit of centralizing this code is that we do not end up with
> multiple copies of similar, but slightly different code.
>
Are you saying that caching values simplifies the show function?
Then I think you're right. I will change that the value should be cached
in the same way as the component register.
> > +
> > + addr = ioremap(rcrb, SZ_4K);
> > + if (!addr)
> > + goto out;
> > +
> > + offset = cxl_rcrb_get_pcie_cap_offset(addr);
> > + if (offset)
> > + dev_dbg(dev, "found PCIe capability (0x%x)\n", offset);
> > + else
> > + goto out;
> > +
> > + linkctrl = readw(addr + offset + PCI_EXP_LNKCTL);
> > + iounmap(addr);
> > +out:
> > + release_mem_region(rcrb, SZ_4K);
> > +
> > + return linkctrl;
> > +}
> > +
> > +static ssize_t rcd_link_ctrl_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);
> > + u16 linkctrl;
> > +
> > + port = cxl_pci_find_port(parent_pdev, &dport);
> > + if (!port)
> > + return -EINVAL;
> > +
> > +
> > + linkctrl = cxl_rcrb_to_linkctr(dev, dport->rcrb.base + SZ_4K);
> > +
> > + return sysfs_emit(buf, "%x\n", linkctrl); } static
> > +DEVICE_ATTR_RO(rcd_link_ctrl);
> > +
> > +static u16 cxl_rcrb_to_linkstatus(struct device *dev, resource_size_t
> > +rcrb) {
> > + void __iomem *addr;
> > + u8 offset;
> > + u16 linksta;
> > +
> > + if (WARN_ON_ONCE(rcrb == CXL_RESOURCE_NONE))
> > + return 0;
> > +
> > + if (!request_mem_region(rcrb, SZ_4K, dev_name(dev)))
> > + return 0;
> > +
> > + addr = ioremap(rcrb, SZ_4K);
> > + if (!addr)
> > + goto out;
> > +
> > + offset = cxl_rcrb_get_pcie_cap_offset(addr);
> > + if (offset)
> > + dev_dbg(dev, "found PCIe capability (0x%x)\n", offset);
> > + else
> > + goto out;
> > +
> > + linksta = readw(addr + offset + PCI_EXP_LNKSTA);
> > + iounmap(addr);
> > +out:
> > + release_mem_region(rcrb, SZ_4K);
> > +
> > + return linksta;
> > +}
> > +
> > +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);
> > + u16 linkstatus;
> > +
> > + port = cxl_pci_find_port(parent_pdev, &dport);
> > + if (!port)
> > + return -EINVAL;
> > +
> > + linkstatus = cxl_rcrb_to_linkstatus(dev, dport->rcrb.base + SZ_4K);
> > +
> > + return sysfs_emit(buf, "%x\n", linkstatus); } 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); @@ -806,6 +995,9 @@ static int
> cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > if (IS_ERR(mds))
> > return PTR_ERR(mds);
> > cxlds = &mds->cxlds;
> > + device_create_file(&pdev->dev, &dev_attr_rcd_link_cap);
> > + device_create_file(&pdev->dev, &dev_attr_rcd_link_ctrl);
> > + device_create_file(&pdev->dev, &dev_attr_rcd_link_status);
>
> No need to manually call device_create_file() when the attribute group is
> already registered below. I am surprised you did not get duplicate sysfs file
> warnings when registering these files twice.
>
Thank you for pointing it out. Remove these calls.
> > pci_set_drvdata(pdev, cxlds);
> >
> > cxlds->rcd = is_cxl_restricted(pdev); @@ -967,6 +1159,7 @@ static
> > struct pci_driver cxl_pci_driver = {
> > .err_handler = &cxl_error_handlers,
> > .driver = {
> > .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > + .dev_groups = cxl_rcd_groups,
> > },
> > };
> >
> > --
> > 2.43.0
> >
> >
>
