| Message ID | 1431008843-28411-5-git-send-email-eric.auger@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 05/13/2015 08:33 PM, Alex Williamson wrote: > On Thu, 2015-05-07 at 16:27 +0200, Eric Auger wrote: >> Add the reset function lookup according to the device compat >> string. This lookup is added at different places: >> - on VFIO_DEVICE_GET_INFO >> - on VFIO_DEVICE_RESET >> - on device release >> >> A reference to the module implementing the reset function is taken >> on first reset function lookup and released on vfio platform device >> release. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> --- >> drivers/vfio/platform/vfio_platform_common.c | 50 ++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c >> index 0d10018..bd7e44c 100644 >> --- a/drivers/vfio/platform/vfio_platform_common.c >> +++ b/drivers/vfio/platform/vfio_platform_common.c >> @@ -28,6 +28,52 @@ LIST_HEAD(reset_list); >> >> static DEFINE_MUTEX(driver_lock); >> >> +static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat) >> +{ >> + struct vfio_platform_reset_node *iter; >> + >> + list_for_each_entry(iter, &reset_list, link) { > > Racy ok > >> + if (!strcmp(iter->compat, compat) && >> + try_module_get(iter->owner)) >> + return iter->reset; >> + } >> + >> + return ERR_PTR(-ENODEV); > > return NULL imo ok > >> +} >> + >> +static vfio_platform_reset_fn_t vfio_platform_get_reset( >> + struct vfio_platform_device *vdev) >> +{ >> + struct device *dev = vdev->get_device(vdev); >> + const char *compat_str_array[2]; >> + vfio_platform_reset_fn_t reset; >> + int ret; >> + >> + if (!IS_ERR_OR_NULL(vdev->reset)) >> + return vdev->reset; >> + >> + ret = device_property_read_string_array(dev, "compatible", >> + compat_str_array, 2); >> + if (!ret) >> + return ERR_PTR(ret); >> + >> + reset = vfio_platform_lookup_reset(compat_str_array[0]); >> + return reset; > > Something got allocated into compat_str_array and gets leaked here. is there any allocation? device_property_read_string_array does not return -ENOMEM. > >> +} >> + >> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev) >> +{ >> + struct vfio_platform_reset_node *iter; >> + >> + list_for_each_entry(iter, &reset_list, link) { > > Racy ok > >> + if (iter->reset == vdev->reset) { >> + module_put(iter->owner); >> + vdev->reset = NULL; >> + return; >> + } >> + } >> +} >> + >> static int vfio_platform_regions_init(struct vfio_platform_device *vdev) >> { >> int cnt = 0, i; >> @@ -103,10 +149,12 @@ static void vfio_platform_release(void *device_data) >> mutex_lock(&driver_lock); >> >> if (!(--vdev->refcnt)) { >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> vdev->reset(vdev); >> vfio_platform_regions_cleanup(vdev); >> vfio_platform_irq_cleanup(vdev); >> + vfio_platform_put_reset(vdev); >> } >> >> mutex_unlock(&driver_lock); >> @@ -164,6 +212,7 @@ static long vfio_platform_ioctl(void *device_data, >> if (info.argsz < minsz) >> return -EINVAL; >> >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> vdev->flags |= VFIO_DEVICE_FLAGS_RESET; >> info.flags = vdev->flags; >> @@ -260,6 +309,7 @@ static long vfio_platform_ioctl(void *device_data, >> return ret; >> >> } else if (cmd == VFIO_DEVICE_RESET) { >> + vdev->reset = vfio_platform_get_reset(vdev); >> if (!IS_ERR_OR_NULL(vdev->reset)) >> return vdev->reset(vdev); >> else > > I count 3 gets and 1 put, isn't the module reference count increase > showing that? vfio_platform_get_reset simply returns if the function pointer already is populated so there is no systematic ref increment. This looks like it hasn't been tested. It did testing with external and in-kernel modules through Why would we do a > get every time we want to do a reset? My doubt were about the order of probing between the vfio-platform_driver and the vfio reset module? This question was the rationale of this implementation choice. But again the actual ref count increment is devised to be done once on the first entry point (iotcl or internal release) Do one get when the device is > registered or opened and one put when the device is unregistered or > closed. We don't want erratic userspace behavior that the reset > property of a device can come and go. >
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index 0d10018..bd7e44c 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -28,6 +28,52 @@ LIST_HEAD(reset_list); static DEFINE_MUTEX(driver_lock); +static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat) +{ + struct vfio_platform_reset_node *iter; + + list_for_each_entry(iter, &reset_list, link) { + if (!strcmp(iter->compat, compat) && + try_module_get(iter->owner)) + return iter->reset; + } + + return ERR_PTR(-ENODEV); +} + +static vfio_platform_reset_fn_t vfio_platform_get_reset( + struct vfio_platform_device *vdev) +{ + struct device *dev = vdev->get_device(vdev); + const char *compat_str_array[2]; + vfio_platform_reset_fn_t reset; + int ret; + + if (!IS_ERR_OR_NULL(vdev->reset)) + return vdev->reset; + + ret = device_property_read_string_array(dev, "compatible", + compat_str_array, 2); + if (!ret) + return ERR_PTR(ret); + + reset = vfio_platform_lookup_reset(compat_str_array[0]); + return reset; +} + +static void vfio_platform_put_reset(struct vfio_platform_device *vdev) +{ + struct vfio_platform_reset_node *iter; + + list_for_each_entry(iter, &reset_list, link) { + if (iter->reset == vdev->reset) { + module_put(iter->owner); + vdev->reset = NULL; + return; + } + } +} + static int vfio_platform_regions_init(struct vfio_platform_device *vdev) { int cnt = 0, i; @@ -103,10 +149,12 @@ static void vfio_platform_release(void *device_data) mutex_lock(&driver_lock); if (!(--vdev->refcnt)) { + vdev->reset = vfio_platform_get_reset(vdev); if (!IS_ERR_OR_NULL(vdev->reset)) vdev->reset(vdev); vfio_platform_regions_cleanup(vdev); vfio_platform_irq_cleanup(vdev); + vfio_platform_put_reset(vdev); } mutex_unlock(&driver_lock); @@ -164,6 +212,7 @@ static long vfio_platform_ioctl(void *device_data, if (info.argsz < minsz) return -EINVAL; + vdev->reset = vfio_platform_get_reset(vdev); if (!IS_ERR_OR_NULL(vdev->reset)) vdev->flags |= VFIO_DEVICE_FLAGS_RESET; info.flags = vdev->flags; @@ -260,6 +309,7 @@ static long vfio_platform_ioctl(void *device_data, return ret; } else if (cmd == VFIO_DEVICE_RESET) { + vdev->reset = vfio_platform_get_reset(vdev); if (!IS_ERR_OR_NULL(vdev->reset)) return vdev->reset(vdev); else
Add the reset function lookup according to the device compat string. This lookup is added at different places: - on VFIO_DEVICE_GET_INFO - on VFIO_DEVICE_RESET - on device release A reference to the module implementing the reset function is taken on first reset function lookup and released on vfio platform device release. Signed-off-by: Eric Auger <eric.auger@linaro.org> --- drivers/vfio/platform/vfio_platform_common.c | 50 ++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)