Some HCI commands don't send a Command Complete Event once the HCI command has
completed so they require special handling by the HCI request framework. HCI
Inquiry command is one of those. The HCI Inquiry command causes the controller
to enter in inquiry mode. A successful Command Status Event for this command
means the the controller is running the inquiry procedure therefore the HCI
Inquiry command has completed. The Inquiry Complete Event, on the other hand,
means the inquiry _procedure_ (not the HCI command) has completed.
For that reason, the HCI request framework should consider the HCI Inquiry
command has completed once the host receives the Command Status Event, instead
of the Inquiry Complete Event.
Additionally, the way inquiry is supported by HCI request framework today, we
cannot use HCI requests to properly implement the BR/EDR neither interleaved
discovery since the request complete callback will be called when the inquiry
procedure finishes, not when the inquiry procedure starts as expected.
This change in HCI request framework have an affect on inquiry ioctl. Since
hci_req_sync will block until the HCI inquiry command completes (not until the
procedure finishes anymore), we need to sleep the user process until the
inquiry procedure finishes.
Patches summary:
* Patch 1 changes the HCI request framework so the HCI Inquiry request
completes once the Command Status Event is received.
* Patch 2 fixes inquiry ioctl.
Both patches of this patch set must be applied together, otherwise the inquiry
ioctl will be broken.
Regards,
Andre
Andre Guedes (2):
Bluetooth: Fix HCI request for Inquiry command
Bluetooth: Fix hci_inquiry
net/bluetooth/hci_core.c | 13 +++++++++++++
net/bluetooth/hci_event.c | 11 +++++++----
2 files changed, 20 insertions(+), 4 deletions(-)
--
1.8.1.2
Hi Andre,
On Tue, Mar 26, 2013, Andre Guedes wrote:
> > On Wed, Mar 20, 2013, Andre Guedes wrote:
> >> HCI Inquiry command causes the controller to enter in inquiry mode. A
> >> successful Command Status Event for this command means the the
> >> controller is running the inquiry procedure therefore the HCI Inquiry
> >> command has completed. The Inquiry Complete Event, on the other hand,
> >> means the inquiry _procedure_ (not the HCI command) has completed.
> >
> > It would be good if you could show exact quotes from the core
> > specification instead of making stuff up. Here's an exact quote from the
> > core spec (page 706) regarding events generated by the Inquiry command:
> >
> > "No Command Complete event will be sent by the BR/EDR Controller to
> > indicate that this command has been completed. Instead, the Inquiry
> > Complete event will indicate that this command has been completed."
> >
> > That's quite the opposite of what you're claiming, with no references to
> > the procedure completion but only the command completion. This makes me
> > feel like you're trying to persuade us to a particular technical outcome
> > that suits your needs with the help of misleading/inaccurate
> > information.
> >
> > It might well be in our best interest to keep processing our
> > request/command queue based on command status events (essentially
> > treating the commands as completed) but let's at least be honest about
> > what the exact wording in the core specification is.
>
> Here go the quotes about the inquiry procedure you requested (they are
> on the same page you cited, 706):
> "A Command Status event shall be sent from the BR/EDR Controller to
> the Host when the BR/EDR Controller has started the Inquiry process."
> [...]
> "An Inquiry Complete event shall be generated when the Inquiry process
> has completed."
>
> Also, on Command Status Event description, on page 979, we have:
> "The Command Status event is used to indicate that the command
> described by the Command_Opcode parameter has been received, and that
> the Controller is currently performing the task for this command. This
> event is needed to provide mechanisms for asynchronous operation,
> which makes it possible to prevent the Host from waiting for a command
> to finish."
>
> So, based on those statements of the Core specification, it makes
> sense to me the HCI request framework should consider the HCI Inquiry
> command has completed once the Command Status Event is received.
Yes. I think the main disagreement here was about the wording. From the
perspective of our request processing logic the commands should be
treated as if they are completed (even though they from the HCI
specification perspective are not). In your original message you made it
sound like even from the HCI specification perspective the commands
really are completed as soon as the cmd_status comes.
Anyway, I'm looking forward to a renewed set that removes the
hci_req_cmd_status() function. Be sure to run mgmt-tester, hcitool
scan/inq, and bluetoothd-based discovery tests and anything else you
think is relevant before sending the new patches.
Johan
Hi Johan,
On Fri, Mar 22, 2013 at 8:09 AM, Johan Hedberg <[email protected]> wrote:
>
> Hi Andre,
>
> On Wed, Mar 20, 2013, Andre Guedes wrote:
> > @@ -1079,6 +1077,8 @@ static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
> > {
> > BT_DBG("%s status 0x%2.2x", hdev->name, status);
> >
> > + hci_req_cmd_complete(hdev, HCI_OP_INQUIRY, status);
> > +
> > if (status) {
> > hci_conn_check_pending(hdev);
> > hci_dev_lock(hdev);
>
> What about the hci_req_cmd_status call in the hci_cmd_status_evt
> function? hci_req_cmd_status could call hci_req_cmd_complete in which
> case you'd get two calls to that function for the same event.
You are right. In case "status" is not zero, the callback function
will be called twice. This hci_req_cmd_complete call should go at the
end of hci_cs_inquiry, after the status check.
> I have a feeling that if you really want to consider all commands as
> completed based on command status events we should just skip the whole
> hci_req_cmd_status/complete distinction and remove the
> hci_req_cmd_status function completely (always calling cmd_complete).
I don't see any problem with this approach you suggested. If nobody
else is against this, I can do this change in the next version of this
patch set.
Regards,
Andre
Hi Johan,
On Fri, Mar 22, 2013 at 8:20 AM, Johan Hedberg <[email protected]> wrote:
> Hi Andre,
>
> On Wed, Mar 20, 2013, Andre Guedes wrote:
>> HCI Inquiry command causes the controller to enter in inquiry mode. A
>> successful Command Status Event for this command means the the
>> controller is running the inquiry procedure therefore the HCI Inquiry
>> command has completed. The Inquiry Complete Event, on the other hand,
>> means the inquiry _procedure_ (not the HCI command) has completed.
>
> It would be good if you could show exact quotes from the core
> specification instead of making stuff up. Here's an exact quote from the
> core spec (page 706) regarding events generated by the Inquiry command:
>
> "No Command Complete event will be sent by the BR/EDR Controller to
> indicate that this command has been completed. Instead, the Inquiry
> Complete event will indicate that this command has been completed."
>
> That's quite the opposite of what you're claiming, with no references to
> the procedure completion but only the command completion. This makes me
> feel like you're trying to persuade us to a particular technical outcome
> that suits your needs with the help of misleading/inaccurate
> information.
>
> It might well be in our best interest to keep processing our
> request/command queue based on command status events (essentially
> treating the commands as completed) but let's at least be honest about
> what the exact wording in the core specification is.
Here go the quotes about the inquiry procedure you requested (they are
on the same page you cited, 706):
"A Command Status event shall be sent from the BR/EDR Controller to
the Host when the BR/EDR Controller has started the Inquiry process."
[...]
"An Inquiry Complete event shall be generated when the Inquiry process
has completed."
Also, on Command Status Event description, on page 979, we have:
"The Command Status event is used to indicate that the command
described by the Command_Opcode parameter has been received, and that
the Controller is currently performing the task for this command. This
event is needed to provide mechanisms for asynchronous operation,
which makes it possible to prevent the Host from waiting for a command
to finish."
So, based on those statements of the Core specification, it makes
sense to me the HCI request framework should consider the HCI Inquiry
command has completed once the Command Status Event is received.
Besides that, calling the complete callback in Inquiry Complete Event
seems broken. This only works if the HCI request has only the Inquiry
command. If we have a HCI request with two HCI commands (inquiry and
any other command), the second HCI command will be sent to the
controller even if the first command has not completed yet.
Additionally, when the second command completes, the complete callback
will be called, but the first command has not completed yet.
This later issue can lead us to situations where the complete callback
is called with success status (the second command returns success),
but the Inquiry Complete Event returns error later.
Regards,
Andre
Hi Andre,
On Wed, Mar 20, 2013, Andre Guedes wrote:
> HCI Inquiry command causes the controller to enter in inquiry mode. A
> successful Command Status Event for this command means the the
> controller is running the inquiry procedure therefore the HCI Inquiry
> command has completed. The Inquiry Complete Event, on the other hand,
> means the inquiry _procedure_ (not the HCI command) has completed.
It would be good if you could show exact quotes from the core
specification instead of making stuff up. Here's an exact quote from the
core spec (page 706) regarding events generated by the Inquiry command:
"No Command Complete event will be sent by the BR/EDR Controller to
indicate that this command has been completed. Instead, the Inquiry
Complete event will indicate that this command has been completed."
That's quite the opposite of what you're claiming, with no references to
the procedure completion but only the command completion. This makes me
feel like you're trying to persuade us to a particular technical outcome
that suits your needs with the help of misleading/inaccurate
information.
It might well be in our best interest to keep processing our
request/command queue based on command status events (essentially
treating the commands as completed) but let's at least be honest about
what the exact wording in the core specification is.
Johan
Hi Andre,
On Wed, Mar 20, 2013, Andre Guedes wrote:
> @@ -1079,6 +1077,8 @@ static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
> {
> BT_DBG("%s status 0x%2.2x", hdev->name, status);
>
> + hci_req_cmd_complete(hdev, HCI_OP_INQUIRY, status);
> +
> if (status) {
> hci_conn_check_pending(hdev);
> hci_dev_lock(hdev);
What about the hci_req_cmd_status call in the hci_cmd_status_evt
function? hci_req_cmd_status could call hci_req_cmd_complete in which
case you'd get two calls to that function for the same event.
I have a feeling that if you really want to consider all commands as
completed based on command status events we should just skip the whole
hci_req_cmd_status/complete distinction and remove the
hci_req_cmd_status function completely (always calling cmd_complete).
Johan
Since the HCI request framework was properly fixed to handle HCI
Inquiry commands, the hci_req_sync call, in hci_inquiry, will return
as soon as the HCI command completes (not the Inquiry procedure).
However, in inquiry ioctl implementation, we want to sleep the user
process until the inquiry procedure finishes.
This patch changes hci_inquiry so, in case the HCI Inquiry command
was executed successfully, it waits the HCI_INQUIRY flag to be cleared.
This way, the user process will sleep until the inquiry procedure
finishes.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/hci_core.c | 13 +++++++++++++
net/bluetooth/hci_event.c | 5 +++++
2 files changed, 18 insertions(+)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index cfcad54..1239929 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -818,6 +818,12 @@ static void hci_inq_req(struct hci_request *req, unsigned long opt)
hci_req_add(req, HCI_OP_INQUIRY, sizeof(cp), &cp);
}
+static int wait_inquiry(void *word)
+{
+ schedule();
+ return signal_pending(current);
+}
+
int hci_inquiry(void __user *arg)
{
__u8 __user *ptr = arg;
@@ -849,6 +855,13 @@ int hci_inquiry(void __user *arg)
timeo);
if (err < 0)
goto done;
+
+ /* Wait until Inquiry procedure finishes (HCI_INQUIRY flag is
+ * cleared). If it is interrupted by a signal, return -EINTR.
+ */
+ if (wait_on_bit(&hdev->flags, HCI_INQUIRY, wait_inquiry,
+ TASK_INTERRUPTIBLE))
+ return -EINTR;
}
/* for unlimited number of responses we will use buffer with
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index b340335..85b42b2 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -48,6 +48,8 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
}
clear_bit(HCI_INQUIRY, &hdev->flags);
+ smp_mb__after_clear_bit(); /* wake_up_bit advises about this barrier */
+ wake_up_bit(&hdev->flags, HCI_INQUIRY);
hci_dev_lock(hdev);
hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
@@ -1605,6 +1607,9 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
return;
+ smp_mb__after_clear_bit(); /* wake_up_bit advises about this barrier */
+ wake_up_bit(&hdev->flags, HCI_INQUIRY);
+
if (!test_bit(HCI_MGMT, &hdev->dev_flags))
return;
--
1.8.1.2
Some HCI commands don't send a Command Complete Event once the
HCI command has completed so they require special handling by
the HCI request framework. HCI Inquiry command is one of those.
The HCI Inquiry command causes the controller to enter in inquiry
mode. A successful Command Status Event for this command means the
the controller is running the inquiry procedure therefore the HCI
Inquiry command has completed.
For that reason, the HCI request framework should consider the
HCI Inquiry command has completed once the host receives the
Command Status Event, instead of the Inquiry Complete Event. The
Inquiry Complete Event means the inquiry procedure has completed
(not the inquiry command).
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/hci_event.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 1385807..b340335 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -53,8 +53,6 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
hci_dev_unlock(hdev);
- hci_req_cmd_complete(hdev, HCI_OP_INQUIRY, status);
-
hci_conn_check_pending(hdev);
}
@@ -1079,6 +1077,8 @@ static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
{
BT_DBG("%s status 0x%2.2x", hdev->name, status);
+ hci_req_cmd_complete(hdev, HCI_OP_INQUIRY, status);
+
if (status) {
hci_conn_check_pending(hdev);
hci_dev_lock(hdev);
@@ -1600,8 +1600,6 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
BT_DBG("%s status 0x%2.2x", hdev->name, status);
- hci_req_cmd_complete(hdev, HCI_OP_INQUIRY, status);
-
hci_conn_check_pending(hdev);
if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
--
1.8.1.2