| Message ID | 20220505083839.1315705-2-schnelle@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Rework pci_scan_slot() and isolated PCI functions | expand |
On Thu, 2022-05-05 at 10:38 +0200, Niklas Schnelle wrote: > While determining the next PCI function is factored out of > pci_scan_slot() into next_fn() the former still handles the first > function as a special case. This duplicates the code from the scan loop. > > Furthermore the non ARI branch of next_fn() is generally hard to > understand and especially the check for multifunction devices is hidden > in the handling of NULL devices for non-contiguous multifunction. It > also signals that no further functions need to be scanned by returning > 0 via wraparound and this is a valid function number. > > Improve upon this by transforming the conditions in next_fn() to be > easier to understand. > > By changing next_fn() to return -ENODEV instead of 0 when there is no > next function we can then handle the initial function inside the loop > and deduplicate the shared handling. This also makes it more explicit > that only function 0 must exist. > > No functional change is intended. > > Cc: Jan Kiszka <jan.kiszka@siemens.com> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> > --- Friendly ping :-)
On Thu, May 12, 2022 at 04:56:42PM +0200, Niklas Schnelle wrote: > On Thu, 2022-05-05 at 10:38 +0200, Niklas Schnelle wrote: > > While determining the next PCI function is factored out of > > pci_scan_slot() into next_fn() the former still handles the first > > function as a special case. This duplicates the code from the scan loop. > > > > Furthermore the non ARI branch of next_fn() is generally hard to > > understand and especially the check for multifunction devices is hidden > > in the handling of NULL devices for non-contiguous multifunction. It > > also signals that no further functions need to be scanned by returning > > 0 via wraparound and this is a valid function number. > > > > Improve upon this by transforming the conditions in next_fn() to be > > easier to understand. > > > > By changing next_fn() to return -ENODEV instead of 0 when there is no > > next function we can then handle the initial function inside the loop > > and deduplicate the shared handling. This also makes it more explicit > > that only function 0 must exist. > > > > No functional change is intended. > > > > Cc: Jan Kiszka <jan.kiszka@siemens.com> > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> > > --- > > Friendly ping :-) Thanks and sorry for the delay. I'm off today for my daughter's wedding reception but will get back to it next week. Just to expose some of my thought process (and not to request more work from you!) I've been wondering whether b1bd58e448f2 ("PCI: Consolidate "next-function" functions") is really causing us more trouble than it's worth. In some ways that makes the single next-function harder to read. But I guess the hypervisor special case is not exactly a "next-function" thing -- it's a "keep scanning even if there's no fn 0" thing. Bjorn
On Fri, 2022-05-13 at 09:07 -0500, Bjorn Helgaas wrote: > On Thu, May 12, 2022 at 04:56:42PM +0200, Niklas Schnelle wrote: > > On Thu, 2022-05-05 at 10:38 +0200, Niklas Schnelle wrote: > > > While determining the next PCI function is factored out of > > > pci_scan_slot() into next_fn() the former still handles the first > > > function as a special case. This duplicates the code from the scan loop. > > > > > > Furthermore the non ARI branch of next_fn() is generally hard to > > > understand and especially the check for multifunction devices is hidden > > > in the handling of NULL devices for non-contiguous multifunction. It > > > also signals that no further functions need to be scanned by returning > > > 0 via wraparound and this is a valid function number. > > > > > > Improve upon this by transforming the conditions in next_fn() to be > > > easier to understand. > > > > > > By changing next_fn() to return -ENODEV instead of 0 when there is no > > > next function we can then handle the initial function inside the loop > > > and deduplicate the shared handling. This also makes it more explicit > > > that only function 0 must exist. > > > > > > No functional change is intended. > > > > > > Cc: Jan Kiszka <jan.kiszka@siemens.com> > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> > > > --- > > > > Friendly ping :-) > > Thanks and sorry for the delay. I'm off today for my daughter's > wedding reception but will get back to it next week. No worries, have a great day and congratulations! > Just to expose > some of my thought process (and not to request more work from you!) > I've been wondering whether b1bd58e448f2 ("PCI: Consolidate > "next-function" functions") is really causing us more trouble than > it's worth. In some ways that makes the single next-function harder > to read. But I guess the hypervisor special case is not exactly a > "next-function" thing -- it's a "keep scanning even if there's no fn > 0" thing. > > Bjorn Yeah I do see your point. Let's discuss next week.
On Fri, 2022-05-13 at 09:07 -0500, Bjorn Helgaas wrote: > On Thu, May 12, 2022 at 04:56:42PM +0200, Niklas Schnelle wrote: > > On Thu, 2022-05-05 at 10:38 +0200, Niklas Schnelle wrote: > > > While determining the next PCI function is factored out of > > > pci_scan_slot() into next_fn() the former still handles the first > > > function as a special case. This duplicates the code from the scan loop. > > > > > > Furthermore the non ARI branch of next_fn() is generally hard to > > > understand and especially the check for multifunction devices is hidden > > > in the handling of NULL devices for non-contiguous multifunction. It > > > also signals that no further functions need to be scanned by returning > > > 0 via wraparound and this is a valid function number. > > > > > > Improve upon this by transforming the conditions in next_fn() to be > > > easier to understand. > > > > > > By changing next_fn() to return -ENODEV instead of 0 when there is no > > > next function we can then handle the initial function inside the loop > > > and deduplicate the shared handling. This also makes it more explicit > > > that only function 0 must exist. > > > > > > No functional change is intended. > > > > > > Cc: Jan Kiszka <jan.kiszka@siemens.com> > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> > > > --- > > > > Friendly ping :-) > > Thanks and sorry for the delay. I'm off today for my daughter's > wedding reception but will get back to it next week. Just to expose > some of my thought process (and not to request more work from you!) > I've been wondering whether b1bd58e448f2 ("PCI: Consolidate > "next-function" functions") is really causing us more trouble than > it's worth. In some ways that makes the single next-function harder > to read. But I guess the hypervisor special case is not exactly a > "next-function" thing -- it's a "keep scanning even if there's no fn > 0" thing. > > Bjorn I've thought again about your comment. Personally what I like about b1bd58e448f2 ("PCI: Consolidate "next-function" functions") is that it got rid of the next_fn function pointer complication. I agree though that on the other hand it removed a nice separation between the ARI and traditional cases. So I'm thinking maybe we should bring that part back. I think my patch as is makes it easier to see the equivalence to the existing code but then we could add a patch on top and turn it into the below, it's a bit more verbose but very easy to follow. static int next_ari_fn(struct pci_bus *bus, struct pci_dev *dev, int fn) { … } static int next_trad_fn(struct pci_bus *bus, struct pci_dev *dev, int fn) { if (fn >= 7) return -ENODEV; /* only multifunction devices may have more functions */ if (dev && !dev->multifunction) return -ENODEV; return fn + 1; } static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn) { if (pci_ari_enabled(bus)) { return next_ari_fn(bus, dev, fn); } return next_trad_fn(bus, dev, fn); }
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 17a969942d37..78aa1bccab2f 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2579,8 +2579,7 @@ struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn) } EXPORT_SYMBOL(pci_scan_single_device); -static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev, - unsigned int fn) +static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn) { int pos; u16 cap = 0; @@ -2588,24 +2587,26 @@ static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev, if (pci_ari_enabled(bus)) { if (!dev) - return 0; + return -ENODEV; pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI); if (!pos) - return 0; + return -ENODEV; pci_read_config_word(dev, pos + PCI_ARI_CAP, &cap); next_fn = PCI_ARI_CAP_NFN(cap); if (next_fn <= fn) - return 0; /* protect against malformed list */ + return -ENODEV; /* protect against malformed list */ return next_fn; } + if (fn >= 7) + return -ENODEV; - /* dev may be NULL for non-contiguous multifunction devices */ - if (!dev || dev->multifunction) - return (fn + 1) % 8; + /* only multifunction devices may have more functions */ + if (dev && !dev->multifunction) + return -ENODEV; - return 0; + return fn + 1; } static int only_one_child(struct pci_bus *bus) @@ -2643,26 +2644,25 @@ static int only_one_child(struct pci_bus *bus) */ int pci_scan_slot(struct pci_bus *bus, int devfn) { - unsigned int fn, nr = 0; struct pci_dev *dev; + int fn = 0, nr = 0; if (only_one_child(bus) && (devfn > 0)) return 0; /* Already scanned the entire slot */ - dev = pci_scan_single_device(bus, devfn); - if (!dev) - return 0; - if (!pci_dev_is_added(dev)) - nr++; - - for (fn = next_fn(bus, dev, 0); fn > 0; fn = next_fn(bus, dev, fn)) { + do { dev = pci_scan_single_device(bus, devfn + fn); if (dev) { if (!pci_dev_is_added(dev)) nr++; - dev->multifunction = 1; + if (fn > 0) + dev->multifunction = 1; + } else if (fn == 0) { + /* function 0 is required */ + break; } - } + fn = next_fn(bus, dev, fn); + } while (fn >= 0); /* Only one slot has PCIe device */ if (bus->self && nr)
While determining the next PCI function is factored out of pci_scan_slot() into next_fn() the former still handles the first function as a special case. This duplicates the code from the scan loop. Furthermore the non ARI branch of next_fn() is generally hard to understand and especially the check for multifunction devices is hidden in the handling of NULL devices for non-contiguous multifunction. It also signals that no further functions need to be scanned by returning 0 via wraparound and this is a valid function number. Improve upon this by transforming the conditions in next_fn() to be easier to understand. By changing next_fn() to return -ENODEV instead of 0 when there is no next function we can then handle the initial function inside the loop and deduplicate the shared handling. This also makes it more explicit that only function 0 must exist. No functional change is intended. Cc: Jan Kiszka <jan.kiszka@siemens.com> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> --- drivers/pci/probe.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)