Messages in this thread Patch in this message | | | Date | Thu, 30 Jun 2022 16:50:18 +0200 | Subject | Re: [PATCH v6 1/5] PCI: Clean up pci_scan_slot() | From | Pierre Morel <> |
| |
On 6/30/22 15:48, Niklas Schnelle wrote: > On Thu, 2022-06-30 at 14:40 +0200, Pierre Morel wrote: >> >> On 6/28/22 16:30, 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> >>> --- >>> drivers/pci/probe.c | 38 +++++++++++++++++++------------------- >>> 1 file changed, 19 insertions(+), 19 deletions(-) >>> >>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>> index 17a969942d37..b05d0ed83a24 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; >>> } >>> >>> - /* dev may be NULL for non-contiguous multifunction devices */ >>> - if (!dev || dev->multifunction) >>> - return (fn + 1) % 8; >>> + if (fn >= 7) >>> + return -ENODEV; >>> + /* only multifunction devices may have more functions */ >>> + if (dev && !dev->multifunction) >>> + return -ENODEV; >>> >>> - return 0; >>> + return fn + 1; >> >> No more % 8 ? >> Even it disapear later shouldn't we keep it ? > > The "% 8" became unnecessary due to the explicit "if (fn >= 7)" > above. > The original "% 8" did what I referred to in the commit message with > "It [the function] also signals that no further functions need to be > scanned by returning 0 via wraparound and this is a valid function > number.". Instead we now explicitly return -ENODEV in this case.
Yes it goes with it. With this code next_fn returns -ENODEV for fn = 8 instead of previously returning 1. (If I am right)
With the previous code, did we assume that next_fn is never called with fn > 7? I guess yes as we test pci_ari_enabled first and without ARI we do not have more than 7 more functions. is it right?
For what I think this new code seems better as it does not make the assumption that it get called with fn < 8.
> >> >> >> >>> } >>> >>> 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) >>> >> >> Otherwise LGTM >> > > Thanks for taking a look! >
-- Pierre Morel IBM Lab Boeblingen
| |