Hi Bing,
On Tue, Sep 24, 2013, Bing Zhao wrote:
> > that is a bug. It should only be ever called once. Could this be due
> > to RFKILL issue we had? Please re-test with Johan's patches applied
> > and check if it makes a difference. Otherwise please send some logs
> > since we want to get this fixed.
>
> Amitkumar Karwar has tested it with latest code on bluetooth-next tree
> but the result is the same.
> Apparently two threads race to call hci_dev_open(). If the thread from
> hci_sock calls hci_dev_open earlier, it ends up not updating HCI_SETUP
> hdev flag in hci_power_on(). This results that the setup handler gets
> called again when user brings up the interface later.
Let's see if I understood this right: the only hci_dev_open call in
hci_sock.c is the one for the HCIDEVUP ioctl. So what you're doing is
having user space call the HCIDEVUP ioctl before our own hci_power_on
callback gets called to initialize the adapter?
You're right that we're missing the clearing of the HCI_SETUP flag for
such a scenario. Could you try the attached patch. It should fix the
issue. One problem that it does have is that if the HCIDEVUP ioctl path
goes through before hci_power_on gets called we will never notify mgmt
of the adapter. However, that might be acceptable here since if you're
using HCIDEVUP like this it seems it's not a mgmt based system anyway.
> I checked the bluetooth-next tree, the following two patches (by
> Johan) are not present in this tree.
>
> bf54303 Bluetooth: Fix rfkill functionality during the HCI setup stage
> 5e13036 Bluetooth: Introduce a new HCI_RFKILLED flag
>
> They are in bluetooth.git tree. So, I'm not certain if Amitkumar has
> applied them manually or not. Anyway we will re-test with Johan's
> patches applied and confirm if they fix the race or not.
I don't think these patches will help you in this case.
Johan
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 634deba..c48bf1a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1164,7 +1164,7 @@ int hci_dev_open(__u16 dev)
atomic_set(&hdev->cmd_cnt, 1);
set_bit(HCI_INIT, &hdev->flags);
- if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags))
+ if (test_and_clear_bit(HCI_SETUP, &hdev->dev_flags) && hdev->setup)
ret = hdev->setup(hdev);
if (!ret) {
@@ -1581,10 +1581,13 @@ static const struct rfkill_ops hci_rfkill_ops = {
static void hci_power_on(struct work_struct *work)
{
struct hci_dev *hdev = container_of(work, struct hci_dev, power_on);
+ bool setup;
int err;
BT_DBG("%s", hdev->name);
+ setup = test_bit(HCI_SETUP, &hdev->dev_flags);
+
err = hci_dev_open(hdev->id);
if (err < 0) {
mgmt_set_powered_failed(hdev, err);
@@ -1595,7 +1598,7 @@ static void hci_power_on(struct work_struct *work)
queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
HCI_AUTO_OFF_TIMEOUT);
- if (test_and_clear_bit(HCI_SETUP, &hdev->dev_flags))
+ if (setup)
mgmt_index_added(hdev);
}
