* [PATCH 0/1] Arch use of pci_dev_is_added()
@ 2021-09-10 14:19 Niklas Schnelle
2021-09-10 14:19 ` [PATCH 1/1] powerpc: Drop superfluous pci_dev_is_added() calls Niklas Schnelle
2021-10-11 12:06 ` [PATCH 0/1] Arch use of pci_dev_is_added() Michael Ellerman
0 siblings, 2 replies; 7+ messages in thread
From: Niklas Schnelle @ 2021-09-10 14:19 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Benjamin Herrenschmidt, Michael Ellerman, Oliver O'Halloran,
linux-pci, linux-kernel, linux-s390, linux-arch, linuxppc-dev
Hi Bjorn, Hi Michael,
In my proposal to make pci_dev_is_added() more regularly usable by arch code
you mentioned[0] that you believe the uses in arch/powerpc are not necessary
anymore. From code reading I agree and so does Oliver O'Halloran[1].
So as promised here is a patch removing them. I only compile tested this as
I don't have access to a powerpc system.
I've also looked a bit more into our use in s390 and as dicussed previously
I don't think we can cleanly get rid of the existing one in
arch/s390/pci_sysfs.c:recover_store() because we need to distinguish an already
removed pdev just by looking at the pdev itself.
As for new uses I think in the upcoming automatic recovery code we can rely on
the fact that a removed device has pdev->driver == NULL and don't need
pci_dev_is_added() but it would make things clearer. I also noticed that before
commit 44bda4b7d26e9 ("PCI: Fix is_added/is_busmaster race condition") there
was simply a pdev->is_added flag that was cleanly accessible by arch code. So
I wanted to ask for your advice.
Thanks,
Niklas
[0] https://lore.kernel.org/lkml/20210825190444.GA3593752@bjorn-Precision-5520/
[1] https://lore.kernel.org/lkml/CAOSf1CFyuf9FaeSNparj+7W0mKTPvtcM8vxjHDSFsNDC6k_7xQ@mail.gmail.com/
Niklas Schnelle (1):
powerpc: Drop superfluous pci_dev_is_added() calls
arch/powerpc/platforms/powernv/pci-sriov.c | 6 ------
arch/powerpc/platforms/pseries/setup.c | 3 +--
2 files changed, 1 insertion(+), 8 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/1] powerpc: Drop superfluous pci_dev_is_added() calls 2021-09-10 14:19 [PATCH 0/1] Arch use of pci_dev_is_added() Niklas Schnelle @ 2021-09-10 14:19 ` Niklas Schnelle 2021-09-11 11:09 ` Michael Ellerman 2021-09-14 19:31 ` Bjorn Helgaas 2021-10-11 12:06 ` [PATCH 0/1] Arch use of pci_dev_is_added() Michael Ellerman 1 sibling, 2 replies; 7+ messages in thread From: Niklas Schnelle @ 2021-09-10 14:19 UTC (permalink / raw) To: Bjorn Helgaas Cc: Benjamin Herrenschmidt, Michael Ellerman, Oliver O'Halloran, linux-pci, linux-kernel, linux-s390, linux-arch, linuxppc-dev On powerpc, pci_dev_is_added() is called as part of SR-IOV fixups that are done under pcibios_add_device() which in turn is only called in pci_device_add() whih is called when a PCI device is scanned. Now pci_dev_assign_added() is called in pci_bus_add_device() which is only called after scanning the device. Thus pci_dev_is_added() is always false and can be dropped. Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> --- arch/powerpc/platforms/powernv/pci-sriov.c | 6 ------ arch/powerpc/platforms/pseries/setup.c | 3 +-- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c index 28aac933a439..deddbb233fde 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" - /* * The majority of the complexity in supporting SR-IOV on PowerNV comes from * the need to put the MMIO space for each VF into a separate PE. Internally @@ -228,9 +225,6 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev) { - if (WARN_ON(pci_dev_is_added(pdev))) - return; - if (pdev->is_virtfn) { struct pnv_ioda_pe *pe = pnv_ioda_get_pe(pdev); diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c index f79126f16258..2188054470c1 100644 --- a/arch/powerpc/platforms/pseries/setup.c +++ b/arch/powerpc/platforms/pseries/setup.c @@ -74,7 +74,6 @@ #include <asm/hvconsole.h> #include "pseries.h" -#include "../../../../drivers/pci/pci.h" DEFINE_STATIC_KEY_FALSE(shared_processor); EXPORT_SYMBOL(shared_processor); @@ -750,7 +749,7 @@ static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev) const int *indexes; struct device_node *dn = pci_device_to_OF_node(pdev); - if (!pdev->is_physfn || pci_dev_is_added(pdev)) + if (!pdev->is_physfn) return; /*Firmware must support open sriov otherwise dont configure*/ indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL); -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] powerpc: Drop superfluous pci_dev_is_added() calls 2021-09-10 14:19 ` [PATCH 1/1] powerpc: Drop superfluous pci_dev_is_added() calls Niklas Schnelle @ 2021-09-11 11:09 ` Michael Ellerman 2021-09-13 14:58 ` Oliver O'Halloran 2021-09-14 19:31 ` Bjorn Helgaas 1 sibling, 1 reply; 7+ messages in thread From: Michael Ellerman @ 2021-09-11 11:09 UTC (permalink / raw) To: Niklas Schnelle, Bjorn Helgaas Cc: Benjamin Herrenschmidt, Oliver O'Halloran, linux-pci, linux-kernel, linux-s390, linux-arch, linuxppc-dev Niklas Schnelle <schnelle@linux.ibm.com> writes: > On powerpc, pci_dev_is_added() is called as part of SR-IOV fixups > that are done under pcibios_add_device() which in turn is only called in > pci_device_add() whih is called when a PCI device is scanned. Thanks for cleaning this up for us. > Now pci_dev_assign_added() is called in pci_bus_add_device() which is > only called after scanning the device. Thus pci_dev_is_added() is always > false and can be dropped. My only query is whether we can pin down when that changed. Oliver said: The use of pci_dev_is_added() in arch/powerpc was because in the past pci_bus_add_device() could be called before pci_device_add(). That was fixed a while ago so It should be safe to remove those calls now. I trawled back through the history a bit but I can't remember/find which commit changed that, Oliver can you remember? cheers > diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c > index 28aac933a439..deddbb233fde 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" > - > /* > * The majority of the complexity in supporting SR-IOV on PowerNV comes from > * the need to put the MMIO space for each VF into a separate PE. Internally > @@ -228,9 +225,6 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) > > void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev) > { > - if (WARN_ON(pci_dev_is_added(pdev))) > - return; > - > if (pdev->is_virtfn) { > struct pnv_ioda_pe *pe = pnv_ioda_get_pe(pdev); > > diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c > index f79126f16258..2188054470c1 100644 > --- a/arch/powerpc/platforms/pseries/setup.c > +++ b/arch/powerpc/platforms/pseries/setup.c > @@ -74,7 +74,6 @@ > #include <asm/hvconsole.h> > > #include "pseries.h" > -#include "../../../../drivers/pci/pci.h" > > DEFINE_STATIC_KEY_FALSE(shared_processor); > EXPORT_SYMBOL(shared_processor); > @@ -750,7 +749,7 @@ static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev) > const int *indexes; > struct device_node *dn = pci_device_to_OF_node(pdev); > > - if (!pdev->is_physfn || pci_dev_is_added(pdev)) > + if (!pdev->is_physfn) > return; > /*Firmware must support open sriov otherwise dont configure*/ > indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL); > -- > 2.25.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] powerpc: Drop superfluous pci_dev_is_added() calls 2021-09-11 11:09 ` Michael Ellerman @ 2021-09-13 14:58 ` Oliver O'Halloran 0 siblings, 0 replies; 7+ messages in thread From: Oliver O'Halloran @ 2021-09-13 14:58 UTC (permalink / raw) To: Michael Ellerman Cc: Niklas Schnelle, Bjorn Helgaas, Benjamin Herrenschmidt, linux-pci, Linux Kernel Mailing List, linux-s390, linux-arch, linuxppc-dev On Sat, Sep 11, 2021 at 9:09 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > > Niklas Schnelle <schnelle@linux.ibm.com> writes: > > On powerpc, pci_dev_is_added() is called as part of SR-IOV fixups > > that are done under pcibios_add_device() which in turn is only called in > > pci_device_add() whih is called when a PCI device is scanned. > > Thanks for cleaning this up for us. > > > Now pci_dev_assign_added() is called in pci_bus_add_device() which is > > only called after scanning the device. Thus pci_dev_is_added() is always > > false and can be dropped. > > My only query is whether we can pin down when that changed. > > Oliver said: > > The use of pci_dev_is_added() in arch/powerpc was because in the past > pci_bus_add_device() could be called before pci_device_add(). That was > fixed a while ago so It should be safe to remove those calls now. > > I trawled back through the history a bit but I can't remember/find which > commit changed that, Oliver can you remember? Yeah, on closer inspection that never happened. The re-ordering I was thinking of was when the boot-time BAR assignments were moved in 3f068aae7a95 so they'd always occur before pci_bus_add_device() was called. I think I got that change mixed up with commit 30d87ef8b38d ("powerpc/pci: Fix pcibios_setup_device() ordering") which moved some of what what pcibios_device_add() did into pcibios_bus_add_device() to harmonise the hot and coldplug paths. As far as I can tell the pci_dev_is_added() check has been pointless since the code was added in 6e628c7d33d9 ("powerpc/powernv: Reserve additional space for IOV BAR according to the number of total_pe"). Even back then pci_device_add() was called first in both the normal and OF based PCI probing paths so there's no circumstance where that code would see the added flag set. That patch was part of the PowerNV SRIOV support series which went through quite a few iterations. My best guess is that check might have been needed in an earlier version and was just carried forward until it got merged. I didn't dig too deeply into the history though. Reviewed-by: Oliver O'Halloran <oohall@gmail.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] powerpc: Drop superfluous pci_dev_is_added() calls 2021-09-10 14:19 ` [PATCH 1/1] powerpc: Drop superfluous pci_dev_is_added() calls Niklas Schnelle 2021-09-11 11:09 ` Michael Ellerman @ 2021-09-14 19:31 ` Bjorn Helgaas 2021-09-15 0:23 ` Michael Ellerman 1 sibling, 1 reply; 7+ messages in thread From: Bjorn Helgaas @ 2021-09-14 19:31 UTC (permalink / raw) To: Niklas Schnelle Cc: Bjorn Helgaas, Benjamin Herrenschmidt, Michael Ellerman, Oliver O'Halloran, linux-pci, linux-kernel, linux-s390, linux-arch, linuxppc-dev On Fri, Sep 10, 2021 at 04:19:40PM +0200, Niklas Schnelle wrote: > On powerpc, pci_dev_is_added() is called as part of SR-IOV fixups > that are done under pcibios_add_device() which in turn is only called in > pci_device_add() whih is called when a PCI device is scanned. > > Now pci_dev_assign_added() is called in pci_bus_add_device() which is > only called after scanning the device. Thus pci_dev_is_added() is always > false and can be dropped. > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com> This doesn't touch the PCI core, so maybe makes sense for you to take it, Michael? But let me know if you think otherwise. Thanks a lot for cleaning this up, Niklas. > --- > arch/powerpc/platforms/powernv/pci-sriov.c | 6 ------ > arch/powerpc/platforms/pseries/setup.c | 3 +-- > 2 files changed, 1 insertion(+), 8 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c > index 28aac933a439..deddbb233fde 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" > - > /* > * The majority of the complexity in supporting SR-IOV on PowerNV comes from > * the need to put the MMIO space for each VF into a separate PE. Internally > @@ -228,9 +225,6 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) > > void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev) > { > - if (WARN_ON(pci_dev_is_added(pdev))) > - return; > - > if (pdev->is_virtfn) { > struct pnv_ioda_pe *pe = pnv_ioda_get_pe(pdev); > > diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c > index f79126f16258..2188054470c1 100644 > --- a/arch/powerpc/platforms/pseries/setup.c > +++ b/arch/powerpc/platforms/pseries/setup.c > @@ -74,7 +74,6 @@ > #include <asm/hvconsole.h> > > #include "pseries.h" > -#include "../../../../drivers/pci/pci.h" > > DEFINE_STATIC_KEY_FALSE(shared_processor); > EXPORT_SYMBOL(shared_processor); > @@ -750,7 +749,7 @@ static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev) > const int *indexes; > struct device_node *dn = pci_device_to_OF_node(pdev); > > - if (!pdev->is_physfn || pci_dev_is_added(pdev)) > + if (!pdev->is_physfn) > return; > /*Firmware must support open sriov otherwise dont configure*/ > indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL); > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] powerpc: Drop superfluous pci_dev_is_added() calls 2021-09-14 19:31 ` Bjorn Helgaas @ 2021-09-15 0:23 ` Michael Ellerman 0 siblings, 0 replies; 7+ messages in thread From: Michael Ellerman @ 2021-09-15 0:23 UTC (permalink / raw) To: Bjorn Helgaas, Niklas Schnelle Cc: Bjorn Helgaas, Benjamin Herrenschmidt, Oliver O'Halloran, linux-pci, linux-kernel, linux-s390, linux-arch, linuxppc-dev Bjorn Helgaas <helgaas@kernel.org> writes: > On Fri, Sep 10, 2021 at 04:19:40PM +0200, Niklas Schnelle wrote: >> On powerpc, pci_dev_is_added() is called as part of SR-IOV fixups >> that are done under pcibios_add_device() which in turn is only called in >> pci_device_add() whih is called when a PCI device is scanned. >> >> Now pci_dev_assign_added() is called in pci_bus_add_device() which is >> only called after scanning the device. Thus pci_dev_is_added() is always >> false and can be dropped. >> >> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> > > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com> > > This doesn't touch the PCI core, so maybe makes sense for you to take > it, Michael? But let me know if you think otherwise. Yeah I'm happy to take it, thanks. cheers ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/1] Arch use of pci_dev_is_added() 2021-09-10 14:19 [PATCH 0/1] Arch use of pci_dev_is_added() Niklas Schnelle 2021-09-10 14:19 ` [PATCH 1/1] powerpc: Drop superfluous pci_dev_is_added() calls Niklas Schnelle @ 2021-10-11 12:06 ` Michael Ellerman 1 sibling, 0 replies; 7+ messages in thread From: Michael Ellerman @ 2021-10-11 12:06 UTC (permalink / raw) To: Niklas Schnelle, Bjorn Helgaas Cc: linuxppc-dev, linux-arch, linux-kernel, linux-s390, Oliver O'Halloran, linux-pci On Fri, 10 Sep 2021 16:19:39 +0200, Niklas Schnelle wrote: > In my proposal to make pci_dev_is_added() more regularly usable by arch code > you mentioned[0] that you believe the uses in arch/powerpc are not necessary > anymore. From code reading I agree and so does Oliver O'Halloran[1]. > > So as promised here is a patch removing them. I only compile tested this as > I don't have access to a powerpc system. > > [...] Applied to powerpc/next. [1/1] powerpc: Drop superfluous pci_dev_is_added() calls https://git.kernel.org/powerpc/c/452f145eca73945222923525a7eba59cf37909cc cheers ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-11 12:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-10 14:19 [PATCH 0/1] Arch use of pci_dev_is_added() Niklas Schnelle 2021-09-10 14:19 ` [PATCH 1/1] powerpc: Drop superfluous pci_dev_is_added() calls Niklas Schnelle 2021-09-11 11:09 ` Michael Ellerman 2021-09-13 14:58 ` Oliver O'Halloran 2021-09-14 19:31 ` Bjorn Helgaas 2021-09-15 0:23 ` Michael Ellerman 2021-10-11 12:06 ` [PATCH 0/1] Arch use of pci_dev_is_added() Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).