Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200118194841.439036-1-marijns95@gmail.com>
In-Reply-To: <20200118194841.439036-1-marijns95@gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 21 Jan 2020 14:48:20 -0800
Message-ID: <CABBYNZ+LW-Lp3q9jOjVwFktuLsKajTrmOpUYmQ5SqCUPrGQ7BA@mail.gmail.com>
Subject: Re: [BlueZ PATCH] audio: avrcp: Ignore peer RT supported_events when
 being the RT.
To:     Marijn Suijten <marijns95@gmail.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Luiz Augusto Von Dentz <luiz.von.dentz@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Marijn,

On Sat, Jan 18, 2020 at 11:52 AM Marijn Suijten <marijns95@gmail.com> wrote:
>
> From: Marijn Suijten <marijns95@gmail.com>
>
> Remove the check of a received event against supported_events in
> avrcp_handle_register_notification, which is only called when BlueZ is
> the RT even though supported_events is only valid when BlueZ is the TG.
>
> supported_events is assigned in target_init with events that the
> corresponding RT on the other side of the Bluetooth connection supports,
> to ensure the local TG will never report anything unexpected in
> avrcp_handle_get_capabilities. This value is specific to what the target
> should support to be compatible with the peer RT, but a locally running
> RT has nothing to do with the external device being the RT.
>
> This addresses the case where Absolute Volume notification registrations
> are rejected when audio is played from an Android device to a computer
> running BlueZ. The Android BT stack report an RT of version 1.3 [1]
> which does not include Absolute Volume support. The RT on the Android
> device is not involved in such a playback session, instead the computer
> is the RT and the Android device the TG.
>
> This has been tested by disabling registration of the RT service in
> Android, to make the device a "pure" media player that cannot receive
> audio: target_init does not get called and supported_events stays 0
> which would have caused any notification registration to be rejected in
> the above case.

I assume you have a typo on RT instead of CT, is that right? From
qualification point of view anything initiated by a device is
considered a CT role, much like GATT server and client roles, so we
may have instances where both CT and TG are supported simultaneously.

> [1]: https://android.googlesource.com/platform/system/bt/+/android-10.0.0_r25/bta/av/bta_av_main.cc#712
> ---
> Hi,
>
> I have a separate patch lying around that - instead of removing
> supported_events - splits it up in two variables; one for the target and
> another for the controller. Let me know if this extra safety is desired.
>
> According to the AVRCP 1.3 specification GetCapabilities is mandatory,
> which I have included in that patch. However the documentation also
> mentions that this function is only supposed to be called by the CT
> meaning that the call in target_init (introduced in 27efc30f0) is not
> valid. What is your view on this?
> Unfortunately even the small pair of in-ears I have lying around report
> AVRCP TG functionality while they are not nearly capable of being a
> target/media-source, so I have not been able to confirm how a pure RT
> device would respond in such case.

As I mentioned above the qualification tests requires both TG and CT
for things like absolute volume to work as notifications for volume
changes originate from the device rendering the audio/sink not the
source, so the typical association of sink/CT, source/TG is no longer
true and before you ask, yes we have some code and comments leading to
that assumption which we should probably fix at some point so I guess
having the supported events in 2 is probably a good idea, though
notice that it should probably be local and remote events since event
afaik are always originated from the TG role.

>
> - Marijn Suijten
>
>  profiles/audio/avrcp.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 7fb8af608..820494d2a 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -1529,10 +1529,6 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
>         if (len != 5)
>                 goto err;
>
> -       /* Check if event is supported otherwise reject */
> -       if (!(session->supported_events & (1 << pdu->params[0])))
> -               goto err;

When receiving a request our role is TG so I don't see why we would
skip this check, a better way to fix this would be to add the
separated supported events like discussed above so we have the roles
operating independent as they should be.

>         switch (pdu->params[0]) {
>         case AVRCP_EVENT_STATUS_CHANGED:
>                 len = 2;
> --
> 2.25.0
>


-- 
Luiz Augusto von Dentz