Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2860-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240327161019.3078-1-iulia.tanasescu@nxp.com> <20240327161019.3078-3-iulia.tanasescu@nxp.com>
In-Reply-To: <20240327161019.3078-3-iulia.tanasescu@nxp.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Wed, 27 Mar 2024 12:50:35 -0400
Message-ID: <CABBYNZJZQf8uvyNzHiqOo4UqwdYb+OEyBZvt1EHvEd3h7t1CMQ@mail.gmail.com>
Subject: Re: [PATCH BlueZ 2/2] iso-tester: Separate iso_defer_accept into
 dedicated functions
To: Iulia Tanasescu <iulia.tanasescu@nxp.com>
Cc: linux-bluetooth@vger.kernel.org, claudia.rosu@nxp.com, 
	mihai-octavian.urzica@nxp.com, silviu.barbulescu@nxp.com, 
	vlad.pruteanu@nxp.com, andrei.istodorescu@nxp.com, 
	Pauli Virtanen <pav@iki.fi>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Iulia,

On Wed, Mar 27, 2024 at 12:10=E2=80=AFPM Iulia Tanasescu
<iulia.tanasescu@nxp.com> wrote:
>
> This separates the iso_defer_accept function into dedicated ones for
> unicast/broadcast, since the flow is different for each scenario:
> For unicast, POLLOUT is checked on socket before calling read and
> adding a G_IO_OUT watch. Checking for POLLOUT is not necessary for
> broadcast, since currently this event is never generated on the
> child socket. Instead, G_IO_IN is generated after BIG sync is
> established and a BIS socket is ready to be accepted.

@Pauli Virtanen Is this inline with TX Timestamping though? Or that
only cares for POLLERR?

Also it would be great to know what are the plans of PW with respect
to broadcast, I was thinking something like this:

1. Broadcast Source: Have some dedicated card that can be configured
via configuration file or a dedicated app that performs the
configuration which can then select what streams shall be broadcasted,
since broadcasting things like system audio notifications probably
doesn't make much sense.
2. Broadcast Sink: This shall be a little bit easier since we now
enumerate the BASE found over the air, so it should work very closely
to unicast, but perhaps you may also want to start a scan session
while the card selection dialog is open so the user don't need to use
yet another app to trigger it, or perhaps this should really be done
at Bluetooth system settings that way PW don't show any Broadcast Sink
until the user selects it at Bluetooth APP, this way we don't clutter
PW interface with unsynchronized Broadcast Sinks. Note that in the
future we are going to remove the step of creating a MediaEndpoint for
Sinks since they are already configured over the air they shall appear
as MediaTransports ready to be Acquired.

> ---
>  tools/iso-tester.c | 45 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/tools/iso-tester.c b/tools/iso-tester.c
> index 1864b9e9d..60afef301 100644
> --- a/tools/iso-tester.c
> +++ b/tools/iso-tester.c
> @@ -4,7 +4,7 @@
>   *  BlueZ - Bluetooth protocol stack for Linux
>   *
>   *  Copyright (C) 2022  Intel Corporation.
> - *  Copyright 2023 NXP
> + *  Copyright 2023-2024 NXP
>   *
>   */
>
> @@ -489,6 +489,8 @@ struct iso_client_data {
>         bool pa_bind;
>  };
>
> +typedef bool (*iso_defer_accept_t)(struct test_data *data, GIOChannel *i=
o);
> +
>  static void mgmt_debug(const char *str, void *user_data)
>  {
>         const char *prefix =3D user_data;
> @@ -2582,11 +2584,10 @@ static void setup_listen(struct test_data *data, =
uint8_t num, GIOFunc func)
>         }
>  }
>
> -static bool iso_defer_accept(struct test_data *data, GIOChannel *io)
> +static bool iso_defer_accept_bcast(struct test_data *data, GIOChannel *i=
o)
>  {
>         int sk;
>         char c;
> -       struct pollfd pfd;
>         const struct iso_client_data *isodata =3D data->test_data;
>         struct sockaddr_iso *addr =3D NULL;
>
> @@ -2610,6 +2611,31 @@ static bool iso_defer_accept(struct test_data *dat=
a, GIOChannel *io)
>                 free(addr);
>         }
>
> +       if (read(sk, &c, 1) < 0) {
> +               tester_warn("read: %s (%d)", strerror(errno), errno);
> +               return false;
> +       }
> +
> +       tester_print("Accept deferred setup");
> +
> +       data->io_queue =3D queue_new();
> +       if (data->io_queue)
> +               queue_push_tail(data->io_queue, io);
> +
> +       data->io_id[0] =3D g_io_add_watch(io, G_IO_IN,
> +                               iso_accept_cb, NULL);
> +
> +       return true;
> +}
> +
> +static bool iso_defer_accept_ucast(struct test_data *data, GIOChannel *i=
o)
> +{
> +       int sk;
> +       char c;
> +       struct pollfd pfd;
> +
> +       sk =3D g_io_channel_unix_get_fd(io);
> +
>         memset(&pfd, 0, sizeof(pfd));
>         pfd.fd =3D sk;
>         pfd.events =3D POLLOUT;
> @@ -2632,12 +2658,8 @@ static bool iso_defer_accept(struct test_data *dat=
a, GIOChannel *io)
>         if (data->io_queue)
>                 queue_push_tail(data->io_queue, io);
>
> -       if (isodata->bcast)
> -               data->io_id[0] =3D g_io_add_watch(io, G_IO_IN,
> -                                       iso_accept_cb, NULL);
> -       else
> -               data->io_id[0] =3D g_io_add_watch(io, G_IO_OUT,
> -                                       iso_connect_cb, NULL);
> +       data->io_id[0] =3D g_io_add_watch(io, G_IO_OUT,
> +                               iso_connect_cb, NULL);
>
>         return true;
>  }
> @@ -2648,6 +2670,9 @@ static gboolean iso_accept_cb(GIOChannel *io, GIOCo=
ndition cond,
>         struct test_data *data =3D tester_get_data();
>         const struct iso_client_data *isodata =3D data->test_data;
>         int sk, new_sk;
> +       iso_defer_accept_t iso_accept =3D isodata->bcast ?
> +                                               iso_defer_accept_bcast :
> +                                               iso_defer_accept_ucast;
>
>         data->io_id[0] =3D 0;
>
> @@ -2676,7 +2701,7 @@ static gboolean iso_accept_cb(GIOChannel *io, GIOCo=
ndition cond,
>                                 return false;
>                 }
>
> -               if (!iso_defer_accept(data, io)) {
> +               if (!iso_accept(data, io)) {
>                         tester_warn("Unable to accept deferred setup");
>                         tester_test_failed();
>                 }
> --
> 2.39.2
>


--=20
Luiz Augusto von Dentz