On 07/31/10 - 11:18:05AM, Chris Wright wrote:
diff --git a/src/util/pci.c b/src/util/pci.c index 26d55b8..f2890bd 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -283,6 +283,7 @@ pciIterDevices(pciIterPredicate predicate, DIR *dir; struct dirent *entry; int ret = 0; + int rc;
*matched = NULL;
@@ -322,11 +323,20 @@ pciIterDevices(pciIterPredicate predicate, break; }
- if (predicate(dev, check, data)) { + rc = predicate(dev, check, data); + if (rc < 0) { + /* the predicate returned an error, bail */ + pciFreeDevice(check); + ret = -1; + break; + } + else if (rc == 1) {
CodingStyle?
$ git grep "else {" | wc -l 1150 $ git grep "else {" | grep -v } | wc -l 67
Guess it's not unprecedented, just rare ;)
Heh, yeah, that's a habit of mine. libvirt does tend to do the other style but *shrug*
VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, check->name); *matched = check; + ret = 1; break; } + pciFreeDevice(check); } closedir(dir); @@ -510,10 +520,11 @@ pciBusContainsActiveDevices(pciDevice *dev,
/* Is @check the parent of @dev ? */ static int -pciIsParent(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED) +pciIsParent(pciDevice *dev, pciDevice *check, void *data) { uint16_t device_class; uint8_t header_type, secondary, subordinate; + pciDevice **best = data;
if (dev->domain != check->domain) return 0; @@ -533,16 +544,54 @@ pciIsParent(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED)
VIR_DEBUG("%s %s: found parent device %s", dev->id, dev->name, check->name);
- /* No, it's superman! */ - return (dev->bus >= secondary && dev->bus <= subordinate); + /* if the secondary bus exactly equals the device's bus, then we found + * the direct parent. No further work is necessary + */ + if (dev->bus == secondary) + return 1;
+ + /* otherwise, SRIOV allows VFs to be on different busses then their PFs. + * In this case, what we need to do is look for the "best" match; i.e. + * the most restrictive match that still satisfies all of the conditions. + */ + if (dev->bus > secondary && dev->bus <= subordinate) { + if (*best == NULL) { + *best = pciGetDevice(check->domain, check->bus, check->slot, + check->function); + if (*best == NULL) + return -1;
too bad it's not refcounted, then we'd never hit a failure case here
Yeah, this is the part of the patch I'm least happy about, but this should do the right thing.
+ } + else {
CodingStyle?
+ /* OK, we had already recorded a previous "best" match for the + * parent. See if the current device is more restrictive than the + * best, and if so, make it the new best + */ + if (secondary > pciRead8(*best, PCI_SECONDARY_BUS)) {
forgot it's not cached, at least this isn't a performance senstitive path
+ pciFreeDevice(*best); + *best = pciGetDevice(check->domain, check->bus, check->slot, + check->function); + if (*best == NULL) + return -1; + } + } + }
Yes, should get the right match.
+ + return 0; }
-static pciDevice * -pciGetParentDevice(pciDevice *dev) +static int +pciGetParentDevice(pciDevice *dev, pciDevice **parent) { - pciDevice *parent = NULL; - pciIterDevices(pciIsParent, dev, &parent, NULL); - return parent; + pciDevice *best = NULL; + int ret; + + *parent = NULL; + ret = pciIterDevices(pciIsParent, dev, parent, &best); + if (ret == 1) + pciFreeDevice(best); + else if (ret == 0) + *parent = best; + return ret;
I think we could keep the old interface, but that's really just splitting hairs, and hey...you did the work already ;)
Well, I really need the "int" return value here to distinguish the error case, which is why I ended up changing it here. It's an internal interface, so I'm not too sad :).
}
/* Secondary Bus Reset is our sledgehammer - it resets all @@ -570,7 +619,8 @@ pciTrySecondaryBusReset(pciDevice *dev, }
/* Find the parent bus */ - parent = pciGetParentDevice(dev); + if (pciGetParentDevice(dev, &parent) < 0) + return -1; if (!parent) { pciReportError(VIR_ERR_NO_SUPPORT, _("Failed to find parent device for %s"), @@ -1377,7 +1427,8 @@ pciDeviceIsBehindSwitchLackingACS(pciDevice *dev) { pciDevice *parent;
- parent = pciGetParentDevice(dev); + if (pciGetParentDevice(dev, &parent) < 0) + return -1; if (!parent) { /* if we have no parent, and this is the root bus, ACS doesn't come * into play since devices on the root bus can't P2P without going @@ -1400,6 +1451,7 @@ pciDeviceIsBehindSwitchLackingACS(pciDevice *dev) do { pciDevice *tmp; int acs; + int ret;
acs = pciDeviceDownstreamLacksACS(parent);
@@ -1412,8 +1464,10 @@ pciDeviceIsBehindSwitchLackingACS(pciDevice *dev) }
tmp = parent; - parent = pciGetParentDevice(parent); + ret = pciGetParentDevice(parent, &parent); pciFreeDevice(tmp); + if (ret < 0) + return -1; } while (parent);
return 0;
Acked-by: Chris Wright <chrisw@redhat.com>
Thanks, I've pushed this now. -- Chris Lalancette