Re: [PATCH v2 3/3] Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990

[Balakrishna Godavarthi <bgodavar@codeaurora.org>]

Hi Matthias,

On 2018-11-27 05:57, Matthias Kaehlcke wrote:
> On Mon, Nov 26, 2018 at 04:09:50PM -0800, Matthias Kaehlcke wrote:
>> On Thu, Nov 22, 2018 at 05:45:12PM +0530, Balakrishna Godavarthi 
>> wrote:
>> > During initalization of wcn3990, we observed UART is reading some
>> > stray bytes on the Rx line. This is logging Frame reassembly errors
>> > on the serial console. This could be because of tristate of Tx line
>> > of wcn3990 during boot up.
>> >
>> > [  176.929612] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84)
>> > [  176.945734] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84)
>> > [  176.953298] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84)
>> > [  177.010660] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84)
>> > [  177.067633] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84)
>> >
>> > Now we enable a flag during bootup to stop executing proto receive
>> > function and clear it back once the initialization is done.
>> >
>> > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > Tested-by: Matthias Kaehlcke <mka@chromium.org>
>> > ---
>> > v2:
>> >  * Updated commit text & comments.
>> > v1:
>> >  * initial patch
>> > ---
>> >  drivers/bluetooth/hci_qca.c | 18 ++++++++++++++++++
>> >  1 file changed, 18 insertions(+)
>> >
>> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > index ed905ea1c58a..2f3d9e16ba5a 100644
>> > --- a/drivers/bluetooth/hci_qca.c
>> > +++ b/drivers/bluetooth/hci_qca.c
>> > @@ -56,6 +56,7 @@
>> >
>> >  /* Controller states */
>> >  #define STATE_IN_BAND_SLEEP_ENABLED	1
>> > +#define STATE_DISCARD_RX		2
>> >
>> >  #define IBS_WAKE_RETRANS_TIMEOUT_MS	100
>> >  #define IBS_TX_IDLE_TIMEOUT_MS		2000
>> > @@ -511,6 +512,7 @@ static int qca_open(struct hci_uart *hu)
>> >  		} else {
>> >  			hu->init_speed = qcadev->init_speed;
>> >  			hu->oper_speed = qcadev->oper_speed;
>> > +			set_bit(STATE_DISCARD_RX, &qca->flags);
>> >  			ret = qca_power_setup(hu, true);
>> >  			if (ret) {
>> >  				destroy_workqueue(qca->workqueue);
>> > @@ -903,6 +905,13 @@ static int qca_recv(struct hci_uart *hu, const void *data, int count)
>> >  	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
>> >  		return -EUNATCH;
>> >
>> > +	/* We discard Rx data received while device is in booting
>> > +	 * stage, This is because of BT chip Tx line is in tristate.
>> > +	 * Due to this we read some garbage data on UART Rx.
>> > +	 */
>> > +	if (test_bit(STATE_DISCARD_RX, &qca->flags))
>> > +		return 0;
>> > +
>> >  	qca->rx_skb = h4_recv_buf(hu->hdev, qca->rx_skb, data, count,
>> >  				  qca_recv_pkts, ARRAY_SIZE(qca_recv_pkts));
>> >  	if (IS_ERR(qca->rx_skb)) {
>> > @@ -1193,6 +1202,7 @@ static int qca_setup(struct hci_uart *hu)
>> >  		if (ret)
>> >  			return ret;
>> >
>> > +		clear_bit(STATE_DISCARD_RX, &qca->flags);
>> >  		ret = qca_read_soc_version(hdev, &soc_ver);
>> >  		if (ret)
>> >  			return ret;
>> > @@ -1269,6 +1279,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,
>> > +	 * disable IBS and discard all the queued data.
>> > +	 */
>> > +	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> 
>> Is IBS actually related to the frame reassembly errors or is this
>> addressing a different issue? In 100 iteratios of 'hciconfig up/down'
>> without clearing the flag I didn't observe any frame reassembly
>> errors.
> 
> Looks like this addresses the "Bluetooth: Received HCI_IBS_SLEEP_IND
> in rx state 0" messages that are seen when the interface is brought up
> on a system with firmware binaries (with IBS support?). Please spin
> this out into a separate patch.
> 

[Bala]: "Bluetooth: Received HCI_IBS_SLEEP_IND in rx state 0 is an 
different issue.
          i suspect that is due to mismatch of ibs timers over flow value 
between wcn3990 & HOST.
          clearing ibs is required, to stop ibs state machine while we do 
hci close
          this is intern help in resolving the frame reassembly errors 
during close (in rare case observed)
          anyways will send this a separate patch.

> Thanks
> 
> Matthias


-- 
Regards
Balakrishna.