Hi Bjorn,
Thanks for your comments
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Thursday, October 24, 2024 12:44 AM
> To: Devegowda, Chandrashekar <chandrashekar.devegowda@xxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Srivatsa, Ravishankar
> <ravishankar.srivatsa@xxxxxxxxx>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@xxxxxxxxx>; K, Kiran <kiran.k@xxxxxxxxx>; Paul
> Menzel <pmenzel@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v1] Bluetooth: btintel_pcie: Device suspend-resume
> support added
>
> [+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@xxxxxxxxx>
> > Signed-off-by: ChandraShekar <chandrashekar.devegowda@xxxxxxxxx>
> > ---
> > 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.
>
The FW needs to perform the vendor specific housekeeping task before the device enters into D3 entry/D0 exit , So it is required to have the handshake between driver and firmware.
> 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".
>
Ack, will move the assignment before the write of the sleep control register in patch v2
> 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.
>
Ack, will incorporate the change in v2 version of the patch
> > +}
> > +
> > +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.
>
Ack, will move the assignment before the write of the sleep control register in patch v2
> > + 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
> >
