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: <8AF72CFC-4978-42AD-8ECC-752EB137DD2A@holtmann.org>
Date:   Tue, 7 Aug 2018 17:54:53 +0200
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Sean Wang <sean.wang@...iatek.com>
Cc:     Mark Rutland <mark.rutland@....com>,
        devicetree <devicetree@...r.kernel.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:BLUETOOTH DRIVERS" <linux-bluetooth@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        linux-mediatek@...ts.infradead.org,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for
 MediaTek serial devices

Hi Sean,

>>>> 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.
> 
> Only one thing needs to be corrected in v9. that is __be16 is required instead of dlen1 + dlen2. I will fix it up in v10 and the other changes all look good to me.
> 
>> 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.
>> 
> 
> hopefully v10 also can be merged :)

send me a v10 and I can merge it.

> I will investigate more about how to add ->set_bdaddr, ->set_diag and STP receive enhancement in later patches.
> 
> but so far I have not much idea about how to make STP multiplexer be a independent driver.
> 
> my thought is that it would be really better and cleaner a chain of serdev is be used as the base of mtkbtuart. something like
> 
> 8250 serial bus <----> STP multiplexer serdev <----> mtkbtuart serdev
> 
> however, STP multiplexer serdev is not a real device, that doesn't no request any resource. I think it should not be allowed to be added in a device tree and even in dt-binding document.

Before we do that, lets get a cleaner parser for it. I just don’t have enough time to wrap my head around this one yet.

>>>> 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.
>> 
> 
> I will consider more WMT event status when I add more Bluetooth devices such as MT7668U usb based Bluetooth which I plan to add the support in later patches in the next weeks 

Are the USB ones also using STP or are they H:2 based like all the others. What are prominent MT7668U based ones that I could buy?

Regards

Marcel