Re: [PATCH] ACPI / PCI: Initialize PCI root drivers after PCI bus enumeration

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday, February 12, 2013 05:54:08 PM Yinghai Lu wrote:
> On Tue, Feb 12, 2013 at 4:16 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > After commit 1aeae82 (ACPI / PCI: avoid building pci_slot as module)
> > the pci_slot driver's .add() callback routine, acpi_pci_slot_add(),
> > is executed from within acpi_pci_root_add() before the PCI devices on
> > the bus are enumerated and that triggers the WARN_ON() in
> > kobject_get():
> >
> > WARNING: at /scratch/rafael/work/test/include/linux/kref.h:42 kobject_get+0x33/0x40()
> > Hardware name: PORTEGE R500
> > Modules linked in:
> > Pid: 1, comm: swapper/0 Not tainted 3.8.0-rc7+ #160
> > Call Trace:
> >  [<ffffffff8103d38a>] warn_slowpath_common+0x7a/0xb0
> >  [<ffffffff8103d3d5>] warn_slowpath_null+0x15/0x20
> >  [<ffffffff812f99b3>] kobject_get+0x33/0x40
> >  [<ffffffff81452b82>] get_device+0x12/0x30
> >  [<ffffffff8138df7b>] register_slot+0x243/0x295
> >  [<ffffffff810a7b6d>] ? trace_hardirqs_on+0xd/0x10
> >  [<ffffffff8137b7b9>] acpi_ns_walk_namespace+0x10f/0x24a
> >  [<ffffffff8138dd38>] ? walk_p2p_bridge+0x13b/0x13b
> >  [<ffffffff8138dd38>] ? walk_p2p_bridge+0x13b/0x13b
> >  [<ffffffff8137bdc3>] acpi_walk_namespace+0xe9/0x132
> >  [<ffffffff8138dd38>] ? walk_p2p_bridge+0x13b/0x13b
> >  [<ffffffff8138dcf5>] walk_p2p_bridge+0xf8/0x13b
> >  [<ffffffff8134d606>] ? acpi_os_signal_semaphore+0x76/0x87
> >  [<ffffffff8138dd38>] ? walk_p2p_bridge+0x13b/0x13b
> >  [<ffffffff8137b7b9>] acpi_ns_walk_namespace+0x10f/0x24a
> >  [<ffffffff8138dbfd>] ? do_sta_before_sun+0x2b/0x2b
> >  [<ffffffff8138dbfd>] ? do_sta_before_sun+0x2b/0x2b
> >  [<ffffffff8137bdc3>] acpi_walk_namespace+0xe9/0x132
> >  [<ffffffff81356071>] ? acpi_pci_root_add+0x3e7/0x49a
> >  [<ffffffff8138e07f>] acpi_pci_slot_add+0xb2/0x103
> >  [<ffffffff8138dd38>] ? walk_p2p_bridge+0x13b/0x13b
> >  [<ffffffff81356089>] acpi_pci_root_add+0x3ff/0x49a
> >
> > which means that pci_bus->dev used in register_slot() has not been
> > registered yet at that point.
> >
> > To fix this use the observation that before commit 1aeae82
> > acpi_pci_slot_add() was always run after pci_bus_add_devices()
> > and that happened to the acpiphp's .add() callback routine too.
> > Thus it is safe to reorder acpi_pci_root_add() to make the PCI root
> > drivers' .add() callbacks be run after pci_bus_add_devices(), so do
> > that.
> >
> > This approach was previously proposed by Myron Stowe.
> >
> > References: https://patchwork.kernel.org/patch/1848781/
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > The commit mentioned in the changelog above is in linux-pm.git/linux-next.
> >
> > Thanks,
> > Rafael
> >
> > ---
> >  drivers/acpi/pci_root.c |   13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > Index: test/drivers/acpi/pci_root.c
> > ===================================================================
> > --- test.orig/drivers/acpi/pci_root.c
> > +++ test/drivers/acpi/pci_root.c
> > @@ -600,17 +600,20 @@ static int acpi_pci_root_add(struct acpi
> >         if (system_state != SYSTEM_BOOTING)
> >                 pci_assign_unassigned_bus_resources(root->bus);
> >
> > +       /* need to after hot-added ioapic is registered */
> > +       if (system_state != SYSTEM_BOOTING)
> > +               pci_enable_bridges(root->bus);
> > +
> > +       pci_bus_add_devices(root->bus);
> > +
> >         mutex_lock(&acpi_pci_root_lock);
> > +
> >         list_for_each_entry(driver, &acpi_pci_drivers, node)
> >                 if (driver->add)
> >                         driver->add(root);
> > -       mutex_unlock(&acpi_pci_root_lock);
> >
> > -       /* need to after hot-added ioapic is registered */
> > -       if (system_state != SYSTEM_BOOTING)
> > -               pci_enable_bridges(root->bus);
> > +       mutex_unlock(&acpi_pci_root_lock);
> >
> > -       pci_bus_add_devices(root->bus);
> >         return 1;
> >
> >  out_del_root:
> >
> 
> No, we don't need this after
> 
> | commit 4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4
> |     PCI: Put pci_dev in device tree as early as possible
> 
> in pci/next.
> 
> So we can move that Jiang's patch from your tree to Bjorn's tree?

OK, I'm dropping that patch from my tree, then (hopefully, no one has merged
my acpi-cleanup branch yet).

Bjorn, could you pick up https://patchwork.kernel.org/patch/2003771/ please?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.