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

Message ID 20180126150300.9691-5-hdegoede@redhat.com
State Not Applicable
Headers show
Series
  • [v3,1/5] ACPI: export acpi_bus_get_status_handle()
Related show

Commit Message

Hans de Goede Jan. 26, 2018, 3:03 p.m.
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(-)

Comments

Moore, Robert Jan. 26, 2018, 8:42 p.m. | #1
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
Hans de Goede Jan. 27, 2018, 2:02 p.m. | #2
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
>
Rafael J. Wysocki Jan. 29, 2018, 3:07 a.m. | #3
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
Hans de Goede Feb. 8, 2018, 12:02 p.m. | #4
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
>>
Hans de Goede Feb. 8, 2018, 12:16 p.m. | #5
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
>>

Patch

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