[PATCH V4 0/2] vfio, platform: add ACPI support

[PATCH V4 0/2] vfio, platform: add ACPI support

[Sinan Kaya]

The patchset makes three different changes.
1. Add support for probing ACPI platform devices.
2. Add support for ACPI _RST method support during reset
3. Make reset driver a requirement by default with an optional
   kernel command line override

Sinan Kaya (2):
  vfio, platform: add support for ACPI during probe and reset
  vfio, platform: make reset driver a requirement by default

 drivers/vfio/platform/vfio_amba.c             |   5 +
 drivers/vfio/platform/vfio_platform.c         |   5 +
 drivers/vfio/platform/vfio_platform_common.c  | 184 +++++++++++++++++++++-----
 drivers/vfio/platform/vfio_platform_private.h |   8 +-
 4 files changed, 165 insertions(+), 37 deletions(-)

-- 
1.8.2.1

[PATCH V4 0/2] vfio, platform: add ACPI support

[Sinan Kaya]

The patchset makes three different changes.
1. Add support for probing ACPI platform devices.
2. Add support for ACPI _RST method support during reset
3. Make reset driver a requirement by default with an optional
   kernel command line override

Sinan Kaya (2):
  vfio, platform: add support for ACPI during probe and reset
  vfio, platform: make reset driver a requirement by default

 drivers/vfio/platform/vfio_amba.c             |   5 +
 drivers/vfio/platform/vfio_platform.c         |   5 +
 drivers/vfio/platform/vfio_platform_common.c  | 184 +++++++++++++++++++++-----
 drivers/vfio/platform/vfio_platform_private.h |   8 +-
 4 files changed, 165 insertions(+), 37 deletions(-)

-- 
1.8.2.1

[PATCH V4 1/2] vfio, platform: add support for ACPI during probe and reset

[Sinan Kaya]

The code is using the compatible DT string to associate a reset driver with
the actual device itself. The compatible string does not exist on ACPI
based systems. HID is the unique identifier for a device driver instead.

When ACPI is enabled, the change will query the presence of _RST method
and will call it instead of querying an external reset driver.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/vfio/platform/vfio_platform_common.c  | 176 +++++++++++++++++++++-----
 drivers/vfio/platform/vfio_platform_private.h |   7 +-
 2 files changed, 147 insertions(+), 36 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e65b142..528ec04 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/acpi.h>
 #include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -41,7 +42,7 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 		if (!strcmp(iter->compat, compat) &&
 			try_module_get(iter->owner)) {
 			*module = iter->owner;
-			reset_fn = iter->reset;
+			reset_fn = iter->of_reset;
 			break;
 		}
 	}
@@ -49,20 +50,111 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 	return reset_fn;
 }
 
-static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
+
+#ifdef CONFIG_ACPI
+int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
+			     struct device *dev)
 {
-	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
-						&vdev->reset_module);
-	if (!vdev->reset) {
-		request_module("vfio-reset:%s", vdev->compat);
-		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
-							 &vdev->reset_module);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	if (!adev)
+		return -ENODEV;
+
+	vdev->acpihid = acpi_device_hid(adev);
+	if (!vdev->acpihid) {
+		pr_err("VFIO: cannot find ACPI HID for %s\n",
+		       vdev->name);
+		return -ENODEV;
 	}
+	return 0;
+}
+
+static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
+{
+	struct device *dev = vdev->device;
+	acpi_handle handle = ACPI_HANDLE(dev);
+	acpi_status acpi_ret;
+	unsigned long long val;
+
+	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
+	if (ACPI_FAILURE(acpi_ret))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
+{
+	struct device *dev = vdev->device;
+	acpi_handle handle = ACPI_HANDLE(dev);
+
+	if (acpi_has_method(handle, "_RST"))
+		return 0;
+
+	return -EINVAL;
+}
+#else
+int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
+			     struct device *dev)
+{
+	return -EINVAL;
+}
+
+static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
+{
+	return -EINVAL;
+}
+
+static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
+{
+	return -EINVAL;
+}
+#endif
+
+static int vfio_platform_has_reset(struct vfio_platform_device *vdev)
+{
+	if (vdev->acpihid)
+		return vfio_platform_acpi_has_reset(vdev);
+
+	if (vdev->of_reset)
+		return 0;
+
+	return -EINVAL;
+}
+
+static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
+{
+	int rc;
+
+	if (vdev->acpihid)
+		return vfio_platform_acpi_has_reset(vdev);
+
+	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
+						    &vdev->reset_module);
+	if (vdev->of_reset)
+		return 0;
+
+	rc = request_module("vfio-reset:%s", vdev->compat);
+	if (rc)
+		return rc;
+
+	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
+						    &vdev->reset_module);
+	if (vdev->of_reset)
+		return 0;
+
+	return -EINVAL;
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
 {
-	if (vdev->reset)
+	if (vdev->acpihid)
+		return;
+
+	if (vdev->of_reset)
 		module_put(vdev->reset_module);
 }
 
@@ -134,6 +226,20 @@ static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
 	kfree(vdev->regions);
 }
 
+static int vfio_platform_call_reset(struct vfio_platform_device *vdev)
+{
+	if (vdev->of_reset) {
+		dev_info(vdev->device, "reset\n");
+		vdev->of_reset(vdev);
+		return 0;
+	} else if (vdev->acpihid) {
+		return vfio_platform_acpi_call_reset(vdev);
+	}
+
+	dev_warn(vdev->device, "no reset function found!\n");
+	return -EINVAL;
+}
+
 static void vfio_platform_release(void *device_data)
 {
 	struct vfio_platform_device *vdev = device_data;
@@ -141,12 +247,7 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
-		if (vdev->reset) {
-			dev_info(vdev->device, "reset\n");
-			vdev->reset(vdev);
-		} else {
-			dev_warn(vdev->device, "no reset function found!\n");
-		}
+		vfio_platform_call_reset(vdev);
 		vfio_platform_regions_cleanup(vdev);
 		vfio_platform_irq_cleanup(vdev);
 	}
@@ -175,12 +276,9 @@ static int vfio_platform_open(void *device_data)
 		if (ret)
 			goto err_irq;
 
-		if (vdev->reset) {
-			dev_info(vdev->device, "reset\n");
-			vdev->reset(vdev);
-		} else {
-			dev_warn(vdev->device, "no reset function found!\n");
-		}
+		ret = vfio_platform_call_reset(vdev);
+		if (ret)
+			goto err_irq;
 	}
 
 	vdev->refcnt++;
@@ -213,7 +311,7 @@ static long vfio_platform_ioctl(void *device_data,
 		if (info.argsz < minsz)
 			return -EINVAL;
 
-		if (vdev->reset)
+		if (!vfio_platform_has_reset(vdev))
 			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
 		info.flags = vdev->flags;
 		info.num_regions = vdev->num_regions;
@@ -312,10 +410,7 @@ static long vfio_platform_ioctl(void *device_data,
 		return ret;
 
 	} else if (cmd == VFIO_DEVICE_RESET) {
-		if (vdev->reset)
-			return vdev->reset(vdev);
-		else
-			return -EINVAL;
+		return vfio_platform_call_reset(vdev);
 	}
 
 	return -ENOTTY;
@@ -544,6 +639,21 @@ static const struct vfio_device_ops vfio_platform_ops = {
 	.mmap		= vfio_platform_mmap,
 };
 
+int vfio_platform_of_probe(struct vfio_platform_device *vdev,
+			   struct device *dev)
+{
+	int ret;
+
+	ret = device_property_read_string(dev, "compatible",
+					  &vdev->compat);
+	if (ret) {
+		pr_err("VFIO: cannot retrieve compat for %s\n",
+			vdev->name);
+		return ret;
+	}
+	return 0;
+}
+
 int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 			       struct device *dev)
 {
@@ -553,14 +663,14 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 	if (!vdev)
 		return -EINVAL;
 
-	ret = device_property_read_string(dev, "compatible", &vdev->compat);
-	if (ret) {
-		pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
-		return -EINVAL;
-	}
+	ret = vfio_platform_acpi_probe(vdev, dev);
+	if (ret)
+		ret = vfio_platform_of_probe(vdev, dev);
 
-	vdev->device = dev;
+	if (ret)
+		return ret;
 
+	vdev->device = dev;
 	group = iommu_group_get(dev);
 	if (!group) {
 		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
@@ -611,7 +721,7 @@ void vfio_platform_unregister_reset(const char *compat,
 
 	mutex_lock(&driver_lock);
 	list_for_each_entry_safe(iter, temp, &reset_list, link) {
-		if (!strcmp(iter->compat, compat) && (iter->reset == fn)) {
+		if (!strcmp(iter->compat, compat) && (iter->of_reset == fn)) {
 			list_del(&iter->link);
 			break;
 		}
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 42816dd..ba9e4f8 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -58,6 +58,7 @@ struct vfio_platform_device {
 	struct mutex			igate;
 	struct module			*parent_module;
 	const char			*compat;
+	const char			*acpihid;
 	struct module			*reset_module;
 	struct device			*device;
 
@@ -71,7 +72,7 @@ struct vfio_platform_device {
 	struct resource*
 		(*get_resource)(struct vfio_platform_device *vdev, int i);
 	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
-	int	(*reset)(struct vfio_platform_device *vdev);
+	int	(*of_reset)(struct vfio_platform_device *vdev);
 };
 
 typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
@@ -80,7 +81,7 @@ struct vfio_platform_reset_node {
 	struct list_head link;
 	char *compat;
 	struct module *owner;
-	vfio_platform_reset_fn_t reset;
+	vfio_platform_reset_fn_t of_reset;
 };
 
 extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
@@ -103,7 +104,7 @@ extern void vfio_platform_unregister_reset(const char *compat,
 static struct vfio_platform_reset_node __reset ## _node = {	\
 	.owner = THIS_MODULE,					\
 	.compat = __compat,					\
-	.reset = __reset,					\
+	.of_reset = __reset,					\
 };								\
 __vfio_platform_register_reset(&__reset ## _node)
 
-- 
1.8.2.1

[PATCH V4 1/2] vfio, platform: add support for ACPI during probe and reset

[Sinan Kaya]

The code is using the compatible DT string to associate a reset driver with
the actual device itself. The compatible string does not exist on ACPI
based systems. HID is the unique identifier for a device driver instead.

When ACPI is enabled, the change will query the presence of _RST method
and will call it instead of querying an external reset driver.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/vfio/platform/vfio_platform_common.c  | 176 +++++++++++++++++++++-----
 drivers/vfio/platform/vfio_platform_private.h |   7 +-
 2 files changed, 147 insertions(+), 36 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e65b142..528ec04 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/acpi.h>
 #include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -41,7 +42,7 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 		if (!strcmp(iter->compat, compat) &&
 			try_module_get(iter->owner)) {
 			*module = iter->owner;
-			reset_fn = iter->reset;
+			reset_fn = iter->of_reset;
 			break;
 		}
 	}
@@ -49,20 +50,111 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 	return reset_fn;
 }
 
-static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
+
+#ifdef CONFIG_ACPI
+int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
+			     struct device *dev)
 {
-	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
-						&vdev->reset_module);
-	if (!vdev->reset) {
-		request_module("vfio-reset:%s", vdev->compat);
-		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
-							 &vdev->reset_module);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	if (!adev)
+		return -ENODEV;
+
+	vdev->acpihid = acpi_device_hid(adev);
+	if (!vdev->acpihid) {
+		pr_err("VFIO: cannot find ACPI HID for %s\n",
+		       vdev->name);
+		return -ENODEV;
 	}
+	return 0;
+}
+
+static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
+{
+	struct device *dev = vdev->device;
+	acpi_handle handle = ACPI_HANDLE(dev);
+	acpi_status acpi_ret;
+	unsigned long long val;
+
+	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
+	if (ACPI_FAILURE(acpi_ret))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
+{
+	struct device *dev = vdev->device;
+	acpi_handle handle = ACPI_HANDLE(dev);
+
+	if (acpi_has_method(handle, "_RST"))
+		return 0;
+
+	return -EINVAL;
+}
+#else
+int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
+			     struct device *dev)
+{
+	return -EINVAL;
+}
+
+static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
+{
+	return -EINVAL;
+}
+
+static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
+{
+	return -EINVAL;
+}
+#endif
+
+static int vfio_platform_has_reset(struct vfio_platform_device *vdev)
+{
+	if (vdev->acpihid)
+		return vfio_platform_acpi_has_reset(vdev);
+
+	if (vdev->of_reset)
+		return 0;
+
+	return -EINVAL;
+}
+
+static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
+{
+	int rc;
+
+	if (vdev->acpihid)
+		return vfio_platform_acpi_has_reset(vdev);
+
+	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
+						    &vdev->reset_module);
+	if (vdev->of_reset)
+		return 0;
+
+	rc = request_module("vfio-reset:%s", vdev->compat);
+	if (rc)
+		return rc;
+
+	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
+						    &vdev->reset_module);
+	if (vdev->of_reset)
+		return 0;
+
+	return -EINVAL;
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
 {
-	if (vdev->reset)
+	if (vdev->acpihid)
+		return;
+
+	if (vdev->of_reset)
 		module_put(vdev->reset_module);
 }
 
@@ -134,6 +226,20 @@ static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
 	kfree(vdev->regions);
 }
 
+static int vfio_platform_call_reset(struct vfio_platform_device *vdev)
+{
+	if (vdev->of_reset) {
+		dev_info(vdev->device, "reset\n");
+		vdev->of_reset(vdev);
+		return 0;
+	} else if (vdev->acpihid) {
+		return vfio_platform_acpi_call_reset(vdev);
+	}
+
+	dev_warn(vdev->device, "no reset function found!\n");
+	return -EINVAL;
+}
+
 static void vfio_platform_release(void *device_data)
 {
 	struct vfio_platform_device *vdev = device_data;
@@ -141,12 +247,7 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
-		if (vdev->reset) {
-			dev_info(vdev->device, "reset\n");
-			vdev->reset(vdev);
-		} else {
-			dev_warn(vdev->device, "no reset function found!\n");
-		}
+		vfio_platform_call_reset(vdev);
 		vfio_platform_regions_cleanup(vdev);
 		vfio_platform_irq_cleanup(vdev);
 	}
@@ -175,12 +276,9 @@ static int vfio_platform_open(void *device_data)
 		if (ret)
 			goto err_irq;
 
-		if (vdev->reset) {
-			dev_info(vdev->device, "reset\n");
-			vdev->reset(vdev);
-		} else {
-			dev_warn(vdev->device, "no reset function found!\n");
-		}
+		ret = vfio_platform_call_reset(vdev);
+		if (ret)
+			goto err_irq;
 	}
 
 	vdev->refcnt++;
@@ -213,7 +311,7 @@ static long vfio_platform_ioctl(void *device_data,
 		if (info.argsz < minsz)
 			return -EINVAL;
 
-		if (vdev->reset)
+		if (!vfio_platform_has_reset(vdev))
 			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
 		info.flags = vdev->flags;
 		info.num_regions = vdev->num_regions;
@@ -312,10 +410,7 @@ static long vfio_platform_ioctl(void *device_data,
 		return ret;
 
 	} else if (cmd == VFIO_DEVICE_RESET) {
-		if (vdev->reset)
-			return vdev->reset(vdev);
-		else
-			return -EINVAL;
+		return vfio_platform_call_reset(vdev);
 	}
 
 	return -ENOTTY;
@@ -544,6 +639,21 @@ static const struct vfio_device_ops vfio_platform_ops = {
 	.mmap		= vfio_platform_mmap,
 };
 
+int vfio_platform_of_probe(struct vfio_platform_device *vdev,
+			   struct device *dev)
+{
+	int ret;
+
+	ret = device_property_read_string(dev, "compatible",
+					  &vdev->compat);
+	if (ret) {
+		pr_err("VFIO: cannot retrieve compat for %s\n",
+			vdev->name);
+		return ret;
+	}
+	return 0;
+}
+
 int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 			       struct device *dev)
 {
@@ -553,14 +663,14 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 	if (!vdev)
 		return -EINVAL;
 
-	ret = device_property_read_string(dev, "compatible", &vdev->compat);
-	if (ret) {
-		pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
-		return -EINVAL;
-	}
+	ret = vfio_platform_acpi_probe(vdev, dev);
+	if (ret)
+		ret = vfio_platform_of_probe(vdev, dev);
 
-	vdev->device = dev;
+	if (ret)
+		return ret;
 
+	vdev->device = dev;
 	group = iommu_group_get(dev);
 	if (!group) {
 		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
@@ -611,7 +721,7 @@ void vfio_platform_unregister_reset(const char *compat,
 
 	mutex_lock(&driver_lock);
 	list_for_each_entry_safe(iter, temp, &reset_list, link) {
-		if (!strcmp(iter->compat, compat) && (iter->reset == fn)) {
+		if (!strcmp(iter->compat, compat) && (iter->of_reset == fn)) {
 			list_del(&iter->link);
 			break;
 		}
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 42816dd..ba9e4f8 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -58,6 +58,7 @@ struct vfio_platform_device {
 	struct mutex			igate;
 	struct module			*parent_module;
 	const char			*compat;
+	const char			*acpihid;
 	struct module			*reset_module;
 	struct device			*device;
 
@@ -71,7 +72,7 @@ struct vfio_platform_device {
 	struct resource*
 		(*get_resource)(struct vfio_platform_device *vdev, int i);
 	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
-	int	(*reset)(struct vfio_platform_device *vdev);
+	int	(*of_reset)(struct vfio_platform_device *vdev);
 };
 
 typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
@@ -80,7 +81,7 @@ struct vfio_platform_reset_node {
 	struct list_head link;
 	char *compat;
 	struct module *owner;
-	vfio_platform_reset_fn_t reset;
+	vfio_platform_reset_fn_t of_reset;
 };
 
 extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
@@ -103,7 +104,7 @@ extern void vfio_platform_unregister_reset(const char *compat,
 static struct vfio_platform_reset_node __reset ## _node = {	\
 	.owner = THIS_MODULE,					\
 	.compat = __compat,					\
-	.reset = __reset,					\
+	.of_reset = __reset,					\
 };								\
 __vfio_platform_register_reset(&__reset ## _node)
 
-- 
1.8.2.1

Re: [PATCH V4 1/2] vfio, platform: add support for ACPI during probe and reset

[Eric Auger]

Hi Sinan,
On 05/01/2016 11:07 PM, Sinan Kaya wrote:
> The code is using the compatible DT string to associate a reset driver with
> the actual device itself. The compatible string does not exist on ACPI
> based systems. HID is the unique identifier for a device driver instead.
> 
> When ACPI is enabled, the change will query the presence of _RST method
> and will call it instead of querying an external reset driver.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 176 +++++++++++++++++++++-----
>  drivers/vfio/platform/vfio_platform_private.h |   7 +-
>  2 files changed, 147 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index e65b142..528ec04 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/device.h>
> +#include <linux/acpi.h>
>  #include <linux/iommu.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -41,7 +42,7 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>  		if (!strcmp(iter->compat, compat) &&
>  			try_module_get(iter->owner)) {
>  			*module = iter->owner;
> -			reset_fn = iter->reset;
> +			reset_fn = iter->of_reset;
>  			break;
>  		}
>  	}
> @@ -49,20 +50,111 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>  	return reset_fn;
>  }
>  
> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
> +
> +#ifdef CONFIG_ACPI
> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> +			     struct device *dev)
>  {
> -	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
> -						&vdev->reset_module);
> -	if (!vdev->reset) {
> -		request_module("vfio-reset:%s", vdev->compat);
> -		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
> -							 &vdev->reset_module);
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> +	if (acpi_disabled)
> +		return -ENODEV;
> +
> +	if (!adev)
> +		return -ENODEV;
> +
> +	vdev->acpihid = acpi_device_hid(adev);
> +	if (!vdev->acpihid) {
> +		pr_err("VFIO: cannot find ACPI HID for %s\n",
> +		       vdev->name);
> +		return -ENODEV;
>  	}
> +	return 0;
> +}
> +
> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
> +{
> +	struct device *dev = vdev->device;
> +	acpi_handle handle = ACPI_HANDLE(dev);
> +	acpi_status acpi_ret;
> +	unsigned long long val;
> +
> +	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
> +	if (ACPI_FAILURE(acpi_ret))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> +{
> +	struct device *dev = vdev->device;
> +	acpi_handle handle = ACPI_HANDLE(dev);
> +
> +	if (acpi_has_method(handle, "_RST"))
> +		return 0;
Can't you directly return acpi_has_method(handle, "_RST"), hence return
a boolean?
> +
> +	return -EINVAL;
> +}
> +#else
> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> +			     struct device *dev)
> +{
> +	return -EINVAL;
> +}
> +
> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
> +{
> +	return -EINVAL;
> +}
> +
> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
> +static int vfio_platform_has_reset(struct vfio_platform_device *vdev)
> +{
return a boolean?
> +	if (vdev->acpihid)
> +		return vfio_platform_acpi_has_reset(vdev);
> +
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +
> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
> +{
> +	int rc;
> +
> +	if (vdev->acpihid)
> +		return vfio_platform_acpi_has_reset(vdev);
> +
> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> +						    &vdev->reset_module);
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	rc = request_module("vfio-reset:%s", vdev->compat);
> +	if (rc)
> +		return rc;
> +
> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> +						    &vdev->reset_module);
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	return -EINVAL;
>  }
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>  {
> -	if (vdev->reset)
> +	if (vdev->acpihid)
> +		return;
> +
> +	if (vdev->of_reset)
>  		module_put(vdev->reset_module);
>  }
>  
> @@ -134,6 +226,20 @@ static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
>  	kfree(vdev->regions);
>  }
>  
> +static int vfio_platform_call_reset(struct vfio_platform_device *vdev)
> +{
> +	if (vdev->of_reset) {
> +		dev_info(vdev->device, "reset\n");
> +		vdev->of_reset(vdev);
return vdev->of_reset(vdev) to align with acpi reset behavior.
> +		return 0;
> +	} else if (vdev->acpihid) {
> +		return vfio_platform_acpi_call_reset(vdev);
I think it would make sense to have the same dev_info traces for dt and
acpi.
> +	}
> +
> +	dev_warn(vdev->device, "no reset function found!\n");
> +	return -EINVAL;
> +}
> +
>  static void vfio_platform_release(void *device_data)
>  {
>  	struct vfio_platform_device *vdev = device_data;
> @@ -141,12 +247,7 @@ static void vfio_platform_release(void *device_data)
>  	mutex_lock(&driver_lock);
>  
>  	if (!(--vdev->refcnt)) {
> -		if (vdev->reset) {
> -			dev_info(vdev->device, "reset\n");
> -			vdev->reset(vdev);
> -		} else {
> -			dev_warn(vdev->device, "no reset function found!\n");
> -		}
> +		vfio_platform_call_reset(vdev);
>  		vfio_platform_regions_cleanup(vdev);
>  		vfio_platform_irq_cleanup(vdev);
>  	}
> @@ -175,12 +276,9 @@ static int vfio_platform_open(void *device_data)
>  		if (ret)
>  			goto err_irq;
>  
> -		if (vdev->reset) {
> -			dev_info(vdev->device, "reset\n");
> -			vdev->reset(vdev);
> -		} else {
> -			dev_warn(vdev->device, "no reset function found!\n");
> -		}
> +		ret = vfio_platform_call_reset(vdev);
> +		if (ret)
> +			goto err_irq;
This change should be in next patch since if you fail finding a reset
function, you clean things up, mandating a reset function to exist.

Also in case the reset function fails you change the behavior which is
not detailed in the commit msg.
>  	}
>  
>  	vdev->refcnt++;
> @@ -213,7 +311,7 @@ static long vfio_platform_ioctl(void *device_data,
>  		if (info.argsz < minsz)
>  			return -EINVAL;
>  
> -		if (vdev->reset)
> +		if (!vfio_platform_has_reset(vdev))
>  			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
>  		info.flags = vdev->flags;
>  		info.num_regions = vdev->num_regions;
> @@ -312,10 +410,7 @@ static long vfio_platform_ioctl(void *device_data,
>  		return ret;
>  
>  	} else if (cmd == VFIO_DEVICE_RESET) {
> -		if (vdev->reset)
> -			return vdev->reset(vdev);
> -		else
> -			return -EINVAL;
> +		return vfio_platform_call_reset(vdev);
Here you also change the behavior: before, in case the dt reset failed
we returned an error. Now we return 0.

To me this patch would deserve to be split into 2 parts: ACPI probing
without reset and then ACPI reset. In case you change the behavior of
existing dt reset, please put that in a separate patch.

Best Regards

Eric
>  	}
>  
>  	return -ENOTTY;
> @@ -544,6 +639,21 @@ static const struct vfio_device_ops vfio_platform_ops = {
>  	.mmap		= vfio_platform_mmap,
>  };
>  
> +int vfio_platform_of_probe(struct vfio_platform_device *vdev,
> +			   struct device *dev)
> +{
> +	int ret;
> +
> +	ret = device_property_read_string(dev, "compatible",
> +					  &vdev->compat);
> +	if (ret) {
> +		pr_err("VFIO: cannot retrieve compat for %s\n",
> +			vdev->name);
> +		return ret;
> +	}
> +	return 0;
> +}
> +
>  int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  			       struct device *dev)
>  {
> @@ -553,14 +663,14 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  	if (!vdev)
>  		return -EINVAL;
>  
> -	ret = device_property_read_string(dev, "compatible", &vdev->compat);
> -	if (ret) {
> -		pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
> -		return -EINVAL;
> -	}
> +	ret = vfio_platform_acpi_probe(vdev, dev);
> +	if (ret)
> +		ret = vfio_platform_of_probe(vdev, dev);
>  
> -	vdev->device = dev;
> +	if (ret)
> +		return ret;
>  
> +	vdev->device = dev;
>  	group = iommu_group_get(dev);
>  	if (!group) {
>  		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
> @@ -611,7 +721,7 @@ void vfio_platform_unregister_reset(const char *compat,
>  
>  	mutex_lock(&driver_lock);
>  	list_for_each_entry_safe(iter, temp, &reset_list, link) {
> -		if (!strcmp(iter->compat, compat) && (iter->reset == fn)) {
> +		if (!strcmp(iter->compat, compat) && (iter->of_reset == fn)) {
>  			list_del(&iter->link);
>  			break;
>  		}
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 42816dd..ba9e4f8 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -58,6 +58,7 @@ struct vfio_platform_device {
>  	struct mutex			igate;
>  	struct module			*parent_module;
>  	const char			*compat;
> +	const char			*acpihid;
>  	struct module			*reset_module;
>  	struct device			*device;
>  
> @@ -71,7 +72,7 @@ struct vfio_platform_device {
>  	struct resource*
>  		(*get_resource)(struct vfio_platform_device *vdev, int i);
>  	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
> -	int	(*reset)(struct vfio_platform_device *vdev);
> +	int	(*of_reset)(struct vfio_platform_device *vdev);
>  };
>  
>  typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
> @@ -80,7 +81,7 @@ struct vfio_platform_reset_node {
>  	struct list_head link;
>  	char *compat;
>  	struct module *owner;
> -	vfio_platform_reset_fn_t reset;
> +	vfio_platform_reset_fn_t of_reset;
>  };
>  
>  extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
> @@ -103,7 +104,7 @@ extern void vfio_platform_unregister_reset(const char *compat,
>  static struct vfio_platform_reset_node __reset ## _node = {	\
>  	.owner = THIS_MODULE,					\
>  	.compat = __compat,					\
> -	.reset = __reset,					\
> +	.of_reset = __reset,					\
>  };								\
>  __vfio_platform_register_reset(&__reset ## _node)
>  
> 

Re: [PATCH V4 1/2] vfio, platform: add support for ACPI during probe and reset

[Eric Auger]

Hi Sinan,
On 05/01/2016 11:07 PM, Sinan Kaya wrote:
> The code is using the compatible DT string to associate a reset driver with
> the actual device itself. The compatible string does not exist on ACPI
> based systems. HID is the unique identifier for a device driver instead.
> 
> When ACPI is enabled, the change will query the presence of _RST method
> and will call it instead of querying an external reset driver.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 176 +++++++++++++++++++++-----
>  drivers/vfio/platform/vfio_platform_private.h |   7 +-
>  2 files changed, 147 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index e65b142..528ec04 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/device.h>
> +#include <linux/acpi.h>
>  #include <linux/iommu.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -41,7 +42,7 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>  		if (!strcmp(iter->compat, compat) &&
>  			try_module_get(iter->owner)) {
>  			*module = iter->owner;
> -			reset_fn = iter->reset;
> +			reset_fn = iter->of_reset;
>  			break;
>  		}
>  	}
> @@ -49,20 +50,111 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>  	return reset_fn;
>  }
>  
> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
> +
> +#ifdef CONFIG_ACPI
> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> +			     struct device *dev)
>  {
> -	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
> -						&vdev->reset_module);
> -	if (!vdev->reset) {
> -		request_module("vfio-reset:%s", vdev->compat);
> -		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
> -							 &vdev->reset_module);
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> +	if (acpi_disabled)
> +		return -ENODEV;
> +
> +	if (!adev)
> +		return -ENODEV;
> +
> +	vdev->acpihid = acpi_device_hid(adev);
> +	if (!vdev->acpihid) {
> +		pr_err("VFIO: cannot find ACPI HID for %s\n",
> +		       vdev->name);
> +		return -ENODEV;
>  	}
> +	return 0;
> +}
> +
> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
> +{
> +	struct device *dev = vdev->device;
> +	acpi_handle handle = ACPI_HANDLE(dev);
> +	acpi_status acpi_ret;
> +	unsigned long long val;
> +
> +	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
> +	if (ACPI_FAILURE(acpi_ret))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> +{
> +	struct device *dev = vdev->device;
> +	acpi_handle handle = ACPI_HANDLE(dev);
> +
> +	if (acpi_has_method(handle, "_RST"))
> +		return 0;
Can't you directly return acpi_has_method(handle, "_RST"), hence return
a boolean?
> +
> +	return -EINVAL;
> +}
> +#else
> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> +			     struct device *dev)
> +{
> +	return -EINVAL;
> +}
> +
> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
> +{
> +	return -EINVAL;
> +}
> +
> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
> +static int vfio_platform_has_reset(struct vfio_platform_device *vdev)
> +{
return a boolean?
> +	if (vdev->acpihid)
> +		return vfio_platform_acpi_has_reset(vdev);
> +
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +
> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
> +{
> +	int rc;
> +
> +	if (vdev->acpihid)
> +		return vfio_platform_acpi_has_reset(vdev);
> +
> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> +						    &vdev->reset_module);
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	rc = request_module("vfio-reset:%s", vdev->compat);
> +	if (rc)
> +		return rc;
> +
> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> +						    &vdev->reset_module);
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	return -EINVAL;
>  }
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>  {
> -	if (vdev->reset)
> +	if (vdev->acpihid)
> +		return;
> +
> +	if (vdev->of_reset)
>  		module_put(vdev->reset_module);
>  }
>  
> @@ -134,6 +226,20 @@ static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
>  	kfree(vdev->regions);
>  }
>  
> +static int vfio_platform_call_reset(struct vfio_platform_device *vdev)
> +{
> +	if (vdev->of_reset) {
> +		dev_info(vdev->device, "reset\n");
> +		vdev->of_reset(vdev);
return vdev->of_reset(vdev) to align with acpi reset behavior.
> +		return 0;
> +	} else if (vdev->acpihid) {
> +		return vfio_platform_acpi_call_reset(vdev);
I think it would make sense to have the same dev_info traces for dt and
acpi.
> +	}
> +
> +	dev_warn(vdev->device, "no reset function found!\n");
> +	return -EINVAL;
> +}
> +
>  static void vfio_platform_release(void *device_data)
>  {
>  	struct vfio_platform_device *vdev = device_data;
> @@ -141,12 +247,7 @@ static void vfio_platform_release(void *device_data)
>  	mutex_lock(&driver_lock);
>  
>  	if (!(--vdev->refcnt)) {
> -		if (vdev->reset) {
> -			dev_info(vdev->device, "reset\n");
> -			vdev->reset(vdev);
> -		} else {
> -			dev_warn(vdev->device, "no reset function found!\n");
> -		}
> +		vfio_platform_call_reset(vdev);
>  		vfio_platform_regions_cleanup(vdev);
>  		vfio_platform_irq_cleanup(vdev);
>  	}
> @@ -175,12 +276,9 @@ static int vfio_platform_open(void *device_data)
>  		if (ret)
>  			goto err_irq;
>  
> -		if (vdev->reset) {
> -			dev_info(vdev->device, "reset\n");
> -			vdev->reset(vdev);
> -		} else {
> -			dev_warn(vdev->device, "no reset function found!\n");
> -		}
> +		ret = vfio_platform_call_reset(vdev);
> +		if (ret)
> +			goto err_irq;
This change should be in next patch since if you fail finding a reset
function, you clean things up, mandating a reset function to exist.

Also in case the reset function fails you change the behavior which is
not detailed in the commit msg.
>  	}
>  
>  	vdev->refcnt++;
> @@ -213,7 +311,7 @@ static long vfio_platform_ioctl(void *device_data,
>  		if (info.argsz < minsz)
>  			return -EINVAL;
>  
> -		if (vdev->reset)
> +		if (!vfio_platform_has_reset(vdev))
>  			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
>  		info.flags = vdev->flags;
>  		info.num_regions = vdev->num_regions;
> @@ -312,10 +410,7 @@ static long vfio_platform_ioctl(void *device_data,
>  		return ret;
>  
>  	} else if (cmd == VFIO_DEVICE_RESET) {
> -		if (vdev->reset)
> -			return vdev->reset(vdev);
> -		else
> -			return -EINVAL;
> +		return vfio_platform_call_reset(vdev);
Here you also change the behavior: before, in case the dt reset failed
we returned an error. Now we return 0.

To me this patch would deserve to be split into 2 parts: ACPI probing
without reset and then ACPI reset. In case you change the behavior of
existing dt reset, please put that in a separate patch.

Best Regards

Eric
>  	}
>  
>  	return -ENOTTY;
> @@ -544,6 +639,21 @@ static const struct vfio_device_ops vfio_platform_ops = {
>  	.mmap		= vfio_platform_mmap,
>  };
>  
> +int vfio_platform_of_probe(struct vfio_platform_device *vdev,
> +			   struct device *dev)
> +{
> +	int ret;
> +
> +	ret = device_property_read_string(dev, "compatible",
> +					  &vdev->compat);
> +	if (ret) {
> +		pr_err("VFIO: cannot retrieve compat for %s\n",
> +			vdev->name);
> +		return ret;
> +	}
> +	return 0;
> +}
> +
>  int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  			       struct device *dev)
>  {
> @@ -553,14 +663,14 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  	if (!vdev)
>  		return -EINVAL;
>  
> -	ret = device_property_read_string(dev, "compatible", &vdev->compat);
> -	if (ret) {
> -		pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
> -		return -EINVAL;
> -	}
> +	ret = vfio_platform_acpi_probe(vdev, dev);
> +	if (ret)
> +		ret = vfio_platform_of_probe(vdev, dev);
>  
> -	vdev->device = dev;
> +	if (ret)
> +		return ret;
>  
> +	vdev->device = dev;
>  	group = iommu_group_get(dev);
>  	if (!group) {
>  		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
> @@ -611,7 +721,7 @@ void vfio_platform_unregister_reset(const char *compat,
>  
>  	mutex_lock(&driver_lock);
>  	list_for_each_entry_safe(iter, temp, &reset_list, link) {
> -		if (!strcmp(iter->compat, compat) && (iter->reset == fn)) {
> +		if (!strcmp(iter->compat, compat) && (iter->of_reset == fn)) {
>  			list_del(&iter->link);
>  			break;
>  		}
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 42816dd..ba9e4f8 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -58,6 +58,7 @@ struct vfio_platform_device {
>  	struct mutex			igate;
>  	struct module			*parent_module;
>  	const char			*compat;
> +	const char			*acpihid;
>  	struct module			*reset_module;
>  	struct device			*device;
>  
> @@ -71,7 +72,7 @@ struct vfio_platform_device {
>  	struct resource*
>  		(*get_resource)(struct vfio_platform_device *vdev, int i);
>  	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
> -	int	(*reset)(struct vfio_platform_device *vdev);
> +	int	(*of_reset)(struct vfio_platform_device *vdev);
>  };
>  
>  typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
> @@ -80,7 +81,7 @@ struct vfio_platform_reset_node {
>  	struct list_head link;
>  	char *compat;
>  	struct module *owner;
> -	vfio_platform_reset_fn_t reset;
> +	vfio_platform_reset_fn_t of_reset;
>  };
>  
>  extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
> @@ -103,7 +104,7 @@ extern void vfio_platform_unregister_reset(const char *compat,
>  static struct vfio_platform_reset_node __reset ## _node = {	\
>  	.owner = THIS_MODULE,					\
>  	.compat = __compat,					\
> -	.reset = __reset,					\
> +	.of_reset = __reset,					\
>  };								\
>  __vfio_platform_register_reset(&__reset ## _node)
>  
> 

[PATCH V4 1/2] vfio, platform: add support for ACPI during probe and reset

[Eric Auger]

Hi Sinan,
On 05/01/2016 11:07 PM, Sinan Kaya wrote:
> The code is using the compatible DT string to associate a reset driver with
> the actual device itself. The compatible string does not exist on ACPI
> based systems. HID is the unique identifier for a device driver instead.
> 
> When ACPI is enabled, the change will query the presence of _RST method
> and will call it instead of querying an external reset driver.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 176 +++++++++++++++++++++-----
>  drivers/vfio/platform/vfio_platform_private.h |   7 +-
>  2 files changed, 147 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index e65b142..528ec04 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/device.h>
> +#include <linux/acpi.h>
>  #include <linux/iommu.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -41,7 +42,7 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>  		if (!strcmp(iter->compat, compat) &&
>  			try_module_get(iter->owner)) {
>  			*module = iter->owner;
> -			reset_fn = iter->reset;
> +			reset_fn = iter->of_reset;
>  			break;
>  		}
>  	}
> @@ -49,20 +50,111 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>  	return reset_fn;
>  }
>  
> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
> +
> +#ifdef CONFIG_ACPI
> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> +			     struct device *dev)
>  {
> -	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
> -						&vdev->reset_module);
> -	if (!vdev->reset) {
> -		request_module("vfio-reset:%s", vdev->compat);
> -		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
> -							 &vdev->reset_module);
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> +	if (acpi_disabled)
> +		return -ENODEV;
> +
> +	if (!adev)
> +		return -ENODEV;
> +
> +	vdev->acpihid = acpi_device_hid(adev);
> +	if (!vdev->acpihid) {
> +		pr_err("VFIO: cannot find ACPI HID for %s\n",
> +		       vdev->name);
> +		return -ENODEV;
>  	}
> +	return 0;
> +}
> +
> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
> +{
> +	struct device *dev = vdev->device;
> +	acpi_handle handle = ACPI_HANDLE(dev);
> +	acpi_status acpi_ret;
> +	unsigned long long val;
> +
> +	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
> +	if (ACPI_FAILURE(acpi_ret))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> +{
> +	struct device *dev = vdev->device;
> +	acpi_handle handle = ACPI_HANDLE(dev);
> +
> +	if (acpi_has_method(handle, "_RST"))
> +		return 0;
Can't you directly return acpi_has_method(handle, "_RST"), hence return
a boolean?
> +
> +	return -EINVAL;
> +}
> +#else
> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> +			     struct device *dev)
> +{
> +	return -EINVAL;
> +}
> +
> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
> +{
> +	return -EINVAL;
> +}
> +
> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
> +static int vfio_platform_has_reset(struct vfio_platform_device *vdev)
> +{
return a boolean?
> +	if (vdev->acpihid)
> +		return vfio_platform_acpi_has_reset(vdev);
> +
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +
> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
> +{
> +	int rc;
> +
> +	if (vdev->acpihid)
> +		return vfio_platform_acpi_has_reset(vdev);
> +
> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> +						    &vdev->reset_module);
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	rc = request_module("vfio-reset:%s", vdev->compat);
> +	if (rc)
> +		return rc;
> +
> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> +						    &vdev->reset_module);
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	return -EINVAL;
>  }
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>  {
> -	if (vdev->reset)
> +	if (vdev->acpihid)
> +		return;
> +
> +	if (vdev->of_reset)
>  		module_put(vdev->reset_module);
>  }
>  
> @@ -134,6 +226,20 @@ static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
>  	kfree(vdev->regions);
>  }
>  
> +static int vfio_platform_call_reset(struct vfio_platform_device *vdev)
> +{
> +	if (vdev->of_reset) {
> +		dev_info(vdev->device, "reset\n");
> +		vdev->of_reset(vdev);
return vdev->of_reset(vdev) to align with acpi reset behavior.
> +		return 0;
> +	} else if (vdev->acpihid) {
> +		return vfio_platform_acpi_call_reset(vdev);
I think it would make sense to have the same dev_info traces for dt and
acpi.
> +	}
> +
> +	dev_warn(vdev->device, "no reset function found!\n");
> +	return -EINVAL;
> +}
> +
>  static void vfio_platform_release(void *device_data)
>  {
>  	struct vfio_platform_device *vdev = device_data;
> @@ -141,12 +247,7 @@ static void vfio_platform_release(void *device_data)
>  	mutex_lock(&driver_lock);
>  
>  	if (!(--vdev->refcnt)) {
> -		if (vdev->reset) {
> -			dev_info(vdev->device, "reset\n");
> -			vdev->reset(vdev);
> -		} else {
> -			dev_warn(vdev->device, "no reset function found!\n");
> -		}
> +		vfio_platform_call_reset(vdev);
>  		vfio_platform_regions_cleanup(vdev);
>  		vfio_platform_irq_cleanup(vdev);
>  	}
> @@ -175,12 +276,9 @@ static int vfio_platform_open(void *device_data)
>  		if (ret)
>  			goto err_irq;
>  
> -		if (vdev->reset) {
> -			dev_info(vdev->device, "reset\n");
> -			vdev->reset(vdev);
> -		} else {
> -			dev_warn(vdev->device, "no reset function found!\n");
> -		}
> +		ret = vfio_platform_call_reset(vdev);
> +		if (ret)
> +			goto err_irq;
This change should be in next patch since if you fail finding a reset
function, you clean things up, mandating a reset function to exist.

Also in case the reset function fails you change the behavior which is
not detailed in the commit msg.
>  	}
>  
>  	vdev->refcnt++;
> @@ -213,7 +311,7 @@ static long vfio_platform_ioctl(void *device_data,
>  		if (info.argsz < minsz)
>  			return -EINVAL;
>  
> -		if (vdev->reset)
> +		if (!vfio_platform_has_reset(vdev))
>  			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
>  		info.flags = vdev->flags;
>  		info.num_regions = vdev->num_regions;
> @@ -312,10 +410,7 @@ static long vfio_platform_ioctl(void *device_data,
>  		return ret;
>  
>  	} else if (cmd == VFIO_DEVICE_RESET) {
> -		if (vdev->reset)
> -			return vdev->reset(vdev);
> -		else
> -			return -EINVAL;
> +		return vfio_platform_call_reset(vdev);
Here you also change the behavior: before, in case the dt reset failed
we returned an error. Now we return 0.

To me this patch would deserve to be split into 2 parts: ACPI probing
without reset and then ACPI reset. In case you change the behavior of
existing dt reset, please put that in a separate patch.

Best Regards

Eric
>  	}
>  
>  	return -ENOTTY;
> @@ -544,6 +639,21 @@ static const struct vfio_device_ops vfio_platform_ops = {
>  	.mmap		= vfio_platform_mmap,
>  };
>  
> +int vfio_platform_of_probe(struct vfio_platform_device *vdev,
> +			   struct device *dev)
> +{
> +	int ret;
> +
> +	ret = device_property_read_string(dev, "compatible",
> +					  &vdev->compat);
> +	if (ret) {
> +		pr_err("VFIO: cannot retrieve compat for %s\n",
> +			vdev->name);
> +		return ret;
> +	}
> +	return 0;
> +}
> +
>  int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  			       struct device *dev)
>  {
> @@ -553,14 +663,14 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  	if (!vdev)
>  		return -EINVAL;
>  
> -	ret = device_property_read_string(dev, "compatible", &vdev->compat);
> -	if (ret) {
> -		pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
> -		return -EINVAL;
> -	}
> +	ret = vfio_platform_acpi_probe(vdev, dev);
> +	if (ret)
> +		ret = vfio_platform_of_probe(vdev, dev);
>  
> -	vdev->device = dev;
> +	if (ret)
> +		return ret;
>  
> +	vdev->device = dev;
>  	group = iommu_group_get(dev);
>  	if (!group) {
>  		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
> @@ -611,7 +721,7 @@ void vfio_platform_unregister_reset(const char *compat,
>  
>  	mutex_lock(&driver_lock);
>  	list_for_each_entry_safe(iter, temp, &reset_list, link) {
> -		if (!strcmp(iter->compat, compat) && (iter->reset == fn)) {
> +		if (!strcmp(iter->compat, compat) && (iter->of_reset == fn)) {
>  			list_del(&iter->link);
>  			break;
>  		}
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 42816dd..ba9e4f8 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -58,6 +58,7 @@ struct vfio_platform_device {
>  	struct mutex			igate;
>  	struct module			*parent_module;
>  	const char			*compat;
> +	const char			*acpihid;
>  	struct module			*reset_module;
>  	struct device			*device;
>  
> @@ -71,7 +72,7 @@ struct vfio_platform_device {
>  	struct resource*
>  		(*get_resource)(struct vfio_platform_device *vdev, int i);
>  	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
> -	int	(*reset)(struct vfio_platform_device *vdev);
> +	int	(*of_reset)(struct vfio_platform_device *vdev);
>  };
>  
>  typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
> @@ -80,7 +81,7 @@ struct vfio_platform_reset_node {
>  	struct list_head link;
>  	char *compat;
>  	struct module *owner;
> -	vfio_platform_reset_fn_t reset;
> +	vfio_platform_reset_fn_t of_reset;
>  };
>  
>  extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
> @@ -103,7 +104,7 @@ extern void vfio_platform_unregister_reset(const char *compat,
>  static struct vfio_platform_reset_node __reset ## _node = {	\
>  	.owner = THIS_MODULE,					\
>  	.compat = __compat,					\
> -	.reset = __reset,					\
> +	.of_reset = __reset,					\
>  };								\
>  __vfio_platform_register_reset(&__reset ## _node)
>  
> 

Re: [PATCH V4 1/2] vfio, platform: add support for ACPI during probe and reset

[Sinan Kaya]

On 5/9/2016 11:47 AM, Eric Auger wrote:
> Hi Sinan,
> On 05/01/2016 11:07 PM, Sinan Kaya wrote:
>> The code is using the compatible DT string to associate a reset driver with
>> the actual device itself. The compatible string does not exist on ACPI
>> based systems. HID is the unique identifier for a device driver instead.
>>
>> When ACPI is enabled, the change will query the presence of _RST method
>> and will call it instead of querying an external reset driver.
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c  | 176 +++++++++++++++++++++-----
>>  drivers/vfio/platform/vfio_platform_private.h |   7 +-
>>  2 files changed, 147 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index e65b142..528ec04 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -13,6 +13,7 @@
>>   */
>>  
>>  #include <linux/device.h>
>> +#include <linux/acpi.h>
>>  #include <linux/iommu.h>
>>  #include <linux/module.h>
>>  #include <linux/mutex.h>
>> @@ -41,7 +42,7 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>>  		if (!strcmp(iter->compat, compat) &&
>>  			try_module_get(iter->owner)) {
>>  			*module = iter->owner;
>> -			reset_fn = iter->reset;
>> +			reset_fn = iter->of_reset;
>>  			break;
>>  		}
>>  	}
>> @@ -49,20 +50,111 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>>  	return reset_fn;
>>  }
>>  
>> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>> +
>> +#ifdef CONFIG_ACPI
>> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>> +			     struct device *dev)
>>  {
>> -	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
>> -						&vdev->reset_module);
>> -	if (!vdev->reset) {
>> -		request_module("vfio-reset:%s", vdev->compat);
>> -		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
>> -							 &vdev->reset_module);
>> +	struct acpi_device *adev = ACPI_COMPANION(dev);
>> +
>> +	if (acpi_disabled)
>> +		return -ENODEV;
>> +
>> +	if (!adev)
>> +		return -ENODEV;
>> +
>> +	vdev->acpihid = acpi_device_hid(adev);
>> +	if (!vdev->acpihid) {
>> +		pr_err("VFIO: cannot find ACPI HID for %s\n",
>> +		       vdev->name);
>> +		return -ENODEV;
>>  	}
>> +	return 0;
>> +}
>> +
>> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
>> +{
>> +	struct device *dev = vdev->device;
>> +	acpi_handle handle = ACPI_HANDLE(dev);
>> +	acpi_status acpi_ret;
>> +	unsigned long long val;
>> +
>> +	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
>> +	if (ACPI_FAILURE(acpi_ret))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
>> +{
>> +	struct device *dev = vdev->device;
>> +	acpi_handle handle = ACPI_HANDLE(dev);
>> +
>> +	if (acpi_has_method(handle, "_RST"))
>> +		return 0;
> Can't you directly return acpi_has_method(handle, "_RST"), hence return
> a boolean?

Makes sense. I'll do that on the next post. 

>> +
>> +	return -EINVAL;
>> +}
>> +#else
>> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>> +			     struct device *dev)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
>> +{
>> +	return -EINVAL;
>> +}
>> +#endif
>> +
>> +static int vfio_platform_has_reset(struct vfio_platform_device *vdev)
>> +{
> return a boolean?

OK

>> +	if (vdev->acpihid)
>> +		return vfio_platform_acpi_has_reset(vdev);
>> +
>> +	if (vdev->of_reset)
>> +		return 0;
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>> +{
>> +	int rc;
>> +
>> +	if (vdev->acpihid)
>> +		return vfio_platform_acpi_has_reset(vdev);
>> +
>> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>> +						    &vdev->reset_module);
>> +	if (vdev->of_reset)
>> +		return 0;
>> +
>> +	rc = request_module("vfio-reset:%s", vdev->compat);
>> +	if (rc)
>> +		return rc;
>> +
>> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>> +						    &vdev->reset_module);
>> +	if (vdev->of_reset)
>> +		return 0;
>> +
>> +	return -EINVAL;
>>  }
>>  
>>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>  {
>> -	if (vdev->reset)
>> +	if (vdev->acpihid)
>> +		return;
>> +
>> +	if (vdev->of_reset)
>>  		module_put(vdev->reset_module);
>>  }
>>  
>> @@ -134,6 +226,20 @@ static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
>>  	kfree(vdev->regions);
>>  }
>>  
>> +static int vfio_platform_call_reset(struct vfio_platform_device *vdev)
>> +{
>> +	if (vdev->of_reset) {
>> +		dev_info(vdev->device, "reset\n");
>> +		vdev->of_reset(vdev);
> return vdev->of_reset(vdev) to align with acpi reset behavior.

OK. I didn't realize it was returning a value. 

>> +		return 0;
>> +	} else if (vdev->acpihid) {
>> +		return vfio_platform_acpi_call_reset(vdev);
> I think it would make sense to have the same dev_info traces for dt and
> acpi.
>> +	}
>> +
>> +	dev_warn(vdev->device, "no reset function found!\n");
>> +	return -EINVAL;
>> +}
>> +
>>  static void vfio_platform_release(void *device_data)
>>  {
>>  	struct vfio_platform_device *vdev = device_data;
>> @@ -141,12 +247,7 @@ static void vfio_platform_release(void *device_data)
>>  	mutex_lock(&driver_lock);
>>  
>>  	if (!(--vdev->refcnt)) {
>> -		if (vdev->reset) {
>> -			dev_info(vdev->device, "reset\n");
>> -			vdev->reset(vdev);
>> -		} else {
>> -			dev_warn(vdev->device, "no reset function found!\n");
>> -		}
>> +		vfio_platform_call_reset(vdev);
>>  		vfio_platform_regions_cleanup(vdev);
>>  		vfio_platform_irq_cleanup(vdev);
>>  	}
>> @@ -175,12 +276,9 @@ static int vfio_platform_open(void *device_data)
>>  		if (ret)
>>  			goto err_irq;
>>  
>> -		if (vdev->reset) {
>> -			dev_info(vdev->device, "reset\n");
>> -			vdev->reset(vdev);
>> -		} else {
>> -			dev_warn(vdev->device, "no reset function found!\n");
>> -		}
>> +		ret = vfio_platform_call_reset(vdev);
>> +		if (ret)
>> +			goto err_irq;
> This change should be in next patch since if you fail finding a reset
> function, you clean things up, mandating a reset function to exist.
> 
> Also in case the reset function fails you change the behavior which is
> not detailed in the commit msg.

I admitted this on another patch. Yes, I'll fix this.

>>  	}
>>  
>>  	vdev->refcnt++;
>> @@ -213,7 +311,7 @@ static long vfio_platform_ioctl(void *device_data,
>>  		if (info.argsz < minsz)
>>  			return -EINVAL;
>>  
>> -		if (vdev->reset)
>> +		if (!vfio_platform_has_reset(vdev))
>>  			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
>>  		info.flags = vdev->flags;
>>  		info.num_regions = vdev->num_regions;
>> @@ -312,10 +410,7 @@ static long vfio_platform_ioctl(void *device_data,
>>  		return ret;
>>  
>>  	} else if (cmd == VFIO_DEVICE_RESET) {
>> -		if (vdev->reset)
>> -			return vdev->reset(vdev);
>> -		else
>> -			return -EINVAL;
>> +		return vfio_platform_call_reset(vdev);
> Here you also change the behavior: before, in case the dt reset failed
> we returned an error. Now we return 0.

Yes, I'll fix it. 
> 
> To me this patch would deserve to be split into 2 parts: ACPI probing
> without reset and then ACPI reset. In case you change the behavior of
> existing dt reset, please put that in a separate patch.

Sure, I'll move vfio_platform_probe_common changes to a seperate patch.
I'll try to break it as much as I can before the post.

> 
> Best Regards
> 
> Eric
>>  	}
>>  
>>  	return -ENOTTY;
>> @@ -544,6 +639,21 @@ static const struct vfio_device_ops vfio_platform_ops = {
>>  	.mmap		= vfio_platform_mmap,
>>  };
>>  
>> +int vfio_platform_of_probe(struct vfio_platform_device *vdev,
>> +			   struct device *dev)
>> +{
>> +	int ret;
>> +
>> +	ret = device_property_read_string(dev, "compatible",
>> +					  &vdev->compat);
>> +	if (ret) {
>> +		pr_err("VFIO: cannot retrieve compat for %s\n",
>> +			vdev->name);
>> +		return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>>  int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>  			       struct device *dev)
>>  {
>> @@ -553,14 +663,14 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>  	if (!vdev)
>>  		return -EINVAL;
>>  
>> -	ret = device_property_read_string(dev, "compatible", &vdev->compat);
>> -	if (ret) {
>> -		pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
>> -		return -EINVAL;
>> -	}
>> +	ret = vfio_platform_acpi_probe(vdev, dev);
>> +	if (ret)
>> +		ret = vfio_platform_of_probe(vdev, dev);
>>  
>> -	vdev->device = dev;
>> +	if (ret)
>> +		return ret;
>>  
>> +	vdev->device = dev;
>>  	group = iommu_group_get(dev);
>>  	if (!group) {
>>  		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
>> @@ -611,7 +721,7 @@ void vfio_platform_unregister_reset(const char *compat,
>>  
>>  	mutex_lock(&driver_lock);
>>  	list_for_each_entry_safe(iter, temp, &reset_list, link) {
>> -		if (!strcmp(iter->compat, compat) && (iter->reset == fn)) {
>> +		if (!strcmp(iter->compat, compat) && (iter->of_reset == fn)) {
>>  			list_del(&iter->link);
>>  			break;
>>  		}
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index 42816dd..ba9e4f8 100644
>> --- a/drivers/vfio/platform/vfio_platform_private.h
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -58,6 +58,7 @@ struct vfio_platform_device {
>>  	struct mutex			igate;
>>  	struct module			*parent_module;
>>  	const char			*compat;
>> +	const char			*acpihid;
>>  	struct module			*reset_module;
>>  	struct device			*device;
>>  
>> @@ -71,7 +72,7 @@ struct vfio_platform_device {
>>  	struct resource*
>>  		(*get_resource)(struct vfio_platform_device *vdev, int i);
>>  	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
>> -	int	(*reset)(struct vfio_platform_device *vdev);
>> +	int	(*of_reset)(struct vfio_platform_device *vdev);
>>  };
>>  
>>  typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
>> @@ -80,7 +81,7 @@ struct vfio_platform_reset_node {
>>  	struct list_head link;
>>  	char *compat;
>>  	struct module *owner;
>> -	vfio_platform_reset_fn_t reset;
>> +	vfio_platform_reset_fn_t of_reset;
>>  };
>>  
>>  extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>> @@ -103,7 +104,7 @@ extern void vfio_platform_unregister_reset(const char *compat,
>>  static struct vfio_platform_reset_node __reset ## _node = {	\
>>  	.owner = THIS_MODULE,					\
>>  	.compat = __compat,					\
>> -	.reset = __reset,					\
>> +	.of_reset = __reset,					\
>>  };								\
>>  __vfio_platform_register_reset(&__reset ## _node)
>>  
>>
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V4 1/2] vfio, platform: add support for ACPI during probe and reset

[Sinan Kaya]

On 5/9/2016 11:47 AM, Eric Auger wrote:
> Hi Sinan,
> On 05/01/2016 11:07 PM, Sinan Kaya wrote:
>> The code is using the compatible DT string to associate a reset driver with
>> the actual device itself. The compatible string does not exist on ACPI
>> based systems. HID is the unique identifier for a device driver instead.
>>
>> When ACPI is enabled, the change will query the presence of _RST method
>> and will call it instead of querying an external reset driver.
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c  | 176 +++++++++++++++++++++-----
>>  drivers/vfio/platform/vfio_platform_private.h |   7 +-
>>  2 files changed, 147 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index e65b142..528ec04 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -13,6 +13,7 @@
>>   */
>>  
>>  #include <linux/device.h>
>> +#include <linux/acpi.h>
>>  #include <linux/iommu.h>
>>  #include <linux/module.h>
>>  #include <linux/mutex.h>
>> @@ -41,7 +42,7 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>>  		if (!strcmp(iter->compat, compat) &&
>>  			try_module_get(iter->owner)) {
>>  			*module = iter->owner;
>> -			reset_fn = iter->reset;
>> +			reset_fn = iter->of_reset;
>>  			break;
>>  		}
>>  	}
>> @@ -49,20 +50,111 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>>  	return reset_fn;
>>  }
>>  
>> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>> +
>> +#ifdef CONFIG_ACPI
>> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>> +			     struct device *dev)
>>  {
>> -	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
>> -						&vdev->reset_module);
>> -	if (!vdev->reset) {
>> -		request_module("vfio-reset:%s", vdev->compat);
>> -		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
>> -							 &vdev->reset_module);
>> +	struct acpi_device *adev = ACPI_COMPANION(dev);
>> +
>> +	if (acpi_disabled)
>> +		return -ENODEV;
>> +
>> +	if (!adev)
>> +		return -ENODEV;
>> +
>> +	vdev->acpihid = acpi_device_hid(adev);
>> +	if (!vdev->acpihid) {
>> +		pr_err("VFIO: cannot find ACPI HID for %s\n",
>> +		       vdev->name);
>> +		return -ENODEV;
>>  	}
>> +	return 0;
>> +}
>> +
>> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
>> +{
>> +	struct device *dev = vdev->device;
>> +	acpi_handle handle = ACPI_HANDLE(dev);
>> +	acpi_status acpi_ret;
>> +	unsigned long long val;
>> +
>> +	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
>> +	if (ACPI_FAILURE(acpi_ret))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
>> +{
>> +	struct device *dev = vdev->device;
>> +	acpi_handle handle = ACPI_HANDLE(dev);
>> +
>> +	if (acpi_has_method(handle, "_RST"))
>> +		return 0;
> Can't you directly return acpi_has_method(handle, "_RST"), hence return
> a boolean?

Makes sense. I'll do that on the next post. 

>> +
>> +	return -EINVAL;
>> +}
>> +#else
>> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>> +			     struct device *dev)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
>> +{
>> +	return -EINVAL;
>> +}
>> +#endif
>> +
>> +static int vfio_platform_has_reset(struct vfio_platform_device *vdev)
>> +{
> return a boolean?

OK

>> +	if (vdev->acpihid)
>> +		return vfio_platform_acpi_has_reset(vdev);
>> +
>> +	if (vdev->of_reset)
>> +		return 0;
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>> +{
>> +	int rc;
>> +
>> +	if (vdev->acpihid)
>> +		return vfio_platform_acpi_has_reset(vdev);
>> +
>> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>> +						    &vdev->reset_module);
>> +	if (vdev->of_reset)
>> +		return 0;
>> +
>> +	rc = request_module("vfio-reset:%s", vdev->compat);
>> +	if (rc)
>> +		return rc;
>> +
>> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>> +						    &vdev->reset_module);
>> +	if (vdev->of_reset)
>> +		return 0;
>> +
>> +	return -EINVAL;
>>  }
>>  
>>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>  {
>> -	if (vdev->reset)
>> +	if (vdev->acpihid)
>> +		return;
>> +
>> +	if (vdev->of_reset)
>>  		module_put(vdev->reset_module);
>>  }
>>  
>> @@ -134,6 +226,20 @@ static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
>>  	kfree(vdev->regions);
>>  }
>>  
>> +static int vfio_platform_call_reset(struct vfio_platform_device *vdev)
>> +{
>> +	if (vdev->of_reset) {
>> +		dev_info(vdev->device, "reset\n");
>> +		vdev->of_reset(vdev);
> return vdev->of_reset(vdev) to align with acpi reset behavior.

OK. I didn't realize it was returning a value. 

>> +		return 0;
>> +	} else if (vdev->acpihid) {
>> +		return vfio_platform_acpi_call_reset(vdev);
> I think it would make sense to have the same dev_info traces for dt and
> acpi.
>> +	}
>> +
>> +	dev_warn(vdev->device, "no reset function found!\n");
>> +	return -EINVAL;
>> +}
>> +
>>  static void vfio_platform_release(void *device_data)
>>  {
>>  	struct vfio_platform_device *vdev = device_data;
>> @@ -141,12 +247,7 @@ static void vfio_platform_release(void *device_data)
>>  	mutex_lock(&driver_lock);
>>  
>>  	if (!(--vdev->refcnt)) {
>> -		if (vdev->reset) {
>> -			dev_info(vdev->device, "reset\n");
>> -			vdev->reset(vdev);
>> -		} else {
>> -			dev_warn(vdev->device, "no reset function found!\n");
>> -		}
>> +		vfio_platform_call_reset(vdev);
>>  		vfio_platform_regions_cleanup(vdev);
>>  		vfio_platform_irq_cleanup(vdev);
>>  	}
>> @@ -175,12 +276,9 @@ static int vfio_platform_open(void *device_data)
>>  		if (ret)
>>  			goto err_irq;
>>  
>> -		if (vdev->reset) {
>> -			dev_info(vdev->device, "reset\n");
>> -			vdev->reset(vdev);
>> -		} else {
>> -			dev_warn(vdev->device, "no reset function found!\n");
>> -		}
>> +		ret = vfio_platform_call_reset(vdev);
>> +		if (ret)
>> +			goto err_irq;
> This change should be in next patch since if you fail finding a reset
> function, you clean things up, mandating a reset function to exist.
> 
> Also in case the reset function fails you change the behavior which is
> not detailed in the commit msg.

I admitted this on another patch. Yes, I'll fix this.

>>  	}
>>  
>>  	vdev->refcnt++;
>> @@ -213,7 +311,7 @@ static long vfio_platform_ioctl(void *device_data,
>>  		if (info.argsz < minsz)
>>  			return -EINVAL;
>>  
>> -		if (vdev->reset)
>> +		if (!vfio_platform_has_reset(vdev))
>>  			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
>>  		info.flags = vdev->flags;
>>  		info.num_regions = vdev->num_regions;
>> @@ -312,10 +410,7 @@ static long vfio_platform_ioctl(void *device_data,
>>  		return ret;
>>  
>>  	} else if (cmd == VFIO_DEVICE_RESET) {
>> -		if (vdev->reset)
>> -			return vdev->reset(vdev);
>> -		else
>> -			return -EINVAL;
>> +		return vfio_platform_call_reset(vdev);
> Here you also change the behavior: before, in case the dt reset failed
> we returned an error. Now we return 0.

Yes, I'll fix it. 
> 
> To me this patch would deserve to be split into 2 parts: ACPI probing
> without reset and then ACPI reset. In case you change the behavior of
> existing dt reset, please put that in a separate patch.

Sure, I'll move vfio_platform_probe_common changes to a seperate patch.
I'll try to break it as much as I can before the post.

> 
> Best Regards
> 
> Eric
>>  	}
>>  
>>  	return -ENOTTY;
>> @@ -544,6 +639,21 @@ static const struct vfio_device_ops vfio_platform_ops = {
>>  	.mmap		= vfio_platform_mmap,
>>  };
>>  
>> +int vfio_platform_of_probe(struct vfio_platform_device *vdev,
>> +			   struct device *dev)
>> +{
>> +	int ret;
>> +
>> +	ret = device_property_read_string(dev, "compatible",
>> +					  &vdev->compat);
>> +	if (ret) {
>> +		pr_err("VFIO: cannot retrieve compat for %s\n",
>> +			vdev->name);
>> +		return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>>  int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>  			       struct device *dev)
>>  {
>> @@ -553,14 +663,14 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>  	if (!vdev)
>>  		return -EINVAL;
>>  
>> -	ret = device_property_read_string(dev, "compatible", &vdev->compat);
>> -	if (ret) {
>> -		pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
>> -		return -EINVAL;
>> -	}
>> +	ret = vfio_platform_acpi_probe(vdev, dev);
>> +	if (ret)
>> +		ret = vfio_platform_of_probe(vdev, dev);
>>  
>> -	vdev->device = dev;
>> +	if (ret)
>> +		return ret;
>>  
>> +	vdev->device = dev;
>>  	group = iommu_group_get(dev);
>>  	if (!group) {
>>  		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
>> @@ -611,7 +721,7 @@ void vfio_platform_unregister_reset(const char *compat,
>>  
>>  	mutex_lock(&driver_lock);
>>  	list_for_each_entry_safe(iter, temp, &reset_list, link) {
>> -		if (!strcmp(iter->compat, compat) && (iter->reset == fn)) {
>> +		if (!strcmp(iter->compat, compat) && (iter->of_reset == fn)) {
>>  			list_del(&iter->link);
>>  			break;
>>  		}
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index 42816dd..ba9e4f8 100644
>> --- a/drivers/vfio/platform/vfio_platform_private.h
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -58,6 +58,7 @@ struct vfio_platform_device {
>>  	struct mutex			igate;
>>  	struct module			*parent_module;
>>  	const char			*compat;
>> +	const char			*acpihid;
>>  	struct module			*reset_module;
>>  	struct device			*device;
>>  
>> @@ -71,7 +72,7 @@ struct vfio_platform_device {
>>  	struct resource*
>>  		(*get_resource)(struct vfio_platform_device *vdev, int i);
>>  	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
>> -	int	(*reset)(struct vfio_platform_device *vdev);
>> +	int	(*of_reset)(struct vfio_platform_device *vdev);
>>  };
>>  
>>  typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
>> @@ -80,7 +81,7 @@ struct vfio_platform_reset_node {
>>  	struct list_head link;
>>  	char *compat;
>>  	struct module *owner;
>> -	vfio_platform_reset_fn_t reset;
>> +	vfio_platform_reset_fn_t of_reset;
>>  };
>>  
>>  extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>> @@ -103,7 +104,7 @@ extern void vfio_platform_unregister_reset(const char *compat,
>>  static struct vfio_platform_reset_node __reset ## _node = {	\
>>  	.owner = THIS_MODULE,					\
>>  	.compat = __compat,					\
>> -	.reset = __reset,					\
>> +	.of_reset = __reset,					\
>>  };								\
>>  __vfio_platform_register_reset(&__reset ## _node)
>>  
>>
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V4 2/2] vfio, platform: make reset driver a requirement by default

[Sinan Kaya]

The code was allowing platform devices to be used without a supporting VFIO
reset driver. The hardware can be left in some inconsistent state after a
guest machine abort.

The reset driver will put the hardware back to safe state and disable
interrupts before returning the control back to the host machine.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/vfio/platform/vfio_amba.c             | 5 +++++
 drivers/vfio/platform/vfio_platform.c         | 5 +++++
 drivers/vfio/platform/vfio_platform_common.c  | 8 +++++++-
 drivers/vfio/platform/vfio_platform_private.h | 1 +
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
index a66479b..7585902 100644
--- a/drivers/vfio/platform/vfio_amba.c
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -23,6 +23,10 @@
 #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
 #define DRIVER_DESC     "VFIO for AMBA devices - User Level meta-driver"
 
+static bool reset_required = true;
+module_param(reset_required, bool, 0644);
+MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)");
+
 /* probing devices from the AMBA bus */
 
 static struct resource *get_amba_resource(struct vfio_platform_device *vdev,
@@ -68,6 +72,7 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
 	vdev->get_resource = get_amba_resource;
 	vdev->get_irq = get_amba_irq;
 	vdev->parent_module = THIS_MODULE;
+	vdev->reset_required = reset_required;
 
 	ret = vfio_platform_probe_common(vdev, &adev->dev);
 	if (ret) {
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index b1cc3a7..ef89146 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -23,6 +23,10 @@
 #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
 #define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
 
+static bool reset_required = true;
+module_param(reset_required, bool, 0644);
+MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)");
+
 /* probing devices from the linux platform bus */
 
 static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
@@ -66,6 +70,7 @@ static int vfio_platform_probe(struct platform_device *pdev)
 	vdev->get_resource = get_platform_resource;
 	vdev->get_irq = get_platform_irq;
 	vdev->parent_module = THIS_MODULE;
+	vdev->reset_required = reset_required;
 
 	ret = vfio_platform_probe_common(vdev, &pdev->dev);
 	if (ret)
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 528ec04..b342538 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -683,7 +683,13 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 		return ret;
 	}
 
-	vfio_platform_get_reset(vdev);
+	ret = vfio_platform_get_reset(vdev);
+	if (ret && vdev->reset_required) {
+		pr_err("vfio: no reset function found for device %s\n",
+		       vdev->name);
+		iommu_group_put(group);
+		return ret;
+	}
 
 	mutex_init(&vdev->igate);
 
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index ba9e4f8..68fbc00 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -50,6 +50,7 @@ struct vfio_platform_region {
 };
 
 struct vfio_platform_device {
+	bool				reset_required;
 	struct vfio_platform_region	*regions;
 	u32				num_regions;
 	struct vfio_platform_irq	*irqs;
-- 
1.8.2.1

[PATCH V4 2/2] vfio, platform: make reset driver a requirement by default

[Sinan Kaya]

The code was allowing platform devices to be used without a supporting VFIO
reset driver. The hardware can be left in some inconsistent state after a
guest machine abort.

The reset driver will put the hardware back to safe state and disable
interrupts before returning the control back to the host machine.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/vfio/platform/vfio_amba.c             | 5 +++++
 drivers/vfio/platform/vfio_platform.c         | 5 +++++
 drivers/vfio/platform/vfio_platform_common.c  | 8 +++++++-
 drivers/vfio/platform/vfio_platform_private.h | 1 +
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
index a66479b..7585902 100644
--- a/drivers/vfio/platform/vfio_amba.c
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -23,6 +23,10 @@
 #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
 #define DRIVER_DESC     "VFIO for AMBA devices - User Level meta-driver"
 
+static bool reset_required = true;
+module_param(reset_required, bool, 0644);
+MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)");
+
 /* probing devices from the AMBA bus */
 
 static struct resource *get_amba_resource(struct vfio_platform_device *vdev,
@@ -68,6 +72,7 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
 	vdev->get_resource = get_amba_resource;
 	vdev->get_irq = get_amba_irq;
 	vdev->parent_module = THIS_MODULE;
+	vdev->reset_required = reset_required;
 
 	ret = vfio_platform_probe_common(vdev, &adev->dev);
 	if (ret) {
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index b1cc3a7..ef89146 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -23,6 +23,10 @@
 #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
 #define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
 
+static bool reset_required = true;
+module_param(reset_required, bool, 0644);
+MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)");
+
 /* probing devices from the linux platform bus */
 
 static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
@@ -66,6 +70,7 @@ static int vfio_platform_probe(struct platform_device *pdev)
 	vdev->get_resource = get_platform_resource;
 	vdev->get_irq = get_platform_irq;
 	vdev->parent_module = THIS_MODULE;
+	vdev->reset_required = reset_required;
 
 	ret = vfio_platform_probe_common(vdev, &pdev->dev);
 	if (ret)
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 528ec04..b342538 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -683,7 +683,13 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 		return ret;
 	}
 
-	vfio_platform_get_reset(vdev);
+	ret = vfio_platform_get_reset(vdev);
+	if (ret && vdev->reset_required) {
+		pr_err("vfio: no reset function found for device %s\n",
+		       vdev->name);
+		iommu_group_put(group);
+		return ret;
+	}
 
 	mutex_init(&vdev->igate);
 
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index ba9e4f8..68fbc00 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -50,6 +50,7 @@ struct vfio_platform_region {
 };
 
 struct vfio_platform_device {
+	bool				reset_required;
 	struct vfio_platform_region	*regions;
 	u32				num_regions;
 	struct vfio_platform_irq	*irqs;
-- 
1.8.2.1

Re: [PATCH V4 0/2] vfio, platform: add ACPI support

[Arnd Bergmann]

On Sunday 01 May 2016 17:07:50 Sinan Kaya wrote:
> The patchset makes three different changes.
> 1. Add support for probing ACPI platform devices.
> 2. Add support for ACPI _RST method support during reset
> 3. Make reset driver a requirement by default with an optional
>    kernel command line override

Looks nice, thanks for rewriting this!

For future submissions, it would help to have a list of changes
compared to the previous versions a the patch set in the cover
letter to help reviewers, but this time my memory was still fresh
enough.

	Arnd

[PATCH V4 0/2] vfio, platform: add ACPI support

[Arnd Bergmann]

On Sunday 01 May 2016 17:07:50 Sinan Kaya wrote:
> The patchset makes three different changes.
> 1. Add support for probing ACPI platform devices.
> 2. Add support for ACPI _RST method support during reset
> 3. Make reset driver a requirement by default with an optional
>    kernel command line override

Looks nice, thanks for rewriting this!

For future submissions, it would help to have a list of changes
compared to the previous versions a the patch set in the cover
letter to help reviewers, but this time my memory was still fresh
enough.

	Arnd

Re: [PATCH V4 0/2] vfio, platform: add ACPI support

[Sinan Kaya]

On 5/3/2016 4:12 PM, Arnd Bergmann wrote:
> On Sunday 01 May 2016 17:07:50 Sinan Kaya wrote:
>> The patchset makes three different changes.
>> 1. Add support for probing ACPI platform devices.
>> 2. Add support for ACPI _RST method support during reset
>> 3. Make reset driver a requirement by default with an optional
>>    kernel command line override
> 
> Looks nice, thanks for rewriting this!
> 
> For future submissions, it would help to have a list of changes
> compared to the previous versions a the patch set in the cover
> letter to help reviewers, but this time my memory was still fresh
> enough.
> 
> 	Arnd
> 

Sure, will do. I asked Rafael to review the ACPI code. 

+		ret = vfio_platform_call_reset(vdev);

This is the only problem I'm seeing in the code. I tested the ACPI path only. 
I'm wondering if Eric could test the device tree version.

+		if (ret)
+			goto err_irq;
 
I'll get rid of this line on the first patch and move it to second
patch along with reset_required check.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V4 0/2] vfio, platform: add ACPI support

[Sinan Kaya]

On 5/3/2016 4:12 PM, Arnd Bergmann wrote:
> On Sunday 01 May 2016 17:07:50 Sinan Kaya wrote:
>> The patchset makes three different changes.
>> 1. Add support for probing ACPI platform devices.
>> 2. Add support for ACPI _RST method support during reset
>> 3. Make reset driver a requirement by default with an optional
>>    kernel command line override
> 
> Looks nice, thanks for rewriting this!
> 
> For future submissions, it would help to have a list of changes
> compared to the previous versions a the patch set in the cover
> letter to help reviewers, but this time my memory was still fresh
> enough.
> 
> 	Arnd
> 

Sure, will do. I asked Rafael to review the ACPI code. 

+		ret = vfio_platform_call_reset(vdev);

This is the only problem I'm seeing in the code. I tested the ACPI path only. 
I'm wondering if Eric could test the device tree version.

+		if (ret)
+			goto err_irq;
 
I'll get rid of this line on the first patch and move it to second
patch along with reset_required check.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V4 0/2] vfio, platform: add ACPI support

[Sinan Kaya]

On 5/3/2016 4:46 PM, Sinan Kaya wrote:
> On 5/3/2016 4:12 PM, Arnd Bergmann wrote:
>> On Sunday 01 May 2016 17:07:50 Sinan Kaya wrote:
>>> The patchset makes three different changes.
>>> 1. Add support for probing ACPI platform devices.
>>> 2. Add support for ACPI _RST method support during reset
>>> 3. Make reset driver a requirement by default with an optional
>>>    kernel command line override
>>
>> Looks nice, thanks for rewriting this!
>>
>> For future submissions, it would help to have a list of changes
>> compared to the previous versions a the patch set in the cover
>> letter to help reviewers, but this time my memory was still fresh
>> enough.
>>
>> 	Arnd
>>
> 
> Sure, will do. I asked Rafael to review the ACPI code. 
> 
> +		ret = vfio_platform_call_reset(vdev);
> 
> This is the only problem I'm seeing in the code. I tested the ACPI path only. 
> I'm wondering if Eric could test the device tree version.
> 
> +		if (ret)
> +			goto err_irq;
>  
> I'll get rid of this line on the first patch and move it to second
> patch along with reset_required check.
> 

Before I post my next patch, I wanted to see if there is any other 
feedback. Rafael mentioned to me that this code should go through
his branch as it is ACPI specific.

Are we OK with this? Alex?

I'm hoping that Eric Auger will be able to test DT in parallel.
I haven't heart from him though.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V4 0/2] vfio, platform: add ACPI support

[Sinan Kaya]

On 5/3/2016 4:46 PM, Sinan Kaya wrote:
> On 5/3/2016 4:12 PM, Arnd Bergmann wrote:
>> On Sunday 01 May 2016 17:07:50 Sinan Kaya wrote:
>>> The patchset makes three different changes.
>>> 1. Add support for probing ACPI platform devices.
>>> 2. Add support for ACPI _RST method support during reset
>>> 3. Make reset driver a requirement by default with an optional
>>>    kernel command line override
>>
>> Looks nice, thanks for rewriting this!
>>
>> For future submissions, it would help to have a list of changes
>> compared to the previous versions a the patch set in the cover
>> letter to help reviewers, but this time my memory was still fresh
>> enough.
>>
>> 	Arnd
>>
> 
> Sure, will do. I asked Rafael to review the ACPI code. 
> 
> +		ret = vfio_platform_call_reset(vdev);
> 
> This is the only problem I'm seeing in the code. I tested the ACPI path only. 
> I'm wondering if Eric could test the device tree version.
> 
> +		if (ret)
> +			goto err_irq;
>  
> I'll get rid of this line on the first patch and move it to second
> patch along with reset_required check.
> 

Before I post my next patch, I wanted to see if there is any other 
feedback. Rafael mentioned to me that this code should go through
his branch as it is ACPI specific.

Are we OK with this? Alex?

I'm hoping that Eric Auger will be able to test DT in parallel.
I haven't heart from him though.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V4 0/2] vfio, platform: add ACPI support

[Rafael J. Wysocki]

On Friday, May 06, 2016 02:35:45 PM Sinan Kaya wrote:
> On 5/3/2016 4:46 PM, Sinan Kaya wrote:
> > On 5/3/2016 4:12 PM, Arnd Bergmann wrote:
> >> On Sunday 01 May 2016 17:07:50 Sinan Kaya wrote:
> >>> The patchset makes three different changes.
> >>> 1. Add support for probing ACPI platform devices.
> >>> 2. Add support for ACPI _RST method support during reset
> >>> 3. Make reset driver a requirement by default with an optional
> >>>    kernel command line override
> >>
> >> Looks nice, thanks for rewriting this!
> >>
> >> For future submissions, it would help to have a list of changes
> >> compared to the previous versions a the patch set in the cover
> >> letter to help reviewers, but this time my memory was still fresh
> >> enough.
> >>
> >> 	Arnd
> >>
> > 
> > Sure, will do. I asked Rafael to review the ACPI code. 
> > 
> > +		ret = vfio_platform_call_reset(vdev);
> > 
> > This is the only problem I'm seeing in the code. I tested the ACPI path only. 
> > I'm wondering if Eric could test the device tree version.
> > 
> > +		if (ret)
> > +			goto err_irq;
> >  
> > I'll get rid of this line on the first patch and move it to second
> > patch along with reset_required check.
> > 
> 
> Before I post my next patch, I wanted to see if there is any other 
> feedback. Rafael mentioned to me that this code should go through
> his branch as it is ACPI specific.

It doesn't have to, but it should be posted to linux-acpi@vger.kernel.org at
least for a review in there.

> Are we OK with this? Alex?
> 
> I'm hoping that Eric Auger will be able to test DT in parallel.
> I haven't heart from him though.

Surely heard?

Thanks,
Rafael

[PATCH V4 0/2] vfio, platform: add ACPI support

[Rafael J. Wysocki]

On Friday, May 06, 2016 02:35:45 PM Sinan Kaya wrote:
> On 5/3/2016 4:46 PM, Sinan Kaya wrote:
> > On 5/3/2016 4:12 PM, Arnd Bergmann wrote:
> >> On Sunday 01 May 2016 17:07:50 Sinan Kaya wrote:
> >>> The patchset makes three different changes.
> >>> 1. Add support for probing ACPI platform devices.
> >>> 2. Add support for ACPI _RST method support during reset
> >>> 3. Make reset driver a requirement by default with an optional
> >>>    kernel command line override
> >>
> >> Looks nice, thanks for rewriting this!
> >>
> >> For future submissions, it would help to have a list of changes
> >> compared to the previous versions a the patch set in the cover
> >> letter to help reviewers, but this time my memory was still fresh
> >> enough.
> >>
> >> 	Arnd
> >>
> > 
> > Sure, will do. I asked Rafael to review the ACPI code. 
> > 
> > +		ret = vfio_platform_call_reset(vdev);
> > 
> > This is the only problem I'm seeing in the code. I tested the ACPI path only. 
> > I'm wondering if Eric could test the device tree version.
> > 
> > +		if (ret)
> > +			goto err_irq;
> >  
> > I'll get rid of this line on the first patch and move it to second
> > patch along with reset_required check.
> > 
> 
> Before I post my next patch, I wanted to see if there is any other 
> feedback. Rafael mentioned to me that this code should go through
> his branch as it is ACPI specific.

It doesn't have to, but it should be posted to linux-acpi at vger.kernel.org at
least for a review in there.

> Are we OK with this? Alex?
> 
> I'm hoping that Eric Auger will be able to test DT in parallel.
> I haven't heart from him though.

Surely heard?

Thanks,
Rafael

Re: [PATCH V4 0/2] vfio, platform: add ACPI support

[Sinan Kaya]

On 5/6/2016 3:54 PM, Rafael J. Wysocki wrote:
>> Before I post my next patch, I wanted to see if there is any other 
>> > feedback. Rafael mentioned to me that this code should go through
>> > his branch as it is ACPI specific.
> It doesn't have to, but it should be posted to linux-acpi@vger.kernel.org at
> least for a review in there.
> 

I'll do that on the next post.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V4 0/2] vfio, platform: add ACPI support

[Sinan Kaya]

On 5/6/2016 3:54 PM, Rafael J. Wysocki wrote:
>> Before I post my next patch, I wanted to see if there is any other 
>> > feedback. Rafael mentioned to me that this code should go through
>> > his branch as it is ACPI specific.
> It doesn't have to, but it should be posted to linux-acpi at vger.kernel.org at
> least for a review in there.
> 

I'll do that on the next post.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project