Re: [PATCH v5] vfio: platform: Add generic reset controller support

[Auger Eric]

Hi Geert,
On 11/13/18 2:15 PM, Geert Uytterhoeven wrote:
> Vfio-platform requires dedicated 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, or
> in lookup tables in platform code, and devices have exactly one
> dedicated reset each, 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 
> Reviewed-by: Philipp Zabel 
> Reviewed-by: Simon Horman 
> ---
> This depends on "[PATCH] reset: Add reset_control_get_count()"
> (https://lore.kernel.org/lkml/20181113124744.7769-1-geert+rene...@glider.be/).
> 
> v5:
>   - Use reset_control_get_exclusive() instead of rejected
> reset_control_get_dedicated(), as exclusive already implies a
> dedicated reset line,
>   - Ensure the device has exactly one reset,
> 
> v4:
>   - Add Reviewed-by,
>   - Use new RFC reset_control_get_dedicated() instead of
> of_reset_control_get_exclusive(), to (a) make it more generic, and
> (b) make sure the reset returned is really a dedicated reset, and
> does not affect other devices,
> 
> v3:
>   - Add Reviewed-by,
>   - Merge similar checks in vfio_platform_has_reset(),
> 
> 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  | 29 +--
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c 
> b/drivers/vfio/platform/vfio_platform_common.c
> index c0cd824be2b767be..ce2aad8e0a8159f9 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -113,11 +114,14 @@ 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;
> + return vdev->of_reset || vdev->reset_control;
>  }
>  
>  static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  {
> + struct reset_control *rstc;
> + int count;
> +
>   if (VFIO_PLATFORM_IS_ACPI(vdev))
>   return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
>  
> @@ -128,8 +132,24 @@ static int vfio_platform_get_reset(struct 
> vfio_platform_device *vdev)
>   vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>   >reset_module);
>   }
> + if (vdev->of_reset)
> + return 0;
> +
> + /* Generic reset handling needs a single, dedicated reset line */
> + count = reset_control_get_count(vdev->device);
> + if (count < 0)
> + return count;
> +
> + if (count != 1)
> + return -EINVAL;
>  
> - return vdev->of_reset ? 0 : -ENOENT;
> + rstc = reset_control_get_exclusive(vdev->device, NULL);
So I understand the semantics of reset_control_get_exclusive() is now
agreed and this means the reset line is not shared with other devices,
correct?

A question about the usage of reset_control_get_count(). Why is it an
issue if the count is > 1? Does it check there are several reset lines,
each of then being used for resetting something different or does it
check there are several reset controllers are able to do the reset job?
My point behind is what's the issue as long as one non shared line can
be grabbed with reset_control_get_exclusive().
> + if (!IS_ERR(rstc)) {
> + vdev->reset_control = rstc;
> + return 0;
> + }
> +
> + return PTR_ERR(rstc);
>  }
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> @@ -139,6 +159,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);
Most of the drivers use devm_reset_control_get_exclusive(), can't we use
that instead and benefit from the fact the reset_control_put() is called
automatically on driver 

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

[Auger Eric]

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 
> Reviewed-by: Simon Horman 
> ---
> v3:
>   - Drop controversial note about unsafeness of exporting fine-grained
> power management from host to guest,
thanks ;-)

> 
> 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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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, _dbg);
While reading the code again, the reset is using the HW. Don't you want
to call the get() before.

Thanks

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

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

[Auger Eric]

Hi Geert,

On 05/18/2018 12:55 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.

I fail to apply this on master branch?
> 
> Note that while more fine-grained power management could be implemented
> on the guest side, if exported by the host, this would be inherently
> unsafe, as abusing it may take down the whole system.
I think I would prefer we get rid of the above paragraph.

Thanks

Eric
> 
> Signed-off-by: Geert Uytterhoeven 
> Reviewed-by: Simon Horman 
> ---
> 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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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, _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(>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);
>   }
> 

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

[Auger Eric]

Hi Philipp,

On 13/04/18 11:22, Philipp Zabel wrote:
[..]
> That also means it is impossible to use just one of the devices that
> share a reset line for vfio individually, while the other ones are still
> in use by the host. Currently the reset line is a shared resource
> similar to the iommu for devices in the same iommu_group.
> 
> Is there any mechanism in vfio that would allow modeling other shared
> resources apart from iommu?

No we only check the VFIO group viability at IOMMU level.
> 
> [...]
>>> For some of those it may be possible, but that is basically just a work-
>>> around for reality not matching expectations. There may be other cases
>>> where devices sharing a reset line are not even in the same parent node
>>> because they are controlled via a different bus. In general, I don't
>>> think it is feasible or desirable to force grouping of devices that
>>> share the same reset line into a common parent node.
>>
>> At least for Renesas R-Car SoCs, I think this is feasible, as all affected
>> devices are currently grouped under the same /soc node.
>> I added subnodes for all devices sharing resets (one for pwm, 4 for USB2,
>> and one for USB3; display doesn't have resets yet), and it still boots ;-)
> 
> Is this grouping enough to make sure all of the pwm/usb2/usb3 devices
> are only ever configured for vfio use together?
> 
> Assuming I have pwm[1-4] all sharing the same reset line, and I want
> pwm2 to be used by a vfio guest, I first have to make sure that all of
> pwm[1-4] are unbound, releasing their shared resets, before vfio-
> platform can request the same reset line as exclusive.
> 
> Thinking about it, if the pwm drivers keep their requested reset control
> around for the duration the device is bound, the reset controller
> framework should already kind of handle this - while any of the shared
> reset control handles is kept around, any exclusive request for the same
> reset control will fail with -EBUSY (and the other way around).
> But that requires all drivers to request the reset control during probe
> and release it during remove.
> 
>> However, ehci_platform_probe() cannot get its (optional) resets anymore.
>> Probably the reset controller framework needs to be taught to look for
>> shared resets in the parent node, too?
> 
> Hm, a generic framework shouldn't do such a thing, the parent node could
> be covered by a completely different binding.
> 
>>> My suggestion would be to relax the language in the reset.txt DT
>>> bindings doc.
>>
>> Which is fine to keep the status quo with the hardware designers, but makes
>> it less likely for non-whitelisted generic reset controller support to
>> become acceptable for the vfio people...
> 
> I still may be missing context, but I fail to see how
> 
>   pwm@0 {
>   resets = <_reset_control>;
>   };
> 
>   pwm@1 {
>   resets = <_reset_control>;
>   };
> 
> ->
> 
>   pwms {
>   resets = <_reset_control>;
> 
>   pwm@0 {
>   };
> 
>   pwm@1 {
>   };
>   };
> 
> makes any difference here, unless pwms gets bound to an actual driver
> that is used for vfio?

I don't think we are ready to assign pwms with VFIO as Alex emphasized
VFIO was meant to be used with IOMMU and I guess those devices do not
belong to any iommu group.

Thanks

Eric
> 
> regards
> Philipp
> 

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

[Auger Eric]

Hi Geert,

On 13/04/18 11:19, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Fri, Apr 13, 2018 at 11:14 AM, Auger Eric <eric.au...@redhat.com> wrote:
>> On 11/04/18 11:24, 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, causing subtle failures, crashes, or system
>>> lock-ups when the device is accessed by the guest.
>> the device may not be powered up/clcoked or power/clock may be switched
>> off while the guest uses it.
>>>
>>> Fix this by adding Runtime PM support, powering the device when the VFIO
>>> device is opened by the guest.
>>>
>>> Note that while more fine-grained power management could be implemented
>>> on the guest side, if exported, this would be inherently unsafe, as
>>> abusing it may kill the whole system.
>>
>> Please can you elaborate on this remark please?
> 
> If power-management of the device would be delegated to the guest, and the
> guest forgets to enable device power before accessing the device's registers,
> this could lock up the system, and thus disturb both the host and other 
> guests.
Wouldn't you need to assign another device or use para-virt to allow the
guest to perform this power management control? I think you can remove
this paragraph from the commit message.

Thanks

Eric
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 

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

[Auger Eric]

Hi Geert,

On 11/04/18 11:24, 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, causing subtle failures, crashes, or system
> lock-ups when the device is accessed by the guest.
the device may not be powered up/clcoked or power/clock may be switched
off while the guest uses it.
> 
> Fix this by adding Runtime PM support, powering the device when the VFIO
> device is opened by the guest.
> 
> Note that while more fine-grained power management could be implemented
> on the guest side, if exported, this would be inherently unsafe, as
> abusing it may kill the whole system.
Please can you elaborate on this remark please?

Thanks

Eric
> 
> Signed-off-by: Geert Uytterhoeven 
> ---
> This depends on "[PATCH v3 2/2] vfio: platform: Add generic DT reset
> support" due to a small contextual change (addition of "#include
> ").
> 
>  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 ef9b9e3220ebe939..4db0a143992c3353 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -249,6 +250,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, _dbg);
>   if (ret && vdev->reset_required) {
>   dev_warn(vdev->device, "reset driver is required and 
> reset call failed in release (%d) %s\n",
> @@ -291,6 +294,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++;
> @@ -706,6 +713,7 @@ int vfio_platform_probe_common(struct 
> vfio_platform_device *vdev,
>  
>   mutex_init(>igate);
>  
> + pm_runtime_enable(vdev->device);
>   return 0;
>  
>  put_iommu:
> @@ -723,6 +731,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);
>   }
> 

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

[Auger Eric]

Hi Geert,
On 11/04/18 11:15, 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 
> Reviewed-by: Eric Auger 
> Reviewed-by: Simon Horman 
Acked-by: Eric Auger 

Thanks

Eric
> ---
> v3:
>   - Add Reviewed-by,
> 
> 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, _platform_ops, vdev);
> - if (ret) {
> - vfio_iommu_group_put(group, dev);
> - return ret;
> - }
> + if (ret)
> + goto put_iommu;
>  
>   mutex_init(>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 v3 2/2] vfio: platform: Add generic DT reset support

[Auger Eric]

Hi Geert, Philipp,

On 12/04/18 18:02, Geert Uytterhoeven wrote:
> Hi Philipp,
> 
> On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel <p.za...@pengutronix.de> wrote:
>> On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote:
>>> On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya <ok...@codeaurora.org> wrote:
>>>> On 4/12/2018 7:49 AM, Auger Eric wrote:
>>>>> On 12/04/18 13:32, Geert Uytterhoeven wrote:
>>>>>> On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.au...@redhat.com> 
>>>>>> wrote:
>>>>>>> On 11/04/18 11:15, 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+rene...@glider.be>
>>>>>>>> Reviewed-by: Philipp Zabel <p.za...@pengutronix.de>
>>>>>>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>>>>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>>>>>>>> @@ -127,8 +130,16 @@ static int vfio_platform_get_reset(struct 
>>>>>>>> vfio_platform_device *vdev)
>>>>>>>>   vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>>>>>>>>   
>>>>>>>> >reset_module);
>>>>>>>>   }
>>>>>>>> + if (vdev->of_reset)
>>>>>>>> + return 0;
>>>>>>>> +
>>>>>>>> + rstc = of_reset_control_get_exclusive(vdev->device->of_node, 
>>>>>>>> NULL);
>>>>>>>
>>>>>>> Shouldn't we prefer the top level reset_control_get_exclusive()?
>>>>>>
>>>>>> I guess that should work, too.
>>>>>>
>>>>>>> To make sure about the exclusive/shared terminology, does
>>>>>>> get_reset_control_get_exclusive() check we have an exclusive wire
>>>>>>> between this device and the reset controller?
>>>>>>
>>>>>> AFAIU, the "exclusive" means that only a single user can obtain access to
>>>>>> the reset, and it does not guarantee that we have an exclusive wire 
>>>>>> between
>>>>>> the device and the reset controller.
>>>>>>
>>>>>> The latter depends on the SoC's reset topology. If a reset wire is shared
>>>>>> by multiple devices (e.g. resets shared by PWM or Display Unit devices on
>>>>>> R-Car SoCs), exporting a subset of these devices to a guest is a bad 
>>>>>> idea,
>>>>>> indeed.
>>>>>
>>>>> So who's going to check this assigned device will not trigger a reset of
>>>>> other non assigned devices sharing the same reset controller?
>>
>> If the reset control is requested as exclusive, any other driver
>> requesting the same reset control will fail (or this reset_control_get
>> will fail, whichever comes last).
>>
>>>> I like the direction in general. I was hoping that you'd call it 
>>>> of_res

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

[Auger Eric]

Hi Geert,
On 12/04/18 13:32, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.au...@redhat.com> wrote:
>> On 11/04/18 11:15, 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+rene...@glider.be>
>>> Reviewed-by: Philipp Zabel <p.za...@pengutronix.de>
> 
>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
> 
>>> @@ -127,8 +130,16 @@ static int vfio_platform_get_reset(struct 
>>> vfio_platform_device *vdev)
>>>   vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>>>   >reset_module);
>>>   }
>>> + if (vdev->of_reset)
>>> + return 0;
>>> +
>>> + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL);
>>
>> Shouldn't we prefer the top level reset_control_get_exclusive()?
> 
> I guess that should work, too.
> 
>> To make sure about the exclusive/shared terminology, does
>> get_reset_control_get_exclusive() check we have an exclusive wire
>> between this device and the reset controller?
> 
> AFAIU, the "exclusive" means that only a single user can obtain access to
> the reset, and it does not guarantee that we have an exclusive wire between
> the device and the reset controller.
> 
> The latter depends on the SoC's reset topology. If a reset wire is shared
> by multiple devices (e.g. resets shared by PWM or Display Unit devices on
> R-Car SoCs), exporting a subset of these devices to a guest is a bad idea,
> indeed.

So who's going to check this assigned device will not trigger a reset of
other non assigned devices sharing the same reset controller?

> 
> I guess the same thing can happen with the ACPI "_RST" method?

ACPI spec _RST chapter says about _RST object:

"This object executes a reset on the associated device
 or devices. If included in a device context, the
reset must not affect any other ACPI-described de
vices; if included in a power resource for reset
(_PRR, Section 7.3.26) the reset must affect all ACPI-described devices
that reference it. When this object is described in
a device context, it executes a function level reset that only affects
the device it is associated with; neither parent nor children should be
affected by the execution of this reset. Executing this must only result
in this device resetting without the device appearing as if it
has been removed from the bus altogether, to prevent OSPM re-enumeration
of devices on hot-pluggable buses (e.g. USB)."

Adding Sinan in copy for clarification.

Thanks

Eric
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 

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

[Auger Eric]

Hi Geert, Philipp,

On 11/04/18 11:15, 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 
> Reviewed-by: Philipp Zabel 
> ---
> v3:
>   - Add Reviewed-by,
>   - Merge similar checks in vfio_platform_has_reset(),
> 
> 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  | 20 ++--
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c 
> b/drivers/vfio/platform/vfio_platform_common.c
> index b60bb5326668498c..ef9b9e3220ebe939 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -112,11 +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;
> + 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 +130,16 @@ static int vfio_platform_get_reset(struct 
> vfio_platform_device *vdev)
>   vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>   >reset_module);
>   }
> + if (vdev->of_reset)
> + return 0;
> +
> + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL);

Shouldn't we prefer the top level reset_control_get_exclusive()?

To make sure about the exclusive/shared terminology, does
get_reset_control_get_exclusive() check we have an exclusive wire
between this device and the reset controller?

Thanks

Eric
> + 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 +149,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 +230,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;
>  
>   /*
> 

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 
Reviewed-by: Eric Auger 

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, _platform_ops, vdev);
> - if (ret) {
> - vfio_iommu_group_put(group, dev);
> - return ret;
> - }
> + if (ret)
> + goto put_iommu;
>  
>   mutex_init(>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

[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
> <ge...@linux-m68k.org> wrote:
>> On Wed, Feb 14, 2018 at 9:36 AM, Auger Eric <eric.au...@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 -- 
> ge...@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/RFC 5/5] hw/arm/sysbus-fdt: Enable rcar-gen3-gpio dynamic instantiation

[Auger Eric]

Hi Geert,

On 09/02/18 16:17, Geert Uytterhoeven wrote:
> Allow the instantiation of a Renesas R-Car Gen3 GPIO controller device
> from the QEMU command line:
> 
> -device vfio-platform,host=,manufacturer=renesas,model=rcar-gen3-gpio
> -device 
> vfio-platform,sysfsdev=,manufacturer=renesas,model=rcar-gen3-gpio
> 
> A specialized device tree node is created for the guest, containing
> compatible, reg, gpio-controller, and #gpio-cells properties.
> 
> Not-Yet-Signed-off-by: Geert Uytterhoeven 
> ---
> Question:
>   - Why do we need the manufacturer=foo,model=bar syntax? Can't this
> just be extracted from the host DT?
I think this could be achieved that way too. We just need to pay
attention to the fact the dt node creation function matches the exact
same compatible property value.

> 
> TODO:
>   - Copy properties from the host DT, as add_amd_xgbe_fdt_node() does,
>   - Make this more generic?

Yes I think devising helpers to generate regs/interrupts properties
could help reducing the amount of code to be written

Thanks

Eric
> ---
>  hw/arm/sysbus-fdt.c | 47 +++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index c5d4fd5604c28118..428175f343d9f3b9 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -416,6 +416,52 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, 
> void *opaque)
>  return 0;
>  }
>  
> +/**
> + * add_rcar_gpio_fdt_node
> + *
> + * Generates a simple node with following properties:
> + * compatible string, regs, #gpio-cells, gpio-controller
> + */
> +static int add_rcar_gpio_fdt_node(SysBusDevice *sbdev, void *opaque)
> +{
> +PlatformBusFDTData *data = opaque;
> +PlatformBusDevice *pbus = data->pbus;
> +void *fdt = data->fdt;
> +const char *parent_node = data->pbus_node_name;
> +int compat_str_len, i;
> +char *nodename;
> +uint32_t *reg_attr;
> +uint64_t mmio_base;
> +VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +VFIODevice *vbasedev = >vbasedev;
> +
> +mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> +nodename = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
> +   vbasedev->name, mmio_base);
> +qemu_fdt_add_subnode(fdt, nodename);
> +
> +compat_str_len = strlen(vdev->compat) + 1;
> +qemu_fdt_setprop(fdt, nodename, "compatible",
> +  vdev->compat, compat_str_len);
> +
> +qemu_fdt_setprop(fdt, nodename, "gpio-controller", NULL, 0);
> +qemu_fdt_setprop_cells(fdt, nodename, "#gpio-cells", 2);
> +
> +reg_attr = g_new(uint32_t, vbasedev->num_regions * 2);
> +for (i = 0; i < vbasedev->num_regions; i++) {
> +mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
> +reg_attr[2 * i] = cpu_to_be32(mmio_base);
> +reg_attr[2 * i + 1] = cpu_to_be32(
> +memory_region_size(vdev->regions[i]->mem));
> +}
> +qemu_fdt_setprop(fdt, nodename, "reg", reg_attr,
> + vbasedev->num_regions * 2 * sizeof(uint32_t));
> +
> +g_free(reg_attr);
> +g_free(nodename);
> +return 0;
> +}
> +
>  /* manufacturer/model matching */
>  static bool vfio_platform_match(SysBusDevice *sbdev,
>  const BindingEntry *entry)
> @@ -454,6 +500,7 @@ static const BindingEntry bindings[] = {
>  TYPE_BINDING(TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node),
>  TYPE_BINDING(TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node),
>  VFIO_PLATFORM_BINDING("amd", "xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
> +VFIO_PLATFORM_BINDING("renesas", "rcar-gen3-gpio", 
> add_rcar_gpio_fdt_node),
>  #endif
>  TYPE_BINDING("", NULL), /* last element */
>  };
> 

Re: [PATCH/RFC 3/5] hw/arm/virt: Allow dynamic sysbus devices again

[Auger Eric]

Hi Geert,
On 09/02/18 16:17, Geert Uytterhoeven wrote:
> Allow the instantation of generic dynamic sysbus devices again, without
> the need to create a new device-specific vfio type.
> 
> This is a partial revert of commit  6f2062b9758ebc64 ("hw/arm/virt:
> Allow only supported dynamic sysbus devices").
> 
> Not-Yet-Signed-off-by: Geert Uytterhoeven 
> ---
>  hw/arm/virt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b334c82edae9fb1f..fa83784fc08ed076 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1596,6 +1596,7 @@ static void virt_machine_class_init(ObjectClass *oc, 
> void *data)
>  mc->max_cpus = 255;
>  machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC);
>  machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
> +machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SYS_BUS_DEVICE);
Couldn't you limit that to TYPE_VFIO_PLATFORM instead?

Thanks

Eric
>  mc->block_default_type = IF_VIRTIO;
>  mc->no_cdrom = 1;
>  mc->pci_allow_0_address = true;
> 

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.au...@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 -- 
> ge...@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 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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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,
>   >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 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 
> ---
>  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, _platform_ops, vdev);
> - if (ret) {
> - vfio_iommu_group_put(group, dev);
> - return ret;
> - }
> + if (ret)
> + goto put_iommu;
>  
>   mutex_init(>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);
>  
>