On Thu, 2018-08-02 at 09:38 +0200, Marcel Holtmann wrote:

Hi Sean,

[ ... ]

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);

quoted

quoted

quoted

+
+	skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
+				HCI_INIT_TIMEOUT);
+
+	if (IS_ERR(skb)) {
+		int err = PTR_ERR(skb);
+
+		bt_dev_err(hdev, "Failed to send wmt cmd (%d)", err);
+		return err;
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+

[ ... ]

quoted

quoted

quoted

+	shdr->dlen2 = dlen & 0xff;
+	shdr->cs = p[0] + p[1] + p[2];

as above discussion about shr->cs , it can be filled with zero to have less computing 

If it has no value, then zero it out and add a comment for it.

okay

quoted

quoted

I would add another comment here that this added the STP trailer. And change the above to mention it adds the STP header.

sure

quoted

And you might want to check if there is space for the trailer as well. Otherwise skb_put tends to call BUG() if I remember correctly. I know this is super unlikely since our bt_skb_alloc is pretty large.

sure, I will add the handling for that. it should be better to make sure all rooms are enough for header and trailer before adding content to them

[ ... ]

quoted

quoted

You want to add a MODULE_FIRMWARE here as well.

okay

Regards

Marcel