Message-ID: <1433874370.4927.113.camel@redhat.com>
Subject: Re: [PATCH v2 3/4] VFIO: platform: populate the reset function on
 probe
From: Alex Williamson <alex.williamson@redhat.com>
To: Eric Auger <eric.auger@linaro.org>
Cc: eric.auger@st.com, linux-arm-kernel@lists.infradead.org,
        b.reynal@virtualopensystems.com, christoffer.dall@linaro.org,
        linux-kernel@vger.kernel.org, patches@linaro.org, agraf@suse.de
Date: Tue, 09 Jun 2015 12:26:10 -0600
In-Reply-To: <1433516792-16397-4-git-send-email-eric.auger@linaro.org>
References: <1433516792-16397-1-git-send-email-eric.auger@linaro.org>
	 <1433516792-16397-4-git-send-email-eric.auger@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3564
Lines: 117

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@linaro.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()? 

> +			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?

> +}
> +
> +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().

> +			__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;
>  }


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