Re: [PATCH] thunderbolt: Call SXFP() on Apple systems with Alpine Ridge when suspending

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] thunderbolt: acpi: tb_acpi_apple_sxfp() in suspend_late breaks noirq suspend
--

commit bf9ec70ca8c7c6c95ce1947e1918dbc5496d5f29
Author: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>

thunderbolt: Call SXFP() on Apple systems with Alpine Ridge when suspending

This commit adds a suspend_late callback for the software connection
manager that clears bit 26 of plug events CS_20 and evaluates the SXFP(0)
ACPI method under the NHI on Apple systems with Alpine Ridge controllers
when suspending to RAM.

> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 4e5d0578fd4bc..f12db9d8bf543 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -3120,6 +3120,12 @@ static int tb_suspend_noirq(struct tb *tb)
>  	return 0;
>  }
>  
> +static int tb_suspend_late(struct tb *tb)
> +{
> +	tb_acpi_apple_sxfp(tb);
> +	return 0;
> +}

[Severity: High]
Does calling tb_acpi_apple_sxfp() in the suspend_late phase cause
regressions during the subsequent suspend_noirq phase on mainline kernels?

During system suspend, dpm_suspend_late() runs for all devices before
dpm_suspend_noirq() begins. In dpm_suspend(), pci_pm_suspend() resumes the
Alpine Ridge xHCI from runtime suspend and sets pci_dev->state_saved = false,
while hcd_pci_suspend() -> suspend_common() only disables the PCI device and
defers pci_save_state() to hcd_pci_suspend_noirq().

When nhi_suspend_late() -> tb_domain_suspend_late() -> tb_suspend_late() ->
tb_acpi_apple_sxfp() evaluates SXFP(0) during dpm_suspend_late():

drivers/thunderbolt/acpi.c:tb_acpi_apple_sxfp()
    status = acpi_execute_simple_method(sxfp, NULL, 0);

SXFP(0) powers down the controller's xHCI when nothing is connected to that
side. Later, when dpm_suspend_noirq() runs, hcd_pci_suspend_noirq() saves the
PCI configuration space of the now powered-off xHCI:

drivers/usb/core/hcd-pci.c:hcd_pci_suspend_noirq()
    pci_save_state(pci_dev);

In mainline, pci_save_state() unconditionally reads standard PCI config
space and PCIe capability registers:

drivers/pci/pci.c:pci_save_state()
    for (i = 0; i < 16; i++) {
        pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
        ...
    }
    dev->state_saved = true;

Because the xHCI is already powered down, pci_save_state() records
0xffffffff into saved_config_space[] and the PCIe capability save buffers,
and marks state_saved = true. On resume, pci_pm_resume_noirq() ->
pci_pm_default_resume_early() -> pci_restore_state() writes those saved
0xffffffff values back into the xHCI's BARs, command register, and PCIe
capability registers.

Additionally, within drivers/thunderbolt/tb.c, running tb_suspend_late()
before tb_suspend_noirq() means SXFP(0) executes while tcm->hotplug_active
is still true and tb->ctl is still running, before tb_suspend_noirq() has
disconnected DisplayPort resources, exited redrive mode, and called
tb_switch_suspend(tb->root_switch, false):

drivers/thunderbolt/tb.c:tb_suspend_noirq()
    tb_disconnect_and_release_dp(tb);
    tb_switch_exit_redrive(tb->root_switch);
    tb_switch_suspend(tb->root_switch, false);
    tcm->hotplug_active = false; /* signal tb_handle_hotplug to quit */

Could evaluating SXFP(0) before tb_suspend_noirq() and hcd_pci_suspend_noirq()
corrupt the xHCI PCI state on resume or interfere with tb_switch_suspend()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019134851.7496-1-fbeltranmillalen@gmail.com?part=1