Message-ID: <f942bf2aa967744fc919a95ceb4acf8ac7a53aa6.camel@linux.ibm.com>
Date:   Fri, 22 Apr 2022 13:16:24 +0200
From:   Niklas Schnelle <schnelle@...ux.ibm.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Jan Kiszka <jan.kiszka@...mens.com>,
        Matthew Rosato <mjrosato@...ux.ibm.com>,
        Pierre Morel <pmorel@...ux.ibm.com>,
        linux-kernel@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        linux-s390@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v3 1/4] PCI: Clean up pci_scan_slot()

On Thu, 2022-04-21 at 12:09 -0500, Bjorn Helgaas wrote:
> On Thu, Apr 21, 2022 at 11:27:42AM +0200, Niklas Schnelle wrote:
> > On Wed, 2022-04-20 at 21:14 -0500, Bjorn Helgaas wrote:
> > > On Tue, Apr 19, 2022 at 12:28:00PM +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 duplicating the code from the scan loop and
> > > > splitting the condition that the first function exits from it being
> > > > multifunction which is tested in next_fn().
> > > > 
> > > > Furthermore the non ARI branch of next_fn() mixes the case that
> > > > multifunction devices may have non-contiguous function ranges and dev
> > > > may thus be NULL with the multifunction requirement. It also signals
> > > > that no further functions need to be scanned by returning 0 which is
> > > > a valid function number.
> > > > 
> > > > Improve upon this by moving all conditions for having to scan for more
> > > > functions into next_fn() and make them obvious and commented.
> > > > 
> > > > 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.
> > > > 
> > > > No functional change is intended.
> > > > 
> > > > Signed-off-by: Niklas Schnelle <schnelle@...ux.ibm.com>
> > > > ---
> > > >  drivers/pci/probe.c | 41 +++++++++++++++++++----------------------
> > > >  1 file changed, 19 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 17a969942d37..389aa1f9cb2c 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -2579,33 +2579,35 @@ 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;
> > > >  	unsigned int next_fn;
> > > >  
> > > > -	if (pci_ari_enabled(bus)) {
> > > > -		if (!dev)
> > > > -			return 0;
> > > > +	if (dev && pci_ari_enabled(bus)) {
> > > 
> > > I think this would be easier to verify if we kept the explicit error
> > > return, e.g.,
> > > 
> > >   if (pci_ari_enabled(bus)) {
> > >     if (!dev)
> > >       return -ENODEV;
> > >     pos = pci_find_ext_capability(...);
> > > 
> > > Otherwise we have to sort through the !dev cases below.  I guess
> > > -ENODEV would come from either the "!fn && !dev" case or the "fn > 6"
> > > case, but it's not obvious to me that those are equivalent to the
> > > previous code.
> > 
> > We could keep this the same for this patch but I think for jailhouse
> > (patch 2) we need the "!dev" case not to fail here such that we can
> > handle the missing function 0 below even if ARI is enabled. For s390
> > this doesn't currently matter because pci_ari_enabled(bus) is always
> > false but I assumed that this isn't necessarily so for jailhouse. I
> > sent a follow up mail on a slight behavior change I can think of for
> > this case for v2 but forgot to send it also for v3. Quoted below:
> 
> I think it would be good to make the first patch change as little as
> possible to make it easier to analyze, then possibly test for
> hypervisor when changing this behavior.

Yes makes sense, in my current local version I've kept the direct
return here. I think I've also found a way to keep this even for the
isolated PCI function case. See below.

> 
> > > > -	/* dev may be NULL for non-contiguous multifunction devices */
> > > > -	if (!dev || dev->multifunction)
> > > > -		return (fn + 1) % 8;
> > > > -
> > > > -	return 0;
> > > > +	/* only multifunction devices may have more functions */
> > > > +	if (dev && !dev->multifunction)
> > > > +		return -ENODEV;
> > > 
> > > I don't understand why the "!dev || dev->multifunction" test needs to
> > > change.  Isn't that valid even in the hypervisor case?  IIUC, you want
> > > to return success in some cases that currently return failure, so this
> > > case that was already success should be fine as it was.
> > 
> > This isn't a change to the test. It's the negation of the logical
> > condition *and* a switch of the branches i.e. keeps the overall
> > behavior exactly the same. The equivalence is !(!A || B) == (A && !B).
> 
> I see the Boolean equivalence, but it's difficult to verify that the
> consequences are equivalent because the new code has the extra "!fn &&
> !dev" test in the middle.

Ok. I would argue that the "fn == 0 && !dev" is just the moved "!dev"
check for the initial pci_scan_single_device() that previously happened
outside the loop. With the modulo gone I can't think of any other way
to get fn == 0 but in the first iteration. 

But you are right, the extra test introduces some extra churn in
next_fn(). I think we can get rid of that new condition such that
next_fn() is more easily verifiable. See below.

> 
> > There are two reasons I did this.
> > 
> > 1. I find (!dev || dev->multifunction) to be much harder to grasp than
> > (dev && !dev->multifunction).
> > 
> > 2. The whole next_fn() in my opinion becomes easier to read if it bails
> > for all bad cases early and the "this is the next fn" is the final
> > return if we didn't bail. This becomes even more true as another
> > condition is added in patch 2.
> 
> Fair enough, and I agree that "this is the next fn" is a nice final
> return.  In general I think it's good to return either an error or the
> next fn as soon as it is known.  It makes it harder to analyze if the
> return value has already been determined but we have to mentally pass
> over subsequent tests that don't affect it.

I agree and I think we can get this with the transformed cases too.

> 
> > > Is this because "(fn + 1) % 8" may be zero, which previously
> > > terminated the loop, but now it doesn't because "fn == 0" is the
> > > *first* execution of the loop?
> > 
> > Yes with function 0 handled in the loop we can't use 0 as the
> > termination indication. Also I find it generally weird to use a wrap
> > around for this.
> 
> Yes, I agree that's weird.  Usually I prefer "for" loops over
> "do ...  while", but this might be a case where it makes sense --
> we *always* want to call pci_scan_single_device() once, and
> "do ... while" would accomplish that without any fuss.  It might even
> allow us to keep the 0 return value as the termination condition,
> which would be nice because fn could stay unsigned and it would reduce
> the size of this patch.

To me the 0 return is part of the weirdness as it is a valid fn value,
so returning it from next_fn() would naturally communicate that the
next fn is fn 0 not that there are no more functions.

It feels like making next_fn() more obvious is worth the larger patch.
I think us having to convince ourselves of these details is testament
that it currently is very hard to understand the interactions here
while the new conditions for stopping the scan are each almost obvious.
So I think if we can convince ourselves that the new code is exactly
equivalent which I believe it is when keeping the check in the ARI
path, then that is what makes the patch save.

> 
> I'm hoping we can end up with something like this:
> 
>   unsigned int next_fn(bus, dev, fn, mf)
>   {
>     if (ari(bus)) {
>       if (!dev)
> 	return 0;
>       return PCI_ARI_CAP_NFN();
>     }
> 
>     if (fn >= 7)
>       return 0;
> 
>     if (mf)
>       return fn + 1;
> 
>     if (hypervisor())
>       return fn + 1;

Ooh, just realized that my series changes the behavior for jailhouse
when the passed through device is not multifunction. In the existing
code pci_scan_single_device() is called for all devfn irrespective of
whether the first function found has dev->multifunction set after
scanning it. I'm not sure if that would happen if e.g. we have multiple
SR-IOV VFs but not the PF.

> 
>     return 0;
>   }
> 
>   int pci_scan_slot(...)
>   {
>     unsigned int fn = 0, mf = 0;
> 
>     do {
>       dev = pci_scan_single_device(bus, devfn + fn);
>       if (dev && dev->multifunction)
> 	mf = 1;
>       fn = next_fn(dev, fn, mf);
>     } while (fn);
>   }
> 
> This would be minimal change to next_fn(): just add the "mf"
> parameter, which removes a lot of the confusing "dev" and "!dev"
> testing, and add the "fn >= 7" to remove the implicit "% 8 == 0"
> failure case.

The extra mf parameter feels a bit superflous as we already have dev-
>multifunction and then would just move the "dev && (!)dev-
>multifunction" test out of next_fn(). 

To me this doesn't look like less of a change to next_fn() either. That
said, it gave me an idea. One way to change next_fn() less is to keep
the "fn == 0 && !dev" test out of it and in pci_scan_slot(). That way
there are no new conditions in next_fn() and the existing conditions
can be transformed as proposed without mixing in new stuff.

With that the scan loop would look something like:

  int pci_scan_slot(...)
  {
    int fn = 0, nr =0 ;

    do {
      dev = pci_scan_single_device(bus, devfn + fn);
      if (dev) {
         ...
      } else if (fn == 0) {
        /* missing function 0*/
        break;
      }
      fn = next_fn(bus, dev, fn);
    } while (fn);
  }

Even better this allows us to keep the "!dev" check in the ARI case as
we don't have to handle the missing function 0 in next_fn().

Let me sent you this variant before we abandon the -ENODEV return and
condition transforms.

> 
> Then the jailhouse/s390 patch would trivially add the new hypervisor
> case, which is clearly separated from everything else.
> 
> > > If so, I wonder if we could avoid that case by adding:
> > > 
> > >   if (fn >= 7)
> > >     return -ENODEV;
> > > 
> > > at the very beginning.  Maybe that would allow a more trivial patch
> > > that just changed the error return from 0 to -ENODEV, i.e., leaving
> > > all the logic in next_fn() unchanged?
> > 
> > I think this is equivalent to the ternary at the return. Both return
> > -ENODEV for fn >= 7. I do like your idea better though as it keeps with
> > the scheme of my point 2 above and ternaries are ever so slightly
> > harder to read.
> 
> Oops, I don't think we can do this directly because in the ARI case,
> fn is basically 8 bits wide so can be 0-255.
> 
> Bjorn

True, we really do need to keep the ARI case separate.


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists