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

[Alex Williamson]

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