From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932260AbdEDHzI (ORCPT ); Thu, 4 May 2017 03:55:08 -0400 Received: from mx0b-00010702.pphosted.com ([148.163.158.57]:60031 "EHLO mx0b-00010702.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751721AbdEDHzC (ORCPT ); Thu, 4 May 2017 03:55:02 -0400 X-Greylist: delayed 1310 seconds by postgrey-1.27 at vger.kernel.org; Thu, 04 May 2017 03:55:01 EDT From: Vee Khee Wong To: "bhelgaas@google.com" CC: "linux-kernel@vger.kernel.org" , Jonathan Hearn , "Ram.Amrani@cavium.com" , Hui Chun Ong , "shhuiw@foxmail.com" , "rajatja@google.com" , "linux-pci@vger.kernel.org" , "keith.busch@intel.com" , "jonathan.yong@intel.com" Subject: Re: [PATCH] pci-sysfs: Make PCI bridge attribute visible in sysfs Thread-Topic: [PATCH] pci-sysfs: Make PCI bridge attribute visible in sysfs Thread-Index: AQHStz6R4dR9GyCgL0umCZXSYBUhbqHJs7sAgBovDYA= Date: Thu, 4 May 2017 07:32:54 +0000 Message-ID: <1493883171.10737.12.camel@ni.com> References: <1492408237-16598-1-git-send-email-vee.khee.wong@ni.com> In-Reply-To: Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: google.com; dkim=none (message not signed) header.d=none;google.com; dmarc=none action=none header.from=ni.com; x-originating-ip: [175.139.240.201] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1;CY1PR0401MB1600;7:vDkRVpwRKKVPZl0Ua56Z2jwsOIW9mHHcux6TJUbho0hOjnbD38nQwXHZyZ48F88SeJJ7ubU7w0iM4bXqTnBWyTfzVNNpFB7il+Rc6fBo0LH+HISOUJKmTj8BYo3YWUAp5HlDw82Pnh1yjujQm5didPiAbut8rzO2yDR8D5yRI/ge0j4nSEZgFg7b+6HrytK3q7QpPY+EdszJrMmd2UZLB1X1EphCgtwatxOHicCWTm1HB3Srk/Cjz6eEDlXu+vPyJApPBnYB6LvGg02hAM3DwybmDU3SvI9R9wxZxVHuBjbJlYwQbEiRaVKKLTxh7vW6CdL7I3Juf845SG5gid/k7A== x-forefront-antispam-report: SFV:SKI;SCL:-1SFV:NSPM;SFS:(10019020)(6009001)(39450400003)(39840400002)(39410400002)(39850400002)(39400400002)(377454003)(24454002)(377424004)(6512007)(103116003)(5660300001)(77096006)(54906002)(33646002)(6486002)(3280700002)(122556002)(2900100001)(3846002)(5640700003)(8936002)(6116002)(102836003)(76176999)(50986999)(99286003)(66066001)(54356999)(478600001)(189998001)(6436002)(6506006)(2351001)(305945005)(2501003)(7736002)(2906002)(53936002)(110136004)(1730700003)(3660700001)(38730400002)(25786009)(53546009)(2950100002)(6916009)(8676002)(4326008)(81166006)(86362001)(229853002)(36756003)(575784001);DIR:OUT;SFP:1102;SCL:1;SRVR:CY1PR0401MB1600;H:CO2PR04MB2341.namprd04.prod.outlook.com;FPR:;SPF:None;MLV:sfv;LANG:en; x-ms-office365-filtering-correlation-id: c1e9ea94-3a33-434f-e488-08d492bfc75d x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(2017030254075)(201703131423075)(201703031133081);SRVR:CY1PR0401MB1600; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(145744241990776); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(6040450)(601004)(2401047)(8121501046)(5005006)(93006095)(93001095)(10201501046)(3002001)(6041248)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123558100)(20161123555025)(20161123562025)(20161123560025)(20161123564025)(6072148);SRVR:CY1PR0401MB1600;BCL:0;PCL:0;RULEID:;SRVR:CY1PR0401MB1600; x-forefront-prvs: 02973C87BC spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 X-OriginatorOrg: ni.com X-MS-Exchange-CrossTenant-originalarrivaltime: 04 May 2017 07:32:54.5058 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 87ba1f9a-44cd-43a6-b008-6fdb45a5204e X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR0401MB1600 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-05-04_05:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_policy_notspam policy=outbound_policy score=30 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=30 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1705040124 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v447tPeR004928 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(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/pciDDDD:BB/DDDD: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.  Maybe that is exposing the bus > numbers directly, or maybe it is something else.  I tend to think > that > if you just want the register contents, lspci is enough, and if you > need something in sysfs, it should be a little more digested, e.g., > the existing subdirectory. > > It'd be helpful to know something about how you want to use this. > > > > > +       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, > > +       NULL, > >  }; > > > > -const struct attribute_group *pci_dev_groups[] = { > > -       &pci_dev_group, > > +static struct attribute *pcie_dev_attrs[] = { > > +       &dev_attr_current_link_speed.attr, > > +       &dev_attr_current_link_width.attr, > > +       &dev_attr_max_link_width.attr, > > +       &dev_attr_max_link_speed.attr, > >         NULL, > >  }; > > > > @@ -1540,6 +1666,57 @@ static umode_t > > pci_dev_hp_attrs_are_visible(struct kobject *kobj, > >         return a->mode; > >  } > > > > +static umode_t pci_bridge_attrs_are_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 (!pci_is_bridge(pdev)) > > +               return 0; >   if (pci_is_bridge(pdev)) >     return a->mode; > >   return 0; > > I think it's easier to read without the negation.  Possibly that's > just my personal preference :) > > > > > + > > +       return a->mode; > > +} > > + > > +static umode_t pcie_dev_attrs_are_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 (pci_find_capability(pdev, PCI_CAP_ID_EXP) == 0) > Use pci_is_pcie(). > > > > > +               return 0; > > + > > +       return a->mode; > > +} > > + > > +static const struct attribute_group pci_dev_group = { > > +       .attrs = pci_dev_attrs, > > +}; > > + > > +const struct attribute_group *pci_dev_groups[] = { > > +       &pci_dev_group, > > +       NULL, > > +}; > > + > > +static const struct attribute_group pci_bridge_group = { > > +       .attrs = pci_bridge_attrs, > > +}; > > + > > +const struct attribute_group *pci_bridge_groups[] = { > > +       &pci_bridge_group, > > +       NULL, > > +}; > > + > > +static const struct attribute_group pcie_dev_group = { > > +       .attrs = pcie_dev_attrs, > > +}; > > + > > +const struct attribute_group *pcie_dev_groups[] = { > > +       &pcie_dev_group, > > +       NULL, > > +}; > > + > >  static struct attribute_group pci_dev_hp_attr_group = { > >         .attrs = pci_dev_hp_attrs, > >         .is_visible = pci_dev_hp_attrs_are_visible, > > @@ -1574,12 +1751,24 @@ static struct attribute_group > > pci_dev_attr_group = { > >         .is_visible = pci_dev_attrs_are_visible, > >  }; > > > > +static struct attribute_group pci_bridge_attr_group = { > > +       .attrs = pci_bridge_attrs, > > +       .is_visible = pci_bridge_attrs_are_visible, > > +}; > > + > > +static struct attribute_group pcie_dev_attr_group = { > > +       .attrs = pcie_dev_attrs, > > +       .is_visible = pcie_dev_attrs_are_visible, > > +}; > > + > >  static const struct attribute_group *pci_dev_attr_groups[] = { > >         &pci_dev_attr_group, > >         &pci_dev_hp_attr_group, > >  #ifdef CONFIG_PCI_IOV > >         &sriov_dev_attr_group, > >  #endif > > +       &pci_bridge_attr_group, > > +       &pcie_dev_attr_group, > >         NULL, > >  }; > > > > diff --git a/include/uapi/linux/pci_regs.h > > b/include/uapi/linux/pci_regs.h > > index 634c9c4..b1770dc 100644 > > --- a/include/uapi/linux/pci_regs.h > > +++ b/include/uapi/linux/pci_regs.h > > @@ -517,6 +517,10 @@ > >  #define  PCI_EXP_LNKCAP_SLS    0x0000000f /* Supported Link Speeds > > */ > >  #define  PCI_EXP_LNKCAP_SLS_2_5GB 0x00000001 /* LNKCAP2 SLS Vector > > bit 0 */ > >  #define  PCI_EXP_LNKCAP_SLS_5_0GB 0x00000002 /* LNKCAP2 SLS Vector > > bit 1 */ > > +#define  PCI_EXP_LNKCAP_MLS    0x0000000f /* Maximum Link Speeds > > */ > > +#define  PCI_EXP_LNKCAP_MLS_2_5GB 0x00000001 /* LNKCAP2 SLS Vector > > bit 0 */ > > +#define  PCI_EXP_LNKCAP_MLS_5_0GB 0x00000002 /* LNKCAP2 SLS Vector > > bit 1 */ > > +#define  PCI_EXP_LNKCAP_MLS_8_0GB 0x00000003 /* LNKCAP2 SLS Vector > > bit 2 */ > Rather than adding these _MLS_ symbols as duplicates of _SLS_, please > just add one SLS_8_0GB symbol. > > The _SLS_ names are an artifact of the fact that prior to PCIe r3.0, > this field was the "Supported Link Speeds" field.  PCIe 3.0 renamed > it > to "Max Link Speed" and added the "Supported Link Speeds Vector" in > the new Link Capabilities 2 register. > > > > >  #define  PCI_EXP_LNKCAP_MLW    0x000003f0 /* Maximum Link Width */ > >  #define  PCI_EXP_LNKCAP_ASPMS  0x00000c00 /* ASPM Support */ > >  #define  PCI_EXP_LNKCAP_L0SEL  0x00007000 /* L0s Exit Latency */ > > -- > > 2.7.4 > >