Re: [PATCH AUTOSEL 6.19-6.12] i3c: mipi-i3c-hci-pci: Add System Suspend support

Adrian Hunter
2026-02-11T07:57:10Z

On 11/02/2026 01:30, Sasha Levin wrote:
> From: Adrian Hunter <adrian.hunter@intel.com>
>
> [ Upstream commit 4280197d154cae1d1d5acb54484da26da04eac32 ]
>
> Assign the driver PM operations pointer, which is necessary for the PCI
> subsystem to put the device into a low power state. Refer to
> pci_pm_suspend_noirq() which bails out if the pointer is NULL, before
> it has the opportunity to call pci_prepare_to_sleep().
>
> No other actions are necessary as the mipi-i3c-hci driver takes care of
> controller state.

Back porting this patch by itself makes no sense. The entire patch set
and previous patch sets are needed to support power management.

>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Link: https://patch.msgid.link/20260123063325.8210-4-adrian.hunter@intel.com
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>
> LLM Generated explanations, may be completely bogus:
>
> Now I have a complete picture. Let me compile my detailed analysis.
>
> ---
>
> ## Comprehensive Analysis
>
> ### 1. COMMIT MESSAGE ANALYSIS
>
> The commit subject "Add System Suspend support" makes this sound like a
> new feature, but the body reveals the real story: it's fixing a bug
> where the PCI subsystem **cannot put the device into a low power state**
> during system suspend. The commit message specifically references
> `pci_pm_suspend_noirq()` which "bails out if the pointer is NULL, before
> it has the opportunity to call `pci_prepare_to_sleep()`."
>
> ### 2. CODE CHANGE ANALYSIS
>
> The change is extremely minimal:
> 1. Adds an **empty** `struct dev_pm_ops` (no callbacks):
> ```c
> static const struct dev_pm_ops mipi_i3c_hci_pci_pm_ops = {
> };
> ```
> 2. Assigns it to the PCI driver structure:
> ```c
> .driver = {
> .pm = pm_ptr(&mipi_i3c_hci_pci_pm_ops)
> },
> ```
>
> ### 3. THE BUG MECHANISM
>
> The critical code path is in `pci_pm_suspend_noirq()`:
>
> ```863:866:drivers/pci/pci-driver.c
> if (!pm) {
> pci_save_state(pci_dev);
> goto Fixup;
> }
> ```
>
> When `dev->driver->pm` is NULL (as in the current driver),
> `pci_pm_suspend_noirq()` takes the early exit at line 863-866: it saves
> PCI state but jumps to `Fixup`, **completely skipping** the call to
> `pci_prepare_to_sleep()` at line 895:
>
> ```886:896:drivers/pci/pci-driver.c
> if (!pci_dev->state_saved) {
> pci_save_state(pci_dev);
>
> /*
> - If the device is a bridge with a child in D0 below it,
> - it needs to stay in D0, so check skip_bus_pm to avoid
> - putting it into a low-power state in that case.
> */
> if (!pci_dev->skip_bus_pm &&
> pci_power_manageable(pci_dev))
> pci_prepare_to_sleep(pci_dev);
> }
> ```
>
> With the fix (non-NULL but empty `pm` ops), the code:
> - Skips the NULL-pm early exit
> - Skips the `pm->suspend_noirq` block (callback is NULL)
> - Falls through to `pci_save_state()` AND `pci_prepare_to_sleep()`,
> which transitions the device to D3hot/D3cold
>
> ### 4. USER IMPACT
>
> Without this fix, the Intel I3C HCI PCI controller stays in **D0 (full
> power)** during system suspend. This has real consequences:
> - **Higher power consumption during suspend** - device draws power when
> it shouldn't
> - **May prevent platform from reaching deeper sleep states** (S0ix/S3)
> on Intel platforms
> - Affects Intel Panther Lake, Wildcat Lake, and Nova Lake platforms
>
> This is a known pattern - other PCI drivers like `surface_dtx` and
> `aggregator/core.c` use the same empty `dev_pm_ops` trick for exactly
> this reason.
>
> ### 5. DRIVER EXISTENCE IN STABLE TREES
>
> - **v6.14**: Driver first added (148 lines). No `.driver.pm` pointer.
> - **v6.15, v6.16**: No changes to this file.
> - **v6.17**: 1 commit (Wildcat Lake-U device IDs). Still no PM.
> - **v6.18**: No changes.
> - **v6.19**: 13 commits (major refactoring). Still no PM.
>
> The bug exists in **ALL versions** of this driver. However, the diff
> from the analyzed commit targets a codebase significantly different from
> any released version (references `intel_mi_1_info`, `intel_si_2_info`,
> `mfd_remove_devices`, `mipi_i3c_hci_pci_rpm_forbid` - none of which
> exist in v6.14-v6.19). This means:
> - The exact diff **will NOT apply cleanly** to any stable tree
> - A manual adaptation would be needed, but it's **trivially simple**
> (just add the empty struct and pointer to the existing `pci_driver`
> struct)
>
> ### 6. CLASSIFICATION
>
> This is a **bug fix**, not a new feature. System suspend is expected to
> work correctly for any PCI device. The driver was missing a required
> piece (`dev_pm_ops` pointer) that the PCI subsystem needs to properly
> power-manage the device. The comment in the code even says "PM ops must
> exist for PCI to put a device to a low power state."
>
> ### 7. RISK ASSESSMENT
>
> - **Risk: Essentially zero.** The `dev_pm_ops` structure is completely
> empty - no callbacks are defined. Its sole purpose is to not be NULL,
> so the PCI core doesn't take the early-exit path.
> - **Lines changed**: ~7 lines added
> - **Files changed**: 1
> - **Reviewed-by**: Frank Li (NXP)
>
> ### 8. CONCERNS
>
> - **Dependency**: The exact diff won't apply cleanly to stable trees
> (context mismatch). A trivial manual adaptation is needed.
> - **Hardware newness**: The supported platforms (Panther Lake, etc.) are
> new Intel hardware with potentially limited user base.
> - **Not a crash/security bug**: This is a power management regression,
> not a crash or data corruption.
>
> ### 9. VERDICT
>
> The fix resolves a real power management bug that affects every user
> with Intel I3C HCI hardware doing system suspend. The change is
> trivially small, zero-risk, and follows an established pattern used by
> other PCI drivers. While the hardware is new and the issue isn't a
> crash, broken system suspend is a significant user-visible issue - users
> with these platforms will see higher battery drain during suspend, and
> the platform may not reach its desired sleep state.
>
> The fix meets stable criteria: it's obviously correct, fixes a real bug,
> is small and contained, and doesn't introduce new features. The empty PM
> ops structure adds no new behavior - it simply allows the existing PCI
> power management infrastructure to work as designed.
>
> **YES**
>
> drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c b/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
> index dc8ede0f8ad8d..f8ac1ae371cc9 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
> @@ -265,6 +265,10 @@ static void mipi_i3c_hci_pci_remove(struct pci_dev *pci)
> ida_free(&mipi_i3c_hci_pci_ida, dev_id);
> }
>
> +/* PM ops must exist for PCI to put a device to a low power state */
> +static const struct dev_pm_ops mipi_i3c_hci_pci_pm_ops = {
> +};
> +
> static const struct pci_device_id mipi_i3c_hci_pci_devices[] = {
> /* Wildcat Lake-U */
> { PCI_VDEVICE(INTEL, 0x4d7c), (kernel_ulong_t)&intel_info},
> @@ -287,6 +291,9 @@ static struct pci_driver mipi_i3c_hci_pci_driver = {
> .id_table = mipi_i3c_hci_pci_devices,
> .probe = mipi_i3c_hci_pci_probe,
> .remove = mipi_i3c_hci_pci_remove,
> + .driver = {
> + .pm = pm_ptr(&mipi_i3c_hci_pci_pm_ops)
> + },
> };
>
> module_pci_driver(mipi_i3c_hci_pci_driver);