[PATCH v2 0/5] ACPI: Do not call _STA on battery devices with unmet dependencies

[PATCH v2 0/5] ACPI: Do not call _STA on battery devices with unmet dependencies

[Hans de Goede]

Hi All,

Here is v2 of this set, new in v2 is the addressing of Bjorn's comments
to the "PCI: acpiphp_ibm: prepare for acpi_get_object_info no longer
returning status" patch and a new patch (the first patch in v2) which
exports acpi_bus_get_status_handle() for use in modules as acpiphp_ibm
will now depend on it.

Here is the old coverletter from v1 (adjusted for the new patch):

The ACPI code already contains quite a bit of code to not bind the
ACPI-battery until all deps for an ACPI battery device have been met,
but on some devices calling _STA before all deps are met is a problem
too because the _STA method uses an i2c OpRegion there.

Here is the DSDT of the device I'm seeing this on:
https://fedorapeople.org/~jwrdegoede/toshiba-click-mini-dsdt.dsl

This series modifies the kernel to not call _STA until all deps are met,
mirroring the binding behavior of the battery driver.

Without this series a total of 32 ACPI errors get printend to the console
on boot, there are 4 errors per _STA call, 2 battery devices on this
system and 4 _STA calls per battery device.

The first 2 commits are preparation commits for making the ACPICA changes
in the 4th commit, these commits are necessary to not break things after
the ACPICA changes.

The 3th commit modifies acpi_bus_get_status to not call _STA on
battery devices until all deps are met. This fixes 2 of the 4 too early
_STA calls triggering these errors.

The 4th commit makes the device instantiation code use
acpi_bus_get_status instead of acpi_bus_get_status_handle so that the
code to get the initial status also does not makes 1 too early _STA call.

The 5th commit changes the ACPICA acpi_get_object_info function to not
call _STA. Only 1 user (which is fixed in the first commit) cares about
acpi_device_info.current_status. And the ACPICA code has this comment:

 * Note: This interface is intended to be used during the initial device
 * discovery namespace traversal. Therefore, no complex methods can be
 * executed, especially those that access operation regions. Therefore, do
 * not add any additional methods that could cause problems in this area.
 * Because of this reason support for the following methods has been removed:
 * this was the fate of the _SUB method which was found to cause such
 * problems and was removed (11/2015).

The described problems with the _SUB method clearly also apply to the _STA
method, so removing it from acpi_get_object_info seems like it is the right
thing to do here. This too fixes 1 too early _STA call, so that with all
5 patches in place we've fixed all 4 too early _STA calls.

Regards,

Hans

[PATCH v2 1/5] ACPI: export acpi_bus_get_status_handle()

[Hans de Goede]

Some modular drivers need this, export it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set
---
 drivers/acpi/bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 4d0979e02a28..db0c05997cc2 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -108,6 +108,7 @@ acpi_status acpi_bus_get_status_handle(acpi_handle handle,
 	}
 	return status;
 }
+EXPORT_SYMBOL(acpi_bus_get_status_handle);
 
 int acpi_bus_get_status(struct acpi_device *device)
 {
-- 
2.14.3

Re: [PATCH v2 1/5] ACPI: export acpi_bus_get_status_handle()

[Rafael J. Wysocki]

On Mon, Jan 22, 2018 at 9:51 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Some modular drivers need this, export it.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---
>  drivers/acpi/bus.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 4d0979e02a28..db0c05997cc2 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -108,6 +108,7 @@ acpi_status acpi_bus_get_status_handle(acpi_handle handle,
>         }
>         return status;
>  }
> +EXPORT_SYMBOL(acpi_bus_get_status_handle);

EXPORT_SYMBOL_GPL(), please.

>
>  int acpi_bus_get_status(struct acpi_device *device)
>  {
> --

Re: [PATCH v2 1/5] ACPI: export acpi_bus_get_status_handle()

[Hans de Goede]

Hi,

On 01/23/2018 12:55 AM, Rafael J. Wysocki wrote:
> On Mon, Jan 22, 2018 at 9:51 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Some modular drivers need this, export it.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -This is a new patch in v2 of this patch-set
>> ---
>>   drivers/acpi/bus.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 4d0979e02a28..db0c05997cc2 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -108,6 +108,7 @@ acpi_status acpi_bus_get_status_handle(acpi_handle handle,
>>          }
>>          return status;
>>   }
>> +EXPORT_SYMBOL(acpi_bus_get_status_handle);
> 
> EXPORT_SYMBOL_GPL(), please.

Ok, v3 with this fixed coming up.

Regards,

Hans

[PATCH v2 2/5] PCI: acpiphp_ibm: prepare for acpi_get_object_info() no longer returning status

[Hans de Goede]

acpi_get_object_info() is intended for early probe usage and as such should
not call any methods which may rely on OpRegions, but it used to also call
_STA to get the status, which on some systems does rely on OpRegions, this
behavior and the acpi_device_info.current_status member are being removed.

This commit prepares the acpiphp_ibm code for this by having it get the
status itself using acpi_bus_get_status_handle(). Note no error handling is
necessary on any errors acpi_bus_get_status_handle() leaves the value of
the passed in current_status at its 0 initialization value.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Use () after function names in commit message
-Add  Bjorn's Acked-by
---
 drivers/pci/hotplug/acpiphp_ibm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/acpiphp_ibm.c b/drivers/pci/hotplug/acpiphp_ibm.c
index 984c7e8cec5a..8472c4a27f70 100644
--- a/drivers/pci/hotplug/acpiphp_ibm.c
+++ b/drivers/pci/hotplug/acpiphp_ibm.c
@@ -399,6 +399,7 @@ static acpi_status __init ibm_find_acpi_device(acpi_handle handle,
 		u32 lvl, void *context, void **rv)
 {
 	acpi_handle *phandle = (acpi_handle *)context;
+	unsigned long long current_status = 0;
 	acpi_status status;
 	struct acpi_device_info *info;
 	int retval = 0;
@@ -410,7 +411,9 @@ static acpi_status __init ibm_find_acpi_device(acpi_handle handle,
 		return retval;
 	}
 
-	if (info->current_status && (info->valid & ACPI_VALID_HID) &&
+	acpi_bus_get_status_handle(handle, &current_status);
+
+	if (current_status && (info->valid & ACPI_VALID_HID) &&
 			(!strcmp(info->hardware_id.string, IBM_HARDWARE_ID1) ||
 			 !strcmp(info->hardware_id.string, IBM_HARDWARE_ID2))) {
 		pr_debug("found hardware: %s, handle: %p\n",
-- 
2.14.3

[PATCH v2 3/5] ACPI / bus: Do not call _STA on battery devices with unmet dependencies

[Hans de Goede]

The battery code uses acpi_device->dep_unmet to check for unmet deps and
if there are unmet deps it does not bind to the device to avoid errors
about missing OpRegions when calling ACPI methods on the device.

The missing OpRegions when there are unmet deps problem also applies to
the _STA method of some battery devices and calling it too early results
in errors like these:

[    0.123579] ACPI Error: No handler for Region [ECRM] (00000000ba9edc4c)
               [GenericSerialBus] (20170831/evregion-166)
[    0.123601] ACPI Error: Region GenericSerialBus (ID=9) has no handler
               (20170831/exfldio-299)
[    0.123618] ACPI Error: Method parse/execution failed
               \_SB.I2C1.BAT1._STA, AE_NOT_EXIST (20170831/psparse-550)

This commit fixes these errors happening when acpi_get_bus_status gets
called by checking dep_unmet for battery devices and reporting a status
of 0 until all dependencies are met.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index db0c05997cc2..d0d2c2543395 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -120,6 +120,12 @@ int acpi_bus_get_status(struct acpi_device *device)
 		return 0;
 	}
 
+	/* Battery devices must have their deps met before calling _STA */
+	if (acpi_device_is_battery(device) && device->dep_unmet) {
+		acpi_set_device_status(device, 0);
+		return 0;
+	}
+
 	status = acpi_bus_get_status_handle(device->handle, &sta);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
-- 
2.14.3

[PATCH v2 4/5] ACPI / scan: Use acpi_bus_get_status for initial status of ACPI_TYPE_DEVICE devs

[Hans de Goede]

The acpi_get_bus_status wrapper for acpi_bus_get_status_handle has some
code to handle certain device quirks, in some cases we also need this
quirk handling for the initial _STA call.

Specifically on some devices calling _STA before all _DEP dependencies
are met results in errors like these:

[    0.123579] ACPI Error: No handler for Region [ECRM] (00000000ba9edc4c)
               [GenericSerialBus] (20170831/evregion-166)
[    0.123601] ACPI Error: Region GenericSerialBus (ID=9) has no handler
               (20170831/exfldio-299)
[    0.123618] ACPI Error: Method parse/execution failed
               \_SB.I2C1.BAT1._STA, AE_NOT_EXIST (20170831/psparse-550)

acpi_get_bus_status already has code to avoid this, so by using it we
also silence these errors from the initial _STA call.

Note that in order for the acpi_get_bus_status handling for this to work,
we initialize dep_unmet to 1 until acpi_device_dep_initialize gets called,
this means that battery devices will be instantiated with an initial
status of 0. This is not a problem, acpi_bus_attach will get called soon
after the instantiation anyways and it will update the status as first
point of order.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note if we were to:
1) move the acpi_bus_init_power call from acpi_bus_get_power_flags to
   acpi_bus_attach, which already calls acpi_bus_init_power for uninitialized
   devices; and
2) move the acpi_bus_get_wakeup_device_flags call from acpi_add_single_object
   to acpi_bus_attach. This one is tricky, because acpi_device_add checks
   wakeup.flags.valid which gets set by this. And the acpi_wakeup_gpe_init
   call actually is the troublesome one because it uses acpi_match_device_ids
   which relies on the status...

Then we can entirely rely on the acpi_bus_get_status call in acpi_bus_attach
and remove the acpi_bus_get_status call from the device-instantiation path.
---
 drivers/acpi/scan.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index e0bd49b4c3f6..ffb64424771b 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1572,6 +1572,8 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	device_initialize(&device->dev);
 	dev_set_uevent_suppress(&device->dev, true);
 	acpi_init_coherency(device);
+	/* Assume there are unmet deps until acpi_device_dep_initialize runs */
+	device->dep_unmet = 1;
 }
 
 void acpi_device_add_finalize(struct acpi_device *device)
@@ -1595,6 +1597,14 @@ static int acpi_add_single_object(struct acpi_device **child,
 	}
 
 	acpi_init_device_object(device, handle, type, sta);
+	/*
+	 * For ACPI_BUS_TYPE_DEVICE getting the status is delayed till here so
+	 * that we can call acpi_bus_get_status and use its quirk handling.
+	 * Note this must be done before the get power-/wakeup_dev-flags calls.
+	 */
+	if (type == ACPI_BUS_TYPE_DEVICE)
+		acpi_bus_get_status(device);
+
 	acpi_bus_get_power_flags(device);
 	acpi_bus_get_wakeup_device_flags(device);
 
@@ -1667,9 +1677,11 @@ static int acpi_bus_type_and_status(acpi_handle handle, int *type,
 			return -ENODEV;
 
 		*type = ACPI_BUS_TYPE_DEVICE;
-		status = acpi_bus_get_status_handle(handle, sta);
-		if (ACPI_FAILURE(status))
-			*sta = 0;
+		/*
+		 * acpi_add_single_object updates this once we've an acpi_device
+		 * so that acpi_bus_get_status' quirk handling can be used.
+		 */
+		*sta = 0;
 		break;
 	case ACPI_TYPE_PROCESSOR:
 		*type = ACPI_BUS_TYPE_PROCESSOR;
@@ -1767,6 +1779,8 @@ static void acpi_device_dep_initialize(struct acpi_device *adev)
 	acpi_status status;
 	int i;
 
+	adev->dep_unmet = 0;
+
 	if (!acpi_has_method(adev->handle, "_DEP"))
 		return;
 
-- 
2.14.3

[PATCH v2 5/5] ACPICA: Remove calling of _STA from acpi_get_object_info

[Hans de Goede]

As the comment above it indicates, acpi_get_object_info is intended for
early probe usage and as such should not call any methods which may rely
on OpRegions, before this commit it was also calling _STA, which on some
systems does rely on OpRegions.

Calling _STA before things are ready leads to errors such as these:

[    0.123579] ACPI Error: No handler for Region [ECRM] (00000000ba9edc4c)
               [GenericSerialBus] (20170831/evregion-166)
[    0.123601] ACPI Error: Region GenericSerialBus (ID=9) has no handler
               (20170831/exfldio-299)
[    0.123618] ACPI Error: Method parse/execution failed
               \_SB.I2C1.BAT1._STA, AE_NOT_EXIST (20170831/psparse-550)

End 2015 support for the _SUB method was removed for exactly the same
reason. Removing current_status from struct acpi_device_info only has
a limit impact. Within ACPICA it is only used by 2 debug messages, both
of which are modified to no longer print it with this commit.

Outside of ACPICA, for Linux there is only one user. For which a patch to
remove the dependency on the current_status field is available.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpica/dbdisply.c |  5 ++---
 drivers/acpi/acpica/nsdumpdv.c |  5 ++---
 drivers/acpi/acpica/nsxfname.c | 19 ++++---------------
 include/acpi/actypes.h         |  2 --
 4 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/acpi/acpica/dbdisply.c b/drivers/acpi/acpica/dbdisply.c
index 5a606eac0c22..7b5eb33fe962 100644
--- a/drivers/acpi/acpica/dbdisply.c
+++ b/drivers/acpi/acpica/dbdisply.c
@@ -642,9 +642,8 @@ void acpi_db_display_object_type(char *object_arg)
 		return;
 	}
 
-	acpi_os_printf("ADR: %8.8X%8.8X, STA: %8.8X, Flags: %X\n",
-		       ACPI_FORMAT_UINT64(info->address),
-		       info->current_status, info->flags);
+	acpi_os_printf("ADR: %8.8X%8.8X, Flags: %X\n",
+		       ACPI_FORMAT_UINT64(info->address), info->flags);
 
 	acpi_os_printf("S1D-%2.2X S2D-%2.2X S3D-%2.2X S4D-%2.2X\n",
 		       info->highest_dstates[0], info->highest_dstates[1],
diff --git a/drivers/acpi/acpica/nsdumpdv.c b/drivers/acpi/acpica/nsdumpdv.c
index 5026594763ea..573a5f36f01a 100644
--- a/drivers/acpi/acpica/nsdumpdv.c
+++ b/drivers/acpi/acpica/nsdumpdv.c
@@ -88,10 +88,9 @@ acpi_ns_dump_one_device(acpi_handle obj_handle,
 		}
 
 		ACPI_DEBUG_PRINT_RAW((ACPI_DB_TABLES,
-				      "    HID: %s, ADR: %8.8X%8.8X, Status: %X\n",
+				      "    HID: %s, ADR: %8.8X%8.8X\n",
 				      info->hardware_id.value,
-				      ACPI_FORMAT_UINT64(info->address),
-				      info->current_status));
+				      ACPI_FORMAT_UINT64(info->address));
 		ACPI_FREE(info);
 	}
 
diff --git a/drivers/acpi/acpica/nsxfname.c b/drivers/acpi/acpica/nsxfname.c
index 106966235805..0a9c600c2599 100644
--- a/drivers/acpi/acpica/nsxfname.c
+++ b/drivers/acpi/acpica/nsxfname.c
@@ -241,7 +241,7 @@ static char *acpi_ns_copy_device_id(struct acpi_pnp_device_id *dest,
  *              namespace node and possibly by running several standard
  *              control methods (Such as in the case of a device.)
  *
- * For Device and Processor objects, run the Device _HID, _UID, _CID, _STA,
+ * For Device and Processor objects, run the Device _HID, _UID, _CID,
  * _CLS, _ADR, _sx_w, and _sx_d methods.
  *
  * Note: Allocates the return buffer, must be freed by the caller.
@@ -250,8 +250,9 @@ static char *acpi_ns_copy_device_id(struct acpi_pnp_device_id *dest,
  * discovery namespace traversal. Therefore, no complex methods can be
  * executed, especially those that access operation regions. Therefore, do
  * not add any additional methods that could cause problems in this area.
- * this was the fate of the _SUB method which was found to cause such
- * problems and was removed (11/2015).
+ * Because of this reason support for the following methods has been removed:
+ * 1) _SUB method was removed (11/2015)
+ * 2) _STA method was removed (01/2018)
  *
  ******************************************************************************/
 
@@ -374,20 +375,8 @@ acpi_get_object_info(acpi_handle handle,
 		 * Notes: none of these methods are required, so they may or may
 		 * not be present for this device. The Info->Valid bitfield is used
 		 * to indicate which methods were found and run successfully.
-		 *
-		 * For _STA, if the method does not exist, then (as per the ACPI
-		 * specification), the returned current_status flags will indicate
-		 * that the device is present/functional/enabled. Otherwise, the
-		 * current_status flags reflect the value returned from _STA.
 		 */
 
-		/* Execute the Device._STA method */
-
-		status = acpi_ut_execute_STA(node, &info->current_status);
-		if (ACPI_SUCCESS(status)) {
-			valid |= ACPI_VALID_STA;
-		}
-
 		/* Execute the Device._ADR method */
 
 		status = acpi_ut_evaluate_numeric_object(METHOD_NAME__ADR, node,
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index 4f077edb9b81..220ef8674763 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -1191,7 +1191,6 @@ struct acpi_device_info {
 	u8 flags;		/* Miscellaneous info */
 	u8 highest_dstates[4];	/* _sx_d values: 0xFF indicates not valid */
 	u8 lowest_dstates[5];	/* _sx_w values: 0xFF indicates not valid */
-	u32 current_status;	/* _STA value */
 	u64 address;	/* _ADR value */
 	struct acpi_pnp_device_id hardware_id;	/* _HID value */
 	struct acpi_pnp_device_id unique_id;	/* _UID value */
@@ -1205,7 +1204,6 @@ struct acpi_device_info {
 
 /* Flags for Valid field above (acpi_get_object_info) */
 
-#define ACPI_VALID_STA                  0x0001
 #define ACPI_VALID_ADR                  0x0002
 #define ACPI_VALID_HID                  0x0004
 #define ACPI_VALID_UID                  0x0008
-- 
2.14.3

Re: [PATCH v2 0/5] ACPI: Do not call _STA on battery devices with unmet dependencies

[Rafael J. Wysocki]

On Monday, January 22, 2018 9:51:30 AM CET Hans de Goede wrote:
> Hi All,
> 
> Here is v2 of this set, new in v2 is the addressing of Bjorn's comments
> to the "PCI: acpiphp_ibm: prepare for acpi_get_object_info no longer
> returning status" patch and a new patch (the first patch in v2) which
> exports acpi_bus_get_status_handle() for use in modules as acpiphp_ibm
> will now depend on it.
> 
> Here is the old coverletter from v1 (adjusted for the new patch):
> 
> The ACPI code already contains quite a bit of code to not bind the
> ACPI-battery until all deps for an ACPI battery device have been met,
> but on some devices calling _STA before all deps are met is a problem
> too because the _STA method uses an i2c OpRegion there.
> 
> Here is the DSDT of the device I'm seeing this on:
> https://fedorapeople.org/~jwrdegoede/toshiba-click-mini-dsdt.dsl
> 
> This series modifies the kernel to not call _STA until all deps are met,
> mirroring the binding behavior of the battery driver.
> 
> Without this series a total of 32 ACPI errors get printend to the console
> on boot, there are 4 errors per _STA call, 2 battery devices on this
> system and 4 _STA calls per battery device.
> 
> The first 2 commits are preparation commits for making the ACPICA changes
> in the 4th commit, these commits are necessary to not break things after
> the ACPICA changes.
> 
> The 3th commit modifies acpi_bus_get_status to not call _STA on
> battery devices until all deps are met. This fixes 2 of the 4 too early
> _STA calls triggering these errors.
> 
> The 4th commit makes the device instantiation code use
> acpi_bus_get_status instead of acpi_bus_get_status_handle so that the
> code to get the initial status also does not makes 1 too early _STA call.
> 
> The 5th commit changes the ACPICA acpi_get_object_info function to not
> call _STA. Only 1 user (which is fixed in the first commit) cares about
> acpi_device_info.current_status. And the ACPICA code has this comment:
> 
>  * Note: This interface is intended to be used during the initial device
>  * discovery namespace traversal. Therefore, no complex methods can be
>  * executed, especially those that access operation regions. Therefore, do
>  * not add any additional methods that could cause problems in this area.
>  * Because of this reason support for the following methods has been removed:
>  * this was the fate of the _SUB method which was found to cause such
>  * problems and was removed (11/2015).
> 
> The described problems with the _SUB method clearly also apply to the _STA
> method, so removing it from acpi_get_object_info seems like it is the right
> thing to do here. This too fixes 1 too early _STA call, so that with all
> 5 patches in place we've fixed all 4 too early _STA calls.

I've applied [1-4/5] from this series.

The last one needs to go in via ACPICA anyway or we'll need to carry
a Linux-specific replacement for acpi_get_object_info().

Re: [PATCH v2 0/5] ACPI: Do not call _STA on battery devices with unmet dependencies

[Hans de Goede]

Hi,

On 08-02-18 10:57, Rafael J. Wysocki wrote:
> On Monday, January 22, 2018 9:51:30 AM CET Hans de Goede wrote:
>> Hi All,
>>
>> Here is v2 of this set, new in v2 is the addressing of Bjorn's comments
>> to the "PCI: acpiphp_ibm: prepare for acpi_get_object_info no longer
>> returning status" patch and a new patch (the first patch in v2) which
>> exports acpi_bus_get_status_handle() for use in modules as acpiphp_ibm
>> will now depend on it.
>>
>> Here is the old coverletter from v1 (adjusted for the new patch):
>>
>> The ACPI code already contains quite a bit of code to not bind the
>> ACPI-battery until all deps for an ACPI battery device have been met,
>> but on some devices calling _STA before all deps are met is a problem
>> too because the _STA method uses an i2c OpRegion there.
>>
>> Here is the DSDT of the device I'm seeing this on:
>> https://fedorapeople.org/~jwrdegoede/toshiba-click-mini-dsdt.dsl
>>
>> This series modifies the kernel to not call _STA until all deps are met,
>> mirroring the binding behavior of the battery driver.
>>
>> Without this series a total of 32 ACPI errors get printend to the console
>> on boot, there are 4 errors per _STA call, 2 battery devices on this
>> system and 4 _STA calls per battery device.
>>
>> The first 2 commits are preparation commits for making the ACPICA changes
>> in the 4th commit, these commits are necessary to not break things after
>> the ACPICA changes.
>>
>> The 3th commit modifies acpi_bus_get_status to not call _STA on
>> battery devices until all deps are met. This fixes 2 of the 4 too early
>> _STA calls triggering these errors.
>>
>> The 4th commit makes the device instantiation code use
>> acpi_bus_get_status instead of acpi_bus_get_status_handle so that the
>> code to get the initial status also does not makes 1 too early _STA call.
>>
>> The 5th commit changes the ACPICA acpi_get_object_info function to not
>> call _STA. Only 1 user (which is fixed in the first commit) cares about
>> acpi_device_info.current_status. And the ACPICA code has this comment:
>>
>>   * Note: This interface is intended to be used during the initial device
>>   * discovery namespace traversal. Therefore, no complex methods can be
>>   * executed, especially those that access operation regions. Therefore, do
>>   * not add any additional methods that could cause problems in this area.
>>   * Because of this reason support for the following methods has been removed:
>>   * this was the fate of the _SUB method which was found to cause such
>>   * problems and was removed (11/2015).
>>
>> The described problems with the _SUB method clearly also apply to the _STA
>> method, so removing it from acpi_get_object_info seems like it is the right
>> thing to do here. This too fixes 1 too early _STA call, so that with all
>> 5 patches in place we've fixed all 4 too early _STA calls.
> 
> I've applied [1-4/5] from this series.

Thank you.

> The last one needs to go in via ACPICA anyway or we'll need to carry
> a Linux-specific replacement for acpi_get_object_info().

Ack, I'm discussing how to proceed with this with the ACPICA people.

Regards,

Hans

RE: [PATCH v2 0/5] ACPI: Do not call _STA on battery devices with unmet dependencies

[Moore, Robert]

DQoNCj4gPiBUaGUgbGFzdCBvbmUgbmVlZHMgdG8gZ28gaW4gdmlhIEFDUElDQSBhbnl3YXkgb3Ig
d2UnbGwgbmVlZCB0byBjYXJyeSBhDQo+ID4gTGludXgtc3BlY2lmaWMgcmVwbGFjZW1lbnQgZm9y
IGFjcGlfZ2V0X29iamVjdF9pbmZvKCkuDQo+IA0KPiBBY2ssIEknbSBkaXNjdXNzaW5nIGhvdyB0
byBwcm9jZWVkIHdpdGggdGhpcyB3aXRoIHRoZSBBQ1BJQ0EgcGVvcGxlLg0KPiANCj4gUmVnYXJk
cywNCj4gDQo+IEhhbnMNCg0KDQpJIG5vcm1hbGx5IGRvbid0IGxpa2UgdG8gY2hhbmdlIGV4dGVy
bmFsIGludGVyZmFjZXMgKHRoZSBiaWdnZXN0IHJlYXNvbiBiZWluZyB0aGUgc2hlZXIgbnVtYmVy
IG9mIGRpZmZlcmVudCBPUyB2ZW5kb3JzIHRoYXQgdXNlIEFDUElDQSksIGJ1dCBpbiB0aGlzIGNh
c2UsIEkgZmVlbCBpdCBtYXkgYmUgT0suIFRoaXMgaW50ZXJmYWNlIGlzIHByb2JhYmx5IG5vdCBo
ZWF2aWx5IHVzZWQsIGFuZCB0aGVyZSBpcyBwcmVjZWRlbnQgZm9yIHRoaXMgd2hlbiB0aGUgaW52
b2NhdGlvbiBvZiBfU1VCIHdhcyByZW1vdmVkLg0KDQpPbiB0aGUgb3RoZXIgaGFuZCwgSSB0aGlu
ayBJIHNhdyBpdCBtZW50aW9uZWQgdGhhdCBMaW51eCBjYWxscyB0aGlzIGludGVyZmFjZSBvbmx5
IG9uY2UsIGlzIHRoaXMgY29ycmVjdD8gSWYgc28sIHBlcmhhcHMgaXQgd291bGQgYmUgZWFzaWVz
dCB0byBzaW1wbHkgcmVtb3ZlIHRoaXMgY2FsbCBhbmQgdXNlIGRpcmVjdCBjb250cm9sIG1ldGhv
ZCBpbnZvY2F0aW9ucyB0byBvYnRhaW4gdGhlIGRlc2lyZWQgZGF0YS4NCg0KSWYgd2UgZ28gYWhl
YWQgYW5kIG1ha2UgdGhpcyBjaGFuZ2UgdG8gQUNQSUNBOiBJIGhhdmUgbm90IHNlZW4gdGhlIHBh
dGNoLCBidXQgcGxlYXNlIG1ha2Ugc3VyZSB0aGF0IGFsbCB2ZXN0aWdlcyBvZiB0aGlzIGFyZSBy
ZW1vdmVkLiBUaGlzIGluY2x1ZGVzIGFsbCBpbnZvY2F0aW9ucyB3aXRoaW4gQUNQSUNBIHRoYXQg
ZXhhbWluZSB0aGUgcmVzdWx0IG9mIHRoZSBfU1RBIGludm9jYXRpb24sIHRoZSBDdXJyZW50U3Rh
dHVzIGZpZWxkIG9mIEFDUElfREVWSUNFX0lORk8sIHRoZSBBQ1BJX1ZBTElEX1NUQSBmbGFnLCBl
dGMuDQoNCkJvYg0KDQo=

Re: [PATCH v2 0/5] ACPI: Do not call _STA on battery devices with unmet dependencies

[Hans de Goede]

Hi,

On 08-02-18 18:34, Moore, Robert wrote:
> 
> 
>>> The last one needs to go in via ACPICA anyway or we'll need to carry a
>>> Linux-specific replacement for acpi_get_object_info().
>>
>> Ack, I'm discussing how to proceed with this with the ACPICA people.
>>
>> Regards,
>>
>> Hans
> 
> 
> I normally don't like to change external interfaces (the biggest reason being the sheer number of different OS vendors that use ACPICA), but in this case, I feel it may be OK. This interface is probably not heavily used, and there is precedent for this when the invocation of _SUB was removed.
> 
> On the other hand, I think I saw it mentioned that Linux calls this interface only once, is this correct?

More or less, Linux calls it once when enumerating all the devices and a few ACPI drivers call it again to get some
extra info they need.

> If so, perhaps it would be easiest to simply remove this call and use direct control method invocations to obtain the desired data.

If we go this route we would probably end up copying the function, since as said several drivers
use it too.

> If we go ahead and make this change to ACPICA: I have not seen the patch, but please make sure that all vestiges of this are removed. This includes all invocations within ACPICA that examine the result of the _STA invocation, the CurrentStatus field of ACPI_DEVICE_INFO, the ACPI_VALID_STA flag, etc.

The patch already does that:

https://patchwork.kernel.org/patch/10186195/

Regards,

Hans

RE: [PATCH v2 0/5] ACPI: Do not call _STA on battery devices with unmet dependencies

[Moore, Robert]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogSGFucyBkZSBHb2VkZSBb
bWFpbHRvOmhkZWdvZWRlQHJlZGhhdC5jb21dDQo+IFNlbnQ6IFdlZG5lc2RheSwgRmVicnVhcnkg
MTQsIDIwMTggMjozMyBBTQ0KPiBUbzogTW9vcmUsIFJvYmVydCA8cm9iZXJ0Lm1vb3JlQGludGVs
LmNvbT47IFJhZmFlbCBKLiBXeXNvY2tpDQo+IDxyandAcmp3eXNvY2tpLm5ldD47IGxpbnV4LWFj
cGlAdmdlci5rZXJuZWwub3JnOyBTY2htYXVzcywgRXJpaw0KPiA8ZXJpay5zY2htYXVzc0BpbnRl
bC5jb20+OyBCb3gsIERhdmlkIEUgPGRhdmlkLmUuYm94QGludGVsLmNvbT4NCj4gQ2M6IExlbiBC
cm93biA8bGVuYkBrZXJuZWwub3JnPjsgQmpvcm4gSGVsZ2FhcyA8YmhlbGdhYXNAZ29vZ2xlLmNv
bT47DQo+IGxpbnV4LXBjaUB2Z2VyLmtlcm5lbC5vcmc7IGRldmVsQGFjcGljYS5vcmcNCj4gU3Vi
amVjdDogUmU6IFtQQVRDSCB2MiAwLzVdIEFDUEk6IERvIG5vdCBjYWxsIF9TVEEgb24gYmF0dGVy
eSBkZXZpY2VzDQo+IHdpdGggdW5tZXQgZGVwZW5kZW5jaWVzDQo+IA0KPiBIaSwNCj4gDQo+IE9u
IDA4LTAyLTE4IDE4OjM0LCBNb29yZSwgUm9iZXJ0IHdyb3RlOg0KPiA+DQo+ID4NCj4gPj4+IFRo
ZSBsYXN0IG9uZSBuZWVkcyB0byBnbyBpbiB2aWEgQUNQSUNBIGFueXdheSBvciB3ZSdsbCBuZWVk
IHRvIGNhcnJ5DQo+ID4+PiBhIExpbnV4LXNwZWNpZmljIHJlcGxhY2VtZW50IGZvciBhY3BpX2dl
dF9vYmplY3RfaW5mbygpLg0KPiA+Pg0KPiA+PiBBY2ssIEknbSBkaXNjdXNzaW5nIGhvdyB0byBw
cm9jZWVkIHdpdGggdGhpcyB3aXRoIHRoZSBBQ1BJQ0EgcGVvcGxlLg0KPiA+Pg0KPiA+PiBSZWdh
cmRzLA0KPiA+Pg0KPiA+PiBIYW5zDQo+ID4NCj4gPg0KPiA+IEkgbm9ybWFsbHkgZG9uJ3QgbGlr
ZSB0byBjaGFuZ2UgZXh0ZXJuYWwgaW50ZXJmYWNlcyAodGhlIGJpZ2dlc3QNCj4gcmVhc29uIGJl
aW5nIHRoZSBzaGVlciBudW1iZXIgb2YgZGlmZmVyZW50IE9TIHZlbmRvcnMgdGhhdCB1c2UgQUNQ
SUNBKSwNCj4gYnV0IGluIHRoaXMgY2FzZSwgSSBmZWVsIGl0IG1heSBiZSBPSy4gVGhpcyBpbnRl
cmZhY2UgaXMgcHJvYmFibHkgbm90DQo+IGhlYXZpbHkgdXNlZCwgYW5kIHRoZXJlIGlzIHByZWNl
ZGVudCBmb3IgdGhpcyB3aGVuIHRoZSBpbnZvY2F0aW9uIG9mDQo+IF9TVUIgd2FzIHJlbW92ZWQu
DQo+ID4NCj4gPiBPbiB0aGUgb3RoZXIgaGFuZCwgSSB0aGluayBJIHNhdyBpdCBtZW50aW9uZWQg
dGhhdCBMaW51eCBjYWxscyB0aGlzDQo+IGludGVyZmFjZSBvbmx5IG9uY2UsIGlzIHRoaXMgY29y
cmVjdD8NCj4gDQo+IE1vcmUgb3IgbGVzcywgTGludXggY2FsbHMgaXQgb25jZSB3aGVuIGVudW1l
cmF0aW5nIGFsbCB0aGUgZGV2aWNlcyBhbmQgYQ0KPiBmZXcgQUNQSSBkcml2ZXJzIGNhbGwgaXQg
YWdhaW4gdG8gZ2V0IHNvbWUgZXh0cmEgaW5mbyB0aGV5IG5lZWQuDQo+IA0KPiA+IElmIHNvLCBw
ZXJoYXBzIGl0IHdvdWxkIGJlIGVhc2llc3QgdG8gc2ltcGx5IHJlbW92ZSB0aGlzIGNhbGwgYW5k
IHVzZQ0KPiBkaXJlY3QgY29udHJvbCBtZXRob2QgaW52b2NhdGlvbnMgdG8gb2J0YWluIHRoZSBk
ZXNpcmVkIGRhdGEuDQo+IA0KPiBJZiB3ZSBnbyB0aGlzIHJvdXRlIHdlIHdvdWxkIHByb2JhYmx5
IGVuZCB1cCBjb3B5aW5nIHRoZSBmdW5jdGlvbiwgc2luY2UNCj4gYXMgc2FpZCBzZXZlcmFsIGRy
aXZlcnMgdXNlIGl0IHRvby4NCj4gDQo+ID4gSWYgd2UgZ28gYWhlYWQgYW5kIG1ha2UgdGhpcyBj
aGFuZ2UgdG8gQUNQSUNBOiBJIGhhdmUgbm90IHNlZW4gdGhlDQo+IHBhdGNoLCBidXQgcGxlYXNl
IG1ha2Ugc3VyZSB0aGF0IGFsbCB2ZXN0aWdlcyBvZiB0aGlzIGFyZSByZW1vdmVkLiBUaGlzDQo+
IGluY2x1ZGVzIGFsbCBpbnZvY2F0aW9ucyB3aXRoaW4gQUNQSUNBIHRoYXQgZXhhbWluZSB0aGUg
cmVzdWx0IG9mIHRoZQ0KPiBfU1RBIGludm9jYXRpb24sIHRoZSBDdXJyZW50U3RhdHVzIGZpZWxk
IG9mIEFDUElfREVWSUNFX0lORk8sIHRoZQ0KPiBBQ1BJX1ZBTElEX1NUQSBmbGFnLCBldGMuDQo+
IA0KPiBUaGUgcGF0Y2ggYWxyZWFkeSBkb2VzIHRoYXQ6DQo+IA0KPiBodHRwczovL3BhdGNod29y
ay5rZXJuZWwub3JnL3BhdGNoLzEwMTg2MTk1Lw0KPiANCg0KDQpPZiBjb3Vyc2UsIHdlIHdpbGwg
bmVlZCB0aGUgQUNQSUNBIHZlcnNpb24gb2YgdGhlIHBhdGNoLCBub3QgdGhlICJsaW51eGl6ZWQi
IHZlcnNpb24uDQpCb2INCg0KDQoNCj4gUmVnYXJkcywNCj4gDQo+IEhhbnMNCj4gDQoNCg==

Re: [PATCH v2 0/5] ACPI: Do not call _STA on battery devices with unmet dependencies

[Hans de Goede]

Hi,

On 14-02-18 16:28, Moore, Robert wrote:
> 
> 
>> -----Original Message-----
>> From: Hans de Goede [mailto:hdegoede@redhat.com]
>> Sent: Wednesday, February 14, 2018 2:33 AM
>> To: Moore, Robert <robert.moore@intel.com>; Rafael J. Wysocki
>> <rjw@rjwysocki.net>; linux-acpi@vger.kernel.org; Schmauss, Erik
>> <erik.schmauss@intel.com>; Box, David E <david.e.box@intel.com>
>> Cc: Len Brown <lenb@kernel.org>; Bjorn Helgaas <bhelgaas@google.com>;
>> linux-pci@vger.kernel.org; devel@acpica.org
>> Subject: Re: [PATCH v2 0/5] ACPI: Do not call _STA on battery devices
>> with unmet dependencies
>>
>> Hi,
>>
>> On 08-02-18 18:34, Moore, Robert wrote:
>>>
>>>
>>>>> The last one needs to go in via ACPICA anyway or we'll need to carry
>>>>> a Linux-specific replacement for acpi_get_object_info().
>>>>
>>>> Ack, I'm discussing how to proceed with this with the ACPICA people.
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>
>>>
>>> I normally don't like to change external interfaces (the biggest
>> reason being the sheer number of different OS vendors that use ACPICA),
>> but in this case, I feel it may be OK. This interface is probably not
>> heavily used, and there is precedent for this when the invocation of
>> _SUB was removed.
>>>
>>> On the other hand, I think I saw it mentioned that Linux calls this
>> interface only once, is this correct?
>>
>> More or less, Linux calls it once when enumerating all the devices and a
>> few ACPI drivers call it again to get some extra info they need.
>>
>>> If so, perhaps it would be easiest to simply remove this call and use
>> direct control method invocations to obtain the desired data.
>>
>> If we go this route we would probably end up copying the function, since
>> as said several drivers use it too.
>>
>>> If we go ahead and make this change to ACPICA: I have not seen the
>> patch, but please make sure that all vestiges of this are removed. This
>> includes all invocations within ACPICA that examine the result of the
>> _STA invocation, the CurrentStatus field of ACPI_DEVICE_INFO, the
>> ACPI_VALID_STA flag, etc.
>>
>> The patch already does that:
>>
>> https://patchwork.kernel.org/patch/10186195/
>>
> 
> 
> Of course, we will need the ACPICA version of the patch, not the "linuxized" version.
> Bob

Ok, I've submitted a pull-request with an ACPICA version of the patch here:

https://github.com/acpica/acpica/pull/364

Regards,

Hans