[PATCH v3] vfio: platform: Fix using devices in PM Domains

If a device is part of a PM Domain (e.g. power and/or clock domain), its
power state is managed using Runtime PM.  Without Runtime PM, the device
may not be powered up or clocked, causing subtle failures, crashes, or
system lock-ups when the device is accessed by the guest.

Fix this by adding Runtime PM support, powering the device when the VFIO
device is opened by the guest.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Simon Horman <redacted>
---
v3:
  - Drop controversial note about unsafeness of exporting fine-grained
    power management from host to guest,

v2:
  - Improve wording,
  - Add Reviewed-by.
---
 drivers/vfio/platform/vfio_platform_common.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index b60bb5326668498c..41f862f055054543 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c

@@ -17,6 +17,7 @@
 #include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>

@@ -233,6 +234,8 @@ static void vfio_platform_release(void *device_data)
 		const char *extra_dbg = NULL;
 		int ret;
 
+		pm_runtime_put(vdev->device);
+
 		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",

@@ -275,6 +278,10 @@ static int vfio_platform_open(void *device_data)
 				 ret, extra_dbg ? extra_dbg : "");
 			goto err_rst;
 		}
+
+		ret = pm_runtime_get_sync(vdev->device);
+		if (ret < 0)
+			goto err_rst;
 	}
 
 	vdev->refcnt++;

@@ -690,6 +697,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 
 	mutex_init(&vdev->igate);
 
+	pm_runtime_enable(vdev->device);
 	return 0;
 
 put_iommu:

@@ -707,6 +715,7 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
 	vdev = vfio_del_group_dev(dev);
 
 	if (vdev) {
+		pm_runtime_disable(vdev->device);
 		vfio_platform_put_reset(vdev);
 		vfio_iommu_group_put(dev->iommu_group, dev);
 	}

-- 
2.7.4

Hi Geert,

On 05/28/2018 05:26 PM, Geert Uytterhoeven wrote:

If a device is part of a PM Domain (e.g. power and/or clock domain), its
power state is managed using Runtime PM.  Without Runtime PM, the device
may not be powered up or clocked, causing subtle failures, crashes, or
system lock-ups when the device is accessed by the guest.

Fix this by adding Runtime PM support, powering the device when the VFIO
device is opened by the guest.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Simon Horman <redacted>
---
v3:
  - Drop controversial note about unsafeness of exporting fine-grained
    power management from host to guest,

thanks ;-)

+
 		ret = vfio_platform_call_reset(vdev, &extra_dbg);

While reading the code again, the reset is using the HW. Don't you want
to call the get() before.

Thanks

Eric

Hi Eric,

On Mon, May 28, 2018 at 10:27 PM, Auger Eric [off-list ref] wrote:

On 05/28/2018 05:26 PM, Geert Uytterhoeven wrote:

quoted

If a device is part of a PM Domain (e.g. power and/or clock domain), its
power state is managed using Runtime PM.  Without Runtime PM, the device
may not be powered up or clocked, causing subtle failures, crashes, or
system lock-ups when the device is accessed by the guest.

Fix this by adding Runtime PM support, powering the device when the VFIO
device is opened by the guest.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Simon Horman <redacted>
---
v3:
  - Drop controversial note about unsafeness of exporting fine-grained
    power management from host to guest,

thanks ;-)

quoted

v2:
  - Improve wording,
  - Add Reviewed-by.
---
 drivers/vfio/platform/vfio_platform_common.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index b60bb5326668498c..41f862f055054543 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c

@@ -17,6 +17,7 @@
 #include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>

@@ -233,6 +234,8 @@ static void vfio_platform_release(void *device_data)
              const char *extra_dbg = NULL;
              int ret;

+             pm_runtime_put(vdev->device);

quoted

+
              ret = vfio_platform_call_reset(vdev, &extra_dbg);

While reading the code again, the reset is using the HW. Don't you want
to call the get() before.

Thanks, you're right, it's safer to do the reset here first...

quoted

              if (ret && vdev->reset_required) {
                      dev_warn(vdev->device, "reset driver is required and reset call failed in release (%d) %s\n",

@@ -275,6 +278,10 @@ static int vfio_platform_open(void *device_data)
                               ret, extra_dbg ? extra_dbg : "");
                      goto err_rst;
              }
+
+             ret = pm_runtime_get_sync(vdev->device);

... and to do the reset here last.

Will fix and send v4.

quoted

+             if (ret < 0)
+                     goto err_rst;
      }

      vdev->refcnt++;

@@ -690,6 +697,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,

      mutex_init(&vdev->igate);

+     pm_runtime_enable(vdev->device);
      return 0;

 put_iommu:

@@ -707,6 +715,7 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
      vdev = vfio_del_group_dev(dev);

      if (vdev) {
+             pm_runtime_disable(vdev->device);
              vfio_platform_put_reset(vdev);
              vfio_iommu_group_put(dev->iommu_group, dev);
      }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds