| Message ID | 20240829161302.607928-4-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vfio: platform: reset: Introduce tegra234 mgbe reset module | expand |
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? > + > 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, 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; \ > }; \
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; \ >> }; \
On Mon, 2 Sep 2024 18:03:23 +0200 Eric Auger <eric.auger@redhat.com> 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@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. 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; \ > >> }; \ >
Hi Alex, On 9/4/24 21:40, Alex Williamson wrote: > On Mon, 2 Sep 2024 18:03:23 +0200 > Eric Auger <eric.auger@redhat.com> 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@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. > 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; \ >>>> }; \
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; +} + 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) */ 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; \ }; \
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(-)