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

[PATCH v3 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

Changes in v3:
-Use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL
---
 drivers/acpi/bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 4d0979e02a28..d44d2c287b91 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_GPL(acpi_bus_get_status_handle);
 
 int acpi_bus_get_status(struct acpi_device *device)
 {
-- 
2.14.3

[PATCH v3 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 v3 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 d44d2c287b91..433bb5a3f327 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 v3 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 b0fe5272c76a..4c1b90e91271 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1565,6 +1565,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)
@@ -1588,6 +1590,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);
 
@@ -1660,9 +1670,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;
@@ -1760,6 +1772,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 v3 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 v3 5/5] ACPICA: Remove calling of _STA from acpi_get_object_info

[Moore, Robert]

For ACPICA, this is a change to a published public interface. A change to this will potentially affect many operating systems, so we would be very hesitant to change it in the core ACPICA code.

Bob


> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede@redhat.com]
> Sent: Friday, January 26, 2018 7:03 AM
> To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
> Moore, Robert <robert.moore@intel.com>; Schmauss, Erik
> <erik.schmauss@intel.com>; Bjorn Helgaas <bhelgaas@google.com>
> Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org;
> devel@acpica.org; linux-pci@vger.kernel.org
> Subject: [PATCH v3 5/5] ACPICA: Remove calling of _STA from
> acpi_get_object_info
> 
> 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: [Devel] [PATCH v3 5/5] ACPICA: Remove calling of _STA from acpi_get_object_info

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 6925 bytes --]

For ACPICA, this is a change to a published public interface. A change to this will potentially affect many operating systems, so we would be very hesitant to change it in the core ACPICA code.

Bob


> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede(a)redhat.com]
> Sent: Friday, January 26, 2018 7:03 AM
> To: Rafael J . Wysocki <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>;
> Moore, Robert <robert.moore(a)intel.com>; Schmauss, Erik
> <erik.schmauss(a)intel.com>; Bjorn Helgaas <bhelgaas(a)google.com>
> Cc: Hans de Goede <hdegoede(a)redhat.com>; linux-acpi(a)vger.kernel.org;
> devel(a)acpica.org; linux-pci(a)vger.kernel.org
> Subject: [PATCH v3 5/5] ACPICA: Remove calling of _STA from
> acpi_get_object_info
> 
> 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(a)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 v3 5/5] ACPICA: Remove calling of _STA from acpi_get_object_info

[Hans de Goede]

Hi,

On 26-01-18 21:42, Moore, Robert wrote:
> For ACPICA, this is a change to a published public interface. A change to this will potentially affect many operating systems, so we would be very hesitant to change it in the core ACPICA code.

 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.

Since _STA can call Opregions, IMHO it does not belong in acpi_get_object_info()
and as Erik mentioned _STA can have side-effects then that seems like another
reason to NOT call it from acpi_get_object_info().

But I can understand that you don't want to outright change the behavior of
acpi_get_object_info() to call _STA since it has always done this, still
calling _STA can be a problem in some cases.

Perhaps we can give acpi_get_object_info() a flags argument, or if you want to
preserve the current interface ad a new acpi_get_object_info_no_sta() call (and
use flags under the hood so that we still have only 1 implementation) ?

Regards,

Hans


> 
> Bob
> 
> 
>> -----Original Message-----
>> From: Hans de Goede [mailto:hdegoede@redhat.com]
>> Sent: Friday, January 26, 2018 7:03 AM
>> To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
>> Moore, Robert <robert.moore@intel.com>; Schmauss, Erik
>> <erik.schmauss@intel.com>; Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org;
>> devel@acpica.org; linux-pci@vger.kernel.org
>> Subject: [PATCH v3 5/5] ACPICA: Remove calling of _STA from
>> acpi_get_object_info
>>
>> 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 v3 5/5] ACPICA: Remove calling of _STA from acpi_get_object_info

[Rafael J. Wysocki]

On Sat, Jan 27, 2018 at 3:02 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 26-01-18 21:42, Moore, Robert wrote:
>>
>> For ACPICA, this is a change to a published public interface. A change to
>> this will potentially affect many operating systems, so we would be very
>> hesitant to change it in the core ACPICA code.
>
>
> 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.
>
> Since _STA can call Opregions, IMHO it does not belong in
> acpi_get_object_info()
> and as Erik mentioned _STA can have side-effects then that seems like
> another
> reason to NOT call it from acpi_get_object_info().
>
> But I can understand that you don't want to outright change the behavior of
> acpi_get_object_info() to call _STA since it has always done this, still
> calling _STA can be a problem in some cases.
>
> Perhaps we can give acpi_get_object_info() a flags argument, or if you want
> to
> preserve the current interface ad a new acpi_get_object_info_no_sta() call
> (and
> use flags under the hood so that we still have only 1 implementation) ?

Let's first get the [1-4/5] from this series in (as the last patch
depends on them AFAICS) and then we'll see.

Thanks,
Rafael

Re: [Devel] [PATCH v3 5/5] ACPICA: Remove calling of _STA from acpi_get_object_info

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

On Sat, Jan 27, 2018 at 3:02 PM, Hans de Goede <hdegoede(a)redhat.com> wrote:
> Hi,
>
> On 26-01-18 21:42, Moore, Robert wrote:
>>
>> For ACPICA, this is a change to a published public interface. A change to
>> this will potentially affect many operating systems, so we would be very
>> hesitant to change it in the core ACPICA code.
>
>
> 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.
>
> Since _STA can call Opregions, IMHO it does not belong in
> acpi_get_object_info()
> and as Erik mentioned _STA can have side-effects then that seems like
> another
> reason to NOT call it from acpi_get_object_info().
>
> But I can understand that you don't want to outright change the behavior of
> acpi_get_object_info() to call _STA since it has always done this, still
> calling _STA can be a problem in some cases.
>
> Perhaps we can give acpi_get_object_info() a flags argument, or if you want
> to
> preserve the current interface ad a new acpi_get_object_info_no_sta() call
> (and
> use flags under the hood so that we still have only 1 implementation) ?

Let's first get the [1-4/5] from this series in (as the last patch
depends on them AFAICS) and then we'll see.

Thanks,
Rafael

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

[Hans de Goede]

Hi,

On 27-01-18 15:02, Hans de Goede wrote:
> Hi,
> 
> On 26-01-18 21:42, Moore, Robert wrote:
>> For ACPICA, this is a change to a published public interface. A change to this will potentially affect many operating systems, so we would be very hesitant to change it in the core ACPICA code.
> 
>  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.
> 
> Since _STA can call Opregions, IMHO it does not belong in acpi_get_object_info()
> and as Erik mentioned _STA can have side-effects then that seems like another
> reason to NOT call it from acpi_get_object_info().
> 
> But I can understand that you don't want to outright change the behavior of
> acpi_get_object_info() to call _STA since it has always done this, still
> calling _STA can be a problem in some cases.
> 
> Perhaps we can give acpi_get_object_info() a flags argument, or if you want to
> preserve the current interface ad a new acpi_get_object_info_no_sta() call (and
> use flags under the hood so that we still have only 1 implementation) ?

Erm, ping? It would be nice to get some feedback on my suggestions how to deal
with this... ?

Regards,

Hans





> 
> Regards,
> 
> Hans
> 
> 
>>
>> Bob
>>
>>
>>> -----Original Message-----
>>> From: Hans de Goede [mailto:hdegoede@redhat.com]
>>> Sent: Friday, January 26, 2018 7:03 AM
>>> To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
>>> Moore, Robert <robert.moore@intel.com>; Schmauss, Erik
>>> <erik.schmauss@intel.com>; Bjorn Helgaas <bhelgaas@google.com>
>>> Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org;
>>> devel@acpica.org; linux-pci@vger.kernel.org
>>> Subject: [PATCH v3 5/5] ACPICA: Remove calling of _STA from
>>> acpi_get_object_info
>>>
>>> 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 v3 5/5] ACPICA: Remove calling of _STA from acpi_get_object_info

[Hans de Goede]

Hi,

On 27-01-18 15:02, Hans de Goede wrote:
> Hi,
> 
> On 26-01-18 21:42, Moore, Robert wrote:
>> For ACPICA, this is a change to a published public interface. A change to this will potentially affect many operating systems, so we would be very hesitant to change it in the core ACPICA code.
> 
>  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.
> 
> Since _STA can call Opregions, IMHO it does not belong in acpi_get_object_info()
> and as Erik mentioned _STA can have side-effects then that seems like another
> reason to NOT call it from acpi_get_object_info().
> 
> But I can understand that you don't want to outright change the behavior of
> acpi_get_object_info() to call _STA since it has always done this, still
> calling _STA can be a problem in some cases.
> 
> Perhaps we can give acpi_get_object_info() a flags argument, or if you want to
> preserve the current interface ad a new acpi_get_object_info_no_sta() call (and
> use flags under the hood so that we still have only 1 implementation) ?

p.s.

A cleaner fix might simply be to make whether or not acpi_get_object_info()
calls _STA configurable, by say doing something like this in acconfig.h:

#ifndef ACPI_GET_OBJ_INFO_WITH_STA
#define ACPI_GET_OBJ_INFO_WITH_STA 1
#endif

And then in aclinux.h:

#define ACPI_GET_OBJ_INFO_WITH_STA 0

And instead of removing the calling of _STA all together as my initial
patch did, wrap it in:

#if ACPI_GET_OBJ_INFO_WITH_STA
#endif

Shall I respin the patch to do this ?

Regards,

Hans




>>> -----Original Message-----
>>> From: Hans de Goede [mailto:hdegoede@redhat.com]
>>> Sent: Friday, January 26, 2018 7:03 AM
>>> To: Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
>>> Moore, Robert <robert.moore@intel.com>; Schmauss, Erik
>>> <erik.schmauss@intel.com>; Bjorn Helgaas <bhelgaas@google.com>
>>> Cc: Hans de Goede <hdegoede@redhat.com>; linux-acpi@vger.kernel.org;
>>> devel@acpica.org; linux-pci@vger.kernel.org
>>> Subject: [PATCH v3 5/5] ACPICA: Remove calling of _STA from
>>> acpi_get_object_info
>>>
>>> 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
>>