| Message ID | 20201104133318.Bluez.v1.1.Ibb93fb0188187d11151855d2dcc737640b2b81da@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [Bluez,v1,1/3] policy: add checks before connecting | expand |
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
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
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
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,