| Message ID | 20190124120808.8275-4-bgodavar@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | Bug fixes for Qualcomm BT chip wcn3990. | expand |
On Thu, Jan 24, 2019 at 05:38:08PM +0530, Balakrishna Godavarthi wrote: > During hci down we observed IBS sleep commands are queued in the Tx > buffer and hci_uart_write_work is sending data to the chip which is > not required as the chip is powered off. This patch will disable IBS > and flush the Tx buffer before we turn off the chip. > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> > --- > Changes v9: > * added lock while disabling the IBS state machine. > > --- > drivers/bluetooth/hci_qca.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index 6b5bcd44e24c..99ddc35f08c6 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -771,16 +771,17 @@ static int qca_enqueue(struct hci_uart *hu, struct sk_buff *skb) > /* Prepend skb with frame type */ > memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > > + spin_lock_irqsave(&qca->hci_ibs_lock, flags); > + > /* Don't go to sleep in middle of patch download or > * Out-Of-Band(GPIOs control) sleep is selected. > */ > if (!test_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags)) { > skb_queue_tail(&qca->txq, skb); > + spin_unlock_irqrestore(&qca->hci_ibs_lock, flags); > return 0; > } > > - spin_lock_irqsave(&qca->hci_ibs_lock, flags); > - > /* Act according to current state */ > switch (qca->tx_ibs_state) { > case HCI_IBS_TX_AWAKE: > @@ -1273,6 +1274,18 @@ static const struct qca_vreg_data qca_soc_data = { > > static void qca_power_shutdown(struct hci_uart *hu) > { > + struct qca_data *qca = hu->priv; > + unsigned long flags; > + > + /* From this point we go into power off state. But serial port is > + * still open, stop queueing the IBS data and flush all the buffered > + * data in skb's. > + */ > + spin_lock_irqsave(&qca->hci_ibs_lock, flags); > + clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); > + qca_flush(hu); > + spin_unlock_irqrestore(&qca->hci_ibs_lock, flags); > + > host_set_baudrate(hu, 2400); > qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE); > qca_power_setup(hu, false); I was about to add my 'Reviewed-by' tag, but I'm still left with a doubt. This patch certainly improves the situation by clearing the IBS bit and flushing the buffered data, however IIUC new data could still be added to the TX queue after releasing the spinlock: static int qca_enqueue(struct hci_uart *hu, struct sk_buff *skb) { ... if (!test_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags)) { skb_queue_tail(&qca->txq, skb); ... } To prevent this a boolean/bit like 'shutting_down' or similar would be needed (I don't think there is something common in the HCI core), which is set in qca_power_shutdown(). If the bit is set qca_enqueue() discards the data. Not sure how important this is, and I don't want to add necessarily more revisions to this series. If it is preferable to have an empty queue after shutdown maybe it can be done in a follow up patch. Thanks Matthias
Hi Matthias, On 2019-01-25 06:25, Matthias Kaehlcke wrote: > On Thu, Jan 24, 2019 at 05:38:08PM +0530, Balakrishna Godavarthi wrote: >> During hci down we observed IBS sleep commands are queued in the Tx >> buffer and hci_uart_write_work is sending data to the chip which is >> not required as the chip is powered off. This patch will disable IBS >> and flush the Tx buffer before we turn off the chip. >> >> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> >> --- >> Changes v9: >> * added lock while disabling the IBS state machine. >> >> --- >> drivers/bluetooth/hci_qca.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index 6b5bcd44e24c..99ddc35f08c6 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -771,16 +771,17 @@ static int qca_enqueue(struct hci_uart *hu, >> struct sk_buff *skb) >> /* Prepend skb with frame type */ >> memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); >> >> + spin_lock_irqsave(&qca->hci_ibs_lock, flags); >> + >> /* Don't go to sleep in middle of patch download or >> * Out-Of-Band(GPIOs control) sleep is selected. >> */ >> if (!test_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags)) { >> skb_queue_tail(&qca->txq, skb); >> + spin_unlock_irqrestore(&qca->hci_ibs_lock, flags); >> return 0; >> } >> >> - spin_lock_irqsave(&qca->hci_ibs_lock, flags); >> - >> /* Act according to current state */ >> switch (qca->tx_ibs_state) { >> case HCI_IBS_TX_AWAKE: >> @@ -1273,6 +1274,18 @@ static const struct qca_vreg_data qca_soc_data >> = { >> >> static void qca_power_shutdown(struct hci_uart *hu) >> { >> + struct qca_data *qca = hu->priv; >> + unsigned long flags; >> + >> + /* From this point we go into power off state. But serial port is >> + * still open, stop queueing the IBS data and flush all the buffered >> + * data in skb's. >> + */ >> + spin_lock_irqsave(&qca->hci_ibs_lock, flags); >> + clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); >> + qca_flush(hu); >> + spin_unlock_irqrestore(&qca->hci_ibs_lock, flags); >> + >> host_set_baudrate(hu, 2400); >> qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE); >> qca_power_setup(hu, false); > > I was about to add my 'Reviewed-by' tag, but I'm still left with a > doubt. This patch certainly improves the situation by clearing the IBS > bit and flushing the buffered data, however IIUC new data could still > be added to the TX queue after releasing the spinlock: > > static int qca_enqueue(struct hci_uart *hu, struct sk_buff *skb) > { > ... > > if (!test_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags)) { > skb_queue_tail(&qca->txq, skb); > > ... > } > > To prevent this a boolean/bit like 'shutting_down' or similar would be > needed (I don't think there is something common in the HCI core), > which is set in qca_power_shutdown(). If the bit is set qca_enqueue() > discards the data. > > Not sure how important this is, and I don't want to add necessarily > more revisions to this series. If it is preferable to have an empty > queue after shutdown maybe it can be done in a follow up patch. > > Thanks > > Matthias [Bala]: during shutdown Bt stack will not send any data to the Bt kernel Tx path. so this call may not be called. once we shutdown chip.
On Fri, Feb 01, 2019 at 05:10:11PM +0530, Balakrishna Godavarthi wrote: > Hi Matthias, > > On 2019-01-25 06:25, Matthias Kaehlcke wrote: > > On Thu, Jan 24, 2019 at 05:38:08PM +0530, Balakrishna Godavarthi wrote: > > > During hci down we observed IBS sleep commands are queued in the Tx > > > buffer and hci_uart_write_work is sending data to the chip which is > > > not required as the chip is powered off. This patch will disable IBS > > > and flush the Tx buffer before we turn off the chip. > > > > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> > > > --- > > > Changes v9: > > > * added lock while disabling the IBS state machine. > > > > > > --- > > > drivers/bluetooth/hci_qca.c | 17 +++++++++++++++-- > > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > > > index 6b5bcd44e24c..99ddc35f08c6 100644 > > > --- a/drivers/bluetooth/hci_qca.c > > > +++ b/drivers/bluetooth/hci_qca.c > > > @@ -771,16 +771,17 @@ static int qca_enqueue(struct hci_uart *hu, > > > struct sk_buff *skb) > > > /* Prepend skb with frame type */ > > > memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > > > > > > + spin_lock_irqsave(&qca->hci_ibs_lock, flags); > > > + > > > /* Don't go to sleep in middle of patch download or > > > * Out-Of-Band(GPIOs control) sleep is selected. > > > */ > > > if (!test_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags)) { > > > skb_queue_tail(&qca->txq, skb); > > > + spin_unlock_irqrestore(&qca->hci_ibs_lock, flags); > > > return 0; > > > } > > > > > > - spin_lock_irqsave(&qca->hci_ibs_lock, flags); > > > - > > > /* Act according to current state */ > > > switch (qca->tx_ibs_state) { > > > case HCI_IBS_TX_AWAKE: > > > @@ -1273,6 +1274,18 @@ static const struct qca_vreg_data > > > qca_soc_data = { > > > > > > static void qca_power_shutdown(struct hci_uart *hu) > > > { > > > + struct qca_data *qca = hu->priv; > > > + unsigned long flags; > > > + > > > + /* From this point we go into power off state. But serial port is > > > + * still open, stop queueing the IBS data and flush all the buffered > > > + * data in skb's. > > > + */ > > > + spin_lock_irqsave(&qca->hci_ibs_lock, flags); > > > + clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); > > > + qca_flush(hu); > > > + spin_unlock_irqrestore(&qca->hci_ibs_lock, flags); > > > + > > > host_set_baudrate(hu, 2400); > > > qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE); > > > qca_power_setup(hu, false); > > > > I was about to add my 'Reviewed-by' tag, but I'm still left with a > > doubt. This patch certainly improves the situation by clearing the IBS > > bit and flushing the buffered data, however IIUC new data could still > > be added to the TX queue after releasing the spinlock: > > > > static int qca_enqueue(struct hci_uart *hu, struct sk_buff *skb) > > { > > ... > > > > if (!test_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags)) { > > skb_queue_tail(&qca->txq, skb); > > > > ... > > } > > > > To prevent this a boolean/bit like 'shutting_down' or similar would be > > needed (I don't think there is something common in the HCI core), > > which is set in qca_power_shutdown(). If the bit is set qca_enqueue() > > discards the data. > > > > Not sure how important this is, and I don't want to add necessarily > > more revisions to this series. If it is preferable to have an empty > > queue after shutdown maybe it can be done in a follow up patch. > > > > Thanks > > > > Matthias > > [Bala]: during shutdown Bt stack will not send any data to the Bt kernel Tx > path. > so this call may not be called. once we shutdown chip. Thanks for the clarification! I kinda expected that, but it wasn't evident to me from the core code. Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index 6b5bcd44e24c..99ddc35f08c6 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -771,16 +771,17 @@ static int qca_enqueue(struct hci_uart *hu, struct sk_buff *skb) /* Prepend skb with frame type */ memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); + spin_lock_irqsave(&qca->hci_ibs_lock, flags); + /* Don't go to sleep in middle of patch download or * Out-Of-Band(GPIOs control) sleep is selected. */ if (!test_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags)) { skb_queue_tail(&qca->txq, skb); + spin_unlock_irqrestore(&qca->hci_ibs_lock, flags); return 0; } - spin_lock_irqsave(&qca->hci_ibs_lock, flags); - /* Act according to current state */ switch (qca->tx_ibs_state) { case HCI_IBS_TX_AWAKE: @@ -1273,6 +1274,18 @@ static const struct qca_vreg_data qca_soc_data = { static void qca_power_shutdown(struct hci_uart *hu) { + struct qca_data *qca = hu->priv; + unsigned long flags; + + /* From this point we go into power off state. But serial port is + * still open, stop queueing the IBS data and flush all the buffered + * data in skb's. + */ + spin_lock_irqsave(&qca->hci_ibs_lock, flags); + clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); + qca_flush(hu); + spin_unlock_irqrestore(&qca->hci_ibs_lock, flags); + host_set_baudrate(hu, 2400); qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE); qca_power_setup(hu, false);
During hci down we observed IBS sleep commands are queued in the Tx buffer and hci_uart_write_work is sending data to the chip which is not required as the chip is powered off. This patch will disable IBS and flush the Tx buffer before we turn off the chip. Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> --- Changes v9: * added lock while disabling the IBS state machine. --- drivers/bluetooth/hci_qca.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)