| 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: <CABBYNZJQjoTzqmivUbLnDnn826pStW+kTuicYN1qVuHDoQjfmw@mail.gmail.com>
Date: Mon, 8 May 2023 12:54:15 -0700
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: sean.wang@...iatek.com, Manish Mandlik <mmandlik@...gle.com>
Cc: marcel@...tmann.org, johan.hedberg@...il.com,
chris.lu@...iatek.com, Soul.Huang@...iatek.com,
Leon.Yen@...iatek.com, Deren.Wu@...iatek.com, km.lin@...iatek.com,
robin.chiu@...iatek.com, Eddie.Chen@...iatek.com,
ch.yeh@...iatek.com, jenhao.yang@...iatek.com,
Stella.Chang@...iatek.com, Tom.Chou@...iatek.com,
steve.lee@...iatek.com, jsiuda@...gle.com, frankgor@...gle.com,
abhishekpandit@...gle.com, michaelfsun@...gle.com,
abhishekpandit@...omium.org, mcchou@...omium.org,
shawnku@...gle.com, linux-bluetooth@...r.kernel.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
Jing Cai <jing.cai@...iatek.com>
Subject: Re: [PATCH v5 3/3] Bluetooth: btusb: mediatek: add MediaTek
devcoredump support
Hi Sean,
On Tue, May 2, 2023 at 4:02 PM <sean.wang@...iatek.com> wrote:
>
> From: Jing Cai <jing.cai@...iatek.com>
>
> This patch implement function .coredump() and dmp_hdr() in btusb
> driver for MediaTek controller. FW core dump was triggered by FW
> specific event to show something unexpected happened in the controller.
>
> The driver would be responsible for collecting and uploading the device
> core dump pieces in hci driver using core dump API. Once we finished
> the whole process, the driver would reset the controller to recover the
> kind of fatal error.
>
> Co-developed-by: Chris Lu <chris.lu@...iatek.com>
> Signed-off-by: Chris Lu <chris.lu@...iatek.com>
> Co-developed-by: Sean Wang <sean.wang@...iatek.com>
> Signed-off-by: Sean Wang <sean.wang@...iatek.com>
> Signed-off-by: Jing Cai <jing.cai@...iatek.com>
> ---
> v2, v3: rebase onto the latest codebase
> v4: update the newest API usage for the coredump which was already
> into the upstream
> v5: support devcoredump on hdev basis
> ---
> drivers/bluetooth/btmtk.c | 117 ++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btmtk.h | 28 +++++++++
> drivers/bluetooth/btusb.c | 14 +++++
> 3 files changed, 159 insertions(+)
>
> diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c
> index ac2fac7e3c5f..657792f9dcab 100644
> --- a/drivers/bluetooth/btmtk.c
> +++ b/drivers/bluetooth/btmtk.c
> @@ -19,6 +19,12 @@
> #define MTK_SEC_MAP_COMMON_SIZE 12
> #define MTK_SEC_MAP_NEED_SEND_SIZE 52
>
> +enum {
> + BTMTK_COREDUMP_INIT,
> + BTMTK_COREDUMP_DISABLED,
> + BTMTK_COREDUMP_ACTIVE,
> +};
> +
> struct btmtk_patch_header {
> u8 datetime[16];
> u8 platform[4];
> @@ -53,6 +59,56 @@ struct btmtk_section_map {
> };
> } __packed;
>
> +static void btmtk_coredump(struct hci_dev *hdev)
> +{
> + int err;
> +
> + err = __hci_cmd_send(hdev, 0xfd5b, 0, NULL);
> + if (err < 0)
> + bt_dev_err(hdev, "Coredump failed (%d)", err);
> +}
> +
> +static void btmtk_coredump_hdr(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> + struct btmtk_data *data = hci_get_priv(hdev);
> + char buf[80];
> +
> + snprintf(buf, sizeof(buf), "Controller Name: 0x%X\n",
> + data->cd_info.dev_id);
> + skb_put_data(skb, buf, strlen(buf));
> +
> + snprintf(buf, sizeof(buf), "Firmware Version: 0x%X\n",
> + data->cd_info.fw_version);
> + skb_put_data(skb, buf, strlen(buf));
> +
> + snprintf(buf, sizeof(buf), "Driver: %s\n",
> + data->cd_info.driver_name);
> + skb_put_data(skb, buf, strlen(buf));
> +
> + snprintf(buf, sizeof(buf), "Vendor: MediaTek\n");
> + skb_put_data(skb, buf, strlen(buf));
> +}
> +
> +static void btmtk_coredump_notify(struct hci_dev *hdev, int state)
> +{
> + struct btmtk_data *data = hci_get_priv(hdev);
> +
> + switch (state) {
> + case HCI_DEVCOREDUMP_IDLE:
> + data->cd_info.state = BTMTK_COREDUMP_INIT;
> + break;
> + case HCI_DEVCOREDUMP_ACTIVE:
> + data->cd_info.state = BTMTK_COREDUMP_ACTIVE;
> + break;
> + case HCI_DEVCOREDUMP_TIMEOUT:
> + case HCI_DEVCOREDUMP_ABORT:
> + case HCI_DEVCOREDUMP_DONE:
> + data->cd_info.state = BTMTK_COREDUMP_INIT;
> + btmtk_reset_sync(hdev);
> + break;
> + }
> +}
Don't really like where this is going, why can't you just use the
state from devcd?
> +
> int btmtk_setup_firmware_79xx(struct hci_dev *hdev, const char *fwname,
> wmt_cmd_sync_func_t wmt_cmd_sync)
> {
> @@ -295,6 +351,67 @@ void btmtk_reset_sync(struct hci_dev *hdev)
> }
> EXPORT_SYMBOL_GPL(btmtk_reset_sync);
>
> +int btmtk_register_coredump(struct hci_dev *hdev, u32 dev_id,
> + const char *name, u32 fw_version)
> +{
> + struct btmtk_data *data = hci_get_priv(hdev);
> +
> + if (!IS_ENABLED(CONFIG_DEV_COREDUMP))
> + return -EOPNOTSUPP;
> +
> + data->cd_info.dev_id = dev_id;
> + data->cd_info.fw_version = fw_version;
> + data->cd_info.state = BTMTK_COREDUMP_INIT;
> + strncpy(data->cd_info.driver_name, name, MTK_DRIVER_NAME_LEN - 1);
Im not really sure why every devcd is having to store the driver name
as a copy, the driver name is already accessible via
hdev->dev->driver->name.
@Manish Mandlik we might want to fix any code that still is doing
copies of driver name like above.
> +
> + return hci_devcd_register(hdev, btmtk_coredump, btmtk_coredump_hdr,
> + btmtk_coredump_notify);
> +}
> +EXPORT_SYMBOL_GPL(btmtk_register_coredump);
> +
> +int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> + struct btmtk_data *data = hci_get_priv(hdev);
> + int err;
> +
> + if (!IS_ENABLED(CONFIG_DEV_COREDUMP))
> + return 0;
> +
> + switch (data->cd_info.state) {
> + case BTMTK_COREDUMP_DISABLED:
> + err = -EINVAL;
> + break;
> + case BTMTK_COREDUMP_INIT:
> + err = hci_devcd_init(hdev, MTK_COREDUMP_SIZE);
> + if (err < 0)
> + break;
> + /* It is supposed coredump can be done within 5 seconds */
> + schedule_delayed_work(&hdev->dump.dump_timeout,
> + msecs_to_jiffies(5000));
> + fallthrough;
> + case BTMTK_COREDUMP_ACTIVE:
> + default:
> + err = hci_devcd_append(hdev, skb);
> + if (err < 0)
> + break;
> +
> + if (skb->len > 12 &&
> + !strncmp((char *)&skb->data[skb->len - 13],
> + MTK_COREDUMP_END, 12))
> + hci_devcd_complete(hdev);
> +
> + break;
> + }
> +
> + if (err < 0) {
> + data->cd_info.state = BTMTK_COREDUMP_DISABLED;
> + kfree_skb(skb);
> + }
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(btmtk_process_coredump);
> +
> MODULE_AUTHOR("Sean Wang <sean.wang@...iatek.com>");
> MODULE_AUTHOR("Mark Chen <mark-yw.chen@...iatek.com>");
> MODULE_DESCRIPTION("Bluetooth support for MediaTek devices ver " VERSION);
> diff --git a/drivers/bluetooth/btmtk.h b/drivers/bluetooth/btmtk.h
> index 6245662f6ccb..7eb162f0e7aa 100644
> --- a/drivers/bluetooth/btmtk.h
> +++ b/drivers/bluetooth/btmtk.h
> @@ -21,6 +21,10 @@
> #define MT7921_DLSTATUS 0x7c053c10
> #define BT_DL_STATE BIT(1)
>
> +#define MTK_DRIVER_NAME_LEN 16
> +#define MTK_COREDUMP_SIZE (1024 * 1000)
> +#define MTK_COREDUMP_END "coredump end"
> +
> enum {
> BTMTK_WMT_PATCH_DWNLD = 0x1,
> BTMTK_WMT_TEST = 0x2,
> @@ -119,12 +123,20 @@ struct btmtk_hci_wmt_params {
> u32 *status;
> };
>
> +struct btmtk_coredump_info {
> + char driver_name[MTK_DRIVER_NAME_LEN];
> + u32 dev_id;
> + u32 fw_version;
> + int state;
> +};
> +
> typedef int (*wmt_cmd_sync_func_t)(struct hci_dev *,
> struct btmtk_hci_wmt_params *);
>
> typedef int (*btmtk_reset_sync_func_t)(struct hci_dev *, void *);
>
> struct btmtk_data {
> + struct btmtk_coredump_info cd_info;
> btmtk_reset_sync_func_t reset_sync;
> };
>
> @@ -139,6 +151,11 @@ int btmtk_setup_firmware(struct hci_dev *hdev, const char *fwname,
> wmt_cmd_sync_func_t wmt_cmd_sync);
>
> void btmtk_reset_sync(struct hci_dev *hdev);
> +
> +int btmtk_register_coredump(struct hci_dev *hdev, u32 dev_id, const char *name,
> + u32 fw_version);
> +
> +int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb);
> #else
>
> static inline int btmtk_set_bdaddr(struct hci_dev *hdev,
> @@ -162,4 +179,15 @@ static int btmtk_setup_firmware(struct hci_dev *hdev, const char *fwname,
> static void btmtk_reset_sync(struct hci_dev *hdev)
> {
> }
> +
> +static int btmtk_register_coredump(struct hci_dev *hdev, u32 dev_id, const char *name,
> + u32 fw_version)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> + return -EOPNOTSUPP;
> +}
> #endif
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 034edd8ad777..1c2a0cbcf62e 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3035,6 +3035,10 @@ static int btusb_mtk_setup(struct hci_dev *hdev)
> }
>
> btmtk_data->reset_sync = btusb_mtk_reset_work;
> + err = btmtk_register_coredump(hdev, dev_id, btusb_driver.name,
> + fw_version);
> + if (err < 0)
> + bt_dev_err(hdev, "Failed to register coredump (%d)", err);
>
> switch (dev_id) {
> case 0x7663:
> @@ -3189,6 +3193,7 @@ static int btusb_recv_acl_mtk(struct hci_dev *hdev, struct sk_buff *skb)
> {
> struct btusb_data *data = hci_get_drvdata(hdev);
> u16 handle = le16_to_cpu(hci_acl_hdr(skb)->handle);
> + struct sk_buff *skb_cd;
>
> switch (handle) {
> case 0xfc6f: /* Firmware dump from device */
> @@ -3196,6 +3201,15 @@ static int btusb_recv_acl_mtk(struct hci_dev *hdev, struct sk_buff *skb)
> * suspend and thus disable auto-suspend.
> */
> usb_disable_autosuspend(data->udev);
> +
> + /* We need to forward the diagnostic packet to userspace daemon
> + * for backward compatibility, so we have to clone the packet
> + * extraly for the in-kernel coredump support.
> + */
> + skb_cd = skb_clone(skb, GFP_ATOMIC);
> + if (skb_cd)
> + btmtk_process_coredump(hdev, skb_cd);
> +
> fallthrough;
> case 0x05ff: /* Firmware debug logging 1 */
> case 0x05fe: /* Firmware debug logging 2 */
> --
> 2.25.1
>
--
Luiz Augusto von Dentz
Powered by blists - more mailing lists