| 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
| ||
|
Message-Id: <24F36C9D-6219-4A0B-A798-029ED4EEAB06@holtmann.org>
Date: Tue, 2 Nov 2021 10:22:24 +0100
From: Marcel Holtmann <marcel@...tmann.org>
To: Zijun Hu <zijuhu@...eaurora.org>
Cc: Greg KH <gregkh@...uxfoundation.org>,
Johan Hedberg <johan.hedberg@...il.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-bluetooth <linux-bluetooth@...r.kernel.org>,
MSM <linux-arm-msm@...r.kernel.org>,
Balakrishna Godavarthi <bgodavar@...eaurora.org>,
c-hbandi@...eaurora.org, Hemantg <hemantg@...eaurora.org>,
Matthias Kaehlcke <mka@...omium.org>,
Rocky Liao <rjliao@...eaurora.org>,
Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH v1 3/3] Bluetooth: hci_qca: Add support for QTI bluetooth
MAPLE
Hi Zijun,
>>>>>> Add support for MAPLE integrated within SOC, it is mounted on
>>>>>> a virtual tty port and powered on/off via relevant IOCTL, neither
>>>>>> IBS nor RAMPATCH downloading is not required.
>>>>>>
>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
>>>>>> ---
>>>>>> drivers/bluetooth/btqca.c | 13 ++++++++++++-
>>>>>> drivers/bluetooth/btqca.h | 13 +++++++++++++
>>>>>> drivers/bluetooth/hci_qca.c | 47 ++++++++++++++++++++++++++++++++++++++++++++-
>>>>>> 3 files changed, 71 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>>>>>> index be04d74037d2..b83d2ecefe5d 100644
>>>>>> --- a/drivers/bluetooth/btqca.c
>>>>>> +++ b/drivers/bluetooth/btqca.c
>>>>>> @@ -255,6 +255,8 @@ static void qca_tlv_check_data(struct hci_dev *hdev,
>>>>>> BT_DBG("TLV Type\t\t : 0x%x", type_len & 0x000000ff);
>>>>>> BT_DBG("Length\t\t : %d bytes", length);
>>>>>>
>>>>>> + if (qca_is_maple(soc_type))
>>>>>> + break;
>>>>>> idx = 0;
>>>>>> data = tlv->data;
>>>>>> while (idx < length) {
>>>>>> @@ -552,6 +554,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>>> rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
>>>>>>
>>>>>> /* Download rampatch file */
>>>>>> + if (qca_is_maple(soc_type))
>>>>>> + goto download_nvm;
>>>>>> +
>>>>>> config.type = TLV_TYPE_PATCH;
>>>>>> if (qca_is_wcn399x(soc_type)) {
>>>>>> snprintf(config.fwname, sizeof(config.fwname),
>>>>>> @@ -580,6 +585,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>>> /* Give the controller some time to get ready to receive the NVM */
>>>>>> msleep(10);
>>>>>>
>>>>>> +download_nvm:
>>>>>> /* Download NVM configuration */
>>>>>> config.type = TLV_TYPE_NVM;
>>>>>> if (firmware_name)
>>>>>> @@ -597,6 +603,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>>> else if (soc_type == QCA_QCA6390)
>>>>>> snprintf(config.fwname, sizeof(config.fwname),
>>>>>> "qca/htnv%02x.bin", rom_ver);
>>>>>> + else if (qca_is_maple(soc_type))
>>>>>> + snprintf(config.fwname, sizeof(config.fwname),
>>>>>> + "qca/mpnv%02x.bin", rom_ver);
>>>>>> else if (soc_type == QCA_WCN6750)
>>>>>> snprintf(config.fwname, sizeof(config.fwname),
>>>>>> "qca/msnv%02x.bin", rom_ver);
>>>>>> @@ -609,6 +618,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>>> bt_dev_err(hdev, "QCA Failed to download NVM (%d)", err);
>>>>>> return err;
>>>>>> }
>>>>>> + if (qca_is_maple(soc_type))
>>>>>> + msleep(MAPLE_NVM_READY_DELAY_MS);
>>>>>>
>>>>>> if (soc_type >= QCA_WCN3991) {
>>>>>> err = qca_disable_soc_logging(hdev);
>>>>>> @@ -637,7 +648,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>>> return err;
>>>>>> }
>>>>>>
>>>>>> - if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750) {
>>>>>> + if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750 || qca_is_maple(soc_type)) {
>>>>>> /* get fw build info */
>>>>>> err = qca_read_fw_build_info(hdev);
>>>>>> if (err < 0)
>>>>>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>>>>>> index 30afa7703afd..0a5a7d1daa71 100644
>>>>>> --- a/drivers/bluetooth/btqca.h
>>>>>> +++ b/drivers/bluetooth/btqca.h
>>>>>> @@ -46,6 +46,8 @@
>>>>>>
>>>>>> #define QCA_FW_BUILD_VER_LEN 255
>>>>>>
>>>>>> +#define MAPLE_NVM_READY_DELAY_MS 1500
>>>>>> +#define MAPLE_POWER_CONTROL_DELAY_MS 50
>>>>>>
>>>>>> enum qca_baudrate {
>>>>>> QCA_BAUDRATE_115200 = 0,
>>>>>> @@ -145,6 +147,7 @@ enum qca_btsoc_type {
>>>>>> QCA_WCN3991,
>>>>>> QCA_QCA6390,
>>>>>> QCA_WCN6750,
>>>>>> + QCA_MAPLE,
>>>>>> };
>>>>>>
>>>>>> #if IS_ENABLED(CONFIG_BT_QCA)
>>>>>> @@ -167,6 +170,11 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
>>>>>> return soc_type == QCA_WCN6750;
>>>>>> }
>>>>>>
>>>>>> +static inline bool qca_is_maple(enum qca_btsoc_type soc_type)
>>>>>> +{
>>>>>> + return soc_type == QCA_MAPLE;
>>>>>> +}
>>>>>> +
>>>>>> #else
>>>>>>
>>>>>> static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>>>>>> @@ -204,6 +212,11 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
>>>>>> return false;
>>>>>> }
>>>>>>
>>>>>> +static inline bool qca_is_maple(enum qca_btsoc_type soc_type)
>>>>>> +{
>>>>>> + return false;
>>>>>> +}
>>>>>> +
>>>>>> static inline int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
>>>>>> {
>>>>>> return -EOPNOTSUPP;
>>>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>>>> index dd768a8ed7cb..f1d9670719c4 100644
>>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>>> @@ -70,6 +70,10 @@
>>>>>> #define QCA_CRASHBYTE_PACKET_LEN 1096
>>>>>> #define QCA_MEMDUMP_BYTE 0xFB
>>>>>>
>>>>>> +#ifndef IOCTL_IPC_BOOT
>>>>>> +#define IOCTL_IPC_BOOT 0xBE
>>>>>> +#endif
>>>>>
>>>>> You send this command, but never use it. Where is the driver code that
>>>>> uses this command?
>>>>>
>>>> qca_maple_power_control() will use it. this driver depends on bt_tty kernel module
>>>> https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/tree/drivers/soc/qcom/bt_tty.c?h=NHSS.QSDK.11.5.0.5.r2
>>>
>>> You can not add code to the kernel that is not used by the kernel
>>> itself. That driver needs to be in the tree as well, why is it not
>>> submitted now too?
>>>
>>>>> And why not tabs?
>>>>>
>>>>> And why is this patch series not properly threaded so tools can pick it
>>>>> up and find them?
>>>>>
>>>>> And why the odd named ioctl that is different from other ones in this
>>>>> file?
>>>>>
>>>> that IOCTL name is defined by that module.
>>>> https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/tree/include/linux/bt.h?h=NHSS.QSDK.11.5.0.5.r2
>>>
>>> Again, it needs to be in the tree.
>>>
>>>>> And why not just use normal power management hooks for doing things like
>>>>> turning on and off the hardware like all other drivers?
>>>>>
>>>> this device is special.
>>>
>>> All drivers and devices are special and unique. Just like all of them :)
>>>
>>> What is so odd about this device that it can not work with the existing
>>> infrastructure that the kernel has for all of the hundreds of thousands
>>> of other devices it supports?
>>>
>>>> it seems BT maintainer decides to drop this patch.
>>>
>>> Of course, at the very least because there is no in-kernel user, why
>>> would you accept such a patch if you were the maintainer?
>>>
>>> Please submit your driver first.
>>
>> this power on via ioctl is nasty business. I am so happy that we got rid of
>> the crucks when we finally landed serdev.
>>
>> Some people are working on power sequence support and alike. This needs to
>> use proper infrastructure or extend existing infrastructure. To fit the
>> needs.
>>
>> I am just 100% certain, that booting an IPC via an ioctl isn’t it. We
>> really suffered through it in the 2.4 kernel days. The hardware needs to
>> be described properly in device tree and the kernel needs to take all
>> the appropriate actions if a Bluetooth device is powered on via its
>> standard power on procedure. And that is through bluetoothd (or if you
>> use some other Bluetooth userspace) via the exposed API from the kernel.
>>
> thank you. the IOCTL purpose is to bootup the special bluetooth controller.
> i have verified this change.
> i will submit this change to linux-ipq-5.4 firstly even if need to pick up many changes firstly.
I am not sure this is fully understood yet. Do _not_ use an ioctl to boot the Bluetooth
controller. Power on/off of Bluetooth hardware happens via the standard interface used
by bluetoothd. The Bluetooth transport driver (in your case hci_qca) has to do everything
needed when a) serdev->probe is called and b) hdev->open is called. Any other path to
power your hardware is (bluntly put) wrong.
Regards
Marcel
Powered by blists - more mailing lists