[PATCH 1/2] vfio, platform: add support for ACPI while detecting the reset driver

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.
The change allows a driver to register with DT compatible string or ACPI
HID and then match the object with one of these conditions.

Rules for loading the reset driver are as follow:
- ACPI HID needs match for ACPI systems
- DT compat needs to match for OF systems

Tested-by: Eric Auger <redacted> (device tree only)
Tested-by: Shanker Donthineni <redacted> (ACPI only)
Signed-off-by: Sinan Kaya <redacted>
---
 .../vfio/platform/reset/vfio_platform_amdxgbe.c    |   4 +-
 .../platform/reset/vfio_platform_calxedaxgmac.c    |   4 +-
 drivers/vfio/platform/vfio_platform_common.c       | 112 +++++++++++++++++----
 drivers/vfio/platform/vfio_platform_private.h      |  43 ++++----
 4 files changed, 125 insertions(+), 38 deletions(-)

diff --git a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
index d4030d0..170dcf5 100644
--- a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
+++ b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c

@@ -119,7 +119,9 @@ int vfio_platform_amdxgbe_reset(struct vfio_platform_device *vdev)
 	return 0;
 }
 
-module_vfio_reset_handler("amd,xgbe-seattle-v1a", vfio_platform_amdxgbe_reset);
+module_vfio_reset_handler("amd,xgbe-seattle-v1a", NULL,
+			  vfio_platform_amdxgbe_reset);
+VFIO_PLATFORM_MODULE_ALIAS("amd,xgbe-seattle-v1a");
 
 MODULE_VERSION("0.1");
 MODULE_LICENSE("GPL v2");

diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
index e3d3d94..635c6e0 100644
--- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
+++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c

@@ -77,7 +77,9 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
 	return 0;
 }
 
-module_vfio_reset_handler("calxeda,hb-xgmac", vfio_platform_calxedaxgmac_reset);
+module_vfio_reset_handler("calxeda,hb-xgmac", NULL,
+			  vfio_platform_calxedaxgmac_reset);
+VFIO_PLATFORM_MODULE_ALIAS("calxeda,hb-xgmac");
 
 MODULE_VERSION(DRIVER_VERSION);
 MODULE_LICENSE("GPL v2");

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 418cdd9..c758e72 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>

@@ -31,14 +32,22 @@ static LIST_HEAD(reset_list);
 static DEFINE_MUTEX(driver_lock);
 
 static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
-					struct module **module)
+				  const char *acpihid, struct module **module)
 {
 	struct vfio_platform_reset_node *iter;
 	vfio_platform_reset_fn_t reset_fn = NULL;
 
 	mutex_lock(&driver_lock);
 	list_for_each_entry(iter, &reset_list, link) {
-		if (!strcmp(iter->compat, compat) &&
+		if (acpihid && iter->acpihid &&
+		    !strcmp(iter->acpihid, acpihid) &&
+			try_module_get(iter->owner)) {
+			*module = iter->owner;
+			reset_fn = iter->reset;
+			break;
+		}
+		if (compat && iter->compat &&
+		    !strcmp(iter->compat, compat) &&
 			try_module_get(iter->owner)) {
 			*module = iter->owner;
 			reset_fn = iter->reset;

@@ -49,15 +58,30 @@ 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)
+static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
-	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);
-	}
+	int rc = 0;
+
+	vdev->reset = vfio_platform_lookup_reset(vdev->compat, vdev->acpihid,
+						 &vdev->reset_module);
+	if (vdev->reset)
+		return 0;
+
+	if (vdev->acpihid)
+		rc = request_module("vfio-reset:%s", vdev->acpihid);
+
+	if (rc && vdev->compat)
+		rc = request_module("vfio-reset:%s", vdev->compat);
+
+	if (rc)
+		return rc;
+
+	vdev->reset = vfio_platform_lookup_reset(vdev->compat, vdev->acpihid,
+						 &vdev->reset_module);
+	if (vdev->reset)
+		return 0;
+
+	return -ENODEV;
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)

@@ -541,6 +565,46 @@ static const struct vfio_device_ops vfio_platform_ops = {
 	.mmap		= vfio_platform_mmap,
 };
 
+#ifdef CONFIG_ACPI
+int vfio_platform_probe_acpi(struct vfio_platform_device *vdev,
+			     struct device *dev)
+{
+	struct acpi_device* adev = ACPI_COMPANION(dev);
+
+	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;
+}
+#else
+int vfio_platform_probe_acpi(struct vfio_platform_device *vdev,
+			     struct device *dev)
+{
+	return -EINVAL;
+}
+#endif
+
+int vfio_platform_probe_of(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)
 {

@@ -550,14 +614,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_probe_acpi(vdev, dev);
+	if (ret)
+		ret = vfio_platform_probe_of(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);

@@ -570,7 +634,11 @@ 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) {
+		iommu_group_put(group);
+		return ret;
+	}
 
 	mutex_init(&vdev->igate);
 

@@ -602,13 +670,21 @@ void __vfio_platform_register_reset(struct vfio_platform_reset_node *node)
 EXPORT_SYMBOL_GPL(__vfio_platform_register_reset);
 
 void vfio_platform_unregister_reset(const char *compat,
+				    const char *acpihid,
 				    vfio_platform_reset_fn_t fn)
 {
 	struct vfio_platform_reset_node *iter, *temp;
 
 	mutex_lock(&driver_lock);
 	list_for_each_entry_safe(iter, temp, &reset_list, link) {
-		if (!strcmp(iter->compat, compat) && (iter->reset == fn)) {
+		if (acpihid && iter->acpihid &&
+		    !strcmp(iter->acpihid, acpihid) && (iter->reset == fn)) {
+			list_del(&iter->link);
+			break;
+		}
+
+		if (compat && iter->compat &&
+		    !strcmp(iter->compat, compat) && (iter->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..20787f7 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;
 

@@ -79,6 +80,7 @@ typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
 struct vfio_platform_reset_node {
 	struct list_head link;
 	char *compat;
+	char *acpihid;
 	struct module *owner;
 	vfio_platform_reset_fn_t reset;
 };

@@ -98,27 +100,32 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
 
 extern void __vfio_platform_register_reset(struct vfio_platform_reset_node *n);
 extern void vfio_platform_unregister_reset(const char *compat,
+					   const char *acpihid,
 					   vfio_platform_reset_fn_t fn);
-#define vfio_platform_register_reset(__compat, __reset)		\
-static struct vfio_platform_reset_node __reset ## _node = {	\
-	.owner = THIS_MODULE,					\
-	.compat = __compat,					\
-	.reset = __reset,					\
-};								\
+
+#define vfio_platform_register_reset(__compat, __acpihid, __reset)	\
+static struct vfio_platform_reset_node __reset ## _node = {		\
+	.owner = THIS_MODULE,						\
+	.compat = __compat,						\
+	.acpihid = __acpihid,						\
+	.reset = __reset,						\
+};									\
 __vfio_platform_register_reset(&__reset ## _node)
 
-#define module_vfio_reset_handler(compat, reset)		\
-MODULE_ALIAS("vfio-reset:" compat);				\
-static int __init reset ## _module_init(void)			\
-{								\
-	vfio_platform_register_reset(compat, reset);		\
-	return 0;						\
-};								\
-static void __exit reset ## _module_exit(void)			\
-{								\
-	vfio_platform_unregister_reset(compat, reset);		\
-};								\
-module_init(reset ## _module_init);				\
+#define VFIO_PLATFORM_MODULE_ALIAS(name)				\
+	MODULE_ALIAS("vfio-reset:" name)
+
+#define module_vfio_reset_handler(compat, acpihid, reset)		\
+static int __init reset ## _module_init(void)				\
+{									\
+	vfio_platform_register_reset(compat, acpihid, reset);		\
+	return 0;							\
+};									\
+static void __exit reset ## _module_exit(void)				\
+{									\
+	vfio_platform_unregister_reset(compat, acpihid, reset);		\
+};									\
+module_init(reset ## _module_init);					\
 module_exit(reset ## _module_exit)
 
 #endif /* VFIO_PLATFORM_PRIVATE_H */

-- 
1.8.2.1

In situations where the userspace driver is stopped abnormally and the
VFIO platform device is released, the assigned HW device currently is
left running. As a consequence the HW device might continue issuing IRQs
and performing DMA accesses.

This patch is implementing a reset driver for HIDMA platform driver.
This gets called by the VFIO platform reset interface.

Reviewed-by: Eric Auger <redacted>
Signed-off-by: Sinan Kaya <redacted>
---
 drivers/vfio/platform/reset/Kconfig                |   9 ++
 drivers/vfio/platform/reset/Makefile               |   2 +
 .../vfio/platform/reset/vfio_platform_qcomhidma.c  | 101 +++++++++++++++++++++
 3 files changed, 112 insertions(+)
 create mode 100644 drivers/vfio/platform/reset/vfio_platform_qcomhidma.c

diff --git a/drivers/vfio/platform/reset/Kconfig b/drivers/vfio/platform/reset/Kconfig
index 70cccc5..d02b3b5 100644
--- a/drivers/vfio/platform/reset/Kconfig
+++ b/drivers/vfio/platform/reset/Kconfig

@@ -13,3 +13,12 @@ config VFIO_PLATFORM_AMDXGBE_RESET
 	  Enables the VFIO platform driver to handle reset for AMD XGBE
 
 	  If you don't know what to do here, say N.
+
+config VFIO_PLATFORM_QCOMHIDMA_RESET
+	tristate "VFIO support for Qualcomm Technologies HIDMA reset"
+	depends on VFIO_PLATFORM
+	help
+	  Enables the VFIO platform driver to handle reset for Qualcomm Technologies
+	  HIDMA Channel.
+
+	  If you don't know what to do here, say N.

diff --git a/drivers/vfio/platform/reset/Makefile b/drivers/vfio/platform/reset/Makefile
index 93f4e23..ec7748a 100644
--- a/drivers/vfio/platform/reset/Makefile
+++ b/drivers/vfio/platform/reset/Makefile

@@ -1,7 +1,9 @@
 vfio-platform-calxedaxgmac-y := vfio_platform_calxedaxgmac.o
 vfio-platform-amdxgbe-y := vfio_platform_amdxgbe.o
+vfio-platform-qcomhidma-y := vfio_platform_qcomhidma.o
 
 ccflags-y += -Idrivers/vfio/platform
 
 obj-$(CONFIG_VFIO_PLATFORM_CALXEDAXGMAC_RESET) += vfio-platform-calxedaxgmac.o
+obj-$(CONFIG_VFIO_PLATFORM_QCOMHIDMA_RESET) += vfio-platform-qcomhidma.o
 obj-$(CONFIG_VFIO_PLATFORM_AMDXGBE_RESET) += vfio-platform-amdxgbe.o

diff --git a/drivers/vfio/platform/reset/vfio_platform_qcomhidma.c b/drivers/vfio/platform/reset/vfio_platform_qcomhidma.c
new file mode 100644
index 0000000..103d481
--- /dev/null
+++ b/drivers/vfio/platform/reset/vfio_platform_qcomhidma.c

@@ -0,0 +1,101 @@
+/*
+ * Qualcomm Technologies HIDMA VFIO Reset Driver
+ *
+ * Copyright (c) 2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/device.h>
+#include <linux/iopoll.h>
+
+#include "vfio_platform_private.h"
+
+#define TRCA_CTRLSTS_OFFSET		0x000
+#define EVCA_CTRLSTS_OFFSET		0x000
+
+#define CH_CONTROL_MASK		GENMASK(7, 0)
+#define CH_STATE_MASK			GENMASK(7, 0)
+#define CH_STATE_BIT_POS		0x8
+
+#define HIDMA_CH_STATE(val)	\
+	((val >> CH_STATE_BIT_POS) & CH_STATE_MASK)
+
+#define EVCA_IRQ_EN_OFFSET		0x110
+
+#define CH_RESET			9
+#define CH_DISABLED			0
+
+int vfio_platform_qcomhidma_reset(struct vfio_platform_device *vdev)
+{
+	struct vfio_platform_region *trreg;
+	struct vfio_platform_region *evreg;
+	u32 val;
+	int ret;
+
+	if (vdev->num_regions != 2)
+		return -ENODEV;
+
+	trreg = &vdev->regions[0];
+	if (!trreg->ioaddr) {
+		trreg->ioaddr =
+			ioremap_nocache(trreg->addr, trreg->size);
+		if (!trreg->ioaddr)
+			return -ENOMEM;
+	}
+
+	evreg = &vdev->regions[1];
+	if (!evreg->ioaddr) {
+		evreg->ioaddr =
+			ioremap_nocache(evreg->addr, evreg->size);
+		if (!evreg->ioaddr)
+			return -ENOMEM;
+	}
+
+	/* disable IRQ */
+	writel(0, evreg->ioaddr + EVCA_IRQ_EN_OFFSET);
+
+	/* reset both transfer and event channels */
+	val = readl(trreg->ioaddr + TRCA_CTRLSTS_OFFSET);
+	val &= ~(CH_CONTROL_MASK << 16);
+	val |= CH_RESET << 16;
+	writel(val, trreg->ioaddr + TRCA_CTRLSTS_OFFSET);
+
+	ret = readl_poll_timeout(trreg->ioaddr + TRCA_CTRLSTS_OFFSET, val,
+				 HIDMA_CH_STATE(val) == CH_DISABLED, 1000,
+				 10000);
+	if (ret)
+		return ret;
+
+	val = readl(evreg->ioaddr + EVCA_CTRLSTS_OFFSET);
+	val &= ~(CH_CONTROL_MASK << 16);
+	val |= CH_RESET << 16;
+	writel(val, evreg->ioaddr + EVCA_CTRLSTS_OFFSET);
+
+	ret = readl_poll_timeout(evreg->ioaddr + EVCA_CTRLSTS_OFFSET, val,
+				 HIDMA_CH_STATE(val) == CH_DISABLED, 1000,
+				 10000);
+	if (ret)
+		return ret;
+
+	pr_info("HIDMA channel reset\n");
+	return 0;
+}
+module_vfio_reset_handler("qcom,hidma-1.0", "QCOM8061",
+			  vfio_platform_qcomhidma_reset);
+VFIO_PLATFORM_MODULE_ALIAS("qcom,hidma-1.0");
+VFIO_PLATFORM_MODULE_ALIAS("QCOM8061");
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Reset support for Qualcomm Technologies HIDMA device");

-- 
1.8.2.1

On 3/8/2016 10:33 AM, Sinan Kaya wrote:

+	int rc = 0;
+

I should have initialized this to -1. 

+	vdev->reset = vfio_platform_lookup_reset(vdev->compat, vdev->acpihid,
+						 &vdev->reset_module);
+	if (vdev->reset)
+		return 0;
+
+	if (vdev->acpihid)
+		rc = request_module("vfio-reset:%s", vdev->acpihid);
+
+	if (rc && vdev->compat)
+		rc = request_module("vfio-reset:%s", vdev->compat);
+

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

On Tue, Mar 08, 2016 at 10:37:34AM -0500, Sinan Kaya wrote:

On 3/8/2016 10:33 AM, Sinan Kaya wrote:

quoted

+	int rc = 0;
+

I should have initialized this to -1. 

-1 is a magic number which stands for -EPERM which is wrong.  Use
-ENODEV.

regards,
dan carpenter

Hi Sinan,
On 03/08/2016 04:33 PM, Sinan Kaya wrote:

Looks the other overridden MODULE_ALIAS have a naming like
MODULE_ALIAS_something? what about MODULE_ALIAS_VFIO_PLATFORM_RESET?
not very compact but maybe closer to what it stands for.

What is the point returning a value here? See my comment at the end.

checkpatch reports: ERROR: "foo* bar" should be "foo *bar"

This change is not related to your commit message. Also here you change
the use model of VFIO platform and forbid any usage if no reset module
is available, right? I don't think this is something we discussed and I
think it removes some flexibility. Currently a warning is emitted in
case we don't have a reset function.

Best Regards

Eric

On 3/10/2016 2:58 AM, Dan Carpenter wrote:

-1 is a magic number which stands for -EPERM which is wrong.  Use
-ENODEV.

OK. done.

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

On 3/10/2016 5:11 AM, Eric Auger wrote:

Hi Sinan,
On 03/08/2016 04:33 PM, Sinan Kaya wrote:

quoted

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.
The change allows a driver to register with DT compatible string or ACPI
HID and then match the object with one of these conditions.

Rules for loading the reset driver are as follow:
- ACPI HID needs match for ACPI systems
- DT compat needs to match for OF systems

Tested-by: Eric Auger <redacted> (device tree only)
Tested-by: Shanker Donthineni <redacted> (ACPI only)
Signed-off-by: Sinan Kaya <redacted>
---
 .../vfio/platform/reset/vfio_platform_amdxgbe.c    |   4 +-
 .../platform/reset/vfio_platform_calxedaxgmac.c    |   4 +-
 drivers/vfio/platform/vfio_platform_common.c       | 112 +++++++++++++++++----
 drivers/vfio/platform/vfio_platform_private.h      |  43 ++++----
 4 files changed, 125 insertions(+), 38 deletions(-)

diff --git a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
index d4030d0..170dcf5 100644
--- a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
+++ b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c

@@ -119,7 +119,9 @@ int vfio_platform_amdxgbe_reset(struct vfio_platform_device *vdev)
 	return 0;
 }
 
-module_vfio_reset_handler("amd,xgbe-seattle-v1a", vfio_platform_amdxgbe_reset);
+module_vfio_reset_handler("amd,xgbe-seattle-v1a", NULL,
+			  vfio_platform_amdxgbe_reset);
+VFIO_PLATFORM_MODULE_ALIAS("amd,xgbe-seattle-v1a");

Looks the other overridden MODULE_ALIAS have a naming like
MODULE_ALIAS_something? what about MODULE_ALIAS_VFIO_PLATFORM_RESET?
not very compact but maybe closer to what it stands for.

alright, I'll follow that.

quoted

 
 MODULE_VERSION("0.1");
 MODULE_LICENSE("GPL v2");

diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
index e3d3d94..635c6e0 100644
--- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
+++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c

@@ -77,7 +77,9 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
 	return 0;
 }
 
-module_vfio_reset_handler("calxeda,hb-xgmac", vfio_platform_calxedaxgmac_reset);
+module_vfio_reset_handler("calxeda,hb-xgmac", NULL,
+			  vfio_platform_calxedaxgmac_reset);
+VFIO_PLATFORM_MODULE_ALIAS("calxeda,hb-xgmac");
 
 MODULE_VERSION(DRIVER_VERSION);
 MODULE_LICENSE("GPL v2");

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 418cdd9..c758e72 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>

@@ -31,14 +32,22 @@ static LIST_HEAD(reset_list);
 static DEFINE_MUTEX(driver_lock);
 
 static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
-					struct module **module)
+				  const char *acpihid, struct module **module)
 {
 	struct vfio_platform_reset_node *iter;
 	vfio_platform_reset_fn_t reset_fn = NULL;
 
 	mutex_lock(&driver_lock);
 	list_for_each_entry(iter, &reset_list, link) {
-		if (!strcmp(iter->compat, compat) &&
+		if (acpihid && iter->acpihid &&
+		    !strcmp(iter->acpihid, acpihid) &&
+			try_module_get(iter->owner)) {
+			*module = iter->owner;
+			reset_fn = iter->reset;
+			break;
+		}
+		if (compat && iter->compat &&
+		    !strcmp(iter->compat, compat) &&
 			try_module_get(iter->owner)) {
 			*module = iter->owner;
 			reset_fn = iter->reset;

@@ -49,15 +58,30 @@ 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)
+static int vfio_platform_get_reset(struct vfio_platform_device *vdev)

What is the point returning a value here? See my comment at the end.

I was trying to do some error handling here. If a driver does not have a
reset implementation, we are letting it go right now. 

I think we need to make reset driver a requirement. Mark Rutland reminded me 
that I need a reset driver. 

I did not think about it during implementation and I have not seen any 
warnings like you said.

quoted

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

quoted

 
-	vfio_platform_get_reset(vdev);
+	ret = vfio_platform_get_reset(vdev);
+	if (ret) {
+		iommu_group_put(group);
+		return ret;
+	}

This change is not related to your commit message. Also here you change
the use model of VFIO platform and forbid any usage if no reset module
is available, right? I don't think this is something we discussed and I
think it removes some flexibility. Currently a warning is emitted in
case we don't have a reset function.

Well, I haven't seen that warning during testing. I was trying to be more 
proactive.

I'm fine removing these checks but not having a reset driver needs a really
big warning here.

Best Regards

Eric

quoted

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

Hi Sinan,
On 03/10/2016 06:13 PM, Sinan Kaya wrote:

On 3/10/2016 5:11 AM, Eric Auger wrote:

quoted

Hi Sinan,
On 03/08/2016 04:33 PM, Sinan Kaya wrote:

quoted

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.
The change allows a driver to register with DT compatible string or ACPI
HID and then match the object with one of these conditions.

Rules for loading the reset driver are as follow:
- ACPI HID needs match for ACPI systems
- DT compat needs to match for OF systems

Tested-by: Eric Auger <redacted> (device tree only)
Tested-by: Shanker Donthineni <redacted> (ACPI only)
Signed-off-by: Sinan Kaya <redacted>
---
 .../vfio/platform/reset/vfio_platform_amdxgbe.c    |   4 +-
 .../platform/reset/vfio_platform_calxedaxgmac.c    |   4 +-
 drivers/vfio/platform/vfio_platform_common.c       | 112 +++++++++++++++++----
 drivers/vfio/platform/vfio_platform_private.h      |  43 ++++----
 4 files changed, 125 insertions(+), 38 deletions(-)

diff --git a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
index d4030d0..170dcf5 100644
--- a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
+++ b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c

@@ -119,7 +119,9 @@ int vfio_platform_amdxgbe_reset(struct vfio_platform_device *vdev)
 	return 0;
 }
 
-module_vfio_reset_handler("amd,xgbe-seattle-v1a", vfio_platform_amdxgbe_reset);
+module_vfio_reset_handler("amd,xgbe-seattle-v1a", NULL,
+			  vfio_platform_amdxgbe_reset);
+VFIO_PLATFORM_MODULE_ALIAS("amd,xgbe-seattle-v1a");

Looks the other overridden MODULE_ALIAS have a naming like
MODULE_ALIAS_something? what about MODULE_ALIAS_VFIO_PLATFORM_RESET?
not very compact but maybe closer to what it stands for.

alright, I'll follow that.

quoted

quoted

 
 MODULE_VERSION("0.1");
 MODULE_LICENSE("GPL v2");

diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
index e3d3d94..635c6e0 100644
--- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
+++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c

@@ -77,7 +77,9 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
 	return 0;
 }
 
-module_vfio_reset_handler("calxeda,hb-xgmac", vfio_platform_calxedaxgmac_reset);
+module_vfio_reset_handler("calxeda,hb-xgmac", NULL,
+			  vfio_platform_calxedaxgmac_reset);
+VFIO_PLATFORM_MODULE_ALIAS("calxeda,hb-xgmac");
 
 MODULE_VERSION(DRIVER_VERSION);
 MODULE_LICENSE("GPL v2");

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 418cdd9..c758e72 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>

@@ -31,14 +32,22 @@ static LIST_HEAD(reset_list);
 static DEFINE_MUTEX(driver_lock);
 
 static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
-					struct module **module)
+				  const char *acpihid, struct module **module)
 {
 	struct vfio_platform_reset_node *iter;
 	vfio_platform_reset_fn_t reset_fn = NULL;
 
 	mutex_lock(&driver_lock);
 	list_for_each_entry(iter, &reset_list, link) {
-		if (!strcmp(iter->compat, compat) &&
+		if (acpihid && iter->acpihid &&
+		    !strcmp(iter->acpihid, acpihid) &&
+			try_module_get(iter->owner)) {
+			*module = iter->owner;
+			reset_fn = iter->reset;
+			break;
+		}
+		if (compat && iter->compat &&
+		    !strcmp(iter->compat, compat) &&
 			try_module_get(iter->owner)) {
 			*module = iter->owner;
 			reset_fn = iter->reset;

@@ -49,15 +58,30 @@ 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)
+static int vfio_platform_get_reset(struct vfio_platform_device *vdev)

What is the point returning a value here? See my comment at the end.

I was trying to do some error handling here. If a driver does not have a
reset implementation, we are letting it go right now. 

I think we need to make reset driver a requirement. Mark Rutland reminded me 
that I need a reset driver. 

I did not think about it during implementation and I have not seen any 
warnings like you said.

the warning currently is emitted on vfio_platform_open/release:
dev_warn(vdev->device, "no reset function found!\n");

quoted

quoted

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

quoted

quoted

 
-	vfio_platform_get_reset(vdev);
+	ret = vfio_platform_get_reset(vdev);
+	if (ret) {
+		iommu_group_put(group);
+		return ret;
+	}

This change is not related to your commit message. Also here you change
the use model of VFIO platform and forbid any usage if no reset module
is available, right? I don't think this is something we discussed and I
think it removes some flexibility. Currently a warning is emitted in
case we don't have a reset function.

Well, I haven't seen that warning during testing. I was trying to be more 
proactive.

Thank you for that. Personally I agree to proceed with the proposed
change, ie. forbid vfio platform driver to be used if there is no reset
function found but this should be in a separate patch with explicit
commit mesg and you should remove dev_warn & related dead code.

Best Regards

Eric

I'm fine removing these checks but not having a reset driver needs a really
big warning here.

quoted

Best Regards

Eric

quoted

On Thu, 10 Mar 2016 12:13:28 -0500
Sinan Kaya [off-list ref] wrote:

On 3/10/2016 5:11 AM, Eric Auger wrote:

quoted

Hi Sinan,
On 03/08/2016 04:33 PM, Sinan Kaya wrote:

quoted

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.
The change allows a driver to register with DT compatible string or ACPI
HID and then match the object with one of these conditions.

Rules for loading the reset driver are as follow:
- ACPI HID needs match for ACPI systems
- DT compat needs to match for OF systems

Tested-by: Eric Auger <redacted> (device tree only)
Tested-by: Shanker Donthineni <redacted> (ACPI only)
Signed-off-by: Sinan Kaya <redacted>
---
 .../vfio/platform/reset/vfio_platform_amdxgbe.c    |   4 +-
 .../platform/reset/vfio_platform_calxedaxgmac.c    |   4 +-
 drivers/vfio/platform/vfio_platform_common.c       | 112 +++++++++++++++++----
 drivers/vfio/platform/vfio_platform_private.h      |  43 ++++----
 4 files changed, 125 insertions(+), 38 deletions(-)

[...]

quoted

quoted

-	vfio_platform_get_reset(vdev);
+	ret = vfio_platform_get_reset(vdev);
+	if (ret) {
+		iommu_group_put(group);
+		return ret;
+	}

This change is not related to your commit message. Also here you change
the use model of VFIO platform and forbid any usage if no reset module
is available, right? I don't think this is something we discussed and I
think it removes some flexibility. Currently a warning is emitted in
case we don't have a reset function.

Well, I haven't seen that warning during testing. I was trying to be more 
proactive.

I'm fine removing these checks but not having a reset driver needs a really
big warning here.

This code really needs to be moved to a separate patch, but I agree
overall: a reset function should be made mandatory - I cannot imagine
trusting the HW to be left in a sane state after being used by a guest.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

On 3/10/2016 10:48 PM, Eric Auger wrote:

quoted

Well, I haven't seen that warning during testing. I was trying to be more 

quoted

proactive.

Thank you for that. Personally I agree to proceed with the proposed
change, ie. forbid vfio platform driver to be used if there is no reset
function found but this should be in a separate patch with explicit
commit mesg and you should remove dev_warn & related dead code.

OK. I'm getting read to post another series and this will be an independent
change.

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