| Message ID | 20130704085844.GF4898@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thursday, July 04, 2013 11:58:44 AM Mika Westerberg wrote: > On Wed, Jul 03, 2013 at 11:40:42PM +0200, 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? > > Sure we can. How about something like the patch below? Well, it goes into the right (to me) direction. :-) Some comments below. > From: Mika Westerberg <mika.westerberg@linux.intel.com> > Subject: [PATCH] PCI: acpiphp: workaround for Thunderbolt on Acer Aspire S5 > > The acpiphp driver finds out whether the device is removable by checking > whether it has _RMV method directly behind it (and if it returns 1). > However, at least on Acer Aspire S5 with Thunderbolt host router has this > method placed behind a device called EPUP (endpoint upstream port?) and not > in the usual place expected by the acpiphp driver. The ASL code below shows > how this is done on that machine: > > 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) > } > } > } > } > ... > > Fix this by adding a DMI quirk for the Acer Aspire S5 machine that gives an > alternative path to the _RMV method. > > 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 | 57 ++++++++++++++++++++++++++++++++++++---- > 1 file changed, 52 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c > index 2a47e82..99fccf3 100644 > --- a/drivers/pci/hotplug/acpi_pcihp.c > +++ b/drivers/pci/hotplug/acpi_pcihp.c > @@ -33,6 +33,7 @@ > #include <linux/acpi.h> > #include <linux/pci-acpi.h> > #include <linux/slab.h> > +#include <linux/dmi.h> > > #define MY_NAME "acpi_pcihp" > > @@ -408,21 +409,67 @@ got_one: > } > EXPORT_SYMBOL(acpi_get_hp_hw_control_from_firmware); > > +/** > + * pcihp_is_removable - is the given ACPI device removable > + * @handle: ACPI handle of the device > + * > + * Try to find out whether the given ACPI device is removable by evaluating > + * its _RMV and returning the result. If we can't find the _RMV directly > + * under the device use system specific quirks to locate it. > + */ > +static bool pcihp_is_removable(acpi_handle handle) > +{ People are generally used to seeing DMI lists outside of functions. > + static const struct dmi_system_id rmv_paths[] = { > + { > + /* > + * On Acer Aspire S5 the _RMV method for the > + * Thunderbolt host router upstream port is not > + * located directly under the device but it is > + * instead placed a bit deeper in the hierarchy. > + */ > + .ident = "Acer Aspire S5", > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "Acer"), > + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire S5-391"), > + }, > + .driver_data = "HRDN.EPUP._RMV", Use .callback instead? -> > + }, > + { } > + }; > + const struct dmi_system_id *id; > + unsigned long long removable; > + acpi_status status; > + > + status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable); > + if (ACPI_SUCCESS(status)) > + return !!removable; > + > + /* Try system specific quirks */ > + id = dmi_first_match(rmv_paths); > + if (id && id->driver_data) { -> And here do if (id && id->callback) return id->callback(id); > + char path[64]; > + > + strlcpy(path, id->driver_data, sizeof(path)); > + status = acpi_evaluate_integer(handle, path, NULL, &removable); > + if (ACPI_SUCCESS(status)) > + return !!removable; > + } > + > + return false; > +} > + > 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; I'd keep the above unchanged and simply add a "platform check" function. > - return 0; > + return pcihp_is_removable(handle); > } > > /** Thanks, Rafael
On Thu, Jul 04, 2013 at 02:36:00PM +0200, Rafael J. Wysocki wrote: > On Thursday, July 04, 2013 11:58:44 AM Mika Westerberg wrote: > > On Wed, Jul 03, 2013 at 11:40:42PM +0200, 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? > > > > Sure we can. How about something like the patch below? > > Well, it goes into the right (to me) direction. :-) > > Some comments below. > > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > Subject: [PATCH] PCI: acpiphp: workaround for Thunderbolt on Acer Aspire S5 > > > > The acpiphp driver finds out whether the device is removable by checking > > whether it has _RMV method directly behind it (and if it returns 1). > > However, at least on Acer Aspire S5 with Thunderbolt host router has this > > method placed behind a device called EPUP (endpoint upstream port?) and not > > in the usual place expected by the acpiphp driver. The ASL code below shows > > how this is done on that machine: > > > > 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) > > } > > } > > } > > } > > ... > > > > Fix this by adding a DMI quirk for the Acer Aspire S5 machine that gives an > > alternative path to the _RMV method. > > > > 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 | 57 ++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 52 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c > > index 2a47e82..99fccf3 100644 > > --- a/drivers/pci/hotplug/acpi_pcihp.c > > +++ b/drivers/pci/hotplug/acpi_pcihp.c > > @@ -33,6 +33,7 @@ > > #include <linux/acpi.h> > > #include <linux/pci-acpi.h> > > #include <linux/slab.h> > > +#include <linux/dmi.h> > > > > #define MY_NAME "acpi_pcihp" > > > > @@ -408,21 +409,67 @@ got_one: > > } > > EXPORT_SYMBOL(acpi_get_hp_hw_control_from_firmware); > > > > +/** > > + * pcihp_is_removable - is the given ACPI device removable > > + * @handle: ACPI handle of the device > > + * > > + * Try to find out whether the given ACPI device is removable by evaluating > > + * its _RMV and returning the result. If we can't find the _RMV directly > > + * under the device use system specific quirks to locate it. > > + */ > > +static bool pcihp_is_removable(acpi_handle handle) > > +{ > > People are generally used to seeing DMI lists outside of functions. OK. > > + static const struct dmi_system_id rmv_paths[] = { > > + { > > + /* > > + * On Acer Aspire S5 the _RMV method for the > > + * Thunderbolt host router upstream port is not > > + * located directly under the device but it is > > + * instead placed a bit deeper in the hierarchy. > > + */ > > + .ident = "Acer Aspire S5", > > + .matches = { > > + DMI_MATCH(DMI_SYS_VENDOR, "Acer"), > > + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire S5-391"), > > + }, > > + .driver_data = "HRDN.EPUP._RMV", > > Use .callback instead? -> > > > + }, > > + { } > > + }; > > + const struct dmi_system_id *id; > > + unsigned long long removable; > > + acpi_status status; > > + > > + status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable); > > + if (ACPI_SUCCESS(status)) > > + return !!removable; > > + > > + /* Try system specific quirks */ > > + id = dmi_first_match(rmv_paths); > > + if (id && id->driver_data) { > > -> And here do > > if (id && id->callback) > return id->callback(id); There is a problem with the above that we can't pass an ACPI handle to the callback function. > > > + char path[64]; > > + > > + strlcpy(path, id->driver_data, sizeof(path)); > > + status = acpi_evaluate_integer(handle, path, NULL, &removable); > > + if (ACPI_SUCCESS(status)) > > + return !!removable; > > + } > > + > > + return false; > > +} > > + > > 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; > > I'd keep the above unchanged and simply add a "platform check" function. OK, 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
On Thursday, July 04, 2013 03:53:38 PM Mika Westerberg wrote: > On Thu, Jul 04, 2013 at 02:36:00PM +0200, Rafael J. Wysocki wrote: > > On Thursday, July 04, 2013 11:58:44 AM Mika Westerberg wrote: > > > On Wed, Jul 03, 2013 at 11:40:42PM +0200, 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? > > > > > > Sure we can. How about something like the patch below? > > > > Well, it goes into the right (to me) direction. :-) > > > > Some comments below. > > > > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > > Subject: [PATCH] PCI: acpiphp: workaround for Thunderbolt on Acer Aspire S5 > > > > > > The acpiphp driver finds out whether the device is removable by checking > > > whether it has _RMV method directly behind it (and if it returns 1). > > > However, at least on Acer Aspire S5 with Thunderbolt host router has this > > > method placed behind a device called EPUP (endpoint upstream port?) and not > > > in the usual place expected by the acpiphp driver. The ASL code below shows > > > how this is done on that machine: > > > > > > 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) > > > } > > > } > > > } > > > } > > > ... > > > > > > Fix this by adding a DMI quirk for the Acer Aspire S5 machine that gives an > > > alternative path to the _RMV method. > > > > > > 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 | 57 ++++++++++++++++++++++++++++++++++++---- > > > 1 file changed, 52 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c > > > index 2a47e82..99fccf3 100644 > > > --- a/drivers/pci/hotplug/acpi_pcihp.c > > > +++ b/drivers/pci/hotplug/acpi_pcihp.c > > > @@ -33,6 +33,7 @@ > > > #include <linux/acpi.h> > > > #include <linux/pci-acpi.h> > > > #include <linux/slab.h> > > > +#include <linux/dmi.h> > > > > > > #define MY_NAME "acpi_pcihp" > > > > > > @@ -408,21 +409,67 @@ got_one: > > > } > > > EXPORT_SYMBOL(acpi_get_hp_hw_control_from_firmware); > > > > > > +/** > > > + * pcihp_is_removable - is the given ACPI device removable > > > + * @handle: ACPI handle of the device > > > + * > > > + * Try to find out whether the given ACPI device is removable by evaluating > > > + * its _RMV and returning the result. If we can't find the _RMV directly > > > + * under the device use system specific quirks to locate it. > > > + */ > > > +static bool pcihp_is_removable(acpi_handle handle) > > > +{ > > > > People are generally used to seeing DMI lists outside of functions. > > OK. > > > > + static const struct dmi_system_id rmv_paths[] = { > > > + { > > > + /* > > > + * On Acer Aspire S5 the _RMV method for the > > > + * Thunderbolt host router upstream port is not > > > + * located directly under the device but it is > > > + * instead placed a bit deeper in the hierarchy. > > > + */ > > > + .ident = "Acer Aspire S5", > > > + .matches = { > > > + DMI_MATCH(DMI_SYS_VENDOR, "Acer"), > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire S5-391"), > > > + }, > > > + .driver_data = "HRDN.EPUP._RMV", > > > > Use .callback instead? -> > > > > > + }, > > > + { } > > > + }; > > > + const struct dmi_system_id *id; > > > + unsigned long long removable; > > > + acpi_status status; > > > + > > > + status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable); > > > + if (ACPI_SUCCESS(status)) > > > + return !!removable; > > > + > > > + /* Try system specific quirks */ > > > + id = dmi_first_match(rmv_paths); > > > + if (id && id->driver_data) { > > > > -> And here do > > > > if (id && id->callback) > > return id->callback(id); > > There is a problem with the above that we can't pass an ACPI handle to the > callback function. Ah, right. Well, you can do if (id && id->driver_data) { bool (*callback)(acpi_handle) = id->driver_data; return callback(handle); } although it's a bit hackish. > > > > > + char path[64]; > > > + > > > + strlcpy(path, id->driver_data, sizeof(path)); BTW, why didn't you want to pass id->driver_data directly here? > > > + status = acpi_evaluate_integer(handle, path, NULL, &removable); > > > + if (ACPI_SUCCESS(status)) > > > + return !!removable; > > > + } > > > + > > > + return false; > > > +} > > > + Thanks, Rafael
On Thu, Jul 04, 2013 at 03:14:58PM +0200, Rafael J. Wysocki wrote: > On Thursday, July 04, 2013 03:53:38 PM Mika Westerberg wrote: > > On Thu, Jul 04, 2013 at 02:36:00PM +0200, Rafael J. Wysocki wrote: > > > On Thursday, July 04, 2013 11:58:44 AM Mika Westerberg wrote: > > > > On Wed, Jul 03, 2013 at 11:40:42PM +0200, 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? > > > > > > > > Sure we can. How about something like the patch below? > > > > > > Well, it goes into the right (to me) direction. :-) > > > > > > Some comments below. > > > > > > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > Subject: [PATCH] PCI: acpiphp: workaround for Thunderbolt on Acer Aspire S5 > > > > > > > > The acpiphp driver finds out whether the device is removable by checking > > > > whether it has _RMV method directly behind it (and if it returns 1). > > > > However, at least on Acer Aspire S5 with Thunderbolt host router has this > > > > method placed behind a device called EPUP (endpoint upstream port?) and not > > > > in the usual place expected by the acpiphp driver. The ASL code below shows > > > > how this is done on that machine: > > > > > > > > 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) > > > > } > > > > } > > > > } > > > > } > > > > ... > > > > > > > > Fix this by adding a DMI quirk for the Acer Aspire S5 machine that gives an > > > > alternative path to the _RMV method. > > > > > > > > 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 | 57 ++++++++++++++++++++++++++++++++++++---- > > > > 1 file changed, 52 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c > > > > index 2a47e82..99fccf3 100644 > > > > --- a/drivers/pci/hotplug/acpi_pcihp.c > > > > +++ b/drivers/pci/hotplug/acpi_pcihp.c > > > > @@ -33,6 +33,7 @@ > > > > #include <linux/acpi.h> > > > > #include <linux/pci-acpi.h> > > > > #include <linux/slab.h> > > > > +#include <linux/dmi.h> > > > > > > > > #define MY_NAME "acpi_pcihp" > > > > > > > > @@ -408,21 +409,67 @@ got_one: > > > > } > > > > EXPORT_SYMBOL(acpi_get_hp_hw_control_from_firmware); > > > > > > > > +/** > > > > + * pcihp_is_removable - is the given ACPI device removable > > > > + * @handle: ACPI handle of the device > > > > + * > > > > + * Try to find out whether the given ACPI device is removable by evaluating > > > > + * its _RMV and returning the result. If we can't find the _RMV directly > > > > + * under the device use system specific quirks to locate it. > > > > + */ > > > > +static bool pcihp_is_removable(acpi_handle handle) > > > > +{ > > > > > > People are generally used to seeing DMI lists outside of functions. > > > > OK. > > > > > > + static const struct dmi_system_id rmv_paths[] = { > > > > + { > > > > + /* > > > > + * On Acer Aspire S5 the _RMV method for the > > > > + * Thunderbolt host router upstream port is not > > > > + * located directly under the device but it is > > > > + * instead placed a bit deeper in the hierarchy. > > > > + */ > > > > + .ident = "Acer Aspire S5", > > > > + .matches = { > > > > + DMI_MATCH(DMI_SYS_VENDOR, "Acer"), > > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire S5-391"), > > > > + }, > > > > + .driver_data = "HRDN.EPUP._RMV", > > > > > > Use .callback instead? -> > > > > > > > + }, > > > > + { } > > > > + }; > > > > + const struct dmi_system_id *id; > > > > + unsigned long long removable; > > > > + acpi_status status; > > > > + > > > > + status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable); > > > > + if (ACPI_SUCCESS(status)) > > > > + return !!removable; > > > > + > > > > + /* Try system specific quirks */ > > > > + id = dmi_first_match(rmv_paths); > > > > + if (id && id->driver_data) { > > > > > > -> And here do > > > > > > if (id && id->callback) > > > return id->callback(id); > > > > There is a problem with the above that we can't pass an ACPI handle to the > > callback function. > > Ah, right. > > Well, you can do > > if (id && id->driver_data) { > bool (*callback)(acpi_handle) = id->driver_data; > > return callback(handle); > } > > although it's a bit hackish. I'm thinking that passing just the path from driver_data might be simpler in this case ;-) But I'm fine with changing it to be a callback as well. > > > > > > > + char path[64]; > > > > + > > > > + strlcpy(path, id->driver_data, sizeof(path)); > > BTW, why didn't you want to pass id->driver_data directly here? acpi_evaluate_integer() takes acpi_string as parameter which is 'char *', not 'const char *'. Doing: .driver_data = "HRDN.EPUP._RMV", might place that string to a read-only area (as it is constant), if I understand C correctly. So even though I know that acpi_evaluate_interger() doesn't change the parameter, there's no guarantee that it doesn't do that in the future. > > > > > + status = acpi_evaluate_integer(handle, path, NULL, &removable); > > > > + if (ACPI_SUCCESS(status)) > > > > + return !!removable; > > > > + } > > > > + > > > > + return false; > > > > +} > > > > + > > Thanks, > Rafael > > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. -- 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
On Thursday, July 04, 2013 04:33:14 PM Mika Westerberg wrote: > On Thu, Jul 04, 2013 at 03:14:58PM +0200, Rafael J. Wysocki wrote: > > On Thursday, July 04, 2013 03:53:38 PM Mika Westerberg wrote: > > > On Thu, Jul 04, 2013 at 02:36:00PM +0200, Rafael J. Wysocki wrote: > > > > On Thursday, July 04, 2013 11:58:44 AM Mika Westerberg wrote: > > > > > On Wed, Jul 03, 2013 at 11:40:42PM +0200, 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? > > > > > > > > > > Sure we can. How about something like the patch below? > > > > > > > > Well, it goes into the right (to me) direction. :-) > > > > > > > > Some comments below. > > > > > > > > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > > Subject: [PATCH] PCI: acpiphp: workaround for Thunderbolt on Acer Aspire S5 > > > > > > > > > > The acpiphp driver finds out whether the device is removable by checking > > > > > whether it has _RMV method directly behind it (and if it returns 1). > > > > > However, at least on Acer Aspire S5 with Thunderbolt host router has this > > > > > method placed behind a device called EPUP (endpoint upstream port?) and not > > > > > in the usual place expected by the acpiphp driver. The ASL code below shows > > > > > how this is done on that machine: > > > > > > > > > > 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) > > > > > } > > > > > } > > > > > } > > > > > } > > > > > ... > > > > > > > > > > Fix this by adding a DMI quirk for the Acer Aspire S5 machine that gives an > > > > > alternative path to the _RMV method. > > > > > > > > > > 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 | 57 ++++++++++++++++++++++++++++++++++++---- > > > > > 1 file changed, 52 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c > > > > > index 2a47e82..99fccf3 100644 > > > > > --- a/drivers/pci/hotplug/acpi_pcihp.c > > > > > +++ b/drivers/pci/hotplug/acpi_pcihp.c > > > > > @@ -33,6 +33,7 @@ > > > > > #include <linux/acpi.h> > > > > > #include <linux/pci-acpi.h> > > > > > #include <linux/slab.h> > > > > > +#include <linux/dmi.h> > > > > > > > > > > #define MY_NAME "acpi_pcihp" > > > > > > > > > > @@ -408,21 +409,67 @@ got_one: > > > > > } > > > > > EXPORT_SYMBOL(acpi_get_hp_hw_control_from_firmware); > > > > > > > > > > +/** > > > > > + * pcihp_is_removable - is the given ACPI device removable > > > > > + * @handle: ACPI handle of the device > > > > > + * > > > > > + * Try to find out whether the given ACPI device is removable by evaluating > > > > > + * its _RMV and returning the result. If we can't find the _RMV directly > > > > > + * under the device use system specific quirks to locate it. > > > > > + */ > > > > > +static bool pcihp_is_removable(acpi_handle handle) > > > > > +{ > > > > > > > > People are generally used to seeing DMI lists outside of functions. > > > > > > OK. > > > > > > > > + static const struct dmi_system_id rmv_paths[] = { > > > > > + { > > > > > + /* > > > > > + * On Acer Aspire S5 the _RMV method for the > > > > > + * Thunderbolt host router upstream port is not > > > > > + * located directly under the device but it is > > > > > + * instead placed a bit deeper in the hierarchy. > > > > > + */ > > > > > + .ident = "Acer Aspire S5", > > > > > + .matches = { > > > > > + DMI_MATCH(DMI_SYS_VENDOR, "Acer"), > > > > > + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire S5-391"), > > > > > + }, > > > > > + .driver_data = "HRDN.EPUP._RMV", > > > > > > > > Use .callback instead? -> > > > > > > > > > + }, > > > > > + { } > > > > > + }; > > > > > + const struct dmi_system_id *id; > > > > > + unsigned long long removable; > > > > > + acpi_status status; > > > > > + > > > > > + status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable); > > > > > + if (ACPI_SUCCESS(status)) > > > > > + return !!removable; > > > > > + > > > > > + /* Try system specific quirks */ > > > > > + id = dmi_first_match(rmv_paths); > > > > > + if (id && id->driver_data) { > > > > > > > > -> And here do > > > > > > > > if (id && id->callback) > > > > return id->callback(id); > > > > > > There is a problem with the above that we can't pass an ACPI handle to the > > > callback function. > > > > Ah, right. > > > > Well, you can do > > > > if (id && id->driver_data) { > > bool (*callback)(acpi_handle) = id->driver_data; > > > > return callback(handle); > > } > > > > although it's a bit hackish. > > I'm thinking that passing just the path from driver_data might be simpler > in this case ;-) But I'm fine with changing it to be a callback as well. No, it probably isn't worth the effort for just one system. > > > > > > > > > + char path[64]; > > > > > + > > > > > + strlcpy(path, id->driver_data, sizeof(path)); > > > > BTW, why didn't you want to pass id->driver_data directly here? > > acpi_evaluate_integer() takes acpi_string as parameter which is 'char *', > not 'const char *'. > > Doing: > > .driver_data = "HRDN.EPUP._RMV", > > might place that string to a read-only area (as it is constant), if I > understand C correctly. So even though I know that acpi_evaluate_interger() > doesn't change the parameter, there's no guarantee that it doesn't do that > in the future. I think you can safely assume that acpi_evaluate_integer() won't try to modify path (there are too many places where string literals are passed to it). IOW, please just pass id->driver_data to it. > > > > > > > > + status = acpi_evaluate_integer(handle, path, NULL, &removable); > > > > > + if (ACPI_SUCCESS(status)) > > > > > + return !!removable; > > > > > + } > > > > > + > > > > > + return false; > > > > > +} > > > > > + Thanks, Rafael
diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c index 2a47e82..99fccf3 100644 --- a/drivers/pci/hotplug/acpi_pcihp.c +++ b/drivers/pci/hotplug/acpi_pcihp.c @@ -33,6 +33,7 @@ #include <linux/acpi.h> #include <linux/pci-acpi.h> #include <linux/slab.h> +#include <linux/dmi.h> #define MY_NAME "acpi_pcihp" @@ -408,21 +409,67 @@ got_one: } EXPORT_SYMBOL(acpi_get_hp_hw_control_from_firmware); +/** + * pcihp_is_removable - is the given ACPI device removable + * @handle: ACPI handle of the device + * + * Try to find out whether the given ACPI device is removable by evaluating + * its _RMV and returning the result. If we can't find the _RMV directly + * under the device use system specific quirks to locate it. + */ +static bool pcihp_is_removable(acpi_handle handle) +{ + static const struct dmi_system_id rmv_paths[] = { + { + /* + * On Acer Aspire S5 the _RMV method for the + * Thunderbolt host router upstream port is not + * located directly under the device but it is + * instead placed a bit deeper in the hierarchy. + */ + .ident = "Acer Aspire S5", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Acer"), + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire S5-391"), + }, + .driver_data = "HRDN.EPUP._RMV", + }, + { } + }; + const struct dmi_system_id *id; + unsigned long long removable; + acpi_status status; + + status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable); + if (ACPI_SUCCESS(status)) + return !!removable; + + /* Try system specific quirks */ + id = dmi_first_match(rmv_paths); + if (id && id->driver_data) { + char path[64]; + + strlcpy(path, id->driver_data, sizeof(path)); + status = acpi_evaluate_integer(handle, path, NULL, &removable); + if (ACPI_SUCCESS(status)) + return !!removable; + } + + return false; +} + 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); } /**