Re: [PATCH v1] Bluetooth: Fix cvsd sco setup failure

From: quic_zijuhu
Date: Mon Jul 18 2022 - 23:37:26 EST

On 7/19/2022 7:59 AM, Luiz Augusto von Dentz wrote:
> Hi Quic_zijuhu,
>
> On Thu, Jul 14, 2022 at 10:25 PM quic_zijuhu <quic_zijuhu@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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.
>
yes, your are right.
so i need to remove D1/D0 from @esco_param_cvsd based on discussion.
right?
>> 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@xxxxxxxxxxx>
>>>>>> Tested-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>>>>>> ---
>>>>>> 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.
>
we maybe take the fallback requirements as another requirement to develop.
it is a good idea to have fallback you suggest.
>>>>>> 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
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>