Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann <marcel@holtmann.org>]

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

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