ACPI: Do not call _STA on battery devices with unmet dependencies

ACPI: Do not call _STA on battery devices with unmet dependencies

[Hans de Goede]

Hi All,

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 commit is a preparation commit for making the ACPICA changes
in the 4th commit, this commit is necessary to not break things after
the ACPICA changes.

The second 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 third 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 fourth 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
4 patches in place we've fixed all 4 too early _STA calls.

Regards,

Hans

[PATCH 1/4] 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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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

Re: [PATCH 1/4] PCI: acpiphp_ibm: prepare for acpi_get_object_info no longer returning status

[Bjorn Helgaas]

On Thu, Jan 18, 2018 at 05:03:56PM +0100, Hans de Goede wrote:
> 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Please add "()" after function names above, then

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I assume this will be merged by Rafael along with the rest of the series.

It would be nice to fix the run-on sentences in the changelog as well.

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

Re: [PATCH 1/4] PCI: acpiphp_ibm: prepare for acpi_get_object_info no longer returning status

[Hans de Goede]

Hi,

On 01/18/2018 08:13 PM, Bjorn Helgaas wrote:
> On Thu, Jan 18, 2018 at 05:03:56PM +0100, Hans de Goede wrote:
>> 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.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Please add "()" after function names above, then

Fixed in my local tree (I will send out a new version when the other patches
have been reviewed).

> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks.

Regards,

Hans


> 
> I assume this will be merged by Rafael along with the rest of the series.
> 
> It would be nice to fix the run-on sentences in the changelog as well.
> 
>> ---
>>   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 2/4] 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 4d0979e02a28..1a5e36fab289 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -119,6 +119,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 3/4] 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 4/4] 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: ACPI: Do not call _STA on battery devices with unmet dependencies

[Bjorn Helgaas]

On Thu, Jan 18, 2018 at 05:03:55PM +0100, Hans de Goede wrote:
> Hi All,
> 
> 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 looks like interesting info, but (a) this link isn't mentioned in
the actual patches, and (b) it's conceivable that fedorapeople.org
could go away someday.  If this were a PCI series, I would suggest
opening a report at bugzilla.kernel.org, attaching the DSL there, and
including the link in the patch changelog.

> 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 commit is a preparation commit for making the ACPICA changes
> in the 4th commit, this commit is necessary to not break things after
> the ACPICA changes.
> 
> The second 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 third 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 fourth 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
> 4 patches in place we've fixed all 4 too early _STA calls.
> 
> Regards,
> 
> Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: ACPI: Do not call _STA on battery devices with unmet dependencies

[Schmauss, Erik]

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> Sent: Thursday, January 18, 2018 11:11 AM
> To: Hans de Goede <hdegoede@redhat.com>
> Cc: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
> Bjorn Helgaas <bhelgaas@google.com>; Moore, Robert
> <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>; linux-
> acpi@vger.kernel.org; linux-pci@vger.kernel.org; devel@acpica.org
> Subject: Re: ACPI: Do not call _STA on battery devices with unmet dependencies
> 
> On Thu, Jan 18, 2018 at 05:03:55PM +0100, Hans de Goede wrote:

Hi Hans,

> > Hi All,
> >
> > 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.

Could you explain why _STA method using an I2C OpRegion is problematic?

Thanks,
Erik

> >
> > Here is the DSDT of the device I'm seeing this on:
> > https://fedorapeople.org/~jwrdegoede/toshiba-click-mini-dsdt.dsl
> 
> This looks like interesting info, but (a) this link isn't mentioned in the actual
> patches, and (b) it's conceivable that fedorapeople.org could go away someday.
> If this were a PCI series, I would suggest opening a report at bugzilla.kernel.org,
> attaching the DSL there, and including the link in the patch changelog.
> 
> > 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 commit is a preparation commit for making the ACPICA changes
> > in the 4th commit, this commit is necessary to not break things after
> > the ACPICA changes.
> >
> > The second 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 third 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 fourth 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
> > 4 patches in place we've fixed all 4 too early _STA calls.
> >
> > Regards,
> >
> > Hans
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of
> a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: ACPI: Do not call _STA on battery devices with unmet dependencies

[Hans de Goede]

Hi,

On 19-01-18 22:03, Schmauss, Erik wrote:
> 
>> -----Original Message-----
>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
>> Sent: Thursday, January 18, 2018 11:11 AM
>> To: Hans de Goede <hdegoede@redhat.com>
>> Cc: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
>> Bjorn Helgaas <bhelgaas@google.com>; Moore, Robert
>> <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>; linux-
>> acpi@vger.kernel.org; linux-pci@vger.kernel.org; devel@acpica.org
>> Subject: Re: ACPI: Do not call _STA on battery devices with unmet dependencies
>>
>> On Thu, Jan 18, 2018 at 05:03:55PM +0100, Hans de Goede wrote:
> 
> Hi Hans,
> 
>>> Hi All,
>>>
>>> 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.
> 
> Could you explain why _STA method using an I2C OpRegion is problematic?

It is problematic if we call the _STA method before the handler for
the OpRegion has been installed, this series delays / avoids calling
_STA before the OpRegion has been installed.

Regards,

Hans


> 
> Thanks,
> Erik
> 
>>>
>>> Here is the DSDT of the device I'm seeing this on:
>>> https://fedorapeople.org/~jwrdegoede/toshiba-click-mini-dsdt.dsl
>>
>> This looks like interesting info, but (a) this link isn't mentioned in the actual
>> patches, and (b) it's conceivable that fedorapeople.org could go away someday.
>> If this were a PCI series, I would suggest opening a report at bugzilla.kernel.org,
>> attaching the DSL there, and including the link in the patch changelog.
>>
>>> 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 commit is a preparation commit for making the ACPICA changes
>>> in the 4th commit, this commit is necessary to not break things after
>>> the ACPICA changes.
>>>
>>> The second 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 third 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 fourth 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
>>> 4 patches in place we've fixed all 4 too early _STA calls.
>>>
>>> Regards,
>>>
>>> Hans
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-acpi"
>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>> info at  http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of
>> a message to majordomo@vger.kernel.org More majordomo info at
>> http://vger.kernel.org/majordomo-info.html

RE: ACPI: Do not call _STA on battery devices with unmet dependencies

[Schmauss, Erik]

Hi,
> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede@redhat.com]
> Sent: Saturday, January 20, 2018 4:48 AM
> To: Schmauss, Erik <erik.schmauss@intel.com>
> Cc: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
> Bjorn Helgaas <bhelgaas@google.com>; Moore, Robert
> <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>; linux-
> acpi@vger.kernel.org; linux-pci@vger.kernel.org; devel@acpica.org
> Subject: Re: ACPI: Do not call _STA on battery devices with unmet dependencies
> 
> Hi,
> 
> On 19-01-18 22:03, Schmauss, Erik wrote:
> >
> >> -----Original Message-----
> >> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> >> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> >> Sent: Thursday, January 18, 2018 11:11 AM
> >> To: Hans de Goede <hdegoede@redhat.com>
> >> Cc: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown
> >> <lenb@kernel.org>; Bjorn Helgaas <bhelgaas@google.com>; Moore, Robert
> >> <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>; linux-
> >> acpi@vger.kernel.org; linux-pci@vger.kernel.org; devel@acpica.org
> >> Subject: Re: ACPI: Do not call _STA on battery devices with unmet
> >> dependencies
> >>
> >> On Thu, Jan 18, 2018 at 05:03:55PM +0100, Hans de Goede wrote:
> >
> > Hi Hans,
> >
> >>> Hi All,
> >>>
> >>> 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.
> >
> > Could you explain why _STA method using an I2C OpRegion is problematic?
> 
> It is problematic if we call the _STA method before the handler for the OpRegion
> has been installed, this series delays / avoids calling _STA before the OpRegion
> has been installed.

Thanks for the explanation.
I'm looking at the ACPI spec and from what it said about _STA, I gathered that _STA is allowed to have side-effects to named objects within the AML namespace. So removing _STA from acpi_get_object_info seems a little dangerous... Is there a reason why you cannot remove or delay the call to acpi_get_object_info instead? Chapter 6 section 1 of the ACPI spec also states that it's possible for _HID and _ADR to read from operation regions as well. Is it common convention that these _HID and _ADR objects do not access operation regions?

Thanks,

Erik
> 
> Regards,
> 
> Hans
> 
> 
> >
> > Thanks,
> > Erik
> >
> >>>
> >>> Here is the DSDT of the device I'm seeing this on:
> >>> https://fedorapeople.org/~jwrdegoede/toshiba-click-mini-dsdt.dsl
> >>
> >> This looks like interesting info, but (a) this link isn't mentioned
> >> in the actual patches, and (b) it's conceivable that fedorapeople.org could go
> away someday.
> >> If this were a PCI series, I would suggest opening a report at
> >> bugzilla.kernel.org, attaching the DSL there, and including the link in the
> patch changelog.
> >>
> >>> 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 commit is a preparation commit for making the ACPICA
> >>> changes in the 4th commit, this commit is necessary to not break
> >>> things after the ACPICA changes.
> >>>
> >>> The second 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 third 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 fourth 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
> >>> 4 patches in place we've fixed all 4 too early _STA calls.
> >>>
> >>> Regards,
> >>>
> >>> Hans
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-acpi"
> >>> in the body of a message to majordomo@vger.kernel.org More majordomo
> >>> info at  http://vger.kernel.org/majordomo-info.html
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-acpi"
> >> in the body of a message to majordomo@vger.kernel.org More majordomo
> >> info at http://vger.kernel.org/majordomo-info.html

Re: ACPI: Do not call _STA on battery devices with unmet dependencies

[Hans de Goede]

Hi,

On 01/23/2018 01:24 AM, Schmauss, Erik wrote:
> Hi,
>> -----Original Message-----
>> From: Hans de Goede [mailto:hdegoede@redhat.com]
>> Sent: Saturday, January 20, 2018 4:48 AM
>> To: Schmauss, Erik <erik.schmauss@intel.com>
>> Cc: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
>> Bjorn Helgaas <bhelgaas@google.com>; Moore, Robert
>> <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>; linux-
>> acpi@vger.kernel.org; linux-pci@vger.kernel.org; devel@acpica.org
>> Subject: Re: ACPI: Do not call _STA on battery devices with unmet dependencies
>>
>> Hi,
>>
>> On 19-01-18 22:03, Schmauss, Erik wrote:
>>>
>>>> -----Original Message-----
>>>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>>>> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
>>>> Sent: Thursday, January 18, 2018 11:11 AM
>>>> To: Hans de Goede <hdegoede@redhat.com>
>>>> Cc: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown
>>>> <lenb@kernel.org>; Bjorn Helgaas <bhelgaas@google.com>; Moore, Robert
>>>> <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>; linux-
>>>> acpi@vger.kernel.org; linux-pci@vger.kernel.org; devel@acpica.org
>>>> Subject: Re: ACPI: Do not call _STA on battery devices with unmet
>>>> dependencies
>>>>
>>>> On Thu, Jan 18, 2018 at 05:03:55PM +0100, Hans de Goede wrote:
>>>
>>> Hi Hans,
>>>
>>>>> Hi All,
>>>>>
>>>>> The ACPI code already contains quite a bit of code to not bind theFrom the acpi_get_object_info() documentation in drivers/acpi/acpica/nsxfname.c:

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

>>>>> 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.
>>>
>>> Could you explain why _STA method using an I2C OpRegion is problematic?
>>
>> It is problematic if we call the _STA method before the handler for the OpRegion
>> has been installed, this series delays / avoids calling _STA before the OpRegion
>> has been installed.
> 
> Thanks for the explanation.
> I'm looking at the ACPI spec and from what it said about _STA, I gathered that _STA is allowed to have side-effects to named objects within the AML namespace. So removing _STA from acpi_get_object_info seems a little dangerous... Is there a reason why you cannot remove or delay the call to acpi_get_object_info instead? Chapter 6 section 1 of the ACPI spec also states that it's possible for _HID and _ADR to read from operation regions as well. Is it common convention that these _HID and _ADR objects do not access operation regions?

 From the acpi_get_object_info() documentation in drivers/acpi/acpica/nsxfname.c:

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

So if _STA can have side-effects then that seems like another reason to NOT
call it from acpi_get_object_info().

If something depends on the side-effects (which I doubt) then the code which
depends on it should really call _STA explicitly itself.

Regards,

Hans



>>>>> Here is the DSDT of the device I'm seeing this on:
>>>>> https://fedorapeople.org/~jwrdegoede/toshiba-click-mini-dsdt.dsl
>>>>
>>>> This looks like interesting info, but (a) this link isn't mentioned
>>>> in the actual patches, and (b) it's conceivable that fedorapeople.org could go
>> away someday.
>>>> If this were a PCI series, I would suggest opening a report at
>>>> bugzilla.kernel.org, attaching the DSL there, and including the link in the
>> patch changelog.
>>>>
>>>>> 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 commit is a preparation commit for making the ACPICA
>>>>> changes in the 4th commit, this commit is necessary to not break
>>>>> things after the ACPICA changes.
>>>>>
>>>>> The second 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 third 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 fourth 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
>>>>> 4 patches in place we've fixed all 4 too early _STA calls.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Hans
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-acpi"
>>>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>>>> info at  http://vger.kernel.org/majordomo-info.html
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-acpi"
>>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>>> info at http://vger.kernel.org/majordomo-info.html