From 82c06b47dea5a38075ce9d56f743360bc47b4c78 Mon Sep 17 00:00:00 2001 From: Martin Mares Date: Mon, 25 May 2020 15:10:07 +0200 Subject: [PATCH] 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. --- TODO | 2 + lib/access.c | 7 +-- lib/generic.c | 27 +++++++++-- lib/hurd.c | 126 +++++++++++++++++++++++++++---------------------- lib/internal.h | 4 +- lib/pci.h | 12 ++++- lib/sysfs.c | 70 ++++++++++++++++++--------- 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 *) ®ions; + 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 *) ®ions != buf) { - buf = (char *) ®ions; - size = sizeof(regions); - - err = pci_get_dev_regions(device_port, &buf, &size); - if (err) - return err; - - if ((char *) ®ions != 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(®ions, 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(®ions, 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 */ -- 2.39.2