| Message ID | 20211117220118.408953-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ACPI: scan: Skip turning off some unused objects during scan | expand |
On Wed, Nov 17, 2021 at 11:01 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Rafael, > > Commit c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by > unused objects") adds a: > > bus_for_each_dev(&acpi_bus_type, NULL, NULL, acpi_dev_turn_off_if_unused); > > call to acpi_scan_init(). On some devices with buggy DSDTs calling > _PS3 for one device may result in it turning off another device. Well, I'm going to revert this commit. I'm sending a pull request with the revert later today. > Specifically the DSDT of the GPD win and GPD pocket devices has a > "\\_SB_.PCI0.SDHB.BRC1" device for a non existing SDIO wifi module > which _PS3 method sets a GPIO causing the PCI wifi card to turn off. > > I've an earlier, in some ways simpler, fix for this here: > https://fedorapeople.org/~jwrdegoede/0001-ACPI-scan-Skip-turning-off-some-unused-objects-durin.patch > > But the sdhci-acpi.c MMC host code already has an older workaround > for it to not toggle power on this broken ACPI object; and this > simpler fix would require keeping that workaround. So then we would > have 2 workarounds for the same issue in the kernel. > > Thus instead I've come up with a slightly different approach which > IMHO has ended up pretty well. > > Patches 1-3 of this series are this different approach and assuming > they are considered ok must be merged into 5.16 to fix the regression > caused by commit c10383e8ddf4 on these devices. So I'll have a look at these and if they look good, we can do that instead of the problematic commit in 5.17. > Patch 4 removes the now no longer necessary workaround for the same > issue from the sdhci-acpi.c code. Once 1-3 are merged this could > also go to 5.16 but 5.17 is fine too. > > Patch 5 is a small bonus cleanup to the sdhci-acpi.c code. Thanks!
Hi, On 11/18/21 12:08, Rafael J. Wysocki wrote: > On Wed, Nov 17, 2021 at 11:01 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Rafael, >> >> Commit c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by >> unused objects") adds a: >> >> bus_for_each_dev(&acpi_bus_type, NULL, NULL, acpi_dev_turn_off_if_unused); >> >> call to acpi_scan_init(). On some devices with buggy DSDTs calling >> _PS3 for one device may result in it turning off another device. > > Well, I'm going to revert this commit. I'm sending a pull request > with the revert later today. > >> Specifically the DSDT of the GPD win and GPD pocket devices has a >> "\\_SB_.PCI0.SDHB.BRC1" device for a non existing SDIO wifi module >> which _PS3 method sets a GPIO causing the PCI wifi card to turn off. >> >> I've an earlier, in some ways simpler, fix for this here: >> https://fedorapeople.org/~jwrdegoede/0001-ACPI-scan-Skip-turning-off-some-unused-objects-durin.patch >> >> But the sdhci-acpi.c MMC host code already has an older workaround >> for it to not toggle power on this broken ACPI object; and this >> simpler fix would require keeping that workaround. So then we would >> have 2 workarounds for the same issue in the kernel. >> >> Thus instead I've come up with a slightly different approach which >> IMHO has ended up pretty well. >> >> Patches 1-3 of this series are this different approach and assuming >> they are considered ok must be merged into 5.16 to fix the regression >> caused by commit c10383e8ddf4 on these devices. > > So I'll have a look at these and if they look good, we can do that > instead of the problematic commit in 5.17. I'm a bit confused now, if the problematic commit is going to get reversed then technically we don't need this series anymore ? Or are you planning on re-introducing it in some form for 5.17 ? With that said getting this series merged would still be good, patch 1 + 2 make the existing always_present quirk code more generic which might be useful later. And then patch 3 (which is small) allows dropping some ugliness from the sdhci-acpi.c code since the DSDT bug we are hitting will now be solved by the new acpi-dev-status-override mechanism. Regards, Hans
On Thursday, November 18, 2021 12:15:28 PM CET Hans de Goede wrote: > Hi, > > On 11/18/21 12:08, Rafael J. Wysocki wrote: > > On Wed, Nov 17, 2021 at 11:01 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi Rafael, > >> > >> Commit c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by > >> unused objects") adds a: > >> > >> bus_for_each_dev(&acpi_bus_type, NULL, NULL, acpi_dev_turn_off_if_unused); > >> > >> call to acpi_scan_init(). On some devices with buggy DSDTs calling > >> _PS3 for one device may result in it turning off another device. > > > > Well, I'm going to revert this commit. I'm sending a pull request > > with the revert later today. > > > >> Specifically the DSDT of the GPD win and GPD pocket devices has a > >> "\\_SB_.PCI0.SDHB.BRC1" device for a non existing SDIO wifi module > >> which _PS3 method sets a GPIO causing the PCI wifi card to turn off. > >> > >> I've an earlier, in some ways simpler, fix for this here: > >> https://fedorapeople.org/~jwrdegoede/0001-ACPI-scan-Skip-turning-off-some-unused-objects-durin.patch > >> > >> But the sdhci-acpi.c MMC host code already has an older workaround > >> for it to not toggle power on this broken ACPI object; and this > >> simpler fix would require keeping that workaround. So then we would > >> have 2 workarounds for the same issue in the kernel. > >> > >> Thus instead I've come up with a slightly different approach which > >> IMHO has ended up pretty well. > >> > >> Patches 1-3 of this series are this different approach and assuming > >> they are considered ok must be merged into 5.16 to fix the regression > >> caused by commit c10383e8ddf4 on these devices. > > > > So I'll have a look at these and if they look good, we can do that > > instead of the problematic commit in 5.17. > > I'm a bit confused now, if the problematic commit is going to get > reversed then technically we don't need this series anymore ? That's correct. > Or are you planning on re-introducing it in some form for 5.17 ? I have been considering this. > With that said getting this series merged would still be good, > patch 1 + 2 make the existing always_present quirk code more generic > which might be useful later. And then patch 3 (which is small) > allows dropping some ugliness from the sdhci-acpi.c code since > the DSDT bug we are hitting will now be solved by the > new acpi-dev-status-override mechanism. OK, so this would be applicable for 5.17, but a couple of changelogs need to be updated if I'm not mistaken. Can you please do that and resend the series?
Hi, On 11/18/21 15:51, Rafael J. Wysocki wrote: > On Thursday, November 18, 2021 12:15:28 PM CET Hans de Goede wrote: >> Hi, >> >> On 11/18/21 12:08, Rafael J. Wysocki wrote: >>> On Wed, Nov 17, 2021 at 11:01 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> Hi Rafael, >>>> >>>> Commit c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by >>>> unused objects") adds a: >>>> >>>> bus_for_each_dev(&acpi_bus_type, NULL, NULL, acpi_dev_turn_off_if_unused); >>>> >>>> call to acpi_scan_init(). On some devices with buggy DSDTs calling >>>> _PS3 for one device may result in it turning off another device. >>> >>> Well, I'm going to revert this commit. I'm sending a pull request >>> with the revert later today. >>> >>>> Specifically the DSDT of the GPD win and GPD pocket devices has a >>>> "\\_SB_.PCI0.SDHB.BRC1" device for a non existing SDIO wifi module >>>> which _PS3 method sets a GPIO causing the PCI wifi card to turn off. >>>> >>>> I've an earlier, in some ways simpler, fix for this here: >>>> https://fedorapeople.org/~jwrdegoede/0001-ACPI-scan-Skip-turning-off-some-unused-objects-durin.patch >>>> >>>> But the sdhci-acpi.c MMC host code already has an older workaround >>>> for it to not toggle power on this broken ACPI object; and this >>>> simpler fix would require keeping that workaround. So then we would >>>> have 2 workarounds for the same issue in the kernel. >>>> >>>> Thus instead I've come up with a slightly different approach which >>>> IMHO has ended up pretty well. >>>> >>>> Patches 1-3 of this series are this different approach and assuming >>>> they are considered ok must be merged into 5.16 to fix the regression >>>> caused by commit c10383e8ddf4 on these devices. >>> >>> So I'll have a look at these and if they look good, we can do that >>> instead of the problematic commit in 5.17. >> >> I'm a bit confused now, if the problematic commit is going to get >> reversed then technically we don't need this series anymore ? > > That's correct. > >> Or are you planning on re-introducing it in some form for 5.17 ? > > I have been considering this. > >> With that said getting this series merged would still be good, >> patch 1 + 2 make the existing always_present quirk code more generic >> which might be useful later. And then patch 3 (which is small) >> allows dropping some ugliness from the sdhci-acpi.c code since >> the DSDT bug we are hitting will now be solved by the >> new acpi-dev-status-override mechanism. > > OK, so this would be applicable for 5.17, but a couple of changelogs > need to be updated if I'm not mistaken. > > Can you please do that and resend the series? I will update some of the commit messages and send a v2 tomorrow. Regards, Hans
Hi Rafael, Commit c10383e8ddf4 ("ACPI: scan: Release PM resources blocked by unused objects") adds a: bus_for_each_dev(&acpi_bus_type, NULL, NULL, acpi_dev_turn_off_if_unused); call to acpi_scan_init(). On some devices with buggy DSDTs calling _PS3 for one device may result in it turning off another device. Specifically the DSDT of the GPD win and GPD pocket devices has a "\\_SB_.PCI0.SDHB.BRC1" device for a non existing SDIO wifi module which _PS3 method sets a GPIO causing the PCI wifi card to turn off. I've an earlier, in some ways simpler, fix for this here: https://fedorapeople.org/~jwrdegoede/0001-ACPI-scan-Skip-turning-off-some-unused-objects-durin.patch But the sdhci-acpi.c MMC host code already has an older workaround for it to not toggle power on this broken ACPI object; and this simpler fix would require keeping that workaround. So then we would have 2 workarounds for the same issue in the kernel. Thus instead I've come up with a slightly different approach which IMHO has ended up pretty well. Patches 1-3 of this series are this different approach and assuming they are considered ok must be merged into 5.16 to fix the regression caused by commit c10383e8ddf4 on these devices. Patch 4 removes the now no longer necessary workaround for the same issue from the sdhci-acpi.c code. Once 1-3 are merged this could also go to 5.16 but 5.17 is fine too. Patch 5 is a small bonus cleanup to the sdhci-acpi.c code. Regards, Hans Hans de Goede (5): ACPI: Change acpi_device_always_present() into acpi_device_override_status() ACPI: x86: Allow specifying acpi_device_override_status() quirks by path ACPI: x86: Add not-present quirk for the PCI0.SDHB.BRC1 device on the GPD win mmc: sdhci-acpi: Remove special handling for GPD win/pocket devices mmc: sdhci-acpi: Use the new soc_intel_is_byt() helper drivers/acpi/bus.c | 4 +- drivers/acpi/x86/utils.c | 114 ++++++++++++++++++++++++---------- drivers/mmc/host/sdhci-acpi.c | 78 ++--------------------- include/acpi/acpi_bus.h | 5 +- 4 files changed, 91 insertions(+), 110 deletions(-)