[PATCH v2 0/2] vfio: platform: Improve reset support

[PATCH v2 0/2] vfio: platform: Improve reset support

[Geert Uytterhoeven]

	Hi all,

This patch series improves reset support for vfio-platform:
  - The first patch fixes a bug I ran into while working on this.
  - The second patch implements generic DT reset controller support, for
    devices that are connected to an SoC-internal reset controller and
    can be reset in a generic way.  This avoids having to write/change a
    vfio-specific reset driver for each and every device to be
    passed-through to a guest.

Changes compared to v1:
  - Add Reviewed-by,
  - Don't store error values in vdev->reset_control,
  - Use of_reset_control_get_exclusive() instead of
    __of_reset_control_get(),
  - Improve description.

This has been tested on R-Car H3/Salvator-XS by exporting a GPIO module
to a QEMU+KWM guest: the GPIO module is reset before QEMU opens the vfio
device, and reset again after QEMU has released it, as can be witnessed
by the LEDs connected to the GPIOs.

Thanks!

Geert Uytterhoeven (2):
  vfio: platform: Fix reset module leak in error path
  vfio: platform: Add generic DT reset controller support

 drivers/vfio/platform/vfio_platform_common.c  | 41 ++++++++++++++++++++++-----
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 35 insertions(+), 7 deletions(-)

-- 
2.7.4

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

[PATCH v2 1/2] vfio: platform: Fix reset module leak in error path

[Geert Uytterhoeven]

If the IOMMU group setup fails, the reset module is not released.

Fixes: b5add544d677d363 ("vfio, platform: make reset driver a requirement by default")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
v2:
  - Add Reviewed-by.
---
 drivers/vfio/platform/vfio_platform_common.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 35469af87f88678e..b60bb5326668498c 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -680,18 +680,23 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 	group = vfio_iommu_group_get(dev);
 	if (!group) {
 		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto put_reset;
 	}
 
 	ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev);
-	if (ret) {
-		vfio_iommu_group_put(group, dev);
-		return ret;
-	}
+	if (ret)
+		goto put_iommu;
 
 	mutex_init(&vdev->igate);
 
 	return 0;
+
+put_iommu:
+	vfio_iommu_group_put(group, dev);
+put_reset:
+	vfio_platform_put_reset(vdev);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
 
-- 
2.7.4

Re: [PATCH v2 1/2] vfio: platform: Fix reset module leak in error path

[Simon Horman]

On Tue, Apr 10, 2018 at 05:53:46PM +0200, Geert Uytterhoeven wrote:
> If the IOMMU group setup fails, the reset module is not released.
> 
> Fixes: b5add544d677d363 ("vfio, platform: make reset driver a requirement by default")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

[PATCH v2 2/2] vfio: platform: Add generic DT reset controller support

[Geert Uytterhoeven]

Vfio-platform requires reset support, provided either by ACPI, or, on DT
platforms, by a device-specific reset driver matching against the
device's compatible value.

On many SoCs, devices are connected to an SoC-internal reset controller.
If the reset hierarchy is described in DT using "resets" properties,
such devices can be reset in a generic way through the reset controller
subsystem.  Hence add support for this, avoiding the need to write
device-specific reset drivers for each single device on affected SoCs.

Devices that do require a more complex reset procedure can still provide
a device-specific reset driver, as that takes precedence.

Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and
becomes a no-op (as in: "No reset function found for device") if reset
controller support is disabled.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Don't store error values in vdev->reset_control,
  - Use of_reset_control_get_exclusive() instead of
    __of_reset_control_get(),
  - Improve description.
---
 drivers/vfio/platform/vfio_platform_common.c  | 26 ++++++++++++++++++++++++--
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index b60bb5326668498c..3c13327b2777f8ec 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/reset.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
@@ -112,11 +113,19 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
 	if (VFIO_PLATFORM_IS_ACPI(vdev))
 		return vfio_platform_acpi_has_reset(vdev);
 
-	return vdev->of_reset ? true : false;
+	if (vdev->of_reset)
+		return true;
+
+	if (vdev->reset_control)
+		return true;
+
+	return false;
 }
 
 static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
+	struct reset_control *rstc;
+
 	if (VFIO_PLATFORM_IS_ACPI(vdev))
 		return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
 
@@ -127,8 +136,16 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
 		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
 							&vdev->reset_module);
 	}
+	if (vdev->of_reset)
+		return 0;
+
+	rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL);
+	if (!IS_ERR(rstc)) {
+		vdev->reset_control = rstc;
+		return 0;
+	}
 
-	return vdev->of_reset ? 0 : -ENOENT;
+	return PTR_ERR(rstc);
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
@@ -138,6 +155,8 @@ static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
 
 	if (vdev->of_reset)
 		module_put(vdev->reset_module);
+
+	reset_control_put(vdev->reset_control);
 }
 
 static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
@@ -217,6 +236,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
 	} else if (vdev->of_reset) {
 		dev_info(vdev->device, "reset\n");
 		return vdev->of_reset(vdev);
+	} else if (vdev->reset_control) {
+		dev_info(vdev->device, "reset\n");
+		return reset_control_reset(vdev->reset_control);
 	}
 
 	dev_warn(vdev->device, "no reset function found!\n");
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 85ffe5d9d1abd94e..a56e80ae5986540b 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -60,6 +60,7 @@ struct vfio_platform_device {
 	const char			*compat;
 	const char			*acpihid;
 	struct module			*reset_module;
+	struct reset_control		*reset_control;
 	struct device			*device;
 
 	/*
-- 
2.7.4

Re: [PATCH v2 2/2] vfio: platform: Add generic DT reset controller support

[Philipp Zabel]

On Tue, 2018-04-10 at 17:53 +0200, Geert Uytterhoeven wrote:
> Vfio-platform requires reset support, provided either by ACPI, or, on DT
> platforms, by a device-specific reset driver matching against the
> device's compatible value.
> 
> On many SoCs, devices are connected to an SoC-internal reset controller.
> If the reset hierarchy is described in DT using "resets" properties,
> such devices can be reset in a generic way through the reset controller
> subsystem.  Hence add support for this, avoiding the need to write
> device-specific reset drivers for each single device on affected SoCs.
> 
> Devices that do require a more complex reset procedure can still provide
> a device-specific reset driver, as that takes precedence.
> 
> Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and
> becomes a no-op (as in: "No reset function found for device") if reset
> controller support is disabled.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

> ---
> v2:
>   - Don't store error values in vdev->reset_control,
>   - Use of_reset_control_get_exclusive() instead of
>     __of_reset_control_get(),
>   - Improve description.
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 26 ++++++++++++++++++++++++--
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index b60bb5326668498c..3c13327b2777f8ec 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
[...]
> @@ -127,8 +136,16 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>  							&vdev->reset_module);
>  	}
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL);

If vdev->device->of_node == NULL, this will return -EINVAL ...

> +	if (!IS_ERR(rstc)) {
> +		vdev->reset_control = rstc;
> +		return 0;
> +	}
>  
> -	return vdev->of_reset ? 0 : -ENOENT;
> +	return PTR_ERR(rstc);

... instead of -ENOENT, if that makes any difference.

regards
Philipp

Re: [PATCH v2 2/2] vfio: platform: Add generic DT reset controller support

[Geert Uytterhoeven]

Hi Philipp,

On Wed, Apr 11, 2018 at 10:22 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Tue, 2018-04-10 at 17:53 +0200, Geert Uytterhoeven wrote:
>> Vfio-platform requires reset support, provided either by ACPI, or, on DT
>> platforms, by a device-specific reset driver matching against the
>> device's compatible value.
>>
>> On many SoCs, devices are connected to an SoC-internal reset controller.
>> If the reset hierarchy is described in DT using "resets" properties,
>> such devices can be reset in a generic way through the reset controller
>> subsystem.  Hence add support for this, avoiding the need to write
>> device-specific reset drivers for each single device on affected SoCs.
>>
>> Devices that do require a more complex reset procedure can still provide
>> a device-specific reset driver, as that takes precedence.
>>
>> Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and
>> becomes a no-op (as in: "No reset function found for device") if reset
>> controller support is disabled.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

Thanks!

>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
> [...]
>> @@ -127,8 +136,16 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>>               vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>>                                                       &vdev->reset_module);
>>       }
>> +     if (vdev->of_reset)
>> +             return 0;
>> +
>> +     rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL);
>
> If vdev->device->of_node == NULL, this will return -EINVAL ...
>
>> +     if (!IS_ERR(rstc)) {
>> +             vdev->reset_control = rstc;
>> +             return 0;
>> +     }
>>
>> -     return vdev->of_reset ? 0 : -ENOENT;
>> +     return PTR_ERR(rstc);
>
> ... instead of -ENOENT, if that makes any difference.

Not really. The single caller (vfio_platform_probe_common()) already returns
-EINVAL if no IOMMU group is found, so this should be handled fine.

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

Re: [PATCH v2 2/2] vfio: platform: Add generic DT reset controller support

[Simon Horman]

On Tue, Apr 10, 2018 at 05:53:47PM +0200, Geert Uytterhoeven wrote:
> Vfio-platform requires reset support, provided either by ACPI, or, on DT
> platforms, by a device-specific reset driver matching against the
> device's compatible value.
> 
> On many SoCs, devices are connected to an SoC-internal reset controller.
> If the reset hierarchy is described in DT using "resets" properties,
> such devices can be reset in a generic way through the reset controller
> subsystem.  Hence add support for this, avoiding the need to write
> device-specific reset drivers for each single device on affected SoCs.
> 
> Devices that do require a more complex reset procedure can still provide
> a device-specific reset driver, as that takes precedence.
> 
> Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and
> becomes a no-op (as in: "No reset function found for device") if reset
> controller support is disabled.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - Don't store error values in vdev->reset_control,
>   - Use of_reset_control_get_exclusive() instead of
>     __of_reset_control_get(),
>   - Improve description.
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 26 ++++++++++++++++++++++++--
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index b60bb5326668498c..3c13327b2777f8ec 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/reset.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
> @@ -112,11 +113,19 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
>  	if (VFIO_PLATFORM_IS_ACPI(vdev))
>  		return vfio_platform_acpi_has_reset(vdev);
>  
> -	return vdev->of_reset ? true : false;
> +	if (vdev->of_reset)
> +		return true;
> +
> +	if (vdev->reset_control)
> +		return true;
> +
> +	return false;

I wonder if the above would be better expressed as:

	return vdev->of_reset || vdev->reset_control;

>  }
>  
>  static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  {
> +	struct reset_control *rstc;
> +
>  	if (VFIO_PLATFORM_IS_ACPI(vdev))
>  		return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
>  
> @@ -127,8 +136,16 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>  							&vdev->reset_module);
>  	}
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL);
> +	if (!IS_ERR(rstc)) {
> +		vdev->reset_control = rstc;
> +		return 0;
> +	}
>  
> -	return vdev->of_reset ? 0 : -ENOENT;
> +	return PTR_ERR(rstc);
>  }
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> @@ -138,6 +155,8 @@ static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>  
>  	if (vdev->of_reset)
>  		module_put(vdev->reset_module);
> +
> +	reset_control_put(vdev->reset_control);
>  }
>  
>  static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
> @@ -217,6 +236,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
>  	} else if (vdev->of_reset) {
>  		dev_info(vdev->device, "reset\n");
>  		return vdev->of_reset(vdev);
> +	} else if (vdev->reset_control) {
> +		dev_info(vdev->device, "reset\n");

Would it be useful to differentiate between the above two informational
messages?

> +		return reset_control_reset(vdev->reset_control);
>  	}
>  
>  	dev_warn(vdev->device, "no reset function found!\n");
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 85ffe5d9d1abd94e..a56e80ae5986540b 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -60,6 +60,7 @@ struct vfio_platform_device {
>  	const char			*compat;
>  	const char			*acpihid;
>  	struct module			*reset_module;
> +	struct reset_control		*reset_control;
>  	struct device			*device;
>  
>  	/*
> -- 
> 2.7.4
> 

Re: [PATCH v2 2/2] vfio: platform: Add generic DT reset controller support

[Geert Uytterhoeven]

Hi Simon,

On Wed, Apr 11, 2018 at 10:22 AM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, Apr 10, 2018 at 05:53:47PM +0200, Geert Uytterhoeven wrote:
>> Vfio-platform requires reset support, provided either by ACPI, or, on DT
>> platforms, by a device-specific reset driver matching against the
>> device's compatible value.
>>
>> On many SoCs, devices are connected to an SoC-internal reset controller.
>> If the reset hierarchy is described in DT using "resets" properties,
>> such devices can be reset in a generic way through the reset controller
>> subsystem.  Hence add support for this, avoiding the need to write
>> device-specific reset drivers for each single device on affected SoCs.
>>
>> Devices that do require a more complex reset procedure can still provide
>> a device-specific reset driver, as that takes precedence.
>>
>> Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and
>> becomes a no-op (as in: "No reset function found for device") if reset
>> controller support is disabled.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> v2:
>>   - Don't store error values in vdev->reset_control,
>>   - Use of_reset_control_get_exclusive() instead of
>>     __of_reset_control_get(),
>>   - Improve description.

>> --- 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/reset.h>
>>  #include <linux/slab.h>
>>  #include <linux/types.h>
>>  #include <linux/uaccess.h>
>> @@ -112,11 +113,19 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
>>       if (VFIO_PLATFORM_IS_ACPI(vdev))
>>               return vfio_platform_acpi_has_reset(vdev);
>>
>> -     return vdev->of_reset ? true : false;
>> +     if (vdev->of_reset)
>> +             return true;
>> +
>> +     if (vdev->reset_control)
>> +             return true;
>> +
>> +     return false;
>
> I wonder if the above would be better expressed as:
>
>         return vdev->of_reset || vdev->reset_control;

Makes sense, now both checks are of the same type.

>> @@ -217,6 +236,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
>>       } else if (vdev->of_reset) {
>>               dev_info(vdev->device, "reset\n");
>>               return vdev->of_reset(vdev);
>> +     } else if (vdev->reset_control) {
>> +             dev_info(vdev->device, "reset\n");
>
> Would it be useful to differentiate between the above two informational
> messages?

Probably not, there's also no differentiation with the message for the
ACPI case above (out of visible context).

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

Re: [PATCH v2 2/2] vfio: platform: Add generic DT reset controller support

[Simon Horman]

Hi Geert,

On Wed, Apr 11, 2018 at 10:39:19AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Wed, Apr 11, 2018 at 10:22 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, Apr 10, 2018 at 05:53:47PM +0200, Geert Uytterhoeven wrote:

...

> >> @@ -217,6 +236,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
> >>       } else if (vdev->of_reset) {
> >>               dev_info(vdev->device, "reset\n");
> >>               return vdev->of_reset(vdev);
> >> +     } else if (vdev->reset_control) {
> >> +             dev_info(vdev->device, "reset\n");
> >
> > Would it be useful to differentiate between the above two informational
> > messages?
> 
> Probably not, there's also no differentiation with the message for the
> ACPI case above (out of visible context).

Thanks, I agree that it seems fine to leave things as you have them above.