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: <CABBYNZLWFNWp=StPQ0=8hQe8bUoJzwSvCBk7Ybd=2oe=NROCgQ@mail.gmail.com>
Date:   Fri, 16 Dec 2022 13:19:07 -0800
From:   Luiz Augusto von Dentz <luiz.dentz@...il.com>
To:     Marcel Holtmann <marcel@...tmann.org>
Cc:     Neeraj Sanjay Kale <neeraj.sanjaykale@....com>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Paul Menzel <pmenzel@...gen.mpg.de>, amitkumar.karwar@....com,
        rohit.fule@....com, sherry.sun@....com,
        linux-kernel@...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 Marcel,

On Fri, Dec 16, 2022 at 12:15 PM Marcel Holtmann <marcel@...tmann.org> wrote:
>
> 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.


-- 
Luiz Augusto von Dentz