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.
---
> 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():
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.
> + 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.
> +}
> +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.
> +
> + 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.
> 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
>
>
