[PATCH] ACPI / scan: Add a scan handler for PRP0001

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j...@intel.com>

If the special PRP0001 device ID is present in the given device's list
of ACPI/PNP IDs and the device has a valid "compatible" property in
the _DSD, it should be enumerated using the default mechanism,
unless some scan handlers match the IDs preceding PRP0001 in the
device's list of ACPI/PNP IDs. In particular, no scan handlers
matching the IDs following PRP0001 in that list should be attached
to the device.

To make that happen, define a scan handler that will match PRP0001
and trigger the default enumeration for the matching devices if the
"compatible" property is present for them.

Since that requires the check for platform_id and device->handler
to be removed from acpi_default_enumeration(), move the fallback
invocation of acpi_default_enumeration() to acpi_bus_attach()
(after it's checked if there's a matching ACPI driver for the
device), which is a better place to call it, and do the platform_id
check in there too (device->handler is guaranteed to be unset at
the point where the function is looking for a matching ACPI driver).

Signed-off-by: Rafael J. Wysocki <rafael.j...@intel.com>
---
drivers/acpi/scan.c | 34 +++++++++++++++++++++++++++++-----
1 file changed, 29 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -2390,9 +2390,6 @@ static void acpi_default_enumeration(str
struct list_head resource_list;
bool is_spi_i2c_slave = false;

- if (!device->pnp.type.platform_id || device->handler)
- return;
-
/*
* Do not enemerate SPI/I2C slaves as they will be enuerated by their
* respective parents.
@@ -2405,6 +2402,30 @@ static void acpi_default_enumeration(str
acpi_create_platform_device(device);
}

+static const struct acpi_device_id generic_device_ids[] = {
+ {"PRP0001", },
+ {"", },
+};
+
+static int acpi_generic_device_attach(struct acpi_device *adev,
+ const struct acpi_device_id *not_used)
+{
+ /*
+ * Since PRP0001 is the only ID handled here, the test below can be
+ * unconditional.
+ */
+ if (adev->data.of_compatible) {
+ acpi_default_enumeration(adev);
+ return 1;
+ }
+ return 0;
+}
+
+static struct acpi_scan_handler generic_device_handler = {
+ .ids = generic_device_ids,
+ .attach = acpi_generic_device_attach,
+};
+
static int acpi_scan_attach_handler(struct acpi_device *device)
{
struct acpi_hardware_id *hwid;
@@ -2430,8 +2451,6 @@ static int acpi_scan_attach_handler(stru
break;
}
}
- if (!ret)
- acpi_default_enumeration(device);

return ret;
}
@@ -2473,6 +2492,9 @@ static void acpi_bus_attach(struct acpi_
ret = device_attach(&device->dev);
if (ret < 0)
return;
+
+ if (!ret && device->pnp.type.platform_id)
+ acpi_default_enumeration(device);
}
device->flags.visited = true;

@@ -2631,6 +2653,8 @@ int __init acpi_scan_init(void)
acpi_pnp_init();
acpi_int340x_thermal_init();

+ acpi_scan_add_handler(&generic_device_handler);
+
mutex_lock(&acpi_scan_lock);
/*
* Enumerate devices in the ACPI namespace.

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

[Darren Hart]

Would a warning be appropriate here? PRP0001 should only appear when paired with
a DSD of GUID Device Properties with a "compatible" entry. If not, it's an
error, correct? I believe we warn on similarly malformed AML?

Otherwise,

Acked-by: Darren Hart <dvh...@linux.intel.com>

--
Darren Hart
Intel Open Source Technology Center

[Rafael J. Wysocki]

We don't do that as a rule as there would be too many warnings that are not
really useful. Users can't do much about those things at this stage (buggy
firmware has shipped already) and for the firmware people it is better to
cover things like that in firmware test suites (which in theory may help to
avoid shipping buggy firmware in the first place).

That said we print a warning in acpi_init_of_compatible() if things are not
consistent (which doesn't cover the case when _DSD is missing entirely, though),
so IMO it'd be better to refine that one instead of adding a new one which
wouldn't cover all cases too (eg. if PRP0001 is not the first ID in the list
and a previous one is matched to a different scan handler).

Rafael

[Rafael J. Wysocki]

Maybe something like the patch below.

---

From: Rafael J. Wysocki <rafael.j...@intel.com>

Subject: ACPI / property: Refine consistency check for PRP0001

Refine the check for the presence of the "compatible" property
if the PRP0001 device ID is present in the device's list of
ACPI/PNP IDs to also print the message if _DSD is missing
entirely or the format of it is incorrect.

While at it, reduce the log level of the message to "info"
and reduce the log level of the "broken _DSD" message to
"debug" (noise reduction).

Signed-off-by: Rafael J. Wysocki <rafael.j...@intel.com>
---

drivers/acpi/property.c | 50 ++++++++++++++++++++++++------------------------
1 file changed, 25 insertions(+), 25 deletions(-)

Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -79,35 +79,15 @@ static bool acpi_properties_format_valid
static void acpi_init_of_compatible(struct acpi_device *adev)
{
const union acpi_object *of_compatible;
- struct acpi_hardware_id *hwid;
- bool acpi_of = false;
int ret;

- /*
- * Check if the special PRP0001 ACPI ID is present and in that
- * case we fill in Device Tree compatible properties for this
- * device.
- */
- list_for_each_entry(hwid, &adev->pnp.ids, list) {
- if (!strcmp(hwid->id, "PRP0001")) {
- acpi_of = true;
- break;
- }
- }
-
- if (!acpi_of)
- return;
-
ret = acpi_dev_get_property_array(adev, "compatible", ACPI_TYPE_STRING,
&of_compatible);
if (ret) {
ret = acpi_dev_get_property(adev, "compatible",
ACPI_TYPE_STRING, &of_compatible);
- if (ret) {
- acpi_handle_warn(adev->handle,
- "PRP0001 requires compatible property\n");
+ if (ret)
return;
- }
}
adev->data.of_compatible = of_compatible;
}
@@ -115,14 +95,27 @@ static void acpi_init_of_compatible(stru
void acpi_init_properties(struct acpi_device *adev)
{
struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
+ bool acpi_of = false;
+ struct acpi_hardware_id *hwid;
const union acpi_object *desc;
acpi_status status;
int i;

+ /*
+ * Check if the special PRP0001 ACPI ID is present and in that case we
+ * fill in Device Tree compatible properties for this device.
+ */
+ list_for_each_entry(hwid, &adev->pnp.ids, list) {
+ if (!strcmp(hwid->id, "PRP0001")) {
+ acpi_of = true;
+ break;
+ }
+ }
+
status = acpi_evaluate_object_typed(adev->handle, "_DSD", NULL, &buf,
ACPI_TYPE_PACKAGE);
if (ACPI_FAILURE(status))
- return;
+ goto out;

desc = buf.pointer;
if (desc->package.count % 2)
@@ -156,13 +149,20 @@ void acpi_init_properties(struct acpi_de
adev->data.pointer = buf.pointer;
adev->data.properties = properties;

- acpi_init_of_compatible(adev);
- return;
+ if (acpi_of)
+ acpi_init_of_compatible(adev);
+
+ goto out;
}

fail:
- dev_warn(&adev->dev, "Returned _DSD data is not valid, skipping\n");
+ dev_dbg(&adev->dev, "Returned _DSD data is not valid, skipping\n");
ACPI_FREE(buf.pointer);
+
+ out:
+ if (acpi_of && !adev->data.of_compatible)
+ acpi_handle_info(adev->handle,
+ "PRP0001 requires 'compatible' property\n");
}

void acpi_free_properties(struct acpi_device *adev)

[Rafael J. Wysocki]

Any comments?

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[Mika Westerberg]

On Wed, Apr 22, 2015 at 03:54:16AM +0200, Rafael J. Wysocki wrote:
> Any comments?
>
> > ---
> > From: Rafael J. Wysocki <rafael.j...@intel.com>
> > Subject: ACPI / property: Refine consistency check for PRP0001
> >
> > Refine the check for the presence of the "compatible" property
> > if the PRP0001 device ID is present in the device's list of
> > ACPI/PNP IDs to also print the message if _DSD is missing
> > entirely or the format of it is incorrect.
> >
> > While at it, reduce the log level of the message to "info"
> > and reduce the log level of the "broken _DSD" message to
> > "debug" (noise reduction).
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j...@intel.com>

Looks good,

Reviewed-by: Mika Westerberg <mika.we...@linux.intel.com>

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j...@intel.com>

If the special PRP0001 device ID is present in the given device's list
of ACPI/PNP IDs and the device has a valid "compatible" property in
the _DSD, it should be enumerated using the default mechanism,
unless some scan handlers match the IDs preceding PRP0001 in the

device's list of ACPI/PNP IDs. In addition to that, no scan handlers

matching the IDs following PRP0001 in that list should be attached
to the device.

To make that happen, define a scan handler that will match PRP0001
and trigger the default enumeration for the matching devices if the
"compatible" property is present for them.

Since that requires the check for platform_id and device->handler
to be removed from acpi_default_enumeration(), move the fallback
invocation of acpi_default_enumeration() to acpi_bus_attach()
(after it's checked if there's a matching ACPI driver for the
device), which is a better place to call it, and do the platform_id
check in there too (device->handler is guaranteed to be unset at
the point where the function is looking for a matching ACPI driver).

Signed-off-by: Rafael J. Wysocki <rafael.j...@intel.com>

Acked-by: Darren Hart <dvh...@linux.intel.com>
---

The change from the original patch is to change the scan handler
behavior to make it return 1 also if the "compatible" property is
not present, in which case the additional scan handlers should not
trigger too *and* the default enumeration should not trigger either
(as there's no ID to match to), which will allow things like
auxiliary nodes (think GPIO buttons/LEDs etc) to be easily represented.

Darren, I've tentatively added your Acked-by tag to this one, please
let me know if that's not appropriate.

Rafael


---
drivers/acpi/scan.c | 33 ++++++++++++++++++++++++++++-----
1 file changed, 28 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c

@@ -2388,9 +2388,6 @@ static void acpi_default_enumeration(str

struct list_head resource_list;
bool is_spi_i2c_slave = false;

- if (!device->pnp.type.platform_id || device->handler)
- return;
-
/*
* Do not enemerate SPI/I2C slaves as they will be enuerated by their
* respective parents.

@@ -2403,6 +2400,29 @@ static void acpi_default_enumeration(str

acpi_create_platform_device(device);
}

+static const struct acpi_device_id generic_device_ids[] = {
+ {"PRP0001", },
+ {"", },
+};
+
+static int acpi_generic_device_attach(struct acpi_device *adev,
+ const struct acpi_device_id *not_used)
+{
+ /*
+ * Since PRP0001 is the only ID handled here, the test below can be
+ * unconditional.
+ */
+ if (adev->data.of_compatible)
+ acpi_default_enumeration(adev);
+
+ return 1;
+}
+

+static struct acpi_scan_handler generic_device_handler = {
+ .ids = generic_device_ids,
+ .attach = acpi_generic_device_attach,
+};
+
static int acpi_scan_attach_handler(struct acpi_device *device)
{
struct acpi_hardware_id *hwid;

@@ -2428,8 +2448,6 @@ static int acpi_scan_attach_handler(stru

break;
}
}
- if (!ret)
- acpi_default_enumeration(device);

return ret;
}

@@ -2471,6 +2489,9 @@ static void acpi_bus_attach(struct acpi_

ret = device_attach(&device->dev);
if (ret < 0)
return;
+
+ if (!ret && device->pnp.type.platform_id)
+ acpi_default_enumeration(device);
}
device->flags.visited = true;

@@ -2629,6 +2650,8 @@ int __init acpi_scan_init(void)

[Darren Hart]

This should probably be spelled out in the commit message itself as it's a fairly
unique condition.

> Darren, I've tentatively added your Acked-by tag to this one, please
> let me know if that's not appropriate.

Spent a bit more time on it this time, so:

Reviewed-by: Darren Hart <dvh...@linux.intel.com>

Thanks,

--
Darren Hart
Intel Open Source Technology Center

[Rafael J. Wysocki]

I'm going to document that in Documentation/acpi/enumeration.txt anyway.

> > Darren, I've tentatively added your Acked-by tag to this one, please
> > let me know if that's not appropriate.
>
> Spent a bit more time on it this time, so:
>
> Reviewed-by: Darren Hart <dvh...@linux.intel.com>

Thanks!

[Rafael J. Wysocki]

On Wednesday, April 22, 2015 12:57:18 PM Mika Westerberg wrote:
> On Wed, Apr 22, 2015 at 03:54:16AM +0200, Rafael J. Wysocki wrote:
> > Any comments?
> >
> > > ---
> > > From: Rafael J. Wysocki <rafael.j...@intel.com>
> > > Subject: ACPI / property: Refine consistency check for PRP0001
> > >
> > > Refine the check for the presence of the "compatible" property
> > > if the PRP0001 device ID is present in the device's list of
> > > ACPI/PNP IDs to also print the message if _DSD is missing
> > > entirely or the format of it is incorrect.
> > >
> > > While at it, reduce the log level of the message to "info"
> > > and reduce the log level of the "broken _DSD" message to
> > > "debug" (noise reduction).
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j...@intel.com>
>
> Looks good,
>
> Reviewed-by: Mika Westerberg <mika.we...@linux.intel.com>

Thanks, I have an update, though.

In a recent discussion with Darren we've come to the conlusion that
having a parent with PRP0001 and "compatible" and a child with PRP0001 only
(without "compatible") is useful in cases when one complex device is
represented by a hierarchy of "device" objects (in analogy with device
nodes in a DT that have no struct device representations). Thus it isn't
useful to complain that "compatible" is not present in such cases.

Updated patch:

---
From: Rafael J. Wysocki <rafael.j...@intel.com>
Subject: ACPI / property: Refine consistency check for PRP0001

Refine the check for the presence of the "compatible" property
if the PRP0001 device ID is present in the device's list of
ACPI/PNP IDs to also print the message if _DSD is missing
entirely or the format of it is incorrect.

One special case to take into accout is that the "compatible"
property need not be provided for devices having the PRP0001
device ID in their lists of ACPI/PNP IDs if they are ancestors
of PRP0001 devices with the "compatible" property present.
This is to cover heriarchies of device objects where the kernel
is only supposed to use a struct device representation for the
topmost one and the others represent, for example, functional
blocks of a composite device.

While at it, reduce the log level of the message to "info"
and reduce the log level of the "broken _DSD" message to
"debug" (noise reduction).

Signed-off-by: Rafael J. Wysocki <rafael.j...@intel.com>

---
drivers/acpi/property.c | 54 +++++++++++++++++++++++++++---------------------
include/acpi/acpi_bus.h | 3 +-
2 files changed, 33 insertions(+), 24 deletions(-)

Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c

@@ -79,50 +79,51 @@ static bool acpi_properties_format_valid

if (ret) {
- acpi_handle_warn(adev->handle,
- "PRP0001 requires compatible property\n");

+ if (adev->parent
+ && adev->parent->flags.of_compatible_ok)
+ goto out;
+
return;
}
}
adev->data.of_compatible = of_compatible;
+
+ out:
+ adev->flags.of_compatible_ok = 1;

@@ -156,13 +157,20 @@ void acpi_init_properties(struct acpi_de

adev->data.pointer = buf.pointer;
adev->data.properties = properties;

- acpi_init_of_compatible(adev);
- return;
+ if (acpi_of)
+ acpi_init_of_compatible(adev);
+
+ goto out;
}

fail:
- dev_warn(&adev->dev, "Returned _DSD data is not valid, skipping\n");
+ dev_dbg(&adev->dev, "Returned _DSD data is not valid, skipping\n");
ACPI_FREE(buf.pointer);
+
+ out:

+ if (acpi_of && !adev->flags.of_compatible_ok)

+ acpi_handle_info(adev->handle,
+ "PRP0001 requires 'compatible' property\n");
}

void acpi_free_properties(struct acpi_device *adev)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -208,7 +208,8 @@ struct acpi_device_flags {
u32 visited:1;
u32 hotplug_notify:1;
u32 is_dock_station:1;
- u32 reserved:23;
+ u32 of_compatible_ok:1;
+ u32 reserved:22;
};

/* File System */

[Mika Westerberg]

On Tue, May 05, 2015 at 02:49:55AM +0200, Rafael J. Wysocki wrote:
>
> Thanks, I have an update, though.
>
> In a recent discussion with Darren we've come to the conlusion that
> having a parent with PRP0001 and "compatible" and a child with PRP0001 only
> (without "compatible") is useful in cases when one complex device is
> represented by a hierarchy of "device" objects (in analogy with device
> nodes in a DT that have no struct device representations). Thus it isn't
> useful to complain that "compatible" is not present in such cases.

OK, I see.

> Updated patch:
>
> ---
> From: Rafael J. Wysocki <rafael.j...@intel.com>
> Subject: ACPI / property: Refine consistency check for PRP0001
>
> Refine the check for the presence of the "compatible" property
> if the PRP0001 device ID is present in the device's list of
> ACPI/PNP IDs to also print the message if _DSD is missing
> entirely or the format of it is incorrect.
>
> One special case to take into accout is that the "compatible"
> property need not be provided for devices having the PRP0001
> device ID in their lists of ACPI/PNP IDs if they are ancestors
> of PRP0001 devices with the "compatible" property present.
> This is to cover heriarchies of device objects where the kernel
> is only supposed to use a struct device representation for the
> topmost one and the others represent, for example, functional
> blocks of a composite device.
>
> While at it, reduce the log level of the message to "info"
> and reduce the log level of the "broken _DSD" message to
> "debug" (noise reduction).
>
> Signed-off-by: Rafael J. Wysocki <rafael.j...@intel.com>

Still looks fine to me,

Reviewed-by: Mika Westerberg <mika.we...@linux.intel.com>

[Rafael J. Wysocki]

Thanks!