linux-kernel - Re: [PATCH v1] Bluetooth: hci_qca: Update baudrate change wait time 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] [day] [month] [year] [list]

Message-ID: <20190225235620.GA67723@google.com>
Date:   Mon, 25 Feb 2019 15:56:20 -0800
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Balakrishna Godavarthi <bgodavar@...eaurora.org>
Cc:     marcel@...tmann.org, johan.hedberg@...il.com,
        linux-kernel@...r.kernel.org, linux-bluetooth@...r.kernel.org,
        hemantg@...eaurora.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v1] Bluetooth: hci_qca: Update baudrate change wait time
 for wcn3990

Hi,

On Wed, Feb 20, 2019 at 03:43:02PM -0800, Matthias Kaehlcke wrote:
> Hi Balakrishna,
> 
> On Wed, Feb 20, 2019 at 04:55:16PM +0530, Balakrishna Godavarthi wrote:
> > This patch will update the baudrate change request wait time from
> > 300 ms to 100 ms. When host sends the change baudrate request to
> > the controller, controller sets its clock and wait until the
> > clocks settle down. Here the Wait time is required for both
> > host and controller to be on sync.
> > 
> > Signed-off-by: Balakrishna Godavarthi <bgodavar@...eaurora.org>
> > ---
> >  drivers/bluetooth/hci_qca.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index 5e03504c4e0c..22f3c983f868 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -59,7 +59,8 @@
> >  
> >  #define IBS_WAKE_RETRANS_TIMEOUT_MS	100
> >  #define IBS_TX_IDLE_TIMEOUT_MS		2000
> > -#define BAUDRATE_SETTLE_TIMEOUT_MS	300
> > +#define ROME_BD_SETTLE_TIMEOUT_MS	300
> > +#define WCN3990_BD_SETTLE_TIMEOUT_MS	100
> 
> nit: _BR_ instead of _BD_?
> 
> >  #define POWER_PULSE_TRANS_TIMEOUT_MS	100
> >  
> >  /* susclk rate */
> > @@ -965,8 +966,11 @@ 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 sk_buff *skb;
> > +	struct qca_serdev *qcadev;
> > +	unsigned int bd_settling_timeout;
> 
> nit: from the context the purpose of the variable is fairly clear,
> calling it just 'timeout' (or 'settling_time', it's not really a
> timeout) should be fine.
> 
> That said, I have a similar change in my pipeline, which further
> reduces the time to the 'strictly necessary'. It's slightly more code
> though. I guess I'll send it and we can discuss/let Marcel decide
> what to adopt.

I haven't sent my patches yet since I encountered initialization
errors when testing, however it turns out these errors are not
related with my changes.

This series fixes the problem:

https://lore.kernel.org/patchwork/project/lkml/list/?series=384571

I'm now retesting my changes and will send them soon unless I
encounter other issues.

Thanks

Matthias