Date:	Wed, 10 Jun 2015 13:44:30 +0200
From:	Eric Auger <eric.auger@...aro.org>
To:	Alex Williamson <alex.williamson@...hat.com>
CC:	b.reynal@...tualopensystems.com, patches@...aro.org,
	linux-kernel@...r.kernel.org, agraf@...e.de,
	linux-arm-kernel@...ts.infradead.org, eric.auger@...com,
	christoffer.dall@...aro.org
Subject: Re: [PATCH v2 3/4] VFIO: platform: populate the reset function on
 probe

On 06/09/2015 08:26 PM, Alex Williamson wrote:
> On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
>> The reset function lookup happens on vfio-platform probe. The reset
>> module load is requested  and a reference to the function symbol is
>> hold. The reference is released on vfio-platform remove.
>>
>> Signed-off-by: Eric Auger <eric.auger@...aro.org>
>>
>> ---
>>
>> v1 -> v2:
>> - [get,put]_reset now is called once on probe/remove
>> - use request_module to automatically load the reset module that
>>   matches the compatibility string
>> - lookup table is used instead of list
>> - remove registration mechanism: reset function name is stored in the
>>   lookup table.
>> - use device_property_read_string instead of
>>   device_property_read_string_array
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index 995929b..d474d6a 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>>  	},
>>  };
>>  
>> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
>> +				   struct device *dev)
>> +{
>> +	const char *compat;
>> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>> +	int (*reset)(struct vfio_platform_device *);
>> +	int ret;
>> +
>> +	vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
>> +	ret = device_property_read_string(dev, "compatible", &compat);
>> +	if (ret)
>> +		return ret;
>> +
>> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>> +		if (!strcmp(iter->compat, compat)) {
>> +			request_module(iter->module_name);
>> +			reset = __symbol_get(iter->reset_function_name);
> 
> symbol_get() appears to be the more robust and dominant interface for
> this, why use __symbol_get()? 
I used this because it takes a const char * as an argument and this is
what I use as a datatype for storing the reset function name. Symbol_get
is provided with the symbol directly? It is also used
drivers/mtd/chips/gen_probe.c.
> 
>> +			if (reset) {
>> +				vdev->type = iter->type;
>> +				vdev->reset = reset;
>> +				return 0;
>> +			}
>> +		}
>> +		iter++;
>> +	}
>> +	return -1;
> 
> -ENODEV seems preferable to -1, but shouldn't this really be a void
> function?
yes indeed
> 
>> +}
>> +
>> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>> +{
>> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>> +
>> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>> +		if (iter->type == vdev->type) {
> 
> Again, I don't see the value in storing the enum, since the table is
> static, it could just as easily be the array index and avoid this loop,
> but we can avoid it anyway with symbol_put_addr().
yes you're definitively right!

Thanks

Eric
> 
>> +			__symbol_put(iter->reset_function_name);
>> +			return;
>> +		}
>> +		iter++;
>> +	}
>> +}
>> +
>>  static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>>  {
>>  	int cnt = 0, i;
>> @@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>  		return ret;
>>  	}
>>  
>> +	vfio_platform_get_reset(vdev, dev);
>> +
>>  	mutex_init(&vdev->igate);
>>  
>>  	return 0;
>> @@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>>  	struct vfio_platform_device *vdev;
>>  
>>  	vdev = vfio_del_group_dev(dev);
>> -	if (vdev)
>> +
>> +	if (vdev) {
>> +		vfio_platform_put_reset(vdev);
>>  		iommu_group_put(dev->iommu_group);
>> +	}
>>  
>>  	return vdev;
>>  }
> 
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists