Hi Gustavo & Andrzej,
On 3/16/2011 2:30 PM, Gustavo F. Padovan wrote:
Hi Andrzej,
* Brian Gix<bgix@xxxxxxxxxxxxxx> [2011-03-04 08:12:45 -0800]:
Hi Andrzej,
On 3/4/2011 4:37 AM, Andrzej Kaczmarek wrote:
Hi Brian,
On 01.03.2011 20:25, Brian Gix wrote:
The problem you describe sounds like one I had to solve in the past, but
unfortunately, I think it may be a little more difficult to solve here.
This particular baseband appears to have an outstanding Cmd queue of
2. It also appears to consume one of them for extended periods of time
when making requests of the remote device, and using the NOP
Cmd-Status-Event to inform the host that the slot is now free.
As you are observing, the completion of the task (triggering additional
requests locally) overlaps with these NOP responses, giving a false
count to the host of available cmd slots.
Personally, I consider this to be a baseband bug, which could have been
avoided by having a max outstanding queue of 1.
This particular controller uses 1 credit for each command that is being
processed and having max outstanding queue of 1 would make some
scenarios impossible - consider authentication with
HCI_Authentication_Request pending and other HCI command to be sent in
parallel.
The adjustment I suggest doesn't disallow this. I was having a
theory-of-operation talk with a baseband guy once, and this is what he
had to say:
The HCI interface is intended to be an interface that immediately
responds to *every* command. The problem is that some commands are
intended for the local baseband (and can be handled immediately) and
others require interaction outside of the control of the local baseband,
and take an indeterminate amount of time.
So two response mechanism were created:
Command Immediate Rsp Delayed Rsp
Cmd --> Cmd Complete Evt (Cmds handled Locally)
Cmd --> Cmd Status Evt --> Cmplt Event ("long" Async Cmds)
The HCI flow control is contained in both the Cmd-Complt-Evt and the
Cmd-Status-Evt.
So it is assumed that both flow control response event types will be
delivered immediately after the baseband receives them. Of course
because of the communication link, these response are still asyncronous
in most cases including the BlueZ case.
The baseband guy basically said that "the baseband" does not expect the
next command until the host has processed the (immediate) response to
the previous one. And that the (immediate) response to the previous one
should be RXed in milliseconds at the most.
So I would always delay sending the next command until the prior
commands CmdStatus or CmdCmplt has been received. This should work
unless there is something seriously wrong with the baseband.
That's exactly how linux stack work, and I'm seeing where we could be doing
wrong, so I believe your patch is fixing nothing. A best version of your
patch is:
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index cebe7588..2d4d441 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1771,7 +1771,7 @@ static inline void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *sk
if (ev->opcode != HCI_OP_NOP)
del_timer(&hdev->cmd_timer);
- if (ev->ncmd) {
+ if (ev->ncmd&& ev->opcode != HCI_OP_NOP) {
atomic_set(&hdev->cmd_cnt, 1);
if (!skb_queue_empty(&hdev->cmd_q))
tasklet_schedule(&hdev->cmd_task);
I don't think that this is correct.
First: Any change made here in hci_cmd_cmplete_evt would need to also be
made in hci_cmd_status_evt. The "NOP" event can be either a Cmd Cmplt
or a Cmd Status, and in the example given by Andrzej, it was a Cmd Status.
Second: How is cmd_cnt being used? I see it being decremented when a
command is sent in hci_cmd_task, or being set to "1" when one of the two
events are received. It is also set to "1" for time-outs and channel
open/close/reset activities.
It looks to me like the cmd_cnt is being misused. It should in fact be
set to the value indicated by the event, and not just "1". It also does
not address the original problem observed by Andrzej, which is that the
Asyncronous link between the Host and Baseband can cause them to be
momentarily out of sync with each other.
Gustavo's suggestion misses the problem, that the reason basebands use
NOPs at all is to hold onto available command slots, only to release
them later with the NOP. It is entirely possible that a baseband could
indicate with Cmd Status an ev->ncmd == 0, and the entire transaction
then end with a NOP (of either type) with ev->ncmd > 0. If that were to
occur, the cmd_q would never get serviced.
I don't see any solution to this other than to *separately* track the
existence of an unAcked cmd, and conditioning the servicing of the cmd_q
on *both* the (non) existence of an unAcked cmd *and* the availablity of
slots in cmd_cnt.
That patch means that want to avoid send new commands when a Command Status
Event arrives, but we want exactly the opposite.
--
Brian Gix
bgix@xxxxxxxxxxxxxx
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
