Hi Kiran,
On Tue, Aug 12, 2025 at 7:30 AM Kiran K <kiran.k@xxxxxxxxx> wrote:
>
> Device coredumps do not include HCI traces, so the dependency on
> hdev_devcd*() for generating device coredumps has been removed. In the
> current implementation, firmware traces are embedded in skb and sent to
> the host for coredump processing. This refactor updates the driver to
> use devcore_dumpv() to generate coredumps directly, streamlining the
> process.
>
> Test:
> 1. cd /sys/bus/pci/devices/0000:00:14.7/
> 2. echo 1 > coredump
> 3. check /sys/class/devcoredump/ to device coredump folder
>
> Fixes: 07e6bddb54b4 ("Bluetooth: btintel_pcie: Add support for device coredump")
> Reviewed-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
> ---
> changes in v3:
> - Make a seperate patch to include vendor and driver name in device coredump
>
> changes in v2:
> - Fix compiler warning reported by android toolchain
>
> drivers/bluetooth/btintel_pcie.c | 211 ++++++++++---------------------
> 1 file changed, 65 insertions(+), 146 deletions(-)
>
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index 22a2953adbd6..a78e24aa5e38 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -15,6 +15,7 @@
> #include <linux/interrupt.h>
>
> #include <linux/unaligned.h>
> +#include <linux/devcoredump.h>
>
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -559,25 +560,6 @@ static void btintel_pcie_mac_init(struct btintel_pcie_data *data)
> btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG, reg);
> }
>
> -static int btintel_pcie_add_dmp_data(struct hci_dev *hdev, const void *data, int size)
> -{
> - struct sk_buff *skb;
> - int err;
> -
> - skb = alloc_skb(size, GFP_ATOMIC);
> - if (!skb)
> - return -ENOMEM;
> -
> - skb_put_data(skb, data, size);
> - err = hci_devcd_append(hdev, skb);
> - if (err) {
> - bt_dev_err(hdev, "Failed to append data in the coredump");
> - return err;
> - }
> -
> - return 0;
> -}
> -
> static int btintel_pcie_get_mac_access(struct btintel_pcie_data *data)
> {
> u32 reg;
> @@ -622,30 +604,33 @@ static void btintel_pcie_release_mac_access(struct btintel_pcie_data *data)
> btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG, reg);
> }
>
> -static void btintel_pcie_copy_tlv(struct sk_buff *skb, enum btintel_pcie_tlv_type type,
> - void *data, int size)
> +static void *btintel_pcie_copy_tlv(void *dest, enum btintel_pcie_tlv_type type,
> + void *data, size_t size)
> {
> struct intel_tlv *tlv;
>
> - tlv = skb_put(skb, sizeof(*tlv) + size);
> + tlv = dest;
> tlv->type = type;
> tlv->len = size;
> memcpy(tlv->val, data, tlv->len);
> + return dest + sizeof(*tlv) + size;
> }
>
> static int btintel_pcie_read_dram_buffers(struct btintel_pcie_data *data)
> {
> - u32 offset, prev_size, wr_ptr_status, dump_size, i;
> + u32 offset, prev_size, wr_ptr_status, dump_size, data_len;
> struct btintel_pcie_dbgc *dbgc = &data->dbgc;
> - u8 buf_idx, dump_time_len, fw_build;
> struct hci_dev *hdev = data->hdev;
> + u8 *pdata, *p, buf_idx;
> struct intel_tlv *tlv;
> struct timespec64 now;
> - struct sk_buff *skb;
> struct tm tm_now;
> - char buf[256];
> - u16 hdr_len;
> - int ret;
> + char fw_build[128];
> + char ts[128];
> +
> + if (!IS_ENABLED(CONFIG_DEV_COREDUMP))
> + return -EOPNOTSUPP;
> +
>
> wr_ptr_status = btintel_pcie_rd_dev_mem(data, BTINTEL_PCIE_DBGC_CUR_DBGBUFF_STATUS);
> offset = wr_ptr_status & BTINTEL_PCIE_DBG_OFFSET_BIT_MASK;
> @@ -664,92 +649,79 @@ static int btintel_pcie_read_dram_buffers(struct btintel_pcie_data *data)
>
> ktime_get_real_ts64(&now);
> time64_to_tm(now.tv_sec, 0, &tm_now);
> - dump_time_len = snprintf(buf, sizeof(buf), "Dump Time: %02d-%02d-%04ld %02d:%02d:%02d",
> + snprintf(ts, sizeof(ts), "Dump Time: %02d-%02d-%04ld %02d:%02d:%02d",
> tm_now.tm_mday, tm_now.tm_mon + 1, tm_now.tm_year + 1900,
> tm_now.tm_hour, tm_now.tm_min, tm_now.tm_sec);
>
> - fw_build = snprintf(buf + dump_time_len, sizeof(buf) - dump_time_len,
> + snprintf(fw_build, sizeof(fw_build),
> "Firmware Timestamp: Year %u WW %02u buildtype %u build %u",
> 2000 + (data->dmp_hdr.fw_timestamp >> 8),
> data->dmp_hdr.fw_timestamp & 0xff, data->dmp_hdr.fw_build_type,
> data->dmp_hdr.fw_build_num);
>
> - hdr_len = sizeof(*tlv) + sizeof(data->dmp_hdr.cnvi_bt) +
> - sizeof(*tlv) + sizeof(data->dmp_hdr.write_ptr) +
> - sizeof(*tlv) + sizeof(data->dmp_hdr.wrap_ctr) +
> - sizeof(*tlv) + sizeof(data->dmp_hdr.trigger_reason) +
> - sizeof(*tlv) + sizeof(data->dmp_hdr.fw_git_sha1) +
> - sizeof(*tlv) + sizeof(data->dmp_hdr.cnvr_top) +
> - sizeof(*tlv) + sizeof(data->dmp_hdr.cnvi_top) +
> - sizeof(*tlv) + dump_time_len +
> - sizeof(*tlv) + fw_build;
> + data_len = sizeof(*tlv) + sizeof(data->dmp_hdr.cnvi_bt) +
> + sizeof(*tlv) + sizeof(data->dmp_hdr.write_ptr) +
> + sizeof(*tlv) + sizeof(data->dmp_hdr.wrap_ctr) +
> + sizeof(*tlv) + sizeof(data->dmp_hdr.trigger_reason) +
> + sizeof(*tlv) + sizeof(data->dmp_hdr.fw_git_sha1) +
> + sizeof(*tlv) + sizeof(data->dmp_hdr.cnvr_top) +
> + sizeof(*tlv) + sizeof(data->dmp_hdr.cnvi_top) +
> + sizeof(*tlv) + strlen(ts) +
> + sizeof(*tlv) + strlen(fw_build);
>
> - dump_size = hdr_len + sizeof(hdr_len);
> + /*
> + * sizeof(u32) - signature
> + * sizeof(data_len) - to store tlv data size
> + * data_len - TLV data
> + */
> + dump_size = sizeof(u32) + sizeof(data_len) + data_len;
>
> - skb = alloc_skb(dump_size, GFP_KERNEL);
> - if (!skb)
> - return -ENOMEM;
>
> /* Add debug buffers data length to dump size */
> dump_size += BTINTEL_PCIE_DBGC_BUFFER_SIZE * dbgc->count;
>
> - ret = hci_devcd_init(hdev, dump_size);
> - if (ret) {
> - bt_dev_err(hdev, "Failed to init devcoredump, err %d", ret);
> - kfree_skb(skb);
> - return ret;
> - }
> + pdata = vmalloc(dump_size);
> + if (!pdata)
> + return -ENOMEM;
> + p = pdata;
>
> - skb_put_data(skb, &hdr_len, sizeof(hdr_len));
> + *(u32 *)p = BTINTEL_PCIE_MAGIC_NUM;
> + p += sizeof(u32);
>
> - btintel_pcie_copy_tlv(skb, BTINTEL_CNVI_BT, &data->dmp_hdr.cnvi_bt,
> - sizeof(data->dmp_hdr.cnvi_bt));
> + *(u32 *)p = data_len;
> + p += sizeof(u32);
>
> - btintel_pcie_copy_tlv(skb, BTINTEL_WRITE_PTR, &data->dmp_hdr.write_ptr,
> - sizeof(data->dmp_hdr.write_ptr));
> + p = btintel_pcie_copy_tlv(p, BTINTEL_DUMP_TIME, ts, strlen(ts));
> + p = btintel_pcie_copy_tlv(p, BTINTEL_FW_BUILD, fw_build,
> + strlen(fw_build));
> + p = btintel_pcie_copy_tlv(p, BTINTEL_CNVI_BT, &data->dmp_hdr.cnvi_bt,
> + sizeof(data->dmp_hdr.cnvi_bt));
> + p = btintel_pcie_copy_tlv(p, BTINTEL_WRITE_PTR, &data->dmp_hdr.write_ptr,
> + sizeof(data->dmp_hdr.write_ptr));
> + p = btintel_pcie_copy_tlv(p, BTINTEL_WRAP_CTR, &data->dmp_hdr.wrap_ctr,
> + sizeof(data->dmp_hdr.wrap_ctr));
>
> data->dmp_hdr.wrap_ctr = btintel_pcie_rd_dev_mem(data,
> BTINTEL_PCIE_DBGC_DBGBUFF_WRAP_ARND);
>
> - btintel_pcie_copy_tlv(skb, BTINTEL_WRAP_CTR, &data->dmp_hdr.wrap_ctr,
> - sizeof(data->dmp_hdr.wrap_ctr));
> -
> - btintel_pcie_copy_tlv(skb, BTINTEL_TRIGGER_REASON, &data->dmp_hdr.trigger_reason,
> - sizeof(data->dmp_hdr.trigger_reason));
> -
> - btintel_pcie_copy_tlv(skb, BTINTEL_FW_SHA, &data->dmp_hdr.fw_git_sha1,
> - sizeof(data->dmp_hdr.fw_git_sha1));
> -
> - btintel_pcie_copy_tlv(skb, BTINTEL_CNVR_TOP, &data->dmp_hdr.cnvr_top,
> - sizeof(data->dmp_hdr.cnvr_top));
> -
> - btintel_pcie_copy_tlv(skb, BTINTEL_CNVI_TOP, &data->dmp_hdr.cnvi_top,
> - sizeof(data->dmp_hdr.cnvi_top));
> -
> - btintel_pcie_copy_tlv(skb, BTINTEL_DUMP_TIME, buf, dump_time_len);
> -
> - btintel_pcie_copy_tlv(skb, BTINTEL_FW_BUILD, buf + dump_time_len, fw_build);
> -
> - ret = hci_devcd_append(hdev, skb);
> - if (ret)
> - goto exit_err;
> -
> - for (i = 0; i < dbgc->count; i++) {
> - ret = btintel_pcie_add_dmp_data(hdev, dbgc->bufs[i].data,
> - BTINTEL_PCIE_DBGC_BUFFER_SIZE);
> - if (ret)
> - break;
> - }
> -
> -exit_err:
> - hci_devcd_complete(hdev);
> - return ret;
> + p = btintel_pcie_copy_tlv(p, BTINTEL_TRIGGER_REASON, &data->dmp_hdr.trigger_reason,
> + sizeof(data->dmp_hdr.trigger_reason));
> + p = btintel_pcie_copy_tlv(p, BTINTEL_FW_SHA, &data->dmp_hdr.fw_git_sha1,
> + sizeof(data->dmp_hdr.fw_git_sha1));
> + p = btintel_pcie_copy_tlv(p, BTINTEL_CNVR_TOP, &data->dmp_hdr.cnvr_top,
> + sizeof(data->dmp_hdr.cnvr_top));
> + p = btintel_pcie_copy_tlv(p, BTINTEL_CNVI_TOP, &data->dmp_hdr.cnvi_top,
> + sizeof(data->dmp_hdr.cnvi_top));
> +
> + memcpy(p, dbgc->bufs[0].data, dbgc->count * BTINTEL_PCIE_DBGC_BUFFER_SIZE);
> + dev_coredumpv(&hdev->dev, pdata, dump_size, GFP_KERNEL);
We are missing the following logic as well:
/* Send a copy to monitor as a diagnostic packet */
skb = bt_skb_alloc(size, GFP_ATOMIC);
if (skb) {
skb_put_data(skb, hdev->dump.head, size);
hci_recv_diag(hdev, skb);
}
So it gets included in the btmon traces for example.
> + return 0;
> }
>
> static void btintel_pcie_dump_traces(struct hci_dev *hdev)
> {
> struct btintel_pcie_data *data = hci_get_drvdata(hdev);
> - int ret = 0;
> + int ret;
>
> ret = btintel_pcie_get_mac_access(data);
> if (ret) {
> @@ -765,51 +737,6 @@ static void btintel_pcie_dump_traces(struct hci_dev *hdev)
> bt_dev_err(hdev, "Failed to dump traces: (%d)", ret);
> }
>
> -static void btintel_pcie_dump_hdr(struct hci_dev *hdev, struct sk_buff *skb)
> -{
> - struct btintel_pcie_data *data = hci_get_drvdata(hdev);
> - u16 len = skb->len;
> - u16 *hdrlen_ptr;
> - char buf[80];
> -
> - hdrlen_ptr = skb_put_zero(skb, sizeof(len));
> -
> - snprintf(buf, sizeof(buf), "Controller Name: 0x%X\n",
> - INTEL_HW_VARIANT(data->dmp_hdr.cnvi_bt));
> - skb_put_data(skb, buf, strlen(buf));
> -
> - snprintf(buf, sizeof(buf), "Firmware Build Number: %u\n",
> - data->dmp_hdr.fw_build_num);
> - skb_put_data(skb, buf, strlen(buf));
> -
> - snprintf(buf, sizeof(buf), "Driver: %s\n", data->dmp_hdr.driver_name);
> - skb_put_data(skb, buf, strlen(buf));
> -
> - snprintf(buf, sizeof(buf), "Vendor: Intel\n");
> - skb_put_data(skb, buf, strlen(buf));
> -
> - *hdrlen_ptr = skb->len - len;
> -}
> -
> -static void btintel_pcie_dump_notify(struct hci_dev *hdev, int state)
> -{
> - struct btintel_pcie_data *data = hci_get_drvdata(hdev);
> -
> - switch (state) {
> - case HCI_DEVCOREDUMP_IDLE:
> - data->dmp_hdr.state = HCI_DEVCOREDUMP_IDLE;
> - break;
> - case HCI_DEVCOREDUMP_ACTIVE:
> - data->dmp_hdr.state = HCI_DEVCOREDUMP_ACTIVE;
> - break;
> - case HCI_DEVCOREDUMP_TIMEOUT:
> - case HCI_DEVCOREDUMP_ABORT:
> - case HCI_DEVCOREDUMP_DONE:
> - data->dmp_hdr.state = HCI_DEVCOREDUMP_IDLE;
> - break;
> - }
> -}
> -
> /* This function enables BT function by setting BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_INIT bit in
> * BTINTEL_PCIE_CSR_FUNC_CTRL_REG register and wait for MSI-X with
> * BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0.
> @@ -1383,6 +1310,11 @@ static void btintel_pcie_rx_work(struct work_struct *work)
> struct btintel_pcie_data, rx_work);
> struct sk_buff *skb;
>
> + if (test_bit(BTINTEL_PCIE_COREDUMP_INPROGRESS, &data->flags)) {
> + btintel_pcie_dump_traces(data->hdev);
> + clear_bit(BTINTEL_PCIE_COREDUMP_INPROGRESS, &data->flags);
> + }
> +
> if (test_bit(BTINTEL_PCIE_HWEXP_INPROGRESS, &data->flags)) {
> /* Unlike usb products, controller will not send hardware
> * exception event on exception. Instead controller writes the
> @@ -1395,11 +1327,6 @@ static void btintel_pcie_rx_work(struct work_struct *work)
> clear_bit(BTINTEL_PCIE_HWEXP_INPROGRESS, &data->flags);
> }
>
> - if (test_bit(BTINTEL_PCIE_COREDUMP_INPROGRESS, &data->flags)) {
> - btintel_pcie_dump_traces(data->hdev);
> - clear_bit(BTINTEL_PCIE_COREDUMP_INPROGRESS, &data->flags);
> - }
> -
> /* Process the sk_buf in queue and send to the HCI layer */
> while ((skb = skb_dequeue(&data->rx_skb_q))) {
> btintel_pcie_recv_frame(data, skb);
> @@ -2190,13 +2117,6 @@ static int btintel_pcie_setup_internal(struct hci_dev *hdev)
> if (ver_tlv.img_type == 0x02 || ver_tlv.img_type == 0x03)
> data->dmp_hdr.fw_git_sha1 = ver_tlv.git_sha1;
>
> - err = hci_devcd_register(hdev, btintel_pcie_dump_traces, btintel_pcie_dump_hdr,
> - btintel_pcie_dump_notify);
> - if (err) {
> - bt_dev_err(hdev, "Failed to register coredump (%d)", err);
> - goto exit_error;
> - }
> -
> btintel_print_fseq_info(hdev);
> exit_error:
> kfree_skb(skb);
> @@ -2325,7 +2245,6 @@ static void btintel_pcie_removal_work(struct work_struct *wk)
> btintel_pcie_synchronize_irqs(data);
>
> flush_work(&data->rx_work);
> - flush_work(&data->hdev->dump.dump_rx);
>
> bt_dev_dbg(data->hdev, "Release bluetooth interface");
> btintel_pcie_release_hdev(data);
> --
> 2.43.0
>
>
--
Luiz Augusto von Dentz
