* [PATCH 0/3 v1] Fix stop discovery not handled if discovery state is STARTING
@ 2012-12-06 11:05 Jaganath Kanakkassery
2012-12-06 11:05 ` [PATCH 1/3 v1] Bluetooth: Rename stop_discovery_failed() to stop_discovery_complete() Jaganath Kanakkassery
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jaganath Kanakkassery @ 2012-12-06 11:05 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jaganath Kanakkassery
If user calls stop_discovery when the discovery state is STARTING, then it fails.
In this scenario discovery will continue.
This patch handles stop_discovery even if state is STARTING
Jaganath Kanakkassery (3):
Bluetooth: Rename stop_discovery_failed() to
stop_discovery_complete()
Bluetooth: Move discovery state check inside hci_dev_lock()
Bluetooth: Fix stop discovery while in STARTING state
include/net/bluetooth/hci_core.h | 3 ++-
net/bluetooth/hci_core.c | 9 ++++++++-
net/bluetooth/hci_event.c | 28 +++++++++++++++++++---------
net/bluetooth/mgmt.c | 15 ++++++++++-----
4 files changed, 39 insertions(+), 16 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/3 v1] Bluetooth: Rename stop_discovery_failed() to stop_discovery_complete() 2012-12-06 11:05 [PATCH 0/3 v1] Fix stop discovery not handled if discovery state is STARTING Jaganath Kanakkassery @ 2012-12-06 11:05 ` Jaganath Kanakkassery 2012-12-06 11:05 ` [PATCH 2/3 v1] Bluetooth: Move discovery state check inside hci_dev_lock() Jaganath Kanakkassery 2012-12-06 11:05 ` [PATCH 3/3 v1] Bluetooth: Fix stop discovery while in STARTING state Jaganath Kanakkassery 2 siblings, 0 replies; 7+ messages in thread From: Jaganath Kanakkassery @ 2012-12-06 11:05 UTC (permalink / raw) To: linux-bluetooth; +Cc: Jaganath Kanakkassery This renaming has done since in success case also this function can be used to send complete event Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com> --- include/net/bluetooth/hci_core.h | 2 +- net/bluetooth/hci_event.c | 4 ++-- net/bluetooth/mgmt.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 119fcb6..bd63a9f 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -1066,7 +1066,7 @@ int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type, int mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type, u8 addr_type, s8 rssi, u8 *name, u8 name_len); int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status); -int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status); +int mgmt_stop_discovery_complete(struct hci_dev *hdev, u8 status); int mgmt_discovering(struct hci_dev *hdev, u8 discovering); int mgmt_interleaved_discovery(struct hci_dev *hdev); int mgmt_device_blocked(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type); diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 9fb656b..e859851 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -40,7 +40,7 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb) if (status) { hci_dev_lock(hdev); - mgmt_stop_discovery_failed(hdev, status); + mgmt_stop_discovery_complete(hdev, status); hci_dev_unlock(hdev); return; } @@ -1099,7 +1099,7 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev, case LE_SCANNING_DISABLED: if (status) { hci_dev_lock(hdev); - mgmt_stop_discovery_failed(hdev, status); + mgmt_stop_discovery_complete(hdev, status); hci_dev_unlock(hdev); return; } diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index dc5698c..b4bba68 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c @@ -3630,7 +3630,7 @@ int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status) return err; } -int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status) +int mgmt_stop_discovery_complete(struct hci_dev *hdev, u8 status) { struct pending_cmd *cmd; int err; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3 v1] Bluetooth: Move discovery state check inside hci_dev_lock() 2012-12-06 11:05 [PATCH 0/3 v1] Fix stop discovery not handled if discovery state is STARTING Jaganath Kanakkassery 2012-12-06 11:05 ` [PATCH 1/3 v1] Bluetooth: Rename stop_discovery_failed() to stop_discovery_complete() Jaganath Kanakkassery @ 2012-12-06 11:05 ` Jaganath Kanakkassery 2012-12-11 22:17 ` Gustavo Padovan 2012-12-06 11:05 ` [PATCH 3/3 v1] Bluetooth: Fix stop discovery while in STARTING state Jaganath Kanakkassery 2 siblings, 1 reply; 7+ messages in thread From: Jaganath Kanakkassery @ 2012-12-06 11:05 UTC (permalink / raw) To: linux-bluetooth; +Cc: Jaganath Kanakkassery Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com> --- net/bluetooth/hci_event.c | 9 ++++----- net/bluetooth/mgmt.c | 4 ---- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index e859851..1fadba5 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -1106,14 +1106,13 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev, clear_bit(HCI_LE_SCAN, &hdev->dev_flags); + hci_dev_lock(hdev); if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED && - hdev->discovery.state == DISCOVERY_FINDING) { + hdev->discovery.state == DISCOVERY_FINDING) mgmt_interleaved_discovery(hdev); - } else { - hci_dev_lock(hdev); + else hci_discovery_set_state(hdev, DISCOVERY_STOPPED); - hci_dev_unlock(hdev); - } + hci_dev_unlock(hdev); break; diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index b4bba68..332152b 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c @@ -2291,14 +2291,10 @@ int mgmt_interleaved_discovery(struct hci_dev *hdev) BT_DBG("%s", hdev->name); - hci_dev_lock(hdev); - err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR_LE); if (err < 0) hci_discovery_set_state(hdev, DISCOVERY_STOPPED); - hci_dev_unlock(hdev); - return err; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3 v1] Bluetooth: Move discovery state check inside hci_dev_lock() 2012-12-06 11:05 ` [PATCH 2/3 v1] Bluetooth: Move discovery state check inside hci_dev_lock() Jaganath Kanakkassery @ 2012-12-11 22:17 ` Gustavo Padovan 0 siblings, 0 replies; 7+ messages in thread From: Gustavo Padovan @ 2012-12-11 22:17 UTC (permalink / raw) To: Jaganath Kanakkassery; +Cc: linux-bluetooth Hi Jaganath, * Jaganath Kanakkassery <jaganath.k@samsung.com> [2012-12-06 16:35:57 +0530]: > Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com> > --- > net/bluetooth/hci_event.c | 9 ++++----- > net/bluetooth/mgmt.c | 4 ---- > 2 files changed, 4 insertions(+), 9 deletions(-) Please add a proper commit message. We do not allow empty commit messages, even if the commit is trivial. Gustavo ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3 v1] Bluetooth: Fix stop discovery while in STARTING state 2012-12-06 11:05 [PATCH 0/3 v1] Fix stop discovery not handled if discovery state is STARTING Jaganath Kanakkassery 2012-12-06 11:05 ` [PATCH 1/3 v1] Bluetooth: Rename stop_discovery_failed() to stop_discovery_complete() Jaganath Kanakkassery 2012-12-06 11:05 ` [PATCH 2/3 v1] Bluetooth: Move discovery state check inside hci_dev_lock() Jaganath Kanakkassery @ 2012-12-06 11:05 ` Jaganath Kanakkassery 2012-12-11 22:15 ` Gustavo Padovan 2 siblings, 1 reply; 7+ messages in thread From: Jaganath Kanakkassery @ 2012-12-06 11:05 UTC (permalink / raw) To: linux-bluetooth; +Cc: Jaganath Kanakkassery If stop_discovery() is called when discovery state is STARTING, it will be failed currently. This patch fixes this. Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com> --- include/net/bluetooth/hci_core.h | 1 + net/bluetooth/hci_core.c | 9 ++++++++- net/bluetooth/hci_event.c | 15 +++++++++++++-- net/bluetooth/mgmt.c | 9 +++++++++ 4 files changed, 31 insertions(+), 3 deletions(-) diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index bd63a9f..d09777a 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -64,6 +64,7 @@ struct discovery_state { DISCOVERY_RESOLVING, DISCOVERY_STOPPING, } state; + int prev_state; struct list_head all; /* All devices found during inquiry */ struct list_head unknown; /* Name state not known */ struct list_head resolve; /* Name needs to be resolved */ diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index c6c67b2..93d0261 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -391,7 +391,13 @@ void hci_discovery_set_state(struct hci_dev *hdev, int state) switch (state) { case DISCOVERY_STOPPED: - if (hdev->discovery.state != DISCOVERY_STARTING) + if (hdev->discovery.state == DISCOVERY_STARTING) + break; + + if (hdev->discovery.state == DISCOVERY_STOPPING && + hdev->discovery.prev_state == DISCOVERY_STARTING) + mgmt_stop_discovery_complete(hdev, 0); + else mgmt_discovering(hdev, 0); break; case DISCOVERY_STARTING: @@ -405,6 +411,7 @@ void hci_discovery_set_state(struct hci_dev *hdev, int state) break; } + hdev->discovery.prev_state = hdev->discovery.state; hdev->discovery.state = state; } diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 1fadba5..3f9f317 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -1085,6 +1085,8 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev, if (status) { hci_dev_lock(hdev); mgmt_start_discovery_failed(hdev, status); + if (hdev->discovery.state == DISCOVERY_STOPPING) + mgmt_stop_discovery_complete(hdev, status); hci_dev_unlock(hdev); return; } @@ -1092,7 +1094,10 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev, set_bit(HCI_LE_SCAN, &hdev->dev_flags); hci_dev_lock(hdev); - hci_discovery_set_state(hdev, DISCOVERY_FINDING); + if (hdev->discovery.state == DISCOVERY_STOPPING) + hci_cancel_le_scan(hdev); + else + hci_discovery_set_state(hdev, DISCOVERY_FINDING); hci_dev_unlock(hdev); break; @@ -1180,8 +1185,11 @@ static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status) hci_req_complete(hdev, HCI_OP_INQUIRY, status); hci_conn_check_pending(hdev); hci_dev_lock(hdev); - if (test_bit(HCI_MGMT, &hdev->dev_flags)) + if (test_bit(HCI_MGMT, &hdev->dev_flags)) { mgmt_start_discovery_failed(hdev, status); + if (hdev->discovery.state == DISCOVERY_STOPPING) + mgmt_stop_discovery_complete(hdev, status); + } hci_dev_unlock(hdev); return; } @@ -1189,6 +1197,9 @@ static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status) set_bit(HCI_INQUIRY, &hdev->flags); hci_dev_lock(hdev); + if (hdev->discovery.state == DISCOVERY_STOPPING) + hci_cancel_inquiry(hdev); + else hci_discovery_set_state(hdev, DISCOVERY_FINDING); hci_dev_unlock(hdev); } diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index 332152b..a645494 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c @@ -2387,6 +2387,11 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data, hci_dev_lock(hdev); + if (hdev->discovery.state == DISCOVERY_STARTING) { + err = 0; + goto proceed; + } + if (!hci_discovery_active(hdev)) { err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY, MGMT_STATUS_REJECTED, &mgmt_cp->type, @@ -2401,6 +2406,7 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data, goto unlock; } +proceed: cmd = mgmt_pending_add(sk, MGMT_OP_STOP_DISCOVERY, hdev, NULL, 0); if (!cmd) { err = -ENOMEM; @@ -2435,6 +2441,9 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data, break; + case DISCOVERY_STARTING: + break; + default: BT_DBG("unknown discovery state %u", hdev->discovery.state); err = -EFAULT; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3 v1] Bluetooth: Fix stop discovery while in STARTING state 2012-12-06 11:05 ` [PATCH 3/3 v1] Bluetooth: Fix stop discovery while in STARTING state Jaganath Kanakkassery @ 2012-12-11 22:15 ` Gustavo Padovan 2012-12-18 13:27 ` Jaganath Kanakkassery 0 siblings, 1 reply; 7+ messages in thread From: Gustavo Padovan @ 2012-12-11 22:15 UTC (permalink / raw) To: Jaganath Kanakkassery; +Cc: linux-bluetooth Hi Jaganath, * Jaganath Kanakkassery <jaganath.k@samsung.com> [2012-12-06 16:35:58 +0530]: > If stop_discovery() is called when discovery state is STARTING, it > will be failed currently. This patch fixes this. > > Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com> > --- > include/net/bluetooth/hci_core.h | 1 + > net/bluetooth/hci_core.c | 9 ++++++++- > net/bluetooth/hci_event.c | 15 +++++++++++++-- > net/bluetooth/mgmt.c | 9 +++++++++ > 4 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index bd63a9f..d09777a 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -64,6 +64,7 @@ struct discovery_state { > DISCOVERY_RESOLVING, > DISCOVERY_STOPPING, > } state; > + int prev_state; > struct list_head all; /* All devices found during inquiry */ > struct list_head unknown; /* Name state not known */ > struct list_head resolve; /* Name needs to be resolved */ > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index c6c67b2..93d0261 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -391,7 +391,13 @@ void hci_discovery_set_state(struct hci_dev *hdev, int state) > > switch (state) { > case DISCOVERY_STOPPED: > - if (hdev->discovery.state != DISCOVERY_STARTING) > + if (hdev->discovery.state == DISCOVERY_STARTING) > + break; > + > + if (hdev->discovery.state == DISCOVERY_STOPPING && > + hdev->discovery.prev_state == DISCOVERY_STARTING) > + mgmt_stop_discovery_complete(hdev, 0); > + else > mgmt_discovering(hdev, 0); I don't think we need to be this complicated here, I don't think that add discovery.prev_state is a good a idea. I would prefer if you add discovery.discovering that will tell if we are discovering or not, then check inside mgmt_discovering() if the value is the same and return right away. You would just need to add the mgmt_stop_discovery_complete() in case state is DISCOVERY_STOPPING. > break; > case DISCOVERY_STARTING: > @@ -405,6 +411,7 @@ void hci_discovery_set_state(struct hci_dev *hdev, int state) > break; > } > > + hdev->discovery.prev_state = hdev->discovery.state; > hdev->discovery.state = state; > } > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index 1fadba5..3f9f317 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -1085,6 +1085,8 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev, > if (status) { > hci_dev_lock(hdev); > mgmt_start_discovery_failed(hdev, status); > + if (hdev->discovery.state == DISCOVERY_STOPPING) > + mgmt_stop_discovery_complete(hdev, status); I think this case will be handled by hci_discovery_set_state(), and you could just remove this from here. And why you return the set_scan_enable status error to the stop discovery command? > hci_dev_unlock(hdev); > return; > } > @@ -1092,7 +1094,10 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev, > set_bit(HCI_LE_SCAN, &hdev->dev_flags); > > hci_dev_lock(hdev); > - hci_discovery_set_state(hdev, DISCOVERY_FINDING); > + if (hdev->discovery.state == DISCOVERY_STOPPING) > + hci_cancel_le_scan(hdev); > + else > + hci_discovery_set_state(hdev, DISCOVERY_FINDING); > hci_dev_unlock(hdev); > break; > > @@ -1180,8 +1185,11 @@ static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status) > hci_req_complete(hdev, HCI_OP_INQUIRY, status); > hci_conn_check_pending(hdev); > hci_dev_lock(hdev); > - if (test_bit(HCI_MGMT, &hdev->dev_flags)) > + if (test_bit(HCI_MGMT, &hdev->dev_flags)) { > mgmt_start_discovery_failed(hdev, status); > + if (hdev->discovery.state == DISCOVERY_STOPPING) > + mgmt_stop_discovery_complete(hdev, status); > + } Saome thing as the previous comment, you don't need this. mgmt_start_discovery_failed() calls hci_discovery_set_state(). > hci_dev_unlock(hdev); > return; > } > @@ -1189,6 +1197,9 @@ static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status) > set_bit(HCI_INQUIRY, &hdev->flags); > > hci_dev_lock(hdev); > + if (hdev->discovery.state == DISCOVERY_STOPPING) > + hci_cancel_inquiry(hdev); > + else > hci_discovery_set_state(hdev, DISCOVERY_FINDING); Wrong indentation level inside the else. > hci_dev_unlock(hdev); > } > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c > index 332152b..a645494 100644 > --- a/net/bluetooth/mgmt.c > +++ b/net/bluetooth/mgmt.c > @@ -2387,6 +2387,11 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data, > > hci_dev_lock(hdev); > > + if (hdev->discovery.state == DISCOVERY_STARTING) { > + err = 0; > + goto proceed; > + } > + > if (!hci_discovery_active(hdev)) { I would do something like this instead: if (hdev->discovery.state != DISCOVERY_STARTING && !hci_discovery_active(hdev)) { And remove the label jump from here... > err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY, > MGMT_STATUS_REJECTED, &mgmt_cp->type, > @@ -2401,6 +2406,7 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data, > goto unlock; > } > > +proceed: > cmd = mgmt_pending_add(sk, MGMT_OP_STOP_DISCOVERY, hdev, NULL, 0); > if (!cmd) { > err = -ENOMEM; > @@ -2435,6 +2441,9 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data, > > break; > > + case DISCOVERY_STARTING: and add err = 0 here. Gustavo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3 v1] Bluetooth: Fix stop discovery while in STARTING state 2012-12-11 22:15 ` Gustavo Padovan @ 2012-12-18 13:27 ` Jaganath Kanakkassery 0 siblings, 0 replies; 7+ messages in thread From: Jaganath Kanakkassery @ 2012-12-18 13:27 UTC (permalink / raw) To: Gustavo Padovan; +Cc: linux-bluetooth Hi Gustavo, -------------------------------------------------- From: "Gustavo Padovan" <gustavo@padovan.org> Sent: Wednesday, December 12, 2012 3:45 AM To: "Jaganath Kanakkassery" <jaganath.k@samsung.com> Cc: <linux-bluetooth@vger.kernel.org> Subject: Re: [PATCH 3/3 v1] Bluetooth: Fix stop discovery while in STARTING state > Hi Jaganath, > > * Jaganath Kanakkassery <jaganath.k@samsung.com> [2012-12-06 16:35:58 > +0530]: > >> If stop_discovery() is called when discovery state is STARTING, it >> will be failed currently. This patch fixes this. >> >> Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com> >> --- >> include/net/bluetooth/hci_core.h | 1 + >> net/bluetooth/hci_core.c | 9 ++++++++- >> net/bluetooth/hci_event.c | 15 +++++++++++++-- >> net/bluetooth/mgmt.c | 9 +++++++++ >> 4 files changed, 31 insertions(+), 3 deletions(-) >> >> diff --git a/include/net/bluetooth/hci_core.h >> b/include/net/bluetooth/hci_core.h >> index bd63a9f..d09777a 100644 >> --- a/include/net/bluetooth/hci_core.h >> +++ b/include/net/bluetooth/hci_core.h >> @@ -64,6 +64,7 @@ struct discovery_state { >> DISCOVERY_RESOLVING, >> DISCOVERY_STOPPING, >> } state; >> + int prev_state; >> struct list_head all; /* All devices found during inquiry */ >> struct list_head unknown; /* Name state not known */ >> struct list_head resolve; /* Name needs to be resolved */ >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c >> index c6c67b2..93d0261 100644 >> --- a/net/bluetooth/hci_core.c >> +++ b/net/bluetooth/hci_core.c >> @@ -391,7 +391,13 @@ void hci_discovery_set_state(struct hci_dev *hdev, >> int state) >> >> switch (state) { >> case DISCOVERY_STOPPED: >> - if (hdev->discovery.state != DISCOVERY_STARTING) >> + if (hdev->discovery.state == DISCOVERY_STARTING) >> + break; >> + >> + if (hdev->discovery.state == DISCOVERY_STOPPING && >> + hdev->discovery.prev_state == DISCOVERY_STARTING) >> + mgmt_stop_discovery_complete(hdev, 0); >> + else >> mgmt_discovering(hdev, 0); > > I don't think we need to be this complicated here, I don't think that add > discovery.prev_state is a good a idea. I would prefer if you add > discovery.discovering that will tell if we are discovering or not, then > check > inside mgmt_discovering() if the value is the same and return right away. > You would just need to add the mgmt_stop_discovery_complete() in case > state is > DISCOVERY_STOPPING. > I think in this case no need to call mgmt_stop_discovery_complete() explicitly. mgmt_discovering() with discovering 0 will send stop_discovery_complete(). So after sending command complete we can check the value of discovering and return. But one thing I see in this scenario is, when we should send start_discovery_complete()? What I think is in SCAN_ENABLED event if state is STOPPING we can send start_discovery_complete with MGMT_STATUS _CANCELLED error code. What do you think? >> break; >> case DISCOVERY_STARTING: >> @@ -405,6 +411,7 @@ void hci_discovery_set_state(struct hci_dev *hdev, >> int state) >> break; >> } >> >> + hdev->discovery.prev_state = hdev->discovery.state; >> hdev->discovery.state = state; >> } >> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c >> index 1fadba5..3f9f317 100644 >> --- a/net/bluetooth/hci_event.c >> +++ b/net/bluetooth/hci_event.c >> @@ -1085,6 +1085,8 @@ static void hci_cc_le_set_scan_enable(struct >> hci_dev *hdev, >> if (status) { >> hci_dev_lock(hdev); >> mgmt_start_discovery_failed(hdev, status); >> + if (hdev->discovery.state == DISCOVERY_STOPPING) >> + mgmt_stop_discovery_complete(hdev, status); > > I think this case will be handled by hci_discovery_set_state(), and you > could > just remove this from here. And why you return the set_scan_enable status > error to the stop discovery command? > Ok >> hci_dev_unlock(hdev); >> return; >> } >> @@ -1092,7 +1094,10 @@ static void hci_cc_le_set_scan_enable(struct >> hci_dev *hdev, >> set_bit(HCI_LE_SCAN, &hdev->dev_flags); >> >> hci_dev_lock(hdev); >> - hci_discovery_set_state(hdev, DISCOVERY_FINDING); >> + if (hdev->discovery.state == DISCOVERY_STOPPING) >> + hci_cancel_le_scan(hdev); >> + else >> + hci_discovery_set_state(hdev, DISCOVERY_FINDING); >> hci_dev_unlock(hdev); >> break; >> >> @@ -1180,8 +1185,11 @@ static void hci_cs_inquiry(struct hci_dev *hdev, >> __u8 status) >> hci_req_complete(hdev, HCI_OP_INQUIRY, status); >> hci_conn_check_pending(hdev); >> hci_dev_lock(hdev); >> - if (test_bit(HCI_MGMT, &hdev->dev_flags)) >> + if (test_bit(HCI_MGMT, &hdev->dev_flags)) { >> mgmt_start_discovery_failed(hdev, status); >> + if (hdev->discovery.state == DISCOVERY_STOPPING) >> + mgmt_stop_discovery_complete(hdev, status); >> + } > > Saome thing as the previous comment, you don't need this. > mgmt_start_discovery_failed() calls hci_discovery_set_state(). > Ok >> hci_dev_unlock(hdev); >> return; >> } >> @@ -1189,6 +1197,9 @@ static void hci_cs_inquiry(struct hci_dev *hdev, >> __u8 status) >> set_bit(HCI_INQUIRY, &hdev->flags); >> >> hci_dev_lock(hdev); >> + if (hdev->discovery.state == DISCOVERY_STOPPING) >> + hci_cancel_inquiry(hdev); >> + else >> hci_discovery_set_state(hdev, DISCOVERY_FINDING); > > Wrong indentation level inside the else. > Will correct in the next patch. >> hci_dev_unlock(hdev); >> } >> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c >> index 332152b..a645494 100644 >> --- a/net/bluetooth/mgmt.c >> +++ b/net/bluetooth/mgmt.c >> @@ -2387,6 +2387,11 @@ static int stop_discovery(struct sock *sk, struct >> hci_dev *hdev, void *data, >> >> hci_dev_lock(hdev); >> >> + if (hdev->discovery.state == DISCOVERY_STARTING) { >> + err = 0; >> + goto proceed; >> + } >> + >> if (!hci_discovery_active(hdev)) { > > I would do something like this instead: > > if (hdev->discovery.state != DISCOVERY_STARTING && > !hci_discovery_active(hdev)) { > > > And remove the label jump from here... > Ok >> err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY, >> MGMT_STATUS_REJECTED, &mgmt_cp->type, >> @@ -2401,6 +2406,7 @@ static int stop_discovery(struct sock *sk, struct >> hci_dev *hdev, void *data, >> goto unlock; >> } >> >> +proceed: >> cmd = mgmt_pending_add(sk, MGMT_OP_STOP_DISCOVERY, hdev, NULL, 0); >> if (!cmd) { >> err = -ENOMEM; >> @@ -2435,6 +2441,9 @@ static int stop_discovery(struct sock *sk, struct >> hci_dev *hdev, void *data, >> >> break; >> >> + case DISCOVERY_STARTING: > > and add err = 0 here. > Ok Thanks, Jaganath ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-12-18 13:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-06 11:05 [PATCH 0/3 v1] Fix stop discovery not handled if discovery state is STARTING Jaganath Kanakkassery 2012-12-06 11:05 ` [PATCH 1/3 v1] Bluetooth: Rename stop_discovery_failed() to stop_discovery_complete() Jaganath Kanakkassery 2012-12-06 11:05 ` [PATCH 2/3 v1] Bluetooth: Move discovery state check inside hci_dev_lock() Jaganath Kanakkassery 2012-12-11 22:17 ` Gustavo Padovan 2012-12-06 11:05 ` [PATCH 3/3 v1] Bluetooth: Fix stop discovery while in STARTING state Jaganath Kanakkassery 2012-12-11 22:15 ` Gustavo Padovan 2012-12-18 13:27 ` Jaganath Kanakkassery
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).