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

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

[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.
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 <eric.auger@linaro.org> (device tree only)
Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only)
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 .../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

[PATCH 2/2] vfio, platform: add QTI HIDMA reset driver

[Sinan Kaya]

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 <eric.auger@linaro.org>
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 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

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

[Sinan Kaya]

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

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

[Dan Carpenter]

On Tue, Mar 08, 2016 at 10:37:34AM -0500, Sinan Kaya wrote:
> On 3/8/2016 10:33 AM, Sinan Kaya wrote:
> > +	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

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

[Eric Auger]

Hi Sinan,
On 03/08/2016 04:33 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.
> 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 <eric.auger@linaro.org> (device tree only)
> Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only)
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  .../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.
>  
>  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.
>  {
> -	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);
checkpatch reports: ERROR: "foo* bar" should be "foo *bar"
> +
> +	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;
> +	}
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
>  
>  	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 */
> 

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

[Sinan Kaya]

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

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

[Sinan Kaya]

On 3/10/2016 5:11 AM, Eric Auger wrote:
> Hi Sinan,
> On 03/08/2016 04:33 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.
>> 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 <eric.auger@linaro.org> (device tree only)
>> Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only)
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>  .../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.

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

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


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



-- 
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 1/2] vfio, platform: add support for ACPI while detecting the reset driver

[Eric Auger]

Hi Sinan,
On 03/10/2016 06:13 PM, Sinan Kaya wrote:
> On 3/10/2016 5:11 AM, Eric Auger wrote:
>> Hi Sinan,
>> On 03/08/2016 04:33 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.
>>> 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 <eric.auger@linaro.org> (device tree only)
>>> Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only)
>>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>>> ---
>>>  .../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.
> 
>>>  
>>>  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");
> 
>>>  {
>>> -	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);
>>> -	}
> 
> 
>>>  
>>> -	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.
> 
>>
>> Best Regards
>>
>> Eric
>>>  
> 
> 
> 

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

[Marc Zyngier]

On Thu, 10 Mar 2016 12:13:28 -0500
Sinan Kaya <okaya@codeaurora.org> wrote:

> On 3/10/2016 5:11 AM, Eric Auger wrote:
> > Hi Sinan,
> > On 03/08/2016 04:33 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.
> >> 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 <eric.auger@linaro.org> (device tree only)
> >> Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only)
> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> >> ---
> >>  .../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(-)

[...]

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

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

[Sinan Kaya]

On 3/10/2016 10:48 PM, Eric Auger wrote:
>> 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.

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