Date:   Mon, 18 Jul 2022 16:59:12 -0700
From:   Luiz Augusto von Dentz <luiz.dentz@...il.com>
To:     quic_zijuhu <quic_zijuhu@...cinc.com>
Cc:     Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Luiz Augusto Von Dentz <luiz.von.dentz@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>
Subject: Re: [PATCH v1] Bluetooth: Fix cvsd sco setup failure

Hi Quic_zijuhu,

On Thu, Jul 14, 2022 at 10:25 PM quic_zijuhu <quic_zijuhu@...cinc.com> wrote:
>
> On 7/15/2022 12:21 PM, Luiz Augusto von Dentz wrote:
> > Hi,
> >
> > On Thu, Jul 14, 2022 at 8:31 PM quic_zijuhu <quic_zijuhu@...cinc.com> wrote:
> >>
> >> On 7/15/2022 5:24 AM, Luiz Augusto von Dentz wrote:
> >>> Hi Zijun,
> >>>
> >>> On Thu, Jul 14, 2022 at 12:14 AM Zijun Hu <quic_zijuhu@...cinc.com> wrote:
> >>>>
> >>>> A cvsd sco setup failure issue is reported as shown by
> >>>> below btmon log, it firstly tries to set up cvsd esco with
> >>>> S3/S2/S1 configs sequentially, but these attempts are all
> >>>> failed with error code "Unspecified Error (0x1f)", then it
> >>>> tries to set up cvsd sco with D1 config, unfortunately, it
> >>>> still fails to set up sco with error code
> >>>> "Invalid HCI Command Parameters (0x12)", this error code
> >>>> terminates attempt with remaining D0 config and marks overall
> >>>> sco/esco setup failure.
> >>>>
> >>>> It is wrong D1/D0 @retrans_effort 0x01 within @esco_param_cvsd
> >>>> that causes D1 config failure with error code
> >>>> "Invalid HCI Command Parameters (0x12)", D1/D0 sco @retrans_effort
> >>>> must not be 0x01 based on spec, so fix this issue by changing D1/D0
> >>>> @retrans_effort from 0x01 to 0xff as present @sco_param_cvsd.
> >>>
> >>> Please quote the spec regarding the invalid parameters:
> >>>
> >> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 1, Part F
> >> page 375
> >>
> >> 2.18 INVALID HCI COMMAND PARAMETERS (0x12)
> >> The Invalid HCI Command Parameters error code indicates that at least one of
> >> the HCI command parameters is invalid.
> >> This shall be used when:
> >> • the parameter total length is invalid.
> >> • a command parameter is an invalid type.
> >> • a connection identifier does not match the corresponding event.
> >> • a parameter is odd when it is required to be even.
> >> • a parameter is outside of the specified range.
> >> • two or more parameter values have inconsistent values.
> >> Note: An invalid type can be, for example, when a SCO Connection_Handle is
> >> used where an ACL Connection_Handle is required.
> >>
> >>> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
> >>> page 1891
> >>>
> >>> 0x01 At least one retransmission, optimize for power consumption (eSCO con-
> >>> nection required).
> >>>
> >>>> < HCI Command: Setup Synchrono.. (0x01|0x0028) plen 17  #3405 [hci0]
> >>>>         Handle: 3
> >>>>         Transmit bandwidth: 8000
> >>>>         Receive bandwidth: 8000
> >>>>         Max latency: 10
> >>>>         Setting: 0x0060
> >>>>           Input Coding: Linear
> >>>>           Input Data Format: 2's complement
> >>>>           Input Sample Size: 16-bit
> >>>>           # of bits padding at MSB: 0
> >>>>           Air Coding Format: CVSD
> >>>>         Retransmission effort: Optimize for power consumption (0x01)
> >>>>         Packet type: 0x0380
> >>>>           3-EV3 may not be used
> >>>>           2-EV5 may not be used
> >>>>           3-EV5 may not be used
> >>>>> HCI Event: Command Status (0x0f) plen 4               #3406 [hci0]
> >>>>       Setup Synchronous Connection (0x01|0x0028) ncmd 1
> >>>>         Status: Success (0x00)
> >>>>> HCI Event: Synchronous Connect Comp.. (0x2c) plen 17  #3408 [hci0]
> >>>>         Status: Unspecified Error (0x1f)
> >>>>         Handle: 4
> >>>>         Address: 14:3F:A6:47:56:15 (OUI 14-3F-A6)
> >>>>         Link type: eSCO (0x02)
> >>>>         Transmission interval: 0x00
> >>>>         Retransmission window: 0x00
> >>>>         RX packet length: 0
> >>>>         TX packet length: 0
> >>>>         Air mode: CVSD (0x02)
> >>>> < HCI Command: Setup Synchrono.. (0x01|0x0028) plen 17  #3409 [hci0]
> >>>>         Handle: 3
> >>>>         Transmit bandwidth: 8000
> >>>>         Receive bandwidth: 8000
> >>>>         Max latency: 7
> >>>>         Setting: 0x0060
> >>>>           Input Coding: Linear
> >>>>           Input Data Format: 2's complement
> >>>>           Input Sample Size: 16-bit
> >>>>           # of bits padding at MSB: 0
> >>>>           Air Coding Format: CVSD
> >>>>         Retransmission effort: Optimize for power consumption (0x01)
> >>>>         Packet type: 0x0380
> >>>>           3-EV3 may not be used
> >>>>           2-EV5 may not be used
> >>>>           3-EV5 may not be used
> >>>>> HCI Event: Command Status (0x0f) plen 4               #3410 [hci0]
> >>>>       Setup Synchronous Connection (0x01|0x0028) ncmd 1
> >>>>         Status: Success (0x00)
> >>>>> HCI Event: Synchronous Connect Comp.. (0x2c) plen 17  #3416 [hci0]
> >>>>         Status: Unspecified Error (0x1f)
> >>>>         Handle: 4
> >>>>         Address: 14:3F:A6:47:56:15 (OUI 14-3F-A6)
> >>>>         Link type: eSCO (0x02)
> >>>>         Transmission interval: 0x00
> >>>>         Retransmission window: 0x00
> >>>>         RX packet length: 0
> >>>>         TX packet length: 0
> >>>>         Air mode: CVSD (0x02)
> >>>> < HCI Command: Setup Synchrono.. (0x01|0x0028) plen 17  #3417 [hci0]
> >>>>         Handle: 3
> >>>>         Transmit bandwidth: 8000
> >>>>         Receive bandwidth: 8000
> >>>>         Max latency: 7
> >>>>         Setting: 0x0060
> >>>>           Input Coding: Linear
> >>>>           Input Data Format: 2's complement
> >>>>           Input Sample Size: 16-bit
> >>>>           # of bits padding at MSB: 0
> >>>>           Air Coding Format: CVSD
> >>>>         Retransmission effort: Optimize for power consumption (0x01)
> >>>>         Packet type: 0x03c8
> >>>>           EV3 may be used
> >>>>           2-EV3 may not be used
> >>>>           3-EV3 may not be used
> >>>>           2-EV5 may not be used
> >>>>           3-EV5 may not be used
> >>>>> HCI Event: Command Status (0x0f) plen 4               #3419 [hci0]
> >>>>       Setup Synchronous Connection (0x01|0x0028) ncmd 1
> >>>>         Status: Success (0x00)
> >>>>> HCI Event: Synchronous Connect Comp.. (0x2c) plen 17  #3426 [hci0]
> >>>>         Status: Unspecified Error (0x1f)
> >>>>         Handle: 4
> >>>>         Address: 14:3F:A6:47:56:15 (OUI 14-3F-A6)
> >>>>         Link type: eSCO (0x02)
> >>>>         Transmission interval: 0x00
> >>>>         Retransmission window: 0x00
> >>>>         RX packet length: 0
> >>>>         TX packet length: 0
> >>>>         Air mode: CVSD (0x02)
> >>>> < HCI Command: Setup Synchrono.. (0x01|0x0028) plen 17  #3427 [hci0]
> >>>>         Handle: 3
> >>>>         Transmit bandwidth: 8000
> >>>>         Receive bandwidth: 8000
> >>>>         Max latency: 65535
> >>>>         Setting: 0x0060
> >>>>           Input Coding: Linear
> >>>>           Input Data Format: 2's complement
> >>>>           Input Sample Size: 16-bit
> >>>>           # of bits padding at MSB: 0
> >>>>           Air Coding Format: CVSD
> >>>>         Retransmission effort: Optimize for power consumption (0x01)
> >>>>         Packet type: 0x03c4
> >>>>           HV3 may be used
> >>>>           2-EV3 may not be used
> >>>>           3-EV3 may not be used
> >>>>           2-EV5 may not be used
> >>>>           3-EV5 may not be used
> >>>>> HCI Event: Command Status (0x0f) plen 4               #3428 [hci0]
> >>>>       Setup Synchronous Connection (0x01|0x0028) ncmd 1
> >>>>         Status: Success (0x00)
> >>>>> HCI Event: Synchronous Connect Comp.. (0x2c) plen 17  #3429 [hci0]
> >>>>         Status: Invalid HCI Command Parameters (0x12)
> >>>>         Handle: 0
> >>>>         Address: 14:3F:A6:47:56:15 (OUI 14-3F-A6)
> >>>>         Link type: SCO (0x00)
> >>>>         Transmission interval: 0x00
> >>>>         Retransmission window: 0x00
> >>>>         RX packet length: 0
> >>>>         TX packet length: 0
> >>>>         Air mode: u-law log (0x00)
> >>>
> >>> This really sounds like the controller fault, it seem to be picking up
> >>> SCO based on packet type alone instead of checking if retransmission
> >>> is suggesting to use eSCO instead, otherwise there is no use to define
> >>> D1/D0 for both eSCO and SCO since the controller will always pick SCO
> >>> instead.
> >>>
> >> i don't agree with you about above opinion:
> >> S3/S2/S1 here is for eSCO but D1/D0 is for SCO, it should try to set up
> >> SCO after all eSCO setup failures based HFP_v1.8 spec, so it is reasonable to
> >> return "Invalid HCI Command Parameters" for SCO setup with retransmission parameter
> >> 0x01 since SCO doesn't need retransmission.
> >>
> >> the spec doesn't say it is available for D1/D0 on eSCO.
> >>
> >> Hands-Free Profile V1.8 | page 133
> >>
> >> 5.7.1.1 Selection of Synchronous Transport
> >> To select the type of synchronous transport (eSCO or SCO) to use, devices shall adhere to the following
> >> logic:
> >> • If eSCO is supported by the responder, the synchronous connection shall first be attempted on an
> >> eSCO logical transport. See section 5.7.1.2
> >> • If eSCO is unavailable for use (e.g., not supported by the Responder or link establishment fails),
> >> and SCO is not currently forbidden because a BR/EDR secure connection is being used, the
> >> Initiator shall open a SCO logical connection. See section 5.7.1.3.
> >>
> >> Hands-Free Profile V1.8 | page 115
> >> 5.7.1.3 Negotiation of SCO Configuration Parameters
> >> Requirements related to the use of SCO links, under the conditions when the use of a SCO logical
> >> transport is permitted, are covered by the parameter sets D0-D1.
> >>
> >> Hands-Free Profile V1.8 | page 24
> >> shows a summary of the mapping of codec requirements on link features for this profile.
> >> Feature Support in HF Support in AG
> >> 1. D0 – CVSD on SCO link (HV1) M M
> >> 2. D1 – CVSD on SCO link (HV3) M M
> >> 3. S1 – CVSD eSCO link (EV3) M M
> >> 4. S2 – CVSD on EDR eSCO link (2-EV3) M M
> >> 5. S3 – CVSD on EDR eSCO link (2-EV3) M M
> >
> > If D0-D1 are SCO only, then yes but then they should not even be part
> > of the eSCO table, still I don't think the controller should be
> > looking just to packet type or these types are not supported in eSCO?
> >
> Per BT 5.3 errata, BT controller will not retry SCO connection if eSCO connection failure. Instead, host shall retry sco connection with retransmission effort =0
> you are right in theory, but it maybe be the simplest fix to correct D1/D0 retransmission parameter within the table in practice
> the table esco_param_cvsd maybe be regarded as eSCO entry table.
>
> the failure reason is shown below based on F/W team explanation:
> we set retransmission parameter with 0x01 to request controller to setup eSCO but the peer only accept SCO within
> suggested retransmission parameter is 0x00.

I guess it must be some LL message then since I don't see anything in
the traces that would indicate the peer only accept SCO within
suggested retransmission.

> so the failure reason is "choice between SCO and eSCO" not "packet type is not supported over eSCO"
>
> >>>> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
> >>>> Tested-by: Zijun Hu <quic_zijuhu@...cinc.com>
> >>>> ---
> >>>>  net/bluetooth/hci_conn.c | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> >>>> index 7829433d54c1..2627d5ac15d6 100644
> >>>> --- a/net/bluetooth/hci_conn.c
> >>>> +++ b/net/bluetooth/hci_conn.c
> >>>> @@ -45,8 +45,8 @@ static const struct sco_param esco_param_cvsd[] = {
> >>>>         { EDR_ESCO_MASK & ~ESCO_2EV3, 0x000a,   0x01 }, /* S3 */
> >>>>         { EDR_ESCO_MASK & ~ESCO_2EV3, 0x0007,   0x01 }, /* S2 */
> >>>>         { EDR_ESCO_MASK | ESCO_EV3,   0x0007,   0x01 }, /* S1 */
> >>>> -       { EDR_ESCO_MASK | ESCO_HV3,   0xffff,   0x01 }, /* D1 */
> >>>> -       { EDR_ESCO_MASK | ESCO_HV1,   0xffff,   0x01 }, /* D0 */
> >>>> +       { EDR_ESCO_MASK | ESCO_HV3,   0xffff,   0xff }, /* D1 */
> >>>> +       { EDR_ESCO_MASK | ESCO_HV1,   0xffff,   0xff }, /* D0 */
> >>>>  };
> >>>
> >>> This doesn't seem right, you are changing the parameters for eSCO
> >>> table not SCO, which further reinforce this is probably the controller
> >>> not really doing its job and checking if retransmission is actually
> >>> meant for eSCO rather than SCO.
> >>>
> >> here it is D1/D0 SCO setup after S3/S2/S1 eSCO attempts failure as above my comments.
> >
> > Well then all we need to do is to remove the last 2 lines since they
> > are already handled by the table below.
> >
>   if so, we must handle the fallback requirement with more complex logic.

Yeah it sounds like we didn't think about adding a fallback but
perhaps it is as simple as changing find_next_esco_param to start
returning the entry itself instead of an index, that would IMO have
made this issue more visible.

> >>>>  static const struct sco_param sco_param_cvsd[] = {
> >>>> --
> >>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
> >>>>
> >>>
> >>>
> >>
> >
> >
>


-- 
Luiz Augusto von Dentz

Powered by blists - more mailing lists