On Wed, 13 Jul 2016 15:36:48 -0400
Sinan Kaya [off-list ref] wrote:

Hi Alex,

On 6/23/2016 2:34 PM, Alex Williamson wrote:

quoted

On Mon, 20 Jun 2016 11:51:14 -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  | 57 ++++++++++++++++++++++++---
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 6be92c3..fbf4565 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,37 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 	return reset_fn;
 }
 
+#ifdef CONFIG_ACPI
+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 -ENODEV;
+
+	if (!adev) {
+		pr_err("VFIO: ACPI companion device not found for %s\n",
+			vdev->name);
+		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;
+	}  

Do you want to try to use different errnos here so you don't rely on
the pr_err() calls for debugging?  I could imagine -EPERM, -ENODEV,
-EINVAL respectively, but maybe there are better options.
  

will do.

quoted

quoted

+	return 0;
+}
+#else
+static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
+					   struct device *dev)
+{
+	return -ENOENT;
+}
+#endif
+
 static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
 {
 	return vdev->of_reset ? true : false;

@@ -547,6 +579,20 @@ 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);  

Previously there was only one probe method and I imagine this pr_err
was useful.  Now we have multiple methods of probing for the device.
Do we really want each one generating pr_err messages or just one at
the end if none of our probes worked?  

IMO, the new approach is better and is more specific. The error messages
are firmware specific. The previous message included missing compat string
for instance doesn't exist on ACPI firmware and ACPI HID also doesn't exist
on DT firmware. 

I'd rather be verbose rather than a simple probe failed message.

quoted

quoted

+
+	return ret;
+}
+
 int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 			       struct device *dev)
 {

@@ -556,11 +602,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);  

The only out way out of vfio_platform_acpi_probe() without hitting a
pr_err is one of (!CONFIG_ACPI || acpi_disabled || success).  Doesn't
that make for some unnecessary warning for a DT user?  

Let me explain the rationale.

As you know, 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, CONFIG_ACPI is stubbed out in this file and 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.

Thanks, this sort of information and assumption should be documented in
a comment since not all of us care whether a DT device can appear in an
ACPI config or not.  Also note that acpi_disabled and ACPI_COMPANION
are defined regardless of CONFIG_ACPI, so really we only need to wrap
acpi_device_hid() in an #ifdef, we could skip the separate stub.
Thanks,

Alex

quoted

quoted

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