Hi Sean,
>>>>>>>>> +
>>>>>>>>> +static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
>>>>>>>>> + const void *param)
>>>>>>>>> +{
>>>>>>>>> + struct mtk_hci_wmt_cmd wc;
>>>>>>>>> + struct mtk_wmt_hdr *hdr;
>>>>>>>>> + struct sk_buff *skb;
>>>>>>>>> + u32 hlen;
>>>>>>>>> +
>>>>>>>>> + hlen = sizeof(*hdr) + plen;
>>>>>>>>> + if (hlen > 255)
>>>>>>>>> + return -EINVAL;
>>>>>>>>> +
>>>>>>>>> + hdr = (struct mtk_wmt_hdr *)&wc;
>>>>>>>>> + hdr->dir = 1;
>>>>>>>>> + hdr->op = op;
>>>>>>>>> + hdr->dlen = cpu_to_le16(plen + 1);
>>>>>>>>> + hdr->flag = flag;
>>>>>>>>> + memcpy(wc.data, param, plen);
>>>>>>>>> +
>>>>>>>>> + atomic_inc(&hdev->cmd_cnt);
>>>>>>>>
>>>>>>>> Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.
>>>>>>>>
>>>>>>>
>>>>>>> An increment on cmd_cnt is really needed because hci_cmd_work would check whether cmd_cnt is positive and then has a decrement on cmd_cnt before a packet is being sent out.
>>>>>>>
>>>>>>> okay will add a comment.
>>>>>>
>>>>>> but you are in ->setup callback this time. So if you need this, then all the other ->setup routines would actually fail as well. Either this is leftover from when you did things in ->probe or ->open or this is some thing we might better fix properly in the core instead of papering over it. Can you recheck if this is really needed.
>>>>>>
>>>>>
>>>>> I added a counter print and the counter increments as below
>>>>>
>>>>> /* atomic_inc(&hdev->cmd_cnt); */
>>>>> pr_info("cmd_cnt = %d\n" , atomic_read(&hdev->cmd_cnt));
>>>>>
>>>>> skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
>>>>> HCI_INIT_TIMEOUT);
>>>>>
>>>>> and the log show up that
>>>>>
>>>>>
>>>>> [ 334.049156] Bluetooth: hci0: command 0xfc6f tx timeout
>>>>> [ 334.054840] cmd_cnt = 0
>>>>> [ 336.065076] Bluetooth: hci0: command 0xfc6f tx timeout
>>>>> [ 336.070795] cmd_cnt = 0
>>>>> [ 338.080997] Bluetooth: hci0: command 0xfc6f tx timeout
>>>>> [ 338.086683] cmd_cnt = 0
>>>>> [ 340.096907] Bluetooth: hci0: command 0xfc6f tx timeout
>>>>> [ 340.102609] cmd_cnt = 0
>>>>> [ 342.112824] Bluetooth: hci0: command 0xfc6f tx timeout
>>>>> [ 342.118520] cmd_cnt = 0
>>>>> [ 344.128747] Bluetooth: hci0: command 0xfc6f tx timeout
>>>>> [ 344.134454] cmd_cnt = 0
>>>>> [ 346.144667] Bluetooth: hci0: command 0xfc6f tx timeout
>>>>> [ 346.150372] cmd_cnt = 0
>>>>>
>>>>>
>>>>> The packet is dropped by hci_cmd_work at [1], so I also wondered why the
>>>>> other vendor driver works, it seems the counter needs to be incremented
>>>>> before every skb is being queued to cmd_q.
>>>>>
>>>>> 4257 static void hci_cmd_work(struct work_struct *work)
>>>>> 4258 {
>>>>> 4259 struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_work);
>>>>> 4260 struct sk_buff *skb;
>>>>> 4261
>>>>> 4262 BT_DBG("%s cmd_cnt %d cmd queued %d", hdev->name,
>>>>> 4263 atomic_read(&hdev->cmd_cnt), skb_queue_len(&hdev->cmd_q));
>>>>> 4264
>>>>> 4265 /* Send queued commands */
>>>>>
>>>>> [1]
>>>>> 4266 if (atomic_read(&hdev->cmd_cnt)) { /* dropped when cmd_cnt is zero */
>>>>> 4267 skb = skb_dequeue(&hdev->cmd_q);
>>>>> 4268 if (!skb)
>>>>> 4269 return;
>>>>> 4270
>>>>> 4271 kfree_skb(hdev->sent_cmd);
>>>>> 4272
>>>>> 4273 hdev->sent_cmd = skb_clone(skb, GFP_KERNEL);
>>>>> 4274 if (hdev->sent_cmd) {
>>>>> 4275 atomic_dec(&hdev->cmd_cnt); /* cmd_cnt-- */
>>>>> 4276 hci_send_frame(hdev, skb);
>>>>
>>>> actually the command also needs to better go via the raw_q anyway since it doesn’t come back with the cmd status or cmd complete. You have it waiting for a vendor event. Maybe with is something we need to consider with __hci_cmd_sync_ev anyway.
>>>>
>>>> Johan would know best since he wrote that code. Anyway, we should fix that in the core and not have you hack around it.
>>>>
>>>
>>> yes, my case is that received event is neither cmd status nor cmd complete. It is completely a vendor event.
>>>
>>> if it wants to be solved by the core layer, do you permit that I remove the hack and then send it in the next version?
>>
>> we need to have a __hci_raw_sync_ev that uses the hdev->raw_q and waits for the specified event to come back. I never realized that you are missing the cmd status or cmd complete. So this is similar to the original CSR vendor commands which had the same behavior.
>>
>> I have the feeling that you hdev->cmd_cnt increment is just hiding the problem here. If you really think that it is not chains any side effects we can merge the driver with a big warning and fix this up. However the clean way would be for you to create a patch that introduces __hci_raw_sync_ev as describe above.
>
> What do you think of this? If I add extra atomic_set 1 on cmd_cnt after driver really got a vendor event back instead of blinding to increment for every packet sent.
>
> the behavior is the same to receive a cmd status or complete. it should not have side effects.
>
> 96 skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
> 97 HCI_INIT_TIMEOUT);
> 98
> 99 if (IS_ERR(skb)) {
> 100 int err = PTR_ERR(skb);
> 101
> 102 bt_dev_err(hdev, "Failed to send wmt cmd (%d)", err);
> 103 return err;
> 104 }
> 105
> 106 if (!test_bit(HCI_RESET, &hdev->flags)) <<<<<<
> 107 atomic_set(&hdev->cmd_cnt, 1); <<<<<<
> 108
> 109 kfree_skb(skb);
this is even more hackish since the __hci_cmd_sync_ev command is really meant to get a cmd status first before waiting for that event.
Are all Mediatek vendor commands this way? Or just the ones for loading the firmware? So only the WMT ones?
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
