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: <20190429120259.17880-1-luiz.dentz@gmail.com> <20190429120259.17880-3-luiz.dentz@gmail.com>
In-Reply-To: <20190429120259.17880-3-luiz.dentz@gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 29 Apr 2019 19:35:24 +0300
Message-ID: <CABBYNZK1LRANfR6cwu+xbLsvy4EnB+ZPS78JNK04mJ=pwTGSDw@mail.gmail.com>
Subject: Re: [PATCH v3 3/3] a2dp: Fix not calling SelectConfiguration on other
 available endpoints
To:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Cc:     =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali.rohar@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Pali,

On Mon, Apr 29, 2019 at 3:03 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Endpoint may not be able to select a valid configuration but there could
> be other endpoints available that could be used, so instead of just
> using the first match this collects all the matching endpoints and put
> them into a queue (ordered by priority) then proceed to next endpoint
> only failing if none of the available endpoits can select a valid
> configuration.
> ---
>  profiles/audio/a2dp.c | 77 ++++++++++++++++++++++++++++---------------
>  1 file changed, 50 insertions(+), 27 deletions(-)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index a23abdd97..4d378a91a 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -105,6 +105,7 @@ struct a2dp_setup_cb {
>  struct a2dp_setup {
>         struct a2dp_channel *chan;
>         struct avdtp *session;
> +       struct queue *eps;
>         struct a2dp_sep *sep;
>         struct a2dp_remote_sep *rsep;
>         struct avdtp_stream *stream;
> @@ -2406,23 +2407,44 @@ void a2dp_remove_sep(struct a2dp_sep *sep)
>
>  static void select_cb(struct a2dp_setup *setup, void *ret, int size)
>  {
> -       if (size < 0) {
> -               DBG("Endpoint replied an invalid configuration");
> +       struct avdtp_service_capability *service;
> +       struct avdtp_media_codec_capability *codec;
> +       int err;
> +
> +       if (size) {
> +               caps_add_codec(&setup->caps, setup->sep->codec, ret, size);
>                 goto done;
>         }
>
> -       caps_add_codec(&setup->caps, setup->sep->codec, ret, size);
> +       setup->sep = queue_pop_head(setup->eps);
> +       if (!setup->sep) {
> +               error("Unable to select a valid configuration");
> +               queue_destroy(setup->eps, NULL);
> +               goto done;
> +       }
> +
> +       setup->rsep = find_remote_sep(setup->chan, setup->sep);
> +       service = avdtp_get_codec(setup->rsep->sep);
> +       codec = (struct avdtp_media_codec_capability *) service->data;
> +
> +       err = setup->sep->endpoint->select_configuration(setup->sep,
> +                                       codec->data,
> +                                       service->length - sizeof(*codec),
> +                                       setup_ref(setup),
> +                                       select_cb, setup->sep->user_data);
> +       if (err == 0)
> +               return;
>
>  done:
>         finalize_select(setup);
>         setup_unref(setup);
>  }
>
> -static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
> +static struct queue *a2dp_find_eps(struct avdtp *session, GSList *list,
>                                         const char *sender)
>  {
> -       struct a2dp_sep *first = NULL;
>         struct a2dp_channel *chan = find_channel(session);
> +       struct queue *seps = NULL;
>
>         for (; list; list = list->next) {
>                 struct a2dp_sep *sep = list->data;
> @@ -2444,26 +2466,25 @@ static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
>                 if (!rsep)
>                         continue;
>
> -               /* Locate last used if set */
> -               if (chan->last_used) {
> -                       if (chan->last_used->sep == rsep)
> -                               return sep;
> -                       first = sep;
> -                       continue;
> -               }
> +               if (!seps)
> +                       seps = queue_new();
>
> -               return sep;
> +               /* Prepend last used so it is preferred over others */
> +               if (chan->last_used && chan->last_used->sep == rsep)
> +                       queue_push_head(seps, sep);
> +               else
> +                       queue_push_tail(seps, sep);
>
>         }
>
> -       return first;
> +       return seps;
>  }
>
> -static struct a2dp_sep *a2dp_select_sep(struct avdtp *session, uint8_t type,
> +static struct queue *a2dp_select_eps(struct avdtp *session, uint8_t type,
>                                         const char *sender)
>  {
>         struct a2dp_server *server;
> -       struct a2dp_sep *sep;
> +       struct queue *seps;
>         GSList *l;
>
>         server = find_server(servers, avdtp_get_adapter(session));
> @@ -2473,11 +2494,11 @@ static struct a2dp_sep *a2dp_select_sep(struct avdtp *session, uint8_t type,
>         l = type == AVDTP_SEP_TYPE_SINK ? server->sources : server->sinks;
>
>         /* Check sender's seps first */
> -       sep = a2dp_find_sep(session, l, sender);
> -       if (sep != NULL)
> -               return sep;
> +       seps = a2dp_find_eps(session, l, sender);
> +       if (seps != NULL)
> +               return seps;
>
> -       return a2dp_find_sep(session, l, NULL);
> +       return a2dp_find_eps(session, l, NULL);
>  }
>
>  static void store_remote_sep(void *data, void *user_data)
> @@ -2580,13 +2601,13 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
>  {
>         struct a2dp_setup *setup;
>         struct a2dp_setup_cb *cb_data;
> -       struct a2dp_sep *sep;
> +       struct queue *eps;
>         struct avdtp_service_capability *service;
>         struct avdtp_media_codec_capability *codec;
>         int err;
>
> -       sep = a2dp_select_sep(session, type, sender);
> -       if (!sep) {
> +       eps = a2dp_select_eps(session, type, sender);
> +       if (!eps) {
>                 error("Unable to select SEP");
>                 return 0;
>         }
> @@ -2599,8 +2620,9 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
>         cb_data->select_cb = cb;
>         cb_data->user_data = user_data;
>
> -       setup->sep = sep;
> -       setup->rsep = find_remote_sep(setup->chan, sep);
> +       setup->eps = eps;
> +       setup->sep = queue_pop_head(eps);
> +       setup->rsep = find_remote_sep(setup->chan, setup->sep);
>
>         if (setup->rsep == NULL) {
>                 error("Could not find remote sep");
> @@ -2610,10 +2632,11 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
>         service = avdtp_get_codec(setup->rsep->sep);
>         codec = (struct avdtp_media_codec_capability *) service->data;
>
> -       err = sep->endpoint->select_configuration(sep, codec->data,
> +       err = setup->sep->endpoint->select_configuration(setup->sep,
> +                                       codec->data,
>                                         service->length - sizeof(*codec),
>                                         setup_ref(setup),
> -                                       select_cb, sep->user_data);
> +                                       select_cb, setup->sep->user_data);
>         if (err == 0)
>                 return cb_data->id;
>
> --
> 2.20.1

Le me know if you find any problem with this change, my headsets seems
to always succeed the first try so I cannot really reproduce the
problem.


-- 
Luiz Augusto von Dentz