On Fri, 15 Jul 2016 21:27:24 -0400
Sinan Kaya [off-list ref] wrote:

On 7/14/2016 5:41 PM, Alex Williamson wrote:

quoted

On Wed, 13 Jul 2016 22:06:30 -0400
Sinan Kaya [off-list ref] wrote:
  

quoted

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 <redacted>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Baptiste Reynal <redacted>
---
 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
    

quoted

+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
  

quoted

+
+	if (acpi_disabled)  

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

quoted

+		return -EPERM;
+  

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

	adev = ACPI_COMPANION(dev);

quoted

quoted

+	if (!adev) {  

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

quoted

+		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
 

quoted

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.
  

quoted

+	if (!vdev->acpihid) {
+		pr_err("VFIO: cannot find ACPI HID for %s\n",
+		       vdev->name);
+		return -EINVAL;
+	}
+	return 0;
+#else
+	return -ENOENT;
+#endif
+}
+  

quoted

quoted

+
+/*
+ * 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.