[-v3] PCI, ACPI, hotplug: Fix BUS_CHECK event handle on root bridge

Commit Message

Yinghai Lu April 26, 2013, 1:46 a.m. UTC

Gavin found that acpiphp does not handle hotplug anymore even after
now we have acpiphp built-in preparing for v3.10.

Bjorn analyzed bootlog, he found that 
    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
And that means:
   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.

Also according to Rafael and Bjorn, it is perfect fine that we
should enumerate bus by sending event to root bridge after hotadd
device to slots under that root bridge or children bridges.

It turns out that it is regression caused by
| commit 668192b678201d2fff27c6cc76bb003c1ec4a52a
| Author: Yinghai Lu <[e-mail]>
| Date:   Mon Jan 21 13:20:48 2013 -0800
|
|    PCI: acpiphp: Move host bridge hotplug to pci_root.c

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

Restore the old behavoir by calling acpi_check_bridge and check_sub_bridge
in acpiphp.

Jiang Liu acctually have simimar patch but it forgets calling
acpi_check_bridge() for system that have slots on root bus directly.
That is still valid, as in QEMU we still have that slots on bus 0 at
least. But my first version patch wrongly check if root bridge exists
before check_sub_bridge for children bridges. 

-v2: Don't check bridge for acpi_walk_namespace with check_sub_bridges.
     also put back bridge reference.
-v3: More changelog and etc.

Reported-by: Gavin Guo <[e-mail]>
Tested-by: Gavin Guo <[e-mail]>
Signed-off-by: Yinghai Lu <[e-mail]>

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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Rafael Wysocki April 26, 2013, 10:58 a.m. UTC | #1

On Thursday, April 25, 2013 06:46:38 PM Yinghai Lu wrote:
> Gavin found that acpiphp does not handle hotplug anymore even after
> now we have acpiphp built-in preparing for v3.10.
> 
> Bjorn analyzed bootlog, he found that 
>     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
> And that means:
>    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.
> 
> Also according to Rafael and Bjorn, it is perfect fine that we
> should enumerate bus by sending event to root bridge after hotadd
> device to slots under that root bridge or children bridges.
> 
> It turns out that it is regression caused by
> | commit 668192b678201d2fff27c6cc76bb003c1ec4a52a
> | Author: Yinghai Lu <[e-mail]>
> | Date:   Mon Jan 21 13:20:48 2013 -0800
> |
> |    PCI: acpiphp: Move host bridge hotplug to pci_root.c
> 
> We should check slots when BUS_CHECK is sent to root bridge acpi handle.
> 
> Restore the old behavoir by calling acpi_check_bridge and check_sub_bridge
> in acpiphp.
> 
> Jiang Liu acctually have simimar patch but it forgets calling
> acpi_check_bridge() for system that have slots on root bus directly.
> That is still valid, as in QEMU we still have that slots on bus 0 at
> least. But my first version patch wrongly check if root bridge exists
> before check_sub_bridge for children bridges. 
> 
> -v2: Don't check bridge for acpi_walk_namespace with check_sub_bridges.
>      also put back bridge reference.
> -v3: More changelog and etc.
> 
> Reported-by: Gavin Guo <[e-mail]>
> Tested-by: Gavin Guo <[e-mail]>
> Signed-off-by: Yinghai Lu <[e-mail]>

Acked-by: Rafael J. Wysocki <[e-mail]>

> ---
>  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 */

Jiang Liu April 29, 2013, 4:19 p.m. UTC | #2

Hi Yinghai,
	Reviewed-by: Jiang Liu <[e-mail]>
	Thanks!

On 04/26/2013 09:46 AM, Yinghai Lu wrote:
> Gavin found that acpiphp does not handle hotplug anymore even after
> now we have acpiphp built-in preparing for v3.10.
> 
> Bjorn analyzed bootlog, he found that 
>     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
> And that means:
>    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.
> 
> Also according to Rafael and Bjorn, it is perfect fine that we
> should enumerate bus by sending event to root bridge after hotadd
> device to slots under that root bridge or children bridges.
> 
> It turns out that it is regression caused by
> | commit 668192b678201d2fff27c6cc76bb003c1ec4a52a
> | Author: Yinghai Lu <[e-mail]>
> | Date:   Mon Jan 21 13:20:48 2013 -0800
> |
> |    PCI: acpiphp: Move host bridge hotplug to pci_root.c
> 
> We should check slots when BUS_CHECK is sent to root bridge acpi handle.
> 
> Restore the old behavoir by calling acpi_check_bridge and check_sub_bridge
> in acpiphp.
> 
> Jiang Liu acctually have simimar patch but it forgets calling
> acpi_check_bridge() for system that have slots on root bus directly.
> That is still valid, as in QEMU we still have that slots on bus 0 at
> least. But my first version patch wrongly check if root bridge exists
> before check_sub_bridge for children bridges. 
> 
> -v2: Don't check bridge for acpi_walk_namespace with check_sub_bridges.
>      also put back bridge reference.
> -v3: More changelog and etc.
> 
> Reported-by: Gavin Guo <[e-mail]>
> Tested-by: Gavin Guo <[e-mail]>
> Signed-off-by: Yinghai Lu <[e-mail]>
> 
> ---
>  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 */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas May 10, 2013, 5:09 p.m. UTC | #3

On Mon, Apr 29, 2013 at 10:19 AM, Jiang Liu <[e-mail]> wrote:
> Hi Yinghai,
>         Reviewed-by: Jiang Liu <[e-mail]>
>         Thanks!
>
> On 04/26/2013 09:46 AM, Yinghai Lu wrote:
>> Gavin found that acpiphp does not handle hotplug anymore even after
>> now we have acpiphp built-in preparing for v3.10.
>>
>> Bjorn analyzed bootlog, he found that
>>     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
>> And that means:
>>    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.
>>
>> Also according to Rafael and Bjorn, it is perfect fine that we
>> should enumerate bus by sending event to root bridge after hotadd
>> device to slots under that root bridge or children bridges.
>>
>> It turns out that it is regression caused by
>> | commit 668192b678201d2fff27c6cc76bb003c1ec4a52a
>> | Author: Yinghai Lu <[e-mail]>
>> | Date:   Mon Jan 21 13:20:48 2013 -0800
>> |
>> |    PCI: acpiphp: Move host bridge hotplug to pci_root.c
>>
>> We should check slots when BUS_CHECK is sent to root bridge acpi handle.
>>
>> Restore the old behavoir by calling acpi_check_bridge and check_sub_bridge
>> in acpiphp.
>>
>> Jiang Liu acctually have simimar patch but it forgets calling
>> acpi_check_bridge() for system that have slots on root bus directly.
>> That is still valid, as in QEMU we still have that slots on bus 0 at
>> least. But my first version patch wrongly check if root bridge exists
>> before check_sub_bridge for children bridges.
>>
>> -v2: Don't check bridge for acpi_walk_namespace with check_sub_bridges.
>>      also put back bridge reference.
>> -v3: More changelog and etc.
>>
>> Reported-by: Gavin Guo <[e-mail]>
>> Tested-by: Gavin Guo <[e-mail]>
>> Signed-off-by: Yinghai Lu <[e-mail]>

I added the Acked-by and Reviewed-by and opened
https://bugzilla.kernel.org/show_bug.cgi?id=57961 just as a place to
store the dmesg logs.  I also backported it to v3.9 and added a CC:
stable tag because I think v3.9 has the same issue.  The backport is
also attached to the bugzilla.

I'll ask Linus to put this in v3.10.

Thanks,
  Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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 */