Re: [PATCH v3] PCI: Move pci_dev_is/assign_added() to pci.h

[Bjorn Helgaas]

On Mon, Aug 23, 2021 at 12:53:39PM +0200, Niklas Schnelle wrote:
> On Fri, 2021-08-20 at 17:37 -0500, Bjorn Helgaas wrote:
> > On Tue, Jul 20, 2021 at 05:01:45PM +0200, Niklas Schnelle wrote:
> > > The helper function pci_dev_is_added() from drivers/pci/pci.h is used in
> > > PCI arch code of both s390 and powerpc leading to awkward relative
> > > includes. Move it to the global include/linux/pci.h and get rid of these
> > > includes just for that one function.
> > 
> > I agree the includes are awkward.
> > 
> > But the arch code *using* pci_dev_is_added() seems awkward, too.
> 
> See below for my interpretation why s390 has some driver like
> functionality in its arch code which isn't necessarily awkward.
> 
> Independent from that I have found pci_dev_is_added() as the only way
> deal with the case that one might be looking at a struct pci_dev
> reference that has been removed via pci_stop_and_remove_bus_device() or
> has never been fully scanned. This is quite useful when handling error
> events which on s390 are part of the adapter event mechanism shared
> with channel I/O devices.
> 
> > AFAICS, in powerpc, pci_dev_is_added() is only used by
> > pnv_pci_ioda_fixup_iov() and pseries_pci_fixup_iov_resources().  Those
> > are only called from pcibios_add_device(), which is only called from
> > pci_device_add().
> > 
> > Is it even possible for pci_dev_is_added() to be true in that path?

If the pci_dev_is_added() in powerpc is unreachable, we can remove it
and at least reduce this to an s390-only problem.

> > s390 uses pci_dev_is_added() in recover_store()
> 
> I'm actually looking into this as I'm working on an s390 implementation
> of the PCI recovery flow described in Documentation/PCI/pci-error-
> recovery.rst that would also call pci_dev_is_added() because when we
> get a platform notification of a PCI reset done by firmware it may be
> that the struct pci_dev is going away i.e. we still have a ref count
> but it is not added to the PCI bus anymore. And pci_dev_is_added() is
> the only way I've found to check for this state.
> 
> > , but I don't know what
> > that is (looks like a sysfs file, but it's not documented) or why s390
> > is the only arch that does this.
> 
> Good point about this not being documented, I'll look into adding docs.
> 
> This is a sysfs attribute that basically removes the pci_dev and re-
> adds it. This has the complication that since the attribute sits at
> /sys/bus/pci/devices/<dev>/recover it deletes its own parent directory
> which requires extra caution and means concurrent accesses block on
> pci_lock_rescan_remove() instead of a kernfs lock.
> Long story short when concurrently triggering the attribute one thread
> proceeds into the pci_lock_rescan_remove() section and does the
> removal, while others would block on pci_lock_rescan_remove(). Now when
> the threads unblock the removal is done. In this case there is a new
> struct pci_dev found in the rescan but the previously blocked threads
> still have references to the old struct pci_dev which was removed and
> as far as I could tell can only be distinguished by checking
> pci_dev_is_added().

Is this locking issue different from concurrently writing to
/sys/.../remove on other architectures?

> > Maybe we should make powerpc and s390 less special?
> 
> On s390, as I see it, the reason for this is that all of the PCI
> functionality is directly defined in the Architecture as special CPU
> instructions which are kind of hypercalls but also an ISA extension.
> 
> These instructions range from the basic PCI memory accesses (no real
> MMIO) to enumeration of the devices and on to reporting of hot-plug and
> and resets/recovery events. Importantly we do not have any kind of
> direct access to a real or virtual PCI controller and the architecture
> has no concept of a comparable entity.
> 
> So in my opinion while there is some of the functionality of a PCI
> controller in arch/s390/pci the cut off between controller
> functionality and arch support isn't clear at all and exposing PCI
> support as CPU instructions doesn't map well to the controller concept.
> 
> That said, in principle I'm open to moving some of that into
> drivers/pci/controller/ if you think that would improve things and we
> can find a good argument what should go where. One possible cut off
> would be to have arch/s390/pci/ provide wrappers to the PCI
> instructions but move all their uses to  e.g.
> drivers/pci/controller/s390/. This would of course be a major
> refactoring and none of that code would be useful on any other
> architecture but it would move a lot the accesses to PCI common code
> functionality out of the arch code.

Looks like hotplug is already in drivers/pci/hotplug/s390_pci_hpc.c.

Might be worth considering putting the other PCI core-ish code in
drivers/pci as well, though it doesn't feel urgent to me.  Maybe a
good internship or mentoring project.

I'm not sure this juggling around is worth it basically to just clean
up the include path.  The downside to me is exposing
pci_dev_is_added() to outside the PCI core, because I don't want
to encourage any other users.

> > > Signed-off-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> > > ---
> > > Since v1 (and bad v2):
> > > - Fixed accidental removal of PCI_DPC_RECOVERED, PCI_DPC_RECOVERING
> > >   defines and also move these to include/linux/pci.h
> > > 
> > >  arch/powerpc/platforms/powernv/pci-sriov.c |  3 ---
> > >  arch/powerpc/platforms/pseries/setup.c     |  1 -
> > >  arch/s390/pci/pci_sysfs.c                  |  2 --
> > >  drivers/pci/hotplug/acpiphp_glue.c         |  1 -
> > >  drivers/pci/pci.h                          | 15 ---------------
> > >  include/linux/pci.h                        | 15 +++++++++++++++
> > >  6 files changed, 15 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
> > > index 28aac933a439..2e0ca5451e85 100644
> > > --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> > > +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> > > @@ -9,9 +9,6 @@
> > >  
> > >  #include "pci.h"
> > >  
> > > -/* for pci_dev_is_added() */
> > > -#include "../../../../drivers/pci/pci.h"
> > > 
> .. snip ..
>