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? > > 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 sturct 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 distinguihsed by checking pci_dev_is_added(). > > 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. > > > 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 ..
