Hi Sean,

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+
+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.

Understood.

I've stopped the hack in v8. could we merge v8 first ? and then I will a fix up with __hci_raw_sync_ev that uses the hdev->raw_q instead of __hci_cmd_sync_ev in TODO.

so I looked into this a bit more. We actually added __hci_cmd_send for a Qualcomm firmware loader that was doing something similar. So instead of trying to add a yet another command to the core, I actually used that and implemented the wait for vendor event in the driver.

You will see my v9 on the mailing list. I also did a bunch of cosmetic minor cleanup and spelling correction. Please test this version. I also make __le16 dlen instead of dlen1 + dlen2 since I think that is what your hardware does.

If this version of the driver works for you then I am happy to merge it. You can then add support for hdev->set_bdaddr and hdev->set_diag in later patches. I also like to clean up the STP receive handler since it can be done a lot simpler and smaller, but that has to wait.

quoted

Are all Mediatek vendor commands this way? Or just the ones for loading the firmware? So only the WMT ones?

Only the WMT ones, WMT commands/events are usually used in system controlling, for example, global function on/off, firmware download, reset and so on. most only appear on device initialization

Since you never checked the result of the vendor event, I opted for just signaling that it arrived. If they can report success or failure, we need to add some extra code for that.

Regards

Marcel