Return-Path: From: Szymon Janc To: Luiz Augusto von Dentz Cc: Marcin Kraglak , "linux-bluetooth@vger.kernel.org" Subject: Re: [PATCH 1/3] android/tester: Init profiles in separate thread Date: Fri, 05 Sep 2014 13:30:02 +0200 Message-ID: <3055306.0vU7R7HGBv@uw000953> In-Reply-To: References: <1409904596-10254-1-git-send-email-marcin.kraglak@tieto.com> <12480623.xF9C9qG3Tj@uw000953> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Luiz, On Friday 05 of September 2014 14:19:48 Luiz Augusto von Dentz wrote: > Hi Szymon, > > On Fri, Sep 5, 2014 at 12:50 PM, Szymon Janc wrote: > > Hi Marcin, > > > > On Friday 05 of September 2014 10:09:54 Marcin Kraglak wrote: > >> This is needed to not block mainloop while initializing stack. > >> In some cases, bluetooth->init blocks mainloop and bluetoothd > >> cannot properly initialize controller, because HCI frames are > >> not serviced in hciemu. > >> Because pthread_join() is blocking, it is added to mainloop > >> from thread right before pthread_exit() is called. Therefore > >> this time is extremely short. > >> --- > >> android/tester-main.c | 230 +++++++++++++++++++++++++++----------------------- > >> android/tester-main.h | 2 + > >> 2 files changed, 127 insertions(+), 105 deletions(-) > >> > >> diff --git a/android/tester-main.c b/android/tester-main.c > >> index f5f46fb..ce13bf7 100644 > >> --- a/android/tester-main.c > >> +++ b/android/tester-main.c > >> @@ -15,8 +15,10 @@ > >> * > >> */ > >> #include > >> +#include > >> > >> #include "emulator/bthost.h" > >> +#include "src/shared/util.h" > >> #include "tester-main.h" > >> > >> #include "monitor/bt.h" > >> @@ -1332,237 +1334,255 @@ static bool setup_base(struct test_data *data) > >> return true; > >> } > >> > >> -static void setup(const void *test_data) > >> +static int init_bluetooth(void) > >> { > >> struct test_data *data = tester_get_data(); > >> - bt_status_t status; > >> - > >> - if (!setup_base(data)) { > >> - tester_setup_failed(); > >> - return; > >> - } > >> - > >> - status = data->if_bluetooth->init(&bt_callbacks); > >> - if (status != BT_STATUS_SUCCESS) { > >> - data->if_bluetooth = NULL; > >> - tester_setup_failed(); > >> - return; > >> - } > >> > >> - tester_setup_complete(); > >> + return data->if_bluetooth->init(&bt_callbacks); > >> } > >> > >> -static void setup_socket(const void *test_data) > >> +static int init_socket(void) > >> { > >> struct test_data *data = tester_get_data(); > >> - bt_status_t status; > >> const void *sock; > >> - > >> - if (!setup_base(data)) { > >> - tester_setup_failed(); > >> - return; > >> - } > >> + int status; > >> > >> status = data->if_bluetooth->init(&bt_callbacks); > >> if (status != BT_STATUS_SUCCESS) { > >> data->if_bluetooth = NULL; > >> - tester_setup_failed(); > >> - return; > >> + return status; > >> } > >> > >> sock = data->if_bluetooth->get_profile_interface(BT_PROFILE_SOCKETS_ID); > >> - if (!sock) { > >> - tester_setup_failed(); > >> - return; > >> - } > >> + if (!sock) > >> + return BT_STATUS_FAIL; > >> > >> data->if_sock = sock; > >> > >> - tester_setup_complete(); > >> + return BT_STATUS_SUCCESS; > >> } > >> > >> -static void setup_hidhost(const void *test_data) > >> +static int init_hidhost(void) > >> { > >> struct test_data *data = tester_get_data(); > >> bt_status_t status; > >> const void *hid; > >> > >> - if (!setup_base(data)) { > >> - tester_setup_failed(); > >> - return; > >> - } > >> - > >> status = data->if_bluetooth->init(&bt_callbacks); > >> if (status != BT_STATUS_SUCCESS) { > >> data->if_bluetooth = NULL; > >> tester_setup_failed(); > >> - return; > >> + return status; > >> } > >> > >> hid = data->if_bluetooth->get_profile_interface(BT_PROFILE_HIDHOST_ID); > >> if (!hid) { > >> tester_setup_failed(); > >> - return; > >> + return BT_STATUS_FAIL; > >> } > >> > >> data->if_hid = hid; > >> > >> status = data->if_hid->init(&bthh_callbacks); > >> - if (status != BT_STATUS_SUCCESS) { > >> + if (status != BT_STATUS_SUCCESS) > >> data->if_hid = NULL; > >> - tester_setup_failed(); > >> - return; > >> - } > >> > >> - tester_setup_complete(); > >> + return status; > >> } > >> > >> -static void setup_pan(const void *test_data) > >> +static int init_pan(void) > >> { > >> struct test_data *data = tester_get_data(); > >> bt_status_t status; > >> const void *pan; > >> > >> - if (!setup_base(data)) { > >> - tester_setup_failed(); > >> - return; > >> - } > >> - > >> status = data->if_bluetooth->init(&bt_callbacks); > >> if (status != BT_STATUS_SUCCESS) { > >> data->if_bluetooth = NULL; > >> - tester_setup_failed(); > >> - return; > >> + return status; > >> } > >> > >> pan = data->if_bluetooth->get_profile_interface(BT_PROFILE_PAN_ID); > >> if (!pan) { > >> - tester_setup_failed(); > >> - return; > >> + return BT_STATUS_FAIL; > >> } > >> > >> data->if_pan = pan; > >> > >> status = data->if_pan->init(&btpan_callbacks); > >> - if (status != BT_STATUS_SUCCESS) { > >> + if (status != BT_STATUS_SUCCESS) > >> data->if_pan = NULL; > >> - tester_setup_failed(); > >> - return; > >> - } > >> > >> - tester_setup_complete(); > >> + return status; > >> } > >> > >> -static void setup_hdp(const void *test_data) > >> +static int init_hdp(void) > >> { > >> struct test_data *data = tester_get_data(); > >> bt_status_t status; > >> const void *hdp; > >> > >> - if (!setup_base(data)) { > >> - tester_setup_failed(); > >> - return; > >> - } > >> - > >> status = data->if_bluetooth->init(&bt_callbacks); > >> if (status != BT_STATUS_SUCCESS) { > >> data->if_bluetooth = NULL; > >> - tester_setup_failed(); > >> - return; > >> + return status; > >> } > >> > >> hdp = data->if_bluetooth->get_profile_interface(BT_PROFILE_HEALTH_ID); > >> - if (!hdp) { > >> - tester_setup_failed(); > >> - return; > >> - } > >> + if (!hdp) > >> + return BT_STATUS_FAIL; > >> > >> data->if_hdp = hdp; > >> > >> status = data->if_hdp->init(&bthl_callbacks); > >> - if (status != BT_STATUS_SUCCESS) { > >> + if (status != BT_STATUS_SUCCESS) > >> data->if_hdp = NULL; > >> - tester_setup_failed(); > >> - return; > >> - } > >> > >> - tester_setup_complete(); > >> + return status; > >> } > >> > >> -static void setup_a2dp(const void *test_data) > >> +static int init_a2dp(void) > >> { > >> struct test_data *data = tester_get_data(); > >> const bt_interface_t *if_bt; > >> bt_status_t status; > >> const void *a2dp; > >> > >> - if (!setup_base(data)) { > >> - tester_setup_failed(); > >> - return; > >> - } > >> - > >> if_bt = data->if_bluetooth; > >> > >> status = if_bt->init(&bt_callbacks); > >> if (status != BT_STATUS_SUCCESS) { > >> data->if_bluetooth = NULL; > >> - tester_setup_failed(); > >> - return; > >> + return status; > >> } > >> > >> a2dp = if_bt->get_profile_interface(BT_PROFILE_ADVANCED_AUDIO_ID); > >> - if (!a2dp) { > >> - tester_setup_failed(); > >> - return; > >> - } > >> + if (!a2dp) > >> + return BT_STATUS_FAIL; > >> > >> data->if_a2dp = a2dp; > >> > >> status = data->if_a2dp->init(&bta2dp_callbacks); > >> - if (status != BT_STATUS_SUCCESS) { > >> + if (status != BT_STATUS_SUCCESS) > >> data->if_a2dp = NULL; > >> - tester_setup_failed(); > >> - return; > >> - } > >> > >> - tester_setup_complete(); > >> + return status; > >> } > >> > >> -static void setup_gatt(const void *test_data) > >> +static int init_gatt(void) > >> { > >> struct test_data *data = tester_get_data(); > >> bt_status_t status; > >> const void *gatt; > >> > >> - if (!setup_base(data)) { > >> - tester_setup_failed(); > >> - return; > >> - } > >> - > >> status = data->if_bluetooth->init(&bt_callbacks); > >> if (status != BT_STATUS_SUCCESS) { > >> data->if_bluetooth = NULL; > >> - tester_setup_failed(); > >> - return; > >> + return status; > >> } > >> > >> gatt = data->if_bluetooth->get_profile_interface(BT_PROFILE_GATT_ID); > >> - if (!gatt) { > >> - tester_setup_failed(); > >> - return; > >> - } > >> + if (!gatt) > >> + return BT_STATUS_FAIL; > >> > >> data->if_gatt = gatt; > >> > >> status = data->if_gatt->init(&btgatt_callbacks); > >> - if (status != BT_STATUS_SUCCESS) { > >> + if (status != BT_STATUS_SUCCESS) > >> data->if_gatt = NULL; > >> > >> + return status; > >> +} > >> + > >> +static gboolean join_init_thread(gpointer user_data) > >> +{ > >> + struct test_data *data = tester_get_data(); > >> + int status; > >> + > >> + /* Note that pthread_join is blocking mainloop, > >> + * therefore thread we want to join must > >> + * exit imediatelly. This is done in init_thread()*/ > >> + status = pthread_join(data->thread, NULL); > >> + if (status != 0) > >> + tester_warn("Failed to join init thread, status %d", status); > >> + > >> + if (PTR_TO_INT(user_data) == BT_STATUS_SUCCESS) > >> + tester_setup_complete(); > >> + else > >> + tester_setup_failed(); > >> + > >> + return FALSE; > >> +} > >> + > >> +static void *init_thread(void *user_data) > >> +{ > >> + int (*setup_func)(void) = user_data; > >> + int status; > >> + > >> + status = setup_func(); > >> + > >> + g_idle_add(join_init_thread, INT_TO_PTR(status)); > >> + > >> + pthread_exit(NULL); > >> +} > >> + > >> +static bool start_init_thread(int (*init)(void)) > >> +{ > >> + struct test_data *data = tester_get_data(); > >> + > >> + if (pthread_create(&data->thread, NULL, init_thread, init) != 0) > >> + return false; > >> + > >> + return true; > >> +} > >> + > >> +static void setup_generic(int (*setup)(void)) > >> +{ > >> + struct test_data *data = tester_get_data(); > >> + > >> + if (!setup_base(data)) { > >> tester_setup_failed(); > >> return; > >> } > >> > >> - tester_setup_complete(); > >> + if (!start_init_thread(setup)) > >> + tester_setup_failed(); > >> +} > >> + > >> +static void setup(const void *test_data) > >> +{ > >> + setup_generic(init_bluetooth); > >> +} > >> + > >> +static void setup_socket(const void *test_data) > >> +{ > >> + > >> + setup_generic(init_socket); > >> +} > >> + > >> +static void setup_hidhost(const void *test_data) > >> +{ > >> + setup_generic(init_hidhost); > >> +} > >> + > >> +static void setup_pan(const void *test_data) > >> +{ > >> + setup_generic(init_pan); > >> +} > >> + > >> +static void setup_hdp(const void *test_data) > >> +{ > >> + setup_generic(init_hdp); > >> +} > >> + > >> +static void setup_a2dp(const void *test_data) > >> +{ > >> + setup_generic(init_a2dp); > >> +} > >> + > >> +static void setup_gatt(const void *test_data) > >> +{ > >> + setup_generic(init_gatt); > >> } > >> > >> static void teardown(const void *test_data) > >> diff --git a/android/tester-main.h b/android/tester-main.h > >> index 46aacce..f90cf86 100644 > >> --- a/android/tester-main.h > >> +++ b/android/tester-main.h > >> @@ -331,6 +331,8 @@ struct test_data { > >> pid_t bluetoothd_pid; > >> > >> struct queue *pdus; > >> + > >> + pthread_t thread; > >> }; > >> > >> /* > >> > > > > I've applied patches 2 and 3. Patch 1 needs to be modified as we discussed > > offline. Thanks. > > We used to fork bluetoothd when bringing up the interface with ioctl, > so I guess we could for here as well just to initialize. We can't use fork here since we would initialize HAL in child process, not parent. -- Best regards, Szymon Janc