Re: [PATCH v1] Bluetooth: btintel_pcie: Device suspend-resume support added

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Paul]

On Wed, Oct 23, 2024 at 02:46:47PM +0300, ChandraShekar wrote:
> This patch contains the changes in driver to support the suspend and
> resume i.e move the controller to D3 state when the platform is entering
> into suspend and move the controller to D0 on resume.
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: ChandraShekar <chandrashekar.devegowda@intel.com>
> ---
>  drivers/bluetooth/btintel_pcie.c | 52 ++++++++++++++++++++++++++++++++
>  drivers/bluetooth/btintel_pcie.h |  4 +++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index fd4a8bd056fa..f2c44b9d7328 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -273,6 +273,12 @@ static int btintel_pcie_reset_bt(struct btintel_pcie_data *data)
>  	return reg == 0 ? 0 : -ENODEV;
>  }
>  
> +static void btintel_pcie_set_persistence_mode(struct btintel_pcie_data *data)
> +{
> +	btintel_pcie_set_reg_bits(data, BTINTEL_PCIE_CSR_HW_BOOT_CONFIG,
> +				  BTINTEL_PCIE_CSR_HW_BOOT_CONFIG_KEEP_ON);
> +}
> +
>  /* This function enables BT function by setting BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_INIT bit in
>   * BTINTEL_PCIE_CSR_FUNC_CTRL_REG register and wait for MSI-X with
>   * BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0.
> @@ -297,6 +303,8 @@ static int btintel_pcie_enable_bt(struct btintel_pcie_data *data)
>  	 */
>  	data->boot_stage_cache = 0x0;
>  
> +	btintel_pcie_set_persistence_mode(data);
> +
>  	/* Set MAC_INIT bit to start primary bootloader */
>  	reg = btintel_pcie_rd_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>  	reg &= ~(BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_INIT |
> @@ -1653,11 +1661,55 @@ static void btintel_pcie_remove(struct pci_dev *pdev)
>  	pci_set_drvdata(pdev, NULL);
>  }
>  
> +static int btintel_pcie_suspend(struct device *dev)
> +{
> +	struct btintel_pcie_data *data;
> +	int err;
> +	struct  pci_dev *pdev = to_pci_dev(dev);
> +
> +	data = pci_get_drvdata(pdev);
> +	btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D3_HOT);
> +	data->gp0_received = false;
> +	err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
> +				 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));

The generic power management code already knows how to put standard
PCI devices in D3 at suspend.  So if you have to use device-specific
code like this, I guess the implication is that this device is not
spec-compliant?  That would surprise me a little bit since Intel is
pretty good about making their devices work correctly.

Some detail about exactly how this device is non-compliant would be
helpful here and in the commit log.

It looks wrong to me that you call btintel_pcie_wr_sleep_cntrl()
(which I assume starts something that will result in an interrupt that
causes gp0_received to be set to "true") *before* you set gp0_received
to "false".

That looks like a race because the interrupt could happen before
"data->gp0_received = false", and you would return -EBUSY when you
shouldn't.  You could test this by inserting a delay before setting
"data->gp0_received = false".  Adding a delay should never break this
functionality.

It looks to me like f4c8e876ef6b ("Bluetooth: btintel_pcie: Add
handshake between driver and firmware") (currently in linux-next) has
the same race, where btintel_pcie_send_frame() starts something that
will result in an interrupt, then sets "data->gp0_received = false".

But I don't understand the workings of this hardware or the driver.

> +	if (!err) {
> +		bt_dev_err(data->hdev, "failed to receive gp0 interrupt for suspend");
> +		goto fail;
> +	}
> +	return 0;
> +fail:
> +	return -EBUSY;

Since there's no cleanup, you could just return -EBUSY directly above
and there would be no need for the goto or the "fail" label.

> +}
> +
> +static int btintel_pcie_resume(struct device *dev)
> +{
> +	struct btintel_pcie_data *data;
> +	struct  pci_dev *pdev = to_pci_dev(dev);
> +	int err;
> +
> +	data = pci_get_drvdata(pdev);
> +	btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D0);
> +	data->gp0_received = false;
> +	err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
> +				 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));

Same potential race here.  And of course I'm still dubious about the
need for this device-specific code in the first place.

> +	if (!err) {
> +		bt_dev_err(data->hdev, "failed to receive gp0 interrupt for resume");
> +		goto fail;
> +	}
> +	return 0;
> +fail:
> +	return -EBUSY;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(btintel_pcie_pm_ops, btintel_pcie_suspend,
> +		btintel_pcie_resume);
> +
>  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 = &btintel_pcie_pm_ops,
>  };
>  module_pci_driver(btintel_pcie_driver);
>  
> diff --git a/drivers/bluetooth/btintel_pcie.h b/drivers/bluetooth/btintel_pcie.h
> index f9aada0543c4..38d0c8ea2b6f 100644
> --- a/drivers/bluetooth/btintel_pcie.h
> +++ b/drivers/bluetooth/btintel_pcie.h
> @@ -8,6 +8,7 @@
>  
>  /* Control and Status Register(BTINTEL_PCIE_CSR) */
>  #define BTINTEL_PCIE_CSR_BASE			(0x000)
> +#define BTINTEL_PCIE_CSR_HW_BOOT_CONFIG		(BTINTEL_PCIE_CSR_BASE + 0x000)
>  #define BTINTEL_PCIE_CSR_FUNC_CTRL_REG		(BTINTEL_PCIE_CSR_BASE + 0x024)
>  #define BTINTEL_PCIE_CSR_HW_REV_REG		(BTINTEL_PCIE_CSR_BASE + 0x028)
>  #define BTINTEL_PCIE_CSR_RF_ID_REG		(BTINTEL_PCIE_CSR_BASE + 0x09C)
> @@ -48,6 +49,9 @@
>  #define BTINTEL_PCIE_CSR_MSIX_IVAR_BASE		(BTINTEL_PCIE_CSR_MSIX_BASE + 0x0880)
>  #define BTINTEL_PCIE_CSR_MSIX_IVAR(cause)	(BTINTEL_PCIE_CSR_MSIX_IVAR_BASE + (cause))
>  
> +/* CSR HW BOOT CONFIG Register */
> +#define BTINTEL_PCIE_CSR_HW_BOOT_CONFIG_KEEP_ON		(BIT(31))
> +
>  /* Causes for the FH register interrupts */
>  enum msix_fh_int_causes {
>  	BTINTEL_PCIE_MSIX_FH_INT_CAUSES_0	= BIT(0),	/* cause 0 */
> -- 
> 2.34.1
>