[PATCH 0/3] Thunderbolt workarounds

[PATCH 0/3] Thunderbolt workarounds

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>

I had chance to test two PC setups with Thunderbolt: Acer Aspire S5 and
Intel DZ77RE-75K motherboard. Unfortunately, both of them are broken in
different ways.

This patchset contains workarounds for the issues.

Kirill A. Shutemov (3):
  PCI Hotplug: workaround for Thunderbolt on Acer Aspire S5
  PCI Hotplug: convert acpiphp_hp_work to use delayed work
  PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K
    motherboard

 drivers/pci/hotplug/acpi_pcihp.c   |   13 +++++++++++++
 drivers/pci/hotplug/acpiphp_glue.c |   33 +++++++++++++++++++++++++--------
 2 files changed, 38 insertions(+), 8 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] PCI Hotplug: workaround for Thunderbolt on Acer Aspire S5

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>

Correct ACPI PCI hotplug imeplementation should have _RMV method in a
PCI slot (device under pci bridge). In Acer Aspire S5 case we have it
deeper in hierarchy:

Device (RP05)
{
   // ...
   Device (HRUP)
   {
       // ...
       Device (HRDN)
       {
           // ...
           Device (EPUP)
           {
               // ...
               Method (_RMV, 0, NotSerialized)  // _RMV: Removal Status
               {
                   Return (One)
               }
           }
       }
   }
}

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 drivers/pci/hotplug/acpi_pcihp.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
index 2a47e82..d92ebfb 100644
--- a/drivers/pci/hotplug/acpi_pcihp.c
+++ b/drivers/pci/hotplug/acpi_pcihp.c
@@ -422,6 +422,19 @@ static int pcihp_is_ejectable(acpi_handle handle)
 	status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable);
 	if (ACPI_SUCCESS(status) && removable)
 		return 1;
+
+	/*
+	 * Workaround for Thunderbolt implementation on Acer Aspire S5.
+	 *
+	 * Correct ACPI PCI hotplug imeplementation has _RMV method in a PCI
+	 * slot (device under pci bridge). In Acer Aspire S5 case we have it
+	 * deeper in hierarchy.
+	 */
+	status = acpi_evaluate_integer(handle, "HRDN.EPUP._RMV", NULL,
+			&removable);
+	if (ACPI_SUCCESS(status) && removable)
+		return 1;
+
 	return 0;
 }
 
-- 
1.7.10.4

Re: [PATCH 1/3] PCI Hotplug: workaround for Thunderbolt on Acer Aspire S5

[Greg KH]

On Thu, Dec 13, 2012 at 05:31:46PM +0200, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> 
> Correct ACPI PCI hotplug imeplementation should have _RMV method in a
> PCI slot (device under pci bridge). In Acer Aspire S5 case we have it
> deeper in hierarchy:
> 
> Device (RP05)
> {
>    // ...
>    Device (HRUP)
>    {
>        // ...
>        Device (HRDN)
>        {
>            // ...
>            Device (EPUP)
>            {
>                // ...
>                Method (_RMV, 0, NotSerialized)  // _RMV: Removal Status
>                {
>                    Return (One)
>                }
>            }
>        }
>    }
> }
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  drivers/pci/hotplug/acpi_pcihp.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> index 2a47e82..d92ebfb 100644
> --- a/drivers/pci/hotplug/acpi_pcihp.c
> +++ b/drivers/pci/hotplug/acpi_pcihp.c
> @@ -422,6 +422,19 @@ static int pcihp_is_ejectable(acpi_handle handle)
>  	status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable);
>  	if (ACPI_SUCCESS(status) && removable)
>  		return 1;
> +
> +	/*
> +	 * Workaround for Thunderbolt implementation on Acer Aspire S5.
> +	 *
> +	 * Correct ACPI PCI hotplug imeplementation has _RMV method in a PCI
> +	 * slot (device under pci bridge). In Acer Aspire S5 case we have it
> +	 * deeper in hierarchy.
> +	 */
> +	status = acpi_evaluate_integer(handle, "HRDN.EPUP._RMV", NULL,
> +			&removable);
> +	if (ACPI_SUCCESS(status) && removable)
> +		return 1;

I have no objection to this patch as-is, but I wonder how will other
BIOSes implement this "incorrectly" in the future.  Should we always
just try to walk the whole PCI slot heirachy looking for the _RMV
attribute?  That should solve the problem where someone else places this
at another location for the slot, right?

Is there any test for Windows that ensures that this gets placed in the
"correct" location that we can rely on?

thanks,

greg k-h

Re: [PATCH 1/3] PCI Hotplug: workaround for Thunderbolt on Acer Aspire S5

[Kirill A. Shutemov]

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

On Thu, Dec 13, 2012 at 10:44:41AM -0800, Greg KH wrote:
> On Thu, Dec 13, 2012 at 05:31:46PM +0200, Kirill A. Shutemov wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > 
> > Correct ACPI PCI hotplug imeplementation should have _RMV method in a
> > PCI slot (device under pci bridge). In Acer Aspire S5 case we have it
> > deeper in hierarchy:
> > 
> > Device (RP05)
> > {
> >    // ...
> >    Device (HRUP)
> >    {
> >        // ...
> >        Device (HRDN)
> >        {
> >            // ...
> >            Device (EPUP)
> >            {
> >                // ...
> >                Method (_RMV, 0, NotSerialized)  // _RMV: Removal Status
> >                {
> >                    Return (One)
> >                }
> >            }
> >        }
> >    }
> > }
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  drivers/pci/hotplug/acpi_pcihp.c |   13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> > index 2a47e82..d92ebfb 100644
> > --- a/drivers/pci/hotplug/acpi_pcihp.c
> > +++ b/drivers/pci/hotplug/acpi_pcihp.c
> > @@ -422,6 +422,19 @@ static int pcihp_is_ejectable(acpi_handle handle)
> >  	status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable);
> >  	if (ACPI_SUCCESS(status) && removable)
> >  		return 1;
> > +
> > +	/*
> > +	 * Workaround for Thunderbolt implementation on Acer Aspire S5.
> > +	 *
> > +	 * Correct ACPI PCI hotplug imeplementation has _RMV method in a PCI
> > +	 * slot (device under pci bridge). In Acer Aspire S5 case we have it
> > +	 * deeper in hierarchy.
> > +	 */
> > +	status = acpi_evaluate_integer(handle, "HRDN.EPUP._RMV", NULL,
> > +			&removable);
> > +	if (ACPI_SUCCESS(status) && removable)
> > +		return 1;
> 
> I have no objection to this patch as-is, but I wonder how will other
> BIOSes implement this "incorrectly" in the future.  Should we always
> just try to walk the whole PCI slot heirachy looking for the _RMV
> attribute?  That should solve the problem where someone else places this
> at another location for the slot, right?

I'm new with PCI and I'm not sure what problems can cause false positive
here.
What will heppend if we find yet another PCI-to-PCI bridge in the hierarchy
and some of slots of that bridge will have _RMV method? Is it possible,
right?

I prefer postpone any generalization before we will find any similar bug
on other HW. (I naively hope BIOS developers will not repeat theirs bugs).

> Is there any test for Windows that ensures that this gets placed in the
> "correct" location that we can rely on?

No idea. I haven't seen any Windows.

-- 
 Kirill A. Shutemov

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] PCI Hotplug: workaround for Thunderbolt on Acer Aspire S5

[Bjorn Helgaas]

[+cc Rafael]

On Thu, Dec 13, 2012 at 8:31 AM, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> Correct ACPI PCI hotplug imeplementation should have _RMV method in a
> PCI slot (device under pci bridge). In Acer Aspire S5 case we have it
> deeper in hierarchy:
>
> Device (RP05)
> {
>    // ...
>    Device (HRUP)
>    {
>        // ...
>        Device (HRDN)
>        {
>            // ...
>            Device (EPUP)
>            {
>                // ...
>                Method (_RMV, 0, NotSerialized)  // _RMV: Removal Status
>                {
>                    Return (One)
>                }
>            }
>        }
>    }
> }
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  drivers/pci/hotplug/acpi_pcihp.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> index 2a47e82..d92ebfb 100644
> --- a/drivers/pci/hotplug/acpi_pcihp.c
> +++ b/drivers/pci/hotplug/acpi_pcihp.c
> @@ -422,6 +422,19 @@ static int pcihp_is_ejectable(acpi_handle handle)
>         status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable);
>         if (ACPI_SUCCESS(status) && removable)
>                 return 1;
> +
> +       /*
> +        * Workaround for Thunderbolt implementation on Acer Aspire S5.
> +        *
> +        * Correct ACPI PCI hotplug imeplementation has _RMV method in a PCI
> +        * slot (device under pci bridge). In Acer Aspire S5 case we have it
> +        * deeper in hierarchy.
> +        */
> +       status = acpi_evaluate_integer(handle, "HRDN.EPUP._RMV", NULL,
> +                       &removable);

I don't think encoding an ACPI path from the BIOS of a random machine
here is the right approach.  The path components are completely
implementation-dependent, and we'll end up carrying this test forever,
long after the last Aspire S5 is in the landfill.

We need a more generic approach that covers this case.  It would be
interesting to see the rest of the ACPI namespace details under
RP05.HRUP.  I guess we have RP05.HRUP._ADR, at least.  I'm not sure
what the BIOS is trying to tell us by providing
RP05.HRUP.HRDN.EPUP._RMV, but maybe we could figure it out if we knew
more about HRDN and EPUP.

> +       if (ACPI_SUCCESS(status) && removable)
> +               return 1;
> +
>         return 0;
>  }
>
> --
> 1.7.10.4
>
> --
> 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 1/3] PCI Hotplug: workaround for Thunderbolt on Acer Aspire S5

[Kirill A. Shutemov]

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

On Thu, Dec 13, 2012 at 05:22:25PM -0700, Bjorn Helgaas wrote:
> [+cc Rafael]
> 
> On Thu, Dec 13, 2012 at 8:31 AM, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >
> > Correct ACPI PCI hotplug imeplementation should have _RMV method in a
> > PCI slot (device under pci bridge). In Acer Aspire S5 case we have it
> > deeper in hierarchy:
> >
> > Device (RP05)
> > {
> >    // ...
> >    Device (HRUP)
> >    {
> >        // ...
> >        Device (HRDN)
> >        {
> >            // ...
> >            Device (EPUP)
> >            {
> >                // ...
> >                Method (_RMV, 0, NotSerialized)  // _RMV: Removal Status
> >                {
> >                    Return (One)
> >                }
> >            }
> >        }
> >    }
> > }
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  drivers/pci/hotplug/acpi_pcihp.c |   13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> > index 2a47e82..d92ebfb 100644
> > --- a/drivers/pci/hotplug/acpi_pcihp.c
> > +++ b/drivers/pci/hotplug/acpi_pcihp.c
> > @@ -422,6 +422,19 @@ static int pcihp_is_ejectable(acpi_handle handle)
> >         status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable);
> >         if (ACPI_SUCCESS(status) && removable)
> >                 return 1;
> > +
> > +       /*
> > +        * Workaround for Thunderbolt implementation on Acer Aspire S5.
> > +        *
> > +        * Correct ACPI PCI hotplug imeplementation has _RMV method in a PCI
> > +        * slot (device under pci bridge). In Acer Aspire S5 case we have it
> > +        * deeper in hierarchy.
> > +        */
> > +       status = acpi_evaluate_integer(handle, "HRDN.EPUP._RMV", NULL,
> > +                       &removable);
> 
> I don't think encoding an ACPI path from the BIOS of a random machine
> here is the right approach.  The path components are completely
> implementation-dependent, and we'll end up carrying this test forever,
> long after the last Aspire S5 is in the landfill.
> 
> We need a more generic approach that covers this case.  It would be
> interesting to see the rest of the ACPI namespace details under
> RP05.HRUP.  I guess we have RP05.HRUP._ADR, at least.  I'm not sure
> what the BIOS is trying to tell us by providing
> RP05.HRUP.HRDN.EPUP._RMV, but maybe we could figure it out if we knew
> more about HRDN and EPUP.

My guesses about acronyms:
HRUP -- host router upstream port (connected to PCI root port).
HRDN -- host router downstream port.
EPUP -- end point upstream port.

Looks like BIOS developers tried to be smart and described actual hierarchy
from root port to end point.
The problem is that it doesn't fit to ACPI PCI hotplug. :(

Don't see anything useful in RP05.HRUP:

Device (HRUP)
{
    Name (_ADR, Zero)  // _ADR: Address
    Name (_PRW, Package (0x02)  // _PRW: Power Resources for Wake
    {
        0x09, 
        0x04
    })
    Device (HRDN)
    {
        Name (_ADR, 0x00040000)  // _ADR: Address
        Name (_PRW, Package (0x02)  // _PRW: Power Resources for Wake
        {
            0x09, 
            0x04
        })
        Device (EPUP)
        {
            Name (_ADR, Zero)  // _ADR: Address
            Method (_RMV, 0, NotSerialized)  // _RMV: Removal Status
            {
                Return (One)
            }
        }
    }
}

-- 
 Kirill A. Shutemov

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/3] PCI Hotplug: convert acpiphp_hp_work to use delayed work

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>

Some broken ACPI PCI Hotplug implementations notify OS about hotplug
before devices ready to be enumerated.

Let's convert acpiphp_hp_work to delayed work. It will allow to delay
enumeration on broken hardware to workaround the issue.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 3d6d4fd..1a2b3ca 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -1191,7 +1191,7 @@ check_sub_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 }
 
 struct acpiphp_hp_work {
-	struct work_struct work;
+	struct delayed_work work;
 	acpi_handle handle;
 	u32 type;
 	void *context;
@@ -1199,7 +1199,8 @@ struct acpiphp_hp_work {
 
 static void alloc_acpiphp_hp_work(acpi_handle handle, u32 type,
 				  void *context,
-				  void (*func)(struct work_struct *work))
+				  void (*func)(struct work_struct *work),
+				  unsigned long delay)
 {
 	struct acpiphp_hp_work *hp_work;
 	int ret;
@@ -1212,8 +1213,8 @@ static void alloc_acpiphp_hp_work(acpi_handle handle, u32 type,
 	hp_work->type = type;
 	hp_work->context = context;
 
-	INIT_WORK(&hp_work->work, func);
-	ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
+	INIT_DELAYED_WORK(&hp_work->work, func);
+	ret = queue_delayed_work(kacpi_hotplug_wq, &hp_work->work, delay);
 	if (!ret)
 		kfree(hp_work);
 }
@@ -1230,7 +1231,8 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
 	acpi_handle handle;
 	u32 type;
 
-	hp_work = container_of(work, struct acpiphp_hp_work, work);
+	hp_work = container_of(to_delayed_work(work),
+			struct acpiphp_hp_work, work);
 	handle = hp_work->handle;
 	type = hp_work->type;
 
@@ -1334,7 +1336,7 @@ static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
 	 * don't deadlock on hotplug actions.
 	 */
 	alloc_acpiphp_hp_work(handle, type, context,
-			      _handle_hotplug_event_bridge);
+			      _handle_hotplug_event_bridge, 0);
 }
 
 static void _handle_hotplug_event_func(struct work_struct *work)
@@ -1348,7 +1350,8 @@ static void _handle_hotplug_event_func(struct work_struct *work)
 	u32 type;
 	void *context;
 
-	hp_work = container_of(work, struct acpiphp_hp_work, work);
+	hp_work = container_of(to_delayed_work(work),
+			struct acpiphp_hp_work, work);
 	handle = hp_work->handle;
 	type = hp_work->type;
 	context = hp_work->context;
@@ -1410,7 +1413,7 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
 	 * don't deadlock on hotplug actions.
 	 */
 	alloc_acpiphp_hp_work(handle, type, context,
-			      _handle_hotplug_event_func);
+			      _handle_hotplug_event_func, 0);
 }
 
 static acpi_status
-- 
1.7.10.4

[PATCH 3/3] PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K motherboard

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>

BIOS on Intel DZ77RE-75K motherboard notifies OS about Thunderbolt
hotplug before devices behind Thunderbolt are ready to be enumerated.

Let's delay enumeration by 2 seconds.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 1a2b3ca..165987a 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -49,6 +49,7 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 
 #include "../pci.h"
 #include "acpiphp.h"
@@ -1327,6 +1328,19 @@ out:
 static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
 					void *context)
 {
+	unsigned long delay = 0;
+	const char *board_name;
+
+	board_name = dmi_get_system_info(DMI_BOARD_NAME);
+	/*
+	 * BIOS on Intel DZ77RE-75K motherboard notifies OS about Thunderbolt
+	 * hotplug before devices behind Thunderbolt are ready to be
+	 * enumerated.
+	 * Let's delay enumeration by 2 seconds.
+	 */
+	if (board_name && !strcmp(board_name, "DZ77RE-75K"))
+		delay = 2 * HZ;
+
 	/*
 	 * Currently the code adds all hotplug events to the kacpid_wq
 	 * queue when it should add hotplug events to the kacpi_hotplug_wq.
@@ -1336,7 +1350,7 @@ static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
 	 * don't deadlock on hotplug actions.
 	 */
 	alloc_acpiphp_hp_work(handle, type, context,
-			      _handle_hotplug_event_bridge, 0);
+			      _handle_hotplug_event_bridge, delay);
 }
 
 static void _handle_hotplug_event_func(struct work_struct *work)
-- 
1.7.10.4

Re: [PATCH 3/3] PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K motherboard

[Greg KH]

On Thu, Dec 13, 2012 at 05:31:48PM +0200, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> 
> BIOS on Intel DZ77RE-75K motherboard notifies OS about Thunderbolt
> hotplug before devices behind Thunderbolt are ready to be enumerated.
> 
> Let's delay enumeration by 2 seconds.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c |   16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 1a2b3ca..165987a 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -49,6 +49,7 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
> +#include <linux/dmi.h>
>  
>  #include "../pci.h"
>  #include "acpiphp.h"
> @@ -1327,6 +1328,19 @@ out:
>  static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
>  					void *context)
>  {
> +	unsigned long delay = 0;
> +	const char *board_name;
> +
> +	board_name = dmi_get_system_info(DMI_BOARD_NAME);
> +	/*
> +	 * BIOS on Intel DZ77RE-75K motherboard notifies OS about Thunderbolt
> +	 * hotplug before devices behind Thunderbolt are ready to be
> +	 * enumerated.
> +	 * Let's delay enumeration by 2 seconds.
> +	 */
> +	if (board_name && !strcmp(board_name, "DZ77RE-75K"))
> +		delay = 2 * HZ;

Again, no objection to this patch as-is, but should we make it a bit
more "general" and provide a quirk table to make this type of fix easier
for other motherboards?  Shouldn't we also match on a manufacturer field
as well as the board_name?  The kernel provides a very easy to match on
any arbritary set of DMI fields for this kind of problem that we could
use here.

I can make that change if needed.

What are the odds that other boards are going to need a longer delay
time?  Actually, is there a spec saying how long we need to wait?  The
ACPI PCI Hotplug had a value somewhere that the hardware was supposed to
follow, but I don't have access to that ACPI spec anymore to verify
this...

thanks,

greg k-h

Re: [PATCH 3/3] PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K motherboard

[Kirill A. Shutemov]

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

On Thu, Dec 13, 2012 at 10:48:20AM -0800, Greg KH wrote:
> On Thu, Dec 13, 2012 at 05:31:48PM +0200, Kirill A. Shutemov wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > 
> > BIOS on Intel DZ77RE-75K motherboard notifies OS about Thunderbolt
> > hotplug before devices behind Thunderbolt are ready to be enumerated.
> > 
> > Let's delay enumeration by 2 seconds.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  drivers/pci/hotplug/acpiphp_glue.c |   16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > index 1a2b3ca..165987a 100644
> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -49,6 +49,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/slab.h>
> >  #include <linux/acpi.h>
> > +#include <linux/dmi.h>
> >  
> >  #include "../pci.h"
> >  #include "acpiphp.h"
> > @@ -1327,6 +1328,19 @@ out:
> >  static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
> >  					void *context)
> >  {
> > +	unsigned long delay = 0;
> > +	const char *board_name;
> > +
> > +	board_name = dmi_get_system_info(DMI_BOARD_NAME);
> > +	/*
> > +	 * BIOS on Intel DZ77RE-75K motherboard notifies OS about Thunderbolt
> > +	 * hotplug before devices behind Thunderbolt are ready to be
> > +	 * enumerated.
> > +	 * Let's delay enumeration by 2 seconds.
> > +	 */
> > +	if (board_name && !strcmp(board_name, "DZ77RE-75K"))
> > +		delay = 2 * HZ;
> 
> Again, no objection to this patch as-is, but should we make it a bit
> more "general" and provide a quirk table to make this type of fix easier
> for other motherboards?  Shouldn't we also match on a manufacturer field
> as well as the board_name?  The kernel provides a very easy to match on
> any arbritary set of DMI fields for this kind of problem that we could
> use here.

Something like this (untested):

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 1a2b3ca..c4db634 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -49,6 +49,7 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 
 #include "../pci.h"
 #include "acpiphp.h"
@@ -1327,6 +1328,31 @@ out:
 static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
 					void *context)
 {
+	unsigned long delay = 0;
+	const struct dmi_system_id *match;
+	static const struct dmi_system_id sysids[] = {
+		/*
+		 * BIOS on Intel DZ77RE-75K motherboard notifies OS about
+		 * Thunderbolt hotplug before devices behind Thunderbolt are
+		 * ready to be enumerated.
+		 * Let's delay enumeration by 2 seconds.
+		 */
+		{
+			.ident = "DZ77RE-75K",
+			.matches = {
+				DMI_MATCH(DMI_BOARD_VENDOR,
+						"Intel Corporation"),
+				DMI_MATCH(DMI_BOARD_NAME,
+						"DZ77RE-75K"),
+			},
+			.driver_data = (void *) (2 * HZ), /* delay */
+		},
+	};
+
+	match = dmi_first_match(sysids);
+	if (match)
+		delay = (unsigned long) match->driver_data;
+
 	/*
 	 * Currently the code adds all hotplug events to the kacpid_wq
 	 * queue when it should add hotplug events to the kacpi_hotplug_wq.
@@ -1336,7 +1362,7 @@ static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
 	 * don't deadlock on hotplug actions.
 	 */
 	alloc_acpiphp_hp_work(handle, type, context,
-			      _handle_hotplug_event_bridge, 0);
+			      _handle_hotplug_event_bridge, delay);
 }
 
 static void _handle_hotplug_event_func(struct work_struct *work)

-- 
 Kirill A. Shutemov

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K motherboard

[Yinghai Lu]

On Thu, Dec 13, 2012 at 12:08 PM, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
> On Thu, Dec 13, 2012 at 10:48:20AM -0800, Greg KH wrote:
>> On Thu, Dec 13, 2012 at 05:31:48PM +0200, Kirill A. Shutemov wrote:
>> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>> >
>> > BIOS on Intel DZ77RE-75K motherboard notifies OS about Thunderbolt
>> > hotplug before devices behind Thunderbolt are ready to be enumerated.
>> >
>> > Let's delay enumeration by 2 seconds.
>> >
>> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> > ---
>> >  drivers/pci/hotplug/acpiphp_glue.c |   16 +++++++++++++++-
>> >  1 file changed, 15 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>> > index 1a2b3ca..165987a 100644
>> > --- a/drivers/pci/hotplug/acpiphp_glue.c
>> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> > @@ -49,6 +49,7 @@
>> >  #include <linux/mutex.h>
>> >  #include <linux/slab.h>
>> >  #include <linux/acpi.h>
>> > +#include <linux/dmi.h>
>> >
>> >  #include "../pci.h"
>> >  #include "acpiphp.h"
>> > @@ -1327,6 +1328,19 @@ out:
>> >  static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
>> >                                     void *context)
>> >  {
>> > +   unsigned long delay = 0;
>> > +   const char *board_name;
>> > +
>> > +   board_name = dmi_get_system_info(DMI_BOARD_NAME);
>> > +   /*
>> > +    * BIOS on Intel DZ77RE-75K motherboard notifies OS about Thunderbolt
>> > +    * hotplug before devices behind Thunderbolt are ready to be
>> > +    * enumerated.
>> > +    * Let's delay enumeration by 2 seconds.
>> > +    */
>> > +   if (board_name && !strcmp(board_name, "DZ77RE-75K"))
>> > +           delay = 2 * HZ;
>>

Linus will not be happy with those kind of delay.

is there any way for kernel to retry before device is declared not there.

pcie hotplug spec: need to retry several times in 1000ms before
delcaring the devices is not present.

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=2f5d8e4ff947ad6673397083b48719cd6c59cd61

Re: [PATCH 3/3] PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K motherboard

[Linus Torvalds]

On Thu, Dec 13, 2012 at 12:25 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>
> Linus will not be happy with those kind of delay.

Indeed. And the DMI check is bogus too, since the "there can be
delays" is apparently part of the pcie hotplug spec.

So do the sane thing. Retry a few times, with increasingly long delays
(ie something like start with 10ms, then double the delay until you
hit 1s, and then just give up: end result, ~2s total wait, but 10ms
for any sane device that doesn't suck).

No DMI checks, no hacks, not insane default delays.

               Linus

Re: [PATCH 3/3] PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K motherboard

[Kirill A. Shutemov]

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

On Thu, Dec 13, 2012 at 12:30:04PM -0800, Linus Torvalds wrote:
> On Thu, Dec 13, 2012 at 12:25 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >
> > Linus will not be happy with those kind of delay.
> 
> Indeed. And the DMI check is bogus too, since the "there can be
> delays" is apparently part of the pcie hotplug spec.

It's ACPI PCI hotplug, not PCIe native hotplug. PCIe hotplug spec is not
relevant.

IIUC, in ACPI case devices should be ready to be enumerated, before you
get notification. Rafael, is it correct?

> So do the sane thing. Retry a few times, with increasingly long delays
> (ie something like start with 10ms, then double the delay until you
> hit 1s, and then just give up: end result, ~2s total wait, but 10ms
> for any sane device that doesn't suck).

PCI rescan is expensive and generate noise in dmesg. We'll end up with
tons of useless messages.

-- 
 Kirill A. Shutemov

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K motherboard

[Linus Torvalds]

On Thu, Dec 13, 2012 at 12:49 PM, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> PCI rescan is expensive and generate noise in dmesg. We'll end up with
> tons of useless messages.

So?

That's still better than adding random DMI string checks.

And christ, this is at hotplug time only. Nobody cares.

            Linus

Re: [PATCH 3/3] PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K motherboard

[Rafael J. Wysocki]

On Thursday, December 13, 2012 10:49:53 PM Kirill A. Shutemov wrote:
> On Thu, Dec 13, 2012 at 12:30:04PM -0800, Linus Torvalds wrote:
> > On Thu, Dec 13, 2012 at 12:25 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > >
> > > Linus will not be happy with those kind of delay.
> > 
> > Indeed. And the DMI check is bogus too, since the "there can be
> > delays" is apparently part of the pcie hotplug spec.
> 
> It's ACPI PCI hotplug, not PCIe native hotplug. PCIe hotplug spec is not
> relevant.
> 
> IIUC, in ACPI case devices should be ready to be enumerated, before you
> get notification. Rafael, is it correct?

Not necessarily.  The ACPI nodes will be, the device themselves are still PCI
devices. :-)

Thanks,
Rafael


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

Re: [PATCH 3/3] PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K motherboard

[Kirill A. Shutemov]

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

On Thu, Dec 13, 2012 at 12:30:04PM -0800, Linus Torvalds wrote:
> On Thu, Dec 13, 2012 at 12:25 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >
> > Linus will not be happy with those kind of delay.
> 
> Indeed. And the DMI check is bogus too, since the "there can be
> delays" is apparently part of the pcie hotplug spec.
> 
> So do the sane thing. Retry a few times, with increasingly long delays
> (ie something like start with 10ms, then double the delay until you
> hit 1s, and then just give up: end result, ~2s total wait, but 10ms
> for any sane device that doesn't suck).
> 
> No DMI checks, no hacks, not insane default delays.

I've realized that there's no strong criteria of hotplug success in ACPI
PCI Hotplug. We can't know when we should stop retrying.

In Thunderbolt case before any devices hotplugged you only see a root
port. Thunderbolt host controller is powered off and kernel can't see it.

On hotplug BIOS enables the host controller, initialize it and notify OS
about hotplug.

Normally kernel will enumerate 6 ports on Thunderbolt host controller, 2
ports on device Thunderbolt controller and target device itself. All this
for simple non-chained case. With device chaining the hierarchy is even
more complex.

On DZ77RE-75K motherboard without the workaround kernel will discover only
ports on host controller, but not device ports or device.

So kernel will find devices on broken implementation, not all of them.

Even worse: there's no way to distinguish between plug and unplug events
and kernel uses the same code path for both cases.

-- 
 Kirill A. Shutemov

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] Thunderbolt workarounds

[Bjorn Helgaas]

On Thu, Dec 13, 2012 at 12:25 PM, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> I had chance to test two PC setups with Thunderbolt: Acer Aspire S5 and
> Intel DZ77RE-75K motherboard. Unfortunately, both of them are broken in
> different ways.
>
> This patchset contains workarounds for the issues.
>
> Kirill A. Shutemov (3):
>   PCI Hotplug: workaround for Thunderbolt on Acer Aspire S5
>   PCI Hotplug: convert acpiphp_hp_work to use delayed work
>   PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K
>     motherboard
>
>  drivers/pci/hotplug/acpi_pcihp.c   |   13 +++++++++++++
>  drivers/pci/hotplug/acpiphp_glue.c |   33 +++++++++++++++++++++++++--------
>  2 files changed, 38 insertions(+), 8 deletions(-)

I'm ignoring these for now.  [1/3] has a hardcoded BIOS path that I
don't think is the right approach and [3/3] has a timeout issue to be
addressed.  [2/3] might be worthwhile by itself, but I'd rather merge
a complete solution when it's ready.

Bjorn

Re: [PATCH 0/3] Thunderbolt workarounds

[Mika Westerberg]

On Wed, Dec 26, 2012 at 05:50:42PM -0700, Bjorn Helgaas wrote:
> On Thu, Dec 13, 2012 at 12:25 PM, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >
> > I had chance to test two PC setups with Thunderbolt: Acer Aspire S5 and
> > Intel DZ77RE-75K motherboard. Unfortunately, both of them are broken in
> > different ways.
> >
> > This patchset contains workarounds for the issues.
> >
> > Kirill A. Shutemov (3):
> >   PCI Hotplug: workaround for Thunderbolt on Acer Aspire S5
> >   PCI Hotplug: convert acpiphp_hp_work to use delayed work
> >   PCI Hotplug: workaround for Thunderbolt on Intel DZ77RE-75K
> >     motherboard
> >
> >  drivers/pci/hotplug/acpi_pcihp.c   |   13 +++++++++++++
> >  drivers/pci/hotplug/acpiphp_glue.c |   33 +++++++++++++++++++++++++--------
> >  2 files changed, 38 insertions(+), 8 deletions(-)
> 
> I'm ignoring these for now.  [1/3] has a hardcoded BIOS path that I
> don't think is the right approach and [3/3] has a timeout issue to be
> addressed.  [2/3] might be worthwhile by itself, but I'd rather merge
> a complete solution when it's ready.

OK. We'll rework the series with Kirill and submit a better solution.