diff mbox series

[5/7] ACPI: scan: Add support for deferring adding devices to the second scan phase based on the _DEP list

Message ID 20201121203040.146252-6-hdegoede@redhat.com
State New
Headers show
Series ACPI: scan: Split root scanning into 2 steps | expand

Commit Message

Hans de Goede Nov. 21, 2020, 8:30 p.m. UTC
The current solution, of deferring adding of some devices because they
need access during the OpRegions of other devices while they are added,
is not very generic.

And support for making the decision to defer adding a device based on
its _DEP list, instead of the device's HID being in a fixed list of HIDs
to defer, which should be a more generic way to deal with this.

Since this is likely to cause issues on some hardware, this new method will
only be used if the new acpi.defer_scan_based_on_dep kernel commandline
option is set to 1.

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

Comments

Rafael J. Wysocki Nov. 23, 2020, 12:17 p.m. UTC | #1
On Sat, Nov 21, 2020 at 9:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>

> The current solution, of deferring adding of some devices because they

> need access during the OpRegions of other devices while they are added,

> is not very generic.

>

> And support for making the decision to defer adding a device based on

> its _DEP list, instead of the device's HID being in a fixed list of HIDs

> to defer, which should be a more generic way to deal with this.


Thanks a lot for working on this!

I'll have a more thorough look at the series later this week, stay tuned.

> Since this is likely to cause issues on some hardware, this new method will

> only be used if the new acpi.defer_scan_based_on_dep kernel commandline

> option is set to 1.


However, I already can say that I don't like the new command line option.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

> ---

>  drivers/acpi/scan.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-

>  1 file changed, 44 insertions(+), 1 deletion(-)

>

> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c

> index 407c8536568b..9927036bfe77 100644

> --- a/drivers/acpi/scan.c

> +++ b/drivers/acpi/scan.c

> @@ -31,6 +31,11 @@ extern struct acpi_device *acpi_root;

>

>  #define INVALID_ACPI_HANDLE    ((acpi_handle)empty_zero_page)

>

> +static int defer_scan_based_on_dep = -1;

> +module_param(defer_scan_based_on_dep, int, 0444);

> +MODULE_PARM_DESC(defer_scan_based_on_dep,

> +       "Use new scan-scheme deferring addition of devices with non empty _DEP list (-1=auto, 0=no, 1=yes)");

> +

>  static const char *dummy_hid = "device";

>

>  static LIST_HEAD(acpi_dep_list);

> @@ -1657,11 +1662,45 @@ void acpi_device_add_finalize(struct acpi_device *device)

>

>  static bool acpi_should_defer_add(acpi_handle handle, struct acpi_device_info *info)

>  {

> +       struct acpi_handle_list dep_devices;

> +       acpi_status status;

> +       int i;

> +

>         if (!acpi_check_defer_add || !info)

>                 return false;

>

> -       if (acpi_info_matches_hids(info, acpi_defer_add_hids))

> +       if (!defer_scan_based_on_dep)

> +               return acpi_info_matches_hids(info, acpi_defer_add_hids);

> +

> +       /*

> +        * We check for _ADR here to avoid deferring the adding of the following:

> +        * 1. PCI devices

> +        * 2. ACPI nodes describing USB ports

> +        * Note checking for _ADR catches more then just these cases...


s/then/than/

> +        */

> +       if (info->valid & ACPI_VALID_ADR)

> +               return false;

> +

> +       status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);

> +       if (ACPI_FAILURE(status))

> +               return false;

> +

> +       for (i = 0; i < dep_devices.count; i++) {

> +               struct acpi_device_info *dep_info;

> +               bool ignore;

> +

> +               status = acpi_get_object_info(dep_devices.handles[i], &dep_info);

> +               if (ACPI_FAILURE(status))

> +                       continue;

> +

> +               ignore = acpi_info_matches_hids(dep_info, acpi_ignore_dep_hids);

> +               kfree(dep_info);

> +

> +               if (ignore)

> +                       continue;

> +

>                 return true;

> +       }

>

>         return false;

>  }

> @@ -2251,6 +2290,10 @@ int __init acpi_scan_init(void)

>         struct acpi_table_stao *stao_ptr;

>         struct acpi_deferred_dev *deferred_dev, *tmp;

>

> +       /* Currently no devices are known which need _dep based scan deferral */

> +       if (defer_scan_based_on_dep == -1)

> +               defer_scan_based_on_dep = 0;

> +

>         acpi_pci_root_init();

>         acpi_pci_link_init();

>         acpi_processor_init();

> --

> 2.28.0

>
Rafael J. Wysocki Nov. 23, 2020, 12:41 p.m. UTC | #2
On Mon, Nov 23, 2020 at 2:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>

> Hi,

>

> On 11/23/20 1:17 PM, Rafael J. Wysocki wrote:

> > On Sat, Nov 21, 2020 at 9:31 PM Hans de Goede <hdegoede@redhat.com> wrote:

> >>

> >> The current solution, of deferring adding of some devices because they

> >> need access during the OpRegions of other devices while they are added,

> >> is not very generic.

> >>

> >> And support for making the decision to defer adding a device based on

> >> its _DEP list, instead of the device's HID being in a fixed list of HIDs

> >> to defer, which should be a more generic way to deal with this.

> >

> > Thanks a lot for working on this!

>

> You're welcome.

>

> > I'll have a more thorough look at the series later this week, stay tuned.

>

> Ok.

>

> >> Since this is likely to cause issues on some hardware, this new method will

> >> only be used if the new acpi.defer_scan_based_on_dep kernel commandline

> >> option is set to 1.

> >

> > However, I already can say that I don't like the new command line option.

>

> You don't like the name, or you don't like having a commandline option for this?


The latter.

> Anyways I'll wait till you have taken a closer look.


OK, thanks!
Hans de Goede Nov. 23, 2020, 1:30 p.m. UTC | #3
Hi,

On 11/23/20 1:17 PM, Rafael J. Wysocki wrote:
> On Sat, Nov 21, 2020 at 9:31 PM Hans de Goede <hdegoede@redhat.com> wrote:

>>

>> The current solution, of deferring adding of some devices because they

>> need access during the OpRegions of other devices while they are added,

>> is not very generic.

>>

>> And support for making the decision to defer adding a device based on

>> its _DEP list, instead of the device's HID being in a fixed list of HIDs

>> to defer, which should be a more generic way to deal with this.

> 

> Thanks a lot for working on this!


You're welcome.

> I'll have a more thorough look at the series later this week, stay tuned.


Ok.

>> Since this is likely to cause issues on some hardware, this new method will

>> only be used if the new acpi.defer_scan_based_on_dep kernel commandline

>> option is set to 1.

> 

> However, I already can say that I don't like the new command line option.


You don't like the name, or you don't like having a commandline option for this?

Anyways I'll wait till you have taken a closer look.

Regards,

Hans


> 

>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

>> ---

>>  drivers/acpi/scan.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-

>>  1 file changed, 44 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c

>> index 407c8536568b..9927036bfe77 100644

>> --- a/drivers/acpi/scan.c

>> +++ b/drivers/acpi/scan.c

>> @@ -31,6 +31,11 @@ extern struct acpi_device *acpi_root;

>>

>>  #define INVALID_ACPI_HANDLE    ((acpi_handle)empty_zero_page)

>>

>> +static int defer_scan_based_on_dep = -1;

>> +module_param(defer_scan_based_on_dep, int, 0444);

>> +MODULE_PARM_DESC(defer_scan_based_on_dep,

>> +       "Use new scan-scheme deferring addition of devices with non empty _DEP list (-1=auto, 0=no, 1=yes)");

>> +

>>  static const char *dummy_hid = "device";

>>

>>  static LIST_HEAD(acpi_dep_list);

>> @@ -1657,11 +1662,45 @@ void acpi_device_add_finalize(struct acpi_device *device)

>>

>>  static bool acpi_should_defer_add(acpi_handle handle, struct acpi_device_info *info)

>>  {

>> +       struct acpi_handle_list dep_devices;

>> +       acpi_status status;

>> +       int i;

>> +

>>         if (!acpi_check_defer_add || !info)

>>                 return false;

>>

>> -       if (acpi_info_matches_hids(info, acpi_defer_add_hids))

>> +       if (!defer_scan_based_on_dep)

>> +               return acpi_info_matches_hids(info, acpi_defer_add_hids);

>> +

>> +       /*

>> +        * We check for _ADR here to avoid deferring the adding of the following:

>> +        * 1. PCI devices

>> +        * 2. ACPI nodes describing USB ports

>> +        * Note checking for _ADR catches more then just these cases...

> 

> s/then/than/

> 

>> +        */

>> +       if (info->valid & ACPI_VALID_ADR)

>> +               return false;

>> +

>> +       status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);

>> +       if (ACPI_FAILURE(status))

>> +               return false;

>> +

>> +       for (i = 0; i < dep_devices.count; i++) {

>> +               struct acpi_device_info *dep_info;

>> +               bool ignore;

>> +

>> +               status = acpi_get_object_info(dep_devices.handles[i], &dep_info);

>> +               if (ACPI_FAILURE(status))

>> +                       continue;

>> +

>> +               ignore = acpi_info_matches_hids(dep_info, acpi_ignore_dep_hids);

>> +               kfree(dep_info);

>> +

>> +               if (ignore)

>> +                       continue;

>> +

>>                 return true;

>> +       }

>>

>>         return false;

>>  }

>> @@ -2251,6 +2290,10 @@ int __init acpi_scan_init(void)

>>         struct acpi_table_stao *stao_ptr;

>>         struct acpi_deferred_dev *deferred_dev, *tmp;

>>

>> +       /* Currently no devices are known which need _dep based scan deferral */

>> +       if (defer_scan_based_on_dep == -1)

>> +               defer_scan_based_on_dep = 0;

>> +

>>         acpi_pci_root_init();

>>         acpi_pci_link_init();

>>         acpi_processor_init();

>> --

>> 2.28.0

>>

>
Hans de Goede Nov. 23, 2020, 1:49 p.m. UTC | #4
Hi,

On 11/23/20 1:41 PM, Rafael J. Wysocki wrote:
> On Mon, Nov 23, 2020 at 2:31 PM Hans de Goede <hdegoede@redhat.com> wrote:

>>

>> Hi,

>>

>> On 11/23/20 1:17 PM, Rafael J. Wysocki wrote:

>>> On Sat, Nov 21, 2020 at 9:31 PM Hans de Goede <hdegoede@redhat.com> wrote:

>>>>

>>>> The current solution, of deferring adding of some devices because they

>>>> need access during the OpRegions of other devices while they are added,

>>>> is not very generic.

>>>>

>>>> And support for making the decision to defer adding a device based on

>>>> its _DEP list, instead of the device's HID being in a fixed list of HIDs

>>>> to defer, which should be a more generic way to deal with this.

>>>

>>> Thanks a lot for working on this!

>>

>> You're welcome.

>>

>>> I'll have a more thorough look at the series later this week, stay tuned.

>>

>> Ok.

>>

>>>> Since this is likely to cause issues on some hardware, this new method will

>>>> only be used if the new acpi.defer_scan_based_on_dep kernel commandline

>>>> option is set to 1.

>>>

>>> However, I already can say that I don't like the new command line option.

>>

>> You don't like the name, or you don't like having a commandline option for this?

> 

> The latter.


I already expected as much. Some initial thoughts on this. Note feel free to
respond later when you are reviewing the set:

I think that this is a bit adventurous. But this was a weekend project for
me and I only had time to test on the Acer Switch Sw3-016 so far, and that
worked well. So maybe it will work better then expected with some more testing.

If we want to do this be default from now on, then we need to take some
measures to avoid the acpi_ignore_dep_hids list added by one of the preparation
patches from growing endlessly. What would help here is extending the new 
acpi_info_matches_hids() helper to not only check the HIDs against
acpi_device_info.hardware_id but also the compatible_id_list and then replace
the System Power Management Controller HIDs in acpi_ignore_dep_hids list with
"PNP0D80" I believe that that should catch all PMC-s without needing to have
a HID per hardware/chipset generation.

Regards,

Hans
diff mbox series

Patch

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 407c8536568b..9927036bfe77 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -31,6 +31,11 @@  extern struct acpi_device *acpi_root;
 
 #define INVALID_ACPI_HANDLE	((acpi_handle)empty_zero_page)
 
+static int defer_scan_based_on_dep = -1;
+module_param(defer_scan_based_on_dep, int, 0444);
+MODULE_PARM_DESC(defer_scan_based_on_dep,
+	"Use new scan-scheme deferring addition of devices with non empty _DEP list (-1=auto, 0=no, 1=yes)");
+
 static const char *dummy_hid = "device";
 
 static LIST_HEAD(acpi_dep_list);
@@ -1657,11 +1662,45 @@  void acpi_device_add_finalize(struct acpi_device *device)
 
 static bool acpi_should_defer_add(acpi_handle handle, struct acpi_device_info *info)
 {
+	struct acpi_handle_list dep_devices;
+	acpi_status status;
+	int i;
+
 	if (!acpi_check_defer_add || !info)
 		return false;
 
-	if (acpi_info_matches_hids(info, acpi_defer_add_hids))
+	if (!defer_scan_based_on_dep)
+		return acpi_info_matches_hids(info, acpi_defer_add_hids);
+
+	/*
+	 * We check for _ADR here to avoid deferring the adding of the following:
+	 * 1. PCI devices
+	 * 2. ACPI nodes describing USB ports
+	 * Note checking for _ADR catches more then just these cases...
+	 */
+	if (info->valid & ACPI_VALID_ADR)
+		return false;
+
+	status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	for (i = 0; i < dep_devices.count; i++) {
+		struct acpi_device_info *dep_info;
+		bool ignore;
+
+		status = acpi_get_object_info(dep_devices.handles[i], &dep_info);
+		if (ACPI_FAILURE(status))
+			continue;
+
+		ignore = acpi_info_matches_hids(dep_info, acpi_ignore_dep_hids);
+		kfree(dep_info);
+
+		if (ignore)
+			continue;
+
 		return true;
+	}
 
 	return false;
 }
@@ -2251,6 +2290,10 @@  int __init acpi_scan_init(void)
 	struct acpi_table_stao *stao_ptr;
 	struct acpi_deferred_dev *deferred_dev, *tmp;
 
+	/* Currently no devices are known which need _dep based scan deferral */
+	if (defer_scan_based_on_dep == -1)
+		defer_scan_based_on_dep = 0;
+
 	acpi_pci_root_init();
 	acpi_pci_link_init();
 	acpi_processor_init();