About dynamically loading the acpiphp.ko

About dynamically loading the acpiphp.ko

[Gavin Guo]

Hi All,

I've a Dell platform which has PCI-to-USB card and would like to let
the module be dynamically insmoded when hotplugging this card.
Firstly, I've submitted a patch, which added acpiphp to the
/etc/modules, to the init-module-tools package in the Debian system,
but maintainer rejected to accept the patch and said it would be done
in the kernel side to enable the automatically loading. Then, I've
tried to read the code about acpiphp under drivers/pci/hotplug/* and
found that acpiphp_ibm.c seems providing a hook to build the
dependency with acpiphp.ko, providing a way to construct a empty
ibm_handle_events-liked callback function, named dell_handle_events,
for acpi_install_notify_handler and the acpiphp_dell.ko could be
insmoded by udev through the MODULE_ALIAS("dmi*...") which describe
the dell platform information. However, writing a code with fake empty
function just to enable the hotplugging PCI is silly in my opinion, I
would be appreciated if anyone could provide a method to enable the
hotplugging automatically.

Thanks,
Gavin Guo

Re: About dynamically loading the acpiphp.ko

[Gavin Guo]

2013/4/18 Gavin Guo <tuffkidtt@gmail.com>:
> Hi All,
>
> I've a Dell platform which has PCI-to-USB card and would like to let
> the module be dynamically insmoded when hotplugging this card.
> Firstly, I've submitted a patch, which added acpiphp to the
> /etc/modules, to the init-module-tools package in the Debian system,
> but maintainer rejected to accept the patch and said it would be done
> in the kernel side to enable the automatically loading. Then, I've
> tried to read the code about acpiphp under drivers/pci/hotplug/* and
> found that acpiphp_ibm.c seems providing a hook to build the
> dependency with acpiphp.ko, providing a way to construct a empty
> ibm_handle_events-liked callback function, named dell_handle_events,
> for acpi_install_notify_handler and the acpiphp_dell.ko could be
> insmoded by udev through the MODULE_ALIAS("dmi*...") which describe
> the dell platform information. However, writing a code with fake empty
> function just to enable the hotplugging PCI is silly in my opinion, I
> would be appreciated if anyone could provide a method to enable the
> hotplugging automatically.
>
> Thanks,
> Gavin Guo

Hi all,

I found that jiang.liu@huawei.com has committed a patch and was
already in the Bjorn Helgaas's git tree. That means the only way to
use the hot-plugging in the future is just to build in the function.

Thanks
Gavin Guo

Re: About dynamically loading the acpiphp.ko

[Bjorn Helgaas]

[+cc Jiang, -cc willy@hp.com (defunct address), -cc t-kochi (I assume
also defunct)]

On Wed, Apr 17, 2013 at 9:27 PM, Gavin Guo <tuffkidtt@gmail.com> wrote:
> Hi All,
>
> I've a Dell platform which has PCI-to-USB card and would like to let
> the module be dynamically insmoded when hotplugging this card.
> Firstly, I've submitted a patch, which added acpiphp to the
> /etc/modules, to the init-module-tools package in the Debian system,
> but maintainer rejected to accept the patch and said it would be done
> in the kernel side to enable the automatically loading. Then, I've
> tried to read the code about acpiphp under drivers/pci/hotplug/* and
> found that acpiphp_ibm.c seems providing a hook to build the
> dependency with acpiphp.ko, providing a way to construct a empty
> ibm_handle_events-liked callback function, named dell_handle_events,
> for acpi_install_notify_handler and the acpiphp_dell.ko could be
> insmoded by udev through the MODULE_ALIAS("dmi*...") which describe
> the dell platform information. However, writing a code with fake empty
> function just to enable the hotplugging PCI is silly in my opinion, I
> would be appreciated if anyone could provide a method to enable the
> hotplugging automatically.

Please test linux-next (20130417 or later) with
CONFIG_HOTPLUG_PCI_ACPI=y and see if that works as you expect.  That
contains Jiang's patch (that you mentioned in a your subsequent email)
which removes the option of building acpiphp as a module.

If that doesn't work, please let us know, along with more details,
such as a complete dmesg log.  There are still known issues with
acpiphp, e.g., https://bugzilla.kernel.org/show_bug.cgi?id=54981, that
you might be seeing.

Bjorn

Re: About dynamically loading the acpiphp.ko

[Bjorn Helgaas]

[+cc Rafael, Yinghai, -cc t-kochi for real this time]

On Wed, Apr 24, 2013 at 2:46 AM, Gavin Guo <tuffkidtt@gmail.com> wrote:
> 2013/4/18 Bjorn Helgaas <bhelgaas@google.com>:
>
>> [+cc Jiang, -cc willy@hp.com (defunct address), -cc t-kochi (I assume
>> also defunct)]
>>
>> On Wed, Apr 17, 2013 at 9:27 PM, Gavin Guo <tuffkidtt@gmail.com> wrote:
>>> Hi All,
>>>
>>> I've a Dell platform which has PCI-to-USB card and would like to let
>>> the module be dynamically insmoded when hotplugging this card.
>>> Firstly, I've submitted a patch, which added acpiphp to the
>>> /etc/modules, to the init-module-tools package in the Debian system,
>>> but maintainer rejected to accept the patch and said it would be done
>>> in the kernel side to enable the automatically loading. Then, I've
>>> tried to read the code about acpiphp under drivers/pci/hotplug/* and
>>> found that acpiphp_ibm.c seems providing a hook to build the
>>> dependency with acpiphp.ko, providing a way to construct a empty
>>> ibm_handle_events-liked callback function, named dell_handle_events,
>>> for acpi_install_notify_handler and the acpiphp_dell.ko could be
>>> insmoded by udev through the MODULE_ALIAS("dmi*...") which describe
>>> the dell platform information. However, writing a code with fake empty
>>> function just to enable the hotplugging PCI is silly in my opinion, I
>>> would be appreciated if anyone could provide a method to enable the
>>> hotplugging automatically.
>>
>> Please test linux-next (20130417 or later) with
>> CONFIG_HOTPLUG_PCI_ACPI=y and see if that works as you expect.  That
>> contains Jiang's patch (that you mentioned in a your subsequent email)
>> which removes the option of building acpiphp as a module.
>>
>> If that doesn't work, please let us know, along with more details,
>> such as a complete dmesg log.  There are still known issues with
>> acpiphp, e.g., https://bugzilla.kernel.org/show_bug.cgi?id=54981, that
>> you might be seeing.
>>
>> Bjorn
>
> Hi Bjorn,
>
> I've tested the helgaas/next tree. And the hotplugging cannot work. The
> sequence I tested the function are as following: Booting with PCI-USB
> express card plugged-in, then plugging in the USB disk; un-plugging the USB
> disk; un-plugging the PCI-USB express card; plugging in the PCI-USB express
> card. The dmesg is attached.

Thanks for testing this!

Your dmesg log includes this:

    acpiphp: ACPI Hot Plug PCI Controller Driver version: 0.5
    ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-3e])
    \_SB_.PCI0:_OSC invalid UUID
    acpiphp: Slot [1] registered
    acpiphp: Slot [1-1] registered
   acpi root: \_SB_.PCI0 notify handler is installed
    _handle_hotplug_event_root: Bus check notify on \_SB_.PCI0
    _handle_hotplug_event_root: Bus check notify on \_SB_.PCI0

We won't use pciehp (PCIe native hotplug) because the _OSC evaluation
failed.  (As a workaround, you could try booting with
"pcie_ports=native".  That's not a fix, of course, but it might be a
temporary hack to get it to work until the bug is fixed.)

So we should be using acpiphp, which you do have built in statically,
and it found a couple slots.  And we did get two bus check notifies on
\_SB_.PCI0, so we *should* be re-enumerating PCI bus 0000:00.  But it
looks like we're handling this as a host bridge hotplug event instead
of a PCI device hotplug.  My guess is that
handle_root_bridge_insertion() does nothing because the PCI0 ACPI
device already exists, though I would expect to see the "acpi device
exists..." in your dmesg log if this were the case.

We install a notify handler on PCI0 in find_root_bridges(), and that
notify handler leads to _handle_hotplug_event_root(), which assumes
this is a host bridge hotplug event.  But I would expect the notify
for a host bridge hot-add would go to the *parent* of the host bridge,
not to the host bridge itself, so something seems wrong here.

Maybe the ACPI & hotplug experts can comment on this.

In the meantime, can you collect complete "lspci -vv" output when
booted with everything plugged in (so we discover everything at
boot-time with no hotplug required) so we can see exactly where the
hotplug devices fit in the hierarchy, and maybe an acpidump, too?
I'll open a bugzilla report and attach all this info to it.

Bjorn

Re: About dynamically loading the acpiphp.ko

[Rafael J. Wysocki]

On Wednesday, April 24, 2013 02:03:04 PM Bjorn Helgaas wrote:
> [+cc Rafael, Yinghai, -cc t-kochi for real this time]
> 
> On Wed, Apr 24, 2013 at 2:46 AM, Gavin Guo <tuffkidtt@gmail.com> wrote:
> > 2013/4/18 Bjorn Helgaas <bhelgaas@google.com>:
> >
> >> [+cc Jiang, -cc willy@hp.com (defunct address), -cc t-kochi (I assume
> >> also defunct)]
> >>
> >> On Wed, Apr 17, 2013 at 9:27 PM, Gavin Guo <tuffkidtt@gmail.com> wrote:
> >>> Hi All,
> >>>
> >>> I've a Dell platform which has PCI-to-USB card and would like to let
> >>> the module be dynamically insmoded when hotplugging this card.
> >>> Firstly, I've submitted a patch, which added acpiphp to the
> >>> /etc/modules, to the init-module-tools package in the Debian system,
> >>> but maintainer rejected to accept the patch and said it would be done
> >>> in the kernel side to enable the automatically loading. Then, I've
> >>> tried to read the code about acpiphp under drivers/pci/hotplug/* and
> >>> found that acpiphp_ibm.c seems providing a hook to build the
> >>> dependency with acpiphp.ko, providing a way to construct a empty
> >>> ibm_handle_events-liked callback function, named dell_handle_events,
> >>> for acpi_install_notify_handler and the acpiphp_dell.ko could be
> >>> insmoded by udev through the MODULE_ALIAS("dmi*...") which describe
> >>> the dell platform information. However, writing a code with fake empty
> >>> function just to enable the hotplugging PCI is silly in my opinion, I
> >>> would be appreciated if anyone could provide a method to enable the
> >>> hotplugging automatically.
> >>
> >> Please test linux-next (20130417 or later) with
> >> CONFIG_HOTPLUG_PCI_ACPI=y and see if that works as you expect.  That
> >> contains Jiang's patch (that you mentioned in a your subsequent email)
> >> which removes the option of building acpiphp as a module.
> >>
> >> If that doesn't work, please let us know, along with more details,
> >> such as a complete dmesg log.  There are still known issues with
> >> acpiphp, e.g., https://bugzilla.kernel.org/show_bug.cgi?id=54981, that
> >> you might be seeing.
> >>
> >> Bjorn
> >
> > Hi Bjorn,
> >
> > I've tested the helgaas/next tree. And the hotplugging cannot work. The
> > sequence I tested the function are as following: Booting with PCI-USB
> > express card plugged-in, then plugging in the USB disk; un-plugging the USB
> > disk; un-plugging the PCI-USB express card; plugging in the PCI-USB express
> > card. The dmesg is attached.
> 
> Thanks for testing this!
> 
> Your dmesg log includes this:
> 
>     acpiphp: ACPI Hot Plug PCI Controller Driver version: 0.5
>     ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-3e])
>     \_SB_.PCI0:_OSC invalid UUID
>     acpiphp: Slot [1] registered
>     acpiphp: Slot [1-1] registered
>    acpi root: \_SB_.PCI0 notify handler is installed
>     _handle_hotplug_event_root: Bus check notify on \_SB_.PCI0
>     _handle_hotplug_event_root: Bus check notify on \_SB_.PCI0
> 
> We won't use pciehp (PCIe native hotplug) because the _OSC evaluation
> failed.  (As a workaround, you could try booting with
> "pcie_ports=native".  That's not a fix, of course, but it might be a
> temporary hack to get it to work until the bug is fixed.)
> 
> So we should be using acpiphp, which you do have built in statically,
> and it found a couple slots.  And we did get two bus check notifies on
> \_SB_.PCI0, so we *should* be re-enumerating PCI bus 0000:00.  But it
> looks like we're handling this as a host bridge hotplug event instead
> of a PCI device hotplug.  My guess is that
> handle_root_bridge_insertion() does nothing because the PCI0 ACPI
> device already exists, though I would expect to see the "acpi device
> exists..." in your dmesg log if this were the case.
> 
> We install a notify handler on PCI0 in find_root_bridges(), and that
> notify handler leads to _handle_hotplug_event_root(), which assumes
> this is a host bridge hotplug event.  But I would expect the notify
> for a host bridge hot-add would go to the *parent* of the host bridge,
> not to the host bridge itself, so something seems wrong here.
> 
> Maybe the ACPI & hotplug experts can comment on this.

I think it may (ie. won't be against the spec) go both ways, so it may go
to the parent or the host bridge object itself.  We should be handling both
cases IMO.

Thanks,
Rafael


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

Re: About dynamically loading the acpiphp.ko

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Wed, Apr 24, 2013 at 3:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, April 24, 2013 02:03:04 PM Bjorn Helgaas wrote:
>>
>> We install a notify handler on PCI0 in find_root_bridges(), and that
>> notify handler leads to _handle_hotplug_event_root(), which assumes
>> this is a host bridge hotplug event.  But I would expect the notify
>> for a host bridge hot-add would go to the *parent* of the host bridge,
>> not to the host bridge itself, so something seems wrong here.
>>
>> Maybe the ACPI & hotplug experts can comment on this.
>
> I think it may (ie. won't be against the spec) go both ways, so it may go
> to the parent or the host bridge object itself.  We should be handling both
> cases IMO.

Yes, we missed that path.

Attached patch should fix the problem.

Gavin,

Can you please check that on top of pci/next?

Thanks

Yinghai

[-- Attachment #2: acpiphp_check_host_bridge.patch --]
[-- Type: application/octet-stream, Size: 2280 bytes --]

Subject: [PATCH] PCI, ACPI, hotplug: Fix BUS_CHECK event handle on root bridge

We should check slots when BUS_CHECK is sent to root bridge
acpi handle.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/acpi/pci_root.c            |    2 ++
 drivers/pci/hotplug/acpiphp_glue.c |   13 +++++++++++++
 include/linux/pci-acpi.h           |    2 ++
 3 files changed, 17 insertions(+)

Index: linux-2.6/drivers/acpi/pci_root.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root.c
+++ linux-2.6/drivers/acpi/pci_root.c
@@ -643,6 +643,8 @@ static void _handle_hotplug_event_root(s
 				 (char *)buffer.pointer);
 		if (!root)
 			handle_root_bridge_insertion(handle);
+		else
+			acpiphp_check_host_bridge(handle);
 
 		break;
 
Index: linux-2.6/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-2.6/drivers/pci/hotplug/acpiphp_glue.c
@@ -950,6 +950,19 @@ check_sub_bridges(acpi_handle handle, u3
 	return AE_OK ;
 }
 
+void acpiphp_check_host_bridge(acpi_handle handle)
+{
+	struct acpiphp_bridge *bridge;
+
+	bridge = acpiphp_handle_to_bridge(handle);
+	if (!bridge)
+		return;
+
+	acpiphp_check_bridge(bridge);
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
+		ACPI_UINT32_MAX, check_sub_bridges, NULL, NULL, NULL);
+}
+
 static void _handle_hotplug_event_bridge(struct work_struct *work)
 {
 	struct acpiphp_bridge *bridge;
Index: linux-2.6/include/linux/pci-acpi.h
===================================================================
--- linux-2.6.orig/include/linux/pci-acpi.h
+++ linux-2.6/include/linux/pci-acpi.h
@@ -60,11 +60,13 @@ static inline void acpi_pci_slot_remove(
 void acpiphp_init(void);
 void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle);
 void acpiphp_remove_slots(struct pci_bus *bus);
+void acpiphp_check_host_bridge(acpi_handle handle);
 #else
 static inline void acpiphp_init(void) { }
 static inline void acpiphp_enumerate_slots(struct pci_bus *bus,
 					   acpi_handle handle) { }
 static inline void acpiphp_remove_slots(struct pci_bus *bus) { }
+static inline void acpiphp_check_host_bridge(acpi_handle handle) { }
 #endif
 
 #else	/* CONFIG_ACPI */

答复: About dynamically loading the acpiphp.ko

[Liujiang (Gerry, Euler)]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

Hi Yinghai,
	I have worked out a similar patch last night. Please refer to attachment.
I think following code may not work because acpiphp_handle_to_bridge()
will return NULL. Could you please help to review my version?
	(Due to work environment limitations, sorry for sending out without
attachment and outlook).
	Regards!
	Gerry

+void acpiphp_check_host_bridge(acpi_handle handle)
+{
+	struct acpiphp_bridge *bridge;
+
+	bridge = acpiphp_handle_to_bridge(handle);
+	if (!bridge)
+		return;
+
+	acpiphp_check_bridge(bridge);
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
+		ACPI_UINT32_MAX, check_sub_bridges, NULL, NULL, NULL);
+}
+

> On Wed, Apr 24, 2013 at 3:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, April 24, 2013 02:03:04 PM Bjorn Helgaas wrote:
> >>
> >> We install a notify handler on PCI0 in find_root_bridges(), and that
> >> notify handler leads to _handle_hotplug_event_root(), which assumes
> >> this is a host bridge hotplug event.  But I would expect the notify
> >> for a host bridge hot-add would go to the *parent* of the host bridge,
> >> not to the host bridge itself, so something seems wrong here.
> >>
> >> Maybe the ACPI & hotplug experts can comment on this.
> >
> > I think it may (ie. won't be against the spec) go both ways, so it may go
> > to the parent or the host bridge object itself.  We should be handling both
> > cases IMO.
> 
> Yes, we missed that path.
> 
> Attached patch should fix the problem.
> 
> Gavin,
> 
> Can you please check that on top of pci/next?
> 
> Thanks
> 
> Yinghai

[-- Attachment #2: 0001-PCI-ACPI-correctly-handle-ACPI_NOTIFY_BUS_CHECK-even.patch --]
[-- Type: application/octet-stream, Size: 3301 bytes --]

From f48f647e4092b0a02183a7c086a6fc889909e415 Mon Sep 17 00:00:00 2001
From: Jiang Liu <jiang.liu@huawei.com>
Date: Wed, 24 Apr 2013 22:50:03 +0800
Subject: [PATCH 1/4] PCI, ACPI: correctly handle ACPI_NOTIFY_BUS_CHECK events
 for PCI host bridges

The code to handle PCI host bridge hotplug has been moved from the the
acpiphp driver into the pci_root driver, which causes regresions for
some platforms.

According to ACPI 5.0 section 5.6.6 "Device Object Notifications",
a BUS_CHECK event may be generated on a PCI host bridge object for
a PCI device hotplug operation. Some BIOSes do exibit sucn a behavior.
But now the hotplug handler in the pci_root driver only verify state
of itself when handling ACPI BUS_CHECK events, and it will drop
BUS_CHECK event for PCI device hotplug operations. So call acpiphp
to handle BUS_CHECK event for PCI device hotplug in addition to PCI
host bridge hotplug.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Reported-by:
---
 drivers/acpi/pci_root.c            |    9 ++++++++-
 drivers/pci/hotplug/acpiphp_glue.c |    6 ++++++
 include/linux/pci-acpi.h           |    2 ++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index b80e06e..b7881b7 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -641,7 +641,14 @@ static void _handle_hotplug_event_root(struct work_struct *work)
 				 (char *)buffer.pointer);
 		if (!root)
 			handle_root_bridge_insertion(handle);
-
+		else {
+			/*
+			 * ACPI may generate a BUS_CHECK event on the host
+			 * bridge for a PCI device hotplug, so call the
+			 * acpiphp handler to handle PCI device hotplug cases.
+			 */
+			acpiphp_handle_bus_check_event(handle);
+		}
 		break;
 
 	case ACPI_NOTIFY_DEVICE_CHECK:
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 96fed19..fe957f4 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -1130,6 +1130,12 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
 	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
 }
 
+void acpiphp_handle_bus_check_event(acpi_handle handle)
+{
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
+		ACPI_UINT32_MAX, check_sub_bridges, NULL, NULL, NULL);
+}
+
 /*
  * Create hotplug slots for the PCI bus.
  * It should always return 0 to avoid skipping following notifiers.
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 596d705..4d537b2 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -60,11 +60,13 @@ static inline void acpi_pci_slot_remove(struct pci_bus *bus) { }
 void acpiphp_init(void);
 void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle);
 void acpiphp_remove_slots(struct pci_bus *bus);
+void acpiphp_handle_bus_check_event(acpi_handle handle);
 #else
 static inline void acpiphp_init(void) { }
 static inline void acpiphp_enumerate_slots(struct pci_bus *bus,
 					   acpi_handle handle) { }
 static inline void acpiphp_remove_slots(struct pci_bus *bus) { }
+static inline void acpiphp_handle_bus_check_event(acpi_handle handle) { }
 #endif
 
 #endif	/* CONFIG_ACPI */
-- 
1.7.9.5

Re: 答复: About dynamically loading the acpiphp.ko

[Yinghai Lu]

On Wed, Apr 24, 2013 at 7:08 PM, Liujiang (Gerry, Euler)
<jiang.liu@huawei.com> wrote:
> Hi Yinghai,
>         I have worked out a similar patch last night. Please refer to attachment.
> I think following code may not work because acpiphp_handle_to_bridge()
> will return NULL.

for root bus we have
pcibios_add_bus. ==> acpi_pci_add_bus==>call acpiphp_enumerate_slots.
so if there is hotplug slot on root bus directly, it will allocate the
acpiphp_bridge and register the slots.

then acpiphp_handle_to_bridge will return the right acpiphp_bridge with
slots list.


> Could you please help to review my version?
>         (Due to work environment limitations, sorry for sending out without
> attachment and outlook).

looks the same to my patch, except you missed the acpiphp_check_bridge
calling.

Thanks

Yinghai

Re: 答复: About dynamically loading the acpiphp.ko

[Yinghai Lu]

On Wed, Apr 24, 2013 at 8:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Apr 24, 2013 at 7:08 PM, Liujiang (Gerry, Euler)
> <jiang.liu@huawei.com> wrote:
>> Hi Yinghai,
>>         I have worked out a similar patch last night. Please refer to attachment.
>> I think following code may not work because acpiphp_handle_to_bridge()
>> will return NULL.
>
> for root bus we have
> pcibios_add_bus. ==> acpi_pci_add_bus==>call acpiphp_enumerate_slots.
> so if there is hotplug slot on root bus directly, it will allocate the
> acpiphp_bridge and register the slots.
>
> then acpiphp_handle_to_bridge will return the right acpiphp_bridge with
> slots list.

even there is not slot directly on root bus, but have slot in following bridge,
acpiphp_bridge for root bus still get created, just no direct slots under it.

>
>
>> Could you please help to review my version?
>>         (Due to work environment limitations, sorry for sending out without
>> attachment and outlook).
>
> looks the same to my patch, except you missed the acpiphp_check_bridge
> calling.
>
> Thanks
>
> Yinghai

Re: About dynamically loading the acpiphp.ko

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On Wed, Apr 24, 2013 at 6:20 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Apr 24, 2013 at 3:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Wednesday, April 24, 2013 02:03:04 PM Bjorn Helgaas wrote:
>>>
>>> We install a notify handler on PCI0 in find_root_bridges(), and that
>>> notify handler leads to _handle_hotplug_event_root(), which assumes
>>> this is a host bridge hotplug event.  But I would expect the notify
>>> for a host bridge hot-add would go to the *parent* of the host bridge,
>>> not to the host bridge itself, so something seems wrong here.
>>>
>>> Maybe the ACPI & hotplug experts can comment on this.
>>
>> I think it may (ie. won't be against the spec) go both ways, so it may go
>> to the parent or the host bridge object itself.  We should be handling both
>> cases IMO.
>
> Yes, we missed that path.
>
> Attached patch should fix the problem.
>
> Gavin,
>
> Can you please check that on top of pci/next?

updated -v2.

[-- Attachment #2: acpiphp_check_host_bridge_v2.patch --]
[-- Type: application/octet-stream, Size: 2413 bytes --]

Subject: [PATCH -v2] PCI, ACPI, hotplug: Fix BUS_CHECK event handle on root bridge

We should check slots when BUS_CHECK is sent to root bridge
acpi handle.

-v2: Don't check bridge for acpi_walk_namespace with check_sub_bridges.
     also put back bridge reference.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/acpi/pci_root.c            |    2 ++
 drivers/pci/hotplug/acpiphp_glue.c |   14 ++++++++++++++
 include/linux/pci-acpi.h           |    2 ++
 3 files changed, 18 insertions(+)

Index: linux-2.6/drivers/acpi/pci_root.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root.c
+++ linux-2.6/drivers/acpi/pci_root.c
@@ -643,6 +643,8 @@ static void _handle_hotplug_event_root(s
 				 (char *)buffer.pointer);
 		if (!root)
 			handle_root_bridge_insertion(handle);
+		else
+			acpiphp_check_host_bridge(handle);
 
 		break;
 
Index: linux-2.6/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-2.6/drivers/pci/hotplug/acpiphp_glue.c
@@ -950,6 +950,20 @@ check_sub_bridges(acpi_handle handle, u3
 	return AE_OK ;
 }
 
+void acpiphp_check_host_bridge(acpi_handle handle)
+{
+	struct acpiphp_bridge *bridge;
+
+	bridge = acpiphp_handle_to_bridge(handle);
+	if (bridge) {
+		acpiphp_check_bridge(bridge);
+		put_bridge(bridge);
+	}
+
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
+		ACPI_UINT32_MAX, check_sub_bridges, NULL, NULL, NULL);
+}
+
 static void _handle_hotplug_event_bridge(struct work_struct *work)
 {
 	struct acpiphp_bridge *bridge;
Index: linux-2.6/include/linux/pci-acpi.h
===================================================================
--- linux-2.6.orig/include/linux/pci-acpi.h
+++ linux-2.6/include/linux/pci-acpi.h
@@ -60,11 +60,13 @@ static inline void acpi_pci_slot_remove(
 void acpiphp_init(void);
 void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle);
 void acpiphp_remove_slots(struct pci_bus *bus);
+void acpiphp_check_host_bridge(acpi_handle handle);
 #else
 static inline void acpiphp_init(void) { }
 static inline void acpiphp_enumerate_slots(struct pci_bus *bus,
 					   acpi_handle handle) { }
 static inline void acpiphp_remove_slots(struct pci_bus *bus) { }
+static inline void acpiphp_check_host_bridge(acpi_handle handle) { }
 #endif
 
 #else	/* CONFIG_ACPI */

Re: About dynamically loading the acpiphp.ko

[Gavin Guo]

2013/4/25 Yinghai Lu <yinghai@kernel.org>
>
> On Wed, Apr 24, 2013 at 6:20 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Wed, Apr 24, 2013 at 3:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> On Wednesday, April 24, 2013 02:03:04 PM Bjorn Helgaas wrote:
> >>>
> >>> We install a notify handler on PCI0 in find_root_bridges(), and that
> >>> notify handler leads to _handle_hotplug_event_root(), which assumes
> >>> this is a host bridge hotplug event.  But I would expect the notify
> >>> for a host bridge hot-add would go to the *parent* of the host bridge,
> >>> not to the host bridge itself, so something seems wrong here.
> >>>
> >>> Maybe the ACPI & hotplug experts can comment on this.
> >>
> >> I think it may (ie. won't be against the spec) go both ways, so it may go
> >> to the parent or the host bridge object itself.  We should be handling both
> >> cases IMO.
> >
> > Yes, we missed that path.
> >
> > Attached patch should fix the problem.
> >
> > Gavin,
> >
> > Can you please check that on top of pci/next?
>
> updated -v2.

Hi Yinghai,

I tested the first version of your patch and it can't work. Now, I'm
testing the v2 of your patch. Please wait a minute.

Gavin Guo