Library: Big cleanup of pci_fill_info()

There was a lot of minor issues in the implementation of the fill_info
call-back in various back-ends. Most importantly, semantics of pci_dev->
known_fields was not formally defined and it was implemented inconsistently.

We now define known_fields as the set of fields which were already
obtained during the lifetime of the pci_dev. We never consider known
fields which are not supported by the back-end. All fields which are
unsupported by either the back-end, the OS, or the particular device,
are guaranteed to have sensible default values (0 or NULL). Also, bit
masks are always unsigned except for the signature of pci_fill_info()
which should be preferably kept stable.

All back-ends and the pci_generic_fill_info() function have been changed
to follow this semantics.

In the sysfs back-end, we read as few attributes as possible during
device initialization, so applications which use pci_get_dev() are not
slowed down unnecessarily.

In the Hurd back-end, we also respect the buscentric mode.

7 files changed, 159 insertions, 89 deletions

diff --git a/TODO b/TODO
index d9efa20..2ff52e2 100644
--- a/TODO
+++ b/TODO
@@ -1,3 +1,5 @@
+- all back-ends: fail on non-zero domain
+
 - review class names
 
 Setpci:
diff --git a/lib/access.c b/lib/access.c
index fefedf6..b257849 100644
--- a/lib/access.c
+++ b/lib/access.c
@@ -191,12 +191,13 @@ pci_reset_properties(struct pci_dev *d)
 int
 pci_fill_info_v35(struct pci_dev *d, int flags)
 {
-  if (flags & PCI_FILL_RESCAN)
+  unsigned int uflags = flags;
+  if (uflags & PCI_FILL_RESCAN)
     {
-      flags &= ~PCI_FILL_RESCAN;
+      uflags &= ~PCI_FILL_RESCAN;
       pci_reset_properties(d);
     }
-  if (flags & ~d->known_fields)
+  if (uflags & ~d->known_fields)
     d->known_fields |= d->methods->fill_info(d, flags & ~d->known_fields);
   return d->known_fields;
 }
diff --git a/lib/generic.c b/lib/generic.c
index c219592..ef9e2a3 100644
--- a/lib/generic.c
+++ b/lib/generic.c
@@ -74,22 +74,34 @@ pci_generic_scan(struct pci_access *a)
   pci_generic_scan_bus(a, busmap, 0);
 }
 
-int
-pci_generic_fill_info(struct pci_dev *d, int flags)
+unsigned int
+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)
     {
       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 (flags & PCI_FILL_IRQ)
-    d->irq = pci_read_byte(d, PCI_INTERRUPT_LINE);
+    {
+      d->irq = pci_read_byte(d, PCI_INTERRUPT_LINE);
+      done |= PCI_FILL_IRQ;
+    }
+
   if (flags & PCI_FILL_BASES)
     {
       int cnt = 0, i;
@@ -136,7 +148,9 @@ pci_generic_fill_info(struct pci_dev *d, int flags)
 		}
 	    }
 	}
+      done |= PCI_FILL_BASES;
     }
+
   if (flags & PCI_FILL_ROM_BASE)
     {
       int reg = 0;
@@ -156,10 +170,13 @@ pci_generic_fill_info(struct pci_dev *d, int flags)
 	  if (u != 0xffffffff)
 	    d->rom_base_addr = u;
 	}
+      done |= PCI_FILL_ROM_BASE;
     }
+
   if (flags & (PCI_FILL_CAPS | PCI_FILL_EXT_CAPS))
-    flags |= pci_scan_caps(d, flags);
-  return flags & ~PCI_FILL_SIZES;
+    done |= pci_scan_caps(d, flags);
+
+  return done;
 }
 
 static int
diff --git a/lib/hurd.c b/lib/hurd.c
index 8cc948b..7b3b2ae 100644
--- a/lib/hurd.c
+++ b/lib/hurd.c
@@ -271,82 +271,96 @@ hurd_write(struct pci_dev *d, int pos, byte * buf, int len)
 }
 
 /* Get requested info from the server */
-static int
-hurd_fill_info(struct pci_dev *d, int flags)
+
+static void
+hurd_fill_regions(struct pci_dev *d)
 {
-  int err, i;
+  mach_port_t device_port = *((mach_port_t *) d->aux);
   struct pci_bar regions[6];
-  struct pci_xrom_bar rom;
-  size_t size;
-  char *buf;
-  mach_port_t device_port;
+  char *buf = (char *) &regions;
+  size_t size = sizeof(regions);
 
-  device_port = *((mach_port_t *) d->aux);
+  int err = pci_get_dev_regions(device_port, &buf, &size);
+  if (err)
+    return;
 
-  if (flags & PCI_FILL_BASES)
+  if ((char *) &regions != buf)
     {
-      buf = (char *) &regions;
-      size = sizeof(regions);
-
-      err = pci_get_dev_regions(device_port, &buf, &size);
-      if (err)
-	return err;
-
-      if ((char *) &regions != buf)
+      /* Sanity check for bogus server.  */
+      if (size > sizeof(regions))
 	{
-	  /* Sanity check for bogus server.  */
-	  if (size > sizeof(regions))
-	    {
-	      vm_deallocate(mach_task_self(), (vm_address_t) buf, size);
-	      return EGRATUITOUS;
-	    }
-
-	  memcpy(&regions, buf, size);
 	  vm_deallocate(mach_task_self(), (vm_address_t) buf, size);
+	  return;
 	}
 
-      for (i = 0; i < 6; i++)
-	{
-	  if (regions[i].size == 0)
-	    continue;
+      memcpy(&regions, buf, size);
+      vm_deallocate(mach_task_self(), (vm_address_t) buf, size);
+    }
 
-	  d->base_addr[i] = regions[i].base_addr;
-	  d->base_addr[i] |= regions[i].is_IO;
-	  d->base_addr[i] |= regions[i].is_64 << 2;
-	  d->base_addr[i] |= regions[i].is_prefetchable << 3;
+  for (int i = 0; i < 6; i++)
+    {
+      if (regions[i].size == 0)
+	continue;
 
-	  if (flags & PCI_FILL_SIZES)
-	    d->size[i] = regions[i].size;
-	}
+      d->base_addr[i] = regions[i].base_addr;
+      d->base_addr[i] |= regions[i].is_IO;
+      d->base_addr[i] |= regions[i].is_64 << 2;
+      d->base_addr[i] |= regions[i].is_prefetchable << 3;
+
+      if (flags & PCI_FILL_SIZES)
+	d->size[i] = regions[i].size;
     }
+}
 
-  if (flags & PCI_FILL_ROM_BASE)
+static void
+hurd_fill_rom(struct pci_dev *d)
+{
+  struct pci_xrom_bar rom;
+  mach_port_t device_port = *((mach_port_t *) d->aux);
+  char *buf = (char *) &rom;
+  size_t size = sizeof(rom);
+
+  int err = pci_get_dev_rom(device_port, &buf, &size);
+  if (err)
+    return;
+
+  if ((char *) &rom != buf)
     {
-      /* Get rom info */
-      buf = (char *) &rom;
-      size = sizeof(rom);
-      err = pci_get_dev_rom(device_port, &buf, &size);
-      if (err)
-	return err;
-
-      if ((char *) &rom != buf)
+      /* Sanity check for bogus server.  */
+      if (size > sizeof(rom))
 	{
-	  /* Sanity check for bogus server.  */
-	  if (size > sizeof(rom))
-	    {
-	      vm_deallocate(mach_task_self(), (vm_address_t) buf, size);
-	      return EGRATUITOUS;
-	    }
-
-	  memcpy(&rom, buf, size);
 	  vm_deallocate(mach_task_self(), (vm_address_t) buf, size);
+	  return;
 	}
 
-      d->rom_base_addr = rom.base_addr;
-      d->rom_size = rom.size;
+      memcpy(&rom, buf, size);
+      vm_deallocate(mach_task_self(), (vm_address_t) buf, size);
+    }
+
+  d->rom_base_addr = rom.base_addr;
+  d->rom_size = rom.size;
+}
+
+static unsigned int
+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;
+	}
     }
 
-  return pci_generic_fill_info(d, flags);
+  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 714a1db..17c27e1 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -41,7 +41,7 @@ struct pci_methods {
   void (*init)(struct pci_access *);
   void (*cleanup)(struct pci_access *);
   void (*scan)(struct pci_access *);
-  int (*fill_info)(struct pci_dev *, int flags);
+  unsigned int (*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 *);
-int pci_generic_fill_info(struct pci_dev *, int flags);
+unsigned int 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);
 
diff --git a/lib/pci.h b/lib/pci.h
index 5a1dac5..f5274df 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -126,7 +126,7 @@ struct pci_dev {
   u8 bus, dev, func;			/* Bus inside domain, device and function */
 
   /* These fields are set by pci_fill_info() */
-  int known_fields;			/* Set of info fields already known */
+  unsigned int known_fields;		/* Set of info fields already known (see pci_fill_info()) */
   u16 vendor_id, device_id;		/* Identity of the device */
   u16 device_class;			/* PCI device class */
   int irq;				/* IRQ number */
@@ -175,6 +175,16 @@ int pci_write_block(struct pci_dev *, int pos, u8 *buf, int len) PCI_ABI;
  *
  * Some properties are stored directly in the pci_dev structure.
  * The remaining ones can be accessed through pci_get_string_property().
+ *
+ * pci_fill_info() returns the current value of pci_dev->known_fields.
+ * This is a bit mask of all fields, which were already obtained during
+ * the lifetime of the device. This includes fields which are not supported
+ * by the particular device -- in that case, the field is left at its default
+ * value, which is 0 for integer fields and NULL for pointers. On the other
+ * hand, we never consider known fields unsupported by the current back-end;
+ * such fields always contain the default value.
+ *
+ * XXX: flags and the result should be unsigned, but we do not want to break the ABI.
  */
 
 int pci_fill_info(struct pci_dev *, int flags) PCI_ABI;
diff --git a/lib/sysfs.c b/lib/sysfs.c
index 42c88c6..c302cbf 100644
--- a/lib/sysfs.c
+++ b/lib/sysfs.c
@@ -223,19 +223,6 @@ static void sysfs_scan(struct pci_access *a)
       d->bus = bus;
       d->dev = dev;
       d->func = func;
-      if (!a->buscentric)
-	{
-	  sysfs_get_resources(d);
-	  d->irq = sysfs_get_value(d, "irq", 1);
-	  /*
-	   *  We could read these faster from the config registers, but we want to give
-	   *  the kernel a chance to fix up ID's and especially classes of broken devices.
-	   */
-	  d->vendor_id = sysfs_get_value(d, "vendor", 1);
-	  d->device_id = sysfs_get_value(d, "device", 1);
-	  d->device_class = sysfs_get_value(d, "class", 1) >> 8;
-	  d->known_fields = PCI_FILL_IDENT | PCI_FILL_CLASS | PCI_FILL_IRQ | PCI_FILL_BASES | PCI_FILL_ROM_BASE | PCI_FILL_SIZES | PCI_FILL_IO_FLAGS;
-	}
       pci_link_dev(a, d);
     }
   closedir(dir);
@@ -301,35 +288,73 @@ sysfs_fill_slots(struct pci_access *a)
   closedir(dir);
 }
 
-static int
-sysfs_fill_info(struct pci_dev *d, int flags)
+static unsigned int
+sysfs_fill_info(struct pci_dev *d, unsigned int flags)
 {
-  if ((flags & PCI_FILL_PHYS_SLOT) && !(d->known_fields & PCI_FILL_PHYS_SLOT))
+  unsigned int done = 0;
+
+  if (!d->access->buscentric)
+    {
+      /*
+       *  These fields can be read from the config registers, but we want to show
+       *  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)
+	{
+	  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)
+	{
+	  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))
+	{
+	  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)
     {
       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) && !(d->known_fields & PCI_FILL_MODULE_ALIAS))
+  if (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) && !(d->known_fields & PCI_FILL_LABEL))
+  if (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->known_fields & PCI_FILL_NUMA_NODE))
-    d->numa_node = sysfs_get_value(d, "numa_node", 0);
+  if (flags & PCI_FILL_NUMA_NODE)
+    {
+      d->numa_node = sysfs_get_value(d, "numa_node", 0);
+      done |= PCI_FILL_NUMA_NODE;
+    }
 
-  if ((flags & PCI_FILL_DT_NODE) && !(d->known_fields & PCI_FILL_DT_NODE))
+  if (flags & PCI_FILL_DT_NODE)
     {
       char *node = sysfs_deref_link(d, "of_node");
       if (node)
@@ -337,9 +362,10 @@ sysfs_fill_info(struct pci_dev *d, int flags)
 	  pci_set_property(d, PCI_FILL_DT_NODE, node);
 	  free(node);
 	}
+      done |= PCI_FILL_DT_NODE;
     }
 
-  return pci_generic_fill_info(d, flags);
+  return done | pci_generic_fill_info(d, flags & ~done);
 }
 
 /* Intent of the sysfs_setup() caller */