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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sean,

On Tue, May 2, 2023 at 4:02 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
> ---
>  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@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..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




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux