Re: [PATCH] Bluetooth: btintel_pcie: fix stale cache in set_dxstate fallback check

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

[Cc: +Chandrashekar, +Kiran]

Dear Vladimir,


Thank you for your patch.

Am 01.09.26 um 22:37 schrieb Vladimir V. Kondratyev:
> btintel_pcie_set_dxstate() falls back to checking the controller state via
> btintel_pcie_in_d3/d0() when the alive interrupt is missed. However, these
> helpers read boot_stage_cache, which is only updated by the interrupt
> handler. As such, if the interrupt was missed, the cache is stale and the
> fallback check always fails, exhausting all retries and returning -EBUSY,
> causing suspend to abort.

Can you reproduce this on real hardware?

> The fix involves re-reading the hardware register before the fallback state
> check, consistent with btintel_pcie_resume().
> 
> Fixes: e57362f4911b ("Bluetooth: btintel_pcie: Add support for _suspend() / _resume()")
> 
> Signed-off-by: Vladimir V. Kondratyev <vladimirkondratyev2@gmail.com>
> Tested-by: Sergey Lebedev <lsa.uz@pm.me>
> ---
>   drivers/bluetooth/btintel_pcie.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index fda474406003..7fca8eea6e01 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -2778,9 +2778,13 @@ static int btintel_pcie_set_dxstate(struct btintel_pcie_data *data, u32 dxstate)
>   					  BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0);
>   
>   		/* A hardware bug may cause the alive interrupt to be missed.
> -		 * Check if the controller reached the expected state and retry
> -		 * the operation only if it hasn't.
> +		 * Refresh boot_stage_cache from hardware since it is only
> +		 * updated by the interrupt handler, then check if the
> +		 * controller reached the expected state and retry the
> +		 * operation only if it hasn't.

Re-flow for less lines?

>   		 */
> +		data->boot_stage_cache = btintel_pcie_rd_reg32(data,
> +							       BTINTEL_PCIE_CSR_BOOT_STAGE_REG);

To me it sounds strange, that a register name *boot stage* should change 
after the boot. ;-)

>   		if (dxstate == BTINTEL_PCIE_STATE_D0) {
>   			if (btintel_pcie_in_d0(data))
>   				return 0;


Kind regards,

Paul