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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <9526E5D9-50BA-4D57-80F5-083DB7D28AFE@holtmann.org>
Date:   Thu, 2 Aug 2018 11:45:04 +0200
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Sean Wang <sean.wang@...iatek.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Johan Hedberg <johan.hedberg@...il.com>,
        devicetree <devicetree@...r.kernel.org>,
        "open list:BLUETOOTH DRIVERS" <linux-bluetooth@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for
 MediaTek serial devices

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.

Regards

Marcel