[PATCH V8 1/9] vfio: platform: rename reset function

[PATCH V8 1/9] vfio: platform: rename reset function

[Sinan Kaya]

Renaming the reset function to of_reset as it is only used
by the device tree based platforms.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@linaro.org>
Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c  | 30 +++++++++++++--------------
 drivers/vfio/platform/vfio_platform_private.h |  6 +++---
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e65b142..08fd7c2 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -41,7 +41,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;
 		}
 	}
@@ -51,18 +51,18 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 
 static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
-	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
-						&vdev->reset_module);
-	if (!vdev->reset) {
+	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
+						    &vdev->reset_module);
+	if (!vdev->of_reset) {
 		request_module("vfio-reset:%s", vdev->compat);
-		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
-							 &vdev->reset_module);
+		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
+							&vdev->reset_module);
 	}
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
 {
-	if (vdev->reset)
+	if (vdev->of_reset)
 		module_put(vdev->reset_module);
 }
 
@@ -141,9 +141,9 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
-		if (vdev->reset) {
+		if (vdev->of_reset) {
 			dev_info(vdev->device, "reset\n");
-			vdev->reset(vdev);
+			vdev->of_reset(vdev);
 		} else {
 			dev_warn(vdev->device, "no reset function found!\n");
 		}
@@ -175,9 +175,9 @@ static int vfio_platform_open(void *device_data)
 		if (ret)
 			goto err_irq;
 
-		if (vdev->reset) {
+		if (vdev->of_reset) {
 			dev_info(vdev->device, "reset\n");
-			vdev->reset(vdev);
+			vdev->of_reset(vdev);
 		} else {
 			dev_warn(vdev->device, "no reset function found!\n");
 		}
@@ -213,7 +213,7 @@ static long vfio_platform_ioctl(void *device_data,
 		if (info.argsz < minsz)
 			return -EINVAL;
 
-		if (vdev->reset)
+		if (vdev->of_reset)
 			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
 		info.flags = vdev->flags;
 		info.num_regions = vdev->num_regions;
@@ -312,8 +312,8 @@ static long vfio_platform_ioctl(void *device_data,
 		return ret;
 
 	} else if (cmd == VFIO_DEVICE_RESET) {
-		if (vdev->reset)
-			return vdev->reset(vdev);
+		if (vdev->of_reset)
+			return vdev->of_reset(vdev);
 		else
 			return -EINVAL;
 	}
@@ -611,7 +611,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..71ed7d1 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -71,7 +71,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 +80,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 +103,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 V8 2/9] vfio: platform: move reset call to a common function

[Sinan Kaya]

The reset call sequence seems to replicate itself multiple times
across the file. Grouping them together for maintenance reasons.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@linaro.org>
Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c | 30 +++++++++++++---------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 08fd7c2..3e2a7c0 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -134,6 +134,17 @@ 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");
+		return vdev->of_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 +152,7 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
-		if (vdev->of_reset) {
-			dev_info(vdev->device, "reset\n");
-			vdev->of_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 +181,7 @@ static int vfio_platform_open(void *device_data)
 		if (ret)
 			goto err_irq;
 
-		if (vdev->of_reset) {
-			dev_info(vdev->device, "reset\n");
-			vdev->of_reset(vdev);
-		} else {
-			dev_warn(vdev->device, "no reset function found!\n");
-		}
+		vfio_platform_call_reset(vdev);
 	}
 
 	vdev->refcnt++;
@@ -312,10 +313,7 @@ static long vfio_platform_ioctl(void *device_data,
 		return ret;
 
 	} else if (cmd == VFIO_DEVICE_RESET) {
-		if (vdev->of_reset)
-			return vdev->of_reset(vdev);
-		else
-			return -EINVAL;
+		return vfio_platform_call_reset(vdev);
 	}
 
 	return -ENOTTY;
-- 
1.8.2.1

[PATCH V8 3/9] vfio: platform: determine reset capability

[Sinan Kaya]

Creating a new function to determine if this driver supports reset
function or not. This is an attempt to abstract device tree calls
from the rest of the code.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@linaro.org>
Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 3e2a7c0..6be92c3 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -49,6 +49,11 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 	return reset_fn;
 }
 
+static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
+{
+	return vdev->of_reset ? true : false;
+}
+
 static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
 	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
@@ -214,7 +219,7 @@ static long vfio_platform_ioctl(void *device_data,
 		if (info.argsz < minsz)
 			return -EINVAL;
 
-		if (vdev->of_reset)
+		if (vfio_platform_has_reset(vdev))
 			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
 		info.flags = vdev->flags;
 		info.num_regions = vdev->num_regions;
-- 
1.8.2.1

[PATCH V8 4/9] vfio: platform: add support for ACPI probe

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

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c  | 57 ++++++++++++++++++++++++---
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 6be92c3..fbf4565 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>
@@ -49,6 +50,37 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 	return reset_fn;
 }
 
+#ifdef CONFIG_ACPI
+static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
+				    struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	if (!adev) {
+		pr_err("VFIO: ACPI companion device not found for %s\n",
+			vdev->name);
+		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
+static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
+					   struct device *dev)
+{
+	return -ENOENT;
+}
+#endif
+
 static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
 {
 	return vdev->of_reset ? true : false;
@@ -547,6 +579,20 @@ 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;
+}
+
 int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 			       struct device *dev)
 {
@@ -556,11 +602,12 @@ 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);
+
+	if (ret)
+		return ret;
 
 	vdev->device = dev;
 
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 71ed7d1..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;
 
-- 
1.8.2.1

Re: [PATCH V8 4/9] vfio: platform: add support for ACPI probe

[Alex Williamson]

On Mon, 20 Jun 2016 11:51:14 -0400
Sinan Kaya <okaya@codeaurora.org> 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.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 57 ++++++++++++++++++++++++---
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  2 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index 6be92c3..fbf4565 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>
> @@ -49,6 +50,37 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>  	return reset_fn;
>  }
>  
> +#ifdef CONFIG_ACPI
> +static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> +				    struct device *dev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> +	if (acpi_disabled)
> +		return -ENODEV;
> +
> +	if (!adev) {
> +		pr_err("VFIO: ACPI companion device not found for %s\n",
> +			vdev->name);
> +		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;
> +	}

Do you want to try to use different errnos here so you don't rely on
the pr_err() calls for debugging?  I could imagine -EPERM, -ENODEV,
-EINVAL respectively, but maybe there are better options.

> +	return 0;
> +}
> +#else
> +static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> +					   struct device *dev)
> +{
> +	return -ENOENT;
> +}
> +#endif
> +
>  static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
>  {
>  	return vdev->of_reset ? true : false;
> @@ -547,6 +579,20 @@ 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);

Previously there was only one probe method and I imagine this pr_err
was useful.  Now we have multiple methods of probing for the device.
Do we really want each one generating pr_err messages or just one at
the end if none of our probes worked?

> +
> +	return ret;
> +}
> +
>  int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  			       struct device *dev)
>  {
> @@ -556,11 +602,12 @@ 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);


The only out way out of vfio_platform_acpi_probe() without hitting a
pr_err is one of (!CONFIG_ACPI || acpi_disabled || success).  Doesn't
that make for some unnecessary warning for a DT user?


> +
> +	if (ret)
> +		return ret;
>  
>  	vdev->device = dev;
>  
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 71ed7d1..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;
>  

Re: [PATCH V8 4/9] vfio: platform: add support for ACPI probe

[Sinan Kaya]

Hi Alex,

On 6/23/2016 2:34 PM, Alex Williamson wrote:
> On Mon, 20 Jun 2016 11:51:14 -0400
> Sinan Kaya <okaya@codeaurora.org> 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.
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c  | 57 ++++++++++++++++++++++++---
>>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>>  2 files changed, 53 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index 6be92c3..fbf4565 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>
>> @@ -49,6 +50,37 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>>  	return reset_fn;
>>  }
>>  
>> +#ifdef CONFIG_ACPI
>> +static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>> +				    struct device *dev)
>> +{
>> +	struct acpi_device *adev = ACPI_COMPANION(dev);
>> +
>> +	if (acpi_disabled)
>> +		return -ENODEV;
>> +
>> +	if (!adev) {
>> +		pr_err("VFIO: ACPI companion device not found for %s\n",
>> +			vdev->name);
>> +		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;
>> +	}
> 
> Do you want to try to use different errnos here so you don't rely on
> the pr_err() calls for debugging?  I could imagine -EPERM, -ENODEV,
> -EINVAL respectively, but maybe there are better options.
> 

will do.

>> +	return 0;
>> +}
>> +#else
>> +static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>> +					   struct device *dev)
>> +{
>> +	return -ENOENT;
>> +}
>> +#endif
>> +
>>  static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
>>  {
>>  	return vdev->of_reset ? true : false;
>> @@ -547,6 +579,20 @@ 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);
> 
> Previously there was only one probe method and I imagine this pr_err
> was useful.  Now we have multiple methods of probing for the device.
> Do we really want each one generating pr_err messages or just one at
> the end if none of our probes worked?

IMO, the new approach is better and is more specific. The error messages
are firmware specific. The previous message included missing compat string
for instance doesn't exist on ACPI firmware and ACPI HID also doesn't exist
on DT firmware. 

I'd rather be verbose rather than a simple probe failed message.

> 
>> +
>> +	return ret;
>> +}
>> +
>>  int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>  			       struct device *dev)
>>  {
>> @@ -556,11 +602,12 @@ 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);
> 
> 
> The only out way out of vfio_platform_acpi_probe() without hitting a
> pr_err is one of (!CONFIG_ACPI || acpi_disabled || success).  Doesn't
> that make for some unnecessary warning for a DT user?

Let me explain the rationale.

As you know, there can be two kernel build combinations. One build where 
ACPI is not selected in Kconfig and another one with the ACPI Kconfig.

In the first case, CONFIG_ACPI is stubbed out in this file and DT user
will not see any kind of messages from ACPI. 

In the second case, both DT and ACPI is compiled in but the system is booting with
any of these combinations. 

If the firmware is DT type, then acpi_disabled is 1. The ACPI probe routine
terminates immediately without any messages. 

If the firmware is ACPI type, then acpi_disabled is 0. All other checks are valid
checks. We cannot claim that this system is DT.

> 
> 
>> +
>> +	if (ret)
>> +		return ret;
>>  
>>  	vdev->device = dev;
>>  
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index 71ed7d1..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;
>>  
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V8 4/9] vfio: platform: add support for ACPI probe

[Alex Williamson]

On Wed, 13 Jul 2016 15:36:48 -0400
Sinan Kaya <okaya@codeaurora.org> wrote:

> Hi Alex,
> 
> On 6/23/2016 2:34 PM, Alex Williamson wrote:
> > On Mon, 20 Jun 2016 11:51:14 -0400
> > Sinan Kaya <okaya@codeaurora.org> 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.
> >>
> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> >> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> >> Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> >> ---
> >>  drivers/vfio/platform/vfio_platform_common.c  | 57 ++++++++++++++++++++++++---
> >>  drivers/vfio/platform/vfio_platform_private.h |  1 +
> >>  2 files changed, 53 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> >> index 6be92c3..fbf4565 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>
> >> @@ -49,6 +50,37 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
> >>  	return reset_fn;
> >>  }
> >>  
> >> +#ifdef CONFIG_ACPI
> >> +static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> >> +				    struct device *dev)
> >> +{
> >> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> >> +
> >> +	if (acpi_disabled)
> >> +		return -ENODEV;
> >> +
> >> +	if (!adev) {
> >> +		pr_err("VFIO: ACPI companion device not found for %s\n",
> >> +			vdev->name);
> >> +		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;
> >> +	}  
> > 
> > Do you want to try to use different errnos here so you don't rely on
> > the pr_err() calls for debugging?  I could imagine -EPERM, -ENODEV,
> > -EINVAL respectively, but maybe there are better options.
> >   
> 
> will do.
> 
> >> +	return 0;
> >> +}
> >> +#else
> >> +static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> >> +					   struct device *dev)
> >> +{
> >> +	return -ENOENT;
> >> +}
> >> +#endif
> >> +
> >>  static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
> >>  {
> >>  	return vdev->of_reset ? true : false;
> >> @@ -547,6 +579,20 @@ 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);  
> > 
> > Previously there was only one probe method and I imagine this pr_err
> > was useful.  Now we have multiple methods of probing for the device.
> > Do we really want each one generating pr_err messages or just one at
> > the end if none of our probes worked?  
> 
> IMO, the new approach is better and is more specific. The error messages
> are firmware specific. The previous message included missing compat string
> for instance doesn't exist on ACPI firmware and ACPI HID also doesn't exist
> on DT firmware. 
> 
> I'd rather be verbose rather than a simple probe failed message.
> 
> >   
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  int vfio_platform_probe_common(struct vfio_platform_device *vdev,
> >>  			       struct device *dev)
> >>  {
> >> @@ -556,11 +602,12 @@ 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);  
> > 
> > 
> > The only out way out of vfio_platform_acpi_probe() without hitting a
> > pr_err is one of (!CONFIG_ACPI || acpi_disabled || success).  Doesn't
> > that make for some unnecessary warning for a DT user?  
> 
> Let me explain the rationale.
> 
> As you know, there can be two kernel build combinations. One build where 
> ACPI is not selected in Kconfig and another one with the ACPI Kconfig.
> 
> In the first case, CONFIG_ACPI is stubbed out in this file and DT user
> will not see any kind of messages from ACPI. 
> 
> In the second case, both DT and ACPI is compiled in but the system is booting with
> any of these combinations. 
> 
> If the firmware is DT type, then acpi_disabled is 1. The ACPI probe routine
> terminates immediately without any messages. 
> 
> If the firmware is ACPI type, then acpi_disabled is 0. All other checks are valid
> checks. We cannot claim that this system is DT.


Thanks, this sort of information and assumption should be documented in
a comment since not all of us care whether a DT device can appear in an
ACPI config or not.  Also note that acpi_disabled and ACPI_COMPANION
are defined regardless of CONFIG_ACPI, so really we only need to wrap
acpi_device_hid() in an #ifdef, we could skip the separate stub.
Thanks,

Alex


> > 
> >   
> >> +
> >> +	if (ret)
> >> +		return ret;
> >>  
> >>  	vdev->device = dev;
> >>  
> >> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> >> index 71ed7d1..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;
> >>    
> >   
> 
> 

Re: [PATCH V8 4/9] vfio: platform: add support for ACPI probe

[Sinan Kaya]

On 7/13/2016 4:09 PM, Alex Williamson wrote:
>> et me explain the rationale.
>> > 
>> > As you know, there can be two kernel build combinations. One build where 
>> > ACPI is not selected in Kconfig and another one with the ACPI Kconfig.
>> > 
>> > In the first case, CONFIG_ACPI is stubbed out in this file and DT user
>> > will not see any kind of messages from ACPI. 
>> > 
>> > In the second case, both DT and ACPI is compiled in but the system is booting with
>> > any of these combinations. 
>> > 
>> > If the firmware is DT type, then acpi_disabled is 1. The ACPI probe routine
>> > terminates immediately without any messages. 
>> > 
>> > If the firmware is ACPI type, then acpi_disabled is 0. All other checks are valid
>> > checks. We cannot claim that this system is DT.
> 
> Thanks, this sort of information and assumption should be documented in
> a comment since not all of us care whether a DT device can appear in an
> ACPI config or not.  Also note that acpi_disabled and ACPI_COMPANION
> are defined regardless of CONFIG_ACPI, so really we only need to wrap
> acpi_device_hid() in an #ifdef, we could skip the separate stub.
> Thanks,

Sure, I'll add the sentences above as a comment to top of the probe function.

I also got rid of stub vfio_platform_acpi_probe function and moved CONFIG_ACPI
inside the vfio_platform_acpi_probe function as you suggested.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V8 5/9] vfio: platform: add extra debug info argument to call reset

[Sinan Kaya]

Getting ready to bring out extra debug information to the caller
so that more verbose information can be printed when an error is
observed.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index fbf4565..e7ce2c2 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -171,7 +171,8 @@ 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)
+static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
+				    const char **extra_dbg)
 {
 	if (vdev->of_reset) {
 		dev_info(vdev->device, "reset\n");
@@ -189,7 +190,7 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
-		vfio_platform_call_reset(vdev);
+		vfio_platform_call_reset(vdev, NULL);
 		vfio_platform_regions_cleanup(vdev);
 		vfio_platform_irq_cleanup(vdev);
 	}
@@ -218,7 +219,7 @@ static int vfio_platform_open(void *device_data)
 		if (ret)
 			goto err_irq;
 
-		vfio_platform_call_reset(vdev);
+		vfio_platform_call_reset(vdev, NULL);
 	}
 
 	vdev->refcnt++;
@@ -350,7 +351,7 @@ static long vfio_platform_ioctl(void *device_data,
 		return ret;
 
 	} else if (cmd == VFIO_DEVICE_RESET) {
-		return vfio_platform_call_reset(vdev);
+		return vfio_platform_call_reset(vdev, NULL);
 	}
 
 	return -ENOTTY;
-- 
1.8.2.1

[PATCH V8 6/9] vfio: platform: call _RST method when using ACPI

[Sinan Kaya]

The device tree code checks for the presence of a reset driver and calls
the of_reset function pointer by looking up the reset driver as a module.

ACPI defines _RST method to perform device level reset. After the _RST
method is executed, the OS can resume using the device. _RST method is
expected to stop DMA transfers and IRQs.

This patch introduces two functions as vfio_platform_acpi_has_reset and
vfio_platform_acpi_call_reset. The has reset method is used to declare
reset capability via the ioctl flag VFIO_DEVICE_FLAGS_RESET. The call
reset function is used to execute the _RST ACPI method.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/vfio/platform/vfio_platform_common.c | 51 ++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e7ce2c2..1d771a6 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -73,21 +73,66 @@ static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
 	}
 	return 0;
 }
+
+static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
+					 const char **extra_dbg)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct device *dev = vdev->device;
+	acpi_handle handle = ACPI_HANDLE(dev);
+	acpi_status acpi_ret;
+
+	acpi_ret = acpi_evaluate_object(handle, "_RST", NULL, &buffer);
+	if (ACPI_FAILURE(acpi_ret)) {
+		if (extra_dbg)
+			*extra_dbg = acpi_format_exception(acpi_ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
+{
+	struct device *dev = vdev->device;
+	acpi_handle handle = ACPI_HANDLE(dev);
+
+	return acpi_has_method(handle, "_RST");
+}
 #else
 static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
 					   struct device *dev)
 {
 	return -ENOENT;
 }
+
+static inline
+int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
+				  const char **extra_dbg)
+{
+	return -ENOENT;
+}
+
+static inline
+bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
+{
+	return false;
+}
 #endif
 
 static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
 {
+	if (vdev->acpihid)
+		return vfio_platform_acpi_has_reset(vdev);
+
 	return vdev->of_reset ? true : false;
 }
 
 static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
+	if (vdev->acpihid)
+		return;
+
 	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
 						    &vdev->reset_module);
 	if (!vdev->of_reset) {
@@ -99,6 +144,9 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
 {
+	if (vdev->acpihid)
+		return;
+
 	if (vdev->of_reset)
 		module_put(vdev->reset_module);
 }
@@ -177,6 +225,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
 	if (vdev->of_reset) {
 		dev_info(vdev->device, "reset\n");
 		return vdev->of_reset(vdev);
+	} else if (vdev->acpihid) {
+		dev_info(vdev->device, "reset\n");
+		return vfio_platform_acpi_call_reset(vdev, extra_dbg);
 	}
 
 	dev_warn(vdev->device, "no reset function found!\n");
-- 
1.8.2.1

[PATCH V8 7/9] 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.

Adding a new reset_required kernel module option to AMBA and platform
VFIO drivers with a default value of true.

New requirements are:
1. A reset function needs to be implemented by the corresponding driver
via DT/ACPI.
2. The reset function needs to be discovered via DT/ACPI.

The probe of the driver will fail if any of the above conditions are
not satisfied.

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  | 24 ++++++++++++++++--------
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 4 files changed, 27 insertions(+), 8 deletions(-)

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 1d771a6..b9ffed4 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -128,10 +128,10 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
 	return vdev->of_reset ? true : false;
 }
 
-static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
+static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
 	if (vdev->acpihid)
-		return;
+		return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
 
 	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
 						    &vdev->reset_module);
@@ -140,6 +140,8 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
 		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
 							&vdev->reset_module);
 	}
+
+	return vdev->of_reset ? 0 : -ENOENT;
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
@@ -663,6 +665,13 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 
 	vdev->device = dev;
 
+	ret = vfio_platform_get_reset(vdev);
+	if (ret && vdev->reset_required) {
+		pr_err("vfio: no reset function found for device %s\n",
+		       vdev->name);
+		return ret;
+	}
+
 	group = iommu_group_get(dev);
 	if (!group) {
 		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
@@ -670,16 +679,15 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 	}
 
 	ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev);
-	if (ret) {
-		iommu_group_put(group);
-		return ret;
-	}
-
-	vfio_platform_get_reset(vdev);
+	if (ret)
+		goto out;
 
 	mutex_init(&vdev->igate);
 
 	return 0;
+out:
+	iommu_group_put(group);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
 
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 V8 7/9] vfio, platform: make reset driver a requirement by default

[Alex Williamson]

On Mon, 20 Jun 2016 11:51:17 -0400
Sinan Kaya <okaya@codeaurora.org> wrote:

> 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.
> 
> Adding a new reset_required kernel module option to AMBA and platform
> VFIO drivers with a default value of true.
> 
> New requirements are:
> 1. A reset function needs to be implemented by the corresponding driver
> via DT/ACPI.
> 2. The reset function needs to be discovered via DT/ACPI.
> 
> The probe of the driver will fail if any of the above conditions are
> not satisfied.
> 
> 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  | 24 ++++++++++++++++--------
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  4 files changed, 27 insertions(+), 8 deletions(-)
> 
> 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;


Do you see value in making the global reset_required changeable, with
the behavior of any given device dependent on the setting of this
variable at the time of probe?  It seems like a bit of a support issue
to me.  Also, we're breaking existing users if there are any with this
change.  Should we introduce a CONFIG option to set the default?  I
think we can get away with changing the default that way, but I'm not
so sure otherwise.

>  
>  	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 1d771a6..b9ffed4 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -128,10 +128,10 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
>  	return vdev->of_reset ? true : false;
>  }
>  
> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  {
>  	if (vdev->acpihid)
> -		return;
> +		return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
>  
>  	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>  						    &vdev->reset_module);
> @@ -140,6 +140,8 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>  							&vdev->reset_module);
>  	}
> +
> +	return vdev->of_reset ? 0 : -ENOENT;
>  }

nit, this looks more like a:

static bool vfio_platform_has_reset(...)
	...
		return vfio_platform_acpi_has_reset() == 0;

	...

	return vdev->of_reset != NULL

>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> @@ -663,6 +665,13 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  
>  	vdev->device = dev;
>  
> +	ret = vfio_platform_get_reset(vdev);
> +	if (ret && vdev->reset_required) {
> +		pr_err("vfio: no reset function found for device %s\n",
> +		       vdev->name);
> +		return ret;
> +	}
> +
>  	group = iommu_group_get(dev);
>  	if (!group) {
>  		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
> @@ -670,16 +679,15 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  	}
>  
>  	ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev);
> -	if (ret) {
> -		iommu_group_put(group);
> -		return ret;
> -	}
> -
> -	vfio_platform_get_reset(vdev);
> +	if (ret)
> +		goto out;
>  
>  	mutex_init(&vdev->igate);
>  
>  	return 0;
> +out:
> +	iommu_group_put(group);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
>  
> 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;

Re: [PATCH V8 7/9] vfio, platform: make reset driver a requirement by default

[Sinan Kaya]

On 6/23/2016 2:59 PM, Alex Williamson wrote:
>> >  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;
> 
> Do you see value in making the global reset_required changeable, with
> the behavior of any given device dependent on the setting of this
> variable at the time of probe?  It seems like a bit of a support issue
> to me.  Also, we're breaking existing users if there are any with this
> change.  Should we introduce a CONFIG option to set the default?  I
> think we can get away with changing the default that way, but I'm not
> so sure otherwise.
> 

We have two groups of existing users.

1. AMBA based drivers
2. DT based drivers

and now we are trying to add the ACPI based drivers in this series.

The AMBA based drivers do not have reset function implemented. Based on
previous conversation with Eric, these devices were mostly used for
bringing up the VFIO framework and were not intended for production. 
If we want to maintain existing functionality, I can change reset_required to
false by default for the AMBA based drivers.

The DT based drivers all have reset functions implemented. They shouldn't be
impacted by the reset_required flag. 

The reset_required flag is again useful for testing purposes when the reset
driver is broken or the ACPI _RST method is missing.

The previously agreed approach was to force the reset required by default
for production environment and be able to clear it for testing purposes.
When I was implementing HIDMA, I never realized that I needed a reset driver
until Arnd told me during the review. We want to avoid this for the long
term for DT and ACPI based implementations.

The reset_required command line parameter would be useful if somebody suspects
that the ACPI _RST implementation is broken or the DT based reset driver is
broken or you quickly want to test the virtualization without having a reset
driver ready yet.

Let us know which way you want to go. I can also add a Kconfig option and
set it by default. But then I have to recompile the kernel when I want to
test without the reset stuff.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V8 7/9] vfio, platform: make reset driver a requirement by default

[Alex Williamson]

On Wed, 13 Jul 2016 16:12:35 -0400
Sinan Kaya <okaya@codeaurora.org> wrote:

> On 6/23/2016 2:59 PM, Alex Williamson wrote:
> >> >  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;  
> > 
> > Do you see value in making the global reset_required changeable, with
> > the behavior of any given device dependent on the setting of this
> > variable at the time of probe?  It seems like a bit of a support issue
> > to me.  Also, we're breaking existing users if there are any with this
> > change.  Should we introduce a CONFIG option to set the default?  I
> > think we can get away with changing the default that way, but I'm not
> > so sure otherwise.
> >   
> 
> We have two groups of existing users.
> 
> 1. AMBA based drivers
> 2. DT based drivers
> 
> and now we are trying to add the ACPI based drivers in this series.
> 
> The AMBA based drivers do not have reset function implemented. Based on
> previous conversation with Eric, these devices were mostly used for
> bringing up the VFIO framework and were not intended for production. 
> If we want to maintain existing functionality, I can change reset_required to
> false by default for the AMBA based drivers.

I think we need to consider them to be in production at this point, so
probably better to make such a change.
 
> The DT based drivers all have reset functions implemented. They shouldn't be
> impacted by the reset_required flag. 

Ok, so we're fine there.

> The reset_required flag is again useful for testing purposes when the reset
> driver is broken or the ACPI _RST method is missing.

I don't doubt that, but it doesn't need to be mode 644 for that, which
allows changing the default dynamically.  We could make it 444 so that
it can be set at module load time and not modified.  I just don't want
to try to guess the state of that variable at the time the device was
probed.

> The previously agreed approach was to force the reset required by default
> for production environment and be able to clear it for testing purposes.
> When I was implementing HIDMA, I never realized that I needed a reset driver
> until Arnd told me during the review. We want to avoid this for the long
> term for DT and ACPI based implementations.

I agree, but we don't need to make it dynamically changeable for that.
Also, nothing prevents us from printing a warning when a device is
probed w/o a reset function, it's just a matter of whether that causes
a probe failure or a complain and continue.

> The reset_required command line parameter would be useful if somebody suspects
> that the ACPI _RST implementation is broken or the DT based reset driver is
> broken or you quickly want to test the virtualization without having a reset
> driver ready yet.
> 
> Let us know which way you want to go. I can also add a Kconfig option and
> set it by default. But then I have to recompile the kernel when I want to
> test without the reset stuff.

Seems like we don't need a Kconfig, but I don't see why the option
needs to be settable except at module load time and we can complain
either way to clue in developers and catch such things in testing.
Thanks,

Alex

Re: [PATCH V8 7/9] vfio, platform: make reset driver a requirement by default

[Sinan Kaya]

On 7/13/2016 4:55 PM, Alex Williamson wrote:
> On Wed, 13 Jul 2016 16:12:35 -0400
> Sinan Kaya <okaya@codeaurora.org> wrote:
> 
>> On 6/23/2016 2:59 PM, Alex Williamson wrote:
>>>>>  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;  
>>>
>>> Do you see value in making the global reset_required changeable, with
>>> the behavior of any given device dependent on the setting of this
>>> variable at the time of probe?  It seems like a bit of a support issue
>>> to me.  Also, we're breaking existing users if there are any with this
>>> change.  Should we introduce a CONFIG option to set the default?  I
>>> think we can get away with changing the default that way, but I'm not
>>> so sure otherwise.
>>>   
>>
>> We have two groups of existing users.
>>
>> 1. AMBA based drivers
>> 2. DT based drivers
>>
>> and now we are trying to add the ACPI based drivers in this series.
>>
>> The AMBA based drivers do not have reset function implemented. Based on
>> previous conversation with Eric, these devices were mostly used for
>> bringing up the VFIO framework and were not intended for production. 
>> If we want to maintain existing functionality, I can change reset_required to
>> false by default for the AMBA based drivers.
> 
> I think we need to consider them to be in production at this point, so
> probably better to make such a change.
>  
>> The DT based drivers all have reset functions implemented. They shouldn't be
>> impacted by the reset_required flag. 
> 
> Ok, so we're fine there.
> 
>> The reset_required flag is again useful for testing purposes when the reset
>> driver is broken or the ACPI _RST method is missing.
> 
> I don't doubt that, but it doesn't need to be mode 644 for that, which
> allows changing the default dynamically.  We could make it 444 so that
> it can be set at module load time and not modified.  I just don't want
> to try to guess the state of that variable at the time the device was
> probed.

OK. I'll change it to 444.

> 
>> The previously agreed approach was to force the reset required by default
>> for production environment and be able to clear it for testing purposes.
>> When I was implementing HIDMA, I never realized that I needed a reset driver
>> until Arnd told me during the review. We want to avoid this for the long
>> term for DT and ACPI based implementations.
> 
> I agree, but we don't need to make it dynamically changeable for that.
> Also, nothing prevents us from printing a warning when a device is
> probed w/o a reset function, it's just a matter of whether that causes
> a probe failure or a complain and continue.
> 
>> The reset_required command line parameter would be useful if somebody suspects
>> that the ACPI _RST implementation is broken or the DT based reset driver is
>> broken or you quickly want to test the virtualization without having a reset
>> driver ready yet.
>>
>> Let us know which way you want to go. I can also add a Kconfig option and
>> set it by default. But then I have to recompile the kernel when I want to
>> test without the reset stuff.
> 
> Seems like we don't need a Kconfig, but I don't see why the option
> needs to be settable except at module load time and we can complain
> either way to clue in developers and catch such things in testing.
> Thanks,

OK. 

> 
> Alex
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V8 7/9] vfio, platform: make reset driver a requirement by default

[Sinan Kaya]

On 6/23/2016 2:59 PM, Alex Williamson wrote:
>> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>> > +static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>> >  {
>> >  	if (vdev->acpihid)
>> > -		return;
>> > +		return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
>> >  
>> >  	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>> >  						    &vdev->reset_module);
>> > @@ -140,6 +140,8 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>> >  		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>> >  							&vdev->reset_module);
>> >  	}
>> > +
>> > +	return vdev->of_reset ? 0 : -ENOENT;
>> >  }
> nit, this looks more like a:
> 
> static bool vfio_platform_has_reset(...)
> 	...
> 		return vfio_platform_acpi_has_reset() == 0;
> 
> 	...
> 
> 	return vdev->of_reset != NULL
> 

Sorry, I didn't understand this comment. The code has get and put functions for DT.
These functions are not useful for ACPI. This is the reason for the above change. 

Can you be more specific?

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V8 7/9] vfio, platform: make reset driver a requirement by default

[Alex Williamson]

On Wed, 13 Jul 2016 16:48:32 -0400
Sinan Kaya <okaya@codeaurora.org> wrote:

> On 6/23/2016 2:59 PM, Alex Williamson wrote:
> >> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev)  
> >> > +static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
> >> >  {
> >> >  	if (vdev->acpihid)
> >> > -		return;
> >> > +		return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
> >> >  
> >> >  	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> >> >  						    &vdev->reset_module);
> >> > @@ -140,6 +140,8 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
> >> >  		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> >> >  							&vdev->reset_module);
> >> >  	}
> >> > +
> >> > +	return vdev->of_reset ? 0 : -ENOENT;
> >> >  }  
> > nit, this looks more like a:
> > 
> > static bool vfio_platform_has_reset(...)
> > 	...
> > 		return vfio_platform_acpi_has_reset() == 0;
> > 
> > 	...
> > 
> > 	return vdev->of_reset != NULL
> >   
> 
> Sorry, I didn't understand this comment. The code has get and put functions for DT.
> These functions are not useful for ACPI. This is the reason for the above change. 
> 
> Can you be more specific?


It was sort of cryptic, I'm not entirely sure I can make sense of it
either.  It think I was mainly suggesting that it looked more like a
bool function so we could just return true/false, but we are actually
setting the of_reset function as part of this, so there is a 'get'
aspect.  Feel free to ignore this comment.  Thanks,

Alex

[PATCH V8 8/9] vfio: platform: check reset call return code during open

[Sinan Kaya]

Open call is ignoring the return code from reset call and can
potentially continue even though reset call failed.

If reset_required module parameter is set, this patch is going
to validate the return code and will abort open if reset fails.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/vfio/platform/vfio_platform_common.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index b9ffed4..8509189 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -264,6 +264,8 @@ static int vfio_platform_open(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!vdev->refcnt) {
+		const char *extra_dbg = NULL;
+
 		ret = vfio_platform_regions_init(vdev);
 		if (ret)
 			goto err_reg;
@@ -272,7 +274,12 @@ static int vfio_platform_open(void *device_data)
 		if (ret)
 			goto err_irq;
 
-		vfio_platform_call_reset(vdev, NULL);
+		ret = vfio_platform_call_reset(vdev, &extra_dbg);
+		if (ret && vdev->reset_required) {
+			dev_warn(vdev->device, "reset driver is required and reset call failed in open (%d) %s\n",
+				 ret, extra_dbg ? extra_dbg : "");
+			goto err_rst;
+		}
 	}
 
 	vdev->refcnt++;
@@ -280,6 +287,8 @@ static int vfio_platform_open(void *device_data)
 	mutex_unlock(&driver_lock);
 	return 0;
 
+err_rst:
+	vfio_platform_irq_cleanup(vdev);
 err_irq:
 	vfio_platform_regions_cleanup(vdev);
 err_reg:
-- 
1.8.2.1

[PATCH V8 9/9] vfio: platform: check reset call return code during release

[Sinan Kaya]

Release call is ignoring the return code from reset call and can
potentially continue even though reset call failed.

If reset_required module parameter is set, this patch is going
to validate the return code and will cause stack dump with
WARN_ON and warn the user of failure.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 8509189..900fd7f 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -243,7 +243,15 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
-		vfio_platform_call_reset(vdev, NULL);
+		const char *extra_dbg = NULL;
+		int ret;
+
+		ret = vfio_platform_call_reset(vdev, &extra_dbg);
+		if (ret && vdev->reset_required) {
+			dev_warn(vdev->device, "reset driver is required and reset call failed in release (%d) %s\n",
+				 ret, extra_dbg ? extra_dbg : "");
+			WARN_ON(1);
+		}
 		vfio_platform_regions_cleanup(vdev);
 		vfio_platform_irq_cleanup(vdev);
 	}
-- 
1.8.2.1