Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2172-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240223152940.40099-1-sibobrenok@salutedevices.com>
In-Reply-To: <20240223152940.40099-1-sibobrenok@salutedevices.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Tue, 27 Feb 2024 11:37:22 -0500
Message-ID: <CABBYNZJYd-6f3qq6S-Km6M2Wr-QOc2UydKx26hR=A8n-375HQw@mail.gmail.com>
Subject: Re: [PATCH] main.conf: Introduce GATT.ReverseClient option
To: Sergey Bobrenok <sibobrenok@salutedevices.com>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Sergey,

On Fri, Feb 23, 2024 at 10:30=E2=80=AFAM Sergey Bobrenok
<sibobrenok@salutedevices.com> wrote:
>
> General.ReverseServiceDiscovery option is responsible for two
> different things:
>  1. It disables SDP reverse discovering. As a side effect, some BR/EDR
>  profiles cannot operate properly. E.g. AVRCP-target profile needs SDP
>  results to determine the peer's AVRCP version.
>  2. It disables GATT-client creation back to the GATT connection
>  initiator. It may be useful for peripheral devices, especially if the
>  peer doesn't expect them to connect back (and currently some IOS
>  versions don't). This behavior was introduced in
>  8de73cd12 ("main.conf: Make ReverseServiceDiscovery work with LE")
>
> For peripheral devices implementing only A2DP-sink, AVRCP-target, and
> GATT profiles (e.g. BT loudspeakers), it may be useful to disable
> GATT-client functionality, but still have SDP reverse discovering.
>
> Unfortunately, splitting the General.ReverseServiceDiscovery option
> into two different options will break backward compatibility on the
> configuration file level. So a new configuration option has been
> introduced in addition to the old one.

While I agree we need some option to handle such cases where the
remote end don't expects us to act as GATT Client the option name
shall probably be just to disable the GATT client entirely not just
reverse discovery so it an independent which affects
ReverseServiceDiscovery implicitly otherwise there is a dependency on
one another which may lead to confusions like setting
ReverseServiceDiscovery=3Dfalse and then ReverseClient=3Dtrue would have
no effect.

> Signed-off-by: Sergey Bobrenok <sibobrenok@salutedevices.com>
> ---
>  src/btd.h     | 1 +
>  src/device.c  | 4 ++++
>  src/main.c    | 4 ++++
>  src/main.conf | 7 ++++++-
>  4 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/src/btd.h b/src/btd.h
> index 7166e2168..f85b119b7 100644
> --- a/src/btd.h
> +++ b/src/btd.h
> @@ -140,6 +140,7 @@ struct btd_opts {
>         bt_gatt_cache_t gatt_cache;
>         uint16_t        gatt_mtu;
>         uint8_t         gatt_channels;
> +       gboolean        gatt_reverse_client;
>         enum mps_mode_t mps;
>
>         struct btd_avdtp_opts avdtp;
> diff --git a/src/device.c b/src/device.c
> index 1db96d9a6..e8d66ffe4 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -5452,6 +5452,10 @@ static void gatt_client_init(struct btd_device *de=
vice)
>                 DBG("Reverse service discovery disabled: skipping GATT cl=
ient");
>                 return;
>         }
> +       if (!device->connect && !btd_opts.gatt_reverse_client) {
> +               DBG("Reverse gatt client disabled: skipping GATT client")=
;
> +               return;
> +       }
>
>         device->client =3D bt_gatt_client_new(device->db, device->att,
>                                                         device->att_mtu, =
0);
> diff --git a/src/main.c b/src/main.c
> index b1339c230..f0c2c4311 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -145,6 +145,7 @@ static const char *gatt_options[] =3D {
>         "KeySize",
>         "ExchangeMTU",
>         "Channels",
> +       "ReverseClient",
>         NULL
>  };
>
> @@ -1058,6 +1059,8 @@ static void parse_gatt(GKeyFile *config)
>                                 BT_ATT_DEFAULT_LE_MTU, BT_ATT_MAX_LE_MTU)=
;
>         parse_config_u8(config, "GATT", "Channels", &btd_opts.gatt_channe=
ls,
>                                 1, 5);
> +       parse_config_bool(config, "GATT", "ReverseClient",
> +                               &btd_opts.gatt_reverse_client);
>  }
>
>  static void parse_csis_sirk(GKeyFile *config)
> @@ -1190,6 +1193,7 @@ static void init_defaults(void)
>         btd_opts.gatt_cache =3D BT_GATT_CACHE_ALWAYS;
>         btd_opts.gatt_mtu =3D BT_ATT_MAX_LE_MTU;
>         btd_opts.gatt_channels =3D 1;
> +       btd_opts.gatt_reverse_client =3D TRUE;
>
>         btd_opts.avdtp.session_mode =3D BT_IO_MODE_BASIC;
>         btd_opts.avdtp.stream_mode =3D BT_IO_MODE_BASIC;
> diff --git a/src/main.conf b/src/main.conf
> index 085c81a46..a617eec6b 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -33,7 +33,7 @@
>  # us. For BR/EDR this option is really only needed for qualification sin=
ce the
>  # BITE tester doesn't like us doing reverse SDP for some test cases, for=
 LE
>  # this disables the GATT client functionally so it can be used in system=
 which
> -# can only operate as peripheral.
> +# can only operate as peripheral (see also GATT ReverseClient).
>  # Defaults to 'true'.
>  #ReverseServiceDiscovery =3D true
>
> @@ -283,6 +283,11 @@
>  # Defaults to 0
>  #Rank =3D 0
>
> +# This disables the GATT client functionally so it can be used in system=
 which
> +# can only operate as peripheral.
> +# Defaults to 'true'.
> +#ReverseClient =3D true

Like explained above I'd suggest we go with just a Client setting here.

>  [AVDTP]
>  # AVDTP L2CAP Signalling Channel Mode.
>  # Possible values:
>
> base-commit: a16c2ccf9c256285188f4549b7b767cf31b100eb
> --
> 2.43.2
>
>


--=20
Luiz Augusto von Dentz