linux-kernel - Re: [PATCH 2/2] hci_qca: Reduce delay after sending baudrate request for WCN3990

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190226202006.GA155681@google.com>
Date:   Tue, 26 Feb 2019 12:20:06 -0800
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>
Cc:     linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
        Balakrishna Godavarthi <bgodavar@...eaurora.org>,
        Hemantg <hemantg@...eaurora.org>
Subject: Re: [PATCH 2/2] hci_qca: Reduce delay after sending baudrate request
 for WCN3990

On Tue, Feb 26, 2019 at 12:08:48PM -0800, Matthias Kaehlcke wrote:
> The current 300ms delay after a baudrate change is extremely long.
> For WCM3990 it is sufficient to wait 10ms after the baudrate change
> request has been sent over the wire.
> 
> Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
> ---
>  drivers/bluetooth/hci_qca.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 703e099515f24..fd018cc5605c6 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -59,8 +59,7 @@
>  
>  #define IBS_WAKE_RETRANS_TIMEOUT_MS	100
>  #define IBS_TX_IDLE_TIMEOUT_MS		2000
> -#define BAUDRATE_SETTLE_TIMEOUT_MS	300
> -#define POWER_PULSE_TRANS_TIMEOUT_MS	100
> +#define CMD_TRANS_TIMEOUT_MS		100
>  
>  /* susclk rate */
>  #define SUSCLK_RATE_32KHZ	32768
> @@ -964,6 +963,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  {
>  	struct hci_uart *hu = hci_get_drvdata(hdev);
>  	struct qca_data *qca = hu->priv;
> +	struct qca_serdev *qcadev;
>  	struct sk_buff *skb;
>  	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
>  
> @@ -985,11 +985,25 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  	skb_queue_tail(&qca->txq, skb);
>  	hci_uart_tx_wakeup(hu);
>  
> -	/* wait 300ms to change new baudrate on controller side
> -	 * controller will come back after they receive this HCI command
> -	 * then host can communicate with new baudrate to controller
> +	qcadev = serdev_device_get_drvdata(hu->serdev);
> +
> +	/* Wait for the baudrate change command to be sent and
> +	 * processed by the controller.
>  	 */
> -	msleep(BAUDRATE_SETTLE_TIMEOUT_MS);
> +	if (qcadev->btsoc_type == QCA_WCN3990) {
> +		while (!skb_queue_empty(&qca->txq))
> +			usleep_range(100, 200);
> +
> +		serdev_device_wait_until_sent(hu->serdev,
> +			      msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));

note: the above could also be in the common path, personally I don't
have a strong preference.

> +
> +		/* Make sure there is enough time for the BT
> +		 * controller to process the baudrate change
> +		*/
> +		msleep(10);
> +	} else {
> +		msleep(300);

I wonder if the non-wcn3990 delay could be reduced too, 300ms seems an
eternity, however I don't have hardware to validate such a change.

That raises a question: how many different chips are actually
supported by this driver? In earlier reviews we were frequently
talking about the ROME family and I assumed it must be a larger
number, however there are only compatible strings for the qca6174 and
the wcn3990. If there is only one other controller (or a low number
of very similar ones that use the same compatible string?) maybe
Qualcomm could help with testing?

Thanks

Matthias