Re: [RFC PATCH 3/5] vfio_platform: reset: Introduce new open and close callbacks

[Eric Auger]

Hi Alex,

On 9/4/24 21:40, Alex Williamson wrote:
> On Mon, 2 Sep 2024 18:03:23 +0200
> Eric Auger <eric.auger@xxxxxxxxxx> wrote:
>
>> Hi Alex,
>>
>> On 8/30/24 01:21, Alex Williamson wrote:
>>> On Thu, 29 Aug 2024 18:11:07 +0200
>>> Eric Auger <eric.auger@xxxxxxxxxx> wrote:
>>>  
>>>> Some devices may require resources such as clocks and resets
>>>> which cannot be handled in the vfio_platform agnostic code. Let's
>>>> add 2 new callbacks to handle those resources. Those new callbacks
>>>> are optional, as opposed to the reset callback. In case they are
>>>> implemented, both need to be.
>>>>
>>>> They are not implemented by the existing reset modules.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>>> ---
>>>>  drivers/vfio/platform/vfio_platform_common.c  | 28 ++++++++++++++++++-
>>>>  drivers/vfio/platform/vfio_platform_private.h |  6 ++++
>>>>  2 files changed, 33 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>>>> index 3be08e58365b..2174e402dc70 100644
>>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>>>> @@ -228,6 +228,23 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
>>>>  	return -EINVAL;
>>>>  }
>>>>  
>>>> +static void vfio_platform_reset_module_close(struct vfio_platform_device *vpdev)
>>>> +{
>>>> +	if (VFIO_PLATFORM_IS_ACPI(vpdev))
>>>> +		return;
>>>> +	if (vpdev->reset_ops && vpdev->reset_ops->close)
>>>> +		vpdev->reset_ops->close(vpdev);
>>>> +}
>>>> +
>>>> +static int vfio_platform_reset_module_open(struct vfio_platform_device *vpdev)
>>>> +{
>>>> +	if (VFIO_PLATFORM_IS_ACPI(vpdev))
>>>> +		return 0;
>>>> +	if (vpdev->reset_ops && vpdev->reset_ops->open)
>>>> +		return vpdev->reset_ops->open(vpdev);
>>>> +	return 0;
>>>> +}  
>>> Hi Eric,
>>>
>>> I didn't get why these are no-op'd on an ACPI platform.  Shouldn't it
>>> be up to the reset ops to decide whether to implement something based
>>> on the system firmware rather than vfio-platform-common?  
>> In case of ACPI boot, ie. VFIO_PLATFORM_IS_ACPI(vpdev) is set, I
>> understand we don't use the vfio platform reset module but the ACPI _RST
>> method. see vfio_platform_acpi_call_reset() and
>> vfio_platform_acpi_has_reset() introduced by d30daa33ec1d ("vfio:
>> platform: call _RST method when using ACPI"). I have never had the
>> opportunity to test acpi boot reset though.
> Aha, I was expecting that VFIO_PLATFORM_IS_ACPI() wouldn't exclusively
> require _RST support, but indeed in various places we only look for the
> acpihid for the device without also checking for a _RST method.  In
> fact commit 7aef80cf3187 ("vfio: platform: rename reset function")
> prefixed the reset function pointer with "of_" to try to make that
> exclusion more clear, but the previous patch of this series introducing
> the ops structure chose a more generic name.  Should we instead use
> "of_reset_ops" to maintain that we have two distinct paths, ACPI vs DT?
Yes I will rename with of_ prefix.
>
> TBH I'm not sure why we couldn't check that an acpihid also supports a
> _RST method and continue to look for reset module support otherwise,
> but that's not the way it's coded and there's apparently no demand for
> it.
I agree. Without explicit request I am reluctant to change that because
I can't test atm
>
>>>> +
>>>>  void vfio_platform_close_device(struct vfio_device *core_vdev)
>>>>  {
>>>>  	struct vfio_platform_device *vdev =
>>>> @@ -242,6 +259,7 @@ void vfio_platform_close_device(struct vfio_device *core_vdev)
>>>>  			"reset driver is required and reset call failed in release (%d) %s\n",
>>>>  			ret, extra_dbg ? extra_dbg : "");
>>>>  	}
>>>> +	vfio_platform_reset_module_close(vdev);
>>>>  	pm_runtime_put(vdev->device);
>>>>  	vfio_platform_regions_cleanup(vdev);
>>>>  	vfio_platform_irq_cleanup(vdev);
>>>> @@ -265,7 +283,13 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
>>>>  
>>>>  	ret = pm_runtime_get_sync(vdev->device);
>>>>  	if (ret < 0)
>>>> -		goto err_rst;
>>>> +		goto err_rst_open;
>>>> +
>>>> +	ret = vfio_platform_reset_module_open(vdev);
>>>> +	if (ret) {
>>>> +		dev_info(vdev->device, "reset module load failed (%d)\n", ret);
>>>> +		goto err_rst_open;
>>>> +	}
>>>>  
>>>>  	ret = vfio_platform_call_reset(vdev, &extra_dbg);
>>>>  	if (ret && vdev->reset_required) {
>>>> @@ -278,6 +302,8 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
>>>>  	return 0;
>>>>  
>>>>  err_rst:
>>>> +	vfio_platform_reset_module_close(vdev);
>>>> +err_rst_open:
>>>>  	pm_runtime_put(vdev->device);
>>>>  	vfio_platform_irq_cleanup(vdev);
>>>>  err_irq:
>>>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>>>> index 90c99d2e70f4..528b01c56de6 100644
>>>> --- a/drivers/vfio/platform/vfio_platform_private.h
>>>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>>>> @@ -74,9 +74,13 @@ struct vfio_platform_device {
>>>>   * struct vfio_platform_reset_ops - reset ops
>>>>   *
>>>>   * @reset:	reset function (required)
>>>> + * @open:	Called when the first fd is opened for this device (optional)
>>>> + * @close:	Called when the last fd is closed for this device (optional)  
>>> This doesn't note any platform firmware dependency.  We should probably
>>> also note here the XOR requirement enforced below here.  Thanks,  
>> To me this is just used along with dt boot, hence the lack of check.
> Per the above comment, I'd just specify the whole struct as a DT reset
> ops interface and sprinkle "_of_" into the name to make that more
> obvious.  Thanks,

agreed

Thanks

Eric
>
> Alex
>
>>>>   */
>>>>  struct vfio_platform_reset_ops {
>>>>  	int (*reset)(struct vfio_platform_device *vdev);
>>>> +	int (*open)(struct vfio_platform_device *vdev);
>>>> +	void (*close)(struct vfio_platform_device *vdev);
>>>>  };
>>>>  
>>>>  
>>>> @@ -129,6 +133,8 @@ __vfio_platform_register_reset(&__ops ## _node)
>>>>  MODULE_ALIAS("vfio-reset:" compat);				\
>>>>  static int __init reset ## _module_init(void)			\
>>>>  {								\
>>>> +	if (!!ops.open ^ !!ops.close)				\
>>>> +		return -EINVAL;					\
>>>>  	vfio_platform_register_reset(compat, ops);		\
>>>>  	return 0;						\
>>>>  };								\