Patchwork [v2,6/8] PCI: acpiphp: workaround for Thunderbolt on Acer Aspire S5

login
register
mail settings
Submitter Mika Westerberg
Date July 3, 2013, 2:04 p.m.
Message ID <1372860295-8306-7-git-send-email-mika.westerberg@linux.intel.com>
Download mbox | patch
Permalink /patch/256645/
State Not Applicable
Headers show

Comments

Mika Westerberg - July 3, 2013, 2:04 p.m.
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>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/acpi_pcihp.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Rafael J. Wysocki - July 3, 2013, 9:40 p.m.
On Wednesday, July 03, 2013 05:04:53 PM Mika Westerberg 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>
> Signed-off-by: Mika Westerberg <mika.westerberg@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);

Well, calling stuff like this directly from a general function is kind of ugly.

Can we use something like a quirk instead?  A DMI check or something?

> +	if (ACPI_SUCCESS(status) && removable)
> +		return 1;
> +
>  	return 0;
>  }

Rafael
Bjorn Helgaas - July 3, 2013, 9:45 p.m.
On Wed, Jul 3, 2013 at 8:04 AM, Mika Westerberg
<mika.westerberg@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>
> Signed-off-by: Mika Westerberg <mika.westerberg@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.

s/imeplementation/implementation/

If you're casting aspersions on Acer for having an incorrect ACPI
implementation, you should probably provide a spec reference.  I think
it's likely that the Acer implementation is actually correct per the
spec, but Linux just isn't smart enough to handle it.

> +        */
> +       status = acpi_evaluate_integer(handle, "HRDN.EPUP._RMV", NULL,
> +                       &removable);
> +       if (ACPI_SUCCESS(status) && removable)
> +               return 1;
> +
>         return 0;
>  }
>
> --
> 1.8.3.2
>
--
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
Rafael J. Wysocki - July 3, 2013, 9:58 p.m.
On Wednesday, July 03, 2013 03:45:50 PM Bjorn Helgaas wrote:
> On Wed, Jul 3, 2013 at 8:04 AM, Mika Westerberg
> <mika.westerberg@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>
> > Signed-off-by: Mika Westerberg <mika.westerberg@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.
> 
> s/imeplementation/implementation/
> 
> If you're casting aspersions on Acer for having an incorrect ACPI
> implementation, you should probably provide a spec reference.  I think
> it's likely that the Acer implementation is actually correct per the
> spec, but Linux just isn't smart enough to handle it.

Agreed.

> > +        */
> > +       status = acpi_evaluate_integer(handle, "HRDN.EPUP._RMV", NULL,
> > +                       &removable);
> > +       if (ACPI_SUCCESS(status) && removable)
> > +               return 1;
> > +
> >         return 0;
> >  }
> >
> > --
> > 1.8.3.2
> >
Robert Hancock - July 19, 2013, 3:57 a.m.
On 07/03/2013 03:40 PM, Rafael J. Wysocki wrote:
> On Wednesday, July 03, 2013 05:04:53 PM Mika Westerberg 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>
>> Signed-off-by: Mika Westerberg <mika.westerberg@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);
>
> Well, calling stuff like this directly from a general function is kind of ugly.
>
> Can we use something like a quirk instead?  A DMI check or something?

Presumably this device functions under Windows so clearly Windows is 
capable of dealing with this case, so we should too.

There are way too many of these silly DMI checks in the kernel - we 
should be way more hesitant to add more of them. They're almost 
guaranteed to be incomplete. I would say they should be avoided whenever 
possible unless there's some reason why a general workaround can't be used.

>
>> +	if (ACPI_SUCCESS(status) && removable)
>> +		return 1;
>> +
>>   	return 0;
>>   }
>
> Rafael
>
>

--
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
Rafael J. Wysocki - July 19, 2013, 12:18 p.m.
On Thursday, July 18, 2013 09:57:23 PM Robert Hancock wrote:
> On 07/03/2013 03:40 PM, Rafael J. Wysocki wrote:
> > On Wednesday, July 03, 2013 05:04:53 PM Mika Westerberg 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>
> >> Signed-off-by: Mika Westerberg <mika.westerberg@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);
> >
> > Well, calling stuff like this directly from a general function is kind of ugly.
> >
> > Can we use something like a quirk instead?  A DMI check or something?
> 
> Presumably this device functions under Windows so clearly Windows is 
> capable of dealing with this case, so we should too.
> 
> There are way too many of these silly DMI checks in the kernel - we 
> should be way more hesitant to add more of them. They're almost 
> guaranteed to be incomplete. I would say they should be avoided whenever 
> possible unless there's some reason why a general workaround can't be used.

This horse is already dead. :-)

Please check the series I posted the day before yesterday.

Thanks,
Rafael

Patch

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;
 }