From mboxrd@z Thu Jan 1 00:00:00 1970 From: Balakrishna Godavarthi Subject: Re: [PATCH v9 3/3] Bluetooth: hci_qca: Disable IBS state machine and flush Tx buffer Date: Fri, 01 Feb 2019 17:10:11 +0530 Message-ID: <75923da910c43e7e5e033f4ff4360a90@codeaurora.org> References: <20190124120808.8275-1-bgodavar@codeaurora.org> <20190124120808.8275-4-bgodavar@codeaurora.org> <20190125005534.GC81583@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20190125005534.GC81583@google.com> Sender: linux-kernel-owner@vger.kernel.org To: Matthias Kaehlcke Cc: marcel@holtmann.org, johan.hedberg@gmail.com, johan@kernel.org, linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org, hemantg@codeaurora.org, linux-arm-msm@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org 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 >> --- >> 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. -- Regards Balakrishna.