Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1443001 > unrolled thread

[PATCH V9 4/9] vfio: platform: add support for ACPI probe

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V9 4/9] vfio: platform: add support for ACPI probe Sinan Kaya <okaya@codeaurora.org> - 2016-07-14 04:10 +0200
    Re: [PATCH V9 4/9] vfio: platform: add support for ACPI probe Alex Williamson <alex.williamson@redhat.com> - 2016-07-14 23:50 +0200
      Re: [PATCH V9 4/9] vfio: platform: add support for ACPI probe Sinan Kaya <okaya@codeaurora.org> - 2016-07-16 03:30 +0200
        Re: [PATCH V9 4/9] vfio: platform: add support for ACPI probe Alex Williamson <alex.williamson@redhat.com> - 2016-07-18 19:40 +0200
          Re: [PATCH V9 4/9] vfio: platform: add support for ACPI probe Sinan Kaya <okaya@codeaurora.org> - 2016-07-18 23:30 +0200

#1443001 — [PATCH V9 4/9] vfio: platform: add support for ACPI probe

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.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c  | 70 +++++++++++++++++++++++++--
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 6be92c3..ff148764 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>
@@ -49,6 +50,33 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 	return reset_fn;
 }
 
+static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
+				    struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (acpi_disabled)
+		return -EPERM;
+
+	if (!adev) {
+		pr_err("VFIO: ACPI companion device not found for %s\n",
+			vdev->name);
+		return -ENODEV;
+	}
+
+#ifdef CONFIG_ACPI
+	vdev->acpihid = acpi_device_hid(adev);
+	if (!vdev->acpihid) {
+		pr_err("VFIO: cannot find ACPI HID for %s\n",
+		       vdev->name);
+		return -EINVAL;
+	}
+	return 0;
+#else
+	return -ENOENT;
+#endif
+}
+
 static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
 {
 	return vdev->of_reset ? true : false;
@@ -547,6 +575,37 @@ 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;
+}
+
+/*
+ * There can be two kernel build combinations. One build where
+ * ACPI is not selected in Kconfig and another one with the ACPI Kconfig.
+ *
+ * In the first case, vfio_platform_acpi_probe will return since
+ * acpi_disabled  * is 1. DT user will not see any kind of messages from
+ * ACPI.
+ *
+ * In the second case, both DT and ACPI is compiled in but the system is
+ * booting with any of these combinations.
+ *
+ * If the firmware is DT type, then acpi_disabled is 1. The ACPI probe routine
+ * terminates immediately without any messages.
+ *
+ * If the firmware is ACPI type, then acpi_disabled is 0. All other checks are
+ * valid checks. We cannot claim that this system is DT.
+ */
 int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 			       struct device *dev)
 {
@@ -556,11 +615,12 @@ 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);
+
+	if (ret)
+		return ret;
 
 	vdev->device = dev;
 
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 71ed7d1..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;
 
-- 
1.8.2.1

[toc] | [next] | [standalone]

#1443794

On Wed, 13 Jul 2016 22:06:30 -0400
Sinan Kaya <okaya@codeaurora.org> 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.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 70 +++++++++++++++++++++++++--
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  2 files changed, 66 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index 6be92c3..ff148764 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>
> @@ -49,6 +50,33 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>  	return reset_fn;
>  }
>

This function still feels a bit sloppy
  
> +static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> +				    struct device *dev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);

When !CONFIG_ACPI, this returns NULL

> +
> +	if (acpi_disabled)

When !CONFIG_ACPI, this is defined as 1, so we'll always exit here.

> +		return -EPERM;
> +
> +	if (!adev) {

This is really the only (ACPI_CONFIG && !acpi_disabled) error exit,
because...

> +		pr_err("VFIO: ACPI companion device not found for %s\n",
> +			vdev->name);
> +		return -ENODEV;
> +	}
> +
> +#ifdef CONFIG_ACPI
> +	vdev->acpihid = acpi_device_hid(adev);

This can't actually return NULL.  So the test below is unreached.
Maybe we should just conclude this function here with:

#endif

return vdev->acpihid ? 0 : -ENOENT;

which is even still a bit paranoid since it can't actually happen.

> +	if (!vdev->acpihid) {
> +		pr_err("VFIO: cannot find ACPI HID for %s\n",
> +		       vdev->name);
> +		return -EINVAL;
> +	}
> +	return 0;
> +#else
> +	return -ENOENT;
> +#endif
> +}
> +
>  static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
>  {
>  	return vdev->of_reset ? true : false;
> @@ -547,6 +575,37 @@ 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;
> +}
> +
> +/*
> + * There can be two kernel build combinations. One build where
> + * ACPI is not selected in Kconfig and another one with the ACPI Kconfig.
> + *
> + * In the first case, vfio_platform_acpi_probe will return since
> + * acpi_disabled  * is 1. DT user will not see any kind of messages from

                    ^^ Previous editing cruft?

> + * ACPI.
> + *
> + * In the second case, both DT and ACPI is compiled in but the system is
> + * booting with any of these combinations.
> + *
> + * If the firmware is DT type, then acpi_disabled is 1. The ACPI probe routine
> + * terminates immediately without any messages.
> + *
> + * If the firmware is ACPI type, then acpi_disabled is 0. All other checks are
> + * valid checks. We cannot claim that this system is DT.
> + */
>  int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  			       struct device *dev)
>  {
> @@ -556,11 +615,12 @@ 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);
> +
> +	if (ret)
> +		return ret;
>  
>  	vdev->device = dev;
>  
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 71ed7d1..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;
>  

[toc] | [prev] | [next] | [standalone]

#1444716

On 7/14/2016 5:41 PM, Alex Williamson wrote:
> On Wed, 13 Jul 2016 22:06:30 -0400
> Sinan Kaya <okaya@codeaurora.org> 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.
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c  | 70 +++++++++++++++++++++++++--
>>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>>  2 files changed, 66 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index 6be92c3..ff148764 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>
>> @@ -49,6 +50,33 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>>  	return reset_fn;
>>  }
>>
> 
> This function still feels a bit sloppy
>   
>> +static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>> +				    struct device *dev)
>> +{
>> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> 
> When !CONFIG_ACPI, this returns NULL
> 
>> +
>> +	if (acpi_disabled)
> 
> When !CONFIG_ACPI, this is defined as 1, so we'll always exit here.
> 
>> +		return -EPERM;
>> +

I'll move this here and leave the variable definition above only.

	adev = ACPI_COMPANION(dev);

>> +	if (!adev) {
> 
> This is really the only (ACPI_CONFIG && !acpi_disabled) error exit,
> because...
> 
>> +		pr_err("VFIO: ACPI companion device not found for %s\n",
>> +			vdev->name);
>> +		return -ENODEV;
>> +	}
>> +
>> +#ifdef CONFIG_ACPI
>> +	vdev->acpihid = acpi_device_hid(adev);
> 

Based on the current implementation of acpi_device_hid, the function wlll 
return a name of "device" when the pnp device id list is empty. 

Do you want to rely on the current implementation behavior rather than be
safe?

> This can't actually return NULL.  So the test below is unreached.
> Maybe we should just conclude this function here with:
> 
> #endif
> 
> return vdev->acpihid ? 0 : -ENOENT;
> 
> which is even still a bit paranoid since it can't actually happen.
> 
>> +	if (!vdev->acpihid) {
>> +		pr_err("VFIO: cannot find ACPI HID for %s\n",
>> +		       vdev->name);
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +#else
>> +	return -ENOENT;
>> +#endif
>> +}
>> +


>> +
>> +/*
>> + * There can be two kernel build combinations. One build where
>> + * ACPI is not selected in Kconfig and another one with the ACPI Kconfig.
>> + *
>> + * In the first case, vfio_platform_acpi_probe will return since
>> + * acpi_disabled  * is 1. DT user will not see any kind of messages from
> 
>                     ^^ Previous editing cruft?

Yep, good catch. Got warned by checkpatch for 80 characters. 



-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1445663

On Fri, 15 Jul 2016 21:27:24 -0400
Sinan Kaya <okaya@codeaurora.org> wrote:

> On 7/14/2016 5:41 PM, Alex Williamson wrote:
> > On Wed, 13 Jul 2016 22:06:30 -0400
> > Sinan Kaya <okaya@codeaurora.org> 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.
> >>
> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> >> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> >> Reviewed-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> >> ---
> >>  drivers/vfio/platform/vfio_platform_common.c  | 70 +++++++++++++++++++++++++--
> >>  drivers/vfio/platform/vfio_platform_private.h |  1 +
> >>  2 files changed, 66 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> >> index 6be92c3..ff148764 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>
> >> @@ -49,6 +50,33 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
> >>  	return reset_fn;
> >>  }
> >>  
> > 
> > This function still feels a bit sloppy
> >     
> >> +static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> >> +				    struct device *dev)
> >> +{
> >> +	struct acpi_device *adev = ACPI_COMPANION(dev);  
> > 
> > When !CONFIG_ACPI, this returns NULL
> >   
> >> +
> >> +	if (acpi_disabled)  
> > 
> > When !CONFIG_ACPI, this is defined as 1, so we'll always exit here.
> >   
> >> +		return -EPERM;
> >> +  
> 
> I'll move this here and leave the variable definition above only.
> 
> 	adev = ACPI_COMPANION(dev);
> 
> >> +	if (!adev) {  
> > 
> > This is really the only (ACPI_CONFIG && !acpi_disabled) error exit,
> > because...
> >   
> >> +		pr_err("VFIO: ACPI companion device not found for %s\n",
> >> +			vdev->name);
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +#ifdef CONFIG_ACPI
> >> +	vdev->acpihid = acpi_device_hid(adev);  
> >   
> 
> Based on the current implementation of acpi_device_hid, the function wlll 
> return a name of "device" when the pnp device id list is empty. 
> 
> Do you want to rely on the current implementation behavior rather than be
> safe?

The implementation looks pretty fixed, it seems like a lot would break
if that were changed, so the callers of the function would need to be
updated, including this one.  The ternary below is already paranoid
enough to handle an API change, but if you want to add one more level
of paranoia, you could use WARN_ON(!vdev->acpihid) to make it user
visible.  It could still be done as:

return WARN_ON(!vdev->acpihid) ? -ENOENT : 0;

Thanks,
Alex
 
> > This can't actually return NULL.  So the test below is unreached.
> > Maybe we should just conclude this function here with:
> > 
> > #endif
> > 
> > return vdev->acpihid ? 0 : -ENOENT;
> > 
> > which is even still a bit paranoid since it can't actually happen.
> >   
> >> +	if (!vdev->acpihid) {
> >> +		pr_err("VFIO: cannot find ACPI HID for %s\n",
> >> +		       vdev->name);
> >> +		return -EINVAL;
> >> +	}
> >> +	return 0;
> >> +#else
> >> +	return -ENOENT;
> >> +#endif
> >> +}
> >> +  
> 
> 
> >> +
> >> +/*
> >> + * There can be two kernel build combinations. One build where
> >> + * ACPI is not selected in Kconfig and another one with the ACPI Kconfig.
> >> + *
> >> + * In the first case, vfio_platform_acpi_probe will return since
> >> + * acpi_disabled  * is 1. DT user will not see any kind of messages from  
> > 
> >                     ^^ Previous editing cruft?  
> 
> Yep, good catch. Got warned by checkpatch for 80 characters. 
> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1445857

On 7/18/2016 1:32 PM, Alex Williamson wrote:
> The implementation looks pretty fixed, it seems like a lot would break
> if that were changed, so the callers of the function would need to be
> updated, including this one.  The ternary below is already paranoid
> enough to handle an API change, but if you want to add one more level
> of paranoia, you could use WARN_ON(!vdev->acpihid) to make it user
> visible.  It could still be done as:
> 
> return WARN_ON(!vdev->acpihid) ? -ENOENT : 0;

OK. I'm adding this and will post a new version after testing.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web