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