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: <CABBYNZKO_02eBmDDKN2ReviM+SpEzozYPbohjBPjYtf1MqAzuQ@mail.gmail.com>
Date: Thu, 18 Jul 2024 15:01:58 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Sai Krishna Gajula <saikrishnag@...vell.com>
Cc: "yang.li@...ogic.com" <yang.li@...ogic.com>, 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

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?

>
> > +
> > +     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.

> > +             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.

> > +     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.
>
> > +     /* 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.
>
> > +     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.
>
> > +     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.

> > +             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.
>
> > +
> > +     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

Powered by Openwall GNU/*/Linux Powered by OpenVZ