| 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: <7cdb7cef-db8a-4a21-af1c-2b29898a5814@amlogic.com>
Date: Fri, 19 Jul 2024 17:05:37 +0800
From: Yang Li <yang.li@...ogic.com>
To: Luiz Augusto von Dentz <luiz.dentz@...il.com>,
Sai Krishna Gajula <saikrishnag@...vell.com>
Cc: Marcel Holtmann <marcel@...tmann.org>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Catalin Marinas
<catalin.marinas@....com>, Will Deacon <will@...nel.org>,
"linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, Ye He <ye.he@...ogic.com>
Subject: Re: [PATCH v2 2/3] Bluetooth: hci_uart: Add support for Amlogic HCI
UART
Dear Sai and Luiz
Thanks for the review.
On 2024/7/19 3:01, Luiz Augusto von Dentz wrote:
> Hi Sai,
>
> On Thu, Jul 18, 2024 at 2:43 PM Sai Krishna Gajula
> <saikrishnag@...vell.com> wrote:
>>
>>> -----Original Message-----
>>> From: Yang Li via B4 Relay <devnull+yang.li.amlogic.com@...nel.org>
>>> Sent: Thursday, July 18, 2024 1:12 PM
>>> To: Marcel Holtmann <marcel@...tmann.org>; Luiz Augusto von Dentz
>>> <luiz.dentz@...il.com>; David S. Miller <davem@...emloft.net>; Eric
>>> Dumazet <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo
>>> Abeni <pabeni@...hat.com>; Rob Herring <robh@...nel.org>; Krzysztof
>>> Kozlowski <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>;
>>> Catalin Marinas <catalin.marinas@....com>; Will Deacon <will@...nel.org>
>>> Cc: linux-bluetooth@...r.kernel.org; netdev@...r.kernel.org;
>>> devicetree@...r.kernel.org; linux-kernel@...r.kernel.org; linux-arm-
>>> kernel@...ts.infradead.org; Yang Li <yang.li@...ogic.com>; Ye He
>>> <ye.he@...ogic.com>
>>> Subject: [PATCH v2 2/3] Bluetooth: hci_uart: Add support for
>>> Amlogic HCI UART
>>>
>>> From: Yang Li <yang. li@ amlogic. com> This patch introduces support for
>>> Amlogic Bluetooth controller over UART. In order to send the final firmware at
>>> full speed. It is a pretty straight forward H4 driver with exception of actually
>>> having
>>> From: Yang Li <yang.li@...ogic.com>
>>>
>>> This patch introduces support for Amlogic Bluetooth controller over UART. In
>>> order to send the final firmware at full speed. It is a pretty straight forward H4
>>> driver with exception of actually having it's own setup address configuration.
>>>
>>> Co-developed-by: Ye He <ye.he@...ogic.com>
>>> Signed-off-by: Ye He <ye.he@...ogic.com>
>>> Signed-off-by: Yang Li <yang.li@...ogic.com>
>>> ---
>>> drivers/bluetooth/Kconfig | 12 +
>>> drivers/bluetooth/Makefile | 1 +
>>> drivers/bluetooth/hci_aml.c | 772
>>> ++++++++++++++++++++++++++++++++++++++++++
>>> drivers/bluetooth/hci_ldisc.c | 8 +-
>>> drivers/bluetooth/hci_uart.h | 8 +-
>>> 5 files changed, 798 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig index
>>> 90a94a111e67..d9ff7a64d032 100644
>>> --- a/drivers/bluetooth/Kconfig
>>> +++ b/drivers/bluetooth/Kconfig
>>> @@ -274,6 +274,18 @@ config BT_HCIUART_MRVL
>>>
>>> Say Y here to compile support for HCI MRVL protocol.
>>>
>>> +config BT_HCIUART_AML
>>> + bool "Amlogic protocol support"
>>> + depends on BT_HCIUART
>>> + depends on BT_HCIUART_SERDEV
>>> + select BT_HCIUART_H4
>>> + select FW_LOADER
>>> + help
>>> + The Amlogic protocol support enables Bluetooth HCI over serial
>>> + port interface for Amlogic Bluetooth controllers.
>>> +
>>> + Say Y here to compile support for HCI AML protocol.
>>> +
>>> config BT_HCIBCM203X
>>> tristate "HCI BCM203x USB driver"
>>> depends on USB
>>> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile index
>>> 0730d6684d1a..81856512ddd0 100644
>>> --- a/drivers/bluetooth/Makefile
>>> +++ b/drivers/bluetooth/Makefile
>>> @@ -51,4 +51,5 @@ hci_uart-$(CONFIG_BT_HCIUART_BCM) += hci_bcm.o
>>> hci_uart-$(CONFIG_BT_HCIUART_QCA) += hci_qca.o
>>> hci_uart-$(CONFIG_BT_HCIUART_AG6XX) += hci_ag6xx.o
>>> hci_uart-$(CONFIG_BT_HCIUART_MRVL) += hci_mrvl.o
>>> +hci_uart-$(CONFIG_BT_HCIUART_AML) += hci_aml.o
>>> hci_uart-objs := $(hci_uart-y)
>>> diff --git a/drivers/bluetooth/hci_aml.c b/drivers/bluetooth/hci_aml.c new file
>>> mode 100644 index 000000000000..575b6361dad6
>>> --- /dev/null
>>> +++ b/drivers/bluetooth/hci_aml.c
>>> @@ -0,0 +1,772 @@
>>> +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
>>> +/*
>>> + * Copyright (C) 2024 Amlogic, Inc. All rights reserved */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/device.h>
>>> +#include <linux/property.h>
>> ......
>>
>>> + * op_code | op_len | op_addr | parameter |
>>> + * --------|-----------------------|---------|-------------|
>>> + * 2B | 1B len(addr+param) | 4B | len(param) |
>>> + */
>>> +static int aml_send_tci_cmd(struct hci_dev *hdev, u16 op_code, u32
>>> op_addr,
>>> + u32 *param, u32 param_len)
>>> +{
>>> + struct sk_buff *skb = NULL;
>>> + struct aml_tci_rsp *rsp = NULL;
>>> + u8 *buf = NULL;
>>> + u32 buf_len = 0;
>>> + int err = 0;
>> Please consider using reverse xmas tree order - longest line to shortest - for local variable declarations in Networking code.
> First time I'm hearing about this, is that just for the sake of readability?
well, for the sake of readability.
>
>>> +
>>> + buf_len = sizeof(op_addr) + param_len;
>>> + buf = kmalloc(buf_len, GFP_KERNEL);
>>> + if (!buf) {
>>> + err = -ENOMEM;
>>> + goto exit;
>>> + }
>>> +
>>> + memcpy(buf, &op_addr, sizeof(op_addr));
>>> + if (param && param_len > 0)
>>> + memcpy(buf + sizeof(op_addr), param, param_len);
>>> +
>>> + skb = __hci_cmd_sync_ev(hdev, op_code, buf_len, buf,
>>> + HCI_EV_CMD_COMPLETE,
>>> HCI_INIT_TIMEOUT);
>>> + if (IS_ERR(skb)) {
>>> + err = PTR_ERR(skb);
>>> + skb = NULL;
>> Better to capture the error before nullifying skb, like below
>> err = PTR_ERR(skb);
>> skb = NULL; // Nullify after capturing the error
> That is exactly what he is doing, and actually it seems to only be
> doing that because it later calls kfree_skb.
Yes, it is right, but there is another issue that it setting skb to NULL
before kfree_skb(skb)
causes memory leaks, and I will delete "skb = NULL;"
>>> + bt_dev_err(hdev, "Failed to send TCI cmd:(%d)", err);
>>> + goto exit;
>>> + }
>>> +
>>> + rsp = (struct aml_tci_rsp *)(skb->data);
> This code is not safe, you need to check if skb->len because trying to
> access skb->data, anyway you are probably much better off using
> skb_pull_data instead.
well, I got it.
>
>>> + if (rsp->opcode != op_code || rsp->status != 0x00) {
>>> + bt_dev_err(hdev, "send TCI cmd(0x%04X),
>>> response(0x%04X):(%d)",
>>> + op_code, rsp->opcode, rsp->status);
>>> + err = -EINVAL;
>>> + goto exit;
>>> + }
>>> +
>>> +exit:
>>> + kfree(buf);
>>> + kfree_skb(skb);
>>> + return err;
>>> +}
>>> +
>>> +static int aml_update_chip_baudrate(struct hci_dev *hdev, u32 baud) {
>>> + u32 value;
>>> +
>>> + value = ((AML_UART_CLK_SOURCE / baud) - 1) & 0x0FFF;
>>> + value |= AML_UART_XMIT_EN | AML_UART_RECV_EN |
>>> +AML_UART_TIMEOUT_INT_EN;
>>> +
>>> + return aml_send_tci_cmd(hdev, AML_TCI_CMD_UPDATE_BAUDRATE,
>>> + AML_OP_UART_MODE, &value,
>>> sizeof(value)); }
>>> +
>>> +static int aml_start_chip(struct hci_dev *hdev) {
>>> + u32 value = 0;
>>> + int ret;
>>> +
>>> + value = AML_MM_CTR_HARD_TRAS_EN;
>>> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_WRITE,
>>> + AML_OP_MEM_HARD_TRANS_EN,
>>> + &value, sizeof(value));
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* controller hardware reset. */
>>> + value = AML_CTR_CPU_RESET | AML_CTR_MAC_RESET |
>>> AML_CTR_PHY_RESET;
>>> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_HARDWARE_RESET,
>>> + AML_OP_HARDWARE_RST,
>>> + &value, sizeof(value));
>>> + return ret;
>>> +}
>>> +
>>> +static int aml_send_firmware_segment(struct hci_dev *hdev,
>>> + u8 fw_type,
>>> + u8 *seg,
>>> + u32 seg_size,
>>> + u32 offset)
>>> +{
>>> + u32 op_addr = 0;
>>> +
>>> + if (fw_type == FW_ICCM)
>>> + op_addr = AML_OP_ICCM_RAM_BASE + offset;
>>> + else if (fw_type == FW_DCCM)
>>> + op_addr = AML_OP_DCCM_RAM_BASE + offset;
>>> +
>>> + return aml_send_tci_cmd(hdev, AML_TCI_CMD_DOWNLOAD_BT_FW,
>>> + op_addr, (u32 *)seg, seg_size); }
>>> +
>>> +static int aml_send_firmware(struct hci_dev *hdev, u8 fw_type,
>>> + u8 *fw, u32 fw_size, u32 offset) {
>>> + u32 seg_size = 0;
>>> + u32 seg_off = 0;
>>> +
>>> + if (fw_size > AML_FIRMWARE_MAX_SIZE) {
>>> + bt_dev_err(hdev, "fw_size error, fw_size:%d, max_size: 512K",
>>> + fw_size);
>>> + return -EINVAL;
>>> + }
>>> + while (fw_size > 0) {
>>> + seg_size = (fw_size > AML_FIRMWARE_OPERATION_SIZE) ?
>>> + AML_FIRMWARE_OPERATION_SIZE : fw_size;
>>> + if (aml_send_firmware_segment(hdev, fw_type, (fw +
>>> seg_off),
>>> + seg_size, offset)) {
>>> + bt_dev_err(hdev, "Failed send firmware, type:%d,
>>> offset:0x%x",
>>> + fw_type, offset);
>>> + return -EINVAL;
>>> + }
>>> + seg_off += seg_size;
>>> + fw_size -= seg_size;
>>> + offset += seg_size;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static int aml_download_firmware(struct hci_dev *hdev, const char
>>> +*fw_name) {
>>> + struct hci_uart *hu = hci_get_drvdata(hdev);
>>> + struct aml_serdev *amldev = serdev_device_get_drvdata(hu-
>>>> serdev);
>>> + const struct firmware *firmware = NULL;
>>> + struct aml_fw_len *fw_len = NULL;
>>> + u8 *iccm_start = NULL, *dccm_start = NULL;
>>> + u32 iccm_len, dccm_len;
>>> + u32 value = 0;
>>> + int ret = 0;
>>> +
>> Please consider using reverse xmas tree order - longest line to shortest - for local variable declarations in Networking code.
Okay, I will do.
>>
>>> + /* Enable firmware download event. */
>>> + value = AML_EVT_EN;
>>> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_WRITE,
>>> + AML_OP_EVT_ENABLE,
>>> + &value, sizeof(value));
>>> + if (ret)
>>> + goto exit;
>>> +
>>> + /* RAM power on */
>>> + value = AML_RAM_POWER_ON;
>>> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_WRITE,
>>> + AML_OP_RAM_POWER_CTR,
>>> + &value, sizeof(value));
>>> + if (ret)
>>> + goto exit;
>>> +
>>> + /* Check RAM power status */
>>> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_READ,
>>> + AML_OP_RAM_POWER_CTR, NULL, 0);
>>> + if (ret)
>>> + goto exit;
>>> +
>>> + ret = request_firmware(&firmware, fw_name, &hdev->dev);
>>> + if (ret < 0) {
>>> + bt_dev_err(hdev, "Failed to load <%s>:(%d)", fw_name, ret);
>>> + goto exit;
>>> + }
>>> +
>>> + fw_len = (struct aml_fw_len *)firmware->data;
>>> +
>>> + /* Download ICCM */
>>> + iccm_start = (u8 *)(firmware->data) + sizeof(struct aml_fw_len)
>>> + + amldev->aml_dev_data->iccm_offset;
>>> + iccm_len = fw_len->iccm_len - amldev->aml_dev_data->iccm_offset;
>>> + ret = aml_send_firmware(hdev, FW_ICCM, iccm_start, iccm_len,
>>> + amldev->aml_dev_data->iccm_offset);
>>> + if (ret) {
>>> + bt_dev_err(hdev, "Failed to send FW_ICCM (%d)", ret);
>>> + goto exit;
>>> + }
>>> +
>>> + /* Download DCCM */
>>> + dccm_start = (u8 *)(firmware->data) + sizeof(struct aml_fw_len) +
>>> fw_len->iccm_len;
>>> + dccm_len = fw_len->dccm_len;
>>> + ret = aml_send_firmware(hdev, FW_DCCM, dccm_start, dccm_len,
>>> + amldev->aml_dev_data->dccm_offset);
>>> + if (ret) {
>>> + bt_dev_err(hdev, "Failed to send FW_DCCM (%d)", ret);
>>> + goto exit;
>>> + }
>>> +
>>> + /* Disable firmware download event. */
>>> + value = 0;
>>> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_WRITE,
>>> + AML_OP_EVT_ENABLE,
>>> + &value, sizeof(value));
>>> + if (ret)
>>> + goto exit;
>>> +
>>> +exit:
>>> + if (firmware)
>>> + release_firmware(firmware);
>>> + return ret;
>>> +}
>>> +
>>> +static int aml_send_reset(struct hci_dev *hdev) {
>>> + struct sk_buff *skb;
>>> + int err;
>>> +
>>> + skb = __hci_cmd_sync_ev(hdev, HCI_OP_RESET, 0, NULL,
>>> + HCI_EV_CMD_COMPLETE,
>>> HCI_INIT_TIMEOUT);
>>> + if (IS_ERR(skb)) {
>>> + err = PTR_ERR(skb);
>>> + bt_dev_err(hdev, "Failed to send hci reset cmd(%d)", err);
>>> + return err;
>>> + }
>>> +
>>> + kfree_skb(skb);
>>> + return 0;
>>> +}
>>> +
>>> +static int aml_dump_fw_version(struct hci_dev *hdev) {
>>> + struct sk_buff *skb;
>>> + struct aml_tci_rsp *rsp = NULL;
>>> + u8 value[6] = {0};
>>> + u8 *fw_ver = NULL;
>>> + int err = 0;
>>> +
>> Please consider using reverse xmas tree order - longest line to shortest - for local variable declarations in Networking code.
Okay, I will do.
>>> + skb = __hci_cmd_sync_ev(hdev, AML_BT_HCI_VENDOR_CMD,
>>> sizeof(value), value,
>>> + HCI_EV_CMD_COMPLETE,
>>> HCI_INIT_TIMEOUT);
>>> + if (IS_ERR(skb)) {
>>> + skb = NULL;
>>> + err = PTR_ERR(skb);
>>> + bt_dev_err(hdev, "Failed get fw version:(%d)", err);
>>> + goto exit;
>>> + }
>>> +
>>> + rsp = (struct aml_tci_rsp *)(skb->data);
>>> + if (rsp->opcode != AML_BT_HCI_VENDOR_CMD || rsp->status !=
>>> 0x00) {
>>> + bt_dev_err(hdev, "dump version, error response
>>> (0x%04X):(%d)",
>>> + rsp->opcode, rsp->status);
>>> + err = -EINVAL;
>>> + goto exit;
>>> + }
>>> +
>>> + fw_ver = skb->data + AML_EVT_HEAD_SIZE;
>>> + bt_dev_info(hdev, "fw_version: date = %02x.%02x, number =
>>> 0x%02x%02x",
>>> + *(fw_ver + 1), *fw_ver, *(fw_ver + 3), *(fw_ver + 2));
>>> +
>>> +exit:
>>> + kfree_skb(skb);
>>> + return err;
>>> +}
>>> +
>>> +static int aml_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>>> +{
>>> + struct sk_buff *skb;
>>> + struct aml_tci_rsp *rsp = NULL;
>>> + int err = 0;
>>> +
>> Please consider using reverse xmas tree order - longest line to shortest - for local variable declarations in Networking code.
Okay, I will do.
>>> + bt_dev_info(hdev, "set bdaddr (%pM)", bdaddr);
>>> + skb = __hci_cmd_sync_ev(hdev, AML_BT_HCI_VENDOR_CMD,
>>> + sizeof(bdaddr_t), bdaddr,
>>> + HCI_EV_CMD_COMPLETE,
>>> HCI_INIT_TIMEOUT);
>>> + if (IS_ERR(skb)) {
>>> + skb = NULL;
>>> + err = PTR_ERR(skb);
>> Same here to capture error before making skb null.
> Ok, this is really the wrong order and will overwrite the error, that
> said replacing goto exit with return PTR_ERR(skb) would be enough here
> since there is nothing else that needs to be cleanup.
Well, I got it.
>
>>> + bt_dev_err(hdev, "Failed to set bdaddr:(%d)", err);
>>> + goto exit;
>>> + }
>>> +
>>> + rsp = (struct aml_tci_rsp *)(skb->data);
>>> + if (rsp->opcode != AML_BT_HCI_VENDOR_CMD || rsp->status !=
>>> 0x00) {
>>> + bt_dev_err(hdev, "error response (0x%x):(%d)", rsp->opcode,
>>> rsp->status);
>>> + err = -EINVAL;
>>> + goto exit;
>>> + }
>>> +
>>> +exit:
>>> + kfree_skb(skb);
>>> + return err;
>>> +}
>>> +
>>> +static int aml_check_bdaddr(struct hci_dev *hdev) {
>> .......
>>
>>> +
>>> +static int aml_close(struct hci_uart *hu) {
>>> + struct aml_data *aml_data = hu->priv;
>>> + struct aml_serdev *amldev = serdev_device_get_drvdata(hu-
>>>> serdev);
>> Please consider using reverse xmas tree order - longest line to shortest - for local variable declarations in Networking code.
Okay, I got it.
>>
>>> +
>>> + if (hu->serdev)
>>> + serdev_device_close(hu->serdev);
>>> +
>>> + skb_queue_purge(&aml_data->txq);
>>> + kfree_skb(aml_data->rx_skb);
>>> + kfree(aml_data);
>>> +
>>> + hu->priv = NULL;
>>> +
>>> + return aml_power_off(amldev);
>>> +}
>> .......
>>
>
> --
> Luiz Augusto von Dentz
Powered by blists - more mailing lists