Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2704-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <0078fcaa-180b-47e9-991a-6f18d920f4b0@126.com>
Date: Sat, 23 Mar 2024 02:06:24 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH BlueZ] avctp: Remove redundant UUID checks and additions
Content-Language: en-GB
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: linux-bluetooth@vger.kernel.org, Xiao Yao <xiaoyao@rock-chips.com>
References: <20240322143830.1476938-1-xiaokeqinhealth@126.com>
 <CABBYNZK64cAFTm7221QxN2T4Eip0=LbpmWUX_bJ3w+O4wAG2UQ@mail.gmail.com>
From: Yao Xiao <xiaokeqinhealth@126.com>
In-Reply-To: <CABBYNZK64cAFTm7221QxN2T4Eip0=LbpmWUX_bJ3w+O4wAG2UQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi,

在 2024/3/22 23:28, Luiz Augusto von Dentz 写道:

> Hi,
>
> On Fri, Mar 22, 2024 at 2:39 PM Xiao Yao <xiaokeqinhealth@126.com> wrote:
>> From: Xiao Yao <xiaoyao@rock-chips.com>
>>
>> ---
>>   profiles/audio/avctp.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
>> index 8ad146df1..6199ee951 100644
>> --- a/profiles/audio/avctp.c
>> +++ b/profiles/audio/avctp.c
>> @@ -1597,12 +1597,6 @@ static void avctp_confirm_cb(GIOChannel *chan, gpointer data)
>>          if (session == NULL)
>>                  return;
>>
>> -       if (btd_device_get_service(device, AVRCP_REMOTE_UUID) == NULL)
>> -               btd_device_add_uuid(device, AVRCP_REMOTE_UUID);
>> -
>> -       if (btd_device_get_service(device, AVRCP_TARGET_UUID) == NULL)
>> -               btd_device_add_uuid(device, AVRCP_TARGET_UUID);
> Patch description makes no sense to me, this code is adding the UUIDs
> of AVCTP if the device over its PSM because at this point perhaps SDP
> discovery is not complete.
In the scenario encountered, the SDP service discovery has already
been completed, with the SDP record only containing AVRCP_REMOTE_UUID.
At this point, the device is added support for AVRCP_TARGET_UUID,
which leads to the following code being executed:
   controller_init(session);
     //avrcp.c:controller_init() 0x55bcdbd690 version 0x0000
     session->controller = controller;
However, avrcp_set_volume relies on session->controller, erroneously
issuing the AVRCP_SET_ABSOLUTE_VOLUME command.

Perhaps add a check to verify that SDP discovery has completed before
adding the UUID?

Additionally, there is another concern: is it appropriate to add support
for both AVRCP_REMOTE_UUID and AVRCP_TARGET_UUID before the completion
of SDP? if one of the UUIDs is not supported,how should it be handled?

>
>>          switch (psm) {
>>          case AVCTP_CONTROL_PSM:
>>                  avctp_control_confirm(session, chan, device);
>> --
>> 2.34.1
>>
>>
>