Re: [PATCH v6] Bluetooth: btintel_pcie: Add support for _suspend() / _resume()

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Kiran,


Thank you for sending the improved version 6.

Am 25.07.25 um 11:01 schrieb Kiran K:
> From: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>
> 
> This patch implements _suspend() and _resume() functions for the
> Bluetooth controller. When the system enters a suspended state, the
> driver notifies the controller to perform necessary housekeeping tasks
> by writing to the sleep control register and waits for an alive
> interrupt. The firmware raises the alive interrupt when it has
> transitioned to the D3 state. The same flow occurs when the system
> resumes.
> 
> Command to test host initiated wakeup after 60 seconds
> sudo rtcwake -m mem -s 60
> 
> dmesg log (tested on Whale Peak2 on Panther Lake platform)
> On system suspend:
> [Fri Jul 25 11:05:37 2025] Bluetooth: hci0: device entered into d3 state from d0 in 80 us
> 
> On system resume:
> [Fri Jul 25 11:06:36 2025] Bluetooth: hci0: device entered into d0 state from d3 in 7117 us
> 
> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>
> Signed-off-by: Kiran K <kiran.k@intel.com>
> ---
> changes in v6:
>       - s/delta/delta_us/g
>       - s/CONFIG_PM/CONFIG_PM_SLEEP/g
>       - use pm_sleep_pr()/pm_str() to avoid #ifdefs
>       - remove the code to set persistance mode as its not relevant to this patch
> 
> changes in v5:
>       - refactor _suspend() / _resume() to set the D3HOT/D3COLD based on power
>         event
>       - remove SIMPLE_DEV_PM_OPS and define the required pm_ops callback
>         functions
> 
> changes in v4:
>       - Moved document and section details from the commit message as comment in code.
> 
> changes in v3:
>       - Corrected the typo's
>       - Updated the CC list as suggested.
>       - Corrected the format specifiers in the logs.
> 
> changes in v2:
>       - Updated the commit message with test steps and logs.
>       - Added logs to include the timeout message.
>       - Fixed a potential race condition during suspend and resume.
> 
>   drivers/bluetooth/btintel_pcie.c | 90 ++++++++++++++++++++++++++++++++
>   1 file changed, 90 insertions(+)
> 
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index 6e7bbbd35279..c419521493fe 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -2573,11 +2573,101 @@ static void btintel_pcie_coredump(struct device *dev)
>   }
>   #endif
>   
> +#ifdef CONFIG_PM_SLEEP
> +static int btintel_pcie_suspend_late(struct device *dev, pm_message_t mesg)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct btintel_pcie_data *data;
> +	ktime_t start;
> +	u32 dxstate;
> +	s64 delta_us;
> +	int err;
> +
> +	data = pci_get_drvdata(pdev);
> +
> +	dxstate = (mesg.event == PM_EVENT_SUSPEND ?
> +		   BTINTEL_PCIE_STATE_D3_HOT : BTINTEL_PCIE_STATE_D3_COLD);

I believe the brackets are uncommon. You actually can leave it out, or, 
if brackets need to be used, only surround the condition.

> +
> +	data->gp0_received = false;
> +
> +	start = ktime_get();
> +
> +	/* Refer: 6.4.11.7 -> Platform power management */
> +	btintel_pcie_wr_sleep_cntrl(data, dxstate);
> +	err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
> +				 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
> +	if (err == 0) {
> +		bt_dev_err(data->hdev, "Timeout (%u ms) on alive interrupt for D3 entry",
> +				BTINTEL_DEFAULT_INTR_TIMEOUT_MS);
> +		return -EBUSY;
> +	}
> +
> +	delta_us = ktime_to_ns(ktime_get() - start) / 1000;
> +	bt_dev_info(data->hdev, "device entered into d3 state from d0 in %lld us",
> +		    delta_us);
> +	return 0;
> +}
> +
> +static int btintel_pcie_suspend(struct device *dev)
> +{
> +	return btintel_pcie_suspend_late(dev, PMSG_SUSPEND);
> +}
> +
> +static int btintel_pcie_hibernate(struct device *dev)
> +{
> +	return btintel_pcie_suspend_late(dev, PMSG_HIBERNATE);
> +}
> +
> +static int btintel_pcie_freeze(struct device *dev)
> +{
> +	return btintel_pcie_suspend_late(dev, PMSG_FREEZE);
> +}
> +
> +static int btintel_pcie_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct btintel_pcie_data *data;
> +	ktime_t start;
> +	int err;
> +	s64 delta_us;
> +
> +	data = pci_get_drvdata(pdev);
> +	data->gp0_received = false;
> +
> +	start = ktime_get();
> +
> +	/* Refer: 6.4.11.7 -> Platform power management */
> +	btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D0);
> +	err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
> +				 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
> +	if (err == 0) {
> +		bt_dev_err(data->hdev, "Timeout (%u ms) on alive interrupt for D0 entry",
> +				BTINTEL_DEFAULT_INTR_TIMEOUT_MS);
> +		return -EBUSY;
> +	}
> +
> +	delta_us = ktime_to_ns(ktime_get() - start) / 1000;
> +	bt_dev_info(data->hdev, "device entered into d0 state from d3 in %lld us",
> +		    delta_us);
> +	return 0;
> +}
> +
> +const struct dev_pm_ops btintel_pcie_pm_ops = {
> +	.suspend = pm_sleep_ptr(btintel_pcie_suspend),
> +	.resume = pm_sleep_ptr(btintel_pcie_resume),
> +	.freeze = pm_sleep_ptr(btintel_pcie_freeze),
> +	.thaw = pm_sleep_ptr(btintel_pcie_resume),
> +	.poweroff = pm_sleep_ptr(btintel_pcie_hibernate),
> +	.restore = pm_sleep_ptr(btintel_pcie_resume),
> +};
> +#endif
> +
>   static struct pci_driver btintel_pcie_driver = {
>   	.name = KBUILD_MODNAME,
>   	.id_table = btintel_pcie_table,
>   	.probe = btintel_pcie_probe,
>   	.remove = btintel_pcie_remove,
> +	.driver.pm = pm_ptr(&btintel_pcie_pm_ops),
>   #ifdef CONFIG_DEV_COREDUMP
>   	.driver.coredump = btintel_pcie_coredump
>   #endif

With the above comment fixed, please feel free to add:

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul