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