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

[Rafael J. Wysocki]

On Tuesday, April 14, 2015 01:50:11 PM Rafael J. Wysocki wrote:
> On Monday, April 13, 2015 07:04:14 PM Darren Hart wrote:
> > On Sat, Apr 11, 2015 at 01:28:45AM +0200, Rafael Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > 
> > > 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.wysocki@xxxxxxxxx>
> > > ---
> > >  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;
> > > +	}
> > 
> > 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?
> 
> 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).

Maybe something like the patch below.

---
From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
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.wysocki@xxxxxxxxx>
---
 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)

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