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 <[email protected]>
> ---
> 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
> + if (!strcmp(iter->compat, compat) &&
> + try_module_get(iter->owner))
> + return iter->reset;
> + }
> +
> + return ERR_PTR(-ENODEV);
return NULL imo
> +}
> +
> +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.
> +}
> +
> +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
> + 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? This looks like it hasn't been tested. Why would we do a
get every time we want to do a reset? 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.
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
