Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-664-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20231208130705.kernel.v1.1.Ic5024b3da99b11e39c247a5b8ba44876c18880a0@changeid>
In-Reply-To: <20231208130705.kernel.v1.1.Ic5024b3da99b11e39c247a5b8ba44876c18880a0@changeid>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Tue, 19 Dec 2023 13:06:57 -0500
Message-ID: <CABBYNZKOKOCzLkggM1PRXuFjsaU9-0=6WmdTxaF-s3v7WSzvhg@mail.gmail.com>
Subject: Re: [kernel PATCH v1] Bluetooth: btmtksdio: clear BTMTKSDIO_BT_WAKE_ENABLED
 after resume
To: Zhengping Jiang <jiangzp@google.com>
Cc: linux-bluetooth@vger.kernel.org, marcel@holtmann.org, 
	chromeos-bluetooth-upstreaming@chromium.org, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, 
	"David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, 
	Jakub Kicinski <kuba@kernel.org>, Johan Hedberg <johan.hedberg@gmail.com>, 
	Matthias Brugger <matthias.bgg@gmail.com>, Paolo Abeni <pabeni@redhat.com>, 
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, 
	linux-mediatek@lists.infradead.org, netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Zhengping,

On Fri, Dec 8, 2023 at 4:07=E2=80=AFPM Zhengping Jiang <jiangzp@google.com>=
 wrote:
>
> Always clear BTMTKSDIO_BT_WAKE_ENABLED bit after resume. When Bluetooth
> does not generate interrupts, the bit will not be cleared and causes
> premature wakeup.
>
> Fixes: 4ed924fc122f ("Bluetooth: btmtksdio: enable bluetooth wakeup in sy=
stem suspend")
> Signed-off-by: Zhengping Jiang <jiangzp@google.com>
> ---
>
> Changes in v1:
> - Clear BTMTKSDIO_BT_WAKE_ENABLED flag on resume
>
>  drivers/bluetooth/btmtksdio.c    | 10 ++++++++++
>  include/net/bluetooth/hci_core.h |  1 +
>  net/bluetooth/hci_sync.c         |  2 ++
>  3 files changed, 13 insertions(+)
>
> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.=
c
> index ff4868c83cd8..8f00b71573c8 100644
> --- a/drivers/bluetooth/btmtksdio.c
> +++ b/drivers/bluetooth/btmtksdio.c
> @@ -1296,6 +1296,15 @@ static bool btmtksdio_sdio_inband_wakeup(struct hc=
i_dev *hdev)
>         return device_may_wakeup(bdev->dev);
>  }
>
> +static void btmtksdio_disable_bt_wakeup(struct hci_dev *hdev)
> +{
> +       struct btmtksdio_dev *bdev =3D hci_get_drvdata(hdev);
> +
> +       if (!bdev)
> +               return;
> +       clear_bit(BTMTKSDIO_BT_WAKE_ENABLED, &bdev->tx_state);
> +}
> +
>  static bool btmtksdio_sdio_wakeup(struct hci_dev *hdev)
>  {
>         struct btmtksdio_dev *bdev =3D hci_get_drvdata(hdev);
> @@ -1363,6 +1372,7 @@ static int btmtksdio_probe(struct sdio_func *func,
>         hdev->shutdown =3D btmtksdio_shutdown;
>         hdev->send     =3D btmtksdio_send_frame;
>         hdev->wakeup   =3D btmtksdio_sdio_wakeup;
> +       hdev->clear_wakeup =3D btmtksdio_disable_bt_wakeup;
>         /*
>          * If SDIO controller supports wake on Bluetooth, sending a wakeo=
n
>          * command is not necessary.
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci=
_core.h
> index 0c1754f416bd..4bbd55335269 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -672,6 +672,7 @@ struct hci_dev {
>         int (*get_codec_config_data)(struct hci_dev *hdev, __u8 type,
>                                      struct bt_codec *codec, __u8 *vnd_le=
n,
>                                      __u8 **vnd_data);
> +       void (*clear_wakeup)(struct hci_dev *hdev);

I wonder if it wouldn't be a better idea to add something like suspend
and resume callbacks to notify the about these hdev states, that way
we can synchronize the states better and avoid having to clear the
wakeup state when it shouldn't be active to begin with since the hdev
is not suspended.

>  };
>
>  #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 3563a90ed2ac..6c4d5ce40524 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -5947,6 +5947,8 @@ int hci_resume_sync(struct hci_dev *hdev)
>                 return 0;
>
>         hdev->suspended =3D false;
> +       if (hdev->clear_wakeup)
> +               hdev->clear_wakeup(hdev);
>
>         /* Restore event mask */
>         hci_set_event_mask_sync(hdev);
> --
> 2.43.0.472.g3155946c3a-goog
>


--=20
Luiz Augusto von Dentz