[TEST] double-check impact of ACPI patch on suspend-resume#3521
Closed
plbossart wants to merge 3 commits intothesofproject:topic/sof-devfrom
Closed
[TEST] double-check impact of ACPI patch on suspend-resume#3521plbossart wants to merge 3 commits intothesofproject:topic/sof-devfrom
plbossart wants to merge 3 commits intothesofproject:topic/sof-devfrom
Conversation
…lid" This reverts commit e38f9ff. Suspend-resume regressions were noticed on Intel TGLU_RVP and CML Helios Chromebook. BugLink: thesofproject#3459 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart
requested review from
ajitkupandey,
bardliao,
dbaluta,
kuanhsuncheng,
kv2019i,
lgirdwood,
ranj063,
ujfalusi and
yaochunhung
as code owners
Member
Author
|
https://sof-ci.01.org/linuxpr/PR3521/build7362/devicetest/ shows that with this revert both TGLU_RVP_SDW and CML_HEL_RT5682 have no problems with suspend-resume. Let's re-add this commit and see what happens. |
Section 6.1.2 of ACPI 6.4 explicitly requires _HID to be present for _CID to be defined, so don't add device IDs from _CID to the device IDs list of a device if _HID is not valid. Link: https://uefi.org/specs/ACPI/6.4/06_Device_Configuration/Device_Configuration.html#cid-compatible-id Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Member
Author
|
Adding this commit shows a regression again for TGL_RVP_SDW and CML_HELIOS: https://sof-ci.01.org/linuxpr/PR3521/build7363/devicetest/ (Zephyr results to be ignored) Let's try to revert one more time and see what happens |
…lid" This reverts commit e38f9ff. Suspend-resume regressions were noticed on Intel TGLU_RVP and CML Helios Chromebook. BugLink: thesofproject#3459 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Member
Author
|
Second revert gives good results again on TGL_RVP_SDW and CML_HELIOS, c.f. https://sof-ci.01.org/linuxpr/PR3521/build7364/devicetest/ |
Member
Author
|
@rafaeljw @andy-shev @ujfalusi I think the results speak for themselves, we have two confirmed regressions with commit e38f9ff, so somehow filtering out the pnp_cid list has a very large impact. I was able to fix the SoundWire regression by using the DPM_FLAG_SMART_SUSPEND to prevent a spurious pm_runtime resume, the other regression on a Chromebook is not clear at all. |
|
For the information of everyone on the mailing lists:
Commit e38f9ff ("ACPI: scan: Do not add device IDs from _CID if _HID
is not valid") is going to be reverted, because it caused multiple
systems to misbehave as per the below.
…On Wed, Mar 16, 2022 at 11:22 AM Rafael J. Wysocki ***@***.***> wrote:
On Wed, Mar 16, 2022 at 3:05 AM Pierre-Louis Bossart ***@***.***> wrote:
>
> @rafaeljw @andy-shev @ujfalusi I think the results speak for themselves, we have two confirmed regressions with commit e38f9ff, so somehow filtering out the pnp_cid list has a very large impact.
>
> I was able to fix the SoundWire regression by using the DPM_FLAG_SMART_SUSPEND to prevent a spurious pm_runtime resume, the other regression on a Chromebook is not clear at all.
OK, I'll revert that commit, but this pretty much means widespread firmware breakage.
|
Collaborator
|
The other thing said patch causes is that the static const struct acpi_device_id container_device_ids[] = {
{"ACPI0004", 0},
{"PNP0A05", 0},
{"PNP0A06", 0},
{"", 0},
};But interestingly the debug (on a laptop which does not break with the offending patch): If I If I don't then the |
Collaborator
|
Not invoking and with the patch (ignoring _CIDs without _HID): Notable differences are the number of physical_node links for the subdevices. |
Member
Author
@rafaeljw can you point us to an upstream commit in the ACPI tree, we'll cherry-pick it in ours. Thank you! |
plbossart
added a commit
to plbossart/sound
that referenced
this pull request
Mar 18, 2022
…lid" This reverts commit e38f9ff. Suspend-resume regressions were noticed on Intel TGLU_RVP and CML Helios Chromebook. Excert from discussion with ACPI maintainer in from thesofproject#3521 (comment): Commit e38f9ff ("ACPI: scan: Do not add device IDs from _CID if _HID is not valid") is going to be reverted, because it caused multiple systems to misbehave as per the below. BugLink: thesofproject#3459 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
ranj063
pushed a commit
that referenced
this pull request
Mar 30, 2022
…lid" This reverts commit e38f9ff. Suspend-resume regressions were noticed on Intel TGLU_RVP and CML Helios Chromebook. Excert from discussion with ACPI maintainer in from #3521 (comment): Commit e38f9ff ("ACPI: scan: Do not add device IDs from _CID if _HID is not valid") is going to be reverted, because it caused multiple systems to misbehave as per the below. BugLink: #3459 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart
added a commit
that referenced
this pull request
Apr 6, 2022
…lid" This reverts commit e38f9ff. Suspend-resume regressions were noticed on Intel TGLU_RVP and CML Helios Chromebook. Excert from discussion with ACPI maintainer in from #3521 (comment): Commit e38f9ff ("ACPI: scan: Do not add device IDs from _CID if _HID is not valid") is going to be reverted, because it caused multiple systems to misbehave as per the below. BugLink: #3459 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
|
On Wed, Mar 16, 2022 at 12:00 PM Péter Ujfalusi ***@***.***> wrote:
The other thing said patch causes is that the drivers/acpi/container.c is
not going to be invoked for PNP0A05,m which is one of the skipped ones
(PRP00001 and PNP0A05 is the _CID under SNDW and UAOL w/o them having _HID).
static const struct acpi_device_id container_device_ids[] = {
{"ACPI0004", 0},
{"PNP0A05", 0},
{"PNP0A06", 0},
{"", 0},
};
But interestingly the debug (on a laptop which does not brake with the
offending patch):
[ 0.389012] ACPI: acpi_set_pnp_ids: CID without HID, name: SNDW
[ 0.389013] ACPI: acpi_set_pnp_ids: CID #0/2: string: PRP00001
[ 0.389015] ACPI: acpi_set_pnp_ids: CID #1/2: string: PNP0A05
[ 0.392189] ACPI: acpi_set_pnp_ids: CID without HID, name: UAOL
[ 0.392190] ACPI: acpi_set_pnp_ids: CID #0/2: string: PRP00001
[ 0.392191] ACPI: acpi_set_pnp_ids: CID #1/2: string: PNP0A05
...
[ 0.666706] acpi PRP00001:00: container_device_attach: ENTER
[ 0.666752] acpi PRP00001:01: container_device_attach: ENTER
If I acpi_add_id() the CIDs without HID. If I don't then the
container_device_attach is not going to be called. The adev->dev gets
it's name from the first CID in this case?
Yes, it does.
|
|
On Wed, Mar 16, 2022 at 3:05 AM Pierre-Louis Bossart < ***@***.***> wrote:
@rafaeljw <https://github.com/rafaeljw> @andy-shev
<https://github.com/andy-shev> @ujfalusi <https://github.com/ujfalusi> I
think the results speak for themselves, we have two confirmed regressions
with commit e38f9ff
<e38f9ff>,
so somehow filtering out the pnp_cid list has a very large impact.
I was able to fix the SoundWire regression by using the
DPM_FLAG_SMART_SUSPEND to prevent a spurious pm_runtime resume, the other
regression on a Chromebook is not clear at all.
OK, I'll revert that commit, but this pretty much means widespread firmware
breakage.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Let's record the results on TGL_RVP_SDW and CML_HELIOS, where suspend-resume seems to be broken due to commit e38f9ff