Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
From: Matthias Kaehlcke <mka@chromium.org>
To: Balakrishna Godavarthi <bgodavar@codeaurora.org>
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
Subject: Re: [PATCH v9 3/3] Bluetooth: hci_qca: Disable IBS state machine and flush Tx buffer
Date: Thu, 24 Jan 2019 16:55:34 -0800
Message-ID: <20190125005534.GC81583@google.com> (raw)
In-Reply-To: <20190124120808.8275-4-bgodavar@codeaurora.org>

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

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-24 12:08 [PATCH v9 0/3] Bug fixes for Qualcomm BT chip wcn3990 Balakrishna Godavarthi
2019-01-24 12:08 ` [PATCH v9 1/3] Bluetooth: hci_qca: use wait_until_sent() for power pulses Balakrishna Godavarthi
2019-01-25  1:14   ` Matthias Kaehlcke
2019-01-28 13:49     ` Balakrishna Godavarthi
2019-01-28 17:47       ` Matthias Kaehlcke
2019-01-29 13:08         ` Balakrishna Godavarthi
2019-01-24 12:08 ` [PATCH v9 2/3] Bluetooth: hci_qca: Deassert RTS while baudrate change command Balakrishna Godavarthi
2019-01-25  0:12   ` Matthias Kaehlcke
2019-01-28 13:45     ` Balakrishna Godavarthi
2019-01-24 12:08 ` [PATCH v9 3/3] Bluetooth: hci_qca: Disable IBS state machine and flush Tx buffer Balakrishna Godavarthi
2019-01-25  0:55   ` Matthias Kaehlcke [this message]
2019-02-01 11:40     ` Balakrishna Godavarthi
2019-02-01 18:35       ` Matthias Kaehlcke

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190125005534.GC81583@google.com \
    --to=mka@chromium.org \
    --cc=bgodavar@codeaurora.org \
    --cc=hemantg@codeaurora.org \
    --cc=johan.hedberg@gmail.com \
    --cc=johan@kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marcel@holtmann.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org linux-bluetooth@archiver.kernel.org
	public-inbox-index linux-bluetooth


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/ public-inbox