Hi Gustavo,
On 17.03.2011 00:27, Gustavo F. Padovan wrote:
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.
Yes, this is wrong. It's just a simplified version of Andrzej's patch.
No, my patch does not break other scenarios, i.e. the one mentioned by
Brian ;-)
I agree with him that we can make proper decision in every case only if
we have both credit and unacked count separately due to asynchronous
nature of HCI acks. And it's a good point that we do not have to count
unacked commands but instead just store information whether last sent
cmd is not yet acked.
I was going to send v2 patch with suggestions from Brian included but I
didn't get answer whether cmd_cnt has to be atomic_t for some reason
other than historical. Both acl_cnt and sco_cnt are simple integers, for
example.
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.
We limit cmd_cnt to 1 in our stack, to be sure that one command a time will be
sent, I don't for the reason for that but Marcel certainly has one.
Pretty good reason could be that last command sent is stored in hci_dev
and then used in cc/cs events. I don't have problem with this kind of
solution, I just think it's not enough here.
I also don't see point of making it a counter while it behaves like a
weird flag: set to 1, but decrease instead of set to 0. A bit
inconsistent in my opinion.
BR,
Andrzej
--
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
