[Bluez PATCH v1 1/3] policy: add checks before connecting

[Bluez PATCH v1 1/3] policy: add checks before connecting

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

When policy_connect() is called, there might be a case where the
device is not ready, or even the adapter is down. Add some checks
by calling btd_device_connect_services() instead of directly calling
btd_service_connect().

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
---

 plugins/policy.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/plugins/policy.c b/plugins/policy.c
index ba9e1be020..42b15cb65f 100644
--- a/plugins/policy.c
+++ b/plugins/policy.c
@@ -106,6 +106,7 @@ static void policy_connect(struct policy_data *data,
 {
 	struct btd_profile *profile = btd_service_get_profile(service);
 	struct reconnect_data *reconnect;
+	GSList *l = NULL;
 
 	reconnect = reconnect_find(btd_service_get_device(service));
 	if (reconnect && reconnect->active)
@@ -113,7 +114,9 @@ static void policy_connect(struct policy_data *data,
 
 	DBG("%s profile %s", device_get_path(data->dev), profile->name);
 
-	btd_service_connect(service);
+	l = g_slist_prepend(l, service);
+	btd_device_connect_services(data->dev, l);
+	g_slist_free(l);
 }
 
 static void policy_disconnect(struct policy_data *data,
-- 
2.29.1.341.ge80a0c044ae-goog

[Bluez PATCH v1 2/3] audio: unref session when failed to setup

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

There is a possibility to miss unref-ing when source/sink fails at
setup.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
---

 profiles/audio/sink.c   | 5 ++++-
 profiles/audio/source.c | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/profiles/audio/sink.c b/profiles/audio/sink.c
index 134d157bc6..d672670e25 100644
--- a/profiles/audio/sink.c
+++ b/profiles/audio/sink.c
@@ -258,8 +258,11 @@ gboolean sink_setup_stream(struct btd_service *service, struct avdtp *session)
 
 	sink->connect_id = a2dp_discover(sink->session, discovery_complete,
 								sink);
-	if (sink->connect_id == 0)
+	if (sink->connect_id == 0) {
+		avdtp_unref(sink->session);
+		sink->session = NULL;
 		return FALSE;
+	}
 
 	return TRUE;
 }
diff --git a/profiles/audio/source.c b/profiles/audio/source.c
index fca85d4cb3..c706c928a7 100644
--- a/profiles/audio/source.c
+++ b/profiles/audio/source.c
@@ -259,8 +259,11 @@ gboolean source_setup_stream(struct btd_service *service,
 
 	source->connect_id = a2dp_discover(source->session, discovery_complete,
 								source);
-	if (source->connect_id == 0)
+	if (source->connect_id == 0) {
+		avdtp_unref(source->session);
+		source->session = NULL;
 		return FALSE;
+	}
 
 	return TRUE;
 }
-- 
2.29.1.341.ge80a0c044ae-goog

[Bluez PATCH v1 3/3] audio/avdtp: Report failure in disconnected state

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

A2DP are relying on the disconnected state callback to do cleanup.
If failure occurs when AVDTP are already in the disconnected state,
we didn't make any transition state, therefore A2DP would miss this
event.

This patch allows the transition to disconnected state, even though
we are previously already in the disconnected state.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
---

 profiles/audio/avdtp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 4c39088b8f..16fa20bba7 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -2612,6 +2612,11 @@ static int send_req(struct avdtp *session, gboolean priority,
 	if (session->state == AVDTP_SESSION_STATE_DISCONNECTED) {
 		session->io = l2cap_connect(session);
 		if (!session->io) {
+			/* Report disconnection anyways, as the other layers
+			 * are using this state for cleanup.
+			 */
+			avdtp_set_state(session,
+					AVDTP_SESSION_STATE_DISCONNECTED);
 			err = -EIO;
 			goto failed;
 		}
-- 
2.29.1.341.ge80a0c044ae-goog

RE: [Bluez,v1,1/3] policy: add checks before connecting

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=377031

---Test result---

##############################
Test: CheckPatch - PASS

##############################
Test: CheckGitLint - PASS

##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth

Re: [Bluez PATCH v1 1/3] policy: add checks before connecting

[Luiz Augusto von Dentz]

Hi Archie,

On Tue, Nov 3, 2020 at 9:35 PM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> When policy_connect() is called, there might be a case where the
> device is not ready, or even the adapter is down. Add some checks
> by calling btd_device_connect_services() instead of directly calling
> btd_service_connect().

But we could perform these checks in btd_service_connect or you also
intended to use the pending list? Im a little hesitant with such a
change though because there could be a pending connect already causing
it to fail instead of just connecting in parallel.

> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> ---
>
>  plugins/policy.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/plugins/policy.c b/plugins/policy.c
> index ba9e1be020..42b15cb65f 100644
> --- a/plugins/policy.c
> +++ b/plugins/policy.c
> @@ -106,6 +106,7 @@ static void policy_connect(struct policy_data *data,
>  {
>         struct btd_profile *profile = btd_service_get_profile(service);
>         struct reconnect_data *reconnect;
> +       GSList *l = NULL;
>
>         reconnect = reconnect_find(btd_service_get_device(service));
>         if (reconnect && reconnect->active)
> @@ -113,7 +114,9 @@ static void policy_connect(struct policy_data *data,
>
>         DBG("%s profile %s", device_get_path(data->dev), profile->name);
>
> -       btd_service_connect(service);
> +       l = g_slist_prepend(l, service);
> +       btd_device_connect_services(data->dev, l);
> +       g_slist_free(l);
>  }
>
>  static void policy_disconnect(struct policy_data *data,
> --
> 2.29.1.341.ge80a0c044ae-goog
>


-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH v1 1/3] policy: add checks before connecting

[Archie Pusaka]

Hi Luiz,

On Thu, 5 Nov 2020 at 05:13, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Tue, Nov 3, 2020 at 9:35 PM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > When policy_connect() is called, there might be a case where the
> > device is not ready, or even the adapter is down. Add some checks
> > by calling btd_device_connect_services() instead of directly calling
> > btd_service_connect().
>
> But we could perform these checks in btd_service_connect or you also
> intended to use the pending list? Im a little hesitant with such a
> change though because there could be a pending connect already causing
> it to fail instead of just connecting in parallel.

Yes, we can also perform the checks in btd_service_connect or in policy_connect.
I have no intention of using the pending list, and to be frank, I also
don't know whether we should let the connection happen in parallel or
we should form a queue.

This issue correlates with the other issues addressed in the second
and third patch.
However, if we leave this patch and only fix the second and third
issue, this one would not cause a problem by itself because even when
the adapter is down and policy_connect is called, it would just fail
and we would recover cleanly.
This patch is merely just to skip the unnecessary connection attempt.
I will submit another patch which performs the check inside
btd_service_connect, and I'll leave it to you to decide whether this
optional patch is needed.

>
> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> > ---
> >
> >  plugins/policy.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/plugins/policy.c b/plugins/policy.c
> > index ba9e1be020..42b15cb65f 100644
> > --- a/plugins/policy.c
> > +++ b/plugins/policy.c
> > @@ -106,6 +106,7 @@ static void policy_connect(struct policy_data *data,
> >  {
> >         struct btd_profile *profile = btd_service_get_profile(service);
> >         struct reconnect_data *reconnect;
> > +       GSList *l = NULL;
> >
> >         reconnect = reconnect_find(btd_service_get_device(service));
> >         if (reconnect && reconnect->active)
> > @@ -113,7 +114,9 @@ static void policy_connect(struct policy_data *data,
> >
> >         DBG("%s profile %s", device_get_path(data->dev), profile->name);
> >
> > -       btd_service_connect(service);
> > +       l = g_slist_prepend(l, service);
> > +       btd_device_connect_services(data->dev, l);
> > +       g_slist_free(l);
> >  }
> >
> >  static void policy_disconnect(struct policy_data *data,
> > --
> > 2.29.1.341.ge80a0c044ae-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie