Re: [PATCH V4 1/2] vfio, platform: add support for ACPI during probe and reset

[Sinan Kaya <okaya@codeaurora.org>]

On 5/9/2016 11:47 AM, Eric Auger wrote:
> Hi Sinan,
> On 05/01/2016 11:07 PM, Sinan Kaya wrote:
>> The code is using the compatible DT string to associate a reset driver with
>> the actual device itself. The compatible string does not exist on ACPI
>> based systems. HID is the unique identifier for a device driver instead.
>>
>> When ACPI is enabled, the change will query the presence of _RST method
>> and will call it instead of querying an external reset driver.
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c  | 176 +++++++++++++++++++++-----
>>  drivers/vfio/platform/vfio_platform_private.h |   7 +-
>>  2 files changed, 147 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index e65b142..528ec04 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -13,6 +13,7 @@
>>   */
>>  
>>  #include <linux/device.h>
>> +#include <linux/acpi.h>
>>  #include <linux/iommu.h>
>>  #include <linux/module.h>
>>  #include <linux/mutex.h>
>> @@ -41,7 +42,7 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>>  		if (!strcmp(iter->compat, compat) &&
>>  			try_module_get(iter->owner)) {
>>  			*module = iter->owner;
>> -			reset_fn = iter->reset;
>> +			reset_fn = iter->of_reset;
>>  			break;
>>  		}
>>  	}
>> @@ -49,20 +50,111 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>>  	return reset_fn;
>>  }
>>  
>> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>> +
>> +#ifdef CONFIG_ACPI
>> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>> +			     struct device *dev)
>>  {
>> -	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
>> -						&vdev->reset_module);
>> -	if (!vdev->reset) {
>> -		request_module("vfio-reset:%s", vdev->compat);
>> -		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
>> -							 &vdev->reset_module);
>> +	struct acpi_device *adev = ACPI_COMPANION(dev);
>> +
>> +	if (acpi_disabled)
>> +		return -ENODEV;
>> +
>> +	if (!adev)
>> +		return -ENODEV;
>> +
>> +	vdev->acpihid = acpi_device_hid(adev);
>> +	if (!vdev->acpihid) {
>> +		pr_err("VFIO: cannot find ACPI HID for %s\n",
>> +		       vdev->name);
>> +		return -ENODEV;
>>  	}
>> +	return 0;
>> +}
>> +
>> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
>> +{
>> +	struct device *dev = vdev->device;
>> +	acpi_handle handle = ACPI_HANDLE(dev);
>> +	acpi_status acpi_ret;
>> +	unsigned long long val;
>> +
>> +	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
>> +	if (ACPI_FAILURE(acpi_ret))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
>> +{
>> +	struct device *dev = vdev->device;
>> +	acpi_handle handle = ACPI_HANDLE(dev);
>> +
>> +	if (acpi_has_method(handle, "_RST"))
>> +		return 0;
> Can't you directly return acpi_has_method(handle, "_RST"), hence return
> a boolean?

Makes sense. I'll do that on the next post. 

>> +
>> +	return -EINVAL;
>> +}
>> +#else
>> +int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>> +			     struct device *dev)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static int vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
>> +{
>> +	return -EINVAL;
>> +}
>> +#endif
>> +
>> +static int vfio_platform_has_reset(struct vfio_platform_device *vdev)
>> +{
> return a boolean?

OK

>> +	if (vdev->acpihid)
>> +		return vfio_platform_acpi_has_reset(vdev);
>> +
>> +	if (vdev->of_reset)
>> +		return 0;
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>> +{
>> +	int rc;
>> +
>> +	if (vdev->acpihid)
>> +		return vfio_platform_acpi_has_reset(vdev);
>> +
>> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>> +						    &vdev->reset_module);
>> +	if (vdev->of_reset)
>> +		return 0;
>> +
>> +	rc = request_module("vfio-reset:%s", vdev->compat);
>> +	if (rc)
>> +		return rc;
>> +
>> +	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>> +						    &vdev->reset_module);
>> +	if (vdev->of_reset)
>> +		return 0;
>> +
>> +	return -EINVAL;
>>  }
>>  
>>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>  {
>> -	if (vdev->reset)
>> +	if (vdev->acpihid)
>> +		return;
>> +
>> +	if (vdev->of_reset)
>>  		module_put(vdev->reset_module);
>>  }
>>  
>> @@ -134,6 +226,20 @@ static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
>>  	kfree(vdev->regions);
>>  }
>>  
>> +static int vfio_platform_call_reset(struct vfio_platform_device *vdev)
>> +{
>> +	if (vdev->of_reset) {
>> +		dev_info(vdev->device, "reset\n");
>> +		vdev->of_reset(vdev);
> return vdev->of_reset(vdev) to align with acpi reset behavior.

OK. I didn't realize it was returning a value. 

>> +		return 0;
>> +	} else if (vdev->acpihid) {
>> +		return vfio_platform_acpi_call_reset(vdev);
> I think it would make sense to have the same dev_info traces for dt and
> acpi.
>> +	}
>> +
>> +	dev_warn(vdev->device, "no reset function found!\n");
>> +	return -EINVAL;
>> +}
>> +
>>  static void vfio_platform_release(void *device_data)
>>  {
>>  	struct vfio_platform_device *vdev = device_data;
>> @@ -141,12 +247,7 @@ static void vfio_platform_release(void *device_data)
>>  	mutex_lock(&driver_lock);
>>  
>>  	if (!(--vdev->refcnt)) {
>> -		if (vdev->reset) {
>> -			dev_info(vdev->device, "reset\n");
>> -			vdev->reset(vdev);
>> -		} else {
>> -			dev_warn(vdev->device, "no reset function found!\n");
>> -		}
>> +		vfio_platform_call_reset(vdev);
>>  		vfio_platform_regions_cleanup(vdev);
>>  		vfio_platform_irq_cleanup(vdev);
>>  	}
>> @@ -175,12 +276,9 @@ static int vfio_platform_open(void *device_data)
>>  		if (ret)
>>  			goto err_irq;
>>  
>> -		if (vdev->reset) {
>> -			dev_info(vdev->device, "reset\n");
>> -			vdev->reset(vdev);
>> -		} else {
>> -			dev_warn(vdev->device, "no reset function found!\n");
>> -		}
>> +		ret = vfio_platform_call_reset(vdev);
>> +		if (ret)
>> +			goto err_irq;
> This change should be in next patch since if you fail finding a reset
> function, you clean things up, mandating a reset function to exist.
> 
> Also in case the reset function fails you change the behavior which is
> not detailed in the commit msg.

I admitted this on another patch. Yes, I'll fix this.

>>  	}
>>  
>>  	vdev->refcnt++;
>> @@ -213,7 +311,7 @@ static long vfio_platform_ioctl(void *device_data,
>>  		if (info.argsz < minsz)
>>  			return -EINVAL;
>>  
>> -		if (vdev->reset)
>> +		if (!vfio_platform_has_reset(vdev))
>>  			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
>>  		info.flags = vdev->flags;
>>  		info.num_regions = vdev->num_regions;
>> @@ -312,10 +410,7 @@ static long vfio_platform_ioctl(void *device_data,
>>  		return ret;
>>  
>>  	} else if (cmd == VFIO_DEVICE_RESET) {
>> -		if (vdev->reset)
>> -			return vdev->reset(vdev);
>> -		else
>> -			return -EINVAL;
>> +		return vfio_platform_call_reset(vdev);
> Here you also change the behavior: before, in case the dt reset failed
> we returned an error. Now we return 0.

Yes, I'll fix it. 
> 
> To me this patch would deserve to be split into 2 parts: ACPI probing
> without reset and then ACPI reset. In case you change the behavior of
> existing dt reset, please put that in a separate patch.

Sure, I'll move vfio_platform_probe_common changes to a seperate patch.
I'll try to break it as much as I can before the post.

> 
> Best Regards
> 
> Eric
>>  	}
>>  
>>  	return -ENOTTY;
>> @@ -544,6 +639,21 @@ static const struct vfio_device_ops vfio_platform_ops = {
>>  	.mmap		= vfio_platform_mmap,
>>  };
>>  
>> +int vfio_platform_of_probe(struct vfio_platform_device *vdev,
>> +			   struct device *dev)
>> +{
>> +	int ret;
>> +
>> +	ret = device_property_read_string(dev, "compatible",
>> +					  &vdev->compat);
>> +	if (ret) {
>> +		pr_err("VFIO: cannot retrieve compat for %s\n",
>> +			vdev->name);
>> +		return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>>  int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>  			       struct device *dev)
>>  {
>> @@ -553,14 +663,14 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>  	if (!vdev)
>>  		return -EINVAL;
>>  
>> -	ret = device_property_read_string(dev, "compatible", &vdev->compat);
>> -	if (ret) {
>> -		pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
>> -		return -EINVAL;
>> -	}
>> +	ret = vfio_platform_acpi_probe(vdev, dev);
>> +	if (ret)
>> +		ret = vfio_platform_of_probe(vdev, dev);
>>  
>> -	vdev->device = dev;
>> +	if (ret)
>> +		return ret;
>>  
>> +	vdev->device = dev;
>>  	group = iommu_group_get(dev);
>>  	if (!group) {
>>  		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
>> @@ -611,7 +721,7 @@ void vfio_platform_unregister_reset(const char *compat,
>>  
>>  	mutex_lock(&driver_lock);
>>  	list_for_each_entry_safe(iter, temp, &reset_list, link) {
>> -		if (!strcmp(iter->compat, compat) && (iter->reset == fn)) {
>> +		if (!strcmp(iter->compat, compat) && (iter->of_reset == fn)) {
>>  			list_del(&iter->link);
>>  			break;
>>  		}
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index 42816dd..ba9e4f8 100644
>> --- a/drivers/vfio/platform/vfio_platform_private.h
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -58,6 +58,7 @@ struct vfio_platform_device {
>>  	struct mutex			igate;
>>  	struct module			*parent_module;
>>  	const char			*compat;
>> +	const char			*acpihid;
>>  	struct module			*reset_module;
>>  	struct device			*device;
>>  
>> @@ -71,7 +72,7 @@ struct vfio_platform_device {
>>  	struct resource*
>>  		(*get_resource)(struct vfio_platform_device *vdev, int i);
>>  	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
>> -	int	(*reset)(struct vfio_platform_device *vdev);
>> +	int	(*of_reset)(struct vfio_platform_device *vdev);
>>  };
>>  
>>  typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
>> @@ -80,7 +81,7 @@ struct vfio_platform_reset_node {
>>  	struct list_head link;
>>  	char *compat;
>>  	struct module *owner;
>> -	vfio_platform_reset_fn_t reset;
>> +	vfio_platform_reset_fn_t of_reset;
>>  };
>>  
>>  extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>> @@ -103,7 +104,7 @@ extern void vfio_platform_unregister_reset(const char *compat,
>>  static struct vfio_platform_reset_node __reset ## _node = {	\
>>  	.owner = THIS_MODULE,					\
>>  	.compat = __compat,					\
>> -	.reset = __reset,					\
>> +	.of_reset = __reset,					\
>>  };								\
>>  __vfio_platform_register_reset(&__reset ## _node)
>>  
>>
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project