Re: [PATCH 4/5] VFIO: platform: populate reset function according to compat

[Eric Auger <eric.auger@linaro.org>]

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.

>