* [PATCH 0/3]
@ 2020-06-05 18:46 Matthias Kaehlcke
2020-06-05 18:46 ` [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed Matthias Kaehlcke
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Matthias Kaehlcke @ 2020-06-05 18:46 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg
Cc: linux-bluetooth, Rocky Liao, Zijun Hu, linux-kernel,
Balakrishna Godavarthi, Abhishek Pandit-Subedi, Claire Chang,
Matthias Kaehlcke
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed 2020-06-05 18:46 [PATCH 0/3] Matthias Kaehlcke @ 2020-06-05 18:46 ` Matthias Kaehlcke 2020-06-05 20:44 ` Abhishek Pandit-Subedi ` (2 more replies) 2020-06-05 18:46 ` [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending Matthias Kaehlcke 2020-06-05 18:46 ` [PATCH 3/3] Bluetooth: hci_qca: Refactor error handling in qca_suspend() Matthias Kaehlcke 2 siblings, 3 replies; 14+ messages in thread From: Matthias Kaehlcke @ 2020-06-05 18:46 UTC (permalink / raw) To: Marcel Holtmann, Johan Hedberg Cc: linux-bluetooth, Rocky Liao, Zijun Hu, linux-kernel, Balakrishna Godavarthi, Abhishek Pandit-Subedi, Claire Chang, Matthias Kaehlcke qca_suspend() removes the vote for the UART TX clock after writing an IBS sleep request to the serial buffer. This is not a good idea since there is no guarantee that the request has been sent at this point. Instead remove the vote after successfully entering IBS sleep. This also fixes the issue of the vote being removed in case of an aborted suspend due to a failure of entering IBS sleep. Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support") Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/bluetooth/hci_qca.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index ece9f91cc3deb..b1d82d32892e9 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -2083,8 +2083,6 @@ static int __maybe_unused qca_suspend(struct device *dev) qca->tx_ibs_state = HCI_IBS_TX_ASLEEP; qca->ibs_sent_slps++; - - qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); break; case HCI_IBS_TX_ASLEEP: @@ -2112,8 +2110,10 @@ static int __maybe_unused qca_suspend(struct device *dev) qca->rx_ibs_state == HCI_IBS_RX_ASLEEP, msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS)); - if (ret > 0) + if (ret > 0) { + qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); return 0; + } if (ret == 0) ret = -ETIMEDOUT; -- 2.27.0.278.ge193c7cf3a9-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed 2020-06-05 18:46 ` [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed Matthias Kaehlcke @ 2020-06-05 20:44 ` Abhishek Pandit-Subedi 2020-06-06 12:53 ` bgodavar 2020-06-08 8:11 ` Marcel Holtmann 2 siblings, 0 replies; 14+ messages in thread From: Abhishek Pandit-Subedi @ 2020-06-05 20:44 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Marcel Holtmann, Johan Hedberg, Bluez mailing list, Rocky Liao, Zijun Hu, LKML, Balakrishna Godavarthi, Claire Chang Hi, On Fri, Jun 5, 2020 at 11:46 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > qca_suspend() removes the vote for the UART TX clock after > writing an IBS sleep request to the serial buffer. This is > not a good idea since there is no guarantee that the request > has been sent at this point. Instead remove the vote after > successfully entering IBS sleep. This also fixes the issue > of the vote being removed in case of an aborted suspend due > to a failure of entering IBS sleep. > > Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support") > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > drivers/bluetooth/hci_qca.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index ece9f91cc3deb..b1d82d32892e9 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -2083,8 +2083,6 @@ static int __maybe_unused qca_suspend(struct device *dev) > > qca->tx_ibs_state = HCI_IBS_TX_ASLEEP; > qca->ibs_sent_slps++; > - > - qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); > break; > > case HCI_IBS_TX_ASLEEP: > @@ -2112,8 +2110,10 @@ static int __maybe_unused qca_suspend(struct device *dev) > qca->rx_ibs_state == HCI_IBS_RX_ASLEEP, > msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS)); > > - if (ret > 0) > + if (ret > 0) { > + qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); > return 0; > + } > > if (ret == 0) > ret = -ETIMEDOUT; > -- > 2.27.0.278.ge193c7cf3a9-goog > I checked the order of calls and it looks correct per commit message. Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed 2020-06-05 18:46 ` [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed Matthias Kaehlcke 2020-06-05 20:44 ` Abhishek Pandit-Subedi @ 2020-06-06 12:53 ` bgodavar 2020-06-06 15:26 ` Matthias Kaehlcke 2020-06-08 8:11 ` Marcel Holtmann 2 siblings, 1 reply; 14+ messages in thread From: bgodavar @ 2020-06-06 12:53 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Marcel Holtmann, Johan Hedberg, linux-bluetooth, Rocky Liao, Zijun Hu, linux-kernel, Abhishek Pandit-Subedi, Claire Chang Hi Matthias, On 2020-06-06 00:16, Matthias Kaehlcke wrote: > qca_suspend() removes the vote for the UART TX clock after > writing an IBS sleep request to the serial buffer. This is > not a good idea since there is no guarantee that the request > has been sent at this point. Instead remove the vote after > successfully entering IBS sleep. This also fixes the issue > of the vote being removed in case of an aborted suspend due > to a failure of entering IBS sleep. > > Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support") > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > drivers/bluetooth/hci_qca.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index ece9f91cc3deb..b1d82d32892e9 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -2083,8 +2083,6 @@ static int __maybe_unused qca_suspend(struct > device *dev) > > qca->tx_ibs_state = HCI_IBS_TX_ASLEEP; > qca->ibs_sent_slps++; > - > - qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); > break; > > case HCI_IBS_TX_ASLEEP: > @@ -2112,8 +2110,10 @@ static int __maybe_unused qca_suspend(struct > device *dev) > qca->rx_ibs_state == HCI_IBS_RX_ASLEEP, > msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS)); > > - if (ret > 0) > + if (ret > 0) { > + qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); [Bala]: qca_wq_serial_tx_clock_vote_off votes for Tx clock off, when both Tx clock and Rx clock voted to off. then only actual call to clock off is called. https://elixir.bootlin.com/linux/latest/source/drivers/bluetooth/hci_qca.c#L312 I would recommend to vote Tx clock off after sending SLEEP BYTE from HOST TO BT SOC. > return 0; > + } > > if (ret == 0) > ret = -ETIMEDOUT; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed 2020-06-06 12:53 ` bgodavar @ 2020-06-06 15:26 ` Matthias Kaehlcke 0 siblings, 0 replies; 14+ messages in thread From: Matthias Kaehlcke @ 2020-06-06 15:26 UTC (permalink / raw) To: bgodavar Cc: Marcel Holtmann, Johan Hedberg, linux-bluetooth, Rocky Liao, Zijun Hu, linux-kernel, Abhishek Pandit-Subedi, Claire Chang Hi Bala, On Sat, Jun 06, 2020 at 06:23:13PM +0530, bgodavar@codeaurora.org wrote: > Hi Matthias, > > On 2020-06-06 00:16, Matthias Kaehlcke wrote: > > qca_suspend() removes the vote for the UART TX clock after > > writing an IBS sleep request to the serial buffer. This is > > not a good idea since there is no guarantee that the request > > has been sent at this point. Instead remove the vote after > > successfully entering IBS sleep. This also fixes the issue > > of the vote being removed in case of an aborted suspend due > > to a failure of entering IBS sleep. > > > > Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support") > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > drivers/bluetooth/hci_qca.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > > index ece9f91cc3deb..b1d82d32892e9 100644 > > --- a/drivers/bluetooth/hci_qca.c > > +++ b/drivers/bluetooth/hci_qca.c > > @@ -2083,8 +2083,6 @@ static int __maybe_unused qca_suspend(struct > > device *dev) > > > > qca->tx_ibs_state = HCI_IBS_TX_ASLEEP; > > qca->ibs_sent_slps++; > > - > > - qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); > > break; > > > > case HCI_IBS_TX_ASLEEP: > > @@ -2112,8 +2110,10 @@ static int __maybe_unused qca_suspend(struct > > device *dev) > > qca->rx_ibs_state == HCI_IBS_RX_ASLEEP, > > msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS)); > > > > - if (ret > 0) > > + if (ret > 0) { > > + qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); > [Bala]: qca_wq_serial_tx_clock_vote_off votes for Tx clock off, when both Tx > clock and Rx clock voted to off. > then only actual call to clock off is called. > https://elixir.bootlin.com/linux/latest/source/drivers/bluetooth/hci_qca.c#L312 > I would recommend to vote Tx clock off after sending SLEEP BYTE from HOST TO > BT SOC. Are you suggesting to move the vote after _wait_until_sent() and before waiting for RX_SLEEP? I think if the vote is done before RX_SLEEP and going to RX_SLEEP fails the variables qca->tx_vote and qca->tx_votes_off would have the wrong state, even if that doesn't lead to actually switching the clock off. I might be missing something though, I'm not very familiar with this part. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed 2020-06-05 18:46 ` [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed Matthias Kaehlcke 2020-06-05 20:44 ` Abhishek Pandit-Subedi 2020-06-06 12:53 ` bgodavar @ 2020-06-08 8:11 ` Marcel Holtmann 2 siblings, 0 replies; 14+ messages in thread From: Marcel Holtmann @ 2020-06-08 8:11 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Johan Hedberg, linux-bluetooth, Rocky Liao, Zijun Hu, linux-kernel, Balakrishna Godavarthi, Abhishek Pandit-Subedi, Claire Chang Hi Matthias, > qca_suspend() removes the vote for the UART TX clock after > writing an IBS sleep request to the serial buffer. This is > not a good idea since there is no guarantee that the request > has been sent at this point. Instead remove the vote after > successfully entering IBS sleep. This also fixes the issue > of the vote being removed in case of an aborted suspend due > to a failure of entering IBS sleep. > > Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support") > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > drivers/bluetooth/hci_qca.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending 2020-06-05 18:46 [PATCH 0/3] Matthias Kaehlcke 2020-06-05 18:46 ` [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed Matthias Kaehlcke @ 2020-06-05 18:46 ` Matthias Kaehlcke 2020-06-05 20:45 ` Abhishek Pandit-Subedi ` (2 more replies) 2020-06-05 18:46 ` [PATCH 3/3] Bluetooth: hci_qca: Refactor error handling in qca_suspend() Matthias Kaehlcke 2 siblings, 3 replies; 14+ messages in thread From: Matthias Kaehlcke @ 2020-06-05 18:46 UTC (permalink / raw) To: Marcel Holtmann, Johan Hedberg Cc: linux-bluetooth, Rocky Liao, Zijun Hu, linux-kernel, Balakrishna Godavarthi, Abhishek Pandit-Subedi, Claire Chang, Matthias Kaehlcke 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 <mka@chromium.org> --- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending 2020-06-05 18:46 ` [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending Matthias Kaehlcke @ 2020-06-05 20:45 ` Abhishek Pandit-Subedi 2020-06-06 12:57 ` bgodavar 2020-06-08 8:13 ` Marcel Holtmann 2 siblings, 0 replies; 14+ messages in thread From: Abhishek Pandit-Subedi @ 2020-06-05 20:45 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Marcel Holtmann, Johan Hedberg, Bluez mailing list, Rocky Liao, Zijun Hu, LKML, Balakrishna Godavarthi, Claire Chang Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> On Fri, Jun 5, 2020 at 11:46 AM Matthias Kaehlcke <mka@chromium.org> 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 <mka@chromium.org> > --- > > 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending 2020-06-05 18:46 ` [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending Matthias Kaehlcke 2020-06-05 20:45 ` Abhishek Pandit-Subedi @ 2020-06-06 12:57 ` bgodavar 2020-06-06 14:42 ` Matthias Kaehlcke 2020-06-08 8:13 ` Marcel Holtmann 2 siblings, 1 reply; 14+ messages in thread From: bgodavar @ 2020-06-06 12:57 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Marcel Holtmann, Johan Hedberg, linux-bluetooth, Rocky Liao, Zijun Hu, linux-kernel, Abhishek Pandit-Subedi, Claire Chang, hemantg 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 <mka@chromium.org> > --- > > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending 2020-06-06 12:57 ` bgodavar @ 2020-06-06 14:42 ` Matthias Kaehlcke 0 siblings, 0 replies; 14+ messages in thread From: Matthias Kaehlcke @ 2020-06-06 14:42 UTC (permalink / raw) To: bgodavar Cc: Marcel Holtmann, Johan Hedberg, linux-bluetooth, Rocky Liao, Zijun Hu, linux-kernel, Abhishek Pandit-Subedi, Claire Chang, hemantg Hi Bala, On Sat, Jun 06, 2020 at 06:27:59PM +0530, bgodavar@codeaurora.org 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 <mka@chromium.org> > > --- > > > > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending 2020-06-05 18:46 ` [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending Matthias Kaehlcke 2020-06-05 20:45 ` Abhishek Pandit-Subedi 2020-06-06 12:57 ` bgodavar @ 2020-06-08 8:13 ` Marcel Holtmann 2 siblings, 0 replies; 14+ messages in thread From: Marcel Holtmann @ 2020-06-08 8:13 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Johan Hedberg, Bluez mailing list, Rocky Liao, Zijun Hu, open list, Balakrishna Godavarthi, Abhishek Pandit-Subedi, Claire Chang 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 <mka@chromium.org> > --- > > drivers/bluetooth/hci_qca.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] Bluetooth: hci_qca: Refactor error handling in qca_suspend() 2020-06-05 18:46 [PATCH 0/3] Matthias Kaehlcke 2020-06-05 18:46 ` [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed Matthias Kaehlcke 2020-06-05 18:46 ` [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending Matthias Kaehlcke @ 2020-06-05 18:46 ` Matthias Kaehlcke 2020-06-05 20:46 ` Abhishek Pandit-Subedi 2020-06-08 8:14 ` Marcel Holtmann 2 siblings, 2 replies; 14+ messages in thread From: Matthias Kaehlcke @ 2020-06-05 18:46 UTC (permalink / raw) To: Marcel Holtmann, Johan Hedberg Cc: linux-bluetooth, Rocky Liao, Zijun Hu, linux-kernel, Balakrishna Godavarthi, Abhishek Pandit-Subedi, Claire Chang, Matthias Kaehlcke 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 <mka@chromium.org> --- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] Bluetooth: hci_qca: Refactor error handling in qca_suspend() 2020-06-05 18:46 ` [PATCH 3/3] Bluetooth: hci_qca: Refactor error handling in qca_suspend() Matthias Kaehlcke @ 2020-06-05 20:46 ` Abhishek Pandit-Subedi 2020-06-08 8:14 ` Marcel Holtmann 1 sibling, 0 replies; 14+ messages in thread From: Abhishek Pandit-Subedi @ 2020-06-05 20:46 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Marcel Holtmann, Johan Hedberg, Bluez mailing list, Rocky Liao, Zijun Hu, LKML, Balakrishna Godavarthi, Claire Chang Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> On Fri, Jun 5, 2020 at 11:46 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > 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 <mka@chromium.org> > --- > > 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] Bluetooth: hci_qca: Refactor error handling in qca_suspend() 2020-06-05 18:46 ` [PATCH 3/3] Bluetooth: hci_qca: Refactor error handling in qca_suspend() Matthias Kaehlcke 2020-06-05 20:46 ` Abhishek Pandit-Subedi @ 2020-06-08 8:14 ` Marcel Holtmann 1 sibling, 0 replies; 14+ messages in thread From: Marcel Holtmann @ 2020-06-08 8:14 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Johan Hedberg, linux-bluetooth, Rocky Liao, Zijun Hu, linux-kernel, Balakrishna Godavarthi, Abhishek Pandit-Subedi, Claire Chang 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 <mka@chromium.org> > --- > > drivers/bluetooth/hci_qca.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-06-08 8:14 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-05 18:46 [PATCH 0/3] Matthias Kaehlcke 2020-06-05 18:46 ` [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed Matthias Kaehlcke 2020-06-05 20:44 ` Abhishek Pandit-Subedi 2020-06-06 12:53 ` bgodavar 2020-06-06 15:26 ` Matthias Kaehlcke 2020-06-08 8:11 ` Marcel Holtmann 2020-06-05 18:46 ` [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending Matthias Kaehlcke 2020-06-05 20:45 ` Abhishek Pandit-Subedi 2020-06-06 12:57 ` bgodavar 2020-06-06 14:42 ` Matthias Kaehlcke 2020-06-08 8:13 ` Marcel Holtmann 2020-06-05 18:46 ` [PATCH 3/3] Bluetooth: hci_qca: Refactor error handling in qca_suspend() Matthias Kaehlcke 2020-06-05 20:46 ` Abhishek Pandit-Subedi 2020-06-08 8:14 ` Marcel Holtmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.