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

[Eric Auger <eric.auger@redhat.com>]

Hi Alex,

On 8/30/24 01:21, Alex Williamson wrote:
> On Thu, 29 Aug 2024 18:11:07 +0200
> Eric Auger <eric.auger@redhat.com> 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@redhat.com>
>> ---
>>  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.
>
>> +
>>  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.

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;						\
>>  };								\