Re: [PATCH v6 3/3] Bluetooth: btusb: mediatek: add MediaTek devcoredump support

From: Sean Wang
Date: Tue May 16 2023 - 16:25:40 EST

Next message: Arnd Bergmann: "[PATCH] staging: vchiq_arm: mark vchiq_platform_init() static"
Previous message: Yazen Ghannam: "[PATCH 6/6] hwmon: (k10temp) Reduce k10temp_get_ccd_support() parameters"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Luiz,

On Mon, May 15, 2023 at 12:08 PM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Sean,
>
> On Fri, May 12, 2023 at 2:20 PM <sean.wang@xxxxxxxxxxxx> wrote:
> >
> > From: Jing Cai <jing.cai@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > Signed-off-by: Chris Lu <chris.lu@xxxxxxxxxxxx>
> > Co-developed-by: Sean Wang <sean.wang@xxxxxxxxxxxx>
> > Signed-off-by: Sean Wang <sean.wang@xxxxxxxxxxxx>
> > Signed-off-by: Jing Cai <jing.cai@xxxxxxxxxxxx>
> > ---
> > 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
> > v6: reuse the coredump state HCI_DEVCOREDUMP_* in driver
> > and drop the driver name copying
> > ---
> > drivers/bluetooth/btmtk.c | 106 ++++++++++++++++++++++++++++++++++++++
> > drivers/bluetooth/btmtk.h | 27 ++++++++++
> > drivers/bluetooth/btusb.c | 14 +++++
> > 3 files changed, 147 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c
> > index ac2fac7e3c5f..60233afc0a9c 100644
> > --- a/drivers/bluetooth/btmtk.c
> > +++ b/drivers/bluetooth/btmtk.c
> > @@ -53,6 +53,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 = HCI_DEVCOREDUMP_IDLE;
> > + break;
> > + case HCI_DEVCOREDUMP_ACTIVE:
> > + data->cd_info.state = HCI_DEVCOREDUMP_ACTIVE;
> > + break;
> > + case HCI_DEVCOREDUMP_TIMEOUT:
> > + case HCI_DEVCOREDUMP_ABORT:
> > + case HCI_DEVCOREDUMP_DONE:
> > + data->cd_info.state = HCI_DEVCOREDUMP_IDLE;
> > + btmtk_reset_sync(hdev);
> > + break;
> > + }
> > +}
> > +
> > int btmtk_setup_firmware_79xx(struct hci_dev *hdev, const char *fwname,
> > wmt_cmd_sync_func_t wmt_cmd_sync)
> > {
> > @@ -295,6 +345,62 @@ 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 = HCI_DEVCOREDUMP_IDLE;
> > + data->cd_info.driver_name = name;
> > +
> > + 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 HCI_DEVCOREDUMP_IDLE:
> > + 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 HCI_DEVCOREDUMP_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);
>
> This is a bad idea, not only it is inefficient since you have to
> compare the end after each append but it also assumes the size of
> MTK_COREDUMP_END to be 12, so Id scrap this code and probably use the
> return of hci_devcd_append or have the code in hci_devcd_append detect
> it has reached the end.

MTK_COREDUMP_END is a unique string of exactly 12 characters. It acts
as a critical marker, signifying the end of the MTK core dump. This
flag is guaranteed to be present in every instance of an MTK core
dump. Also, MTK_COREDUMP_END must take the last position in the core
dump, emphasizing its importance. While this method might be a bit
inefficient, which doesn't happen usually but also it provides an only
accurate way to determine the conclusion of an MTK core dump.
>
> > +
> > + break;
> > + }
> > +
> > + if (err < 0)
> > + kfree_skb(skb);
> > +
> > + return err;
> > +}
> > +EXPORT_SYMBOL_GPL(btmtk_process_coredump);
> > +
> > MODULE_AUTHOR("Sean Wang <sean.wang@xxxxxxxxxxxx>");
> > MODULE_AUTHOR("Mark Chen <mark-yw.chen@xxxxxxxxxxxx>");
> > MODULE_DESCRIPTION("Bluetooth support for MediaTek devices ver " VERSION);
> > diff --git a/drivers/bluetooth/btmtk.h b/drivers/bluetooth/btmtk.h
> > index 6245662f6ccb..852a67db8f80 100644
> > --- a/drivers/bluetooth/btmtk.h
> > +++ b/drivers/bluetooth/btmtk.h
> > @@ -21,6 +21,9 @@
> > #define MT7921_DLSTATUS 0x7c053c10
> > #define BT_DL_STATE BIT(1)
> >
> > +#define MTK_COREDUMP_SIZE (1024 * 1000)
> > +#define MTK_COREDUMP_END "coredump end"
> > +
> > enum {
> > BTMTK_WMT_PATCH_DWNLD = 0x1,
> > BTMTK_WMT_TEST = 0x2,
> > @@ -119,12 +122,20 @@ struct btmtk_hci_wmt_params {
> > u32 *status;
> > };
> >
> > +struct btmtk_coredump_info {
> > + const char *driver_name;
> > + 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 +150,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 +178,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

Next message: Arnd Bergmann: "[PATCH] staging: vchiq_arm: mark vchiq_platform_init() static"
Previous message: Yazen Ghannam: "[PATCH 6/6] hwmon: (k10temp) Reduce k10temp_get_ccd_support() parameters"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]