[PATCH pciutils 0/5] Support for PROGIF, REVID and SUBSYS

[PATCH pciutils 0/5] Support for PROGIF, REVID and SUBSYS

[Pali Rohár]

libpci currently provides only access to bits [23:8] of class id via
dev->device_class member. Remaining bits [7:0] of class id can be only
accessed via reading config space.

lspci in some places reads class id from dev->device_class and on some
places from config space because it does not have access to all class bits.

For some broken devices kernel reports different class id via sysfs as
what is stored in device config space. Value reported by sysfs should be
the correct one.

lspci has -b option (Bus-centric view) to choose if information from
kernel or from config space should be showed. But this option is not
respected on all places because of missing bits.

Same applies for vendor+device ids, subsystem ids and revision id.

Export all these information from sysfs via libpci API use it in lspci.
With this change lspci should now respect -b option for all these
information.

With this change are in libpci and lspci also available subsystem ids
for PCI-to-PCI bridges.

This patch series is based on top of another patch series:
https://lore.kernel.org/linux-pci/20211220155448.1233-3-pali@kernel.org/

Pali Rohár (5):
  libpci: Add new options for pci_fill_info: PROGIF, REVID and SUBSYS
  libpci: generic: Implement PROGIF, REVID and SUBSYS support
  libpci: generic: Implement SUBSYS also for PCI_HEADER_TYPE_BRIDGE
  libpci: sysfs: Implement PROGIF, REVID and SUBSYS support
  lspci: Retrieve prog if, subsystem ids and revision id via libpci

 lib/generic.c | 55 +++++++++++++++++++++++++++++++++++++++-
 lib/pci.h     |  6 +++++
 lib/sysfs.c   | 39 +++++++++++++++++++++++++---
 ls-kernel.c   |  8 +++---
 lspci.c       | 70 ++++++++++++++++++---------------------------------
 lspci.h       |  2 --
 6 files changed, 123 insertions(+), 57 deletions(-)

-- 
2.20.1

[PATCH pciutils 1/5] libpci: Add new options for pci_fill_info: PROGIF, REVID and SUBSYS

[Pali Rohár]

This change extends libpci library and allows providers to fill these
informations (Programming interface, Revision id and Subsystem ids) via
native system APIs, which sometimes may differs from what is stored in PCI
config space.

Programming interface is part of 24-bit Device Class number but apparently
libpci exports only high 16-bit of this number via device_class member.
---
 lib/pci.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/pci.h b/lib/pci.h
index b9fd9bfb9b5b..8c3c11b9ebeb 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -145,6 +145,9 @@ struct pci_dev {
   pciaddr_t bridge_base_addr[4];	/* Bridge base addresses (without flags) */
   pciaddr_t bridge_size[4];		/* Bridge sizes */
   pciaddr_t bridge_flags[4];		/* PCI_IORESOURCE_* flags for bridge addresses */
+  u8 prog_if;				/* Programming interface for device_class */
+  u8 rev_id;				/* Revision id */
+  u16 subsys_vendor_id, subsys_id;	/* Subsystem vendor id and subsystem id */
 
   /* Fields used internally */
   struct pci_access *access;
@@ -210,6 +213,9 @@ char *pci_get_string_property(struct pci_dev *d, u32 prop) PCI_ABI;
 #define PCI_FILL_IOMMU_GROUP	0x4000
 #define PCI_FILL_BRIDGE_BASES	0x8000
 #define PCI_FILL_RESCAN		0x00010000
+#define PCI_FILL_PROGIF		0x00020000
+#define PCI_FILL_REVID		0x00040000
+#define PCI_FILL_SUBSYS		0x00080000
 
 void pci_setup_cache(struct pci_dev *, u8 *cache, int len) PCI_ABI;
 
-- 
2.20.1

[PATCH pciutils 2/5] libpci: generic: Implement PROGIF, REVID and SUBSYS support

[Pali Rohár]

PCI_FILL_SUBSYS is implemented only for PCI_HEADER_TYPE_NORMAL and
PCI_HEADER_TYPE_CARDBUS like in lspci.
---
 lib/generic.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/lib/generic.c b/lib/generic.c
index ef9e2a34b4f4..f4b6918cb55b 100644
--- a/lib/generic.c
+++ b/lib/generic.c
@@ -80,7 +80,7 @@ pci_generic_fill_info(struct pci_dev *d, unsigned int flags)
   struct pci_access *a = d->access;
   unsigned int done = 0;
 
-  if ((flags & (PCI_FILL_BASES | PCI_FILL_ROM_BASE)) && d->hdrtype < 0)
+  if ((flags & (PCI_FILL_SUBSYS | PCI_FILL_BASES | PCI_FILL_ROM_BASE)) && d->hdrtype < 0)
     d->hdrtype = pci_read_byte(d, PCI_HEADER_TYPE) & 0x7f;
 
   if (flags & PCI_FILL_IDENT)
@@ -96,6 +96,35 @@ pci_generic_fill_info(struct pci_dev *d, unsigned int flags)
       done |= PCI_FILL_CLASS;
     }
 
+  if (flags & PCI_FILL_PROGIF)
+    {
+      d->prog_if = pci_read_byte(d, PCI_CLASS_PROG);
+      done |= PCI_FILL_PROGIF;
+    }
+
+  if (flags & PCI_FILL_REVID)
+    {
+      d->rev_id = pci_read_byte(d, PCI_REVISION_ID);
+      done |= PCI_FILL_REVID;
+    }
+
+  if (flags & PCI_FILL_SUBSYS)
+    {
+      switch (d->hdrtype)
+        {
+        case PCI_HEADER_TYPE_NORMAL:
+          d->subsys_vendor_id = pci_read_word(d, PCI_SUBSYSTEM_VENDOR_ID);
+          d->subsys_id = pci_read_word(d, PCI_SUBSYSTEM_ID);
+          done |= PCI_FILL_SUBSYS;
+          break;
+        case PCI_HEADER_TYPE_CARDBUS:
+          d->subsys_vendor_id = pci_read_word(d, PCI_CB_SUBSYSTEM_VENDOR_ID);
+          d->subsys_id = pci_read_word(d, PCI_CB_SUBSYSTEM_ID);
+          done |= PCI_FILL_SUBSYS;
+          break;
+        }
+    }
+
   if (flags & PCI_FILL_IRQ)
     {
       d->irq = pci_read_byte(d, PCI_INTERRUPT_LINE);
-- 
2.20.1

[PATCH pciutils 3/5] libpci: generic: Implement SUBSYS also for PCI_HEADER_TYPE_BRIDGE

[Pali Rohár]

Subsystem ids for PCI Bridges are stored in extended capability
PCI_CAP_ID_SSVID.
---
 lib/generic.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/lib/generic.c b/lib/generic.c
index f4b6918cb55b..2cdb7956754c 100644
--- a/lib/generic.c
+++ b/lib/generic.c
@@ -117,6 +117,30 @@ pci_generic_fill_info(struct pci_dev *d, unsigned int flags)
           d->subsys_id = pci_read_word(d, PCI_SUBSYSTEM_ID);
           done |= PCI_FILL_SUBSYS;
           break;
+        case PCI_HEADER_TYPE_BRIDGE:
+          if (pci_read_word(d, PCI_STATUS) & PCI_STATUS_CAP_LIST)
+            {
+              byte been_there[256];
+              int where, id;
+
+              memset(been_there, 0, 256);
+              where = pci_read_byte(d, PCI_CAPABILITY_LIST) & ~3;
+              while (where && !been_there[where]++)
+                {
+                  id = pci_read_byte(d, where + PCI_CAP_LIST_ID);
+                  if (id == PCI_CAP_ID_SSVID)
+                    {
+                      d->subsys_vendor_id = pci_read_word(d, where + PCI_SSVID_VENDOR);
+                      d->subsys_id = pci_read_word(d, where + PCI_SSVID_DEVICE);
+                      done |= PCI_FILL_SUBSYS;
+                      break;
+                    }
+                  if (id == 0xff)
+                    break;
+                  where = pci_read_byte(d, where + PCI_CAP_LIST_NEXT) & ~3;
+                }
+            }
+          break;
         case PCI_HEADER_TYPE_CARDBUS:
           d->subsys_vendor_id = pci_read_word(d, PCI_CB_SUBSYSTEM_VENDOR_ID);
           d->subsys_id = pci_read_word(d, PCI_CB_SUBSYSTEM_ID);
-- 
2.20.1

[PATCH pciutils 4/5] libpci: sysfs: Implement PROGIF, REVID and SUBSYS support

[Pali Rohár]

In sysfs there are optional nodes with this information.
---
 lib/sysfs.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/lib/sysfs.c b/lib/sysfs.c
index 7c157a2688ad..7714607f66a0 100644
--- a/lib/sysfs.c
+++ b/lib/sysfs.c
@@ -329,6 +329,7 @@ static unsigned int
 sysfs_fill_info(struct pci_dev *d, unsigned int flags)
 {
   unsigned int done = 0;
+  int value;
 
   if (!d->access->buscentric)
     {
@@ -343,10 +344,42 @@ sysfs_fill_info(struct pci_dev *d, unsigned int flags)
 	  d->device_id = sysfs_get_value(d, "device", 1);
 	  done |= PCI_FILL_IDENT;
 	}
-      if (flags & PCI_FILL_CLASS)
+      if (flags & (PCI_FILL_CLASS | PCI_FILL_PROGIF))
 	{
-	  d->device_class = sysfs_get_value(d, "class", 1) >> 8;
-	  done |= PCI_FILL_CLASS;
+	  value = sysfs_get_value(d, "class", 1);
+	  if (flags & PCI_FILL_CLASS)
+	    {
+	      d->device_class = value >> 8;
+	      done |= PCI_FILL_CLASS;
+	    }
+	  if (flags & PCI_FILL_PROGIF)
+	    {
+	      d->prog_if = value & 0xff;
+	      done |= PCI_FILL_PROGIF;
+	    }
+	}
+      if (flags & PCI_FILL_REVID)
+	{
+	  value = sysfs_get_value(d, "revision", 0);
+	  if (value >= 0)
+	    {
+	      d->rev_id = value;
+	      done |= PCI_FILL_REVID;
+	    }
+        }
+      if (flags & PCI_FILL_SUBSYS)
+	{
+	  value = sysfs_get_value(d, "subsystem_vendor", 0);
+	  if (value >= 0)
+	    {
+	      d->subsys_vendor_id = value;
+	      value = sysfs_get_value(d, "subsystem_device", 0);
+	      if (value >= 0)
+	        {
+	          d->subsys_id = value;
+	          done |= PCI_FILL_SUBSYS;
+	        }
+	    }
 	}
       if (flags & PCI_FILL_IRQ)
 	{
-- 
2.20.1

[PATCH pciutils 5/5] lspci: Retrieve prog if, subsystem ids and revision id via libpci

[Pali Rohár]

Use pci_fill_info with PROGIF, REVID and SUBSYS to fill this information.

lspci in some places reads class from what libpci provider fills in
dev->device_class and in some other places it reads directly from config
space. In dev->device_class is stored class possible different class as in
config space (e.g. if kernel is fixing class because device has bogus
information stored in config space).

With this change is class always read from dev->device_class which reflects
and respects lspci -b option (Bus-centric view). Same applies for subsystem
ids and revision id (note that prog if is part of class).
---
 ls-kernel.c |  8 +++---
 lspci.c     | 70 ++++++++++++++++++-----------------------------------
 lspci.h     |  2 --
 3 files changed, 27 insertions(+), 53 deletions(-)

diff --git a/ls-kernel.c b/ls-kernel.c
index ecacd0e65dce..2284b4625b12 100644
--- a/ls-kernel.c
+++ b/ls-kernel.c
@@ -174,16 +174,14 @@ static int
 match_pcimap(struct device *d, struct pcimap_entry *e)
 {
   struct pci_dev *dev = d->dev;
-  unsigned int class = get_conf_long(d, PCI_REVISION_ID) >> 8;
-  word subv, subd;
+  unsigned int class = (((unsigned int)dev->device_class << 8) | dev->prog_if);
 
 #define MATCH(x, y) ((y) > 0xffff || (x) == (y))
-  get_subid(d, &subv, &subd);
   return
     MATCH(dev->vendor_id, e->vendor) &&
     MATCH(dev->device_id, e->device) &&
-    MATCH(subv, e->subvendor) &&
-    MATCH(subd, e->subdevice) &&
+    MATCH(dev->subsys_vendor_id, e->subvendor) &&
+    MATCH(dev->subsys_id, e->subdevice) &&
     (class & e->class_mask) == e->class;
 #undef MATCH
 }
diff --git a/lspci.c b/lspci.c
index d14d1b9185d6..c4e6c93bc67a 100644
--- a/lspci.c
+++ b/lspci.c
@@ -143,7 +143,7 @@ scan_device(struct pci_dev *p)
 	d->config_cached += 64;
     }
   pci_setup_cache(p, d->config, d->config_cached);
-  pci_fill_info(p, PCI_FILL_IDENT | PCI_FILL_CLASS);
+  pci_fill_info(p, PCI_FILL_IDENT | PCI_FILL_CLASS | PCI_FILL_PROGIF | PCI_FILL_REVID | PCI_FILL_SUBSYS);
   return d;
 }
 
@@ -285,25 +285,6 @@ show_slot_name(struct device *d)
   show_slot_path(d);
 }
 
-void
-get_subid(struct device *d, word *subvp, word *subdp)
-{
-  byte htype = get_conf_byte(d, PCI_HEADER_TYPE) & 0x7f;
-
-  if (htype == PCI_HEADER_TYPE_NORMAL)
-    {
-      *subvp = get_conf_word(d, PCI_SUBSYSTEM_VENDOR_ID);
-      *subdp = get_conf_word(d, PCI_SUBSYSTEM_ID);
-    }
-  else if (htype == PCI_HEADER_TYPE_CARDBUS && d->config_cached >= 128)
-    {
-      *subvp = get_conf_word(d, PCI_CB_SUBSYSTEM_VENDOR_ID);
-      *subdp = get_conf_word(d, PCI_CB_SUBSYSTEM_ID);
-    }
-  else
-    *subvp = *subdp = 0xffff;
-}
-
 static void
 show_terse(struct device *d)
 {
@@ -319,12 +300,12 @@ show_terse(struct device *d)
 	 pci_lookup_name(pacc, devbuf, sizeof(devbuf),
 			 PCI_LOOKUP_VENDOR | PCI_LOOKUP_DEVICE,
 			 p->vendor_id, p->device_id));
-  if (c = get_conf_byte(d, PCI_REVISION_ID))
-    printf(" (rev %02x)", c);
+  if ((p->known_fields & PCI_FILL_REVID) && p->rev_id)
+    printf(" (rev %02x)", p->rev_id);
   if (verbose)
     {
       char *x;
-      c = get_conf_byte(d, PCI_CLASS_PROG);
+      c = (p->known_fields & PCI_FILL_PROGIF) ? p->prog_if : 0;
       x = pci_lookup_name(pacc, devbuf, sizeof(devbuf),
 			  PCI_LOOKUP_PROGIF | PCI_LOOKUP_NO_NUMBERS,
 			  p->device_class, c);
@@ -340,19 +321,18 @@ show_terse(struct device *d)
 
   if (verbose || opt_kernel)
     {
-      word subsys_v, subsys_d;
       char ssnamebuf[256];
 
       pci_fill_info(p, PCI_FILL_LABEL);
 
       if (p->label)
         printf("\tDeviceName: %s", p->label);
-      get_subid(d, &subsys_v, &subsys_d);
-      if (subsys_v && subsys_v != 0xffff)
+      if ((p->known_fields & PCI_FILL_SUBSYS) &&
+	  p->subsys_vendor_id && p->subsys_vendor_id != 0xffff)
 	printf("\tSubsystem: %s\n",
 		pci_lookup_name(pacc, ssnamebuf, sizeof(ssnamebuf),
 			PCI_LOOKUP_SUBSYSTEM | PCI_LOOKUP_VENDOR | PCI_LOOKUP_DEVICE,
-			p->vendor_id, p->device_id, subsys_v, subsys_d));
+			p->vendor_id, p->device_id, p->subsys_vendor_id, p->subsys_id));
     }
 }
 
@@ -766,7 +746,7 @@ show_verbose(struct device *d)
 
   pci_fill_info(p, PCI_FILL_IRQ | PCI_FILL_BASES | PCI_FILL_ROM_BASE | PCI_FILL_SIZES |
     PCI_FILL_PHYS_SLOT | PCI_FILL_NUMA_NODE | PCI_FILL_DT_NODE | PCI_FILL_IOMMU_GROUP |
-    PCI_FILL_BRIDGE_BASES);
+    PCI_FILL_BRIDGE_BASES | PCI_FILL_PROGIF | PCI_FILL_REVID | PCI_FILL_SUBSYS);
   irq = p->irq;
 
   switch (htype)
@@ -947,13 +927,9 @@ static void
 show_machine(struct device *d)
 {
   struct pci_dev *p = d->dev;
-  int c;
-  word sv_id, sd_id;
   char classbuf[128], vendbuf[128], devbuf[128], svbuf[128], sdbuf[128];
   char *dt_node, *iommu_group;
 
-  get_subid(d, &sv_id, &sd_id);
-
   if (verbose)
     {
       pci_fill_info(p, PCI_FILL_PHYS_SLOT | PCI_FILL_NUMA_NODE | PCI_FILL_DT_NODE | PCI_FILL_IOMMU_GROUP);
@@ -966,19 +942,20 @@ show_machine(struct device *d)
 	     pci_lookup_name(pacc, vendbuf, sizeof(vendbuf), PCI_LOOKUP_VENDOR, p->vendor_id, p->device_id));
       printf("Device:\t%s\n",
 	     pci_lookup_name(pacc, devbuf, sizeof(devbuf), PCI_LOOKUP_DEVICE, p->vendor_id, p->device_id));
-      if (sv_id && sv_id != 0xffff)
+      if ((p->known_fields & PCI_FILL_SUBSYS) &&
+	  p->subsys_vendor_id && p->subsys_vendor_id != 0xffff)
 	{
 	  printf("SVendor:\t%s\n",
-		 pci_lookup_name(pacc, svbuf, sizeof(svbuf), PCI_LOOKUP_SUBSYSTEM | PCI_LOOKUP_VENDOR, sv_id));
+		 pci_lookup_name(pacc, svbuf, sizeof(svbuf), PCI_LOOKUP_SUBSYSTEM | PCI_LOOKUP_VENDOR, p->subsys_vendor_id));
 	  printf("SDevice:\t%s\n",
-		 pci_lookup_name(pacc, sdbuf, sizeof(sdbuf), PCI_LOOKUP_SUBSYSTEM | PCI_LOOKUP_DEVICE, p->vendor_id, p->device_id, sv_id, sd_id));
+		 pci_lookup_name(pacc, sdbuf, sizeof(sdbuf), PCI_LOOKUP_SUBSYSTEM | PCI_LOOKUP_DEVICE, p->vendor_id, p->device_id, p->subsys_vendor_id, p->subsys_id));
 	}
       if (p->phy_slot)
 	printf("PhySlot:\t%s\n", p->phy_slot);
-      if (c = get_conf_byte(d, PCI_REVISION_ID))
-	printf("Rev:\t%02x\n", c);
-      if (c = get_conf_byte(d, PCI_CLASS_PROG))
-	printf("ProgIf:\t%02x\n", c);
+      if ((p->known_fields & PCI_FILL_REVID) && p->rev_id)
+	printf("Rev:\t%02x\n", p->rev_id);
+      if (p->known_fields & PCI_FILL_PROGIF)
+	printf("ProgIf:\t%02x\n", p->prog_if);
       if (opt_kernel)
 	show_kernel_machine(d);
       if (p->numa_node != -1)
@@ -994,14 +971,15 @@ show_machine(struct device *d)
       print_shell_escaped(pci_lookup_name(pacc, classbuf, sizeof(classbuf), PCI_LOOKUP_CLASS, p->device_class));
       print_shell_escaped(pci_lookup_name(pacc, vendbuf, sizeof(vendbuf), PCI_LOOKUP_VENDOR, p->vendor_id, p->device_id));
       print_shell_escaped(pci_lookup_name(pacc, devbuf, sizeof(devbuf), PCI_LOOKUP_DEVICE, p->vendor_id, p->device_id));
-      if (c = get_conf_byte(d, PCI_REVISION_ID))
-	printf(" -r%02x", c);
-      if (c = get_conf_byte(d, PCI_CLASS_PROG))
-	printf(" -p%02x", c);
-      if (sv_id && sv_id != 0xffff)
+      if ((p->known_fields & PCI_FILL_REVID) && p->rev_id)
+	printf(" -r%02x", p->rev_id);
+      if (p->known_fields & PCI_FILL_PROGIF)
+	printf(" -p%02x", p->prog_if);
+      if ((p->known_fields & PCI_FILL_SUBSYS) &&
+	  p->subsys_vendor_id && p->subsys_vendor_id != 0xffff)
 	{
-	  print_shell_escaped(pci_lookup_name(pacc, svbuf, sizeof(svbuf), PCI_LOOKUP_SUBSYSTEM | PCI_LOOKUP_VENDOR, sv_id));
-	  print_shell_escaped(pci_lookup_name(pacc, sdbuf, sizeof(sdbuf), PCI_LOOKUP_SUBSYSTEM | PCI_LOOKUP_DEVICE, p->vendor_id, p->device_id, sv_id, sd_id));
+	  print_shell_escaped(pci_lookup_name(pacc, svbuf, sizeof(svbuf), PCI_LOOKUP_SUBSYSTEM | PCI_LOOKUP_VENDOR, p->subsys_vendor_id));
+	  print_shell_escaped(pci_lookup_name(pacc, sdbuf, sizeof(sdbuf), PCI_LOOKUP_SUBSYSTEM | PCI_LOOKUP_DEVICE, p->vendor_id, p->device_id, p->subsys_vendor_id, p->subsys_id));
 	}
       else
 	printf(" \"\" \"\"");
diff --git a/lspci.h b/lspci.h
index 352177fcce7b..6e0bb2492fd5 100644
--- a/lspci.h
+++ b/lspci.h
@@ -55,8 +55,6 @@ u32 get_conf_long(struct device *d, unsigned int pos);
 word get_conf_word(struct device *d, unsigned int pos);
 byte get_conf_byte(struct device *d, unsigned int pos);
 
-void get_subid(struct device *d, word *subvp, word *subdp);
-
 /* Useful macros for decoding of bits and bit fields */
 
 #define FLAG(x,y) ((x & y) ? '+' : '-')
-- 
2.20.1

Re: [PATCH pciutils 0/5] Support for PROGIF, REVID and SUBSYS

[Martin Mareš]

Hello!

> libpci currently provides only access to bits [23:8] of class id via
> dev->device_class member. Remaining bits [7:0] of class id can be only
> accessed via reading config space.
[...]

I really do not like the explosion of PCI_FILL_xxx flags for trivial things.

Add a single PCI_FILL_CLASS_EXT, which will fill the class, subclass,
revision and programming interface.

					Martin

Re: [PATCH pciutils 3/5] libpci: generic: Implement SUBSYS also for PCI_HEADER_TYPE_BRIDGE

[Martin Mareš]

Hello!

> +        case PCI_HEADER_TYPE_BRIDGE:
> +          if (pci_read_word(d, PCI_STATUS) & PCI_STATUS_CAP_LIST)
> +            {
> +              byte been_there[256];
> +              int where, id;
> +
> +              memset(been_there, 0, 256);
> +              where = pci_read_byte(d, PCI_CAPABILITY_LIST) & ~3;
> +              while (where && !been_there[where]++)

Please don't. There should be a single implementation of capability list
walking in libpci, not everybody doing his own.

				Martin

Re: [PATCH pciutils 0/5] Support for PROGIF, REVID and SUBSYS

[Pali Rohár]

On Friday 21 January 2022 15:40:14 Martin Mareš wrote:
> Hello!
> 
> > libpci currently provides only access to bits [23:8] of class id via
> > dev->device_class member. Remaining bits [7:0] of class id can be only
> > accessed via reading config space.
> [...]
> 
> I really do not like the explosion of PCI_FILL_xxx flags for trivial things.
> 
> Add a single PCI_FILL_CLASS_EXT, which will fill the class, subclass,
> revision and programming interface.

Ok!

How to handle situation when "class+subclass+prog_if" is provided and
revision is not provided? What should libpci backends set in this case?
Because on both Linux and Windows systems are these information provided
separately. On Linux you can chmod 000 revision sysfs file and let class
sysfs file still readable. Windows can probably decide itself that it
would not report revision at all...

And what to do with subsystem ids? They are not part of
class/subclass/prog_if/revision fields and different devices have them
stored on different locations... And for PCI-to-PCI bridges they are
only optional and does not have to be present at all.

Re: [PATCH pciutils 0/5] Support for PROGIF, REVID and SUBSYS

[Martin Mareš]

Hello!

> How to handle situation when "class+subclass+prog_if" is provided and
> revision is not provided? What should libpci backends set in this case?
> Because on both Linux and Windows systems are these information provided
> separately. On Linux you can chmod 000 revision sysfs file and let class
> sysfs file still readable. Windows can probably decide itself that it
> would not report revision at all...

Read it from the config registers in that case.

> And what to do with subsystem ids? They are not part of
> class/subclass/prog_if/revision fields and different devices have them
> stored on different locations... And for PCI-to-PCI bridges they are
> only optional and does not have to be present at all.

I would prefer a separate PCI_FILL_xxx flag for them.

			Have a nice fortnight
-- 
Martin `MJ' Mareš                        <mj@ucw.cz>   http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe
"I don't give a damn for a man that can only spell a word one way." -- M. Twain

Re: [PATCH pciutils 0/5] Support for PROGIF, REVID and SUBSYS

[Pali Rohár]

On Friday 21 January 2022 16:15:32 Martin Mareš wrote:
> Hello!
> 
> > How to handle situation when "class+subclass+prog_if" is provided and
> > revision is not provided? What should libpci backends set in this case?
> > Because on both Linux and Windows systems are these information provided
> > separately. On Linux you can chmod 000 revision sysfs file and let class
> > sysfs file still readable. Windows can probably decide itself that it
> > would not report revision at all...
> 
> Read it from the config registers in that case.

On Linux "class+subclass+prog_if" can be different than what is in
config registers and the purpose of this patch series with new fields is
to allow user to see difference in lspci.

On Windows access to real config space is not available for non-system
users.

> > And what to do with subsystem ids? They are not part of
> > class/subclass/prog_if/revision fields and different devices have them
> > stored on different locations... And for PCI-to-PCI bridges they are
> > only optional and does not have to be present at all.
> 
> I would prefer a separate PCI_FILL_xxx flag for them.

So like PCI_FILL_SUBSYS flag in this patch series?

Re: [PATCH pciutils 0/5] Support for PROGIF, REVID and SUBSYS

[Martin Mareš]

> On Linux "class+subclass+prog_if" can be different than what is in
> config registers and the purpose of this patch series with new fields is
> to allow user to see difference in lspci.
> 
> On Windows access to real config space is not available for non-system
> users.

Sure, I meant to read it from the config registers only if the back-end
is unable to supply it.

> > > And what to do with subsystem ids? They are not part of
> > > class/subclass/prog_if/revision fields and different devices have them
> > > stored on different locations... And for PCI-to-PCI bridges they are
> > > only optional and does not have to be present at all.
> > 
> > I would prefer a separate PCI_FILL_xxx flag for them.
> 
> So like PCI_FILL_SUBSYS flag in this patch series?

Yes, that part was OK.

			Have a nice fortnight
-- 
Martin `MJ' Mareš                        <mj@ucw.cz>   http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe
Not all rumors are as misleading as this one.

Re: [PATCH pciutils 0/5] Support for PROGIF, REVID and SUBSYS

[Pali Rohár]

On Friday 21 January 2022 16:29:44 Martin Mareš wrote:
> > On Linux "class+subclass+prog_if" can be different than what is in
> > config registers and the purpose of this patch series with new fields is
> > to allow user to see difference in lspci.
> > 
> > On Windows access to real config space is not available for non-system
> > users.
> 
> Sure, I meant to read it from the config registers only if the back-end
> is unable to supply it.

And in case "class+subclass+prog_if" is available but "revision" is not
available and even it is not possible to read revision from config space
(because e.g. due to missing permissions or limitations of win backend)?

Re: [PATCH pciutils 0/5] Support for PROGIF, REVID and SUBSYS

[Martin Mareš]

> And in case "class+subclass+prog_if" is available but "revision" is not
> available and even it is not possible to read revision from config space
> (because e.g. due to missing permissions or limitations of win backend)?

I bet that nobody will ever check if reading of something as basic as the
revision ID succeeded. It can never fail on sane systems.

In such cases, just return a default value and log a warning.

				Have a nice fortnight
-- 
Martin `MJ' Mareš                        <mj@ucw.cz>   http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe
Nothing is smiple enough to be not screwed up.

Re: [PATCH pciutils 0/5] Support for PROGIF, REVID and SUBSYS

[Pali Rohár]

On Friday 21 January 2022 16:38:50 Martin Mareš wrote:
> > And in case "class+subclass+prog_if" is available but "revision" is not
> > available and even it is not possible to read revision from config space
> > (because e.g. due to missing permissions or limitations of win backend)?
> 
> I bet that nobody will ever check if reading of something as basic as the
> revision ID succeeded. It can never fail on sane systems.
> 
> In such cases, just return a default value and log a warning.

Ok! Sorry for a longer discussion as it is only corner case, but it can
happen (probably more often happens on windows) and therefore needs to
be somehow handled in the code.

Re: [PATCH pciutils 3/5] libpci: generic: Implement SUBSYS also for PCI_HEADER_TYPE_BRIDGE

[Pali Rohár]

On Friday 21 January 2022 15:40:49 Martin Mareš wrote:
> Hello!
> 
> > +        case PCI_HEADER_TYPE_BRIDGE:
> > +          if (pci_read_word(d, PCI_STATUS) & PCI_STATUS_CAP_LIST)
> > +            {
> > +              byte been_there[256];
> > +              int where, id;
> > +
> > +              memset(been_there, 0, 256);
> > +              where = pci_read_byte(d, PCI_CAPABILITY_LIST) & ~3;
> > +              while (where && !been_there[where]++)
> 
> Please don't. There should be a single implementation of capability list
> walking in libpci, not everybody doing his own.

Current libpci code which walks capability list is in functions
pci_scan_caps() and pci_find_cap(). But pci_find_cap() calls
pci_fill_info() (only with PCI_FILL_CAPS flag). So I'm not sure if it is
a good idea to call pci_find_cap() from pci_generic_fill_info().

Re: [PATCH pciutils 3/5] libpci: generic: Implement SUBSYS also for PCI_HEADER_TYPE_BRIDGE

[Martin Mareš]

Hello!

> Current libpci code which walks capability list is in functions
> pci_scan_caps() and pci_find_cap(). But pci_find_cap() calls
> pci_fill_info() (only with PCI_FILL_CAPS flag). So I'm not sure if it is
> a good idea to call pci_find_cap() from pci_generic_fill_info().

Still, the right solution is to change pci_fill_info() to allow recursion :)

Here is my try.

					Martin



commit 8e9299e4dde0bb73f2d571dc6e3b522f88d5765d
Author: Martin Mares <mj@ucw.cz>
Date:   Fri Jan 21 21:39:18 2022 +0100

    Simplified pci_fill_info() and friends
    
    Previously, we kept track of which fields were already filled, which
    was quite brittle.
    
    Now we keep only the set of already known fields in struct pci_dev.
    We check if the current field is needed against this information.
    
    Not only this simplifies the whole thing, but it also enables future
    back-ends to call pci_fill_info() recursively as needed.

diff --git a/lib/access.c b/lib/access.c
index b257849..9bd6989 100644
--- a/lib/access.c
+++ b/lib/access.c
@@ -1,7 +1,7 @@
 /*
  *	The PCI Library -- User Access
  *
- *	Copyright (c) 1997--2018 Martin Mares <mj@ucw.cz>
+ *	Copyright (c) 1997--2022 Martin Mares <mj@ucw.cz>
  *
  *	Can be freely distributed and used under the terms of the GNU GPL.
  */
@@ -198,7 +198,7 @@ pci_fill_info_v35(struct pci_dev *d, int flags)
       pci_reset_properties(d);
     }
   if (uflags & ~d->known_fields)
-    d->known_fields |= d->methods->fill_info(d, flags & ~d->known_fields);
+    d->methods->fill_info(d, uflags);
   return d->known_fields;
 }
 
diff --git a/lib/caps.c b/lib/caps.c
index c3b9180..3c025a9 100644
--- a/lib/caps.c
+++ b/lib/caps.c
@@ -80,17 +80,16 @@ pci_scan_ext_caps(struct pci_dev *d)
   while (where);
 }
 
-unsigned int
+void
 pci_scan_caps(struct pci_dev *d, unsigned int want_fields)
 {
-  if ((want_fields & PCI_FILL_EXT_CAPS) && !(d->known_fields & PCI_FILL_CAPS))
+  if (want_fields & PCI_FILL_EXT_CAPS)
     want_fields |= PCI_FILL_CAPS;
 
-  if (want_fields & PCI_FILL_CAPS)
+  if (want_fill(d, want_fields, PCI_FILL_CAPS))
     pci_scan_trad_caps(d);
-  if (want_fields & PCI_FILL_EXT_CAPS)
+  if (want_fill(d, want_fields, PCI_FILL_EXT_CAPS))
     pci_scan_ext_caps(d);
-  return want_fields;
 }
 
 void
diff --git a/lib/fbsd-device.c b/lib/fbsd-device.c
index cffab69..396ff1d 100644
--- a/lib/fbsd-device.c
+++ b/lib/fbsd-device.c
@@ -159,7 +159,7 @@ fbsd_scan(struct pci_access *a)
   free(matches);
 }
 
-static int
+static void
 fbsd_fill_info(struct pci_dev *d, int flags)
 {
   struct pci_conf_io conf;
@@ -200,16 +200,14 @@ fbsd_fill_info(struct pci_dev *d, int flags)
       d->access->error("fbsd_fill_info: ioctl(PCIOCGETCONF) failed: %s", strerror(errno));
     }
 
-  if (flags & PCI_FILL_IDENT)
+  if (want_fill(d, flags, PCI_FILL_IDENT))
     {
       d->vendor_id = match.pc_vendor;
       d->device_id = match.pc_device;
     }
-  if (flags & PCI_FILL_CLASS)
-    {
-      d->device_class = (match.pc_class << 8) | match.pc_subclass;
-    }
-  if (flags & (PCI_FILL_BASES | PCI_FILL_SIZES))
+  if (want_fill(d, flags, PCI_FILL_CLASS))
+    d->device_class = (match.pc_class << 8) | match.pc_subclass;
+  if (want_fill(d, flags PCI_FILL_BASES | PCI_FILL_SIZES))
     {
       d->rom_base_addr = 0;
       d->rom_size = 0;
@@ -242,9 +240,6 @@ fbsd_fill_info(struct pci_dev *d, int flags)
 	    }
 	}
     }
-
-  return flags & (PCI_FILL_IDENT | PCI_FILL_CLASS | PCI_FILL_BASES |
-		  PCI_FILL_SIZES);
 }
 
 static int
diff --git a/lib/generic.c b/lib/generic.c
index ef9e2a3..e80158f 100644
--- a/lib/generic.c
+++ b/lib/generic.c
@@ -1,7 +1,7 @@
 /*
  *	The PCI Library -- Generic Direct Access Functions
  *
- *	Copyright (c) 1997--2000 Martin Mares <mj@ucw.cz>
+ *	Copyright (c) 1997--2022 Martin Mares <mj@ucw.cz>
  *
  *	Can be freely distributed and used under the terms of the GNU GPL.
  */
@@ -74,39 +74,36 @@ pci_generic_scan(struct pci_access *a)
   pci_generic_scan_bus(a, busmap, 0);
 }
 
-unsigned int
+static int
+get_hdr_type(struct pci_dev *d)
+{
+  if (d->hdrtype < 0)
+    d->hdrtype = pci_read_byte(d, PCI_HEADER_TYPE) & 0x7f;
+  return d->hdrtype;
+}
+
+void
 pci_generic_fill_info(struct pci_dev *d, unsigned int flags)
 {
   struct pci_access *a = d->access;
-  unsigned int done = 0;
 
-  if ((flags & (PCI_FILL_BASES | PCI_FILL_ROM_BASE)) && d->hdrtype < 0)
-    d->hdrtype = pci_read_byte(d, PCI_HEADER_TYPE) & 0x7f;
-
-  if (flags & PCI_FILL_IDENT)
+  if (want_fill(d, flags, PCI_FILL_IDENT))
     {
       d->vendor_id = pci_read_word(d, PCI_VENDOR_ID);
       d->device_id = pci_read_word(d, PCI_DEVICE_ID);
-      done |= PCI_FILL_IDENT;
     }
 
-  if (flags & PCI_FILL_CLASS)
-    {
-      d->device_class = pci_read_word(d, PCI_CLASS_DEVICE);
-      done |= PCI_FILL_CLASS;
-    }
+  if (want_fill(d, flags, PCI_FILL_CLASS))
+    d->device_class = pci_read_word(d, PCI_CLASS_DEVICE);
 
-  if (flags & PCI_FILL_IRQ)
-    {
-      d->irq = pci_read_byte(d, PCI_INTERRUPT_LINE);
-      done |= PCI_FILL_IRQ;
-    }
+  if (want_fill(d, flags, PCI_FILL_IRQ))
+    d->irq = pci_read_byte(d, PCI_INTERRUPT_LINE);
 
-  if (flags & PCI_FILL_BASES)
+  if (want_fill(d, flags, PCI_FILL_BASES))
     {
       int cnt = 0, i;
       memset(d->base_addr, 0, sizeof(d->base_addr));
-      switch (d->hdrtype)
+      switch (get_hdr_type(d))
 	{
 	case PCI_HEADER_TYPE_NORMAL:
 	  cnt = 6;
@@ -148,14 +145,13 @@ pci_generic_fill_info(struct pci_dev *d, unsigned int flags)
 		}
 	    }
 	}
-      done |= PCI_FILL_BASES;
     }
 
-  if (flags & PCI_FILL_ROM_BASE)
+  if (want_fill(d, flags, PCI_FILL_ROM_BASE))
     {
       int reg = 0;
       d->rom_base_addr = 0;
-      switch (d->hdrtype)
+      switch (get_hdr_type(d))
 	{
 	case PCI_HEADER_TYPE_NORMAL:
 	  reg = PCI_ROM_ADDRESS;
@@ -170,13 +166,9 @@ pci_generic_fill_info(struct pci_dev *d, unsigned int flags)
 	  if (u != 0xffffffff)
 	    d->rom_base_addr = u;
 	}
-      done |= PCI_FILL_ROM_BASE;
     }
 
-  if (flags & (PCI_FILL_CAPS | PCI_FILL_EXT_CAPS))
-    done |= pci_scan_caps(d, flags);
-
-  return done;
+  pci_scan_caps(d, flags);
 }
 
 static int
diff --git a/lib/hurd.c b/lib/hurd.c
index 90cf89f..0939a20 100644
--- a/lib/hurd.c
+++ b/lib/hurd.c
@@ -328,26 +328,16 @@ hurd_fill_rom(struct pci_dev *d)
   d->rom_size = rom.size;
 }
 
-static unsigned int
+static void
 hurd_fill_info(struct pci_dev *d, unsigned int flags)
 {
-  unsigned int done = 0;
-
   if (!d->access->buscentric)
     {
-      if (flags & (PCI_FILL_BASES | PCI_FILL_SIZES))
-	{
-	  hurd_fill_regions(d);
-	  done |= PCI_FILL_BASES | PCI_FILL_SIZES;
-	}
-      if (flags & PCI_FILL_ROM_BASE)
-	{
-	  hurd_fill_rom(d);
-	  done |= PCI_FILL_ROM_BASE;
-	}
+      if (want_fill(d, flags, PCI_FILL_BASES | PCI_FILL_SIZES))
+	hurd_fill_regions(d);
+      if (want_fill(d, flags, PCI_FILL_ROM_BASE))
+	hurd_fill_rom(d);
     }
-
-  return done | pci_generic_fill_info(d, flags & ~done);
 }
 
 struct pci_methods pm_hurd = {
diff --git a/lib/internal.h b/lib/internal.h
index 17c27e1..942e3cb 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -1,7 +1,7 @@
 /*
  *	The PCI Library -- Internal Stuff
  *
- *	Copyright (c) 1997--2018 Martin Mares <mj@ucw.cz>
+ *	Copyright (c) 1997--2022 Martin Mares <mj@ucw.cz>
  *
  *	Can be freely distributed and used under the terms of the GNU GPL.
  */
@@ -41,7 +41,7 @@ struct pci_methods {
   void (*init)(struct pci_access *);
   void (*cleanup)(struct pci_access *);
   void (*scan)(struct pci_access *);
-  unsigned int (*fill_info)(struct pci_dev *, unsigned int flags);
+  void (*fill_info)(struct pci_dev *, unsigned int flags);
   int (*read)(struct pci_dev *, int pos, byte *buf, int len);
   int (*write)(struct pci_dev *, int pos, byte *buf, int len);
   int (*read_vpd)(struct pci_dev *, int pos, byte *buf, int len);
@@ -52,7 +52,7 @@ struct pci_methods {
 /* generic.c */
 void pci_generic_scan_bus(struct pci_access *, byte *busmap, int bus);
 void pci_generic_scan(struct pci_access *);
-unsigned int pci_generic_fill_info(struct pci_dev *, unsigned int flags);
+void pci_generic_fill_info(struct pci_dev *, unsigned int flags);
 int pci_generic_block_read(struct pci_dev *, int pos, byte *buf, int len);
 int pci_generic_block_write(struct pci_dev *, int pos, byte *buf, int len);
 
@@ -75,6 +75,18 @@ int pci_fill_info_v33(struct pci_dev *, int flags) VERSIONED_ABI;
 int pci_fill_info_v34(struct pci_dev *, int flags) VERSIONED_ABI;
 int pci_fill_info_v35(struct pci_dev *, int flags) VERSIONED_ABI;
 
+static inline int want_fill(struct pci_dev *d, unsigned want_fields, unsigned int try_fields)
+{
+  want_fields &= try_fields;
+  if ((d->known_fields & want_fields) == want_fields)
+    return 0;
+  else
+    {
+      d->known_fields |= try_fields;
+      return 1;
+    }
+}
+
 struct pci_property {
   struct pci_property *next;
   u32 key;
@@ -89,7 +101,7 @@ int pci_set_param_internal(struct pci_access *acc, char *param, char *val, int c
 void pci_free_params(struct pci_access *acc);
 
 /* caps.c */
-unsigned int pci_scan_caps(struct pci_dev *, unsigned int want_fields);
+void pci_scan_caps(struct pci_dev *, unsigned int want_fields);
 void pci_free_caps(struct pci_dev *);
 
 extern struct pci_methods pm_intel_conf1, pm_intel_conf2, pm_linux_proc,
diff --git a/lib/sysfs.c b/lib/sysfs.c
index fb64241..4b8b2cd 100644
--- a/lib/sysfs.c
+++ b/lib/sysfs.c
@@ -288,11 +288,9 @@ sysfs_fill_slots(struct pci_access *a)
   closedir(dir);
 }
 
-static unsigned int
+static void
 sysfs_fill_info(struct pci_dev *d, unsigned int flags)
 {
-  unsigned int done = 0;
-
   if (!d->access->buscentric)
     {
       /*
@@ -300,61 +298,45 @@ sysfs_fill_info(struct pci_dev *d, unsigned int flags)
        *  the kernel's view, which has regions and IRQs remapped and other fields
        *  (most importantly classes) possibly fixed if the device is known broken.
        */
-      if (flags & PCI_FILL_IDENT)
+      if (want_fill(d, flags, PCI_FILL_IDENT))
 	{
 	  d->vendor_id = sysfs_get_value(d, "vendor", 1);
 	  d->device_id = sysfs_get_value(d, "device", 1);
-	  done |= PCI_FILL_IDENT;
 	}
-      if (flags & PCI_FILL_CLASS)
-	{
-	  d->device_class = sysfs_get_value(d, "class", 1) >> 8;
-	  done |= PCI_FILL_CLASS;
-	}
-      if (flags & PCI_FILL_IRQ)
-	{
+      if (want_fill(d, flags, PCI_FILL_CLASS))
+	d->device_class = sysfs_get_value(d, "class", 1) >> 8;
+      if (want_fill(d, flags, PCI_FILL_IRQ))
 	  d->irq = sysfs_get_value(d, "irq", 1);
-	  done |= PCI_FILL_IRQ;
-	}
-      if (flags & (PCI_FILL_BASES | PCI_FILL_ROM_BASE | PCI_FILL_SIZES | PCI_FILL_IO_FLAGS))
-	{
+      if (want_fill(d, flags, PCI_FILL_BASES | PCI_FILL_ROM_BASE | PCI_FILL_SIZES | PCI_FILL_IO_FLAGS))
 	  sysfs_get_resources(d);
-	  done |= PCI_FILL_BASES | PCI_FILL_ROM_BASE | PCI_FILL_SIZES | PCI_FILL_IO_FLAGS;
-	}
     }
 
-  if (flags & PCI_FILL_PHYS_SLOT)
+  if (want_fill(d, flags, PCI_FILL_PHYS_SLOT))
     {
       struct pci_dev *pd;
       sysfs_fill_slots(d->access);
       for (pd = d->access->devices; pd; pd = pd->next)
 	pd->known_fields |= PCI_FILL_PHYS_SLOT;
-      done |= PCI_FILL_PHYS_SLOT;
     }
 
-  if (flags & PCI_FILL_MODULE_ALIAS)
+  if (want_fill(d, flags, PCI_FILL_MODULE_ALIAS))
     {
       char buf[OBJBUFSIZE];
       if (sysfs_get_string(d, "modalias", buf, 0))
 	d->module_alias = pci_set_property(d, PCI_FILL_MODULE_ALIAS, buf);
-      done |= PCI_FILL_MODULE_ALIAS;
     }
 
-  if (flags & PCI_FILL_LABEL)
+  if (want_fill(d, flags, PCI_FILL_LABEL))
     {
       char buf[OBJBUFSIZE];
       if (sysfs_get_string(d, "label", buf, 0))
 	d->label = pci_set_property(d, PCI_FILL_LABEL, buf);
-      done |= PCI_FILL_LABEL;
     }
 
-  if (flags & PCI_FILL_NUMA_NODE)
-    {
-      d->numa_node = sysfs_get_value(d, "numa_node", 0);
-      done |= PCI_FILL_NUMA_NODE;
-    }
+  if (want_fill(d, flags, PCI_FILL_NUMA_NODE))
+    d->numa_node = sysfs_get_value(d, "numa_node", 0);
 
-  if (flags & PCI_FILL_IOMMU_GROUP)
+  if (want_fill(d, flags, PCI_FILL_IOMMU_GROUP))
     {
       char *group_link = sysfs_deref_link(d, "iommu_group");
       if (group_link)
@@ -362,10 +344,9 @@ sysfs_fill_info(struct pci_dev *d, unsigned int flags)
           pci_set_property(d, PCI_FILL_IOMMU_GROUP, basename(group_link));
           free(group_link);
         }
-      done |= PCI_FILL_IOMMU_GROUP;
     }
 
-  if (flags & PCI_FILL_DT_NODE)
+  if (want_fill(d, flags, PCI_FILL_DT_NODE))
     {
       char *node = sysfs_deref_link(d, "of_node");
       if (node)
@@ -373,10 +354,9 @@ sysfs_fill_info(struct pci_dev *d, unsigned int flags)
 	  pci_set_property(d, PCI_FILL_DT_NODE, node);
 	  free(node);
 	}
-      done |= PCI_FILL_DT_NODE;
     }
 
-  return done | pci_generic_fill_info(d, flags & ~done);
+  pci_generic_fill_info(d, flags);
 }
 
 /* Intent of the sysfs_setup() caller */