| Message ID | 20190116114603.500-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 Wed, Jan 16, 2019 at 05:16:03PM +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> > --- > drivers/bluetooth/hci_qca.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index 7e4afcf40da2..7330ba71ada4 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -1275,6 +1275,14 @@ static const struct qca_vreg_data qca_soc_data = { > > static void qca_power_shutdown(struct hci_uart *hu) > { > + struct qca_data *qca = hu->priv; > + > + /* 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. > + */ > + clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); > + qca_flush(hu); > host_set_baudrate(hu, 2400); > qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE); > qca_power_setup(hu, false); Due to a race-condition there could be an IBS sleep command queued even after clearing the bit and flushing the queue. In qca_enqueue() we have this: static int qca_enqueue(struct hci_uart *hu, struct sk_buff *skb) { ... /* 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); return 0; } spin_lock_irqsave(&qca->hci_ibs_lock, flags); } With process X executing qca_power_shutdown() and process Y running qca_enqueue() this could happen: [X] test_bit(STATE_IN_BAND_SLEEP_ENABLED) => set [Y] clear_bit(STATE_IN_BAND_SLEEP_ENABLED) [Y] qca_flush(hu); [X] skb_queue_tail(&qca->txq, skb); The following should fix this race: --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -770,16 +770,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: @@ -1275,13 +1276,17 @@ 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); Cheers Matthias
On 2019-01-17 04:38, Matthias Kaehlcke wrote: > On Wed, Jan 16, 2019 at 05:16:03PM +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> >> --- >> drivers/bluetooth/hci_qca.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index 7e4afcf40da2..7330ba71ada4 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -1275,6 +1275,14 @@ static const struct qca_vreg_data qca_soc_data >> = { >> >> static void qca_power_shutdown(struct hci_uart *hu) >> { >> + struct qca_data *qca = hu->priv; >> + >> + /* 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. >> + */ >> + clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); >> + qca_flush(hu); >> host_set_baudrate(hu, 2400); >> qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE); >> qca_power_setup(hu, false); > > Due to a race-condition there could be an IBS sleep command queued > even after clearing the bit and flushing the queue. > > In qca_enqueue() we have this: > > static int qca_enqueue(struct hci_uart *hu, struct sk_buff *skb) > { > ... > > /* 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); > return 0; > } > > spin_lock_irqsave(&qca->hci_ibs_lock, flags); > } > > With process X executing qca_power_shutdown() and process Y running > qca_enqueue() this could happen: > > [X] test_bit(STATE_IN_BAND_SLEEP_ENABLED) => set > [Y] clear_bit(STATE_IN_BAND_SLEEP_ENABLED) > [Y] qca_flush(hu); > [X] skb_queue_tail(&qca->txq, skb); > > The following should fix this race: > > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -770,16 +770,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: > @@ -1275,13 +1276,17 @@ 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); > + [Bala]: Thanks for catch this. yes you are rite, we will have an byte queued in the skb. will update with the lock. > host_set_baudrate(hu, 2400); > qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE); > qca_power_setup(hu, false); > > Cheers > > Matthias
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index 7e4afcf40da2..7330ba71ada4 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -1275,6 +1275,14 @@ static const struct qca_vreg_data qca_soc_data = { static void qca_power_shutdown(struct hci_uart *hu) { + struct qca_data *qca = hu->priv; + + /* 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. + */ + clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); + qca_flush(hu); 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> --- drivers/bluetooth/hci_qca.c | 8 ++++++++ 1 file changed, 8 insertions(+)