From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
To: Zijun Hu <zijuhu@codeaurora.org>
Cc: Marcel Holtmann <marcel@holtmann.org>,
Johan Hedberg <johan.hedberg@gmail.com>,
LKML <linux-kernel@vger.kernel.org>,
Bluez mailing list <linux-bluetooth@vger.kernel.org>,
linux-arm-msm@vger.kernel.org, bgodavar@codeaurora.org,
c-hbandi@codeaurora.org, hemantg@codeaurora.org,
Matthias Kaehlcke <mka@chromium.org>,
rjliao@codeaurora.org
Subject: Re: [PATCH v2] bluetooth: hci_qca: Fix QCA6390 memdump failure
Date: Wed, 27 May 2020 20:42:46 -0700 [thread overview]
Message-ID: <CANFp7mXMiYKY-33xZX2MaHd5RyicbRb2fZHo8mk4-VM_Jf47UQ@mail.gmail.com> (raw)
In-Reply-To: <1590550627-24618-1-git-send-email-zijuhu@codeaurora.org>
Hi Zijun,
On Tue, May 26, 2020 at 8:37 PM Zijun Hu <zijuhu@codeaurora.org> wrote:
>
> QCA6390 memdump VSE sometimes come to bluetooth driver
> with wrong sequence number as illustrated as follows:
> frame # in DEC: frame data in HEX
> 1396: ff fd 01 08 74 05 00 37 8f 14
> 1397: ff fd 01 08 75 05 00 ff bf 38
> 1414: ff fd 01 08 86 05 00 fb 5e 4b
> 1399: ff fd 01 08 77 05 00 f3 44 0a
> 1400: ff fd 01 08 78 05 00 ca f7 41
> it is mistook for controller missing packets, so results
> in page fault after overwriting memdump buffer allocated.
>
> it is fixed by ignoring QCA6390 sequence number error
> and checking buffer space before writing.
>
> Signed-off-by: Zijun Hu <zijuhu@codeaurora.org>
> ---
> drivers/bluetooth/hci_qca.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index e4a6823..388fe01b 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -114,6 +114,7 @@ struct qca_memdump_data {
> char *memdump_buf_tail;
> u32 current_seq_no;
> u32 received_dump;
> + u32 ram_dump_size;
> };
>
> struct qca_memdump_event_hdr {
> @@ -976,6 +977,8 @@ static void qca_controller_memdump(struct work_struct *work)
> char nullBuff[QCA_DUMP_PACKET_SIZE] = { 0 };
> u16 seq_no;
> u32 dump_size;
> + u32 rx_size;
> + enum qca_btsoc_type soc_type = qca_soc_type(hu);
>
> while ((skb = skb_dequeue(&qca->rx_memdump_q))) {
>
> @@ -1029,6 +1032,7 @@ static void qca_controller_memdump(struct work_struct *work)
>
> skb_pull(skb, sizeof(dump_size));
> memdump_buf = vmalloc(dump_size);
> + qca_memdump->ram_dump_size = dump_size;
> qca_memdump->memdump_buf_head = memdump_buf;
> qca_memdump->memdump_buf_tail = memdump_buf;
> }
> @@ -1052,25 +1056,52 @@ static void qca_controller_memdump(struct work_struct *work)
> * packets in the buffer.
> */
> while ((seq_no > qca_memdump->current_seq_no + 1) &&
> + (soc_type != QCA_QCA6390) &&
This probably shouldn't be SOC specific.
> seq_no != QCA_LAST_SEQUENCE_NUM) {
> bt_dev_err(hu->hdev, "QCA controller missed packet:%d",
> qca_memdump->current_seq_no);
> + rx_size = qca_memdump->received_dump;
> + rx_size += QCA_DUMP_PACKET_SIZE;
> + if (rx_size > qca_memdump->ram_dump_size) {
> + bt_dev_err(hu->hdev,
> + "QCA memdump received %d, no space for missed packet",
> + qca_memdump->received_dump);
> + break;
> + }
> memcpy(memdump_buf, nullBuff, QCA_DUMP_PACKET_SIZE);
> memdump_buf = memdump_buf + QCA_DUMP_PACKET_SIZE;
> qca_memdump->received_dump += QCA_DUMP_PACKET_SIZE;
> qca_memdump->current_seq_no++;
> }
You can replace this loop with a memset(memdump_buf, 0, (seq_no -
qca_memdump->current_seq_no) * QCA_DUMP_PACKET_SIZE). This simplifies
the ram_dump_size check as well because it won't zero fill until the
end anymore (meaning a single bad seq_no doesn't make the rest of the
dump incorrect).
>
> - memcpy(memdump_buf, (unsigned char *) skb->data, skb->len);
> - memdump_buf = memdump_buf + skb->len;
> - qca_memdump->memdump_buf_tail = memdump_buf;
> - qca_memdump->current_seq_no = seq_no + 1;
> - qca_memdump->received_dump += skb->len;
> + rx_size = qca_memdump->received_dump + skb->len;
> + if (rx_size <= qca_memdump->ram_dump_size) {
> + if ((seq_no != QCA_LAST_SEQUENCE_NUM) &&
> + (seq_no != qca_memdump->current_seq_no))
> + bt_dev_err(hu->hdev,
> + "QCA memdump unexpected packet %d",
> + seq_no);
> + bt_dev_dbg(hu->hdev,
> + "QCA memdump packet %d with length %d",
> + seq_no, skb->len);
> + memcpy(memdump_buf, (unsigned char *)skb->data,
> + skb->len);
> + memdump_buf = memdump_buf + skb->len;
> + qca_memdump->memdump_buf_tail = memdump_buf;
> + qca_memdump->current_seq_no = seq_no + 1;
> + qca_memdump->received_dump += skb->len;
> + } else {
> + bt_dev_err(hu->hdev,
> + "QCA memdump received %d, no space for packet %d",
> + qca_memdump->received_dump, seq_no);
> + }
> qca->qca_memdump = qca_memdump;
> kfree_skb(skb);
> if (seq_no == QCA_LAST_SEQUENCE_NUM) {
> - bt_dev_info(hu->hdev, "QCA writing crash dump of size %d bytes",
> - qca_memdump->received_dump);
> + bt_dev_info(hu->hdev,
> + "QCA memdump Done, received %d, total %d",
> + qca_memdump->received_dump,
> + qca_memdump->ram_dump_size);
> memdump_buf = qca_memdump->memdump_buf_head;
> dev_coredumpv(&hu->serdev->dev, memdump_buf,
> qca_memdump->received_dump, GFP_KERNEL);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
>
next prev parent reply other threads:[~2020-05-28 3:43 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-27 3:37 [PATCH v2] bluetooth: hci_qca: Fix QCA6390 memdump failure Zijun Hu
2020-05-28 3:42 ` Abhishek Pandit-Subedi [this message]
2020-05-28 6:19 ` Zijun Hu
2020-05-28 18:26 ` Abhishek Pandit-Subedi
2020-05-28 21:20 ` Zijun Hu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANFp7mXMiYKY-33xZX2MaHd5RyicbRb2fZHo8mk4-VM_Jf47UQ@mail.gmail.com \
--to=abhishekpandit@chromium.org \
--cc=bgodavar@codeaurora.org \
--cc=c-hbandi@codeaurora.org \
--cc=hemantg@codeaurora.org \
--cc=johan.hedberg@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marcel@holtmann.org \
--cc=mka@chromium.org \
--cc=rjliao@codeaurora.org \
--cc=zijuhu@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).