| Message ID | 20180717203900.GA1771@bombadil.infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 17/07/18 02:39 PM, Matthew Wilcox wrote: > On Tue, Jul 17, 2018 at 11:02:00AM -0600, Logan Gunthorpe wrote: >> The second patch expands the new helper to optionally take a path of >> PCI devfns. This is to address Alex's renumbering concern when using >> simple bus-devfns. The implementation is essentially how he described it and >> similar to the Intel VT-d spec (Section 8.3.1). > > I don't like telling the user to grovel around lspci -t by hand. It's > not many lines of code to add a new -P option to lspci to show the path > to each device instead of bus:dev.fn Thanks, this looks great! I also found parsing the lspci -t output cumbersome. I've also got patches pending for switchtec-user[1] that help users find the path of downstream ports for Microsemi switches. (An example is shown below). As the ACS feature is primarily for PCI switch users, this should help a good segment of people. The lspci patches should cover a lot more people though. Logan sudo switchtec status /dev/switchtec0 -v Partition 0: (LOCAL) Logical Port ID 0 (USP): Phys Port ID: 32 (Stack 4, Port 0) Bus-Dev-Func: 0000:02:00.0 Bus-Dev-Func Path: 0000:00:02:0/00.0 Status: UP LTSSM: L0 Max-Width: x16 Neg Width: x16 Rate: Gen3 - 8 GT/s 15.76 GB/s Out Bytes: 70.3 GB In Bytes: 70.8 GB Logical Port ID 1 (DSP): Phys Port ID: 8 (Stack 1, Port 0) Bus-Dev-Func: 0000:03:00.0 Bus-Dev-Func Path: 0000:00:02:0/00.0/00.0 Status: UP LTSSM: L0 Max-Width: x8 Neg Width: x8 Rate: Gen3 - 8 GT/s 7.88 GB/s Out Bytes: 12.2 MB In Bytes: 441 MB ACS: SrcValid- TransBlk- ReqRedir- CmpltRedir- UpstreamFwd- EgressCtrl- DirectTrans- Device: 10b5:8724 (0000:04:00.0) 0000:05 Logical Port ID 2 (DSP): Phys Port ID: 12 (Stack 1, Port 4) Bus-Dev-Func: 0000:03:01.0 Bus-Dev-Func Path: 0000:00:02:0/00.0/01.0 Status: UP LTSSM: L0 Max-Width: x8 Neg Width: x8 Rate: Gen3 - 8 GT/s 7.88 GB/s Out Bytes: 1.65 MB In Bytes: 107 MB ACS: SrcValid- TransBlk- ReqRedir- CmpltRedir- UpstreamFwd- EgressCtrl- DirectTrans- Device: 11f8:f117 (0000:0b:00.0) nvme4 [1] https://github.com/Microsemi/switchtec-user/pull/25
On Tue, Jul 17, 2018 at 01:39:00PM -0700, Matthew Wilcox wrote: > I don't like telling the user to grovel around lspci -t by hand. It's > not many lines of code to add a new -P option to lspci to show the path > to each device instead of bus:dev.fn > > Here's three examples, first without, then with -P. > ... > The Nehalem system makes an interesting testcase because it exposes some > registers in fake PCIe devices that aren't behind the root ports. eg: > > ff:06.3 Host bridge: Intel Corporation Xeon 5500/Core i7 Integrated Memory Controller Channel 2 Thermal Control Registers (rev 04) I think these appear as conventional PCI devices; at least the ones I've seen, e.g., [1], don't have a PCIe capability, so I think it makes sense that they're not behind a root port. [1] https://bugzilla5.redhat.com/attachment.cgi?id=433169
On Tue, Jul 17, 2018 at 04:00:53PM -0500, Bjorn Helgaas wrote: > On Tue, Jul 17, 2018 at 01:39:00PM -0700, Matthew Wilcox wrote: > > The Nehalem system makes an interesting testcase because it exposes some > > registers in fake PCIe devices that aren't behind the root ports. eg: > > > > ff:06.3 Host bridge: Intel Corporation Xeon 5500/Core i7 Integrated Memory Controller Channel 2 Thermal Control Registers (rev 04) > > I think these appear as conventional PCI devices; at least the ones > I've seen, e.g., [1], don't have a PCIe capability, so I think it > makes sense that they're not behind a root port. > > [1] https://bugzilla5.redhat.com/attachment.cgi?id=433169 Oh, I don't think we're doing anything wrong with how we're displaying them or what we're doing with what the system presents to us. My only point was that this is a good test-case for code which assumes that all PCI devices lie under a PCIe root port. At one point during development, my code reported that device up there as /06.3 Host bridge: Intel Corporation Xeon 5500/Core i7 Integrated Memory Controller Channel 2 Thermal Control Registers (rev 04) but since I had that system available to test with, I spotted that problem and made it present that device as ff:06.3 (both with and without -P).
Martin? Bjorn's looking to merge this soon and it'd be nice to have the support in lspci too. On Tue, Jul 17, 2018 at 01:39:00PM -0700, Matthew Wilcox wrote: > On Tue, Jul 17, 2018 at 11:02:00AM -0600, Logan Gunthorpe wrote: > > The second patch expands the new helper to optionally take a path of > > PCI devfns. This is to address Alex's renumbering concern when using > > simple bus-devfns. The implementation is essentially how he described it and > > similar to the Intel VT-d spec (Section 8.3.1). > > I don't like telling the user to grovel around lspci -t by hand. It's > not many lines of code to add a new -P option to lspci to show the path > to each device instead of bus:dev.fn > > Here's three examples, first without, then with -P. > > (my laptop): > 6d:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller SM961/PM961 > 00:1d.0/00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller SM961/PM961 > > (tests/PCI-X-bridges-and-domains): > 0002:42:00.0 Ethernet controller: Trident Microsystems 4DWave DX (rev 26) > 0002:00:02.4/01.0/00.0 Ethernet controller: Trident Microsystems 4DWave DX (rev 26) > > (my Nehalem system): > 04:00.0 Serial Attached SCSI controller: LSI Logic / Symbios Logic SAS2008 PCI-Express Fusion-MPT SAS-2 [Falcon] (rev 02) > 00:03.0/00.0/00.0/00.0 Serial Attached SCSI controller: LSI Logic / Symbios Logic SAS2008 PCI-Express Fusion-MPT SAS-2 [Falcon] (rev 02) > > The Nehalem system makes an interesting testcase because it exposes some > registers in fake PCIe devices that aren't behind the root ports. eg: > > ff:06.3 Host bridge: Intel Corporation Xeon 5500/Core i7 Integrated Memory Controller Channel 2 Thermal Control Registers (rev 04) > > Martin, what do you think to this patch? Also, I'm happy to send you > the lspci -xxxx from the Nehalem system to add to tests/ > > diff --git a/lspci.c b/lspci.c > index 3bf1925..ae0fdd2 100644 > --- a/lspci.c > +++ b/lspci.c > @@ -19,6 +19,7 @@ int verbose; /* Show detailed information */ > static int opt_hex; /* Show contents of config space as hexadecimal numbers */ > struct pci_filter filter; /* Device filter */ > static int opt_tree; /* Show bus tree */ > +static int opt_path; /* Show bridge path */ > static int opt_machine; /* Generate machine-readable output */ > static int opt_map_mode; /* Bus mapping mode enabled */ > static int opt_domains; /* Show domain numbers (0=disabled, 1=auto-detected, 2=requested) */ > @@ -29,7 +30,7 @@ char *opt_pcimap; /* Override path to Linux modules.pcimap */ > > const char program_name[] = "lspci"; > > -static char options[] = "nvbxs:d:ti:mgp:qkMDQ" GENERIC_OPTIONS ; > +static char options[] = "nvbxs:d:tPi:mgp:qkMDQ" GENERIC_OPTIONS ; > > static char help_msg[] = > "Usage: lspci [<switches>]\n" > @@ -247,6 +248,34 @@ sort_them(void) > > /*** Normal output ***/ > > +static void > +show_slot_path(struct pci_dev *p) > +{ > + struct pci_dev *d = NULL; > + > + if (opt_path && p->bus) > + { > + for (d = p->access->devices; d; d = d->next) { > + if (d->hdrtype == -1) > + d->hdrtype = pci_read_byte(d, PCI_HEADER_TYPE) & 0x7f; > + if (d->hdrtype != PCI_HEADER_TYPE_BRIDGE && > + d->hdrtype != PCI_HEADER_TYPE_CARDBUS) > + continue; > + if (pci_read_byte(d, PCI_SECONDARY_BUS) > p->bus) > + continue; > + if (pci_read_byte(d, PCI_SUBORDINATE_BUS) < p->bus) > + continue; > + show_slot_path(d); > + break; > + } > + } > + > + if (d) > + printf("/%02x.%d", p->dev, p->func); > + else > + printf("%02x:%02x.%d", p->bus, p->dev, p->func); > +} > + > static void > show_slot_name(struct device *d) > { > @@ -254,7 +283,7 @@ show_slot_name(struct device *d) > > if (!opt_machine ? opt_domains : (p->domain || opt_domains >= 2)) > printf("%04x:", p->domain); > - printf("%02x:%02x.%d", p->bus, p->dev, p->func); > + show_slot_path(p); > } > > void > @@ -989,6 +1018,9 @@ main(int argc, char **argv) > case 'x': > opt_hex++; > break; > + case 'P': > + opt_path++; > + break; > case 't': > opt_tree++; > break; > diff --git a/lspci.man b/lspci.man > index 35b3620..565dd5b 100644 > --- a/lspci.man > +++ b/lspci.man > @@ -95,6 +95,9 @@ PCI bus instead of as seen by the kernel. > .B -D > Always show PCI domain numbers. By default, lspci suppresses them on machines which > have only domain 0. > +.TP > +.B -P > +Name PCI devices by path through each bridge, instead of by bus number. > > .SS Options to control resolving ID's to names > .TP > -- > To unsubscribe from this list: send the line "unsubscribe linux-doc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello! > I don't like telling the user to grovel around lspci -t by hand. It's > not many lines of code to add a new -P option to lspci to show the path > to each device instead of bus:dev.fn I like the feature, but I have a couple of minor objections to the implementation: > -static char options[] = "nvbxs:d:ti:mgp:qkMDQ" GENERIC_OPTIONS ; > +static char options[] = "nvbxs:d:tPi:mgp:qkMDQ" GENERIC_OPTIONS ; Please update the help_msg, too. > +static void > +show_slot_path(struct pci_dev *p) > +{ > + struct pci_dev *d = NULL; > + > + if (opt_path && p->bus) > + { > + for (d = p->access->devices; d; d = d->next) { > + if (d->hdrtype == -1) > + d->hdrtype = pci_read_byte(d, PCI_HEADER_TYPE) & 0x7f; Please do not modify the hdrtype field, it is private to libpci. Instead, always read the PCI_HEADER_TYPE register. It will be fast since lspci caches configuration space accesses. > + if (d->hdrtype != PCI_HEADER_TYPE_BRIDGE && > + d->hdrtype != PCI_HEADER_TYPE_CARDBUS) > + continue; > + if (pci_read_byte(d, PCI_SECONDARY_BUS) > p->bus) > + continue; > + if (pci_read_byte(d, PCI_SUBORDINATE_BUS) < p->bus) > + continue; Beware, cardbus bridges use different registers for secondary and subordinate bus. ... the more I think about it, the more I am convinced that we want to re-use the bus topology builder from ls-tree.c. I will give it a try, stay tuned. Martin
Hello! > ... the more I think about it, the more I am convinced that we want to > re-use the bus topology builder from ls-tree.c. I will give it a try, > stay tuned. Please see the "topology" branch in pciutils.git. Martin
On Fri, Aug 10, 2018 at 12:30:35PM +0200, Martin Mares wrote: > Hello! > > > ... the more I think about it, the more I am convinced that we want to > > re-use the bus topology builder from ls-tree.c. I will give it a try, > > stay tuned. > > Please see the "topology" branch in pciutils.git. Thanks. I found two problems so far. One is that using -P and -s together doesn't work because we haven't scanned the entire topology. $ ./lspci-mw -PF tests/fujitsu-p8010.lspci -s 1d:00.0 00:1e.0/03.0/00.0 Network controller: 3Com Corporation 3com 3CRWE154G72 [Office Connect Wireless LAN Adapter] (rev 01) $ ./lspci-mm -PF tests/fujitsu-p8010.lspci -s 1d:00.0 1d:00.0 Network controller: 3Com Corporation 3com 3CRWE154G72 [Office Connect Wireless LAN Adapter] (rev 01) The other is that even when not using -s, the topology isn't fully represented: $ ./lspci-mm -PF tests/fujitsu-p8010.lspci |grep 3com 00:1e.0/00.0 Network controller: 3Com Corporation 3com 3CRWE154G72 [Office Connect Wireless LAN Adapter] (rev 01) I've attached a compressed form of the fujitsu-p8010.lspci dump for your testing.
Hello! > One is that using -P and -s together doesn't work because we haven't > scanned the entire topology. > > $ ./lspci-mw -PF tests/fujitsu-p8010.lspci -s 1d:00.0 > 00:1e.0/03.0/00.0 Network controller: 3Com Corporation 3com 3CRWE154G72 [Office Connect Wireless LAN Adapter] (rev 01) > $ ./lspci-mm -PF tests/fujitsu-p8010.lspci -s 1d:00.0 > 1d:00.0 Network controller: 3Com Corporation 3com 3CRWE154G72 [Office Connect Wireless LAN Adapter] (rev 01) Fixed. When topology is required, we now scan all devices and apply the filters later. > The other is that even when not using -s, the topology isn't fully represented: > > $ ./lspci-mm -PF tests/fujitsu-p8010.lspci |grep 3com > 00:1e.0/00.0 Network controller: 3Com Corporation 3com 3CRWE154G72 [Office Connect Wireless LAN Adapter] (rev 01) Ah well, it seems that the tree mode never worked with CardBus bridges. Fixed. After some pondering, I changed the format of the paths to include bus numbers in all steps. I think it is more intuitive. Please give it a try. If it works, I will merge the branch to master. Martin
On Sun, Aug 12, 2018 at 11:28:37AM +0200, Martin Mares wrote: > Hello! > > > One is that using -P and -s together doesn't work because we haven't > > scanned the entire topology. > > Fixed. When topology is required, we now scan all devices and apply the > filters later. Thanks! > > The other is that even when not using -s, the topology isn't fully represented: > > > > $ ./lspci-mm -PF tests/fujitsu-p8010.lspci |grep 3com > > 00:1e.0/00.0 Network controller: 3Com Corporation 3com 3CRWE154G72 [Office Connect Wireless LAN Adapter] (rev 01) > > Ah well, it seems that the tree mode never worked with CardBus bridges. Fixed. Haha! I can't believe we never noticed that in the last twenty years! And we're fixing it even though PCI CardBus bridges are now completely obsolete (my current laptop has no slots of that form factor; my previous laptop has an ExpressCard slot; I had to go back to my previous-previous laptop from 2008 to find sample hardware to test CardBus). > After some pondering, I changed the format of the paths to include bus numbers > in all steps. I think it is more intuitive. I agree it's more intuitive, but it's not the format that Logan's code is expecting, so it's not as useful for my purposes. How about this? $ ./lspci -PF tests/fujitsu-p8010.lspci -s 1d:00.0 00:1e.0/03.0/00.0 Network controller: 3Com Corporation 3com 3CRWE154G72 [Office Connect Wireless LAN Adapter] (rev 01) $ ./lspci -PPF tests/fujitsu-p8010.lspci -s 1d:00.0 00:1e.0/1c:03.0/1d:00.0 Network controller: 3Com Corporation 3com 3CRWE154G72 [Office Connect Wireless LAN Adapter] (rev 01) I pondered asking Logan to change his parser to include the bus number as a solution, but then I remembered the entire point of this is to make specifying a device robust against bus number assignmnet changes. I suppose we could have the parser accept and ignore the bus number ... diff --git a/lspci.c b/lspci.c index 75cb5b9..3dabbde 100644 --- a/lspci.c +++ b/lspci.c @@ -50,7 +50,8 @@ static char help_msg[] = "-xxxx\t\tShow hex-dump of the 4096-byte extended config space (root only)\n" "-b\t\tBus-centric view (addresses and IRQ's as seen by the bus)\n" "-D\t\tAlways show domain numbers\n" -"-P\t\tDisplay bus path in addition to bus and device number\n" +"-P\t\tDisplay bridge path in addition to bus and device number\n" +"-PP\t\tDisplay bus path in addition to bus and device number\n" "\n" "Resolving of device ID's to names:\n" "-n\t\tShow numeric ID's\n" @@ -264,7 +265,10 @@ show_slot_path(struct device *d) if (br && br->br_dev) { show_slot_path(br->br_dev); - printf("/%02x:%02x.%d", p->bus, p->dev, p->func); + if (opt_path > 1) + printf("/%02x:%02x.%d", p->bus, p->dev, p->func); + else + printf("/%02x.%d", p->dev, p->func); return; } } diff --git a/lspci.man b/lspci.man index 78b5c96..55fadb1 100644 --- a/lspci.man +++ b/lspci.man @@ -98,6 +98,10 @@ have only domain 0. .TP .B -P Identify PCI devices by path through each bridge, instead of by bus number. +.TP +.B -PP +Identify PCI devices by path through each bridge, showing the bus number as +well as the device number. .SS Options to control resolving ID's to names .TP
Hello! > I agree it's more intuitive, but it's not the format that Logan's code > is expecting, so it's not as useful for my purposes. How about this? > > $ ./lspci -PF tests/fujitsu-p8010.lspci -s 1d:00.0 > 00:1e.0/03.0/00.0 Network controller: 3Com Corporation 3com 3CRWE154G72 [Office Connect Wireless LAN Adapter] (rev 01) > $ ./lspci -PPF tests/fujitsu-p8010.lspci -s 1d:00.0 > 00:1e.0/1c:03.0/1d:00.0 Network controller: 3Com Corporation 3com 3CRWE154G72 [Office Connect Wireless LAN Adapter] (rev 01) Yes, this looks fine. It is merged to master now and I will push a new release out of the door soon. Thanks! Martin
On 12/08/18 04:31 AM, Matthew Wilcox wrote: > I pondered asking Logan to change his parser to include the bus number > as a solution, but then I remembered the entire point of this is to make > specifying a device robust against bus number assignmnet changes. I suppose > we could have the parser accept and ignore the bus number ... Yes, exactly. Bjorn's already accepted the series but we could add support for ignored bus letters in another patch if we want. I just think that might be confusing when you end up in a situation where the path includes numbers that no longer actually match the actual addresses after the bus numbers change. Thanks, Logan
diff --git a/lspci.c b/lspci.c index 3bf1925..ae0fdd2 100644 --- a/lspci.c +++ b/lspci.c @@ -19,6 +19,7 @@ int verbose; /* Show detailed information */ static int opt_hex; /* Show contents of config space as hexadecimal numbers */ struct pci_filter filter; /* Device filter */ static int opt_tree; /* Show bus tree */ +static int opt_path; /* Show bridge path */ static int opt_machine; /* Generate machine-readable output */ static int opt_map_mode; /* Bus mapping mode enabled */ static int opt_domains; /* Show domain numbers (0=disabled, 1=auto-detected, 2=requested) */ @@ -29,7 +30,7 @@ char *opt_pcimap; /* Override path to Linux modules.pcimap */ const char program_name[] = "lspci"; -static char options[] = "nvbxs:d:ti:mgp:qkMDQ" GENERIC_OPTIONS ; +static char options[] = "nvbxs:d:tPi:mgp:qkMDQ" GENERIC_OPTIONS ; static char help_msg[] = "Usage: lspci [<switches>]\n" @@ -247,6 +248,34 @@ sort_them(void) /*** Normal output ***/ +static void +show_slot_path(struct pci_dev *p) +{ + struct pci_dev *d = NULL; + + if (opt_path && p->bus) + { + for (d = p->access->devices; d; d = d->next) { + if (d->hdrtype == -1) + d->hdrtype = pci_read_byte(d, PCI_HEADER_TYPE) & 0x7f; + if (d->hdrtype != PCI_HEADER_TYPE_BRIDGE && + d->hdrtype != PCI_HEADER_TYPE_CARDBUS) + continue; + if (pci_read_byte(d, PCI_SECONDARY_BUS) > p->bus) + continue; + if (pci_read_byte(d, PCI_SUBORDINATE_BUS) < p->bus) + continue; + show_slot_path(d); + break; + } + } + + if (d) + printf("/%02x.%d", p->dev, p->func); + else + printf("%02x:%02x.%d", p->bus, p->dev, p->func); +} + static void show_slot_name(struct device *d) { @@ -254,7 +283,7 @@ show_slot_name(struct device *d) if (!opt_machine ? opt_domains : (p->domain || opt_domains >= 2)) printf("%04x:", p->domain); - printf("%02x:%02x.%d", p->bus, p->dev, p->func); + show_slot_path(p); } void @@ -989,6 +1018,9 @@ main(int argc, char **argv) case 'x': opt_hex++; break; + case 'P': + opt_path++; + break; case 't': opt_tree++; break; diff --git a/lspci.man b/lspci.man index 35b3620..565dd5b 100644 --- a/lspci.man +++ b/lspci.man @@ -95,6 +95,9 @@ PCI bus instead of as seen by the kernel. .B -D Always show PCI domain numbers. By default, lspci suppresses them on machines which have only domain 0. +.TP +.B -P +Name PCI devices by path through each bridge, instead of by bus number. .SS Options to control resolving ID's to names .TP