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

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

This has been tested using the R-Car Gen3 GPIO Pass-Through Prototype
posted last week: 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 on the Salvator-XS board.

Thanks for your comments!

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

 drivers/vfio/platform/vfio_platform_common.c  | 38 ++++++++++++++++++++++-----
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 32 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 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>
---
 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 1/2] vfio: platform: Fix reset module leak in error path

[Auger Eric]

Hi Geert,

On 13/02/18 17:36, 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>
> ---
>  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,

Thanks for fixing this.

If I am not wrong we also leak the reset_module if
vfio_platform_get_reset() fails to find the reset function (of_reset ==
NULL), in which case we should do the module_put() in
vfio_platform_get_reset().

Thanks

Eric
>  	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);
>  
> 

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

[Geert Uytterhoeven]

Hi Eric,

On Wed, Feb 14, 2018 at 9:36 AM, Auger Eric <eric.auger@redhat.com> wrote:
> If I am not wrong we also leak the reset_module if
> vfio_platform_get_reset() fails to find the reset function (of_reset ==
> NULL), in which case we should do the module_put() in
> vfio_platform_get_reset().

Correct. Will look into fixing it...

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 1/2] vfio: platform: Fix reset module leak in error path

[Geert Uytterhoeven]

Hi Eric,

On Wed, Feb 14, 2018 at 10:32 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Feb 14, 2018 at 9:36 AM, Auger Eric <eric.auger@redhat.com> wrote:
>> If I am not wrong we also leak the reset_module if
>> vfio_platform_get_reset() fails to find the reset function (of_reset ==
>> NULL), in which case we should do the module_put() in
>> vfio_platform_get_reset().
>
> Correct. Will look into fixing it...

Upon second look, I don't think there's a leak in vfio_platform_get_reset().

If try_module_get() succeeded, there will always be a valid reset function
(unless someone registered a vfio_reset_handler with a NULL reset function).

Or do you mean the call to request_module()?
That one doesn't do a module_get(), it merely tries to load a module.
Hence there's no need to do a module_put() afterwards.

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 1/2] vfio: platform: Fix reset module leak in error path

[Auger Eric]

Hi Geert,

On 21/02/18 17:07, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Wed, Feb 14, 2018 at 10:32 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Wed, Feb 14, 2018 at 9:36 AM, Auger Eric <eric.auger@redhat.com> wrote:
>>> If I am not wrong we also leak the reset_module if
>>> vfio_platform_get_reset() fails to find the reset function (of_reset ==
>>> NULL), in which case we should do the module_put() in
>>> vfio_platform_get_reset().
>>
>> Correct. Will look into fixing it...
> 
> Upon second look, I don't think there's a leak in vfio_platform_get_reset().
> 
> If try_module_get() succeeded, there will always be a valid reset function
> (unless someone registered a vfio_reset_handler with a NULL reset function).
Hum yes, you are right. So the code is fine as is. Sorry for the noise.

Thanks

Eric


> 
> Or do you mean the call to request_module()?
> That one doesn't do a module_get(), it merely tries to load a module.
> Hence there's no need to do a module_put() afterwards.
> 
> 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 1/2] vfio: platform: Fix reset module leak in error path

[Auger Eric]

Hi Geert,

On 13/02/18 17:36, 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>

Thanks

Eric
> ---
>  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);
>  
> 

[PATCH 2/2] vfio: platform: Add generic DT reset 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,
and can be reset in a generic way.  Hence add support to reset such
devices using the reset controller subsystem, provided the reset
hierarchy is described correctly in DT using the "resets" property.

Devices that 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 if reset controller support is disabled.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/vfio/platform/vfio_platform_common.c  | 23 +++++++++++++++++++++--
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index b60bb5326668498c..5d1e48f96e423508 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,7 +113,13 @@ 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 (!IS_ERR_OR_NULL(vdev->reset_control))
+		return true;
+
+	return false;
 }
 
 static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
@@ -127,8 +134,15 @@ 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;
+
+	vdev->reset_control = __of_reset_control_get(vdev->device->of_node,
+						     NULL, 0, false, false);
+	if (!IS_ERR(vdev->reset_control))
+		return 0;
 
-	return vdev->of_reset ? 0 : -ENOENT;
+	return PTR_ERR(vdev->reset_control);
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
@@ -138,6 +152,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 +233,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 (!IS_ERR_OR_NULL(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 2/2] vfio: platform: Add generic DT reset support

[Auger Eric]

Hi Geert,

On 13/02/18 17:36, 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,
> and can be reset in a generic way.  Hence add support to reset such
> devices using the reset controller subsystem, provided the reset
> hierarchy is described correctly in DT using the "resets" property.

I first acknowledge I am not familiar with what those reset controllers
do in practice. My fear is that we may rely on generic SW pieces that
may not be adapted to passthrough constraints. We must guarantee that
any DMA access attempted by the devices are stopped and any interrupts
gets stopped. Can we guarantee that the reset controller always induce
that? Otherwise we may leave the door opened to badly reset assigned
devices.
> 
> Devices that 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 if reset controller support is disabled.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 23 +++++++++++++++++++++--
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index b60bb5326668498c..5d1e48f96e423508 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,7 +113,13 @@ 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 (!IS_ERR_OR_NULL(vdev->reset_control))
> +		return true;
> +
> +	return false;
>  }
>  
>  static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
> @@ -127,8 +134,15 @@ 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;
> +
> +	vdev->reset_control = __of_reset_control_get(vdev->device->of_node,
> +						     NULL, 0, false, false);
> +	if (!IS_ERR(vdev->reset_control))
> +		return 0;
>  
> -	return vdev->of_reset ? 0 : -ENOENT;
> +	return PTR_ERR(vdev->reset_control);
>  }
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> @@ -138,6 +152,8 @@ static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>  
>  	if (vdev->of_reset)
>  		module_put(vdev->reset_module);
> +
if (vdev->reset_control) ?
reset_control_put seems to only check IS_ERR()

Thanks

Eric
> +	reset_control_put(vdev->reset_control);
>  }
>  
>  static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
> @@ -217,6 +233,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 (!IS_ERR_OR_NULL(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;
>  
>  	/*
> 

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

[Geert Uytterhoeven]

Hi Eric,

On Wed, Feb 14, 2018 at 10:09 AM, Auger Eric <eric.auger@redhat.com> wrote:
> On 13/02/18 17:36, 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,
>> and can be reset in a generic way.  Hence add support to reset such
>> devices using the reset controller subsystem, provided the reset
>> hierarchy is described correctly in DT using the "resets" property.
>
> I first acknowledge I am not familiar with what those reset controllers
> do in practice. My fear is that we may rely on generic SW pieces that
> may not be adapted to passthrough constraints. We must guarantee that
> any DMA access attempted by the devices are stopped and any interrupts
> gets stopped. Can we guarantee that the reset controller always induce
> that? Otherwise we may leave the door opened to badly reset assigned
> devices.

An on-SoC reset controller is basically a block controlling signals to the
reset inputs of the individual on-SoC devices.
On Renesas ARM SoCs, this allows to do a full reset of the attached device.

Of course the exact semantics depend on the actual SoC.
If e.g. DMA and interrupts are not stopped for a specific device on a
specific SoC, it still needs a device-specific reset driver, matching against
the appropriate compatible value, cfr. the quoted paragraph below.

You could add a whitelist for of_machine_is_compatible() or
of_device_is_compatible(), but that will grow large fast.

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

>> @@ -138,6 +152,8 @@ static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>
>>       if (vdev->of_reset)
>>               module_put(vdev->reset_module);
>> +
> if (vdev->reset_control) ?
> reset_control_put seems to only check IS_ERR()

    void reset_control_put(struct reset_control *rstc)
    {
            if (IS_ERR_OR_NULL(rstc))
                    return;

So it does handle NULL.

>> +     reset_control_put(vdev->reset_control);

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 2/2] vfio: platform: Add generic DT reset support

[Auger Eric]

Hi Geert,

On 14/02/18 10:43, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Wed, Feb 14, 2018 at 10:09 AM, Auger Eric <eric.auger@redhat.com> wrote:
>> On 13/02/18 17:36, 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,
>>> and can be reset in a generic way.  Hence add support to reset such
>>> devices using the reset controller subsystem, provided the reset
>>> hierarchy is described correctly in DT using the "resets" property.
>>
>> I first acknowledge I am not familiar with what those reset controllers
>> do in practice. My fear is that we may rely on generic SW pieces that
>> may not be adapted to passthrough constraints. We must guarantee that
>> any DMA access attempted by the devices are stopped and any interrupts
>> gets stopped. Can we guarantee that the reset controller always induce
>> that? Otherwise we may leave the door opened to badly reset assigned
>> devices.
> 
> An on-SoC reset controller is basically a block controlling signals to the
> reset inputs of the individual on-SoC devices.
> On Renesas ARM SoCs, this allows to do a full reset of the attached device.
> 
> Of course the exact semantics depend on the actual SoC.
that's the issue actually.
> If e.g. DMA and interrupts are not stopped for a specific device on a
> specific SoC, it still needs a device-specific reset driver, matching against
> the appropriate compatible value, cfr. the quoted paragraph below.
yes but by default we still accept the reset controller solution.
> 
> You could add a whitelist for of_machine_is_compatible() or
> of_device_is_compatible(), but that will grow large fast.
Could be the kind of solution needed. At the moment the list of assigned
platform devices is pretty reduced.

Couldn't we imagine additional dt properties to emphasize the fact a
platform device is passthrough friendly in terms of reset, either
through a reset controller or exposing a single reg that need to be
reset for full reset to be achieved, in accordance with assignment
constraints. That way, the driver writer would somehow certify the
device is eligible for passthrough. One of the issue today is that the
vfio platform reset driver is not maintained by the native driver
maintainer.

I think if people want to do platform passthrough, they need to devise
their HW IPs so that this reset modality is simplified by exposing this
kind of single reg and then dt description may expose this. Also if
possible, the dt node must be as simple/generic as possible to avoid
writing a huge dt node creation function on QEMU side and avoid
dependencies on other nodes.


> 
>>> Devices that require a more complex reset procedure can still
>>> provide a device-specific reset driver, as that takes precedence.
> 
>>> @@ -138,6 +152,8 @@ static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>>
>>>       if (vdev->of_reset)
>>>               module_put(vdev->reset_module);
>>> +
>> if (vdev->reset_control) ?
>> reset_control_put seems to only check IS_ERR()
> 
>     void reset_control_put(struct reset_control *rstc)
>     {
>             if (IS_ERR_OR_NULL(rstc))
>                     return;
> 
> So it does handle NULL.
Sorry I checked an older 4.3 kernel version.

Thanks

Eric
> 
>>> +     reset_control_put(vdev->reset_control);
> 
> 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 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Eric,

On Wed, Feb 14, 2018 at 11:11 AM, Auger Eric <eric.auger@redhat.com> wrote:
> On 14/02/18 10:43, Geert Uytterhoeven wrote:
>> On Wed, Feb 14, 2018 at 10:09 AM, Auger Eric <eric.auger@redhat.com> wrote:
>>> On 13/02/18 17:36, 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,
>>>> and can be reset in a generic way.  Hence add support to reset such
>>>> devices using the reset controller subsystem, provided the reset
>>>> hierarchy is described correctly in DT using the "resets" property.
>>>
>>> I first acknowledge I am not familiar with what those reset controllers
>>> do in practice. My fear is that we may rely on generic SW pieces that
>>> may not be adapted to passthrough constraints. We must guarantee that
>>> any DMA access attempted by the devices are stopped and any interrupts
>>> gets stopped. Can we guarantee that the reset controller always induce
>>> that? Otherwise we may leave the door opened to badly reset assigned
>>> devices.
>>
>> An on-SoC reset controller is basically a block controlling signals to the
>> reset inputs of the individual on-SoC devices.
>> On Renesas ARM SoCs, this allows to do a full reset of the attached device.
>>
>> Of course the exact semantics depend on the actual SoC.
> that's the issue actually.
>> If e.g. DMA and interrupts are not stopped for a specific device on a
>> specific SoC, it still needs a device-specific reset driver, matching against
>> the appropriate compatible value, cfr. the quoted paragraph below.
> yes but by default we still accept the reset controller solution.
>>
>> You could add a whitelist for of_machine_is_compatible() or
>> of_device_is_compatible(), but that will grow large fast.
> Could be the kind of solution needed. At the moment the list of assigned
> platform devices is pretty reduced.
>
> Couldn't we imagine additional dt properties to emphasize the fact a
> platform device is passthrough friendly in terms of reset, either
> through a reset controller or exposing a single reg that need to be
> reset for full reset to be achieved, in accordance with assignment
> constraints. That way, the driver writer would somehow certify the
> device is eligible for passthrough. One of the issue today is that the
> vfio platform reset driver is not maintained by the native driver
> maintainer.

I'm not so fond of adding more DT properties for this. They can be abused
as well.

In general, if there's a "resets" DT property, it means the device can be
reset through the pointed-by reset controller. So that's the common case,
which I'd like to optimize/simplify for.

If more is needed, a separate (device-specific) vfio_reset handler needs
to be written, by the people that know the hardware.

> I think if people want to do platform passthrough, they need to devise
> their HW IPs so that this reset modality is simplified by exposing this
> kind of single reg and then dt description may expose this. Also if
> possible, the dt node must be as simple/generic as possible to avoid
> writing a huge dt node creation function on QEMU side and avoid
> dependencies on other nodes.

Yes. It all depends on sane hardware ;-)

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 2/2] vfio: platform: Add generic DT reset support

[Philipp Zabel]

Hi Geert,

I have a suggestion to avoid having to use the IS_ERR_OR_NULL macro, see
below:

On Tue, 2018-02-13 at 17:36 +0100, 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,
> and can be reset in a generic way.  Hence add support to reset such
> devices using the reset controller subsystem, provided the reset
> hierarchy is described correctly in DT using the "resets" property.
> 
> Devices that 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 if reset controller support is disabled.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 23 +++++++++++++++++++++--
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index b60bb5326668498c..5d1e48f96e423508 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,7 +113,13 @@ 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 (!IS_ERR_OR_NULL(vdev->reset_control))
> +		return true;

I'd avoid storing error values in vdev->reset_control at all, so this
could be:

	if (vdev->reset_control)
		return true;

> +
> +	return false;
>  }
>  
>  static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
> @@ -127,8 +134,15 @@ 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;
> +
> +	vdev->reset_control = __of_reset_control_get(vdev->device->of_node,
> +						     NULL, 0, false, false);
> +	if (!IS_ERR(vdev->reset_control))
> +		return 0;

if you assign to a local variable first here:

	struct reset_control *rstc;

	 ...

	rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL);
	if (!IS_ERR(rstc)) {
		vdev->reset_control = rstc;
		return 0;
	}

Also, please don't use __of_reset_control_get directly.

>  
> -	return vdev->of_reset ? 0 : -ENOENT;
> +	return PTR_ERR(vdev->reset_control);

	return PTR_ERR(rstc);

>  }
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> @@ -138,6 +152,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 +233,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 (!IS_ERR_OR_NULL(vdev->reset_control)) {
> +		dev_info(vdev->device, "reset\n");
> +		return reset_control_reset(vdev->reset_control);

	} 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;
>  
>  	/*

regards
Philipp

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

[Geert Uytterhoeven]

Hi Philipp,

On Wed, Feb 21, 2018 at 5:51 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> I have a suggestion to avoid having to use the IS_ERR_OR_NULL macro, see
> below:
>
> On Tue, 2018-02-13 at 17:36 +0100, 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,
>> and can be reset in a generic way.  Hence add support to reset such
>> devices using the reset controller subsystem, provided the reset
>> hierarchy is described correctly in DT using the "resets" property.
>>
>> Devices that 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 if reset controller support is disabled.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c  | 23 +++++++++++++++++++++--
>>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>>  2 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index b60bb5326668498c..5d1e48f96e423508 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,7 +113,13 @@ 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 (!IS_ERR_OR_NULL(vdev->reset_control))
>> +             return true;
>
> I'd avoid storing error values in vdev->reset_control at all, so this
> could be:
>
>         if (vdev->reset_control)
>                 return true;

Thanks, much better!

>> @@ -127,8 +134,15 @@ 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;
>> +
>> +     vdev->reset_control = __of_reset_control_get(vdev->device->of_node,
>> +                                                  NULL, 0, false, false);
>> +     if (!IS_ERR(vdev->reset_control))
>> +             return 0;
>
> if you assign to a local variable first here:
>
>         struct reset_control *rstc;
>
>          ...
>
>         rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL);
>         if (!IS_ERR(rstc)) {
>                 vdev->reset_control = rstc;
>                 return 0;
>         }
>
> Also, please don't use __of_reset_control_get directly.

OK, apparently I didn't read <linux/reset.h> beyond the first #else...

>> @@ -217,6 +233,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 (!IS_ERR_OR_NULL(vdev->reset_control)) {
>> +             dev_info(vdev->device, "reset\n");
>> +             return reset_control_reset(vdev->reset_control);
>
>         } else {
>                 if (vdev->reset_control)
>                         dev_info(vdev->device, "reset\n");
>                 return reset_control_reset(vdev->reset_control);
>
>>       }

I'd like to keep the "else if", as that's the pattern used by the blocks above.

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 2/2] vfio: platform: Add generic DT reset support

[Philipp Zabel]

Hi Geert,

On Thu, 2018-02-22 at 09:50 +0100, Geert Uytterhoeven wrote:
[...]
> > > @@ -127,8 +134,15 @@ 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;
> > > +
> > > +     vdev->reset_control = __of_reset_control_get(vdev->device->of_node,
> > > +                                                  NULL, 0, false, false);
> > > +     if (!IS_ERR(vdev->reset_control))
> > > +             return 0;
> > 
> > if you assign to a local variable first here:
> > 
> >         struct reset_control *rstc;
> > 
> >          ...
> > 
> >         rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL);
> >         if (!IS_ERR(rstc)) {
> >                 vdev->reset_control = rstc;
> >                 return 0;
> >         }
> > 
> > Also, please don't use __of_reset_control_get directly.
> 
> OK, apparently I didn't read <linux/reset.h> beyond the first #else...

I don't blame you, this is missing some documentation.

> > > @@ -217,6 +233,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 (!IS_ERR_OR_NULL(vdev->reset_control)) {
> > > +             dev_info(vdev->device, "reset\n");
> > > +             return reset_control_reset(vdev->reset_control);
> > 
> >         } else {
> >                 if (vdev->reset_control)
> >                         dev_info(vdev->device, "reset\n");
> >                 return reset_control_reset(vdev->reset_control);
> > 
> > >       }
> 
> I'd like to keep the "else if", as that's the pattern used by the blocks above.

my bad, there's more code after this.

	} else if (vdev->reset_control) {
		dev_info(vdev->device, "reset\n");
		return reset_control_reset(vdev->reset_control);
	}

is better as it doesn't lose the warning if vdev->reset_control == NULL.
It seems I've focused too much on getting rid of the IS_ERR_OR_NULL
macro.

regards
Philipp