Message-ID: <CAGp9LzpXp8T2-ZwNzX3pn3eG5MPedqVO0Z_5gikmfgHEQdwiXg@mail.gmail.com>
Date:   Tue, 16 May 2023 13:24:56 -0700
From:   Sean Wang <sean.wang@...nel.org>
To:     Luiz Augusto von Dentz <luiz.dentz@...il.com>
Cc:     sean.wang@...iatek.com, 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 v6 3/3] Bluetooth: btusb: mediatek: add MediaTek
 devcoredump support

Hi Luiz,

On Mon, May 15, 2023 at 12:08 PM Luiz Augusto von Dentz
<luiz.dentz@...il.com> wrote:
>
> Hi Sean,
>
> On Fri, May 12, 2023 at 2:20 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
> > 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@...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..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

Powered by blists - more mailing lists