| Message ID | 1359265003-16166-17-git-send-email-yinghai@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jan 26, 2013 at 09:36:37PM -0800, Yinghai Lu wrote: > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: linux-arm-kernel@lists.infradead.org So... what's this about. This email is all I've recieved, and the only thing that I have to go on is one single subject line and not description about what's going on. I guess there's some other patch introducing this for_each_pci_host_bridge() macro somewhere? I guess that's part of this patch set? Yet... I guess you want an ack for this or something... which would be irresponsible to give without knowing the purpose behind this, the reasoning, or even being able to tell whether the replacement code is functionally equivalent. So, no ack (or nack) at the moment. > --- > arch/arm/kernel/bios32.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index 9b72261..d0befe4 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -57,13 +57,10 @@ static void pcibios_bus_report_status(struct pci_bus *bus, u_int status_mask, in > > void pcibios_report_status(u_int status_mask, int warn) > { > - struct list_head *l; > + struct pci_host_bridge *host_bridge = NULL; > > - list_for_each(l, &pci_root_buses) { > - struct pci_bus *bus = pci_bus_b(l); > - > - pcibios_bus_report_status(bus, status_mask, warn); > - } > + for_each_pci_host_bridge(host_bridge) > + pcibios_bus_report_status(host_bridge->bus, status_mask, warn); > } > > /* > -- > 1.7.10.4 >
On Sun, Jan 27, 2013 at 9:38 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Sat, Jan 26, 2013 at 09:36:37PM -0800, Yinghai Lu wrote: >> Signed-off-by: Yinghai Lu <yinghai@kernel.org> >> Cc: Russell King <linux@arm.linux.org.uk> >> Cc: linux-arm-kernel@lists.infradead.org > > So... what's this about. This email is all I've recieved, and the only > thing that I have to go on is one single subject line and not description > about what's going on. I guess there's some other patch introducing this > for_each_pci_host_bridge() macro somewhere? I guess that's part of this > patch set? yes. sorry for not put you in the cc list of the whole patchset. will do that next reversion. > > Yet... I guess you want an ack for this or something... which would be > irresponsible to give without knowing the purpose behind this, the > reasoning, or even being able to tell whether the replacement code is > functionally equivalent. the purpose is to make iteration for root bus to be root-bus hotplug safe. will update change log. > > So, no ack (or nack) at the moment. > >> --- >> arch/arm/kernel/bios32.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c >> index 9b72261..d0befe4 100644 >> --- a/arch/arm/kernel/bios32.c >> +++ b/arch/arm/kernel/bios32.c >> @@ -57,13 +57,10 @@ static void pcibios_bus_report_status(struct pci_bus *bus, u_int status_mask, in >> >> void pcibios_report_status(u_int status_mask, int warn) >> { >> - struct list_head *l; >> + struct pci_host_bridge *host_bridge = NULL; >> >> - list_for_each(l, &pci_root_buses) { >> - struct pci_bus *bus = pci_bus_b(l); >> - >> - pcibios_bus_report_status(bus, status_mask, warn); >> - } >> + for_each_pci_host_bridge(host_bridge) >> + pcibios_bus_report_status(host_bridge->bus, status_mask, warn); >> } >> >> /* >> -- >> 1.7.10.4 >>
Now we have pci_root_buses list, and there is lots of iteration with
list_of_each of it, that is not safe after we add pci root bus hotplug
support after booting stage.
Add pci_get_next_host_bridge and use bus_find_device in driver core to
iterate host bridge and the same time get root bus.
We replace searching root bus with searching host_bridge,
as host_bridge->bus is the root bus.
After those replacing, we even could kill pci_root_buses list.
based on pci/next
could get from
git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-for-each-host-bridge
-v2: updated after pci_root_bus_hotplug get into pci/next
-v3: update changelog and add cc for pci core change for arch guys.
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: Doug Thompson <dougthompson@xmission.com>
Cc: linux-edac@vger.kernel.org
Cc: x86@kernel.org
Cc: David Airlie <airlied@linux.ie>
Cc: dri-devel@lists.freedesktop.org
Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: linux-ia64@vger.kernel.org
Cc: linux-altix@sgi.com
Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
Cc: Russell King <linux@arm.linux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org
Cc: David Howells <dhowells@redhat.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: microblaze-uclinux@itee.uq.edu.au
Cc: Koichi Yasutake <yasutake.koichi@jp.panasonic.com>
Cc: linux-am33-list@redhat.com
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org
Yinghai Lu (22):
PCI: Rename pci_release_bus_bridge_dev to pci_release_host_bridge_dev
PCI: Add dummy bus_type for pci_host_bridge
PCI, libata: remove find_bridge in acpi_bus_type
PCI, ACPI: Update comments for find_bridge in acpi_bus_type
PCI: Add for_each_pci_host_bridge() and pci_get_next_host_bridge
PCI, hotplug: Kill pci_find_next_bus in sgi_hotplug
PCI: Kill pci_find_next_bus in pci_sysfs
PCI, edac: Kill pci_find_next_bus in edac
PCI, x86: Kill pci_find_next_bus in pcibios_scan_root
PCI, x86: Kill pci_root_buses in resources reservations
PCI, drm: Kill pci_root_buses in alpha hose setting
PCI: Kill pci_root_buses in setup-bus
PCI, sparc: Kill pci_find_next_bus
PCI, ia64: Kill pci_find_next_bus
PCI, alpha: Kill pci_root_buses
PCI, arm: Kill pci_root_buses
PCI, frv: Kill pci_root_buses in resources reservations
PCI, microblaze: Kill pci_root_buses in resources reservations
PCI, mn10300: Kill pci_root_buses in resources reservations
PCI, powerpc: Kill pci_root_buses in resources reservations
PCI: Kill pci_find_next_bus
PCI: Kill pci_root_buses
arch/alpha/kernel/pci.c | 6 +--
arch/arm/kernel/bios32.c | 9 ++---
arch/frv/mb93090-mb00/pci-frv.c | 37 +++++++++---------
arch/ia64/hp/common/sba_iommu.c | 7 ++--
arch/ia64/sn/kernel/io_common.c | 5 ++-
arch/microblaze/pci/pci-common.c | 10 ++---
arch/mn10300/unit-asb2305/pci-asb2305.c | 62 ++++++++++++++++---------------
arch/powerpc/kernel/pci-common.c | 13 +++----
arch/powerpc/kernel/pci_64.c | 8 ++--
arch/sparc/kernel/pci.c | 6 ++-
arch/x86/pci/common.c | 9 +++--
arch/x86/pci/i386.c | 20 +++++-----
drivers/ata/libata-acpi.c | 6 ---
drivers/edac/i7core_edac.c | 6 +--
drivers/gpu/drm/drm_fops.c | 10 +++--
drivers/pci/hotplug/sgi_hotplug.c | 6 ++-
drivers/pci/pci-driver.c | 11 +++++-
drivers/pci/pci-sysfs.c | 6 +--
drivers/pci/probe.c | 13 ++-----
drivers/pci/search.c | 61 +++++++++++++++---------------
drivers/pci/setup-bus.c | 24 ++++++------
include/acpi/acpi_bus.h | 2 +-
include/linux/pci.h | 18 +++++----
23 files changed, 187 insertions(+), 168 deletions(-)
On Sun, Jan 27, 2013 at 12:23 PM, Yinghai Lu <yinghai@kernel.org> wrote: > Now we have pci_root_buses list, and there is lots of iteration with > list_of_each of it, that is not safe after we add pci root bus hotplug > support after booting stage. > > Add pci_get_next_host_bridge and use bus_find_device in driver core to > iterate host bridge and the same time get root bus. > > We replace searching root bus with searching host_bridge, > as host_bridge->bus is the root bus. > After those replacing, we even could kill pci_root_buses list. These are the problems I think you're fixing: 1) pci_find_next_bus() is not safe because even though it holds pci_bus_sem while walking the pci_root_buses list, it doesn't hold a reference on the bus it returns. The bus could be removed while the caller is using it. 2) "list_for_each_entry(bus, &pci_root_buses, node)" is not safe because hotplug might modify the pci_root_buses list. Replacing that with for_each_pci_host_bridge() solves that problem by using bus_find_device(), which is built on klists, which are designed for that problem. 3) pci_find_next_bus() claims to iterate through all known PCI buses, but in fact only iterates through root buses. So far, so good. Those are problems we need to fix. Your solution is to introduce for_each_pci_host_bridge() as an iterator through the known host bridges. There are two scenarios where we use something like this: 1) We want to perform an operation on every known host bridge. 2) We want to initialize something for every host bridge. In my opinion, the only instance of scenario 1) is bus_rescan_store(), where we want to rescan all PCI host bridges. In every other case, we're doing some kind of initialization of all the host bridges. For these cases, for_each_pci_host_bridge() is the wrong solution because it doesn't work for hot-added bridges. I think these cases should be changed to use pcibios_root_bridge_prepare() or something something else called in the host bridge add path. Bjorn
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c index 9b72261..d0befe4 100644 --- a/arch/arm/kernel/bios32.c +++ b/arch/arm/kernel/bios32.c @@ -57,13 +57,10 @@ static void pcibios_bus_report_status(struct pci_bus *bus, u_int status_mask, in void pcibios_report_status(u_int status_mask, int warn) { - struct list_head *l; + struct pci_host_bridge *host_bridge = NULL; - list_for_each(l, &pci_root_buses) { - struct pci_bus *bus = pci_bus_b(l); - - pcibios_bus_report_status(bus, status_mask, warn); - } + for_each_pci_host_bridge(host_bridge) + pcibios_bus_report_status(host_bridge->bus, status_mask, warn); } /*
Signed-off-by: Yinghai Lu <yinghai@kernel.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: linux-arm-kernel@lists.infradead.org --- arch/arm/kernel/bios32.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)