This series includes a fix for a possible race in qca_suspend() and
some minor refactoring of the same function.
Matthias Kaehlcke (3):
Bluetooth: hci_qca: Only remove TX clock vote after TX is completed
Bluetooth: hci_qca: Skip serdev wait when no transfer is pending
Bluetooth: hci_qca: Refactor error handling in qca_suspend()
drivers/bluetooth/hci_qca.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
--
2.27.0.278.ge193c7cf3a9-goog
If waiting for IBS sleep times out jump to the error handler, this is
easier to read than multiple 'if' branches and a fall through to the
error handler.
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
drivers/bluetooth/hci_qca.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 90ffd8ca1fb0d..cf76f128e9834 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2109,18 +2109,16 @@ static int __maybe_unused qca_suspend(struct device *dev)
/* Wait for HCI_IBS_SLEEP_IND sent by device to indicate its Tx is going
* to sleep, so that the packet does not wake the system later.
*/
-
ret = wait_event_interruptible_timeout(qca->suspend_wait_q,
qca->rx_ibs_state == HCI_IBS_RX_ASLEEP,
msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS));
-
- if (ret > 0) {
- qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off);
- return 0;
+ if (ret == 0) {
+ ret = -ETIMEDOUT;
+ goto error;
}
- if (ret == 0)
- ret = -ETIMEDOUT;
+ qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off);
+ return 0;
error:
clear_bit(QCA_SUSPENDING, &qca->flags);
--
2.27.0.278.ge193c7cf3a9-goog
qca_suspend() calls serdev_device_wait_until_sent() regardless of
whether a transfer is pending. While it does no active harm since
the function should return immediately it makes the code more
confusing. Add a flag to track whether a transfer is pending and
only call serdev_device_wait_until_sent() is needed.
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
drivers/bluetooth/hci_qca.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index b1d82d32892e9..90ffd8ca1fb0d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2050,6 +2050,7 @@ static int __maybe_unused qca_suspend(struct device *dev)
struct hci_uart *hu = &qcadev->serdev_hu;
struct qca_data *qca = hu->priv;
unsigned long flags;
+ bool tx_pending = false;
int ret = 0;
u8 cmd;
@@ -2083,6 +2084,7 @@ static int __maybe_unused qca_suspend(struct device *dev)
qca->tx_ibs_state = HCI_IBS_TX_ASLEEP;
qca->ibs_sent_slps++;
+ tx_pending = true;
break;
case HCI_IBS_TX_ASLEEP:
@@ -2099,8 +2101,10 @@ static int __maybe_unused qca_suspend(struct device *dev)
if (ret < 0)
goto error;
- serdev_device_wait_until_sent(hu->serdev,
- msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
+ if (tx_pending) {
+ serdev_device_wait_until_sent(hu->serdev,
+ msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
+ }
/* Wait for HCI_IBS_SLEEP_IND sent by device to indicate its Tx is going
* to sleep, so that the packet does not wake the system later.
--
2.27.0.278.ge193c7cf3a9-goog
Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
On Fri, Jun 5, 2020 at 11:46 AM Matthias Kaehlcke <[email protected]> wrote:
>
> qca_suspend() calls serdev_device_wait_until_sent() regardless of
> whether a transfer is pending. While it does no active harm since
> the function should return immediately it makes the code more
> confusing. Add a flag to track whether a transfer is pending and
> only call serdev_device_wait_until_sent() is needed.
>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
>
> drivers/bluetooth/hci_qca.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index b1d82d32892e9..90ffd8ca1fb0d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2050,6 +2050,7 @@ static int __maybe_unused qca_suspend(struct device *dev)
> struct hci_uart *hu = &qcadev->serdev_hu;
> struct qca_data *qca = hu->priv;
> unsigned long flags;
> + bool tx_pending = false;
> int ret = 0;
> u8 cmd;
>
> @@ -2083,6 +2084,7 @@ static int __maybe_unused qca_suspend(struct device *dev)
>
> qca->tx_ibs_state = HCI_IBS_TX_ASLEEP;
> qca->ibs_sent_slps++;
> + tx_pending = true;
> break;
>
> case HCI_IBS_TX_ASLEEP:
> @@ -2099,8 +2101,10 @@ static int __maybe_unused qca_suspend(struct device *dev)
> if (ret < 0)
> goto error;
>
> - serdev_device_wait_until_sent(hu->serdev,
> - msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
> + if (tx_pending) {
> + serdev_device_wait_until_sent(hu->serdev,
> + msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
> + }
>
> /* Wait for HCI_IBS_SLEEP_IND sent by device to indicate its Tx is going
> * to sleep, so that the packet does not wake the system later.
> --
> 2.27.0.278.ge193c7cf3a9-goog
>
Hi matthias,
On 2020-06-06 00:16, Matthias Kaehlcke wrote:
> qca_suspend() calls serdev_device_wait_until_sent() regardless of
> whether a transfer is pending. While it does no active harm since
> the function should return immediately it makes the code more
> confusing. Add a flag to track whether a transfer is pending and
> only call serdev_device_wait_until_sent() is needed.
>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
>
> drivers/bluetooth/hci_qca.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index b1d82d32892e9..90ffd8ca1fb0d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2050,6 +2050,7 @@ static int __maybe_unused qca_suspend(struct
> device *dev)
> struct hci_uart *hu = &qcadev->serdev_hu;
> struct qca_data *qca = hu->priv;
> unsigned long flags;
> + bool tx_pending = false;
> int ret = 0;
> u8 cmd;
>
> @@ -2083,6 +2084,7 @@ static int __maybe_unused qca_suspend(struct
> device *dev)
>
> qca->tx_ibs_state = HCI_IBS_TX_ASLEEP;
> qca->ibs_sent_slps++;
> + tx_pending = true;
> break;
>
> case HCI_IBS_TX_ASLEEP:
> @@ -2099,8 +2101,10 @@ static int __maybe_unused qca_suspend(struct
> device *dev)
> if (ret < 0)
> goto error;
>
> - serdev_device_wait_until_sent(hu->serdev,
> - msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
> + if (tx_pending) {
[Bala]: Good idea why don't we move this call to switch case under
HCI_IBS_TX_AWAKE
https://elixir.bootlin.com/linux/latest/source/drivers/bluetooth/hci_qca.c#L1994
i.e. i would recommend below sequence
1. Send SLEEP BYTE
2. wait for some time to write SLEEP Byte on Tx line
3. call for Tx clock off qca_wq_serial_tx_clock_vote_off
> + serdev_device_wait_until_sent(hu->serdev,
> + msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
> + }
>
> /* Wait for HCI_IBS_SLEEP_IND sent by device to indicate its Tx is
> going
> * to sleep, so that the packet does not wake the system later.
Hi Bala,
On Sat, Jun 06, 2020 at 06:27:59PM +0530, [email protected] wrote:
> Hi matthias,
>
> On 2020-06-06 00:16, Matthias Kaehlcke wrote:
> > qca_suspend() calls serdev_device_wait_until_sent() regardless of
> > whether a transfer is pending. While it does no active harm since
> > the function should return immediately it makes the code more
> > confusing. Add a flag to track whether a transfer is pending and
> > only call serdev_device_wait_until_sent() is needed.
> >
> > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > ---
> >
> > drivers/bluetooth/hci_qca.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index b1d82d32892e9..90ffd8ca1fb0d 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -2050,6 +2050,7 @@ static int __maybe_unused qca_suspend(struct
> > device *dev)
> > struct hci_uart *hu = &qcadev->serdev_hu;
> > struct qca_data *qca = hu->priv;
> > unsigned long flags;
> > + bool tx_pending = false;
> > int ret = 0;
> > u8 cmd;
> >
> > @@ -2083,6 +2084,7 @@ static int __maybe_unused qca_suspend(struct
> > device *dev)
> >
> > qca->tx_ibs_state = HCI_IBS_TX_ASLEEP;
> > qca->ibs_sent_slps++;
> > + tx_pending = true;
> > break;
> >
> > case HCI_IBS_TX_ASLEEP:
> > @@ -2099,8 +2101,10 @@ static int __maybe_unused qca_suspend(struct
> > device *dev)
> > if (ret < 0)
> > goto error;
> >
> > - serdev_device_wait_until_sent(hu->serdev,
> > - msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
> > + if (tx_pending) {
> [Bala]: Good idea why don't we move this call to switch case under
> HCI_IBS_TX_AWAKE
> https://elixir.bootlin.com/linux/latest/source/drivers/bluetooth/hci_qca.c#L1994
I agree that this would make the code easier to follow, however the
reason this wasn't done in the first place is (probably) that the
IBS spinlock is held during the switch/case block.
In principle the unlock could be done before calling _wait_until_sent(),
but then the unlock also needs to happen in the other 'case' branches.
It's not ideal, but also not necessarily worse than introducing
'tx_pending', the repeated unlocks might be preferable in terms of
readability.
> i.e. i would recommend below sequence
>
> 1. Send SLEEP BYTE
> 2. wait for some time to write SLEEP Byte on Tx line
> 3. call for Tx clock off qca_wq_serial_tx_clock_vote_off
>
> > + serdev_device_wait_until_sent(hu->serdev,
> > + msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
> > + }
> >
> > /* Wait for HCI_IBS_SLEEP_IND sent by device to indicate its Tx is
> > going
> > * to sleep, so that the packet does not wake the system later.
Hi Matthias,
> qca_suspend() calls serdev_device_wait_until_sent() regardless of
> whether a transfer is pending. While it does no active harm since
> the function should return immediately it makes the code more
> confusing. Add a flag to track whether a transfer is pending and
> only call serdev_device_wait_until_sent() is needed.
>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
>
> drivers/bluetooth/hci_qca.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
Hi Matthias,
> If waiting for IBS sleep times out jump to the error handler, this is
> easier to read than multiple 'if' branches and a fall through to the
> error handler.
>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
>
> drivers/bluetooth/hci_qca.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel