[PATCH 0/5] PCI: hotplug related misc patches

[PATCH 0/5] PCI: hotplug related misc patches

[Yinghai Lu]

minor changes that should be safe for 3.6

could get from
        git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-next

Yinghai Lu (5):
  PCI, acpiphp: remove not used res_lock
  pciehp: Don't enable presence notification while surprise removal is
    not supported.
  PCI, acpiphp: Merge acpiphp_debug and debug
  PCI, acpiphp: add is_hotplug_bridge detection
  PCI: add root bus children dev's res to fail list

 drivers/pci/hotplug/acpiphp.h      |    4 +---
 drivers/pci/hotplug/acpiphp_core.c |    7 ++-----
 drivers/pci/hotplug/acpiphp_glue.c |   30 ++++++++++++++++++++++++++----
 drivers/pci/hotplug/pciehp_hpc.c   |    5 +++--
 drivers/pci/setup-bus.c            |    2 +-
 5 files changed, 33 insertions(+), 15 deletions(-)

-- 
1.7.7

[PATCH 1/5] PCI, acpiphp: remove not used res_lock

[Yinghai Lu]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/hotplug/acpiphp.h      |    2 --
 drivers/pci/hotplug/acpiphp_glue.c |    3 ---
 2 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index 7722108..6b58ed0 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -89,8 +89,6 @@ struct acpiphp_bridge {
 
 	/* PCI-to-PCI bridge device */
 	struct pci_dev *pci_dev;
-
-	spinlock_t res_lock;
 };
 
 
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 395c67d..ad6fd66 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -396,8 +396,6 @@ static void add_host_bridge(acpi_handle *handle)
 
 	bridge->pci_bus = root->bus;
 
-	spin_lock_init(&bridge->res_lock);
-
 	init_bridge_misc(bridge);
 }
 
@@ -430,7 +428,6 @@ static void add_p2p_bridge(acpi_handle *handle)
 	 * (which we access during module unload).
 	 */
 	get_device(&bridge->pci_bus->dev);
-	spin_lock_init(&bridge->res_lock);
 
 	init_bridge_misc(bridge);
 	return;
-- 
1.7.7

[PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Yinghai Lu]

If surprise removal is not supported, that event get dropped later.
So there is no point to enable that.

Also some sick chipset have those bit flip around when the card is not present.
and make log full of useless warning.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/hotplug/pciehp_hpc.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index a960fae..d4fa705 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -808,7 +808,7 @@ int pciehp_get_cur_lnk_width(struct slot *slot,
 
 int pcie_enable_notification(struct controller *ctrl)
 {
-	u16 cmd, mask;
+	u16 cmd = 0, mask;
 
 	/*
 	 * TBD: Power fault detected software notification support.
@@ -820,7 +820,8 @@ int pcie_enable_notification(struct controller *ctrl)
 	 * when it is cleared in the interrupt service routine, and
 	 * next power fault detected interrupt was notified again.
 	 */
-	cmd = PCI_EXP_SLTCTL_PDCE;
+	if (HP_SUPR_RM(ctrl))
+		cmd |= PCI_EXP_SLTCTL_PDCE;
 	if (ATTN_BUTTN(ctrl))
 		cmd |= PCI_EXP_SLTCTL_ABPE;
 	if (MRL_SENS(ctrl))
-- 
1.7.7

RE: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Kaneshige, Kenji]

> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org
> [mailto:linux-pci-owner@vger.kernel.org] On Behalf Of Yinghai Lu
> Sent: Saturday, June 23, 2012 4:42 PM
> To: Bjorn Helgaas
> Cc: linux-pci@vger.kernel.org; Yinghai Lu
> Subject: [PATCH 2/5] pciehp: Don't enable presence notification while
> surprise removal is not supported.
> 
> If surprise removal is not supported, that event get dropped later.
> So there is no point to enable that.

Presence changed event is currently just for printing a kernel message when
surprise removal is not supported, and I think the message is not so important.
So I agree this change.

Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

> 
> Also some sick chipset have those bit flip around when the card is not
> present.
> and make log full of useless warning.

Is there any other way to avoid this issue other than disabling presence
changed notification? If we need enabling presence changed event in the
future, this will happen again. I'm worrying about this.

Regards,
Kenji Kaneshige


> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c
> b/drivers/pci/hotplug/pciehp_hpc.c
> index a960fae..d4fa705 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -808,7 +808,7 @@ int pciehp_get_cur_lnk_width(struct slot *slot,
> 
>  int pcie_enable_notification(struct controller *ctrl)
>  {
> -	u16 cmd, mask;
> +	u16 cmd = 0, mask;
> 
>  	/*
>  	 * TBD: Power fault detected software notification support.
> @@ -820,7 +820,8 @@ int pcie_enable_notification(struct controller *ctrl)
>  	 * when it is cleared in the interrupt service routine, and
>  	 * next power fault detected interrupt was notified again.
>  	 */
> -	cmd = PCI_EXP_SLTCTL_PDCE;
> +	if (HP_SUPR_RM(ctrl))
> +		cmd |= PCI_EXP_SLTCTL_PDCE;
>  	if (ATTN_BUTTN(ctrl))
>  		cmd |= PCI_EXP_SLTCTL_ABPE;
>  	if (MRL_SENS(ctrl))
> --
> 1.7.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Yinghai Lu]

On Mon, Jun 25, 2012 at 6:26 PM, Kaneshige, Kenji
<kaneshige.kenji@jp.fujitsu.com> wrote:
>> -----Original Message-----
>> From: linux-pci-owner@vger.kernel.org
>> [mailto:linux-pci-owner@vger.kernel.org] On Behalf Of Yinghai Lu
>> Sent: Saturday, June 23, 2012 4:42 PM
>> To: Bjorn Helgaas
>> Cc: linux-pci@vger.kernel.org; Yinghai Lu
>> Subject: [PATCH 2/5] pciehp: Don't enable presence notification while
>> surprise removal is not supported.
>>
>> If surprise removal is not supported, that event get dropped later.
>> So there is no point to enable that.
>
> Presence changed event is currently just for printing a kernel message when
> surprise removal is not supported, and I think the message is not so important.
> So I agree this change.
>
> Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

Thanks

>
>>
>> Also some sick chipset have those bit flip around when the card is not
>> present.
>> and make log full of useless warning.
>
> Is there any other way to avoid this issue other than disabling presence
> changed notification? If we need enabling presence changed event in the
> future, this will happen again. I'm worrying about this.

could be fixed from HW change and CPLD change.

Thanks

Yinghai

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Bjorn Helgaas]

On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> If surprise removal is not supported, that event get dropped later.
> So there is no point to enable that.
>
> Also some sick chipset have those bit flip around when the card is not present.
> and make log full of useless warning.

HP_SUPR_RM tests the Slot Capabilities "Hot-Plug Surprise" bit, which
indicates that an adapter might be *removed* without prior
notification (sec 7.8.9).

PCI_EXP_SLTCTL_PDCE is the Slot Control "Presence Detect Changed
Enable" bit, which enables interrupts for any change in the Slot
Status "Presence Detect State", i.e., we may get interrupts for either
add or remove events.

In interrupt_event_handler(), we drop both add and remove events if
!HP_SUPR_RM().  Specifically, we drop *add* events if surprise
*removal* isn't supported.  That seems strange -- just from reading
the spec, it seems that a surprise *add* could occur even if the
"Hot-Plug Surprise" bit is not set.

So I'm not convinced that we should even bother looking at the
"Hot-Plug Surprise" bit...  Maybe we'd be better off if we just
removed that HP_SUPR_RM() test in interrupt_event_handler() and always
called handle_surprise_event().

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index a960fae..d4fa705 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -808,7 +808,7 @@ int pciehp_get_cur_lnk_width(struct slot *slot,
>
>  int pcie_enable_notification(struct controller *ctrl)
>  {
> -       u16 cmd, mask;
> +       u16 cmd = 0, mask;
>
>         /*
>          * TBD: Power fault detected software notification support.
> @@ -820,7 +820,8 @@ int pcie_enable_notification(struct controller *ctrl)
>          * when it is cleared in the interrupt service routine, and
>          * next power fault detected interrupt was notified again.
>          */
> -       cmd = PCI_EXP_SLTCTL_PDCE;
> +       if (HP_SUPR_RM(ctrl))
> +               cmd |= PCI_EXP_SLTCTL_PDCE;
>         if (ATTN_BUTTN(ctrl))
>                 cmd |= PCI_EXP_SLTCTL_ABPE;
>         if (MRL_SENS(ctrl))
> --
> 1.7.7
>

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Yinghai Lu]

On Tue, Jul 10, 2012 at 3:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> If surprise removal is not supported, that event get dropped later.
>> So there is no point to enable that.
>>
>> Also some sick chipset have those bit flip around when the card is not present.
>> and make log full of useless warning.
>
> HP_SUPR_RM tests the Slot Capabilities "Hot-Plug Surprise" bit, which
> indicates that an adapter might be *removed* without prior
> notification (sec 7.8.9).
>
> PCI_EXP_SLTCTL_PDCE is the Slot Control "Presence Detect Changed
> Enable" bit, which enables interrupts for any change in the Slot
> Status "Presence Detect State", i.e., we may get interrupts for either
> add or remove events.
>
> In interrupt_event_handler(), we drop both add and remove events if
> !HP_SUPR_RM().  Specifically, we drop *add* events if surprise
> *removal* isn't supported.  That seems strange -- just from reading
> the spec, it seems that a surprise *add* could occur even if the
> "Hot-Plug Surprise" bit is not set.

Interesting, that means after card is plugged in, we don't need to
press attention button to bring
it online.

And other oses like windows and solaris do need to press the button
like current linux doing.

>
> So I'm not convinced that we should even bother looking at the
> "Hot-Plug Surprise" bit...  Maybe we'd be better off if we just
> removed that HP_SUPR_RM() test in interrupt_event_handler() and always
> called handle_surprise_event().

Ah, that means some system with chip problem will keep feeding
pciehp_wq with pciehp_disable_slot and pciehp_enable_slot.

Anyway please drop this patch now.

Thanks

Yinghai

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Bjorn Helgaas]

On Tue, Jul 10, 2012 at 5:12 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Jul 10, 2012 at 3:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> If surprise removal is not supported, that event get dropped later.
>>> So there is no point to enable that.
>>>
>>> Also some sick chipset have those bit flip around when the card is not present.
>>> and make log full of useless warning.
>>
>> HP_SUPR_RM tests the Slot Capabilities "Hot-Plug Surprise" bit, which
>> indicates that an adapter might be *removed* without prior
>> notification (sec 7.8.9).
>>
>> PCI_EXP_SLTCTL_PDCE is the Slot Control "Presence Detect Changed
>> Enable" bit, which enables interrupts for any change in the Slot
>> Status "Presence Detect State", i.e., we may get interrupts for either
>> add or remove events.
>>
>> In interrupt_event_handler(), we drop both add and remove events if
>> !HP_SUPR_RM().  Specifically, we drop *add* events if surprise
>> *removal* isn't supported.  That seems strange -- just from reading
>> the spec, it seems that a surprise *add* could occur even if the
>> "Hot-Plug Surprise" bit is not set.
>
> Interesting, that means after card is plugged in, we don't need to
> press attention button to bring
> it online.
>
> And other oses like windows and solaris do need to press the button
> like current linux doing.

The attention button is apparently optional, depending on the form factor.

I would guess that on a platform with no attention button and Hot-Plug
Surprise == 0, Linux would be unable to do a hot-add.

>> So I'm not convinced that we should even bother looking at the
>> "Hot-Plug Surprise" bit...  Maybe we'd be better off if we just
>> removed that HP_SUPR_RM() test in interrupt_event_handler() and always
>> called handle_surprise_event().
>
> Ah, that means some system with chip problem will keep feeding
> pciehp_wq with pciehp_disable_slot and pciehp_enable_slot.
>
> Anyway please drop this patch now.

Dropped.

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Yinghai Lu]

On Tue, Jul 10, 2012 at 4:28 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Jul 10, 2012 at 5:12 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> Interesting, that means after card is plugged in, we don't need to
>> press attention button to bring
>> it online.
>>
>> And other oses like windows and solaris do need to press the button
>> like current linux doing.
>
> The attention button is apparently optional, depending on the form factor.
>
> I would guess that on a platform with no attention button and Hot-Plug
> Surprise == 0, Linux would be unable to do a hot-add.

yes.

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Bjorn Helgaas]

On Tue, Jul 10, 2012 at 04:56:15PM -0600, Bjorn Helgaas wrote:
> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> > If surprise removal is not supported, that event get dropped later.
> > So there is no point to enable that.
> >
> > Also some sick chipset have those bit flip around when the card is not present.
> > and make log full of useless warning.
> 
> HP_SUPR_RM tests the Slot Capabilities "Hot-Plug Surprise" bit, which
> indicates that an adapter might be *removed* without prior
> notification (sec 7.8.9).
> 
> PCI_EXP_SLTCTL_PDCE is the Slot Control "Presence Detect Changed
> Enable" bit, which enables interrupts for any change in the Slot
> Status "Presence Detect State", i.e., we may get interrupts for either
> add or remove events.
> 
> In interrupt_event_handler(), we drop both add and remove events if
> !HP_SUPR_RM().  Specifically, we drop *add* events if surprise
> *removal* isn't supported.  That seems strange -- just from reading
> the spec, it seems that a surprise *add* could occur even if the
> "Hot-Plug Surprise" bit is not set.
> 
> So I'm not convinced that we should even bother looking at the
> "Hot-Plug Surprise" bit...  Maybe we'd be better off if we just
> removed that HP_SUPR_RM() test in interrupt_event_handler() and always
> called handle_surprise_event().

What bad things would happen if we did the following?

I have no way to test this, but I don't understand what benefit
there is in testing HP_SUPR_RM() here.

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 27f4429..4bbe257 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -463,9 +463,8 @@ static void interrupt_event_handler(struct work_struct *work)
 		break;
 	case INT_PRESENCE_ON:
 	case INT_PRESENCE_OFF:
-		if (!HP_SUPR_RM(ctrl))
-			break;
-		ctrl_dbg(ctrl, "Surprise Removal\n");
+		ctrl_dbg(ctrl, "Presence Detect changed (now %spresent)\n",
+			 info->event_type == INT_PRESENCE_OFF ? "not " : "");
 		handle_surprise_event(p_slot);
 		break;
 	default:

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Yinghai Lu]

On Wed, Jul 11, 2012 at 9:21 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Jul 10, 2012 at 04:56:15PM -0600, Bjorn Helgaas wrote:
>
> What bad things would happen if we did the following?
>
> I have no way to test this, but I don't understand what benefit
> there is in testing HP_SUPR_RM() here.
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 27f4429..4bbe257 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -463,9 +463,8 @@ static void interrupt_event_handler(struct work_struct *work)
>                 break;
>         case INT_PRESENCE_ON:
>         case INT_PRESENCE_OFF:
> -               if (!HP_SUPR_RM(ctrl))
> -                       break;
> -               ctrl_dbg(ctrl, "Surprise Removal\n");
> +               ctrl_dbg(ctrl, "Presence Detect changed (now %spresent)\n",
> +                        info->event_type == INT_PRESENCE_OFF ? "not " : "");
>                 handle_surprise_event(p_slot);
>                 break;
>         default:

some chipsets (from cpu vendor) have some problem, when you remove the
card, the present bit
will keep flip around.
Because that present bit is "OR" with inband (pcie link) and outband
(input from VPP) detection.
and silicon is trying to retrain the link to see if there is any device there.
That means handle_surprise_event would get called frequently.

Thanks

Yinghai

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Bjorn Helgaas]

On Wed, Jul 11, 2012 at 11:58 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Jul 11, 2012 at 9:21 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Jul 10, 2012 at 04:56:15PM -0600, Bjorn Helgaas wrote:
>>
>> What bad things would happen if we did the following?
>>
>> I have no way to test this, but I don't understand what benefit
>> there is in testing HP_SUPR_RM() here.
>>
>> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
>> index 27f4429..4bbe257 100644
>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>> @@ -463,9 +463,8 @@ static void interrupt_event_handler(struct work_struct *work)
>>                 break;
>>         case INT_PRESENCE_ON:
>>         case INT_PRESENCE_OFF:
>> -               if (!HP_SUPR_RM(ctrl))
>> -                       break;
>> -               ctrl_dbg(ctrl, "Surprise Removal\n");
>> +               ctrl_dbg(ctrl, "Presence Detect changed (now %spresent)\n",
>> +                        info->event_type == INT_PRESENCE_OFF ? "not " : "");
>>                 handle_surprise_event(p_slot);
>>                 break;
>>         default:
>
> some chipsets (from cpu vendor) have some problem, when you remove the
> card, the present bit
> will keep flip around.
> Because that present bit is "OR" with inband (pcie link) and outband
> (input from VPP) detection.
> and silicon is trying to retrain the link to see if there is any device there.
> That means handle_surprise_event would get called frequently.

What's the connection with HP_SUPR_RM()?  Is it just a coincidence
that chipsets that set the "Hot-Plug Surprise" bit don't have this
problem with the Presence Detect State bit?

Using HP_SUPR_RM() seems like a totally bogus way to work around a
presence detect issue.

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Yinghai Lu]

On Wed, Jul 11, 2012 at 11:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> What's the connection with HP_SUPR_RM()?  Is it just a coincidence
> that chipsets that set the "Hot-Plug Surprise" bit don't have this
> problem with the Presence Detect State bit?
>
> Using HP_SUPR_RM() seems like a totally bogus way to work around a
> presence detect issue.

then we should blame the spec.

and if you do the above changing, when plug the card into system,
kernel will bring that card online automatically without press
attention button.
that will be big change.

Thanks

Yinghai

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Bjorn Helgaas]

On Wed, Jul 11, 2012 at 12:49 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Jul 11, 2012 at 11:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> What's the connection with HP_SUPR_RM()?  Is it just a coincidence
>> that chipsets that set the "Hot-Plug Surprise" bit don't have this
>> problem with the Presence Detect State bit?
>>
>> Using HP_SUPR_RM() seems like a totally bogus way to work around a
>> presence detect issue.
>
> then we should blame the spec.

What specifically are you referring to?  I see this Presence Detect State text:

  Presence Detect State – This bit indicates the presence of an
  adapter in the slot, reflected by the logical “OR” of the Physical
  Layer in-band presence detect mechanism and, if present, any
  out-of-band presence detect mechanism defined for the slot’s
  corresponding form factor. Note that the in-band presence
  detect mechanism requires that power be applied to an adapter
  for its presence to be detected. Consequently, form factors that
  require a power controller for hot-plug must implement a
  physical pin presence detect mechanism.

But I don't yet see the connection with the Hot-Plug Surprise bit.

> and if you do the above changing, when plug the card into system,
> kernel will bring that card online automatically without press
> attention button.
> that will be big change.

I don't want to make a fundamental change in behavior like that.  I'm
just trying to understand why we should handle Presence Detect
differently based on the Hot-Plug Surprise bit.

The attention button is optional.  What happens today when you plug a
card into a slot with no attention button?

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Yinghai Lu]

On Wed, Jul 11, 2012 at 12:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Jul 11, 2012 at 12:49 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Jul 11, 2012 at 11:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>
>>> What's the connection with HP_SUPR_RM()?  Is it just a coincidence
>>> that chipsets that set the "Hot-Plug Surprise" bit don't have this
>>> problem with the Presence Detect State bit?
>>>
>>> Using HP_SUPR_RM() seems like a totally bogus way to work around a
>>> presence detect issue.
>>
>> then we should blame the spec.
>
> What specifically are you referring to?  I see this Presence Detect State text:
>
>   Presence Detect State – This bit indicates the presence of an
>   adapter in the slot, reflected by the logical “OR” of the Physical
>   Layer in-band presence detect mechanism and, if present, any
>   out-of-band presence detect mechanism defined for the slot’s
>   corresponding form factor. Note that the in-band presence
>   detect mechanism requires that power be applied to an adapter
>   for its presence to be detected. Consequently, form factors that
>   require a power controller for hot-plug must implement a
>   physical pin presence detect mechanism.
>
> But I don't yet see the connection with the Hot-Plug Surprise bit.

Spec does not mention about surprise add clearly.

>
>> and if you do the above changing, when plug the card into system,
>> kernel will bring that card online automatically without press
>> attention button.
>> that will be big change.
>
> I don't want to make a fundamental change in behavior like that.  I'm
> just trying to understand why we should handle Presence Detect
> differently based on the Hot-Plug Surprise bit.

When hotplug surprise is supported, attention button may not there.
So have to use present bit to trigger the sequence online work, and
offline clean up work.

When hotplug surprise is not there, why should we handle Presence Bit change?

>
> The attention button is optional.  What happens today when you plug a
> card into a slot with no attention button?
if the slot support hotplug surprise, that card will be online automatically.
if the slot does not support hotplug surprise, but that slot have
power control,  then could use need sw interface /sys/..../power to
turn on the power and bring that card online.


Yinghai

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Bjorn Helgaas]

On Wed, Jul 11, 2012 at 2:28 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Jul 11, 2012 at 12:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Wed, Jul 11, 2012 at 12:49 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> On Wed, Jul 11, 2012 at 11:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>>
>>>> What's the connection with HP_SUPR_RM()?  Is it just a coincidence
>>>> that chipsets that set the "Hot-Plug Surprise" bit don't have this
>>>> problem with the Presence Detect State bit?
>>>>
>>>> Using HP_SUPR_RM() seems like a totally bogus way to work around a
>>>> presence detect issue.
>>>
>>> then we should blame the spec.
>>
>> What specifically are you referring to?  I see this Presence Detect State text:
>>
>>   Presence Detect State – This bit indicates the presence of an
>>   adapter in the slot, reflected by the logical “OR” of the Physical
>>   Layer in-band presence detect mechanism and, if present, any
>>   out-of-band presence detect mechanism defined for the slot’s
>>   corresponding form factor. Note that the in-band presence
>>   detect mechanism requires that power be applied to an adapter
>>   for its presence to be detected. Consequently, form factors that
>>   require a power controller for hot-plug must implement a
>>   physical pin presence detect mechanism.
>>
>> But I don't yet see the connection with the Hot-Plug Surprise bit.
>
> Spec does not mention about surprise add clearly.

No, in fact the Hot-Plug Surprise description mentions *removal* twice
and doesn't mention *add* at all.

>>> and if you do the above changing, when plug the card into system,
>>> kernel will bring that card online automatically without press
>>> attention button.
>>> that will be big change.
>>
>> I don't want to make a fundamental change in behavior like that.  I'm
>> just trying to understand why we should handle Presence Detect
>> differently based on the Hot-Plug Surprise bit.
>
> When hotplug surprise is supported, attention button may not there.
> So have to use present bit to trigger the sequence online work, and
> offline clean up work.

Well, there is an "Attention Button Present" bit.  Why wouldn't we use
that instead of trying to infer the button's presence from Hot-Plug
Surprise?

> When hotplug surprise is not there, why should we handle Presence Bit change?
>
>>
>> The attention button is optional.  What happens today when you plug a
>> card into a slot with no attention button?
> if the slot support hotplug surprise, that card will be online automatically.
> if the slot does not support hotplug surprise, but that slot have
> power control,  then could use need sw interface /sys/..../power to
> turn on the power and bring that card online.
>
>
> Yinghai

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Yinghai Lu]

On Wed, Jul 11, 2012 at 1:48 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> When hotplug surprise is supported, attention button may not there.
>> So have to use present bit to trigger the sequence online work, and
>> offline clean up work.
>
> Well, there is an "Attention Button Present" bit.  Why wouldn't we use
> that instead of trying to infer the button's presence from Hot-Plug
> Surprise?

so you want this ?

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c
b/drivers/pci/hotplug/pciehp_ctrl.c
index 27f4429..f103a4ca 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -463,7 +463,7 @@ static void interrupt_event_handler(struct
work_struct *work)
                break;
        case INT_PRESENCE_ON:
        case INT_PRESENCE_OFF:
-               if (!HP_SUPR_RM(ctrl))
+               if (ATTN_BUTTN(ctrl))
                        break;
                ctrl_dbg(ctrl, "Surprise Removal\n");
                handle_surprise_event(p_slot);

that should keep current user expected behavior that kernel only bring
the card online after press the button.

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Bjorn Helgaas]

On Wed, Jul 11, 2012 at 2:56 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Jul 11, 2012 at 1:48 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>
>>> When hotplug surprise is supported, attention button may not there.
>>> So have to use present bit to trigger the sequence online work, and
>>> offline clean up work.
>>
>> Well, there is an "Attention Button Present" bit.  Why wouldn't we use
>> that instead of trying to infer the button's presence from Hot-Plug
>> Surprise?
>
> so you want this ?
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c
> b/drivers/pci/hotplug/pciehp_ctrl.c
> index 27f4429..f103a4ca 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -463,7 +463,7 @@ static void interrupt_event_handler(struct
> work_struct *work)
>                 break;
>         case INT_PRESENCE_ON:
>         case INT_PRESENCE_OFF:
> -               if (!HP_SUPR_RM(ctrl))
> +               if (ATTN_BUTTN(ctrl))
>                         break;
>                 ctrl_dbg(ctrl, "Surprise Removal\n");
>                 handle_surprise_event(p_slot);
>
> that should keep current user expected behavior that kernel only bring
> the card online after press the button.

Well, I was actually only interested in reviewing your original patch,
and this seemed like a possible bug.  If it's not a bug, I don't want
to change anything.

If we *do* want to change something there, I don't like the proposal
above any better.  It's still basically saying "presence detect is
only reliable when X is set" when X is not clearly related to presence
detect.

I think it's better to disable the presence detect interrupt
completely if it's not reliable, as your original patch did.  My
complaint with that is that HP_SUPR_RM() doesn't seem like the right
test for "the interrupt is not reliable."

Having a "Presence Detect State" bit and an interrupt that tells you
when it changed is only meaningful if that bit gives you useful
information.  If hardware supplies that bit but it toggles all the
time when the slot is empty because it's hooked up to link training
attempts, that just means the hardware screwed up.  The hardware
*should* have included some logic to filter out the attempts and
toggle the bit only when a card is actually added or removed.  I
believe the functionality of "Presence Detect State" is logically
independent of "Hot-Plug Surprise" and "Attention Button Present."

So if we want to disable the "Presence Detect Changed" interrupt,
that's fine, but I think we should do it based on a quirk or
blacklist, or based on the fact that we have no need for it.  One
reason to want the interrupt is if "Hot-Plug Surprise" is set,
indicating that an adapter might be removed without notice, and if
that's the only reason, we could use your original patch.  But if we
do, I think we should change interrupt_event_handler() to look
something like this:

    case INT_PRESENCE_ON:
        if (!ATTN_BUTTN(ctrl))
            handle_surprise_event(p_slot);  /* omit this if you don't
think it's useful */
        break;
    case INT_PRESENCE_OFF:
        handle_surprise_event(p_slot);
        break;

If you did make a change like this, I propose (as a separate patch)
passing info->event_type into handle_surprise_event().  We've already
read the "Presence Detect State" bit, so there's no need for
handle_surprise_event() to do it again.

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Yinghai Lu]

On Wed, Jul 11, 2012 at 3:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

> If we *do* want to change something there, I don't like the proposal
> above any better.  It's still basically saying "presence detect is
> only reliable when X is set" when X is not clearly related to presence
> detect.
>
> I think it's better to disable the presence detect interrupt
> completely if it's not reliable, as your original patch did.  My
> complaint with that is that HP_SUPR_RM() doesn't seem like the right
> test for "the interrupt is not reliable."

ok.

>
> Having a "Presence Detect State" bit and an interrupt that tells you
> when it changed is only meaningful if that bit gives you useful
> information.  If hardware supplies that bit but it toggles all the
> time when the slot is empty because it's hooked up to link training
> attempts, that just means the hardware screwed up.  The hardware
> *should* have included some logic to filter out the attempts and
> toggle the bit only when a card is actually added or removed.  I
> believe the functionality of "Presence Detect State" is logically
> independent of "Hot-Plug Surprise" and "Attention Button Present."

the cpu vendor already agreed that is out of spec for that.

>
> So if we want to disable the "Presence Detect Changed" interrupt,
> that's fine, but I think we should do it based on a quirk or
> blacklist, or based on the fact that we have no need for it.  One
> reason to want the interrupt is if "Hot-Plug Surprise" is set,
> indicating that an adapter might be removed without notice, and if
> that's the only reason, we could use your original patch.

no,  with that patch, we will not get interrupt for present bit change
for non-hotplug-surprise
case.

> But if we
> do, I think we should change interrupt_event_handler() to look
> something like this:
>
>     case INT_PRESENCE_ON:
>         if (!ATTN_BUTTN(ctrl))
>             handle_surprise_event(p_slot);  /* omit this if you don't
> think it's useful */
>         break;
>     case INT_PRESENCE_OFF:
>         handle_surprise_event(p_slot);
>         break;

yes, this one should be good. and it is enhancement.

>
> If you did make a change like this, I propose (as a separate patch)
> passing info->event_type into handle_surprise_event().  We've already
> read the "Presence Detect State" bit, so there's no need for
> handle_surprise_event() to do it again.

ok. will prepare patches for that.

Yinghai

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Bjorn Helgaas]

On Wed, Jul 11, 2012 at 6:05 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Jul 11, 2012 at 3:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> If we *do* want to change something there, I don't like the proposal
>> above any better.  It's still basically saying "presence detect is
>> only reliable when X is set" when X is not clearly related to presence
>> detect.
>>
>> I think it's better to disable the presence detect interrupt
>> completely if it's not reliable, as your original patch did.  My
>> complaint with that is that HP_SUPR_RM() doesn't seem like the right
>> test for "the interrupt is not reliable."
>
> ok.
>
>>
>> Having a "Presence Detect State" bit and an interrupt that tells you
>> when it changed is only meaningful if that bit gives you useful
>> information.  If hardware supplies that bit but it toggles all the
>> time when the slot is empty because it's hooked up to link training
>> attempts, that just means the hardware screwed up.  The hardware
>> *should* have included some logic to filter out the attempts and
>> toggle the bit only when a card is actually added or removed.  I
>> believe the functionality of "Presence Detect State" is logically
>> independent of "Hot-Plug Surprise" and "Attention Button Present."
>
> the cpu vendor already agreed that is out of spec for that.
>
>>
>> So if we want to disable the "Presence Detect Changed" interrupt,
>> that's fine, but I think we should do it based on a quirk or
>> blacklist, or based on the fact that we have no need for it.  One
>> reason to want the interrupt is if "Hot-Plug Surprise" is set,
>> indicating that an adapter might be removed without notice, and if
>> that's the only reason, we could use your original patch.
>
> no,  with that patch, we will not get interrupt for present bit change
> for non-hotplug-surprise
> case.
>
>> But if we
>> do, I think we should change interrupt_event_handler() to look
>> something like this:
>>
>>     case INT_PRESENCE_ON:

        ctrl_info(ctrl, "adapter now present\n");

>>         if (!ATTN_BUTTN(ctrl))
>>             handle_surprise_event(p_slot);  /* omit this if you don't
>> think it's useful */
>>         break;
>>     case INT_PRESENCE_OFF:

        ctrl_warn(ctrl, "adapter removed unexpectedly\n");

>>         handle_surprise_event(p_slot);
>>         break;
>
> yes, this one should be good. and it is enhancement.

When we don't have an attention button, I don't know that it's a good
idea to automatically power up a new card.  I was thinking about
things like ExpressCard, where the standard usage model probably *is*
"plug in the card and it automatically starts working, no button press
or software UI required."  But my guess is that ExpressCard would not
be handled by pciehp, would it?

The pciehp flow seems to be basically the same as SHPC, and the
Standard Hot-Plug Controller spec, rev 1.0, sec 2.5, has a flow of
hot-add without an attention button.  It shows:

  1) User selects empty, disabled slot and opens MRL
  2) Adapter insertion:
      2a) User inserts add-in-card
      2b) User closes MRL
      2c) User attaches cables
  3) User requests via software UI that slot be enabled

We'll get the Presence Detect interrupt at step 2a, before the user
can close the MRL or attach cables.  I don't know if it's a good thing
to power-on the slot and attach the driver while this is happening.
It certainly doesn't seem to follow the intent of the spec.

>> If you did make a change like this, I propose (as a separate patch)
>> passing info->event_type into handle_surprise_event().  We've already
>> read the "Presence Detect State" bit, so there's no need for
>> handle_surprise_event() to do it again.
>
> ok. will prepare patches for that.
>
> Yinghai

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Yinghai Lu]

On Thu, Jul 12, 2012 at 1:20 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Jul 11, 2012 at 6:05 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Jul 11, 2012 at 3:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>>> If we *do* want to change something there, I don't like the proposal
>>> above any better.  It's still basically saying "presence detect is
>>> only reliable when X is set" when X is not clearly related to presence
>>> detect.
>>>
>>> I think it's better to disable the presence detect interrupt
>>> completely if it's not reliable, as your original patch did.  My
>>> complaint with that is that HP_SUPR_RM() doesn't seem like the right
>>> test for "the interrupt is not reliable."
>>
>> ok.
>>
>>>
>>> Having a "Presence Detect State" bit and an interrupt that tells you
>>> when it changed is only meaningful if that bit gives you useful
>>> information.  If hardware supplies that bit but it toggles all the
>>> time when the slot is empty because it's hooked up to link training
>>> attempts, that just means the hardware screwed up.  The hardware
>>> *should* have included some logic to filter out the attempts and
>>> toggle the bit only when a card is actually added or removed.  I
>>> believe the functionality of "Presence Detect State" is logically
>>> independent of "Hot-Plug Surprise" and "Attention Button Present."
>>
>> the cpu vendor already agreed that is out of spec for that.
>>
>>>
>>> So if we want to disable the "Presence Detect Changed" interrupt,
>>> that's fine, but I think we should do it based on a quirk or
>>> blacklist, or based on the fact that we have no need for it.  One
>>> reason to want the interrupt is if "Hot-Plug Surprise" is set,
>>> indicating that an adapter might be removed without notice, and if
>>> that's the only reason, we could use your original patch.
>>
>> no,  with that patch, we will not get interrupt for present bit change
>> for non-hotplug-surprise
>> case.
>>
>>> But if we
>>> do, I think we should change interrupt_event_handler() to look
>>> something like this:
>>>
>>>     case INT_PRESENCE_ON:
>
>         ctrl_info(ctrl, "adapter now present\n");
>
>>>         if (!ATTN_BUTTN(ctrl))
>>>             handle_surprise_event(p_slot);  /* omit this if you don't
>>> think it's useful */
>>>         break;
>>>     case INT_PRESENCE_OFF:
>
>         ctrl_warn(ctrl, "adapter removed unexpectedly\n");

or keep the old

ctrl_dbg(ctrl, "Surprise Removal\n");

?

>
>>>         handle_surprise_event(p_slot);
>>>         break;
>>
>> yes, this one should be good. and it is enhancement.
>
> When we don't have an attention button, I don't know that it's a good
> idea to automatically power up a new card.  I was thinking about
> things like ExpressCard, where the standard usage model probably *is*
> "plug in the card and it automatically starts working, no button press
> or software UI required."  But my guess is that ExpressCard would not
> be handled by pciehp, would it?

some are by pciehp.

but my T420 BIOS _OSC doesn't give away pcie cap access, so pciehp can
not be loaded.
Try to file BIOS bug to the vendor, find no where to do that.

>
> The pciehp flow seems to be basically the same as SHPC, and the
> Standard Hot-Plug Controller spec, rev 1.0, sec 2.5, has a flow of
> hot-add without an attention button.  It shows:
>
>   1) User selects empty, disabled slot and opens MRL
>   2) Adapter insertion:
>       2a) User inserts add-in-card
>       2b) User closes MRL
>       2c) User attaches cables
>   3) User requests via software UI that slot be enabled
>
> We'll get the Presence Detect interrupt at step 2a, before the user
> can close the MRL or attach cables.  I don't know if it's a good thing
> to power-on the slot and attach the driver while this is happening.
> It certainly doesn't seem to follow the intent of the spec.
>

I think we should just keep old behavior like user need to press the
attention button.
and other os do that same way.

Thanks

Yinghai

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Bjorn Helgaas]

On Thu, Jul 12, 2012 at 6:19 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Jul 12, 2012 at 1:20 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Wed, Jul 11, 2012 at 6:05 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> On Wed, Jul 11, 2012 at 3:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>
>>>> If we *do* want to change something there, I don't like the proposal
>>>> above any better.  It's still basically saying "presence detect is
>>>> only reliable when X is set" when X is not clearly related to presence
>>>> detect.
>>>>
>>>> I think it's better to disable the presence detect interrupt
>>>> completely if it's not reliable, as your original patch did.  My
>>>> complaint with that is that HP_SUPR_RM() doesn't seem like the right
>>>> test for "the interrupt is not reliable."
>>>
>>> ok.
>>>
>>>>
>>>> Having a "Presence Detect State" bit and an interrupt that tells you
>>>> when it changed is only meaningful if that bit gives you useful
>>>> information.  If hardware supplies that bit but it toggles all the
>>>> time when the slot is empty because it's hooked up to link training
>>>> attempts, that just means the hardware screwed up.  The hardware
>>>> *should* have included some logic to filter out the attempts and
>>>> toggle the bit only when a card is actually added or removed.  I
>>>> believe the functionality of "Presence Detect State" is logically
>>>> independent of "Hot-Plug Surprise" and "Attention Button Present."
>>>
>>> the cpu vendor already agreed that is out of spec for that.
>>>
>>>>
>>>> So if we want to disable the "Presence Detect Changed" interrupt,
>>>> that's fine, but I think we should do it based on a quirk or
>>>> blacklist, or based on the fact that we have no need for it.  One
>>>> reason to want the interrupt is if "Hot-Plug Surprise" is set,
>>>> indicating that an adapter might be removed without notice, and if
>>>> that's the only reason, we could use your original patch.
>>>
>>> no,  with that patch, we will not get interrupt for present bit change
>>> for non-hotplug-surprise
>>> case.
>>>
>>>> But if we
>>>> do, I think we should change interrupt_event_handler() to look
>>>> something like this:
>>>>
>>>>     case INT_PRESENCE_ON:
>>
>>         ctrl_info(ctrl, "adapter now present\n");
>>
>>>>         if (!ATTN_BUTTN(ctrl))
>>>>             handle_surprise_event(p_slot);  /* omit this if you don't
>>>> think it's useful */
>>>>         break;
>>>>     case INT_PRESENCE_OFF:
>>
>>         ctrl_warn(ctrl, "adapter removed unexpectedly\n");
>
> or keep the old
>
> ctrl_dbg(ctrl, "Surprise Removal\n");

ctrl_dbg() only produces output if pciehp_debug is enabled, and I
thought the message was useful enough for debugging issues that it
should always appear.  And I thought it was useful to include the word
"adapter" to make the message more meaningful to an end-user.

>>>>         handle_surprise_event(p_slot);
>>>>         break;
>>>
>>> yes, this one should be good. and it is enhancement.
>>
>> When we don't have an attention button, I don't know that it's a good
>> idea to automatically power up a new card.  I was thinking about
>> things like ExpressCard, where the standard usage model probably *is*
>> "plug in the card and it automatically starts working, no button press
>> or software UI required."  But my guess is that ExpressCard would not
>> be handled by pciehp, would it?
>
> some are by pciehp.
>
> but my T420 BIOS _OSC doesn't give away pcie cap access, so pciehp can
> not be loaded.
> Try to file BIOS bug to the vendor, find no where to do that.
>
>>
>> The pciehp flow seems to be basically the same as SHPC, and the
>> Standard Hot-Plug Controller spec, rev 1.0, sec 2.5, has a flow of
>> hot-add without an attention button.  It shows:
>>
>>   1) User selects empty, disabled slot and opens MRL
>>   2) Adapter insertion:
>>       2a) User inserts add-in-card
>>       2b) User closes MRL
>>       2c) User attaches cables
>>   3) User requests via software UI that slot be enabled
>>
>> We'll get the Presence Detect interrupt at step 2a, before the user
>> can close the MRL or attach cables.  I don't know if it's a good thing
>> to power-on the slot and attach the driver while this is happening.
>> It certainly doesn't seem to follow the intent of the spec.
>>
>
> I think we should just keep old behavior like user need to press the
> attention button.
> and other os do that same way.

If there is an attention button, I think we should definitely wait
until the user presses it.

The question is what to do when a new card appears and there's no
attention button.  In general I think we should follow the SHPC flow
and wait for some software equivalent of a button push, e.g., a sysfs
poke.

But if pciehp handles ExpressCards, that doesn't sound like the
desired user experience -- if we insert an ExpressCard in a laptop,
don't we expect it to just start working, without having to poke
around in software?

Re: [PATCH 2/5] pciehp: Don't enable presence notification while surprise removal is not supported.

[Yinghai Lu]

On Fri, Jul 13, 2012 at 8:30 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Jul 12, 2012 at 6:19 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>
> The question is what to do when a new card appears and there's no
> attention button.  In general I think we should follow the SHPC flow
> and wait for some software equivalent of a button push, e.g., a sysfs
> poke.
>
> But if pciehp handles ExpressCards, that doesn't sound like the
> desired user experience -- if we insert an ExpressCard in a laptop,
> don't we expect it to just start working, without having to poke
> around in software?

ExpressCards does support surprise removal. so in current code, it
still get online
automatically.

Yinghai

[PATCH 3/5] PCI, acpiphp: Merge acpiphp_debug and debug

[Yinghai Lu]

Should not have two, just remove debug, and use module_param_named
instead.

Also change acpiphp_debug to bool.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/hotplug/acpiphp.h      |    2 +-
 drivers/pci/hotplug/acpiphp_core.c |    7 ++-----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index 6b58ed0..a1afb5b 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -205,6 +205,6 @@ extern u8 acpiphp_get_latch_status (struct acpiphp_slot *slot);
 extern u8 acpiphp_get_adapter_status (struct acpiphp_slot *slot);
 
 /* variables */
-extern int acpiphp_debug;
+extern bool acpiphp_debug;
 
 #endif /* _ACPIPHP_H */
diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
index aa41631..96316b7 100644
--- a/drivers/pci/hotplug/acpiphp_core.c
+++ b/drivers/pci/hotplug/acpiphp_core.c
@@ -47,8 +47,7 @@
 /* name size which is used for entries in pcihpfs */
 #define SLOT_NAME_SIZE  21              /* {_SUN} */
 
-static bool debug;
-int acpiphp_debug;
+bool acpiphp_debug;
 
 /* local variables */
 static int num_slots;
@@ -62,7 +61,7 @@ MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL");
 MODULE_PARM_DESC(debug, "Debugging mode enabled or not");
-module_param(debug, bool, 0644);
+module_param_named(debug, acpiphp_debug, bool, 0644);
 
 /* export the attention callback registration methods */
 EXPORT_SYMBOL_GPL(acpiphp_register_attention);
@@ -379,8 +378,6 @@ static int __init acpiphp_init(void)
 	if (acpi_pci_disabled)
 		return 0;
 
-	acpiphp_debug = debug;
-
 	/* read all the ACPI info from the system */
 	return init_acpi();
 }
-- 
1.7.7

[PATCH 4/5] PCI, acpiphp: add is_hotplug_bridge detection

[Yinghai Lu]

When system support hotplug bridge with children hotplug slots, we need to make sure
that parent bridge get preallocated resource so later when device is plugged into
children slot, those children devices will get resource allocated.

We do not meet this problem, because for pcie hotplug card, when acpiphp is used,
pci_scan_bridge will set that for us when detect hotplug bit in slot cap.

Reported-and-tested-by: Jason Baron <jbaron@redhat.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Jason Baron <jbaron@redhat.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index ad6fd66..0f2b72d 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
 	}
 }
 
+static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
+{
+	struct acpiphp_func *func;
+
+	if (!dev->subordinate)
+		return;
+
+	/* quirk, or pcie could set it already */
+	if (dev->is_hotplug_bridge)
+		return;
+
+	if (PCI_SLOT(dev->devfn) != slot->device)
+		return;
+
+	list_for_each_entry(func, &slot->funcs, sibling) {
+		if (PCI_FUNC(dev->devfn) == func->function) {
+			/* check if this bridge has ejectable slots */
+			if ((detect_ejectable_slots(func->handle) > 0))
+				dev->is_hotplug_bridge = 1;
+			break;
+		}
+	}
+}
 /**
  * enable_device - enable, configure a slot
  * @slot: slot to be enabled
@@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 			if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
 			    dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
 				max = pci_scan_bridge(bus, dev, max, pass);
-				if (pass && dev->subordinate)
+				if (pass && dev->subordinate) {
+					check_hotplug_bridge(slot, dev);
 					pci_bus_size_bridges(dev->subordinate);
+				}
 			}
 		}
 	}
-- 
1.7.7

Re: [PATCH 4/5] PCI, acpiphp: add is_hotplug_bridge detection

[Bjorn Helgaas]

On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> When system support hotplug bridge with children hotplug slots, we need to make sure
> that parent bridge get preallocated resource so later when device is plugged into
> children slot, those children devices will get resource allocated.
>
> We do not meet this problem, because for pcie hotplug card, when acpiphp is used,
> pci_scan_bridge will set that for us when detect hotplug bit in slot cap.
>
> Reported-and-tested-by: Jason Baron <jbaron@redhat.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Acked-by: Jason Baron <jbaron@redhat.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
>  1 files changed, 26 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index ad6fd66..0f2b72d 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
>         }
>  }
>
> +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
> +{
> +       struct acpiphp_func *func;
> +
> +       if (!dev->subordinate)
> +               return;
> +
> +       /* quirk, or pcie could set it already */
> +       if (dev->is_hotplug_bridge)
> +               return;
> +
> +       if (PCI_SLOT(dev->devfn) != slot->device)
> +               return;
> +
> +       list_for_each_entry(func, &slot->funcs, sibling) {
> +               if (PCI_FUNC(dev->devfn) == func->function) {
> +                       /* check if this bridge has ejectable slots */
> +                       if ((detect_ejectable_slots(func->handle) > 0))
> +                               dev->is_hotplug_bridge = 1;
> +                       break;
> +               }
> +       }
> +}
>  /**
>   * enable_device - enable, configure a slot
>   * @slot: slot to be enabled
> @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>                         if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
>                             dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
>                                 max = pci_scan_bridge(bus, dev, max, pass);
> -                               if (pass && dev->subordinate)
> +                               if (pass && dev->subordinate) {
> +                                       check_hotplug_bridge(slot, dev);

I don't like this patch because it increases the differences between
the hotplug drivers, rather than decreasing them.

For PCI Express devices, we set dev->is_hotplug_bridge in the
pci_setup_device() path (in set_pcie_hotplug_bridge()).  I think it
would make sense to try to expand that path to also handle SHPC and
ACPI hotplug as well.  ACPI is harder because it's not PCI-specified,
so we'd need some sort of pcibios or other optional hook.

I don't have a clear picture of how this works -- if I understand
correctly, the situation is that we have a bridge managed by acpiphp.
That part makes sense because the bridge is on the motherboard and can
have a DSDT device.  Now we plug something into the slot below the
bridge.  I *think* this patch handles the case where this new
hot-added thing is also a bridge managed by acpiphp.  But where does
the ACPI device for this hot-added bridge come from?  It's an
arbitrary device the BIOS knows nothing about, so it can't be in the
DSDT.

>                                         pci_bus_size_bridges(dev->subordinate);
> +                               }
>                         }
>                 }
>         }
> --
> 1.7.7
>

Re: [PATCH 4/5] PCI, acpiphp: add is_hotplug_bridge detection

[Jason Baron]

On Wed, Jul 11, 2012 at 09:50:19AM -0600, Bjorn Helgaas wrote:
> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> > When system support hotplug bridge with children hotplug slots, we need to make sure
> > that parent bridge get preallocated resource so later when device is plugged into
> > children slot, those children devices will get resource allocated.
> >
> > We do not meet this problem, because for pcie hotplug card, when acpiphp is used,
> > pci_scan_bridge will set that for us when detect hotplug bit in slot cap.
> >
> > Reported-and-tested-by: Jason Baron <jbaron@redhat.com>
> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> > Acked-by: Jason Baron <jbaron@redhat.com>
> > ---
> >  drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
> >  1 files changed, 26 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > index ad6fd66..0f2b72d 100644
> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
> >         }
> >  }
> >
> > +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
> > +{
> > +       struct acpiphp_func *func;
> > +
> > +       if (!dev->subordinate)
> > +               return;
> > +
> > +       /* quirk, or pcie could set it already */
> > +       if (dev->is_hotplug_bridge)
> > +               return;
> > +
> > +       if (PCI_SLOT(dev->devfn) != slot->device)
> > +               return;
> > +
> > +       list_for_each_entry(func, &slot->funcs, sibling) {
> > +               if (PCI_FUNC(dev->devfn) == func->function) {
> > +                       /* check if this bridge has ejectable slots */
> > +                       if ((detect_ejectable_slots(func->handle) > 0))
> > +                               dev->is_hotplug_bridge = 1;
> > +                       break;
> > +               }
> > +       }
> > +}
> >  /**
> >   * enable_device - enable, configure a slot
> >   * @slot: slot to be enabled
> > @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >                         if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
> >                             dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
> >                                 max = pci_scan_bridge(bus, dev, max, pass);
> > -                               if (pass && dev->subordinate)
> > +                               if (pass && dev->subordinate) {
> > +                                       check_hotplug_bridge(slot, dev);
> 
> I don't like this patch because it increases the differences between
> the hotplug drivers, rather than decreasing them.
> 
> For PCI Express devices, we set dev->is_hotplug_bridge in the
> pci_setup_device() path (in set_pcie_hotplug_bridge()).  I think it
> would make sense to try to expand that path to also handle SHPC and
> ACPI hotplug as well.  ACPI is harder because it's not PCI-specified,
> so we'd need some sort of pcibios or other optional hook.
> 
> I don't have a clear picture of how this works -- if I understand
> correctly, the situation is that we have a bridge managed by acpiphp.
> That part makes sense because the bridge is on the motherboard and can
> have a DSDT device.  Now we plug something into the slot below the
> bridge.  I *think* this patch handles the case where this new
> hot-added thing is also a bridge managed by acpiphp.  But where does
> the ACPI device for this hot-added bridge come from?  It's an
> arbitrary device the BIOS knows nothing about, so it can't be in the
> DSDT.
> 

So this came up while I was developing pci bridge hotplug for qemu.
Currently, there is a top level host bus (with ACPI device definitions), where
devices can be hot-plugged. What I've done is added a second level
of hotplug pci busses (again with ACPI device definitions). Thus, we can
now hotplug a bridge into the top-level bus and then devices behind it.
Effectively increasing the hot-plug space from n -> n^2.

Before the above pci patch, the devices behind the bridge would not
configure their PCI BARs properly, since there were no i/o, mem resources
assigned to the bridge. However, with the above patch in place things
work as expected.

Using the same code base I was able to do acpi hotplug on Windows 7,
which correctly configured the both the bridge window and devices behind
it on hot-plug. So currently, the above usage pattern works on Windows
7, but not on Linux.

Thanks,

-Jason

Re: [PATCH 4/5] PCI, acpiphp: add is_hotplug_bridge detection

[Bjorn Helgaas]

On Wed, Jul 11, 2012 at 11:14 AM, Jason Baron <jbaron@redhat.com> wrote:
> On Wed, Jul 11, 2012 at 09:50:19AM -0600, Bjorn Helgaas wrote:
>> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> > When system support hotplug bridge with children hotplug slots, we need to make sure
>> > that parent bridge get preallocated resource so later when device is plugged into
>> > children slot, those children devices will get resource allocated.
>> >
>> > We do not meet this problem, because for pcie hotplug card, when acpiphp is used,
>> > pci_scan_bridge will set that for us when detect hotplug bit in slot cap.
>> >
>> > Reported-and-tested-by: Jason Baron <jbaron@redhat.com>
>> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> > Acked-by: Jason Baron <jbaron@redhat.com>
>> > ---
>> >  drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
>> >  1 files changed, 26 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>> > index ad6fd66..0f2b72d 100644
>> > --- a/drivers/pci/hotplug/acpiphp_glue.c
>> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> > @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
>> >         }
>> >  }
>> >
>> > +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
>> > +{
>> > +       struct acpiphp_func *func;
>> > +
>> > +       if (!dev->subordinate)
>> > +               return;
>> > +
>> > +       /* quirk, or pcie could set it already */
>> > +       if (dev->is_hotplug_bridge)
>> > +               return;
>> > +
>> > +       if (PCI_SLOT(dev->devfn) != slot->device)
>> > +               return;
>> > +
>> > +       list_for_each_entry(func, &slot->funcs, sibling) {
>> > +               if (PCI_FUNC(dev->devfn) == func->function) {
>> > +                       /* check if this bridge has ejectable slots */
>> > +                       if ((detect_ejectable_slots(func->handle) > 0))
>> > +                               dev->is_hotplug_bridge = 1;
>> > +                       break;
>> > +               }
>> > +       }
>> > +}
>> >  /**
>> >   * enable_device - enable, configure a slot
>> >   * @slot: slot to be enabled
>> > @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>> >                         if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
>> >                             dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
>> >                                 max = pci_scan_bridge(bus, dev, max, pass);
>> > -                               if (pass && dev->subordinate)
>> > +                               if (pass && dev->subordinate) {
>> > +                                       check_hotplug_bridge(slot, dev);
>>
>> I don't like this patch because it increases the differences between
>> the hotplug drivers, rather than decreasing them.
>>
>> For PCI Express devices, we set dev->is_hotplug_bridge in the
>> pci_setup_device() path (in set_pcie_hotplug_bridge()).  I think it
>> would make sense to try to expand that path to also handle SHPC and
>> ACPI hotplug as well.  ACPI is harder because it's not PCI-specified,
>> so we'd need some sort of pcibios or other optional hook.
>>
>> I don't have a clear picture of how this works -- if I understand
>> correctly, the situation is that we have a bridge managed by acpiphp.
>> That part makes sense because the bridge is on the motherboard and can
>> have a DSDT device.  Now we plug something into the slot below the
>> bridge.  I *think* this patch handles the case where this new
>> hot-added thing is also a bridge managed by acpiphp.  But where does
>> the ACPI device for this hot-added bridge come from?  It's an
>> arbitrary device the BIOS knows nothing about, so it can't be in the
>> DSDT.
>>
>
> So this came up while I was developing pci bridge hotplug for qemu.
> Currently, there is a top level host bus (with ACPI device definitions), where
> devices can be hot-plugged. What I've done is added a second level
> of hotplug pci busses (again with ACPI device definitions). Thus, we can
> now hotplug a bridge into the top-level bus and then devices behind it.
> Effectively increasing the hot-plug space from n -> n^2.
>
> Before the above pci patch, the devices behind the bridge would not
> configure their PCI BARs properly, since there were no i/o, mem resources
> assigned to the bridge. However, with the above patch in place things
> work as expected.
>
> Using the same code base I was able to do acpi hotplug on Windows 7,
> which correctly configured the both the bridge window and devices behind
> it on hot-plug. So currently, the above usage pattern works on Windows
> 7, but not on Linux.

Thanks, Jason.  Do you have "lspci -v" output and the DSDT AML handy?
I'd like to look in more detail at what we're missing.

Re: [PATCH 4/5] PCI, acpiphp: add is_hotplug_bridge detection

[Bjorn Helgaas]

On Wed, Jul 11, 2012 at 1:42 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Jul 11, 2012 at 11:14 AM, Jason Baron <jbaron@redhat.com> wrote:

>> Using the same code base I was able to do acpi hotplug on Windows 7,
>> which correctly configured the both the bridge window and devices behind
>> it on hot-plug. So currently, the above usage pattern works on Windows
>> 7, but not on Linux.
>
> Thanks, Jason.  Do you have "lspci -v" output and the DSDT AML handy?
> I'd like to look in more detail at what we're missing.

Ping?

Re: [PATCH 4/5] PCI, acpiphp: add is_hotplug_bridge detection

[Jason Baron]

On Wed, Jul 11, 2012 at 01:42:15PM -0600, Bjorn Helgaas wrote:
> On Wed, Jul 11, 2012 at 11:14 AM, Jason Baron <jbaron@redhat.com> wrote:
> > On Wed, Jul 11, 2012 at 09:50:19AM -0600, Bjorn Helgaas wrote:
> >> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> > When system support hotplug bridge with children hotplug slots, we need to make sure
> >> > that parent bridge get preallocated resource so later when device is plugged into
> >> > children slot, those children devices will get resource allocated.
> >> >
> >> > We do not meet this problem, because for pcie hotplug card, when acpiphp is used,
> >> > pci_scan_bridge will set that for us when detect hotplug bit in slot cap.
> >> >
> >> > Reported-and-tested-by: Jason Baron <jbaron@redhat.com>
> >> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> > Acked-by: Jason Baron <jbaron@redhat.com>
> >> > ---
> >> >  drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
> >> >  1 files changed, 26 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> >> > index ad6fd66..0f2b72d 100644
> >> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> >> > @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
> >> >         }
> >> >  }
> >> >
> >> > +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
> >> > +{
> >> > +       struct acpiphp_func *func;
> >> > +
> >> > +       if (!dev->subordinate)
> >> > +               return;
> >> > +
> >> > +       /* quirk, or pcie could set it already */
> >> > +       if (dev->is_hotplug_bridge)
> >> > +               return;
> >> > +
> >> > +       if (PCI_SLOT(dev->devfn) != slot->device)
> >> > +               return;
> >> > +
> >> > +       list_for_each_entry(func, &slot->funcs, sibling) {
> >> > +               if (PCI_FUNC(dev->devfn) == func->function) {
> >> > +                       /* check if this bridge has ejectable slots */
> >> > +                       if ((detect_ejectable_slots(func->handle) > 0))
> >> > +                               dev->is_hotplug_bridge = 1;
> >> > +                       break;
> >> > +               }
> >> > +       }
> >> > +}
> >> >  /**
> >> >   * enable_device - enable, configure a slot
> >> >   * @slot: slot to be enabled
> >> > @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >> >                         if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
> >> >                             dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
> >> >                                 max = pci_scan_bridge(bus, dev, max, pass);
> >> > -                               if (pass && dev->subordinate)
> >> > +                               if (pass && dev->subordinate) {
> >> > +                                       check_hotplug_bridge(slot, dev);
> >>
> >> I don't like this patch because it increases the differences between
> >> the hotplug drivers, rather than decreasing them.
> >>
> >> For PCI Express devices, we set dev->is_hotplug_bridge in the
> >> pci_setup_device() path (in set_pcie_hotplug_bridge()).  I think it
> >> would make sense to try to expand that path to also handle SHPC and
> >> ACPI hotplug as well.  ACPI is harder because it's not PCI-specified,
> >> so we'd need some sort of pcibios or other optional hook.
> >>
> >> I don't have a clear picture of how this works -- if I understand
> >> correctly, the situation is that we have a bridge managed by acpiphp.
> >> That part makes sense because the bridge is on the motherboard and can
> >> have a DSDT device.  Now we plug something into the slot below the
> >> bridge.  I *think* this patch handles the case where this new
> >> hot-added thing is also a bridge managed by acpiphp.  But where does
> >> the ACPI device for this hot-added bridge come from?  It's an
> >> arbitrary device the BIOS knows nothing about, so it can't be in the
> >> DSDT.
> >>
> >
> > So this came up while I was developing pci bridge hotplug for qemu.
> > Currently, there is a top level host bus (with ACPI device definitions), where
> > devices can be hot-plugged. What I've done is added a second level
> > of hotplug pci busses (again with ACPI device definitions). Thus, we can
> > now hotplug a bridge into the top-level bus and then devices behind it.
> > Effectively increasing the hot-plug space from n -> n^2.
> >
> > Before the above pci patch, the devices behind the bridge would not
> > configure their PCI BARs properly, since there were no i/o, mem resources
> > assigned to the bridge. However, with the above patch in place things
> > work as expected.
> >
> > Using the same code base I was able to do acpi hotplug on Windows 7,
> > which correctly configured the both the bridge window and devices behind
> > it on hot-plug. So currently, the above usage pattern works on Windows
> > 7, but not on Linux.
> 
> Thanks, Jason.  Do you have "lspci -v" output and the DSDT AML handy?
> I'd like to look in more detail at what we're missing.

Hi,

Sorry for the delay...was on vacation.

Anyways, below is the patch I have to the seabios acpi table. However,
there are other pieces for seabios and qemu required. I still need to
clean them up and send them out. But this pci patch (or something
similar) is a required dependency.

What 'lspci -v' output are you looking for?

Let me know what else I can provide.

Thanks,

-Jason


--- a/src/ssdt-pcihp.dsl
+++ b/src/ssdt-pcihp.dsl
@@ -17,82 +17,162 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1)
         // at runtime, if the slot is detected to not support hotplug.
         // Extract the offset of the address dword and the
         // _EJ0 name to allow this patching.
-#define hotplug_slot(slot)                              \
-        Device (S##slot) {                              \
-           ACPI_EXTRACT_NAME_DWORD_CONST aml_adr_dword  \
-           Name (_ADR, 0x##slot##0000)                  \
-           ACPI_EXTRACT_METHOD_STRING aml_ej0_name      \
-           Method (_EJ0, 1) { Return(PCEJ(0x##slot)) }  \
-           Name (_SUN, 0x##slot)                        \
-        }
+#define hotplug_level2_slot(slot1, slot2)                   \
+        Device (S##slot2) {                                 \
+           Name (_ADR, 0x##slot2##0000)                     \
+           Method (_EJ0, 1) { Return(PCEJ(0x##slot1, 0x##slot2)) } \
+           Name (_SUN, 0x##slot2)                           \
+        }                                                   \
+
+#define hotplug_top_level_slot(slot)                          \
+        Device (S##slot) {                                    \
+            ACPI_EXTRACT_NAME_DWORD_CONST aml_adr_dword       \
+            Name (_ADR,0x##slot##0000)                        \
+            ACPI_EXTRACT_METHOD_STRING aml_ej0_name           \
+            Method (_EJ0, 1) { Return(PCEJ(0x##slot, 0x00)) } \
+            Name (_SUN, 0x##slot)                             \
+            hotplug_level2_slot(slot, 01)                     \
+            hotplug_level2_slot(slot, 02)                     \
+            hotplug_level2_slot(slot, 03)                     \
+            hotplug_level2_slot(slot, 04)                     \
+            hotplug_level2_slot(slot, 05)                     \
+            hotplug_level2_slot(slot, 06)                     \
+            hotplug_level2_slot(slot, 07)                     \
+            hotplug_level2_slot(slot, 08)                     \
+            hotplug_level2_slot(slot, 09)                     \
+            hotplug_level2_slot(slot, 0a)                     \
+            hotplug_level2_slot(slot, 0b)                     \
+            hotplug_level2_slot(slot, 0c)                     \
+            hotplug_level2_slot(slot, 0d)                     \
+            hotplug_level2_slot(slot, 0e)                     \
+            hotplug_level2_slot(slot, 0f)                     \
+            hotplug_level2_slot(slot, 10)                     \
+            hotplug_level2_slot(slot, 11)                     \
+            hotplug_level2_slot(slot, 12)                     \
+            hotplug_level2_slot(slot, 13)                     \
+            hotplug_level2_slot(slot, 14)                     \
+            hotplug_level2_slot(slot, 15)                     \
+            hotplug_level2_slot(slot, 16)                     \
+            hotplug_level2_slot(slot, 17)                     \
+            hotplug_level2_slot(slot, 18)                     \
+            hotplug_level2_slot(slot, 19)                     \
+            hotplug_level2_slot(slot, 1a)                     \
+            hotplug_level2_slot(slot, 1b)                     \
+            hotplug_level2_slot(slot, 1c)                     \
+            hotplug_level2_slot(slot, 1d)                     \
+            hotplug_level2_slot(slot, 1e)                     \
+            hotplug_level2_slot(slot, 1f)                     \
+        }                                                     \
+
+        hotplug_top_level_slot(01)
+        hotplug_top_level_slot(02)
+        hotplug_top_level_slot(03)
+        hotplug_top_level_slot(04)
+        hotplug_top_level_slot(05)
+        hotplug_top_level_slot(06)
+        hotplug_top_level_slot(07)
+        hotplug_top_level_slot(08)
+        hotplug_top_level_slot(09)
+        hotplug_top_level_slot(0a)
+        hotplug_top_level_slot(0b)
+        hotplug_top_level_slot(0c)
+        hotplug_top_level_slot(0d)
+        hotplug_top_level_slot(0e)
+        hotplug_top_level_slot(0f)
+        hotplug_top_level_slot(10)
+        hotplug_top_level_slot(11)
+        hotplug_top_level_slot(12)
+        hotplug_top_level_slot(13)
+        hotplug_top_level_slot(14)
+        hotplug_top_level_slot(15)
+        hotplug_top_level_slot(16)
+        hotplug_top_level_slot(17)
+        hotplug_top_level_slot(18)
+        hotplug_top_level_slot(19)
+        hotplug_top_level_slot(1a)
+        hotplug_top_level_slot(1b)
+        hotplug_top_level_slot(1c)
+        hotplug_top_level_slot(1d)
+        hotplug_top_level_slot(1e)
+        hotplug_top_level_slot(1f)
 
-        hotplug_slot(01)
-        hotplug_slot(02)
-        hotplug_slot(03)
-        hotplug_slot(04)
-        hotplug_slot(05)
-        hotplug_slot(06)
-        hotplug_slot(07)
-        hotplug_slot(08)
-        hotplug_slot(09)
-        hotplug_slot(0a)
-        hotplug_slot(0b)
-        hotplug_slot(0c)
-        hotplug_slot(0d)
-        hotplug_slot(0e)
-        hotplug_slot(0f)
-        hotplug_slot(10)
-        hotplug_slot(11)
-        hotplug_slot(12)
-        hotplug_slot(13)
-        hotplug_slot(14)
-        hotplug_slot(15)
-        hotplug_slot(16)
-        hotplug_slot(17)
-        hotplug_slot(18)
-        hotplug_slot(19)
-        hotplug_slot(1a)
-        hotplug_slot(1b)
-        hotplug_slot(1c)
-        hotplug_slot(1d)
-        hotplug_slot(1e)
-        hotplug_slot(1f)
+#define gen_pci_level2_hotplug(slot1, slot2) \
+            If (LEqual(Arg0, 0x##slot1)) { \
+                If (LEqual(Arg1, 0x##slot2)) { \
+                    Notify(\_SB.PCI0.S##slot1.S##slot2, Arg2) \
+                } \
+            } \
 
-#define gen_pci_hotplug(slot)   \
-            If (LEqual(Arg0, 0x##slot)) { Notify(S##slot, Arg1) }
+#define gen_pci_top_level_hotplug(slot)   \
+            If (LEqual(Arg1, Zero)) { \
+                If (LEqual(Arg0, 0x##slot)) { \
+                    Notify(S##slot, Arg2) \
+                } \
+            } \
+            gen_pci_level2_hotplug(slot, 01) \
+            gen_pci_level2_hotplug(slot, 02) \
+            gen_pci_level2_hotplug(slot, 03) \
+            gen_pci_level2_hotplug(slot, 04) \
+            gen_pci_level2_hotplug(slot, 05) \
+            gen_pci_level2_hotplug(slot, 06) \
+            gen_pci_level2_hotplug(slot, 07) \
+            gen_pci_level2_hotplug(slot, 08) \
+            gen_pci_level2_hotplug(slot, 09) \
+            gen_pci_level2_hotplug(slot, 0a) \
+            gen_pci_level2_hotplug(slot, 0b) \
+            gen_pci_level2_hotplug(slot, 0c) \
+            gen_pci_level2_hotplug(slot, 0d) \
+            gen_pci_level2_hotplug(slot, 0e) \
+            gen_pci_level2_hotplug(slot, 0f) \
+            gen_pci_level2_hotplug(slot, 10) \
+            gen_pci_level2_hotplug(slot, 11) \
+            gen_pci_level2_hotplug(slot, 12) \
+            gen_pci_level2_hotplug(slot, 13) \
+            gen_pci_level2_hotplug(slot, 14) \
+            gen_pci_level2_hotplug(slot, 15) \
+            gen_pci_level2_hotplug(slot, 16) \
+            gen_pci_level2_hotplug(slot, 17) \
+            gen_pci_level2_hotplug(slot, 18) \
+            gen_pci_level2_hotplug(slot, 19) \
+            gen_pci_level2_hotplug(slot, 1a) \
+            gen_pci_level2_hotplug(slot, 1b) \
+            gen_pci_level2_hotplug(slot, 1c) \
+            gen_pci_level2_hotplug(slot, 1d) \
+            gen_pci_level2_hotplug(slot, 1e) \
+            gen_pci_level2_hotplug(slot, 1f) \
 
-        Method(PCNT, 2) {
-            gen_pci_hotplug(01)
-            gen_pci_hotplug(02)
-            gen_pci_hotplug(03)
-            gen_pci_hotplug(04)
-            gen_pci_hotplug(05)
-            gen_pci_hotplug(06)
-            gen_pci_hotplug(07)
-            gen_pci_hotplug(08)
-            gen_pci_hotplug(09)
-            gen_pci_hotplug(0a)
-            gen_pci_hotplug(0b)
-            gen_pci_hotplug(0c)
-            gen_pci_hotplug(0d)
-            gen_pci_hotplug(0e)
-            gen_pci_hotplug(0f)
-            gen_pci_hotplug(10)
-            gen_pci_hotplug(11)
-            gen_pci_hotplug(12)
-            gen_pci_hotplug(13)
-            gen_pci_hotplug(14)
-            gen_pci_hotplug(15)
-            gen_pci_hotplug(16)
-            gen_pci_hotplug(17)
-            gen_pci_hotplug(18)
-            gen_pci_hotplug(19)
-            gen_pci_hotplug(1a)
-            gen_pci_hotplug(1b)
-            gen_pci_hotplug(1c)
-            gen_pci_hotplug(1d)
-            gen_pci_hotplug(1e)
-            gen_pci_hotplug(1f)
+        Method(PCNT, 3) {
+            gen_pci_top_level_hotplug(01)
+            gen_pci_top_level_hotplug(02)
+            gen_pci_top_level_hotplug(03)
+            gen_pci_top_level_hotplug(04)
+            gen_pci_top_level_hotplug(05)
+            gen_pci_top_level_hotplug(06)
+            gen_pci_top_level_hotplug(07)
+            gen_pci_top_level_hotplug(08)
+            gen_pci_top_level_hotplug(09)
+            gen_pci_top_level_hotplug(0a)
+            gen_pci_top_level_hotplug(0b)
+            gen_pci_top_level_hotplug(0c)
+            gen_pci_top_level_hotplug(0d)
+            gen_pci_top_level_hotplug(0e)
+            gen_pci_top_level_hotplug(0f)
+            gen_pci_top_level_hotplug(10)
+            gen_pci_top_level_hotplug(11)
+            gen_pci_top_level_hotplug(12)
+            gen_pci_top_level_hotplug(13)
+            gen_pci_top_level_hotplug(14)
+            gen_pci_top_level_hotplug(15)
+            gen_pci_top_level_hotplug(16)
+            gen_pci_top_level_hotplug(17)
+            gen_pci_top_level_hotplug(18)
+            gen_pci_top_level_hotplug(19)
+            gen_pci_top_level_hotplug(1a)
+            gen_pci_top_level_hotplug(1b)
+            gen_pci_top_level_hotplug(1c)
+            gen_pci_top_level_hotplug(1d)
+            gen_pci_top_level_hotplug(1e)
+            gen_pci_top_level_hotplug(1f)
         }
     }
 

Re: [PATCH 4/5] PCI, acpiphp: add is_hotplug_bridge detection

[Bjorn Helgaas]

On Tue, Jul 17, 2012 at 8:14 AM, Jason Baron <jbaron@redhat.com> wrote:
> On Wed, Jul 11, 2012 at 01:42:15PM -0600, Bjorn Helgaas wrote:
>> On Wed, Jul 11, 2012 at 11:14 AM, Jason Baron <jbaron@redhat.com> wrote:
>> > On Wed, Jul 11, 2012 at 09:50:19AM -0600, Bjorn Helgaas wrote:
>> >> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> >> > When system support hotplug bridge with children hotplug slots, we need to make sure
>> >> > that parent bridge get preallocated resource so later when device is plugged into
>> >> > children slot, those children devices will get resource allocated.
>> >> >
>> >> > We do not meet this problem, because for pcie hotplug card, when acpiphp is used,
>> >> > pci_scan_bridge will set that for us when detect hotplug bit in slot cap.
>> >> >
>> >> > Reported-and-tested-by: Jason Baron <jbaron@redhat.com>
>> >> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> >> > Acked-by: Jason Baron <jbaron@redhat.com>
>> >> > ---
>> >> >  drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
>> >> >  1 files changed, 26 insertions(+), 1 deletions(-)
>> >> >
>> >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>> >> > index ad6fd66..0f2b72d 100644
>> >> > --- a/drivers/pci/hotplug/acpiphp_glue.c
>> >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> >> > @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
>> >> >         }
>> >> >  }
>> >> >
>> >> > +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
>> >> > +{
>> >> > +       struct acpiphp_func *func;
>> >> > +
>> >> > +       if (!dev->subordinate)
>> >> > +               return;
>> >> > +
>> >> > +       /* quirk, or pcie could set it already */
>> >> > +       if (dev->is_hotplug_bridge)
>> >> > +               return;
>> >> > +
>> >> > +       if (PCI_SLOT(dev->devfn) != slot->device)
>> >> > +               return;
>> >> > +
>> >> > +       list_for_each_entry(func, &slot->funcs, sibling) {
>> >> > +               if (PCI_FUNC(dev->devfn) == func->function) {
>> >> > +                       /* check if this bridge has ejectable slots */
>> >> > +                       if ((detect_ejectable_slots(func->handle) > 0))
>> >> > +                               dev->is_hotplug_bridge = 1;
>> >> > +                       break;
>> >> > +               }
>> >> > +       }
>> >> > +}
>> >> >  /**
>> >> >   * enable_device - enable, configure a slot
>> >> >   * @slot: slot to be enabled
>> >> > @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>> >> >                         if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
>> >> >                             dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
>> >> >                                 max = pci_scan_bridge(bus, dev, max, pass);
>> >> > -                               if (pass && dev->subordinate)
>> >> > +                               if (pass && dev->subordinate) {
>> >> > +                                       check_hotplug_bridge(slot, dev);
>> >>
>> >> I don't like this patch because it increases the differences between
>> >> the hotplug drivers, rather than decreasing them.
>> >>
>> >> For PCI Express devices, we set dev->is_hotplug_bridge in the
>> >> pci_setup_device() path (in set_pcie_hotplug_bridge()).  I think it
>> >> would make sense to try to expand that path to also handle SHPC and
>> >> ACPI hotplug as well.  ACPI is harder because it's not PCI-specified,
>> >> so we'd need some sort of pcibios or other optional hook.
>> >>
>> >> I don't have a clear picture of how this works -- if I understand
>> >> correctly, the situation is that we have a bridge managed by acpiphp.
>> >> That part makes sense because the bridge is on the motherboard and can
>> >> have a DSDT device.  Now we plug something into the slot below the
>> >> bridge.  I *think* this patch handles the case where this new
>> >> hot-added thing is also a bridge managed by acpiphp.  But where does
>> >> the ACPI device for this hot-added bridge come from?  It's an
>> >> arbitrary device the BIOS knows nothing about, so it can't be in the
>> >> DSDT.
>> >>
>> >
>> > So this came up while I was developing pci bridge hotplug for qemu.
>> > Currently, there is a top level host bus (with ACPI device definitions), where
>> > devices can be hot-plugged. What I've done is added a second level
>> > of hotplug pci busses (again with ACPI device definitions). Thus, we can
>> > now hotplug a bridge into the top-level bus and then devices behind it.
>> > Effectively increasing the hot-plug space from n -> n^2.
>> >
>> > Before the above pci patch, the devices behind the bridge would not
>> > configure their PCI BARs properly, since there were no i/o, mem resources
>> > assigned to the bridge. However, with the above patch in place things
>> > work as expected.
>> >
>> > Using the same code base I was able to do acpi hotplug on Windows 7,
>> > which correctly configured the both the bridge window and devices behind
>> > it on hot-plug. So currently, the above usage pattern works on Windows
>> > 7, but not on Linux.
>>
>> Thanks, Jason.  Do you have "lspci -v" output and the DSDT AML handy?
>> I'd like to look in more detail at what we're missing.
>
> Hi,
>
> Sorry for the delay...was on vacation.
>
> Anyways, below is the patch I have to the seabios acpi table. However,
> there are other pieces for seabios and qemu required. I still need to
> clean them up and send them out. But this pci patch (or something
> similar) is a required dependency.
>
> What 'lspci -v' output are you looking for?
>
> Let me know what else I can provide.

Now it's my turn to be sorry for dropping this for so long :)

I was thinking of the "lspci -v" output from your system (qemu guest)
and the disassembled DSDT extracted from the same system.  Then we
could talk about concrete devices and point to the corresponding
entries in lspci output and the ACPI namespace.

Re: [PATCH 4/5] PCI, acpiphp: add is_hotplug_bridge detection

[Jason Baron]

On Wed, Aug 15, 2012 at 01:36:43PM -0600, Bjorn Helgaas wrote:
> On Tue, Jul 17, 2012 at 8:14 AM, Jason Baron <jbaron@redhat.com> wrote:
> > On Wed, Jul 11, 2012 at 01:42:15PM -0600, Bjorn Helgaas wrote:
> >> On Wed, Jul 11, 2012 at 11:14 AM, Jason Baron <jbaron@redhat.com> wrote:
> >> > On Wed, Jul 11, 2012 at 09:50:19AM -0600, Bjorn Helgaas wrote:
> >> >> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> >> > When system support hotplug bridge with children hotplug slots, we need to make sure
> >> >> > that parent bridge get preallocated resource so later when device is plugged into
> >> >> > children slot, those children devices will get resource allocated.
> >> >> >
> >> >> > We do not meet this problem, because for pcie hotplug card, when acpiphp is used,
> >> >> > pci_scan_bridge will set that for us when detect hotplug bit in slot cap.
> >> >> >
> >> >> > Reported-and-tested-by: Jason Baron <jbaron@redhat.com>
> >> >> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> >> > Acked-by: Jason Baron <jbaron@redhat.com>
> >> >> > ---
> >> >> >  drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
> >> >> >  1 files changed, 26 insertions(+), 1 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> >> >> > index ad6fd66..0f2b72d 100644
> >> >> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> >> >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> >> >> > @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
> >> >> >         }
> >> >> >  }
> >> >> >
> >> >> > +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
> >> >> > +{
> >> >> > +       struct acpiphp_func *func;
> >> >> > +
> >> >> > +       if (!dev->subordinate)
> >> >> > +               return;
> >> >> > +
> >> >> > +       /* quirk, or pcie could set it already */
> >> >> > +       if (dev->is_hotplug_bridge)
> >> >> > +               return;
> >> >> > +
> >> >> > +       if (PCI_SLOT(dev->devfn) != slot->device)
> >> >> > +               return;
> >> >> > +
> >> >> > +       list_for_each_entry(func, &slot->funcs, sibling) {
> >> >> > +               if (PCI_FUNC(dev->devfn) == func->function) {
> >> >> > +                       /* check if this bridge has ejectable slots */
> >> >> > +                       if ((detect_ejectable_slots(func->handle) > 0))
> >> >> > +                               dev->is_hotplug_bridge = 1;
> >> >> > +                       break;
> >> >> > +               }
> >> >> > +       }
> >> >> > +}
> >> >> >  /**
> >> >> >   * enable_device - enable, configure a slot
> >> >> >   * @slot: slot to be enabled
> >> >> > @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >> >> >                         if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
> >> >> >                             dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
> >> >> >                                 max = pci_scan_bridge(bus, dev, max, pass);
> >> >> > -                               if (pass && dev->subordinate)
> >> >> > +                               if (pass && dev->subordinate) {
> >> >> > +                                       check_hotplug_bridge(slot, dev);
> >> >>
> >> >> I don't like this patch because it increases the differences between
> >> >> the hotplug drivers, rather than decreasing them.
> >> >>
> >> >> For PCI Express devices, we set dev->is_hotplug_bridge in the
> >> >> pci_setup_device() path (in set_pcie_hotplug_bridge()).  I think it
> >> >> would make sense to try to expand that path to also handle SHPC and
> >> >> ACPI hotplug as well.  ACPI is harder because it's not PCI-specified,
> >> >> so we'd need some sort of pcibios or other optional hook.
> >> >>
> >> >> I don't have a clear picture of how this works -- if I understand
> >> >> correctly, the situation is that we have a bridge managed by acpiphp.
> >> >> That part makes sense because the bridge is on the motherboard and can
> >> >> have a DSDT device.  Now we plug something into the slot below the
> >> >> bridge.  I *think* this patch handles the case where this new
> >> >> hot-added thing is also a bridge managed by acpiphp.  But where does
> >> >> the ACPI device for this hot-added bridge come from?  It's an
> >> >> arbitrary device the BIOS knows nothing about, so it can't be in the
> >> >> DSDT.
> >> >>
> >> >
> >> > So this came up while I was developing pci bridge hotplug for qemu.
> >> > Currently, there is a top level host bus (with ACPI device definitions), where
> >> > devices can be hot-plugged. What I've done is added a second level
> >> > of hotplug pci busses (again with ACPI device definitions). Thus, we can
> >> > now hotplug a bridge into the top-level bus and then devices behind it.
> >> > Effectively increasing the hot-plug space from n -> n^2.
> >> >
> >> > Before the above pci patch, the devices behind the bridge would not
> >> > configure their PCI BARs properly, since there were no i/o, mem resources
> >> > assigned to the bridge. However, with the above patch in place things
> >> > work as expected.
> >> >
> >> > Using the same code base I was able to do acpi hotplug on Windows 7,
> >> > which correctly configured the both the bridge window and devices behind
> >> > it on hot-plug. So currently, the above usage pattern works on Windows
> >> > 7, but not on Linux.
> >>
> >> Thanks, Jason.  Do you have "lspci -v" output and the DSDT AML handy?
> >> I'd like to look in more detail at what we're missing.
> >
> > Hi,
> >
> > Sorry for the delay...was on vacation.
> >
> > Anyways, below is the patch I have to the seabios acpi table. However,
> > there are other pieces for seabios and qemu required. I still need to
> > clean them up and send them out. But this pci patch (or something
> > similar) is a required dependency.
> >
> > What 'lspci -v' output are you looking for?
> >
> > Let me know what else I can provide.
> 
> Now it's my turn to be sorry for dropping this for so long :)
> 
> I was thinking of the "lspci -v" output from your system (qemu guest)
> and the disassembled DSDT extracted from the same system.  Then we
> could talk about concrete devices and point to the corresponding
> entries in lspci output and the ACPI namespace.

Hi,

posted to: http://people.redhat.com/~jbaron/bridge-hotplug/

I've extracted the ssdt and dsdt tables.

Thanks,

-Jason

Re: [PATCH 4/5] PCI, acpiphp: add is_hotplug_bridge detection

[Bjorn Helgaas]

On Mon, Aug 20, 2012 at 8:35 AM, Jason Baron <jbaron@redhat.com> wrote:
> On Wed, Aug 15, 2012 at 01:36:43PM -0600, Bjorn Helgaas wrote:
>> On Tue, Jul 17, 2012 at 8:14 AM, Jason Baron <jbaron@redhat.com> wrote:
>> > On Wed, Jul 11, 2012 at 01:42:15PM -0600, Bjorn Helgaas wrote:
>> >> On Wed, Jul 11, 2012 at 11:14 AM, Jason Baron <jbaron@redhat.com> wrote:
>> >> > On Wed, Jul 11, 2012 at 09:50:19AM -0600, Bjorn Helgaas wrote:
>> >> >> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> >> >> > When system support hotplug bridge with children hotplug slots, we need to make sure
>> >> >> > that parent bridge get preallocated resource so later when device is plugged into
>> >> >> > children slot, those children devices will get resource allocated.
>> >> >> >
>> >> >> > We do not meet this problem, because for pcie hotplug card, when acpiphp is used,
>> >> >> > pci_scan_bridge will set that for us when detect hotplug bit in slot cap.
>> >> >> >
>> >> >> > Reported-and-tested-by: Jason Baron <jbaron@redhat.com>
>> >> >> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> >> >> > Acked-by: Jason Baron <jbaron@redhat.com>
>> >> >> > ---
>> >> >> >  drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
>> >> >> >  1 files changed, 26 insertions(+), 1 deletions(-)
>> >> >> >
>> >> >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>> >> >> > index ad6fd66..0f2b72d 100644
>> >> >> > --- a/drivers/pci/hotplug/acpiphp_glue.c
>> >> >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> >> >> > @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
>> >> >> >         }
>> >> >> >  }
>> >> >> >
>> >> >> > +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
>> >> >> > +{
>> >> >> > +       struct acpiphp_func *func;
>> >> >> > +
>> >> >> > +       if (!dev->subordinate)
>> >> >> > +               return;
>> >> >> > +
>> >> >> > +       /* quirk, or pcie could set it already */
>> >> >> > +       if (dev->is_hotplug_bridge)
>> >> >> > +               return;
>> >> >> > +
>> >> >> > +       if (PCI_SLOT(dev->devfn) != slot->device)
>> >> >> > +               return;
>> >> >> > +
>> >> >> > +       list_for_each_entry(func, &slot->funcs, sibling) {
>> >> >> > +               if (PCI_FUNC(dev->devfn) == func->function) {
>> >> >> > +                       /* check if this bridge has ejectable slots */
>> >> >> > +                       if ((detect_ejectable_slots(func->handle) > 0))
>> >> >> > +                               dev->is_hotplug_bridge = 1;
>> >> >> > +                       break;
>> >> >> > +               }
>> >> >> > +       }
>> >> >> > +}
>> >> >> >  /**
>> >> >> >   * enable_device - enable, configure a slot
>> >> >> >   * @slot: slot to be enabled
>> >> >> > @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>> >> >> >                         if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
>> >> >> >                             dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
>> >> >> >                                 max = pci_scan_bridge(bus, dev, max, pass);
>> >> >> > -                               if (pass && dev->subordinate)
>> >> >> > +                               if (pass && dev->subordinate) {
>> >> >> > +                                       check_hotplug_bridge(slot, dev);
>> >> >>
>> >> >> I don't like this patch because it increases the differences between
>> >> >> the hotplug drivers, rather than decreasing them.
>> >> >>
>> >> >> For PCI Express devices, we set dev->is_hotplug_bridge in the
>> >> >> pci_setup_device() path (in set_pcie_hotplug_bridge()).  I think it
>> >> >> would make sense to try to expand that path to also handle SHPC and
>> >> >> ACPI hotplug as well.  ACPI is harder because it's not PCI-specified,
>> >> >> so we'd need some sort of pcibios or other optional hook.
>> >> >>
>> >> >> I don't have a clear picture of how this works -- if I understand
>> >> >> correctly, the situation is that we have a bridge managed by acpiphp.
>> >> >> That part makes sense because the bridge is on the motherboard and can
>> >> >> have a DSDT device.  Now we plug something into the slot below the
>> >> >> bridge.  I *think* this patch handles the case where this new
>> >> >> hot-added thing is also a bridge managed by acpiphp.  But where does
>> >> >> the ACPI device for this hot-added bridge come from?  It's an
>> >> >> arbitrary device the BIOS knows nothing about, so it can't be in the
>> >> >> DSDT.
>> >> >>
>> >> >
>> >> > So this came up while I was developing pci bridge hotplug for qemu.
>> >> > Currently, there is a top level host bus (with ACPI device definitions), where
>> >> > devices can be hot-plugged. What I've done is added a second level
>> >> > of hotplug pci busses (again with ACPI device definitions). Thus, we can
>> >> > now hotplug a bridge into the top-level bus and then devices behind it.
>> >> > Effectively increasing the hot-plug space from n -> n^2.
>> >> >
>> >> > Before the above pci patch, the devices behind the bridge would not
>> >> > configure their PCI BARs properly, since there were no i/o, mem resources
>> >> > assigned to the bridge. However, with the above patch in place things
>> >> > work as expected.
>> >> >
>> >> > Using the same code base I was able to do acpi hotplug on Windows 7,
>> >> > which correctly configured the both the bridge window and devices behind
>> >> > it on hot-plug. So currently, the above usage pattern works on Windows
>> >> > 7, but not on Linux.
>> >>
>> >> Thanks, Jason.  Do you have "lspci -v" output and the DSDT AML handy?
>> >> I'd like to look in more detail at what we're missing.
>> >
>> > Hi,
>> >
>> > Sorry for the delay...was on vacation.
>> >
>> > Anyways, below is the patch I have to the seabios acpi table. However,
>> > there are other pieces for seabios and qemu required. I still need to
>> > clean them up and send them out. But this pci patch (or something
>> > similar) is a required dependency.
>> >
>> > What 'lspci -v' output are you looking for?
>> >
>> > Let me know what else I can provide.
>>
>> Now it's my turn to be sorry for dropping this for so long :)
>>
>> I was thinking of the "lspci -v" output from your system (qemu guest)
>> and the disassembled DSDT extracted from the same system.  Then we
>> could talk about concrete devices and point to the corresponding
>> entries in lspci output and the ACPI namespace.
>
> Hi,
>
> posted to: http://people.redhat.com/~jbaron/bridge-hotplug/
>
> I've extracted the ssdt and dsdt tables.

Thanks, Jason.

I'd like to play with this on qemu myself so I don't have to ask so
many stupid questions.  Earlier you mentioned some other qemu and
seabios patches.  Are they usable yet?  I see the src/ssdt-pcihp.dsl
patch you posted Jul 17, but I think you were referring to some other
ones.

Bjorn

Re: [PATCH 4/5] PCI, acpiphp: add is_hotplug_bridge detection

[Jason Baron]

On Wed, Aug 22, 2012 at 05:19:35PM -0600, Bjorn Helgaas wrote:
> > On Wed, Aug 15, 2012 at 01:36:43PM -0600, Bjorn Helgaas wrote:
> >> On Tue, Jul 17, 2012 at 8:14 AM, Jason Baron <jbaron@redhat.com> wrote:
> >> > On Wed, Jul 11, 2012 at 01:42:15PM -0600, Bjorn Helgaas wrote:
> >> >> On Wed, Jul 11, 2012 at 11:14 AM, Jason Baron <jbaron@redhat.com> wrote:
> >> >> > On Wed, Jul 11, 2012 at 09:50:19AM -0600, Bjorn Helgaas wrote:
> >> >> >> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> >> >> > When system support hotplug bridge with children hotplug slots, we need to make sure
> >> >> >> > that parent bridge get preallocated resource so later when device is plugged into
> >> >> >> > children slot, those children devices will get resource allocated.
> >> >> >> >
> >> >> >> > We do not meet this problem, because for pcie hotplug card, when acpiphp is used,
> >> >> >> > pci_scan_bridge will set that for us when detect hotplug bit in slot cap.
> >> >> >> >
> >> >> >> > Reported-and-tested-by: Jason Baron <jbaron@redhat.com>
> >> >> >> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> >> >> > Acked-by: Jason Baron <jbaron@redhat.com>
> >> >> >> > ---
> >> >> >> >  drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
> >> >> >> >  1 files changed, 26 insertions(+), 1 deletions(-)
> >> >> >> >
> >> >> >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> >> >> >> > index ad6fd66..0f2b72d 100644
> >> >> >> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> >> >> >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> >> >> >> > @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
> >> >> >> >         }
> >> >> >> >  }
> >> >> >> >
> >> >> >> > +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
> >> >> >> > +{
> >> >> >> > +       struct acpiphp_func *func;
> >> >> >> > +
> >> >> >> > +       if (!dev->subordinate)
> >> >> >> > +               return;
> >> >> >> > +
> >> >> >> > +       /* quirk, or pcie could set it already */
> >> >> >> > +       if (dev->is_hotplug_bridge)
> >> >> >> > +               return;
> >> >> >> > +
> >> >> >> > +       if (PCI_SLOT(dev->devfn) != slot->device)
> >> >> >> > +               return;
> >> >> >> > +
> >> >> >> > +       list_for_each_entry(func, &slot->funcs, sibling) {
> >> >> >> > +               if (PCI_FUNC(dev->devfn) == func->function) {
> >> >> >> > +                       /* check if this bridge has ejectable slots */
> >> >> >> > +                       if ((detect_ejectable_slots(func->handle) > 0))
> >> >> >> > +                               dev->is_hotplug_bridge = 1;
> >> >> >> > +                       break;
> >> >> >> > +               }
> >> >> >> > +       }
> >> >> >> > +}
> >> >> >> >  /**
> >> >> >> >   * enable_device - enable, configure a slot
> >> >> >> >   * @slot: slot to be enabled
> >> >> >> > @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >> >> >> >                         if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
> >> >> >> >                             dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
> >> >> >> >                                 max = pci_scan_bridge(bus, dev, max, pass);
> >> >> >> > -                               if (pass && dev->subordinate)
> >> >> >> > +                               if (pass && dev->subordinate) {
> >> >> >> > +                                       check_hotplug_bridge(slot, dev);
> >> >> >>
> >> >> >> I don't like this patch because it increases the differences between
> >> >> >> the hotplug drivers, rather than decreasing them.
> >> >> >>
> >> >> >> For PCI Express devices, we set dev->is_hotplug_bridge in the
> >> >> >> pci_setup_device() path (in set_pcie_hotplug_bridge()).  I think it
> >> >> >> would make sense to try to expand that path to also handle SHPC and
> >> >> >> ACPI hotplug as well.  ACPI is harder because it's not PCI-specified,
> >> >> >> so we'd need some sort of pcibios or other optional hook.
> >> >> >>
> >> >> >> I don't have a clear picture of how this works -- if I understand
> >> >> >> correctly, the situation is that we have a bridge managed by acpiphp.
> >> >> >> That part makes sense because the bridge is on the motherboard and can
> >> >> >> have a DSDT device.  Now we plug something into the slot below the
> >> >> >> bridge.  I *think* this patch handles the case where this new
> >> >> >> hot-added thing is also a bridge managed by acpiphp.  But where does
> >> >> >> the ACPI device for this hot-added bridge come from?  It's an
> >> >> >> arbitrary device the BIOS knows nothing about, so it can't be in the
> >> >> >> DSDT.
> >> >> >>
> >> >> >
> >> >> > So this came up while I was developing pci bridge hotplug for qemu.
> >> >> > Currently, there is a top level host bus (with ACPI device definitions), where
> >> >> > devices can be hot-plugged. What I've done is added a second level
> >> >> > of hotplug pci busses (again with ACPI device definitions). Thus, we can
> >> >> > now hotplug a bridge into the top-level bus and then devices behind it.
> >> >> > Effectively increasing the hot-plug space from n -> n^2.
> >> >> >
> >> >> > Before the above pci patch, the devices behind the bridge would not
> >> >> > configure their PCI BARs properly, since there were no i/o, mem resources
> >> >> > assigned to the bridge. However, with the above patch in place things
> >> >> > work as expected.
> >> >> >
> >> >> > Using the same code base I was able to do acpi hotplug on Windows 7,
> >> >> > which correctly configured the both the bridge window and devices behind
> >> >> > it on hot-plug. So currently, the above usage pattern works on Windows
> >> >> > 7, but not on Linux.
> >> >>
> >> >> Thanks, Jason.  Do you have "lspci -v" output and the DSDT AML handy?
> >> >> I'd like to look in more detail at what we're missing.
> >> >
> >> > Hi,
> >> >
> >> > Sorry for the delay...was on vacation.
> >> >
> >> > Anyways, below is the patch I have to the seabios acpi table. However,
> >> > there are other pieces for seabios and qemu required. I still need to
> >> > clean them up and send them out. But this pci patch (or something
> >> > similar) is a required dependency.
> >> >
> >> > What 'lspci -v' output are you looking for?
> >> >
> >> > Let me know what else I can provide.
> >>
> >> Now it's my turn to be sorry for dropping this for so long :)
> >>
> >> I was thinking of the "lspci -v" output from your system (qemu guest)
> >> and the disassembled DSDT extracted from the same system.  Then we
> >> could talk about concrete devices and point to the corresponding
> >> entries in lspci output and the ACPI namespace.
> >
> > Hi,
> >
> > posted to: http://people.redhat.com/~jbaron/bridge-hotplug/
> >
> > I've extracted the ssdt and dsdt tables.
> 
> Thanks, Jason.
> 
> I'd like to play with this on qemu myself so I don't have to ask so
> many stupid questions.  Earlier you mentioned some other qemu and
> seabios patches.  Are they usable yet?  I see the src/ssdt-pcihp.dsl
> patch you posted Jul 17, but I think you were referring to some other
> ones.
> 
> Bjorn


Hi Bjorn,

So you can try this out for yourself by cloning:

git://github.com/jibaron/q35-qemu.git
git://github.com/jibaron/q35-seabios.git

You will need to use the hotplug branches: 'bridge-hotplug'.

Then, you can start up a guest using something like:

/usr/local/bin/qemu-system-x86_64  -name "f16" -M pc -m 4G -smp 4
-drive file=/images/f16.img,index=0,media=disk,format=raw
-netdev tap,id=hostnet0,script=/home/scripts/qemu-ifup -vnc :5 --enable-kvm
-monitor stdio -bios /home/seabios/seabios/out/bios.bin 

Then, to test the hotplug from the qemu monitor command line you do:

(qemu) device_add pci-bridge,chassis_nr=10,id=br10,addr=0x1f [to add a
bridge at slot 0x1f]
(qemu) device_add e1000,netdev=hostnet0,id=net1,mac=52:54:00:05:11:10,bus=br10,addr=0x1
[to add an e1000 device off of the bridge in slot 0x1]

You'll notice that the resource for the e1000 wouldn't configure without
the patch in this thread in the guest kernel.

Please let me know if you need any more help re-producing this.

Thanks!

-Jason

[PATCH 5/5] PCI: add root bus children dev's res to fail list

[Yinghai Lu]

We can stop according to try number now and do not need to use root_bus checking
as stop sign anymore.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/setup-bus.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 561e41c..bb02b25 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -283,7 +283,7 @@ static void assign_requested_resources_sorted(struct list_head *head,
 		idx = res - &dev_res->dev->resource[0];
 		if (resource_size(res) &&
 		    pci_assign_resource(dev_res->dev, idx)) {
-			if (fail_head && !pci_is_root_bus(dev_res->dev->bus)) {
+			if (fail_head) {
 				/*
 				 * if the failed res is for ROM BAR, and it will
 				 * be enabled later, don't add it to the list
-- 
1.7.7

Re: [PATCH 0/5] PCI: hotplug related misc patches

[Yinghai Lu]

On Sat, Jun 23, 2012 at 12:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> minor changes that should be safe for 3.6
>
> could get from
>         git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-next
>
> Yinghai Lu (5):
>   PCI, acpiphp: remove not used res_lock
>   pciehp: Don't enable presence notification while surprise removal is
>     not supported.
>   PCI, acpiphp: Merge acpiphp_debug and debug
>   PCI, acpiphp: add is_hotplug_bridge detection
>   PCI: add root bus children dev's res to fail list
>
>  drivers/pci/hotplug/acpiphp.h      |    4 +---
>  drivers/pci/hotplug/acpiphp_core.c |    7 ++-----
>  drivers/pci/hotplug/acpiphp_glue.c |   30 ++++++++++++++++++++++++++----
>  drivers/pci/hotplug/pciehp_hpc.c   |    5 +++--
>  drivers/pci/setup-bus.c            |    2 +-
>  5 files changed, 33 insertions(+), 15 deletions(-)

can you put those 5 patches into pci/next?

Those should be safe for v3.6 merging window.

Thanks

Yinghai

Re: [PATCH 0/5] PCI: hotplug related misc patches

[Bjorn Helgaas]

On Tue, Jul 10, 2012 at 1:30 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Sat, Jun 23, 2012 at 12:42 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> minor changes that should be safe for 3.6
>>
>> could get from
>>         git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-next
>>
>> Yinghai Lu (5):
>>   PCI, acpiphp: remove not used res_lock
>>   PCI, acpiphp: Merge acpiphp_debug and debug

I merged the two patches above to my "next" branch.

>>   PCI, acpiphp: add is_hotplug_bridge detection
>>   pciehp: Don't enable presence notification while surprise removal is
>>     not supported.
>>   PCI: add root bus children dev's res to fail list

The above seem to need more work.  We've discussed the acpiphp and
pciehp patches on the list, and the last doesn't seem like enough of a
cleanup to warrant a commit.  I hope that someday resource assignment
becomes intelligible, but I don't want to do it one line at a time.

>>  drivers/pci/hotplug/acpiphp.h      |    4 +---
>>  drivers/pci/hotplug/acpiphp_core.c |    7 ++-----
>>  drivers/pci/hotplug/acpiphp_glue.c |   30 ++++++++++++++++++++++++++----
>>  drivers/pci/hotplug/pciehp_hpc.c   |    5 +++--
>>  drivers/pci/setup-bus.c            |    2 +-
>>  5 files changed, 33 insertions(+), 15 deletions(-)
>
> can you put those 5 patches into pci/next?
>
> Those should be safe for v3.6 merging window.
>
> Thanks
>
> Yinghai