diff mbox

[5/6] PCI: acpiphp: look _RMV method a bit deeper in the hierarhcy

Message ID 1372177330-28013-6-git-send-email-mika.westerberg@linux.intel.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Mika Westerberg June 25, 2013, 4:22 p.m. UTC
The acpiphp driver finds out whether the device is hotpluggable by checking
whether it has _RMV method behind it (and if it returns 1). However, at
least Acer Aspire S5 with Thunderbolt host router has this method placed
behind device called EPUP (endpoint upstream port?) and not directly behind
the root port as can be seen from the ASL code below:

Device (RP05)
{
	...
	Device (HRUP)
	{
		Name (_ADR, Zero)
		Name (_PRW, Package (0x02)
		{
			0x09,
			0x04
		})
		Device (HRDN)
		{
			Name (_ADR, 0x00040000)
			Name (_PRW, Package (0x02)
			{
				0x09,
				0x04
			})
			Device (EPUP)
			{
				Name (_ADR, Zero)
				Method (_RMV, 0, NotSerialized)
				{
					Return (One)
				}
			}
		}
	}

If we want to support such machines we must look for the _RMV method a bit
deeper in the hierarchy. Fix this by changing pcihp_is_ejectable() to check
few more devices down from the root port.

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 | 53 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 5 deletions(-)

Comments

Andy Shevchenko June 25, 2013, 6:15 p.m. UTC | #1
On Tue, Jun 25, 2013 at 7:22 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> The acpiphp driver finds out whether the device is hotpluggable by checking
> whether it has _RMV method behind it (and if it returns 1). However, at
> least Acer Aspire S5 with Thunderbolt host router has this method placed
> behind device called EPUP (endpoint upstream port?) and not directly behind
> the root port as can be seen from the ASL code below:

[]


> +static acpi_status pcihp_evaluate_rmv(acpi_handle handle, u32 lvl,
> +                                     void *context, void **return_not_used)
> +{
> +       unsigned long long *removable = context;
> +       unsigned long long value;
> +       acpi_status status;
> +
> +       status = acpi_evaluate_integer(handle, "_RMV", NULL, &value);
> +       if (ACPI_SUCCESS(status) && value) {

So, there is a chance the caller gets back uninitialized *context.
Let's assume that is by design.

> +               *removable = value;
> +               return AE_CTRL_TERMINATE;
> +       }
> +       return AE_OK;
> +}


> +static bool pcihp_is_removable(acpi_handle handle, size_t depth)
> +{
> +       unsigned long long removable = 0;
> +       acpi_status status;
> +
> +       status = pcihp_evaluate_rmv(handle, 0, &removable, NULL);
> +       if ((status == AE_CTRL_TERMINATE) && removable)

Here you already have removable not equal zero.

Internal braces could be removed, but it's up top you.

> +               return true;
> +
> +       acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, depth, pcihp_evaluate_rmv,
> +                           NULL, &removable, NULL);
> +       return !!removable;
> +}


--
With Best Regards,
Andy Shevchenko
--
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
Mika Westerberg June 25, 2013, 6:31 p.m. UTC | #2
On Tue, Jun 25, 2013 at 09:15:47PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 25, 2013 at 7:22 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > The acpiphp driver finds out whether the device is hotpluggable by checking
> > whether it has _RMV method behind it (and if it returns 1). However, at
> > least Acer Aspire S5 with Thunderbolt host router has this method placed
> > behind device called EPUP (endpoint upstream port?) and not directly behind
> > the root port as can be seen from the ASL code below:
> 
> []
> 
> 
> > +static acpi_status pcihp_evaluate_rmv(acpi_handle handle, u32 lvl,
> > +                                     void *context, void **return_not_used)
> > +{
> > +       unsigned long long *removable = context;
> > +       unsigned long long value;
> > +       acpi_status status;
> > +
> > +       status = acpi_evaluate_integer(handle, "_RMV", NULL, &value);
> > +       if (ACPI_SUCCESS(status) && value) {
> 
> So, there is a chance the caller gets back uninitialized *context.
> Let's assume that is by design.
> 
> > +               *removable = value;
> > +               return AE_CTRL_TERMINATE;
> > +       }
> > +       return AE_OK;
> > +}
> 
> 
> > +static bool pcihp_is_removable(acpi_handle handle, size_t depth)
> > +{
> > +       unsigned long long removable = 0;
> > +       acpi_status status;
> > +
> > +       status = pcihp_evaluate_rmv(handle, 0, &removable, NULL);
> > +       if ((status == AE_CTRL_TERMINATE) && removable)
> 
> Here you already have removable not equal zero.

Hmm, removable is initialized to zero just few lines above... Did I miss
something obvious?

> Internal braces could be removed, but it's up top you.

I'll remove them in the next revision.

Thanks.
--
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
Andy Shevchenko June 25, 2013, 6:31 p.m. UTC | #3
On Tue, Jun 25, 2013 at 9:31 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Jun 25, 2013 at 09:15:47PM +0300, Andy Shevchenko wrote:
>> On Tue, Jun 25, 2013 at 7:22 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:

[]

>> > +static acpi_status pcihp_evaluate_rmv(acpi_handle handle, u32 lvl,
>> > +                                     void *context, void **return_not_used)
>> > +{
>> > +       unsigned long long *removable = context;
>> > +       unsigned long long value;
>> > +       acpi_status status;
>> > +
>> > +       status = acpi_evaluate_integer(handle, "_RMV", NULL, &value);
>> > +       if (ACPI_SUCCESS(status) && value) {
>>
>> So, there is a chance the caller gets back uninitialized *context.
>> Let's assume that is by design.
>>
>> > +               *removable = value;
>> > +               return AE_CTRL_TERMINATE;
>> > +       }
>> > +       return AE_OK;
>> > +}
>>
>>
>> > +static bool pcihp_is_removable(acpi_handle handle, size_t depth)
>> > +{
>> > +       unsigned long long removable = 0;
>> > +       acpi_status status;
>> > +
>> > +       status = pcihp_evaluate_rmv(handle, 0, &removable, NULL);
>> > +       if ((status == AE_CTRL_TERMINATE) && removable)
>>
>> Here you already have removable not equal zero.
>
> Hmm, removable is initialized to zero just few lines above... Did I miss
> something obvious?

Yes, that's correct, however, you already did this check when you call
evaluate_rmv. Thus, second check '&& removable' is not needed.

--
With Best Regards,
Andy Shevchenko
--
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
Mika Westerberg June 25, 2013, 6:51 p.m. UTC | #4
On Tue, Jun 25, 2013 at 09:31:52PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 25, 2013 at 9:31 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Jun 25, 2013 at 09:15:47PM +0300, Andy Shevchenko wrote:
> >> On Tue, Jun 25, 2013 at 7:22 PM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> 
> []
> 
> >> > +static acpi_status pcihp_evaluate_rmv(acpi_handle handle, u32 lvl,
> >> > +                                     void *context, void **return_not_used)
> >> > +{
> >> > +       unsigned long long *removable = context;
> >> > +       unsigned long long value;
> >> > +       acpi_status status;
> >> > +
> >> > +       status = acpi_evaluate_integer(handle, "_RMV", NULL, &value);
> >> > +       if (ACPI_SUCCESS(status) && value) {
> >>
> >> So, there is a chance the caller gets back uninitialized *context.
> >> Let's assume that is by design.
> >>
> >> > +               *removable = value;
> >> > +               return AE_CTRL_TERMINATE;
> >> > +       }
> >> > +       return AE_OK;
> >> > +}
> >>
> >>
> >> > +static bool pcihp_is_removable(acpi_handle handle, size_t depth)
> >> > +{
> >> > +       unsigned long long removable = 0;
> >> > +       acpi_status status;
> >> > +
> >> > +       status = pcihp_evaluate_rmv(handle, 0, &removable, NULL);
> >> > +       if ((status == AE_CTRL_TERMINATE) && removable)
> >>
> >> Here you already have removable not equal zero.
> >
> > Hmm, removable is initialized to zero just few lines above... Did I miss
> > something obvious?
> 
> Yes, that's correct, however, you already did this check when you call
> evaluate_rmv. Thus, second check '&& removable' is not needed.

Ah, right. Got it now :-)

I think it is better to remove the first value check from the
pcihp_evaluate_rmv() and keep the check here.

I'll fix that in the next revision as well.
--
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
Andy Shevchenko June 25, 2013, 7:30 p.m. UTC | #5
On Tue, Jun 25, 2013 at 9:51 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Jun 25, 2013 at 09:31:52PM +0300, Andy Shevchenko wrote:

> I think it is better to remove the first value check from the
> pcihp_evaluate_rmv() and keep the check here.
>
> I'll fix that in the next revision as well.

In that case you may remove the removable assignment as well.

--
With Best Regards,
Andy Shevchenko
--
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
Kirill A . Shutemov July 2, 2013, 10:44 a.m. UTC | #6
Mika Westerberg wrote:
> The acpiphp driver finds out whether the device is hotpluggable by checking
> whether it has _RMV method behind it (and if it returns 1). However, at
> least Acer Aspire S5 with Thunderbolt host router has this method placed
> behind device called EPUP (endpoint upstream port?) and not directly behind
> the root port as can be seen from the ASL code below:
> 
> Device (RP05)
> {
> 	...
> 	Device (HRUP)
> 	{
> 		Name (_ADR, Zero)
> 		Name (_PRW, Package (0x02)
> 		{
> 			0x09,
> 			0x04
> 		})
> 		Device (HRDN)
> 		{
> 			Name (_ADR, 0x00040000)
> 			Name (_PRW, Package (0x02)
> 			{
> 				0x09,
> 				0x04
> 			})
> 			Device (EPUP)
> 			{
> 				Name (_ADR, Zero)
> 				Method (_RMV, 0, NotSerialized)
> 				{
> 					Return (One)
> 				}
> 			}
> 		}
> 	}
> 
> If we want to support such machines we must look for the _RMV method a bit
> deeper in the hierarchy. Fix this by changing pcihp_is_ejectable() to check
> few more devices down from the root port.

We found that this approach is broken. We've got false positive: host
bridge itself was detected as hotplugable slot %) I think it's not
acceptable.

Mika has tried few more approaches, but we haven't found anything better
then hardcoded path like in original workaround patch[1]. It's not generic
at all, but safe from false positives.

Any thoughts?

[1] http://article.gmane.org/gmane.linux.kernel.pci/19102
Bjorn Helgaas July 2, 2013, 5:09 p.m. UTC | #7
On Tue, Jul 2, 2013 at 4:44 AM, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
> Mika Westerberg wrote:
>> The acpiphp driver finds out whether the device is hotpluggable by checking
>> whether it has _RMV method behind it (and if it returns 1). However, at
>> least Acer Aspire S5 with Thunderbolt host router has this method placed
>> behind device called EPUP (endpoint upstream port?) and not directly behind
>> the root port as can be seen from the ASL code below:
>>
>> Device (RP05)
>> {
>>       ...
>>       Device (HRUP)
>>       {
>>               Name (_ADR, Zero)
>>               Name (_PRW, Package (0x02)
>>               {
>>                       0x09,
>>                       0x04
>>               })
>>               Device (HRDN)
>>               {
>>                       Name (_ADR, 0x00040000)
>>                       Name (_PRW, Package (0x02)
>>                       {
>>                               0x09,
>>                               0x04
>>                       })
>>                       Device (EPUP)
>>                       {
>>                               Name (_ADR, Zero)
>>                               Method (_RMV, 0, NotSerialized)
>>                               {
>>                                       Return (One)
>>                               }
>>                       }
>>               }
>>       }
>>
>> If we want to support such machines we must look for the _RMV method a bit
>> deeper in the hierarchy. Fix this by changing pcihp_is_ejectable() to check
>> few more devices down from the root port.
>
> We found that this approach is broken. We've got false positive: host
> bridge itself was detected as hotplugable slot %) I think it's not
> acceptable.
>
> Mika has tried few more approaches, but we haven't found anything better
> then hardcoded path like in original workaround patch[1]. It's not generic
> at all, but safe from false positives.
>
> Any thoughts?
>
> [1] http://article.gmane.org/gmane.linux.kernel.pci/19102

The changelog of this patch ([1]) says "Correct ACPI PCI hotplug
implementation should have _RMV method in a PCI slot (device under pci
bridge). In Acer Aspire S5 case we have it deeper in hierarchy ..."

This is exactly what I mean about acpiphp being brittle because of the
assumptions it makes about the ACPI namespace.  Is there actually
something in the spec that requires the _RMV method to be where
pcihp_is_ejectable() expects it?

I'm not 100% dead-set against merging the workaround with hard-coded
path, but I still don't think it's a good idea.  It "fixes" it for one
particular machine, but there will likely be other machines that
require similar fixes in the future.  It makes it harder for somebody
to clean up the design later, because that person won't have an Aspire
S5 to test.  It makes it less likely that somebody *will* clean it up
later, because "everything is already working."

That's why my preference (given infinite resources) would be to rework
acpiphp now, while people are interested and can test it.  I'm sure
this would be a major redesign of acpiphp and its interaction with
pciehp, but I think it's something we're going to have to do at some
point.

Bjorn
--
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
Kirill A . Shutemov July 2, 2013, 5:45 p.m. UTC | #8
Bjorn Helgaas wrote:
> On Tue, Jul 2, 2013 at 4:44 AM, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> > Mika Westerberg wrote:
> >> The acpiphp driver finds out whether the device is hotpluggable by checking
> >> whether it has _RMV method behind it (and if it returns 1). However, at
> >> least Acer Aspire S5 with Thunderbolt host router has this method placed
> >> behind device called EPUP (endpoint upstream port?) and not directly behind
> >> the root port as can be seen from the ASL code below:
> >>
> >> Device (RP05)
> >> {
> >>       ...
> >>       Device (HRUP)
> >>       {
> >>               Name (_ADR, Zero)
> >>               Name (_PRW, Package (0x02)
> >>               {
> >>                       0x09,
> >>                       0x04
> >>               })
> >>               Device (HRDN)
> >>               {
> >>                       Name (_ADR, 0x00040000)
> >>                       Name (_PRW, Package (0x02)
> >>                       {
> >>                               0x09,
> >>                               0x04
> >>                       })
> >>                       Device (EPUP)
> >>                       {
> >>                               Name (_ADR, Zero)
> >>                               Method (_RMV, 0, NotSerialized)
> >>                               {
> >>                                       Return (One)
> >>                               }
> >>                       }
> >>               }
> >>       }
> >>
> >> If we want to support such machines we must look for the _RMV method a bit
> >> deeper in the hierarchy. Fix this by changing pcihp_is_ejectable() to check
> >> few more devices down from the root port.
> >
> > We found that this approach is broken. We've got false positive: host
> > bridge itself was detected as hotplugable slot %) I think it's not
> > acceptable.
> >
> > Mika has tried few more approaches, but we haven't found anything better
> > then hardcoded path like in original workaround patch[1]. It's not generic
> > at all, but safe from false positives.
> >
> > Any thoughts?
> >
> > [1] http://article.gmane.org/gmane.linux.kernel.pci/19102
> 
> The changelog of this patch ([1]) says "Correct ACPI PCI hotplug
> implementation should have _RMV method in a PCI slot (device under pci
> bridge). In Acer Aspire S5 case we have it deeper in hierarchy ..."
> 
> This is exactly what I mean about acpiphp being brittle because of the
> assumptions it makes about the ACPI namespace.  Is there actually
> something in the spec that requires the _RMV method to be where
> pcihp_is_ejectable() expects it?

Spec says that _RMV indicates the device which can be removed.
Rest, I believe, are assumptions: if the device is removable, then parent
must be a hotpluggable slot.

It looks reasonable, but doesn't work in this case. And it's not obvious
how we can generalize the assumption to cover the case.

> I'm not 100% dead-set against merging the workaround with hard-coded
> path, but I still don't think it's a good idea.  It "fixes" it for one
> particular machine, but there will likely be other machines that
> require similar fixes in the future.  It makes it harder for somebody
> to clean up the design later, because that person won't have an Aspire
> S5 to test.  It makes it less likely that somebody *will* clean it up
> later, because "everything is already working."
> 
> That's why my preference (given infinite resources) would be to rework
> acpiphp now, while people are interested and can test it.  I'm sure
> this would be a major redesign of acpiphp and its interaction with
> pciehp, but I think it's something we're going to have to do at some
> point.

Basically, we run out of ideas. Any input is welcome! :)

For now we can post new version of patchset without the workaround and
come back later with workaround (or proper solution if we'll find any).

Does it work for you? Or you prefer everything in one patchset?
Rafael Wysocki July 2, 2013, 8:40 p.m. UTC | #9
On Tuesday, July 02, 2013 11:09:19 AM Bjorn Helgaas wrote:
> On Tue, Jul 2, 2013 at 4:44 AM, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> > Mika Westerberg wrote:
> >> The acpiphp driver finds out whether the device is hotpluggable by checking
> >> whether it has _RMV method behind it (and if it returns 1). However, at
> >> least Acer Aspire S5 with Thunderbolt host router has this method placed
> >> behind device called EPUP (endpoint upstream port?) and not directly behind
> >> the root port as can be seen from the ASL code below:
> >>
> >> Device (RP05)
> >> {
> >>       ...
> >>       Device (HRUP)
> >>       {
> >>               Name (_ADR, Zero)
> >>               Name (_PRW, Package (0x02)
> >>               {
> >>                       0x09,
> >>                       0x04
> >>               })
> >>               Device (HRDN)
> >>               {
> >>                       Name (_ADR, 0x00040000)
> >>                       Name (_PRW, Package (0x02)
> >>                       {
> >>                               0x09,
> >>                               0x04
> >>                       })
> >>                       Device (EPUP)
> >>                       {
> >>                               Name (_ADR, Zero)
> >>                               Method (_RMV, 0, NotSerialized)
> >>                               {
> >>                                       Return (One)
> >>                               }
> >>                       }
> >>               }
> >>       }
> >>
> >> If we want to support such machines we must look for the _RMV method a bit
> >> deeper in the hierarchy. Fix this by changing pcihp_is_ejectable() to check
> >> few more devices down from the root port.
> >
> > We found that this approach is broken. We've got false positive: host
> > bridge itself was detected as hotplugable slot %) I think it's not
> > acceptable.
> >
> > Mika has tried few more approaches, but we haven't found anything better
> > then hardcoded path like in original workaround patch[1]. It's not generic
> > at all, but safe from false positives.
> >
> > Any thoughts?
> >
> > [1] http://article.gmane.org/gmane.linux.kernel.pci/19102
> 
> The changelog of this patch ([1]) says "Correct ACPI PCI hotplug
> implementation should have _RMV method in a PCI slot (device under pci
> bridge). In Acer Aspire S5 case we have it deeper in hierarchy ..."
> 
> This is exactly what I mean about acpiphp being brittle because of the
> assumptions it makes about the ACPI namespace.  Is there actually
> something in the spec that requires the _RMV method to be where
> pcihp_is_ejectable() expects it?
> 
> I'm not 100% dead-set against merging the workaround with hard-coded
> path, but I still don't think it's a good idea.  It "fixes" it for one
> particular machine, but there will likely be other machines that
> require similar fixes in the future.  It makes it harder for somebody
> to clean up the design later, because that person won't have an Aspire
> S5 to test.  It makes it less likely that somebody *will* clean it up
> later, because "everything is already working."
> 
> That's why my preference (given infinite resources) would be to rework
> acpiphp now, while people are interested and can test it.

Well, I have a plan to do just that, but quite frankly I'd prefer to make
things physically work first and *then* to make them nicer.

> I'm sure this would be a major redesign of acpiphp and its interaction with
> pciehp, but I think it's something we're going to have to do at some
> point.

Yes, we need to do that pretty much as soon as reasonably possible, but let's
not hold back changes allowing people to use their hardware with Linux because
of that.  We very well may need to spend a couple of releases on it.

Thanks,
Rafael
Rafael Wysocki July 2, 2013, 8:48 p.m. UTC | #10
On Tuesday, July 02, 2013 08:45:42 PM Kirill A. Shutemov wrote:
> Bjorn Helgaas wrote:
> > On Tue, Jul 2, 2013 at 4:44 AM, Kirill A. Shutemov
> > <kirill.shutemov@linux.intel.com> wrote:
> > > Mika Westerberg wrote:
> > >> The acpiphp driver finds out whether the device is hotpluggable by checking
> > >> whether it has _RMV method behind it (and if it returns 1). However, at
> > >> least Acer Aspire S5 with Thunderbolt host router has this method placed
> > >> behind device called EPUP (endpoint upstream port?) and not directly behind
> > >> the root port as can be seen from the ASL code below:
> > >>
> > >> Device (RP05)
> > >> {
> > >>       ...
> > >>       Device (HRUP)
> > >>       {
> > >>               Name (_ADR, Zero)
> > >>               Name (_PRW, Package (0x02)
> > >>               {
> > >>                       0x09,
> > >>                       0x04
> > >>               })
> > >>               Device (HRDN)
> > >>               {
> > >>                       Name (_ADR, 0x00040000)
> > >>                       Name (_PRW, Package (0x02)
> > >>                       {
> > >>                               0x09,
> > >>                               0x04
> > >>                       })
> > >>                       Device (EPUP)
> > >>                       {
> > >>                               Name (_ADR, Zero)
> > >>                               Method (_RMV, 0, NotSerialized)
> > >>                               {
> > >>                                       Return (One)
> > >>                               }
> > >>                       }
> > >>               }
> > >>       }
> > >>
> > >> If we want to support such machines we must look for the _RMV method a bit
> > >> deeper in the hierarchy. Fix this by changing pcihp_is_ejectable() to check
> > >> few more devices down from the root port.
> > >
> > > We found that this approach is broken. We've got false positive: host
> > > bridge itself was detected as hotplugable slot %) I think it's not
> > > acceptable.
> > >
> > > Mika has tried few more approaches, but we haven't found anything better
> > > then hardcoded path like in original workaround patch[1]. It's not generic
> > > at all, but safe from false positives.
> > >
> > > Any thoughts?
> > >
> > > [1] http://article.gmane.org/gmane.linux.kernel.pci/19102
> > 
> > The changelog of this patch ([1]) says "Correct ACPI PCI hotplug
> > implementation should have _RMV method in a PCI slot (device under pci
> > bridge). In Acer Aspire S5 case we have it deeper in hierarchy ..."
> > 
> > This is exactly what I mean about acpiphp being brittle because of the
> > assumptions it makes about the ACPI namespace.  Is there actually
> > something in the spec that requires the _RMV method to be where
> > pcihp_is_ejectable() expects it?
> 
> Spec says that _RMV indicates the device which can be removed.
> Rest, I believe, are assumptions: if the device is removable, then parent
> must be a hotpluggable slot.

Where "hotpluggable slot" is a very weakly defined entity. :-)

_RMV means: this device only supports surprise-style removal.  Moreover, it
is mandatory for removable devices without _LCK or _EJx.  _RMV on one device
doesn't actually mean anything for other devices.

> It looks reasonable, but doesn't work in this case. And it's not obvious
> how we can generalize the assumption to cover the case.
> 
> > I'm not 100% dead-set against merging the workaround with hard-coded
> > path, but I still don't think it's a good idea.  It "fixes" it for one
> > particular machine, but there will likely be other machines that
> > require similar fixes in the future.  It makes it harder for somebody
> > to clean up the design later, because that person won't have an Aspire
> > S5 to test.  It makes it less likely that somebody *will* clean it up
> > later, because "everything is already working."
> > 
> > That's why my preference (given infinite resources) would be to rework
> > acpiphp now, while people are interested and can test it.  I'm sure
> > this would be a major redesign of acpiphp and its interaction with
> > pciehp, but I think it's something we're going to have to do at some
> > point.
> 
> Basically, we run out of ideas. Any input is welcome! :)
> 
> For now we can post new version of patchset without the workaround and
> come back later with workaround (or proper solution if we'll find any).
> 
> Does it work for you? Or you prefer everything in one patchset?

Please post the current version of the patchset with as much of the feedback
taken into account as possible without breaking functionality.

Thanks,
Rafael
diff mbox

Patch

diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
index 2a47e82..2760954 100644
--- a/drivers/pci/hotplug/acpi_pcihp.c
+++ b/drivers/pci/hotplug/acpi_pcihp.c
@@ -408,21 +408,64 @@  got_one:
 }
 EXPORT_SYMBOL(acpi_get_hp_hw_control_from_firmware);
 
+static acpi_status pcihp_evaluate_rmv(acpi_handle handle, u32 lvl,
+				      void *context, void **return_not_used)
+{
+	unsigned long long *removable = context;
+	unsigned long long value;
+	acpi_status status;
+
+	status = acpi_evaluate_integer(handle, "_RMV", NULL, &value);
+	if (ACPI_SUCCESS(status) && value) {
+		*removable = value;
+		return AE_CTRL_TERMINATE;
+	}
+	return AE_OK;
+}
+
+/*
+ * pcihp_is_removable - check if this device is removable
+ * @handle: ACPI handle of the device
+ * @depth: how deep in the hierarchy we look for the _RMV
+ *
+ * Look for _RMV method behind the device. @depth specifies how deep in the
+ * hierarchy we search.
+ *
+ * Returns %true if the device is removable, %false otherwise.
+ */
+static bool pcihp_is_removable(acpi_handle handle, size_t depth)
+{
+	unsigned long long removable = 0;
+	acpi_status status;
+
+	status = pcihp_evaluate_rmv(handle, 0, &removable, NULL);
+	if ((status == AE_CTRL_TERMINATE) && removable)
+		return true;
+
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, depth, pcihp_evaluate_rmv,
+			    NULL, &removable, NULL);
+	return !!removable;
+}
+
+/*
+ * Some BIOSes, like the one in Acer Aspire S5 places the _RMV method a bit
+ * deeper in the hierarhcy. So check at least 3 levels behind this device.
+ */
+#define PCIHP_RMV_MAX_DEPTH	3
+
 static int pcihp_is_ejectable(acpi_handle handle)
 {
 	acpi_status status;
 	acpi_handle tmp;
-	unsigned long long removable;
+
 	status = acpi_get_handle(handle, "_ADR", &tmp);
 	if (ACPI_FAILURE(status))
 		return 0;
 	status = acpi_get_handle(handle, "_EJ0", &tmp);
 	if (ACPI_SUCCESS(status))
 		return 1;
-	status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable);
-	if (ACPI_SUCCESS(status) && removable)
-		return 1;
-	return 0;
+
+	return pcihp_is_removable(handle, PCIHP_RMV_MAX_DEPTH);
 }
 
 /**