Re: [PATCH] pci-sysfs: Make PCI bridge attribute visible in sysfs
On Thu, May 4, 2017 at 2:32 AM, Vee Khee Wong wrote: > > > On Mon, 2017-04-17 at 10:41 -0500, Bjorn Helgaas wrote: >> On Mon, Apr 17, 2017 at 12:50 AM, Wong Vee Khee > > wrote: >> > >> > From: vwong >> > >> > Export the PCIe link attributes of PCI bridges to sysfs. >> This needs justification for *why* we should export these via sysfs. >> >> Some of these things, e.g., secondary/subordinate bus numbers, are >> already visible to non-privileged users via "lspci -v". >> > We need to expose these attributes via sysfs due to several reasons > listed below: > > 1) PCIe capabilities info such as Maximum/Actual link width and link speed > only visible to privileged users via "lspci -vvv". The software that my team > is working on need to get PCIe link information as non-root user. > > 2) From a client perspective, it require a lot of overhead parsing output of > lspci to get PCIe capabilities. Our application is only interested in getting > PCIe bridges but lspci print info for all PCIe devices. I'm looking for a revised patch that incorporates the justification in the changelog and addresses the code comments I made. Bjorn
Re: [PATCH] pci-sysfs: Make PCI bridge attribute visible in sysfs
On Mon, 2017-04-17 at 10:41 -0500, Bjorn Helgaas wrote:
> On Mon, Apr 17, 2017 at 12:50 AM, Wong Vee Khee > wrote:
> >
> > From: vwong
> >
> > Export the PCIe link attributes of PCI bridges to sysfs.
> This needs justification for *why* we should export these via sysfs.
>
> Some of these things, e.g., secondary/subordinate bus numbers, are
> already visible to non-privileged users via "lspci -v".
>
We need to expose these attributes via sysfs due to several reasons
listed below:
1) PCIe capabilities info such as Maximum/Actual link width and link speed only
visible to privileged users via "lspci -vvv". The software that my team is
working on need to get PCIe link information as non-root user.
2) From a client perspective, it require a lot of overhead parsing output of
lspci to get PCIe capabilities. Our application is only interested in getting
PCIe bridges but lspci print info for all PCIe devices.
> >
> > Signed-off-by: Wong Vee Khee
> > Signed-off-by: Hui Chun Ong
> > ---
> > drivers/pci/pci-sysfs.c | 197
> > +-
> > include/uapi/linux/pci_regs.h | 4 +
> > 2 files changed, 197 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 25d010d..a218c43 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -154,6 +154,127 @@ static ssize_t resource_show(struct device
> > *dev, struct device_attribute *attr,
> > }
> > static DEVICE_ATTR_RO(resource);
> >
> > +static ssize_t max_link_speed_show(struct device *dev,
> > + struct device_attribute *attr,
> > char *buf)
> > +{
> > + struct pci_dev *pci_dev = to_pci_dev(dev);
> > + u32 linkcap;
> > + int err;
> > + const char *speed;
> > +
> > + err = pcie_capability_read_dword(pci_dev, PCI_EXP_LNKCAP,
> > &linkcap);
> > +
> > + if (!err && linkcap) {
> if (err)
> return -EINVAL;
>
> I don't think there's a reason to test "linkcap" here. Per spec,
> zero
> is not a valid value of LNKCAP, so if we got a zero, I think showing
> "Unknown speed" would be fine.
>
> >
> > + switch (linkcap & PCI_EXP_LNKCAP_MLS) {
> > + case PCI_EXP_LNKCAP_MLS_8_0GB:
> > + speed = "8 GT/s";
> > + break;
> > + case PCI_EXP_LNKCAP_MLS_5_0GB:
> > + speed = "5 GT/s";
> > + break;
> > + case PCI_EXP_LNKCAP_MLS_2_5GB:
> > + speed = "2.5 GT/s";
> > + break;
> > + default:
> > + speed = "Unknown speed";
> > + }
> > +
> > + return sprintf(buf, "%s\n", speed);
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +static DEVICE_ATTR_RO(max_link_speed);
> > +
> > +static ssize_t max_link_width_show(struct device *dev,
> > + struct device_attribute *attr,
> > char *buf)
> > +{
> > + struct pci_dev *pci_dev = to_pci_dev(dev);
> > + u32 linkcap;
> > + int err;
> > +
> > + err = pcie_capability_read_dword(pci_dev, PCI_EXP_LNKCAP,
> > &linkcap);
> > +
> > + return err ? -EINVAL : sprintf(
> > + buf, "%u\n", (linkcap & PCI_EXP_LNKCAP_MLW) >> 4);
> if (err)
> return -EINVAL;
>
> return sprintf(...);
>
> >
> > +}
> > +static DEVICE_ATTR_RO(max_link_width);
> > +
> > +static ssize_t current_link_speed_show(struct device *dev,
> > + struct device_attribute
> > *attr, char *buf)
> > +{
> > + struct pci_dev *pci_dev = to_pci_dev(dev);
> > + u16 linkstat;
> > + int err;
> > + const char *speed;
> > +
> > + err = pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA,
> > &linkstat);
> > +
> > + if (!err && linkstat) {
> See max_link_speed above.
>
> >
> > + switch (linkstat & PCI_EXP_LNKSTA_CLS) {
> > + case PCI_EXP_LNKSTA_CLS_8_0GB:
> > + speed = "8 GT/s";
> > + break;
> > + case PCI_EXP_LNKSTA_CLS_5_0GB:
> > + speed = "5 GT/s";
> > + break;
> > + case PCI_EXP_LNKSTA_CLS_2_5GB:
> > + speed = "2.5 GT/s";
> > + break;
> > + default:
> > + speed = "Unknown speed";
> > + }
> > +
> > + return sprintf(buf, "%s\n", speed);
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +static DEVICE_ATTR_RO(current_link_speed);
> > +
> > +static ssize_t current_link_width_show(struct device *dev,
> > + struct device_attribute
> > *attr, char *buf)
> > +{
> > + struct pci_dev *pci_dev = to_pci_dev(dev);
> > + u16 linkstat;
> > + int err;
> > +
> > + err = pcie_capability_read_word
Re: [PATCH] pci-sysfs: Make PCI bridge attribute visible in sysfs
On Mon, Apr 17, 2017 at 12:50 AM, Wong Vee Khee wrote:
> From: vwong
>
> Export the PCIe link attributes of PCI bridges to sysfs.
This needs justification for *why* we should export these via sysfs.
Some of these things, e.g., secondary/subordinate bus numbers, are
already visible to non-privileged users via "lspci -v".
> Signed-off-by: Wong Vee Khee
> Signed-off-by: Hui Chun Ong
> ---
> drivers/pci/pci-sysfs.c | 197
> +-
> include/uapi/linux/pci_regs.h | 4 +
> 2 files changed, 197 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 25d010d..a218c43 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -154,6 +154,127 @@ static ssize_t resource_show(struct device *dev, struct
> device_attribute *attr,
> }
> static DEVICE_ATTR_RO(resource);
>
> +static ssize_t max_link_speed_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + u32 linkcap;
> + int err;
> + const char *speed;
> +
> + err = pcie_capability_read_dword(pci_dev, PCI_EXP_LNKCAP, &linkcap);
> +
> + if (!err && linkcap) {
if (err)
return -EINVAL;
I don't think there's a reason to test "linkcap" here. Per spec, zero
is not a valid value of LNKCAP, so if we got a zero, I think showing
"Unknown speed" would be fine.
> + switch (linkcap & PCI_EXP_LNKCAP_MLS) {
> + case PCI_EXP_LNKCAP_MLS_8_0GB:
> + speed = "8 GT/s";
> + break;
> + case PCI_EXP_LNKCAP_MLS_5_0GB:
> + speed = "5 GT/s";
> + break;
> + case PCI_EXP_LNKCAP_MLS_2_5GB:
> + speed = "2.5 GT/s";
> + break;
> + default:
> + speed = "Unknown speed";
> + }
> +
> + return sprintf(buf, "%s\n", speed);
> + }
> +
> + return -EINVAL;
> +}
> +static DEVICE_ATTR_RO(max_link_speed);
> +
> +static ssize_t max_link_width_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + u32 linkcap;
> + int err;
> +
> + err = pcie_capability_read_dword(pci_dev, PCI_EXP_LNKCAP, &linkcap);
> +
> + return err ? -EINVAL : sprintf(
> + buf, "%u\n", (linkcap & PCI_EXP_LNKCAP_MLW) >> 4);
if (err)
return -EINVAL;
return sprintf(...);
> +}
> +static DEVICE_ATTR_RO(max_link_width);
> +
> +static ssize_t current_link_speed_show(struct device *dev,
> + struct device_attribute *attr, char
> *buf)
> +{
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + u16 linkstat;
> + int err;
> + const char *speed;
> +
> + err = pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &linkstat);
> +
> + if (!err && linkstat) {
See max_link_speed above.
> + switch (linkstat & PCI_EXP_LNKSTA_CLS) {
> + case PCI_EXP_LNKSTA_CLS_8_0GB:
> + speed = "8 GT/s";
> + break;
> + case PCI_EXP_LNKSTA_CLS_5_0GB:
> + speed = "5 GT/s";
> + break;
> + case PCI_EXP_LNKSTA_CLS_2_5GB:
> + speed = "2.5 GT/s";
> + break;
> + default:
> + speed = "Unknown speed";
> + }
> +
> + return sprintf(buf, "%s\n", speed);
> + }
> +
> + return -EINVAL;
> +}
> +static DEVICE_ATTR_RO(current_link_speed);
> +
> +static ssize_t current_link_width_show(struct device *dev,
> + struct device_attribute *attr, char
> *buf)
> +{
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + u16 linkstat;
> + int err;
> +
> + err = pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &linkstat);
> +
> + return err ? -EINVAL : sprintf(
> + buf, "%u\n",
> + (linkstat & PCI_EXP_LNKSTA_NLW) >> PCI_EXP_LNKSTA_NLW_SHIFT);
> +}
> +static DEVICE_ATTR_RO(current_link_width);
> +
> +static ssize_t secondary_bus_number_show(struct device *dev,
> +struct device_attribute *attr,
> +char *buf)
> +{
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + u8 sec_bus;
> + int err;
> +
> + err = pci_read_config_byte(pci_dev, PCI_SECONDARY_BUS, &sec_bus);
There is already a /sys/devices/pci:BB/:BB:dd.f//
directory and a .../pci_bus/ directory that looks like it is the
secondary bus. Is that enough?
If we also need this file, I would like it to do something sensible
when there is no secondary bus
[PATCH] pci-sysfs: Make PCI bridge attribute visible in sysfs
From: vwong
Export the PCIe link attributes of PCI bridges to sysfs.
Signed-off-by: Wong Vee Khee
Signed-off-by: Hui Chun Ong
---
drivers/pci/pci-sysfs.c | 197 +-
include/uapi/linux/pci_regs.h | 4 +
2 files changed, 197 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 25d010d..a218c43 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -154,6 +154,127 @@ static ssize_t resource_show(struct device *dev, struct
device_attribute *attr,
}
static DEVICE_ATTR_RO(resource);
+static ssize_t max_link_speed_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct pci_dev *pci_dev = to_pci_dev(dev);
+ u32 linkcap;
+ int err;
+ const char *speed;
+
+ err = pcie_capability_read_dword(pci_dev, PCI_EXP_LNKCAP, &linkcap);
+
+ if (!err && linkcap) {
+ switch (linkcap & PCI_EXP_LNKCAP_MLS) {
+ case PCI_EXP_LNKCAP_MLS_8_0GB:
+ speed = "8 GT/s";
+ break;
+ case PCI_EXP_LNKCAP_MLS_5_0GB:
+ speed = "5 GT/s";
+ break;
+ case PCI_EXP_LNKCAP_MLS_2_5GB:
+ speed = "2.5 GT/s";
+ break;
+ default:
+ speed = "Unknown speed";
+ }
+
+ return sprintf(buf, "%s\n", speed);
+ }
+
+ return -EINVAL;
+}
+static DEVICE_ATTR_RO(max_link_speed);
+
+static ssize_t max_link_width_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct pci_dev *pci_dev = to_pci_dev(dev);
+ u32 linkcap;
+ int err;
+
+ err = pcie_capability_read_dword(pci_dev, PCI_EXP_LNKCAP, &linkcap);
+
+ return err ? -EINVAL : sprintf(
+ buf, "%u\n", (linkcap & PCI_EXP_LNKCAP_MLW) >> 4);
+}
+static DEVICE_ATTR_RO(max_link_width);
+
+static ssize_t current_link_speed_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct pci_dev *pci_dev = to_pci_dev(dev);
+ u16 linkstat;
+ int err;
+ const char *speed;
+
+ err = pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &linkstat);
+
+ if (!err && linkstat) {
+ switch (linkstat & PCI_EXP_LNKSTA_CLS) {
+ case PCI_EXP_LNKSTA_CLS_8_0GB:
+ speed = "8 GT/s";
+ break;
+ case PCI_EXP_LNKSTA_CLS_5_0GB:
+ speed = "5 GT/s";
+ break;
+ case PCI_EXP_LNKSTA_CLS_2_5GB:
+ speed = "2.5 GT/s";
+ break;
+ default:
+ speed = "Unknown speed";
+ }
+
+ return sprintf(buf, "%s\n", speed);
+ }
+
+ return -EINVAL;
+}
+static DEVICE_ATTR_RO(current_link_speed);
+
+static ssize_t current_link_width_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct pci_dev *pci_dev = to_pci_dev(dev);
+ u16 linkstat;
+ int err;
+
+ err = pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &linkstat);
+
+ return err ? -EINVAL : sprintf(
+ buf, "%u\n",
+ (linkstat & PCI_EXP_LNKSTA_NLW) >> PCI_EXP_LNKSTA_NLW_SHIFT);
+}
+static DEVICE_ATTR_RO(current_link_width);
+
+static ssize_t secondary_bus_number_show(struct device *dev,
+struct device_attribute *attr,
+char *buf)
+{
+ struct pci_dev *pci_dev = to_pci_dev(dev);
+ u8 sec_bus;
+ int err;
+
+ err = pci_read_config_byte(pci_dev, PCI_SECONDARY_BUS, &sec_bus);
+
+ return err ? -EINVAL : sprintf(buf, "%u\n", sec_bus);
+}
+static DEVICE_ATTR_RO(secondary_bus_number);
+
+static ssize_t subordinate_bus_number_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct pci_dev *pci_dev = to_pci_dev(dev);
+ u8 sub_bus;
+ int err;
+
+ err = pci_read_config_byte(pci_dev, PCI_SUBORDINATE_BUS, &sub_bus);
+
+ return err ? -EINVAL : sprintf(buf, "%u\n", sub_bus);
+}
+static DEVICE_ATTR_RO(subordinate_bus_number);
+
static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
@@ -602,12 +723,17 @@ static struct attribute *pci_dev_attrs[] = {
NULL,
};
-static const struct attribute_group pci_dev_group = {
- .attrs = pci_dev_attrs,
+static struct attribute *pci_bridge_attrs[] = {
+ &dev_attr_subordinate_bus_number.attr,
+ &dev_attr_secondary_bus_number.attr,
+ NU
