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

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

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]

SGksDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IEhhbnMgZGUgR29lZGUg
W21haWx0bzpoZGVnb2VkZUByZWRoYXQuY29tXQ0KPiBTZW50OiBTYXR1cmRheSwgSmFudWFyeSAy
MCwgMjAxOCA0OjQ4IEFNDQo+IFRvOiBTY2htYXVzcywgRXJpayA8ZXJpay5zY2htYXVzc0BpbnRl
bC5jb20+DQo+IENjOiBSYWZhZWwgSiAuIFd5c29ja2kgPHJqd0Byand5c29ja2kubmV0PjsgTGVu
IEJyb3duIDxsZW5iQGtlcm5lbC5vcmc+Ow0KPiBCam9ybiBIZWxnYWFzIDxiaGVsZ2Fhc0Bnb29n
bGUuY29tPjsgTW9vcmUsIFJvYmVydA0KPiA8cm9iZXJ0Lm1vb3JlQGludGVsLmNvbT47IFpoZW5n
LCBMdiA8bHYuemhlbmdAaW50ZWwuY29tPjsgbGludXgtDQo+IGFjcGlAdmdlci5rZXJuZWwub3Jn
OyBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBkZXZlbEBhY3BpY2Eub3JnDQo+IFN1YmplY3Q6
IFJlOiBBQ1BJOiBEbyBub3QgY2FsbCBfU1RBIG9uIGJhdHRlcnkgZGV2aWNlcyB3aXRoIHVubWV0
IGRlcGVuZGVuY2llcw0KPiANCj4gSGksDQo+IA0KPiBPbiAxOS0wMS0xOCAyMjowMywgU2NobWF1
c3MsIEVyaWsgd3JvdGU6DQo+ID4NCj4gPj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4g
Pj4gRnJvbTogbGludXgtYWNwaS1vd25lckB2Z2VyLmtlcm5lbC5vcmcgW21haWx0bzpsaW51eC1h
Y3BpLQ0KPiA+PiBvd25lckB2Z2VyLmtlcm5lbC5vcmddIE9uIEJlaGFsZiBPZiBCam9ybiBIZWxn
YWFzDQo+ID4+IFNlbnQ6IFRodXJzZGF5LCBKYW51YXJ5IDE4LCAyMDE4IDExOjExIEFNDQo+ID4+
IFRvOiBIYW5zIGRlIEdvZWRlIDxoZGVnb2VkZUByZWRoYXQuY29tPg0KPiA+PiBDYzogUmFmYWVs
IEogLiBXeXNvY2tpIDxyandAcmp3eXNvY2tpLm5ldD47IExlbiBCcm93bg0KPiA+PiA8bGVuYkBr
ZXJuZWwub3JnPjsgQmpvcm4gSGVsZ2FhcyA8YmhlbGdhYXNAZ29vZ2xlLmNvbT47IE1vb3JlLCBS
b2JlcnQNCj4gPj4gPHJvYmVydC5tb29yZUBpbnRlbC5jb20+OyBaaGVuZywgTHYgPGx2LnpoZW5n
QGludGVsLmNvbT47IGxpbnV4LQ0KPiA+PiBhY3BpQHZnZXIua2VybmVsLm9yZzsgbGludXgtcGNp
QHZnZXIua2VybmVsLm9yZzsgZGV2ZWxAYWNwaWNhLm9yZw0KPiA+PiBTdWJqZWN0OiBSZTogQUNQ
STogRG8gbm90IGNhbGwgX1NUQSBvbiBiYXR0ZXJ5IGRldmljZXMgd2l0aCB1bm1ldA0KPiA+PiBk
ZXBlbmRlbmNpZXMNCj4gPj4NCj4gPj4gT24gVGh1LCBKYW4gMTgsIDIwMTggYXQgMDU6MDM6NTVQ
TSArMDEwMCwgSGFucyBkZSBHb2VkZSB3cm90ZToNCj4gPg0KPiA+IEhpIEhhbnMsDQo+ID4NCj4g
Pj4+IEhpIEFsbCwNCj4gPj4+DQo+ID4+PiBUaGUgQUNQSSBjb2RlIGFscmVhZHkgY29udGFpbnMg
cXVpdGUgYSBiaXQgb2YgY29kZSB0byBub3QgYmluZCB0aGUNCj4gPj4+IEFDUEktYmF0dGVyeSB1
bnRpbCBhbGwgZGVwcyBmb3IgYW4gQUNQSSBiYXR0ZXJ5IGRldmljZSBoYXZlIGJlZW4NCj4gPj4+
IG1ldCwgYnV0IG9uIHNvbWUgZGV2aWNlcyBjYWxsaW5nIF9TVEEgYmVmb3JlIGFsbCBkZXBzIGFy
ZSBtZXQgaXMgYQ0KPiA+Pj4gcHJvYmxlbSB0b28gYmVjYXVzZSB0aGUgX1NUQSBtZXRob2QgdXNl
cyBhbiBpMmMgT3BSZWdpb24gdGhlcmUuDQo+ID4NCj4gPiBDb3VsZCB5b3UgZXhwbGFpbiB3aHkg
X1NUQSBtZXRob2QgdXNpbmcgYW4gSTJDIE9wUmVnaW9uIGlzIHByb2JsZW1hdGljPw0KPiANCj4g
SXQgaXMgcHJvYmxlbWF0aWMgaWYgd2UgY2FsbCB0aGUgX1NUQSBtZXRob2QgYmVmb3JlIHRoZSBo
YW5kbGVyIGZvciB0aGUgT3BSZWdpb24NCj4gaGFzIGJlZW4gaW5zdGFsbGVkLCB0aGlzIHNlcmll
cyBkZWxheXMgLyBhdm9pZHMgY2FsbGluZyBfU1RBIGJlZm9yZSB0aGUgT3BSZWdpb24NCj4gaGFz
IGJlZW4gaW5zdGFsbGVkLg0KDQpUaGFua3MgZm9yIHRoZSBleHBsYW5hdGlvbi4NCkknbSBsb29r
aW5nIGF0IHRoZSBBQ1BJIHNwZWMgYW5kIGZyb20gd2hhdCBpdCBzYWlkIGFib3V0IF9TVEEsIEkg
Z2F0aGVyZWQgdGhhdCBfU1RBIGlzIGFsbG93ZWQgdG8gaGF2ZSBzaWRlLWVmZmVjdHMgdG8gbmFt
ZWQgb2JqZWN0cyB3aXRoaW4gdGhlIEFNTCBuYW1lc3BhY2UuIFNvIHJlbW92aW5nIF9TVEEgZnJv
bSBhY3BpX2dldF9vYmplY3RfaW5mbyBzZWVtcyBhIGxpdHRsZSBkYW5nZXJvdXMuLi4gSXMgdGhl
cmUgYSByZWFzb24gd2h5IHlvdSBjYW5ub3QgcmVtb3ZlIG9yIGRlbGF5IHRoZSBjYWxsIHRvIGFj
cGlfZ2V0X29iamVjdF9pbmZvIGluc3RlYWQ/IENoYXB0ZXIgNiBzZWN0aW9uIDEgb2YgdGhlIEFD
UEkgc3BlYyBhbHNvIHN0YXRlcyB0aGF0IGl0J3MgcG9zc2libGUgZm9yIF9ISUQgYW5kIF9BRFIg
dG8gcmVhZCBmcm9tIG9wZXJhdGlvbiByZWdpb25zIGFzIHdlbGwuIElzIGl0IGNvbW1vbiBjb252
ZW50aW9uIHRoYXQgdGhlc2UgX0hJRCBhbmQgX0FEUiBvYmplY3RzIGRvIG5vdCBhY2Nlc3Mgb3Bl
cmF0aW9uIHJlZ2lvbnM/DQoNClRoYW5rcywNCg0KRXJpaw0KPiANCj4gUmVnYXJkcywNCj4gDQo+
IEhhbnMNCj4gDQo+IA0KPiA+DQo+ID4gVGhhbmtzLA0KPiA+IEVyaWsNCj4gPg0KPiA+Pj4NCj4g
Pj4+IEhlcmUgaXMgdGhlIERTRFQgb2YgdGhlIGRldmljZSBJJ20gc2VlaW5nIHRoaXMgb246DQo+
ID4+PiBodHRwczovL2ZlZG9yYXBlb3BsZS5vcmcvfmp3cmRlZ29lZGUvdG9zaGliYS1jbGljay1t
aW5pLWRzZHQuZHNsDQo+ID4+DQo+ID4+IFRoaXMgbG9va3MgbGlrZSBpbnRlcmVzdGluZyBpbmZv
LCBidXQgKGEpIHRoaXMgbGluayBpc24ndCBtZW50aW9uZWQNCj4gPj4gaW4gdGhlIGFjdHVhbCBw
YXRjaGVzLCBhbmQgKGIpIGl0J3MgY29uY2VpdmFibGUgdGhhdCBmZWRvcmFwZW9wbGUub3JnIGNv
dWxkIGdvDQo+IGF3YXkgc29tZWRheS4NCj4gPj4gSWYgdGhpcyB3ZXJlIGEgUENJIHNlcmllcywg
SSB3b3VsZCBzdWdnZXN0IG9wZW5pbmcgYSByZXBvcnQgYXQNCj4gPj4gYnVnemlsbGEua2VybmVs
Lm9yZywgYXR0YWNoaW5nIHRoZSBEU0wgdGhlcmUsIGFuZCBpbmNsdWRpbmcgdGhlIGxpbmsgaW4g
dGhlDQo+IHBhdGNoIGNoYW5nZWxvZy4NCj4gPj4NCj4gPj4+IFRoaXMgc2VyaWVzIG1vZGlmaWVz
IHRoZSBrZXJuZWwgdG8gbm90IGNhbGwgX1NUQSB1bnRpbCBhbGwgZGVwcyBhcmUNCj4gPj4+IG1l
dCwgbWlycm9yaW5nIHRoZSBiaW5kaW5nIGJlaGF2aW9yIG9mIHRoZSBiYXR0ZXJ5IGRyaXZlci4N
Cj4gPj4+DQo+ID4+PiBXaXRob3V0IHRoaXMgc2VyaWVzIGEgdG90YWwgb2YgMzIgQUNQSSBlcnJv
cnMgZ2V0IHByaW50ZW5kIHRvIHRoZQ0KPiA+Pj4gY29uc29sZSBvbiBib290LCB0aGVyZSBhcmUg
NCBlcnJvcnMgcGVyIF9TVEEgY2FsbCwgMiBiYXR0ZXJ5IGRldmljZXMNCj4gPj4+IG9uIHRoaXMg
c3lzdGVtIGFuZCA0IF9TVEEgY2FsbHMgcGVyIGJhdHRlcnkgZGV2aWNlLg0KPiA+Pj4NCj4gPj4+
IFRoZSBmaXJzdCBjb21taXQgaXMgYSBwcmVwYXJhdGlvbiBjb21taXQgZm9yIG1ha2luZyB0aGUg
QUNQSUNBDQo+ID4+PiBjaGFuZ2VzIGluIHRoZSA0dGggY29tbWl0LCB0aGlzIGNvbW1pdCBpcyBu
ZWNlc3NhcnkgdG8gbm90IGJyZWFrDQo+ID4+PiB0aGluZ3MgYWZ0ZXIgdGhlIEFDUElDQSBjaGFu
Z2VzLg0KPiA+Pj4NCj4gPj4+IFRoZSBzZWNvbmQgY29tbWl0IG1vZGlmaWVzIGFjcGlfYnVzX2dl
dF9zdGF0dXMgdG8gbm90IGNhbGwgX1NUQSBvbg0KPiA+Pj4gYmF0dGVyeSBkZXZpY2VzIHVudGls
IGFsbCBkZXBzIGFyZSBtZXQuIFRoaXMgZml4ZXMgMiBvZiB0aGUgNCB0b28NCj4gPj4+IGVhcmx5
IF9TVEEgY2FsbHMgdHJpZ2dlcmluZyB0aGVzZSBlcnJvcnMuDQo+ID4+Pg0KPiA+Pj4gVGhlIHRo
aXJkIGNvbW1pdCBtYWtlcyB0aGUgZGV2aWNlIGluc3RhbnRpYXRpb24gY29kZSB1c2UNCj4gPj4+
IGFjcGlfYnVzX2dldF9zdGF0dXMgaW5zdGVhZCBvZiBhY3BpX2J1c19nZXRfc3RhdHVzX2hhbmRs
ZSBzbyB0aGF0DQo+ID4+PiB0aGUgY29kZSB0byBnZXQgdGhlIGluaXRpYWwgc3RhdHVzIGFsc28g
ZG9lcyBub3QgbWFrZXMgMSB0b28gZWFybHkgX1NUQSBjYWxsLg0KPiA+Pj4NCj4gPj4+IFRoZSBm
b3VydGggY29tbWl0IGNoYW5nZXMgdGhlIEFDUElDQSBhY3BpX2dldF9vYmplY3RfaW5mbyBmdW5j
dGlvbg0KPiA+Pj4gdG8gbm90IGNhbGwgX1NUQS4gT25seSAxIHVzZXIgKHdoaWNoIGlzIGZpeGVk
IGluIHRoZSBmaXJzdCBjb21taXQpDQo+ID4+PiBjYXJlcyBhYm91dCBhY3BpX2RldmljZV9pbmZv
LmN1cnJlbnRfc3RhdHVzLiBBbmQgdGhlIEFDUElDQSBjb2RlIGhhcw0KPiA+Pj4gdGhpcw0KPiA+
PiBjb21tZW50Og0KPiA+Pj4NCj4gPj4+ICAgKiBOb3RlOiBUaGlzIGludGVyZmFjZSBpcyBpbnRl
bmRlZCB0byBiZSB1c2VkIGR1cmluZyB0aGUgaW5pdGlhbA0KPiA+Pj4gZGV2aWNlDQo+ID4+PiAg
ICogZGlzY292ZXJ5IG5hbWVzcGFjZSB0cmF2ZXJzYWwuIFRoZXJlZm9yZSwgbm8gY29tcGxleCBt
ZXRob2RzIGNhbiBiZQ0KPiA+Pj4gICAqIGV4ZWN1dGVkLCBlc3BlY2lhbGx5IHRob3NlIHRoYXQg
YWNjZXNzIG9wZXJhdGlvbiByZWdpb25zLg0KPiA+Pj4gVGhlcmVmb3JlLCBkbw0KPiA+Pj4gICAq
IG5vdCBhZGQgYW55IGFkZGl0aW9uYWwgbWV0aG9kcyB0aGF0IGNvdWxkIGNhdXNlIHByb2JsZW1z
IGluIHRoaXMgYXJlYS4NCj4gPj4+ICAgKiBCZWNhdXNlIG9mIHRoaXMgcmVhc29uIHN1cHBvcnQg
Zm9yIHRoZSBmb2xsb3dpbmcgbWV0aG9kcyBoYXMNCj4gPj4+IGJlZW4NCj4gPj4gcmVtb3ZlZDoN
Cj4gPj4+ICAgKiB0aGlzIHdhcyB0aGUgZmF0ZSBvZiB0aGUgX1NVQiBtZXRob2Qgd2hpY2ggd2Fz
IGZvdW5kIHRvIGNhdXNlIHN1Y2gNCj4gPj4+ICAgKiBwcm9ibGVtcyBhbmQgd2FzIHJlbW92ZWQg
KDExLzIwMTUpLg0KPiA+Pj4NCj4gPj4+IFRoZSBkZXNjcmliZWQgcHJvYmxlbXMgd2l0aCB0aGUg
X1NVQiBtZXRob2QgY2xlYXJseSBhbHNvIGFwcGx5IHRvDQo+ID4+PiB0aGUgX1NUQSBtZXRob2Qs
IHNvIHJlbW92aW5nIGl0IGZyb20gYWNwaV9nZXRfb2JqZWN0X2luZm8gc2VlbXMgbGlrZQ0KPiA+
Pj4gaXQgaXMgdGhlIHJpZ2h0IHRoaW5nIHRvIGRvIGhlcmUuIFRoaXMgdG9vIGZpeGVzIDEgdG9v
IGVhcmx5IF9TVEENCj4gPj4+IGNhbGwsIHNvIHRoYXQgd2l0aCBhbGwNCj4gPj4+IDQgcGF0Y2hl
cyBpbiBwbGFjZSB3ZSd2ZSBmaXhlZCBhbGwgNCB0b28gZWFybHkgX1NUQSBjYWxscy4NCj4gPj4+
DQo+ID4+PiBSZWdhcmRzLA0KPiA+Pj4NCj4gPj4+IEhhbnMNCj4gPj4+IC0tDQo+ID4+PiBUbyB1
bnN1YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgbGlu
dXgtYWNwaSINCj4gPj4+IGluIHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdl
ci5rZXJuZWwub3JnIE1vcmUgbWFqb3Jkb21vDQo+ID4+PiBpbmZvIGF0ICBodHRwOi8vdmdlci5r
ZXJuZWwub3JnL21ham9yZG9tby1pbmZvLmh0bWwNCj4gPj4gLS0NCj4gPj4gVG8gdW5zdWJzY3Jp
YmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LWFjcGki
DQo+ID4+IGluIHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwu
b3JnIE1vcmUgbWFqb3Jkb21vDQo+ID4+IGluZm8gYXQgaHR0cDovL3ZnZXIua2VybmVsLm9yZy9t
YWpvcmRvbW8taW5mby5odG1sDQo=

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