| Message ID | 20220628143100.3228092-2-schnelle@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | PCI: Rework pci_scan_slot() and isolated PCI functions | expand |
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 ? > } > > 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
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. > > > > > } > > > > 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!
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! >
On Thu, 2022-06-30 at 16:50 +0200, Pierre Morel wrote: > > > > > 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. > The fn value in this case iterates through the least significant 3 bits of the geographical PCI address so yes this limits it to 7 functions. My main qualm with the old code was that returning 0 for the end is ambiguous because that is also a valid devfn.
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; } 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(-)