Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230311003826.454858-1-marijn.suijten@somainline.org>
 <20230311003826.454858-2-marijn.suijten@somainline.org> <CABBYNZJimKVz46=6b=B4M9O=FW7hXJZBaRSM50U47oWtxcj0rQ@mail.gmail.com>
 <sh2dylmzfw4whlcru7gxwnaddzc5u7padivf47bllk7mwlfu5b@ebugmc2vjghg>
In-Reply-To: <sh2dylmzfw4whlcru7gxwnaddzc5u7padivf47bllk7mwlfu5b@ebugmc2vjghg>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 30 May 2023 15:38:23 -0700
Message-ID: <CABBYNZJVZ=obOAwcDhRd1Y877b-aT4itPdiqi2v4-hm9d5YZCw@mail.gmail.com>
Subject: Re: [PATCH BlueZ v3 1/3] audio/avrcp: Guard SetAbsoluteVolume without
 target behind config value
To:     Marijn Suijten <marijn.suijten@somainline.org>
Cc:     linux-bluetooth@vger.kernel.org, Yu Liu <yudiliu@google.com>,
        Bartosz Fabianowski <bartosz@fabianowski.eu>,
        Pauli Virtanen <pav@iki.fi>,
        Marek Czerski <ma.czerski@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Marijn,

On Tue, May 30, 2023 at 3:18=E2=80=AFPM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Hi Luiz,
>
> I'm still clueless why this reply didn't reach my inbox.  I thought it
> was going into ignore-land again (sorry) but randomly found your replies
> on the BlueZ mailing list.  The bot message didn't make it through
> either, but it did notify me about "audio/transport: Propagate errors
> from avrcp_set_volume to DBus" being applied.

Now that you mentioned it, I did receive:

The response from the remote server was:

550 5.7.1 Blocked due to message content - please contact postmaster@seeweb=
.it

So perhaps that is the reason you didn't get any responses for a while.


> On 2023-03-14 16:16:55, Luiz Augusto von Dentz wrote:
> > Hi Marijn,
> >
> > On Fri, Mar 10, 2023 at 4:39=E2=80=AFPM Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> > >
> > > Commit 179ccb936 ("avrcp: Set volume if volume changed event is
> > > registered") introduced a catch that allows SetAbsoluteVolume to be s=
ent
> > > to a remote device that does _not_ implement the AVRCP TG profile.  T=
his
> > > is strange as the TG role is required to be able to send commands to =
the
> > > peer, but the commit must have been applied to the tree for a reason.
> > >
> > > We discussed in [1] that workarounds for dubious peers and software
> > > stacks should be guarded behind a config entry in main.conf, so this
> > > starts out by introducing a new [AVRCP] category to to it that will
> > > later be extended with other workarounds.
> > >
> > > [1]: https://marc.info/?l=3Dlinux-bluetooth&m=3D163519566912788&w=3D2
> > > ---
> > >  profiles/audio/avrcp.c | 12 +++++++++---
> > >  src/btd.h              |  5 +++++
> > >  src/main.c             | 13 +++++++++++++
> > >  src/main.conf          |  6 ++++++
> > >  4 files changed, 33 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > > index 80f34c7a7..5e6322916 100644
> > > --- a/profiles/audio/avrcp.c
> > > +++ b/profiles/audio/avrcp.c
> > > @@ -48,6 +48,7 @@
> > >  #include "src/dbus-common.h"
> > >  #include "src/shared/timeout.h"
> > >  #include "src/shared/util.h"
> > > +#include "src/btd.h"
> > >
> > >  #include "avctp.h"
> > >  #include "avrcp.h"
> > > @@ -4577,9 +4578,14 @@ int avrcp_set_volume(struct btd_device *dev, i=
nt8_t volume, bool notify)
> > >                                                                 &volu=
me);
> > >         }
> > >
> > > -       if (!session->controller && !avrcp_event_registered(session,
> > > -                                       AVRCP_EVENT_VOLUME_CHANGED))
> > > -               return -ENOTSUP;
> > > +       if (btd_opts.avrcp.set_absolute_volume_without_target) {
> > > +               if (!session->controller && !avrcp_event_registered(s=
ession,
> > > +                                               AVRCP_EVENT_VOLUME_CH=
ANGED))
> > > +                       return -ENOTSUP;
> > > +       } else {
> > > +               if (!session->controller || session->controller->vers=
ion < 0x0104)
> > > +                       return -ENOTSUP;
> > > +       }
> > >
> > >         memset(buf, 0, sizeof(buf));
> > >
> > > diff --git a/src/btd.h b/src/btd.h
> > > index 42cffcde4..31c04a990 100644
> > > --- a/src/btd.h
> > > +++ b/src/btd.h
> > > @@ -97,6 +97,10 @@ struct btd_avdtp_opts {
> > >         uint8_t  stream_mode;
> > >  };
> > >
> > > +struct btd_avrcp_opts {
> > > +       gboolean set_absolute_volume_without_target;
> > > +};
> > > +
> > >  struct btd_advmon_opts {
> > >         uint8_t         rssi_sampling_period;
> > >  };
> > > @@ -136,6 +140,7 @@ struct btd_opts {
> > >         enum mps_mode_t mps;
> > >
> > >         struct btd_avdtp_opts avdtp;
> > > +       struct btd_avrcp_opts avrcp;
> > >
> > >         uint8_t         key_size;
> > >
> > > diff --git a/src/main.c b/src/main.c
> > > index 99d9c508f..92f74e381 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> > > @@ -152,6 +152,11 @@ static const char *avdtp_options[] =3D {
> > >         NULL
> > >  };
> > >
> > > +static const char *avrcp_options[] =3D {
> > > +       "SetAbsoluteVolumeWithoutTarget",
> > > +       NULL
> > > +};
> > > +
> > >  static const char *advmon_options[] =3D {
> > >         "RSSISamplingPeriod",
> > >         NULL
> > > @@ -167,6 +172,7 @@ static const struct group_table {
> > >         { "Policy",     policy_options },
> > >         { "GATT",       gatt_options },
> > >         { "AVDTP",      avdtp_options },
> > > +       { "AVRCP",      avrcp_options },
> > >         { "AdvMon",     advmon_options },
> > >         { }
> > >  };
> > > @@ -975,6 +981,13 @@ static void parse_config(GKeyFile *config)
> > >                 g_free(str);
> > >         }
> > >
> > > +       boolean =3D g_key_file_get_boolean(config, "AVRCP",
> > > +                                               "SetAbsoluteVolumeWit=
houtTarget", &err);
> > > +       if (err)
> > > +               g_clear_error(&err);
> > > +       else
> > > +               btd_opts.avrcp.set_absolute_volume_without_target =3D=
 boolean;
> > > +
> > >         val =3D g_key_file_get_integer(config, "AdvMon", "RSSISamplin=
gPeriod",
> > >                                                                      =
   &err);
> > >         if (err) {
> > > diff --git a/src/main.conf b/src/main.conf
> > > index f187c9aaa..ca00ed03e 100644
> > > --- a/src/main.conf
> > > +++ b/src/main.conf
> > > @@ -271,6 +271,12 @@
> > >  # streaming: Use L2CAP Streaming Mode
> > >  #StreamMode =3D basic
> > >
> > > +[AVRCP]
> > > +# Allow SetAbsoluteVolume calls to a peer device that
> > > +# does not advertise the AVRCP remote control target
> > > +# profile.
> > > +#SetAbsoluteVolumeWithoutTarget =3D false
> >
> > Let's do just VolumeWithoutTarget and we should probably mention that
> > it would ignore the version as well.
>
> Sure, sounds good.  We can also invert the condition to validate the
> version in the event that ->controller is non-NULL, even if
> VolumeWithoutTarget is set.
>
> - Marijn
>
> >
> > > +
> > >  [Policy]
> > >  #
> > >  # The ReconnectUUIDs defines the set of remote services that should =
try
> > > --
> > > 2.39.2
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz


--=20
Luiz Augusto von Dentz