[PATCH v2 0/8] Thunderbolt workarounds

[PATCH v2 0/8] Thunderbolt workarounds

[Mika Westerberg]

Here's the next revision of the workarounds for Thunderbolt machines. The
previous revision can be found here:

  https://lkml.org/lkml/2013/6/25/400

The series fixes problems seen in ACPI hotplug driver and resource
re-allocation issue in pcibios_resource_survey_bus(). With these applied we
have been able to connect Thunderbolt chain as long as:

  PC +--+ eSata Hub #0 +--+ eSata Hub #1 +--+ Apple Thunderbolt display +--+ Apple ethernet dongle

That's all Thunderbolt devices we have for testing. This results the
following PCIe tree (output of lspci -tv):

  +-1c.4-[06-80]----00.0-[07-80]--+-00.0-[08]--
  |                               +-03.0-[09-7d]----00.0-[0a-7d]--+-03.0-[0b]----00.0  Marvell Technology Group Ltd. Device 9182
  |                               |                               +-04.0-[0c-7c]----00.0-[0d-7c]--+-03.0-[0e]----00.0  Marvell Technology Group Ltd. Device 9182
  |                               |                               |                               +-04.0-[0f-7b]----00.0-[10-7b]--+-00.0-[11-13]----00.0-[12-13]----03.0-[13]--+-00.0  Pericom Semiconductor Device 400e
  |                               |                               |                               |                               |                                            +-00.1  Pericom Semiconductor Device 400e
  |                               |                               |                               |                               |                                            \-00.2  Pericom Semiconductor Device 400f
  |                               |                               |                               |                               +-01.0-[14]----00.0  Broadcom Corporation NetXtreme BCM57761 Gigabit Ethernet PCIe
  |                               |                               |                               |                               +-02.0-[15]----00.0  Agere Systems FW643 PCI Express 1394b Controller (PHY/Link)
  |                               |                               |                               |                               +-03.0-[16]--
  |                               |                               |                               |                               +-04.0-[17-7a]----00.0-[18-7a]----00.0-[19]----00.0  Broadcom Corporation Device 1682
  |                               |                               |                               |                               \-05.0-[7b]--
  |                               |                               |                               \-05.0-[7c]--
  |                               |                               \-05.0-[7d]--
  |                               +-04.0-[7e]--
  |                               +-05.0-[7f]--
  |                               \-06.0-[80]--


There are still problems with individual device drivers not handling the
surprise hotplug nature of Thunderbolt devices correctly but that can be
handled per driver later on.

This has been tested on Acer Aspire S5 ultrabook and in addition to that on
Intel DZ77RE-75K desktop.

Changes to the previous version:
  [1/8] A new patch that fixes pcibios_resource_survey_bus() to handle
        re-allocation of resources properly. This patch should be first one
	in the series because the following patches then enable things that
	depend on this one.
  [3/8] Dropped !dev->subordinate->is_added check. This is handled in [1/6].
  [4/8] Removed 'continue' and call directly pci_trim_stale_devices() and
        fold the pci_trim_stale_devices() to this patch.
  [5/8] A new patch that removes SLOT_ENABLED flag completely from the
        acpiphp driver. This is a separate patch to make reverting easy
	in case there appears regressions but still make Thunderbolt
	working.
  [6/8] Use the original Kirill's workaround for Acer Aspire S5.
  [7,8/8] New cleanups for acpiphp that were found during development.

The series applies on top of v3.10.

Kirill A. Shutemov (4):
  PCI: acpiphp: do not check for SLOT_ENABLED in enable_device()
  PCI: acpiphp: enable_device(): rescan even if no new devices on slot
  PCI: acpiphp: check for new devices on enabled host
  PCI: acpiphp: workaround for Thunderbolt on Acer Aspire S5

Mika Westerberg (4):
  x86/PCI: prevent re-allocation of already existing bridge and ROM
    resources
  PCI: acpiphp: kill SLOT_ENABLED in favor of always re-enumerating the
    devices
  PCI: acpiphp: get rid of unused constants in acpiphp.h
  PCI: acpiphp: sanitize acpiphp_get_[latch|adapter]_status()

 arch/x86/pci/i386.c                |  4 ++
 drivers/pci/hotplug/acpi_pcihp.c   | 13 +++++
 drivers/pci/hotplug/acpiphp.h      |  5 --
 drivers/pci/hotplug/acpiphp_glue.c | 97 ++++++++++++--------------------------
 drivers/pci/remove.c               | 20 ++++++++
 include/linux/pci.h                |  1 +
 6 files changed, 69 insertions(+), 71 deletions(-)

-- 
1.8.3.2

[PATCH v2 1/8] x86/PCI: prevent re-allocation of already existing bridge and ROM resources

[Mika Westerberg]

In hotplug case (especially with Thunderbolt enabled systems) we might need
to call pcibios_resource_survey_bus() several times for a bus. The function
ends up calling pci_claim_resource() for each bridge resource that then
fails claiming that the resource exists already (which it does). Once this
happens the resource is invalidated thus preventing devices behind the
bridge to allocate their resources.

To fix this we do what has been done in pcibios_allocate_dev_resources()
and check 'parent' of the given resource. If it is non-NULL it means that
the resource has been allocated already and we can skip it. We do the same
for ROM resources as well.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 arch/x86/pci/i386.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 94919e3..db6b1ab 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -210,6 +210,8 @@ static void pcibios_allocate_bridge_resources(struct pci_dev *dev)
 		r = &dev->resource[idx];
 		if (!r->flags)
 			continue;
+		if (r->parent)	/* Already allocated */
+			continue;
 		if (!r->start || pci_claim_resource(dev, idx) < 0) {
 			/*
 			 * Something is wrong with the region.
@@ -318,6 +320,8 @@ static void pcibios_allocate_dev_rom_resource(struct pci_dev *dev)
 	r = &dev->resource[PCI_ROM_RESOURCE];
 	if (!r->flags || !r->start)
 		return;
+	if (r->parent) /* Already allocated */
+		return;
 
 	if (pci_claim_resource(dev, PCI_ROM_RESOURCE) < 0) {
 		r->end -= r->start;
-- 
1.8.3.2

[PATCH v2 2/8] PCI: acpiphp: do not check for SLOT_ENABLED in enable_device()

[Mika Westerberg]

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

With Thunderbolt you can chain devices: connect a new devices to plugged
one. In this case the slot is already enabled, but we still want to look
for new devices behind it.

We're going to reuse enable_device() for rescan for new devices on the
enabled slot. Let's push the check up by stack.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 59df857..b983e29 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -688,9 +688,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 	int num, max, pass;
 	LIST_HEAD(add_list);
 
-	if (slot->flags & SLOT_ENABLED)
-		goto err_exit;
-
 	list_for_each_entry(func, &slot->funcs, sibling)
 		acpiphp_bus_add(func);
 
@@ -1242,6 +1239,8 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot)
 		goto err_exit;
 
 	if (get_slot_status(slot) == ACPI_STA_ALL) {
+		if (slot->flags & SLOT_ENABLED)
+			goto err_exit;
 		/* configure all functions */
 		retval = enable_device(slot);
 		if (retval)
-- 
1.8.3.2

[PATCH v2 3/8] PCI: acpiphp: enable_device(): rescan even if no new devices on slot

[Mika Westerberg]

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

pci_scan_slot() returns number of new devices connected *directly*
connected to the slot. Current enable_device() checks the return value
and stops if it doesn't see a new device.

In Thunderbolt chaining case the new device can be deeper in hierarchy, so
do the rescan anyway.

Because of that we must make sure that pcibios_resource_survey_bus() and
check_hotplug_bridge() get called only for a just found bus and not the
ones already added to the system. Failure to do so will lead to resource
conflicts.

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

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index b983e29..0f4da3b 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -685,18 +685,13 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 	struct pci_dev *dev;
 	struct pci_bus *bus = slot->bridge->pci_bus;
 	struct acpiphp_func *func;
-	int num, max, pass;
+	int max, pass;
 	LIST_HEAD(add_list);
 
 	list_for_each_entry(func, &slot->funcs, sibling)
 		acpiphp_bus_add(func);
 
-	num = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
-	if (num == 0) {
-		/* Maybe only part of funcs are added. */
-		dbg("No new device found\n");
-		goto err_exit;
-	}
+	pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
 
 	max = acpiphp_max_busnr(bus);
 	for (pass = 0; pass < 2; pass++) {
@@ -742,8 +737,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 		}
 	}
 
-
- err_exit:
 	return 0;
 }
 
-- 
1.8.3.2

[PATCH v2 4/8] PCI: acpiphp: check for new devices on enabled host

[Mika Westerberg]

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

Current acpiphp_check_bridge() implementation is pretty dumb:
 - it enables the slot if it's not enabled and the slot status is
   ACPI_STA_ALL;
 - it disables the slot if it's enabled and slot is not in ACPI_STA_ALL
   state.

This behavior is not enough to handle Thunderbolt chaining case
properly. We need to actually rescan for new devices even if a device
has already in the slot.

The new implementation disables and stops the slot if it's not in
ACPI_STA_ALL state.

For ACPI_STA_ALL state we first trim devices which don't respond. In order
to support that we introduce a new function pci_trim_stale_devices() that
goes through the child devices of a given bus, check if the device is gone
and in that case stop and remove the device.

Once we have trimmed the bus we start looking for any new devices that
might have appeared on the bus. We do that even if the slot is already
enabled (SLOT_ENABLED).

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c | 55 ++++++++++++++++++--------------------
 drivers/pci/remove.c               | 20 ++++++++++++++
 include/linux/pci.h                |  1 +
 3 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 0f4da3b..c9ec06e 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -865,43 +865,40 @@ int acpiphp_eject_slot(struct acpiphp_slot *slot)
  * Iterate over all slots under this bridge and make sure that if a
  * card is present they are enabled, and if not they are disabled.
  */
-static int acpiphp_check_bridge(struct acpiphp_bridge *bridge)
+static void acpiphp_check_bridge(struct acpiphp_bridge *bridge)
 {
 	struct acpiphp_slot *slot;
-	int retval = 0;
-	int enabled, disabled;
-
-	enabled = disabled = 0;
 
 	list_for_each_entry(slot, &bridge->slots, node) {
-		unsigned int status = get_slot_status(slot);
-		if (slot->flags & SLOT_ENABLED) {
-			if (status == ACPI_STA_ALL)
-				continue;
-			retval = acpiphp_disable_slot(slot);
-			if (retval) {
-				err("Error occurred in disabling\n");
-				goto err_exit;
-			} else {
-				acpiphp_eject_slot(slot);
+		struct pci_bus *bus = slot->bridge->pci_bus;
+		struct pci_dev *dev, *tmp;
+		int retval;
+
+		mutex_lock(&slot->crit_sect);
+		/* wake up all functions */
+		retval = power_on_slot(slot);
+		if (retval)
+			goto unlock;
+
+		if (get_slot_status(slot) == ACPI_STA_ALL) {
+			/* remove stale devices if any */
+			list_for_each_entry_safe(dev, tmp, &bus->devices,
+						 bus_list) {
+				if (PCI_SLOT(dev->devfn) == slot->device)
+					pci_trim_stale_devices(dev);
 			}
-			disabled++;
+
+			/* configure all functions */
+			retval = enable_device(slot);
+			if (retval)
+				power_off_slot(slot);
 		} else {
-			if (status != ACPI_STA_ALL)
-				continue;
-			retval = acpiphp_enable_slot(slot);
-			if (retval) {
-				err("Error occurred in enabling\n");
-				goto err_exit;
-			}
-			enabled++;
+			disable_device(slot);
+			power_off_slot(slot);
 		}
+unlock:
+		mutex_unlock(&slot->crit_sect);
 	}
-
-	dbg("%s: %d enabled, %d disabled\n", __func__, enabled, disabled);
-
- err_exit:
-	return retval;
 }
 
 static void acpiphp_set_hpp_values(struct pci_bus *bus)
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 8fc54b7..77b7a64 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -112,6 +112,26 @@ void pci_stop_and_remove_bus_device(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
 
+/**
+ * pci_trim_stale_devices - remove stale device or any stale child
+ */
+void pci_trim_stale_devices(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->subordinate;
+	struct pci_dev *child, *tmp;
+	bool alive;
+	u32 l;
+
+	/* check if the device responds */
+	alive = pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &l, 0);
+	if (!alive)
+		pci_stop_and_remove_bus_device(dev);
+
+	if (alive && bus)
+		list_for_each_entry_safe(child, tmp, &bus->devices, bus_list)
+			pci_trim_stale_devices(child);
+}
+
 void pci_stop_root_bus(struct pci_bus *bus)
 {
 	struct pci_dev *child, *tmp;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3a24e4f..8f6e7a0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -754,6 +754,7 @@ struct pci_dev *pci_dev_get(struct pci_dev *dev);
 void pci_dev_put(struct pci_dev *dev);
 void pci_remove_bus(struct pci_bus *b);
 void pci_stop_and_remove_bus_device(struct pci_dev *dev);
+void pci_trim_stale_devices(struct pci_dev *dev);
 void pci_stop_root_bus(struct pci_bus *bus);
 void pci_remove_root_bus(struct pci_bus *bus);
 void pci_setup_cardbus(struct pci_bus *bus);
-- 
1.8.3.2

[PATCH v2 5/8] PCI: acpiphp: kill SLOT_ENABLED in favor of always re-enumerating the devices

[Mika Westerberg]

The acpiphp driver checks SLOT_ENABLED flag only in aciphp_enable_slot() to
prevent re-enumeration of the already enabled slot.  Because of that it is
not possible to add new devices behind the slot later on (like it is the
case with Thunderbolt devices).

Now, there are two places where acpiphp_enable_slot() gets called:
 1) On Bus Check event for a function (in case of a dock).
 2) When userpace enables a slot by writing '1' to
    /sys/bus/pci/slots/*/power.

In the first case it doesn't make sense to do the check because we are
ought to re-enumerate the bus anyway and see if there are any new devices
added farther in the hierarchy, according the the ACPI specification.

The second case where userspace explicitly enables the device also doesn't
need this check as we already check if the slot is powered on or off in
power_on_slot() and don't do it twice.

So get rid of the SLOT_ENABLED flag and always re-enumerate devices behind
a slot when acpiphp_enable_slot() is called. In addition doing this allows
userspace to simulate Bus Check event by writing '1' to the 'power' file
under the slot.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/acpiphp.h      |  1 -
 drivers/pci/hotplug/acpiphp_glue.c | 18 +-----------------
 2 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index 6fdd49c..ecc61b7 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -154,7 +154,6 @@ struct acpiphp_attention_info
 /* slot flags */
 
 #define SLOT_POWEREDON		(0x00000001)
-#define SLOT_ENABLED		(0x00000002)
 #define SLOT_MULTIFUNCTION	(0x00000004)
 
 /* function flags */
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index c9ec06e..506f38f 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -304,7 +304,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 
 	if (pci_bus_read_dev_vendor_id(pbus, PCI_DEVFN(device, function),
 				       &val, 60*1000))
-		slot->flags |= (SLOT_ENABLED | SLOT_POWEREDON);
+		slot->flags |= SLOT_POWEREDON;
 
 	if (is_dock_device(handle)) {
 		/* we don't want to call this device's _EJ0
@@ -725,18 +725,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 
 	pci_bus_add_devices(bus);
 
-	slot->flags |= SLOT_ENABLED;
-	list_for_each_entry(func, &slot->funcs, sibling) {
-		dev = pci_get_slot(bus, PCI_DEVFN(slot->device,
-						  func->function));
-		if (!dev) {
-			/* Do not set SLOT_ENABLED flag if some funcs
-			   are not added. */
-			slot->flags &= (~SLOT_ENABLED);
-			continue;
-		}
-	}
-
 	return 0;
 }
 
@@ -782,8 +770,6 @@ static int disable_device(struct acpiphp_slot *slot)
 		acpiphp_bus_trim(func->handle);
 	}
 
-	slot->flags &= (~SLOT_ENABLED);
-
 	return 0;
 }
 
@@ -1229,8 +1215,6 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot)
 		goto err_exit;
 
 	if (get_slot_status(slot) == ACPI_STA_ALL) {
-		if (slot->flags & SLOT_ENABLED)
-			goto err_exit;
 		/* configure all functions */
 		retval = enable_device(slot);
 		if (retval)
-- 
1.8.3.2

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

[Mika Westerberg]

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);
+	if (ACPI_SUCCESS(status) && removable)
+		return 1;
+
 	return 0;
 }
 
-- 
1.8.3.2

[PATCH v2 7/8] PCI: acpiphp: get rid of unused constants in acpiphp.h

[Mika Westerberg]

These are not used anywhere so kill them.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/acpiphp.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index ecc61b7..5fc9977 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -142,9 +142,6 @@ struct acpiphp_attention_info
 	struct module *owner;
 };
 
-/* PCI bus bridge HID */
-#define ACPI_PCI_HOST_HID		"PNP0A03"
-
 /* ACPI _STA method value (ignore bit 4; battery present) */
 #define ACPI_STA_ALL			(0x0000000f)
 
@@ -154,7 +151,6 @@ struct acpiphp_attention_info
 /* slot flags */
 
 #define SLOT_POWEREDON		(0x00000001)
-#define SLOT_MULTIFUNCTION	(0x00000004)
 
 /* function flags */
 
-- 
1.8.3.2

[PATCH v2 8/8] PCI: acpiphp: sanitize acpiphp_get_[latch|adapter]_status()

[Mika Westerberg]

There is no need for a temporary variable and all the tricks with ternary
operators. Change the functions to be a bit more straightforward.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 506f38f..810ca4c 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -1271,11 +1271,7 @@ u8 acpiphp_get_power_status(struct acpiphp_slot *slot)
  */
 u8 acpiphp_get_latch_status(struct acpiphp_slot *slot)
 {
-	unsigned int sta;
-
-	sta = get_slot_status(slot);
-
-	return (sta & ACPI_STA_DEVICE_UI) ? 0 : 1;
+	return !(get_slot_status(slot) & ACPI_STA_DEVICE_UI);
 }
 
 
@@ -1285,9 +1281,5 @@ u8 acpiphp_get_latch_status(struct acpiphp_slot *slot)
  */
 u8 acpiphp_get_adapter_status(struct acpiphp_slot *slot)
 {
-	unsigned int sta;
-
-	sta = get_slot_status(slot);
-
-	return (sta == 0) ? 0 : 1;
+	return !!get_slot_status(slot);
 }
-- 
1.8.3.2

Re: [PATCH v2 0/8] Thunderbolt workarounds

[Matthew Garrett]

Are there any plans to provide native support for the Thunderbolt 
controller, rather than relying on system management mode?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v2 0/8] Thunderbolt workarounds

[Greg Kroah-Hartman]

On Wed, Jul 03, 2013 at 07:29:00PM +0100, Matthew Garrett wrote:
> Are there any plans to provide native support for the Thunderbolt 
> controller, rather than relying on system management mode?

For what specific hardware platform?  For the ones that this patchset
controls, the OS doesn't have access to the Thunderbolt controller as
far as I can tell, it all happens through the ACPI and PCI hotplug
interface.

thanks,

greg k-h

Re: [PATCH v2 0/8] Thunderbolt workarounds

[Mika Westerberg]

On Wed, Jul 03, 2013 at 07:29:00PM +0100, Matthew Garrett wrote:
> Are there any plans to provide native support for the Thunderbolt 
> controller, rather than relying on system management mode?

I believe there are some to implement a native Thunderbolt controller
driver but I don't know the details. 

Bruce Allan and Miles Penner (who are CC'd here) might have a better answer
for you.

Re: [PATCH v2 0/8] Thunderbolt workarounds

[Matthew Garrett]

On Wed, Jul 03, 2013 at 11:33:11AM -0700, Greg Kroah-Hartman wrote:
> On Wed, Jul 03, 2013 at 07:29:00PM +0100, Matthew Garrett wrote:
> > Are there any plans to provide native support for the Thunderbolt 
> > controller, rather than relying on system management mode?
> 
> For what specific hardware platform?  For the ones that this patchset
> controls, the OS doesn't have access to the Thunderbolt controller as
> far as I can tell, it all happens through the ACPI and PCI hotplug
> interface.

Given that there exist platforms without the SMM implementation, there's 
presumably either a controller or chipset register that controls whether 
SMIs are generated in response to Thunderbolt events.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

RE: [PATCH v2 0/8] Thunderbolt workarounds

[Ronciak, John]

Are you talking about Apple platforms specifically?

Cheers,
John


> -----Original Message-----
> From: Matthew Garrett [mailto:mjg59@srcf.ucam.org]
> Sent: Wednesday, July 03, 2013 11:45 AM
> To: Greg Kroah-Hartman
> Cc: Mika Westerberg; Bjorn Helgaas; Wysocki, Rafael J; Jesse Barnes;
> Yinghai Lu; Ronciak, John; Penner, Miles J; Allan, Bruce W; Heikki
> Krogerus; Kirill A. Shutemov; linux-kernel@vger.kernel.org; linux-
> acpi@vger.kernel.org; linux-pci@vger.kernel.org; x86@kernel.org
> Subject: Re: [PATCH v2 0/8] Thunderbolt workarounds
> 
> On Wed, Jul 03, 2013 at 11:33:11AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, Jul 03, 2013 at 07:29:00PM +0100, Matthew Garrett wrote:
> > > Are there any plans to provide native support for the Thunderbolt
> > > controller, rather than relying on system management mode?
> >
> > For what specific hardware platform?  For the ones that this patchset
> > controls, the OS doesn't have access to the Thunderbolt controller as
> > far as I can tell, it all happens through the ACPI and PCI hotplug
> > interface.
> 
> Given that there exist platforms without the SMM implementation,
> there's presumably either a controller or chipset register that
> controls whether SMIs are generated in response to Thunderbolt events.
> 
> --
> Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v2 0/8] Thunderbolt workarounds

[Matthew Garrett]

On Wed, Jul 03, 2013 at 07:57:21PM +0000, Ronciak, John wrote:
> Are you talking about Apple platforms specifically?

Yeah, that's the specific example.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

RE: [PATCH v2 0/8] Thunderbolt workarounds

[Ronciak, John]

Apple does their own thing and do not talk to Intel about it at all. This includes writing special drivers and user-space components.  So we (Intel) don't know so we can't comment about it.  Sorry.

Cheers,
John


> -----Original Message-----
> From: Matthew Garrett [mailto:mjg59@srcf.ucam.org]
> Sent: Wednesday, July 03, 2013 1:36 PM
> To: Ronciak, John
> Cc: Greg Kroah-Hartman; Mika Westerberg; Bjorn Helgaas; Wysocki, Rafael
> J; Jesse Barnes; Yinghai Lu; Penner, Miles J; Allan, Bruce W; Heikki
> Krogerus; Kirill A. Shutemov; linux-kernel@vger.kernel.org; linux-
> acpi@vger.kernel.org; linux-pci@vger.kernel.org; x86@kernel.org
> Subject: Re: [PATCH v2 0/8] Thunderbolt workarounds
> 
> On Wed, Jul 03, 2013 at 07:57:21PM +0000, Ronciak, John wrote:
> > Are you talking about Apple platforms specifically?
> 
> Yeah, that's the specific example.
> 
> --
> Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v2 0/8] Thunderbolt workarounds

[Matthew Garrett]

On Wed, Jul 03, 2013 at 08:46:57PM +0000, Ronciak, John wrote:
> Apple does their own thing and do not talk to Intel about it at all. This includes writing special drivers and user-space components.  So we (Intel) don't know so we can't comment about it.  Sorry.

The hardware has an Intel part number, so I'm pretty sure someone there 
has some idea what the register map looks like.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Rafael J. Wysocki]

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


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

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

[Bjorn Helgaas]

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
>

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

[Rafael J. Wysocki]

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

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

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

[Mika Westerberg]

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?

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)
+{
+	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);
 }
 
 /**
-- 
1.8.3.2

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

[Rafael J. Wysocki]

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


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

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

[Mika Westerberg]

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.

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

[Rafael J. Wysocki]

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


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

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

[Mika Westerberg]

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.

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

[Rafael J. Wysocki]

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


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

[PATCH v2.1 6/8] PCI: acpiphp: workaround for Thunderbolt on Acer Aspire S5

[Mika Westerberg]

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 | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
index 2a47e82..eae1511 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,11 +409,31 @@ got_one:
 }
 EXPORT_SYMBOL(acpi_get_hp_hw_control_from_firmware);
 
+static const struct dmi_system_id pcihp_platform_quirks[] = {
+	{
+		/*
+		 * 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",
+	},
+	{ }
+};
+
 static int pcihp_is_ejectable(acpi_handle handle)
 {
 	acpi_status status;
 	acpi_handle tmp;
 	unsigned long long removable;
+	const struct dmi_system_id *id;
+
 	status = acpi_get_handle(handle, "_ADR", &tmp);
 	if (ACPI_FAILURE(status))
 		return 0;
@@ -422,6 +443,19 @@ static int pcihp_is_ejectable(acpi_handle handle)
 	status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable);
 	if (ACPI_SUCCESS(status) && removable)
 		return 1;
+
+	/*
+	 * Try to look if there is a platform specific method that we can
+	 * use to determine if the device is removable or not.
+	 */
+	id = dmi_first_match(pcihp_platform_quirks);
+	if (id && id->driver_data) {
+		status = acpi_evaluate_integer(handle, id->driver_data, NULL,
+					       &removable);
+		if (ACPI_SUCCESS(status) && removable)
+			return 1;
+	}
+
 	return 0;
 }
 
-- 
1.8.3.2

Re: [PATCH v2.1 6/8] PCI: acpiphp: workaround for Thunderbolt on Acer Aspire S5

[Rafael J. Wysocki]

On Thursday, July 04, 2013 05:29:51 PM Mika Westerberg wrote:
> 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>

This is fine by me and same for the other patches in the series.

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/hotplug/acpi_pcihp.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> index 2a47e82..eae1511 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,11 +409,31 @@ got_one:
>  }
>  EXPORT_SYMBOL(acpi_get_hp_hw_control_from_firmware);
>  
> +static const struct dmi_system_id pcihp_platform_quirks[] = {
> +	{
> +		/*
> +		 * 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",
> +	},
> +	{ }
> +};
> +
>  static int pcihp_is_ejectable(acpi_handle handle)
>  {
>  	acpi_status status;
>  	acpi_handle tmp;
>  	unsigned long long removable;
> +	const struct dmi_system_id *id;
> +
>  	status = acpi_get_handle(handle, "_ADR", &tmp);
>  	if (ACPI_FAILURE(status))
>  		return 0;
> @@ -422,6 +443,19 @@ static int pcihp_is_ejectable(acpi_handle handle)
>  	status = acpi_evaluate_integer(handle, "_RMV", NULL, &removable);
>  	if (ACPI_SUCCESS(status) && removable)
>  		return 1;
> +
> +	/*
> +	 * Try to look if there is a platform specific method that we can
> +	 * use to determine if the device is removable or not.
> +	 */
> +	id = dmi_first_match(pcihp_platform_quirks);
> +	if (id && id->driver_data) {
> +		status = acpi_evaluate_integer(handle, id->driver_data, NULL,
> +					       &removable);
> +		if (ACPI_SUCCESS(status) && removable)
> +			return 1;
> +	}
> +
>  	return 0;
>  }
>  
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2.1 6/8] PCI: acpiphp: workaround for Thunderbolt on Acer Aspire S5

[Mika Westerberg]

On Fri, Jul 05, 2013 at 01:48:01AM +0200, Rafael J. Wysocki wrote:
> This is fine by me and same for the other patches in the series.
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks!

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

[Robert Hancock]

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

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

[Rafael J. Wysocki]

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


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

Re: [PATCH v2 1/8] x86/PCI: prevent re-allocation of already existing bridge and ROM resources

[Bjorn Helgaas]

On Wed, Jul 3, 2013 at 8:04 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> In hotplug case (especially with Thunderbolt enabled systems) we might need
> to call pcibios_resource_survey_bus() several times for a bus. The function
> ends up calling pci_claim_resource() for each bridge resource that then
> fails claiming that the resource exists already (which it does). Once this
> happens the resource is invalidated thus preventing devices behind the
> bridge to allocate their resources.
>
> To fix this we do what has been done in pcibios_allocate_dev_resources()
> and check 'parent' of the given resource. If it is non-NULL it means that
> the resource has been allocated already and we can skip it. We do the same
> for ROM resources as well.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

This looks reasonable to me.

However, the use of "r->parent" to determine whether the resource is
already allocated is nothing specific to x86.  So I think we might be
missing an opportunity to make this more consistent across
architectures.  I looked at other callers of pci_claim_resource() for
bridge and ROM resources, and it looks like we might be able to make
similar changes in:

  frv pcibios_allocate_bus_resources()
  ia64 pcibios_fixup_resources() (via pcibios_fixup_bridge_resources())
  mn10300 pcibios_allocate_bus_resources()
  mn10300 pcibios_assign_resources() (ROM)
  mn10300 pcibios_fixup_device_resources()
  parisc lba_fixup_bus()

I really hate the idea of fixing something for x86 but not for other
arches, so maybe somebody could take a deeper look at this and see if
it would make sense to change the other arches, too.

Bjorn

> ---
>  arch/x86/pci/i386.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
> index 94919e3..db6b1ab 100644
> --- a/arch/x86/pci/i386.c
> +++ b/arch/x86/pci/i386.c
> @@ -210,6 +210,8 @@ static void pcibios_allocate_bridge_resources(struct pci_dev *dev)
>                 r = &dev->resource[idx];
>                 if (!r->flags)
>                         continue;
> +               if (r->parent)  /* Already allocated */
> +                       continue;
>                 if (!r->start || pci_claim_resource(dev, idx) < 0) {
>                         /*
>                          * Something is wrong with the region.
> @@ -318,6 +320,8 @@ static void pcibios_allocate_dev_rom_resource(struct pci_dev *dev)
>         r = &dev->resource[PCI_ROM_RESOURCE];
>         if (!r->flags || !r->start)
>                 return;
> +       if (r->parent) /* Already allocated */
> +               return;
>
>         if (pci_claim_resource(dev, PCI_ROM_RESOURCE) < 0) {
>                 r->end -= r->start;
> --
> 1.8.3.2
>

Re: [PATCH v2 1/8] x86/PCI: prevent re-allocation of already existing bridge and ROM resources

[Bjorn Helgaas]

On Mon, Jul 22, 2013 at 6:08 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Jul 3, 2013 at 8:04 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
>> In hotplug case (especially with Thunderbolt enabled systems) we might need
>> to call pcibios_resource_survey_bus() several times for a bus. The function
>> ends up calling pci_claim_resource() for each bridge resource that then
>> fails claiming that the resource exists already (which it does). Once this
>> happens the resource is invalidated thus preventing devices behind the
>> bridge to allocate their resources.
>>
>> To fix this we do what has been done in pcibios_allocate_dev_resources()
>> and check 'parent' of the given resource. If it is non-NULL it means that
>> the resource has been allocated already and we can skip it. We do the same
>> for ROM resources as well.
>>
>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> This looks reasonable to me.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> However, the use of "r->parent" to determine whether the resource is
> already allocated is nothing specific to x86.  So I think we might be
> missing an opportunity to make this more consistent across
> architectures.  I looked at other callers of pci_claim_resource() for
> bridge and ROM resources, and it looks like we might be able to make
> similar changes in:
>
>   frv pcibios_allocate_bus_resources()
>   ia64 pcibios_fixup_resources() (via pcibios_fixup_bridge_resources())
>   mn10300 pcibios_allocate_bus_resources()
>   mn10300 pcibios_assign_resources() (ROM)
>   mn10300 pcibios_fixup_device_resources()
>   parisc lba_fixup_bus()
>
> I really hate the idea of fixing something for x86 but not for other
> arches, so maybe somebody could take a deeper look at this and see if
> it would make sense to change the other arches, too.
>
> Bjorn
>
>> ---
>>  arch/x86/pci/i386.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
>> index 94919e3..db6b1ab 100644
>> --- a/arch/x86/pci/i386.c
>> +++ b/arch/x86/pci/i386.c
>> @@ -210,6 +210,8 @@ static void pcibios_allocate_bridge_resources(struct pci_dev *dev)
>>                 r = &dev->resource[idx];
>>                 if (!r->flags)
>>                         continue;
>> +               if (r->parent)  /* Already allocated */
>> +                       continue;
>>                 if (!r->start || pci_claim_resource(dev, idx) < 0) {
>>                         /*
>>                          * Something is wrong with the region.
>> @@ -318,6 +320,8 @@ static void pcibios_allocate_dev_rom_resource(struct pci_dev *dev)
>>         r = &dev->resource[PCI_ROM_RESOURCE];
>>         if (!r->flags || !r->start)
>>                 return;
>> +       if (r->parent) /* Already allocated */
>> +               return;
>>
>>         if (pci_claim_resource(dev, PCI_ROM_RESOURCE) < 0) {
>>                 r->end -= r->start;
>> --
>> 1.8.3.2
>>

Re: [PATCH v2 1/8] x86/PCI: prevent re-allocation of already existing bridge and ROM resources

[Bjorn Helgaas]

On Mon, Jul 22, 2013 at 7:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Jul 22, 2013 at 6:08 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Wed, Jul 3, 2013 at 8:04 AM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>>> In hotplug case (especially with Thunderbolt enabled systems) we might need
>>> to call pcibios_resource_survey_bus() several times for a bus. The function
>>> ends up calling pci_claim_resource() for each bridge resource that then
>>> fails claiming that the resource exists already (which it does). Once this
>>> happens the resource is invalidated thus preventing devices behind the
>>> bridge to allocate their resources.
>>>
>>> To fix this we do what has been done in pcibios_allocate_dev_resources()
>>> and check 'parent' of the given resource. If it is non-NULL it means that
>>> the resource has been allocated already and we can skip it. We do the same
>>> for ROM resources as well.
>>>
>>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>
>> This looks reasonable to me.
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
>> However, the use of "r->parent" to determine whether the resource is
>> already allocated is nothing specific to x86.  So I think we might be
>> missing an opportunity to make this more consistent across
>> architectures.  I looked at other callers of pci_claim_resource() for
>> bridge and ROM resources, and it looks like we might be able to make
>> similar changes in:
>>
>>   frv pcibios_allocate_bus_resources()
>>   ia64 pcibios_fixup_resources() (via pcibios_fixup_bridge_resources())
>>   mn10300 pcibios_allocate_bus_resources()
>>   mn10300 pcibios_assign_resources() (ROM)
>>   mn10300 pcibios_fixup_device_resources()
>>   parisc lba_fixup_bus()
>>
>> I really hate the idea of fixing something for x86 but not for other
>> arches, so maybe somebody could take a deeper look at this and see if
>> it would make sense to change the other arches, too.

I misspoke here.  The change below helps fix an issue on x86.  It's
only an issue on x86 because only x86 has acpiphp and
pcibios_resource_survey_bus().  Other arches *do* call
pci_claim_resource(), but making similar changes on other arches does
not fix similar issues there, so it's not really a matter of "fixing
only x86."

My motivation is to move toward unification of how we claim resources.
 There's nothing inherently arch-specific about calling
pci_claim_resource(), but arches use a variety of tests involving
"r->flags", "r->parent", and "r->start" to decide whether to do so.
Ideally we would call pci_claim_resource() from the core, not from
arch code, and we would use a set of tests that work for all arches
and situations.

We can always do the minimum of changing only what's absolutely
required, but the result is divergence and increased maintenance work
in the long term.  I'm trying to counter that a little bit by
encouraging people to consider refactorings that fix the current issue
while reducing maintenance effort.

Bjorn

>>> ---
>>>  arch/x86/pci/i386.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
>>> index 94919e3..db6b1ab 100644
>>> --- a/arch/x86/pci/i386.c
>>> +++ b/arch/x86/pci/i386.c
>>> @@ -210,6 +210,8 @@ static void pcibios_allocate_bridge_resources(struct pci_dev *dev)
>>>                 r = &dev->resource[idx];
>>>                 if (!r->flags)
>>>                         continue;
>>> +               if (r->parent)  /* Already allocated */
>>> +                       continue;
>>>                 if (!r->start || pci_claim_resource(dev, idx) < 0) {
>>>                         /*
>>>                          * Something is wrong with the region.
>>> @@ -318,6 +320,8 @@ static void pcibios_allocate_dev_rom_resource(struct pci_dev *dev)
>>>         r = &dev->resource[PCI_ROM_RESOURCE];
>>>         if (!r->flags || !r->start)
>>>                 return;
>>> +       if (r->parent) /* Already allocated */
>>> +               return;
>>>
>>>         if (pci_claim_resource(dev, PCI_ROM_RESOURCE) < 0) {
>>>                 r->end -= r->start;
>>> --
>>> 1.8.3.2
>>>

Re: [PATCH v2 1/8] x86/PCI: prevent re-allocation of already existing bridge and ROM resources

[Rafael J. Wysocki]

On Monday, July 22, 2013 07:18:38 PM Bjorn Helgaas wrote:
> On Mon, Jul 22, 2013 at 6:08 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Wed, Jul 3, 2013 at 8:04 AM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> >> In hotplug case (especially with Thunderbolt enabled systems) we might need
> >> to call pcibios_resource_survey_bus() several times for a bus. The function
> >> ends up calling pci_claim_resource() for each bridge resource that then
> >> fails claiming that the resource exists already (which it does). Once this
> >> happens the resource is invalidated thus preventing devices behind the
> >> bridge to allocate their resources.
> >>
> >> To fix this we do what has been done in pcibios_allocate_dev_resources()
> >> and check 'parent' of the given resource. If it is non-NULL it means that
> >> the resource has been allocated already and we can skip it. We do the same
> >> for ROM resources as well.
> >>
> >> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >
> > This looks reasonable to me.
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks a lot!

> > However, the use of "r->parent" to determine whether the resource is
> > already allocated is nothing specific to x86.  So I think we might be
> > missing an opportunity to make this more consistent across
> > architectures.  I looked at other callers of pci_claim_resource() for
> > bridge and ROM resources, and it looks like we might be able to make
> > similar changes in:
> >
> >   frv pcibios_allocate_bus_resources()
> >   ia64 pcibios_fixup_resources() (via pcibios_fixup_bridge_resources())
> >   mn10300 pcibios_allocate_bus_resources()
> >   mn10300 pcibios_assign_resources() (ROM)
> >   mn10300 pcibios_fixup_device_resources()
> >   parisc lba_fixup_bus()
> >
> > I really hate the idea of fixing something for x86 but not for other
> > arches, so maybe somebody could take a deeper look at this and see if
> > it would make sense to change the other arches, too.
> >
> > Bjorn
> >
> >> ---
> >>  arch/x86/pci/i386.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
> >> index 94919e3..db6b1ab 100644
> >> --- a/arch/x86/pci/i386.c
> >> +++ b/arch/x86/pci/i386.c
> >> @@ -210,6 +210,8 @@ static void pcibios_allocate_bridge_resources(struct pci_dev *dev)
> >>                 r = &dev->resource[idx];
> >>                 if (!r->flags)
> >>                         continue;
> >> +               if (r->parent)  /* Already allocated */
> >> +                       continue;
> >>                 if (!r->start || pci_claim_resource(dev, idx) < 0) {
> >>                         /*
> >>                          * Something is wrong with the region.
> >> @@ -318,6 +320,8 @@ static void pcibios_allocate_dev_rom_resource(struct pci_dev *dev)
> >>         r = &dev->resource[PCI_ROM_RESOURCE];
> >>         if (!r->flags || !r->start)
> >>                 return;
> >> +       if (r->parent) /* Already allocated */
> >> +               return;
> >>
> >>         if (pci_claim_resource(dev, PCI_ROM_RESOURCE) < 0) {
> >>                 r->end -= r->start;
> >> --
> >> 1.8.3.2
> >>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 1/8] x86/PCI: prevent re-allocation of already existing bridge and ROM resources

[Rafael J. Wysocki]

On Monday, July 22, 2013 07:33:32 PM Bjorn Helgaas wrote:
> On Mon, Jul 22, 2013 at 7:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Mon, Jul 22, 2013 at 6:08 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> On Wed, Jul 3, 2013 at 8:04 AM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> >>> In hotplug case (especially with Thunderbolt enabled systems) we might need
> >>> to call pcibios_resource_survey_bus() several times for a bus. The function
> >>> ends up calling pci_claim_resource() for each bridge resource that then
> >>> fails claiming that the resource exists already (which it does). Once this
> >>> happens the resource is invalidated thus preventing devices behind the
> >>> bridge to allocate their resources.
> >>>
> >>> To fix this we do what has been done in pcibios_allocate_dev_resources()
> >>> and check 'parent' of the given resource. If it is non-NULL it means that
> >>> the resource has been allocated already and we can skip it. We do the same
> >>> for ROM resources as well.
> >>>
> >>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >>
> >> This looks reasonable to me.
> >
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> >> However, the use of "r->parent" to determine whether the resource is
> >> already allocated is nothing specific to x86.  So I think we might be
> >> missing an opportunity to make this more consistent across
> >> architectures.  I looked at other callers of pci_claim_resource() for
> >> bridge and ROM resources, and it looks like we might be able to make
> >> similar changes in:
> >>
> >>   frv pcibios_allocate_bus_resources()
> >>   ia64 pcibios_fixup_resources() (via pcibios_fixup_bridge_resources())
> >>   mn10300 pcibios_allocate_bus_resources()
> >>   mn10300 pcibios_assign_resources() (ROM)
> >>   mn10300 pcibios_fixup_device_resources()
> >>   parisc lba_fixup_bus()
> >>
> >> I really hate the idea of fixing something for x86 but not for other
> >> arches, so maybe somebody could take a deeper look at this and see if
> >> it would make sense to change the other arches, too.
> 
> I misspoke here.  The change below helps fix an issue on x86.  It's
> only an issue on x86 because only x86 has acpiphp and
> pcibios_resource_survey_bus().  Other arches *do* call
> pci_claim_resource(), but making similar changes on other arches does
> not fix similar issues there, so it's not really a matter of "fixing
> only x86."
> 
> My motivation is to move toward unification of how we claim resources.
>  There's nothing inherently arch-specific about calling
> pci_claim_resource(), but arches use a variety of tests involving
> "r->flags", "r->parent", and "r->start" to decide whether to do so.
> Ideally we would call pci_claim_resource() from the core, not from
> arch code, and we would use a set of tests that work for all arches
> and situations.

Agreed.

Do you think we can discuss that at the miniconf during the LPC?
We have resources handling on our agenda anyway.

> We can always do the minimum of changing only what's absolutely
> required, but the result is divergence and increased maintenance work
> in the long term.  I'm trying to counter that a little bit by
> encouraging people to consider refactorings that fix the current issue
> while reducing maintenance effort.

I understand the motivation, but I also think that such changes should be
coordinated, for example so that we know that they have been tested on all
architectures in question before they go to the mainline for good.

Thanks,
Rafael


> >>> ---
> >>>  arch/x86/pci/i386.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
> >>> index 94919e3..db6b1ab 100644
> >>> --- a/arch/x86/pci/i386.c
> >>> +++ b/arch/x86/pci/i386.c
> >>> @@ -210,6 +210,8 @@ static void pcibios_allocate_bridge_resources(struct pci_dev *dev)
> >>>                 r = &dev->resource[idx];
> >>>                 if (!r->flags)
> >>>                         continue;
> >>> +               if (r->parent)  /* Already allocated */
> >>> +                       continue;
> >>>                 if (!r->start || pci_claim_resource(dev, idx) < 0) {
> >>>                         /*
> >>>                          * Something is wrong with the region.
> >>> @@ -318,6 +320,8 @@ static void pcibios_allocate_dev_rom_resource(struct pci_dev *dev)
> >>>         r = &dev->resource[PCI_ROM_RESOURCE];
> >>>         if (!r->flags || !r->start)
> >>>                 return;
> >>> +       if (r->parent) /* Already allocated */
> >>> +               return;
> >>>
> >>>         if (pci_claim_resource(dev, PCI_ROM_RESOURCE) < 0) {
> >>>                 r->end -= r->start;
> >>> --
> >>> 1.8.3.2
> >>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.