| Message ID | 20201121203040.146252-6-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | ACPI: scan: Split root scanning into 2 steps | expand |
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 >
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!
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 >> >
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 --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();
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(-)