Re: [PATCH v2 0/3] Bluetooth: btmtk: fix ISO data reception

["Chris Lu (陸稚泓)" <Chris.Lu@mediatek.com>]

Hi Luiz,

Thanks for reminder and review, I've seen Sashiko's feedback and just
about to discuss with you.

On Thu, 2026-10-15 at 09:47 -0400, Luiz Augusto von Dentz wrote:
> Hi Chris,
> 
> On Thu, Oct 15, 2026 at 4:15 AM Chris Lu <chris.lu@mediatek.com>
> wrote:
> > 
> > Three fixes for ISO data handling in btmtk, found while debugging
> > LE Audio
> > on MT7925. The code is shared by every MediaTek USB controller that
> > supports ISO data, so none of this is specific to that part.
> > 
> > Patch 1 drops the zero padding that the ISO interrupt IN endpoint
> > appends
> > to each transfer instead of parsing it as ISO packets. The driver
> > turns
> > that padding into handle 0 packets with a Data_Total_Length of zero
> > and
> > hands them to the stack, which is what fills the HCI log with
> > malformed
> > ISO frames, and a valid SDU is lost whenever handle 0 belongs to a
> > live
> > CIS/BIS.
> > 
> > Patch 2 bounds Data_Total_Length by hdev->iso_mtu. The existing
> > skb_tailroom() test enforces no protocol limit, because the skb
> > head
> > allocation is rounded up to a kmalloc bucket and skb_tailroom()
> > reports
> > the bucket size rather than what was requested.
> > 
> > Patch 3 updates the mt79xx ISO data transmission setting.
> > 
> > The three hunks do not overlap, so any prefix of the series applies
> > on its
> > own.
> > 
> > Changes in v2:
> > - Patch 2 reworked and retitled. v1 only enlarged the receive skb,
> > which
> >   turned out to be a no-op, and added no bound at all. v2 bounds
> >   Data_Total_Length by hdev->iso_mtu and sizes the skb from the
> > same
> >   value.
> > - Patch 3 added.
> > - Patch 1 unchanged.
> > 
> > Based on bluetooth-next.
> > 
> > Chris Lu (3):
> >   Bluetooth: btmtk: drop zero-length ISO packets from endpoint
> > padding
> >   Bluetooth: btmtk: bound the ISO payload length by the
> > controller's MTU
> >   Bluetooth: btmtk: derive the firmware ISO setting from
> >     MTK_ISO_THRESHOLD
> > 
> >  drivers/bluetooth/btmtk.c | 37 ++++++++++++++++++++++++++++++++---
> > --
> >  1 file changed, 32 insertions(+), 5 deletions(-)
> > 
> > --
> > 2.45.2
> > 
> 
> In case you haven't seen it sashiko flagged a problem with this set:
> 
> https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20261015081450.1241194-1-chris.lu*40mediatek.com__;IyU!!CTRNKA9wMg0ARbw!mYdsqeN7_YBH4bm3y5AVx7pNg8Vf3PCejWejyil945dveZUs2iYohrBujiU-AlOerUeQuTMdUpu6UKOXKRY$
>  
> 
> Unsuccessful packets shall still be delivered to the host, this is
> used, for example, to implement Packet Loss Concealment (PLC) logic.
> 

Agreed, and that is why the condition here is Data_Total_Length == 0
rather than ISO_SDU_Length == 0:

-unsuccessful SDU : Data_Total_Length = 4 (8 with TS), ISO_SDU_Length =
0
-endpoint padding : Data_Total_Length = 0, no ISO_Data_Load at all

An unsuccessful packet carries its Packet_Status_Flag inside the
ISO_Data_Load, alongside the Packet_Sequence_Number and ISO_SDU_Length,
so it still has a Data_Total_Length of 4 even when the SDU itself is
empty. That is what reaches the socket today: iso_recv() pulls
HCI_ISO_DATA_HDR_SIZE, then derives ISO_SDU_Length via
hci_iso_data_len() and the Packet_Status_Flag via hci_iso_data_flags()
from that same slen field, sees len == skb->len == 0, and delivers a
zero-length skb with hci_skb_pkt_status() set, which af_bluetooth.c
passes up as BT_SCM_PKT_STATUS. This patch does not touch any
of that, so PLC keeps getting its indications.

What this patch drops is the second line: a packet with no
ISO_Data_Load at all. It cannot carry a Packet_Status_Flag, since the
four octets holding it are simply not there, so it cannot express an
unsuccessful packet in the first place.

The reason this is handled in the driver is how MediaTek controllers
transport ISO data: the ISO interrupt IN endpoint delivers a fixed-size
transfer per completion and zero-fills whatever is left after the ISO
packet it carries. That padding belongs to the transfer rather than to
the stream, and since it is always trailing, parsing can stop at the
first zero-length header instead of skipping it and looking for more
packets behind it.

On the AI review's other suggestion, gating this on a non-zero
connection handle: handle 0 is a valid connection handle, so that would
treat a stream on handle 0 differently from the same stream on any
other handle, which is the behaviour this patch removes.

I can make the comment say explicitly that unsuccessful packets are
preserved, if that helps the next reader.

My preference is to keep the series as proposed, since
Data_Total_Length == 0 already identifies the padding without
ambiguity. If you'd still rather not rely on that check alone, there is
another way to confirm this is padding rather than infer it from the
header, and I'm glad to send a v3 with that instead if you'd prefer.

Thanks,
Chris Lu