linux-kernel - Re: [PATCH v5] Bluetooth: Add hci_nxp to hci

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: <61D5389C-013E-452C-ACE7-30708F0C1A35@holtmann.org>
Date:   Tue, 3 Jan 2023 23:19:44 +0100
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Neeraj sanjay kale <neeraj.sanjaykale@....com>
Cc:     Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Paul Menzel <pmenzel@...gen.mpg.de>,
        Amitkumar Karwar <amitkumar.karwar@....com>,
        Rohit Fule <rohit.fule@....com>,
        Sherry Sun <sherry.sun@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>
Subject: Re: [PATCH v5] Bluetooth: Add hci_nxp to hci_uart module to support
 NXP BT chipsets

Hi Neeraj,

>>>>> Add hci_nxp to the hci_uart module which adds support for the NXP BT
>>>>> chips. This driver has Power Save feature that will put the NXP
>>>>> bluetooth chip into sleep state, whenever there is no activity for
>>>>> certain duration of time (2000ms), and will be woken up when any
>>>>> activity is to be initiated.
>>>>> 
>>>>> The Power Save feature can be configured with the following set of
>>>>> commands (optional):
>>>>> hcitool -i hci0 cmd 3F 23 02 00 00    (enable Power Save)
>>>>> hcitool -i hci0 cmd 3F 23 03 00 00    (disable Power Save)
>>>>> where,
>>>>> OGF = 0x3F (vendor specific command) OCF = 0x23 (command to set
>>>>> Power Save state) arg[0] = 0x02 (disable Power Save) arg[0] = 0x03
>>>>> (enable Power Save) arg[1,2,...] = XX (don't care)
>>>>> 
>>>>> The sleep/wake-up source can be configured with the following set of
>>>>> commands (optional):
>>>>> hcitool -i hci0 cmd 3F 53 03 14 01 FF    (set UART break method)
>>>>> hcitool -i hci0 cmd 3F 53 03 14 00 FF    (set UART DSR method)
>>>>> where,
>>>>> OGF = 0x3F (vendor specific command) OCF = 0x53 (command to set
>>>>> sleep and wake-up source) arg[0] = 0x00 (Chip to host method NONE)
>>>>> arg[0] = 0x01 (Chip to host method UART DTR) arg[0] = 0x02 (Chip to
>>>>> host method UART BREAK) arg[0] = 0x03 (Chip to host method GPIO)
>>>>> arg[1] = 0x14 (Chip to host GPIO[20] if arg[0] is 0x03, else 0xFF)
>>>>> arg[2] = 0x00 (Host to chip method UART DSR) arg[2] = 0x01 (Host to
>>>>> chip method UART BREAK) arg[3] = 0xXX (Reserved for future use)
>>>>> 
>>>>> By default, the hci_nxp sets power save enable, chip to host wake-up
>>>>> source as GPIO and host to chip sleep and wake-up source as UART
>>>>> break during driver initialization, by sending the respective
>>>>> commands to the chip.
>>>>> 
>>>>> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@....com>
>>>>> ---
>>>>> v2: Changed the subject/summary lines and added more details in the
>>>>> description. (Paul Menzel)
>>>>> v3: Made internal functions static, optimized the code, added few
>>>>> comments. (Sherry Sun)
>>>>> v4: Reworked entire code to send vendor commands cmd23 and cmd53
>> by
>>>>> using __hci_cmd_sync. (Luiz Augusto von Dentz)
>>>>> v5: Used hci_command_hdr and combined OGF+OCF into a single
>> opcode.
>>>>> (Luiz Augusto von Dentz)
>>>>> ---
>>>>> MAINTAINERS                   |   6 +
>>>>> drivers/bluetooth/Kconfig     |  10 +
>>>>> drivers/bluetooth/Makefile    |   1 +
>>>>> drivers/bluetooth/hci_ldisc.c |   6 +
>>>>> drivers/bluetooth/hci_nxp.c   | 592
>> ++++++++++++++++++++++++++++++++++
>>>>> drivers/bluetooth/hci_nxp.h   |  94 ++++++
>>>>> drivers/bluetooth/hci_uart.h  |   8 +-
>>>>> 7 files changed, 716 insertions(+), 1 deletion(-) create mode 100644
>>>>> drivers/bluetooth/hci_nxp.c create mode 100644
>>>>> drivers/bluetooth/hci_nxp.h
>>>> 
>>>> so this is a clear NAK. Add this as serdev driver and not hook
>>>> further into the mess that is the HCI line discipline.
>>> 
>>> I wonder if we should make it more clear somehow, perhaps include a
>>> text on the likes of BT_HCIUART that is deprecated and new drivers
>>> shall use BT_HCIUART_SERDEV instead.
>> 
>> not even that. They need to be separate drivers. A long time ago I posted the
>> skeleton for btuart.ko and bt3wire.ko and that is where this has to go.
>> 
>> Regards
>> 
>> Marcel
> 
> Thanks for your comment. 
> Based on your comment, I re-worked the entire driver with reference to following patches:
> https://www.spinics.net/lists/linux-bluetooth/msg74918.html
> https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/bluetooth/btmtkuart.c
> 
> I am able to create a standalone btnxp.ko which is able to run basic BT operations along with FW download with NXP chipsets.
> 
> However I have now hit a blocker. The NXP chipsets require support for break signal, by which the host can put the chip into sleep, and wake it up.
> 
> However, it seems that the serdev API's in https://elixir.bootlin.com/linux/v6.1-rc8/source/include/linux/serdev.h do not support break assertion over serial TX line.
> 
> Is there any plan for serdev to support break signaling?
> 
> Any help on this blocker would be highly appreciated.

I recall some posts about that. Either on LKML or some other mailing list. Don’t
remember what the outcome was though.

Regards

Marcel