Re: [PATCH] Bluetooth: btnxpuart: Keep FW dump header in coredump chunks

[Luiz Augusto von Dentz]

Hi Ali,

On Tue, Aug 18, 2026 at 3:30 PM Ali Ahmet Memis <ali@xxxxxxxxxxxxxx> wrote:
>
> Hi Luiz,
>
> On Tue, Aug 18, 2026 at 12:59 PM Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
> >
> > Yeah, and it concatenates the header states into a dump skb,
> > interleaving data with headers. The worst part is that if the analyzer
> > finds an issue with the header it can consider the entire trace
> > malformed. This means both the kernel and userspace try to parse the
> > same thing but may interpret what each chunk means differently.
>
> The kernel uses these two fields only for framing. It does not interpret
> them as part of the dump format or validate their contents. It also does
> not compare buf_len with the actual payload length.
>
> This cannot be moved to userspace because the driver has no other way to
> know where a dump starts and ends. Without seq_num and buf_len, it
> cannot know when to call hci_devcd_init() and hci_devcd_complete().
>
> I agree there is some overlap here, and I think this is also why a short
> chunk should not be dropped silently.
>
> > Well, it does change, it doesn't append anything if the chunk is less
> > than a header and that won't even show up for the analyzer to analyze.
>
> You're right. That statement was too broad. A chunk shorter than the
> header is dropped, so the analyzer never sees it.
>
> > Now, regarding this, I wonder if the driver receives a chunk smaller
> > than the header size, whether we should either append it and leave
> > interpretation to userspace, or 2 drop the entire dump because we
> > can't guarantee its consistency if we suspect data corruption when the
> > firmware is not supposed to generate chunks smaller than the header
> > size.
>
> I would go with the second option.
>
> Appending it does not really leave the interpretation to userspace. Each
> header tells the analyzer how many bytes belong to that chunk. If we add
> a fragment without a header, all following headers are shifted and the
> rest of the dump becomes difficult to parse.
>
> Aborting seems like a better fit. The firmware should not generate a
> chunk smaller than the header, so seeing one means something has already
> gone wrong. We can no longer rely on the rest of the dump being
> consistent.
>
> hci_devcd_abort() keeps the data collected so far and marks the dump
> with HCI_DEVCOREDUMP_ABORT in the State line. Userspace can then see
> that the dump is truncated. hci_qca and hci_vhci already use it on
> their error paths.
>
> > Yeah, the decision of considering a chunk valid or not seems to be
> > split between kernel and userspace. The kernel evaluates if the chunk
> > is too short but won't check any data consistency after that;
> > userspace will evaluate the headers but won't get a chance to evaluate
> > if any small chunk was dropped in the process.
>
> I think aborting closes that gap. The kernel decides that a chunk too
> short to contain a header is invalid, and userspace can see that the
> dump was aborted instead of silently missing bytes.
>
> Would you prefer this as a separate patch on top? This patch only
> restores the header in the appended chunk, so its Fixes tag can stay
> focused on that regression. Handling short chunks with an abort would
> be a separate behavior change.

Lets spin a new version aborting if the chunk is too short, I will
probably have it fixed up in place with the origina change since that
wasn't pulled yet so it makes it easier to not have multiple Fixes tag
applying one on top of another.

> Thanks,
> Ali



-- 
Luiz Augusto von Dentz