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