* [PATCH] Bluetooth: remove duplicate h4_recv_buf() in header
@ 2025-08-26 4:11 Calvin Owens
2025-08-26 4:38 ` bluez.test.bot
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Calvin Owens @ 2025-08-26 4:11 UTC (permalink / raw)
To: linux-kernel
Cc: Marcel Holtmann, Luiz Augusto von Dentz, Matthias Brugger,
AngeloGioacchino Del Regno, Sean Wang, Amitkumar Karwar,
Neeraj Kale, linux-bluetooth, linux-arm-kernel, linux-mediatek,
johan.hedberg
The "h4_recv.h" header contains a duplicate h4_recv_buf() that is nearly
but not quite identical to the h4_recv_buf() in hci_h4.c.
This duplicated header was added in commit 07eb96a5a7b0 ("Bluetooth:
bpa10x: Use separate h4_recv_buf helper"). I wasn't able to find any
explanation for duplicating the code in the discussion:
https://lore.kernel.org/all/20180320181855.37297-1-marcel@holtmann.org/
https://lore.kernel.org/all/20180324091954.73229-2-marcel@holtmann.org/
Unfortunately, in the years since, several other drivers have come to
also rely on this duplicated function, probably by accident. This is, at
the very least, *extremely* confusing. It's also caused real issues when
it's become out-of-sync, see the following:
ef564119ba83 ("Bluetooth: hci_h4: Add support for ISO packets")
61b27cdf025b ("Bluetooth: hci_h4: Add support for ISO packets in h4_recv.h")
This is the full diff between the two implementations today:
--- orig.c
+++ copy.c
@@ -1,117 +1,100 @@
{
- struct hci_uart *hu = hci_get_drvdata(hdev);
- u8 alignment = hu->alignment ? hu->alignment : 1;
-
/* Check for error from previous call */
if (IS_ERR(skb))
skb = NULL;
while (count) {
int i, len;
- /* remove padding bytes from buffer */
- for (; hu->padding && count > 0; hu->padding--) {
- count--;
- buffer++;
- }
- if (!count)
- break;
-
if (!skb) {
for (i = 0; i < pkts_count; i++) {
if (buffer[0] != (&pkts[i])->type)
continue;
skb = bt_skb_alloc((&pkts[i])->maxlen,
GFP_ATOMIC);
if (!skb)
return ERR_PTR(-ENOMEM);
hci_skb_pkt_type(skb) = (&pkts[i])->type;
hci_skb_expect(skb) = (&pkts[i])->hlen;
break;
}
/* Check for invalid packet type */
if (!skb)
return ERR_PTR(-EILSEQ);
count -= 1;
buffer += 1;
}
len = min_t(uint, hci_skb_expect(skb) - skb->len, count);
skb_put_data(skb, buffer, len);
count -= len;
buffer += len;
/* Check for partial packet */
if (skb->len < hci_skb_expect(skb))
continue;
for (i = 0; i < pkts_count; i++) {
if (hci_skb_pkt_type(skb) == (&pkts[i])->type)
break;
}
if (i >= pkts_count) {
kfree_skb(skb);
return ERR_PTR(-EILSEQ);
}
if (skb->len == (&pkts[i])->hlen) {
u16 dlen;
switch ((&pkts[i])->lsize) {
case 0:
/* No variable data length */
dlen = 0;
break;
case 1:
/* Single octet variable length */
dlen = skb->data[(&pkts[i])->loff];
hci_skb_expect(skb) += dlen;
if (skb_tailroom(skb) < dlen) {
kfree_skb(skb);
return ERR_PTR(-EMSGSIZE);
}
break;
case 2:
/* Double octet variable length */
dlen = get_unaligned_le16(skb->data +
(&pkts[i])->loff);
hci_skb_expect(skb) += dlen;
if (skb_tailroom(skb) < dlen) {
kfree_skb(skb);
return ERR_PTR(-EMSGSIZE);
}
break;
default:
/* Unsupported variable length */
kfree_skb(skb);
return ERR_PTR(-EILSEQ);
}
if (!dlen) {
- hu->padding = (skb->len + 1) % alignment;
- hu->padding = (alignment - hu->padding) % alignment;
-
/* No more data, complete frame */
(&pkts[i])->recv(hdev, skb);
skb = NULL;
}
} else {
- hu->padding = (skb->len + 1) % alignment;
- hu->padding = (alignment - hu->padding) % alignment;
-
/* Complete frame */
(&pkts[i])->recv(hdev, skb);
skb = NULL;
}
}
return skb;
}
-EXPORT_SYMBOL_GPL(h4_recv_buf)
As I read this: If alignment is one, and padding is zero, padding
remains zero throughout the loop. So it seems to me that the two
functions behave strictly identically in that case. All the duplicated
defines are also identical, as is the duplicated h4_recv_pkt structure
declaration.
All four drivers which use the duplicated function use the default
alignment of one, and the default padding of zero. I therefore conclude
the duplicate function may be safely replaced with the core one.
I raised this in an RFC a few months ago, and didn't get much interest:
https://lore.kernel.org/all/CABBYNZ+ONkYtq2fR-8PtL3X-vetvJ0BdP4MTw9cNpjLDzG3HUQ@mail.gmail.com/
...but I'm still wary I've missed something, and I'd really appreciate
more eyeballs on it.
I tested this successfully on btnxpuart a few months ago, but
unfortunately I no longer have access to that hardware.
Cc: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Calvin Owens <calvin@wbinvd.org>
---
drivers/bluetooth/bpa10x.c | 2 +-
drivers/bluetooth/btmtksdio.c | 2 +-
drivers/bluetooth/btmtkuart.c | 2 +-
drivers/bluetooth/btnxpuart.c | 2 +-
drivers/bluetooth/h4_recv.h | 153 ----------------------------------
5 files changed, 4 insertions(+), 157 deletions(-)
delete mode 100644 drivers/bluetooth/h4_recv.h
diff --git a/drivers/bluetooth/bpa10x.c b/drivers/bluetooth/bpa10x.c
index 8b43dfc755de..b7ba667a3d09 100644
--- a/drivers/bluetooth/bpa10x.c
+++ b/drivers/bluetooth/bpa10x.c
@@ -20,7 +20,7 @@
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>
-#include "h4_recv.h"
+#include "hci_uart.h"
#define VERSION "0.11"
diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index 4fc673640bfc..50abefba6d04 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -29,7 +29,7 @@
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>
-#include "h4_recv.h"
+#include "hci_uart.h"
#include "btmtk.h"
#define VERSION "0.1"
diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
index 76995cfcd534..d9b90ea2ad38 100644
--- a/drivers/bluetooth/btmtkuart.c
+++ b/drivers/bluetooth/btmtkuart.c
@@ -27,7 +27,7 @@
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>
-#include "h4_recv.h"
+#include "hci_uart.h"
#include "btmtk.h"
#define VERSION "0.2"
diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 76e7f857fb7d..d5153fed0518 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -24,7 +24,7 @@
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>
-#include "h4_recv.h"
+#include "hci_uart.h"
#define MANUFACTURER_NXP 37
diff --git a/drivers/bluetooth/h4_recv.h b/drivers/bluetooth/h4_recv.h
deleted file mode 100644
index 28cf2d8c2d48..000000000000
--- a/drivers/bluetooth/h4_recv.h
+++ /dev/null
@@ -1,153 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- *
- * Generic Bluetooth HCI UART driver
- *
- * Copyright (C) 2015-2018 Intel Corporation
- */
-
-#include <linux/unaligned.h>
-
-struct h4_recv_pkt {
- u8 type; /* Packet type */
- u8 hlen; /* Header length */
- u8 loff; /* Data length offset in header */
- u8 lsize; /* Data length field size */
- u16 maxlen; /* Max overall packet length */
- int (*recv)(struct hci_dev *hdev, struct sk_buff *skb);
-};
-
-#define H4_RECV_ACL \
- .type = HCI_ACLDATA_PKT, \
- .hlen = HCI_ACL_HDR_SIZE, \
- .loff = 2, \
- .lsize = 2, \
- .maxlen = HCI_MAX_FRAME_SIZE \
-
-#define H4_RECV_SCO \
- .type = HCI_SCODATA_PKT, \
- .hlen = HCI_SCO_HDR_SIZE, \
- .loff = 2, \
- .lsize = 1, \
- .maxlen = HCI_MAX_SCO_SIZE
-
-#define H4_RECV_EVENT \
- .type = HCI_EVENT_PKT, \
- .hlen = HCI_EVENT_HDR_SIZE, \
- .loff = 1, \
- .lsize = 1, \
- .maxlen = HCI_MAX_EVENT_SIZE
-
-#define H4_RECV_ISO \
- .type = HCI_ISODATA_PKT, \
- .hlen = HCI_ISO_HDR_SIZE, \
- .loff = 2, \
- .lsize = 2, \
- .maxlen = HCI_MAX_FRAME_SIZE
-
-static inline struct sk_buff *h4_recv_buf(struct hci_dev *hdev,
- struct sk_buff *skb,
- const unsigned char *buffer,
- int count,
- const struct h4_recv_pkt *pkts,
- int pkts_count)
-{
- /* Check for error from previous call */
- if (IS_ERR(skb))
- skb = NULL;
-
- while (count) {
- int i, len;
-
- if (!skb) {
- for (i = 0; i < pkts_count; i++) {
- if (buffer[0] != (&pkts[i])->type)
- continue;
-
- skb = bt_skb_alloc((&pkts[i])->maxlen,
- GFP_ATOMIC);
- if (!skb)
- return ERR_PTR(-ENOMEM);
-
- hci_skb_pkt_type(skb) = (&pkts[i])->type;
- hci_skb_expect(skb) = (&pkts[i])->hlen;
- break;
- }
-
- /* Check for invalid packet type */
- if (!skb)
- return ERR_PTR(-EILSEQ);
-
- count -= 1;
- buffer += 1;
- }
-
- len = min_t(uint, hci_skb_expect(skb) - skb->len, count);
- skb_put_data(skb, buffer, len);
-
- count -= len;
- buffer += len;
-
- /* Check for partial packet */
- if (skb->len < hci_skb_expect(skb))
- continue;
-
- for (i = 0; i < pkts_count; i++) {
- if (hci_skb_pkt_type(skb) == (&pkts[i])->type)
- break;
- }
-
- if (i >= pkts_count) {
- kfree_skb(skb);
- return ERR_PTR(-EILSEQ);
- }
-
- if (skb->len == (&pkts[i])->hlen) {
- u16 dlen;
-
- switch ((&pkts[i])->lsize) {
- case 0:
- /* No variable data length */
- dlen = 0;
- break;
- case 1:
- /* Single octet variable length */
- dlen = skb->data[(&pkts[i])->loff];
- hci_skb_expect(skb) += dlen;
-
- if (skb_tailroom(skb) < dlen) {
- kfree_skb(skb);
- return ERR_PTR(-EMSGSIZE);
- }
- break;
- case 2:
- /* Double octet variable length */
- dlen = get_unaligned_le16(skb->data +
- (&pkts[i])->loff);
- hci_skb_expect(skb) += dlen;
-
- if (skb_tailroom(skb) < dlen) {
- kfree_skb(skb);
- return ERR_PTR(-EMSGSIZE);
- }
- break;
- default:
- /* Unsupported variable length */
- kfree_skb(skb);
- return ERR_PTR(-EILSEQ);
- }
-
- if (!dlen) {
- /* No more data, complete frame */
- (&pkts[i])->recv(hdev, skb);
- skb = NULL;
- }
- } else {
- /* Complete frame */
- (&pkts[i])->recv(hdev, skb);
- skb = NULL;
- }
- }
-
- return skb;
-}
--
2.49.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: Bluetooth: remove duplicate h4_recv_buf() in header
2025-08-26 4:11 [PATCH] Bluetooth: remove duplicate h4_recv_buf() in header Calvin Owens
@ 2025-08-26 4:38 ` bluez.test.bot
2025-08-26 5:40 ` [PATCH] " Paul Menzel
2025-08-29 16:40 ` patchwork-bot+bluetooth
2 siblings, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2025-08-26 4:38 UTC (permalink / raw)
To: linux-bluetooth, calvin
[-- Attachment #1: Type: text/plain, Size: 2295 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=995480
---Test result---
Test Summary:
CheckPatch PENDING 0.35 seconds
GitLint PENDING 0.36 seconds
SubjectPrefix PASS 0.06 seconds
BuildKernel PASS 24.14 seconds
CheckAllWarning PASS 26.51 seconds
CheckSparse PASS 29.75 seconds
BuildKernel32 PASS 23.75 seconds
TestRunnerSetup PASS 475.33 seconds
TestRunner_l2cap-tester PASS 24.79 seconds
TestRunner_iso-tester PASS 39.19 seconds
TestRunner_bnep-tester PASS 5.95 seconds
TestRunner_mgmt-tester FAIL 123.83 seconds
TestRunner_rfcomm-tester PASS 9.36 seconds
TestRunner_sco-tester PASS 14.64 seconds
TestRunner_ioctl-tester PASS 9.93 seconds
TestRunner_mesh-tester FAIL 11.48 seconds
TestRunner_smp-tester PASS 8.61 seconds
TestRunner_userchan-tester PASS 6.22 seconds
IncrementalBuild PENDING 0.75 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 485 (99.0%), Failed: 1, Not Run: 4
Failed Test Cases
Read Exp Feature - Success Failed 0.100 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0
Failed Test Cases
Mesh - Send cancel - 1 Timed out 2.082 seconds
Mesh - Send cancel - 2 Timed out 1.998 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: remove duplicate h4_recv_buf() in header
2025-08-26 4:11 [PATCH] Bluetooth: remove duplicate h4_recv_buf() in header Calvin Owens
2025-08-26 4:38 ` bluez.test.bot
@ 2025-08-26 5:40 ` Paul Menzel
2025-08-26 5:44 ` Paul Menzel
2025-08-29 16:40 ` patchwork-bot+bluetooth
2 siblings, 1 reply; 5+ messages in thread
From: Paul Menzel @ 2025-08-26 5:40 UTC (permalink / raw)
To: Calvin Owens
Cc: linux-kernel, Marcel Holtmann, Luiz Augusto von Dentz,
Matthias Brugger, AngeloGioacchino Del Regno, Sean Wang,
Amitkumar Karwar, Neeraj Kale, linux-bluetooth, linux-arm-kernel,
linux-mediatek, johan.hedberg
Dear Calvin,
Thank you for your patch.
Am 26.08.25 um 06:11 schrieb Calvin Owens:
> The "h4_recv.h" header contains a duplicate h4_recv_buf() that is nearly
> but not quite identical to the h4_recv_buf() in hci_h4.c.
>
> This duplicated header was added in commit 07eb96a5a7b0 ("Bluetooth:
> bpa10x: Use separate h4_recv_buf helper"). I wasn't able to find any
> explanation for duplicating the code in the discussion:
>
> https://lore.kernel.org/all/20180320181855.37297-1-marcel@holtmann.org/
> https://lore.kernel.org/all/20180324091954.73229-2-marcel@holtmann.org/
>
> Unfortunately, in the years since, several other drivers have come to
> also rely on this duplicated function, probably by accident. This is, at
> the very least, *extremely* confusing. It's also caused real issues when
> it's become out-of-sync, see the following:
>
> ef564119ba83 ("Bluetooth: hci_h4: Add support for ISO packets")
> 61b27cdf025b ("Bluetooth: hci_h4: Add support for ISO packets in h4_recv.h")
>
> This is the full diff between the two implementations today:
>
> --- orig.c
> +++ copy.c
> @@ -1,117 +1,100 @@
> {
> - struct hci_uart *hu = hci_get_drvdata(hdev);
> - u8 alignment = hu->alignment ? hu->alignment : 1;
> -
> /* Check for error from previous call */
> if (IS_ERR(skb))
> skb = NULL;
>
> while (count) {
> int i, len;
>
> - /* remove padding bytes from buffer */
> - for (; hu->padding && count > 0; hu->padding--) {
> - count--;
> - buffer++;
> - }
> - if (!count)
> - break;
> -
> if (!skb) {
> for (i = 0; i < pkts_count; i++) {
> if (buffer[0] != (&pkts[i])->type)
> continue;
>
> skb = bt_skb_alloc((&pkts[i])->maxlen,
> GFP_ATOMIC);
> if (!skb)
> return ERR_PTR(-ENOMEM);
>
> hci_skb_pkt_type(skb) = (&pkts[i])->type;
> hci_skb_expect(skb) = (&pkts[i])->hlen;
> break;
> }
>
> /* Check for invalid packet type */
> if (!skb)
> return ERR_PTR(-EILSEQ);
>
> count -= 1;
> buffer += 1;
> }
>
> len = min_t(uint, hci_skb_expect(skb) - skb->len, count);
> skb_put_data(skb, buffer, len);
>
> count -= len;
> buffer += len;
>
> /* Check for partial packet */
> if (skb->len < hci_skb_expect(skb))
> continue;
>
> for (i = 0; i < pkts_count; i++) {
> if (hci_skb_pkt_type(skb) == (&pkts[i])->type)
> break;
> }
>
> if (i >= pkts_count) {
> kfree_skb(skb);
> return ERR_PTR(-EILSEQ);
> }
>
> if (skb->len == (&pkts[i])->hlen) {
> u16 dlen;
>
> switch ((&pkts[i])->lsize) {
> case 0:
> /* No variable data length */
> dlen = 0;
> break;
> case 1:
> /* Single octet variable length */
> dlen = skb->data[(&pkts[i])->loff];
> hci_skb_expect(skb) += dlen;
>
> if (skb_tailroom(skb) < dlen) {
> kfree_skb(skb);
> return ERR_PTR(-EMSGSIZE);
> }
> break;
> case 2:
> /* Double octet variable length */
> dlen = get_unaligned_le16(skb->data +
> (&pkts[i])->loff);
> hci_skb_expect(skb) += dlen;
>
> if (skb_tailroom(skb) < dlen) {
> kfree_skb(skb);
> return ERR_PTR(-EMSGSIZE);
> }
> break;
> default:
> /* Unsupported variable length */
> kfree_skb(skb);
> return ERR_PTR(-EILSEQ);
> }
>
> if (!dlen) {
> - hu->padding = (skb->len + 1) % alignment;
> - hu->padding = (alignment - hu->padding) % alignment;
> -
> /* No more data, complete frame */
> (&pkts[i])->recv(hdev, skb);
> skb = NULL;
> }
> } else {
> - hu->padding = (skb->len + 1) % alignment;
> - hu->padding = (alignment - hu->padding) % alignment;
> -
> /* Complete frame */
> (&pkts[i])->recv(hdev, skb);
> skb = NULL;
> }
> }
>
> return skb;
> }
> -EXPORT_SYMBOL_GPL(h4_recv_buf)
>
> As I read this: If alignment is one, and padding is zero, padding
> remains zero throughout the loop. So it seems to me that the two
> functions behave strictly identically in that case. All the duplicated
> defines are also identical, as is the duplicated h4_recv_pkt structure
> declaration.
>
> All four drivers which use the duplicated function use the default
> alignment of one, and the default padding of zero. I therefore conclude
> the duplicate function may be safely replaced with the core one.
>
> I raised this in an RFC a few months ago, and didn't get much interest:
>
> https://lore.kernel.org/all/CABBYNZ+ONkYtq2fR-8PtL3X-vetvJ0BdP4MTw9cNpjLDzG3HUQ@mail.gmail.com/
>
> ...but I'm still wary I've missed something, and I'd really appreciate
> more eyeballs on it.
>
> I tested this successfully on btnxpuart a few months ago, but
> unfortunately I no longer have access to that hardware.
Great analysis. Thank you for your time writing this up.
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Signed-off-by: Calvin Owens <calvin@wbinvd.org>
> ---
> drivers/bluetooth/bpa10x.c | 2 +-
> drivers/bluetooth/btmtksdio.c | 2 +-
> drivers/bluetooth/btmtkuart.c | 2 +-
> drivers/bluetooth/btnxpuart.c | 2 +-
> drivers/bluetooth/h4_recv.h | 153 ----------------------------------
> 5 files changed, 4 insertions(+), 157 deletions(-)
> delete mode 100644 drivers/bluetooth/h4_recv.h
>
> diff --git a/drivers/bluetooth/bpa10x.c b/drivers/bluetooth/bpa10x.c
> index 8b43dfc755de..b7ba667a3d09 100644
> --- a/drivers/bluetooth/bpa10x.c
> +++ b/drivers/bluetooth/bpa10x.c
> @@ -20,7 +20,7 @@
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
>
> -#include "h4_recv.h"
> +#include "hci_uart.h"
>
> #define VERSION "0.11"
>
> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
> index 4fc673640bfc..50abefba6d04 100644
> --- a/drivers/bluetooth/btmtksdio.c
> +++ b/drivers/bluetooth/btmtksdio.c
> @@ -29,7 +29,7 @@
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
>
> -#include "h4_recv.h"
> +#include "hci_uart.h"
> #include "btmtk.h"
>
> #define VERSION "0.1"
> diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
> index 76995cfcd534..d9b90ea2ad38 100644
> --- a/drivers/bluetooth/btmtkuart.c
> +++ b/drivers/bluetooth/btmtkuart.c
> @@ -27,7 +27,7 @@
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
>
> -#include "h4_recv.h"
> +#include "hci_uart.h"
> #include "btmtk.h"
>
> #define VERSION "0.2"
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index 76e7f857fb7d..d5153fed0518 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -24,7 +24,7 @@
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
>
> -#include "h4_recv.h"
> +#include "hci_uart.h"
>
> #define MANUFACTURER_NXP 37
>
> diff --git a/drivers/bluetooth/h4_recv.h b/drivers/bluetooth/h4_recv.h
> deleted file mode 100644
> index 28cf2d8c2d48..000000000000
> --- a/drivers/bluetooth/h4_recv.h
> +++ /dev/null
> @@ -1,153 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - *
> - * Generic Bluetooth HCI UART driver
> - *
> - * Copyright (C) 2015-2018 Intel Corporation
> - */
> -
> -#include <linux/unaligned.h>
> -
> -struct h4_recv_pkt {
> - u8 type; /* Packet type */
> - u8 hlen; /* Header length */
> - u8 loff; /* Data length offset in header */
> - u8 lsize; /* Data length field size */
> - u16 maxlen; /* Max overall packet length */
> - int (*recv)(struct hci_dev *hdev, struct sk_buff *skb);
> -};
> -
> -#define H4_RECV_ACL \
> - .type = HCI_ACLDATA_PKT, \
> - .hlen = HCI_ACL_HDR_SIZE, \
> - .loff = 2, \
> - .lsize = 2, \
> - .maxlen = HCI_MAX_FRAME_SIZE \
> -
> -#define H4_RECV_SCO \
> - .type = HCI_SCODATA_PKT, \
> - .hlen = HCI_SCO_HDR_SIZE, \
> - .loff = 2, \
> - .lsize = 1, \
> - .maxlen = HCI_MAX_SCO_SIZE
> -
> -#define H4_RECV_EVENT \
> - .type = HCI_EVENT_PKT, \
> - .hlen = HCI_EVENT_HDR_SIZE, \
> - .loff = 1, \
> - .lsize = 1, \
> - .maxlen = HCI_MAX_EVENT_SIZE
> -
> -#define H4_RECV_ISO \
> - .type = HCI_ISODATA_PKT, \
> - .hlen = HCI_ISO_HDR_SIZE, \
> - .loff = 2, \
> - .lsize = 2, \
> - .maxlen = HCI_MAX_FRAME_SIZE
> -
> -static inline struct sk_buff *h4_recv_buf(struct hci_dev *hdev,
> - struct sk_buff *skb,
> - const unsigned char *buffer,
> - int count,
> - const struct h4_recv_pkt *pkts,
> - int pkts_count)
> -{
> - /* Check for error from previous call */
> - if (IS_ERR(skb))
> - skb = NULL;
> -
> - while (count) {
> - int i, len;
> -
> - if (!skb) {
> - for (i = 0; i < pkts_count; i++) {
> - if (buffer[0] != (&pkts[i])->type)
> - continue;
> -
> - skb = bt_skb_alloc((&pkts[i])->maxlen,
> - GFP_ATOMIC);
> - if (!skb)
> - return ERR_PTR(-ENOMEM);
> -
> - hci_skb_pkt_type(skb) = (&pkts[i])->type;
> - hci_skb_expect(skb) = (&pkts[i])->hlen;
> - break;
> - }
> -
> - /* Check for invalid packet type */
> - if (!skb)
> - return ERR_PTR(-EILSEQ);
> -
> - count -= 1;
> - buffer += 1;
> - }
> -
> - len = min_t(uint, hci_skb_expect(skb) - skb->len, count);
> - skb_put_data(skb, buffer, len);
> -
> - count -= len;
> - buffer += len;
> -
> - /* Check for partial packet */
> - if (skb->len < hci_skb_expect(skb))
> - continue;
> -
> - for (i = 0; i < pkts_count; i++) {
> - if (hci_skb_pkt_type(skb) == (&pkts[i])->type)
> - break;
> - }
> -
> - if (i >= pkts_count) {
> - kfree_skb(skb);
> - return ERR_PTR(-EILSEQ);
> - }
> -
> - if (skb->len == (&pkts[i])->hlen) {
> - u16 dlen;
> -
> - switch ((&pkts[i])->lsize) {
> - case 0:
> - /* No variable data length */
> - dlen = 0;
> - break;
> - case 1:
> - /* Single octet variable length */
> - dlen = skb->data[(&pkts[i])->loff];
> - hci_skb_expect(skb) += dlen;
> -
> - if (skb_tailroom(skb) < dlen) {
> - kfree_skb(skb);
> - return ERR_PTR(-EMSGSIZE);
> - }
> - break;
> - case 2:
> - /* Double octet variable length */
> - dlen = get_unaligned_le16(skb->data +
> - (&pkts[i])->loff);
> - hci_skb_expect(skb) += dlen;
> -
> - if (skb_tailroom(skb) < dlen) {
> - kfree_skb(skb);
> - return ERR_PTR(-EMSGSIZE);
> - }
> - break;
> - default:
> - /* Unsupported variable length */
> - kfree_skb(skb);
> - return ERR_PTR(-EILSEQ);
> - }
> -
> - if (!dlen) {
> - /* No more data, complete frame */
> - (&pkts[i])->recv(hdev, skb);
> - skb = NULL;
> - }
> - } else {
> - /* Complete frame */
> - (&pkts[i])->recv(hdev, skb);
> - skb = NULL;
> - }
> - }
> -
> - return skb;
> -}
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Kind regards,
Paul
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: remove duplicate h4_recv_buf() in header
2025-08-26 5:40 ` [PATCH] " Paul Menzel
@ 2025-08-26 5:44 ` Paul Menzel
0 siblings, 0 replies; 5+ messages in thread
From: Paul Menzel @ 2025-08-26 5:44 UTC (permalink / raw)
To: Calvin Owens
Cc: linux-kernel, Marcel Holtmann, Luiz Augusto von Dentz,
Matthias Brugger, AngeloGioacchino Del Regno, Sean Wang,
Amitkumar Karwar, Neeraj Kale, linux-bluetooth, linux-arm-kernel,
linux-mediatek
[Cc: Remove Johan’s bouncing address]
Am 26.08.25 um 07:40 schrieb Paul Menzel:
> Dear Calvin,
>
>
> Thank you for your patch.
>
> Am 26.08.25 um 06:11 schrieb Calvin Owens:
>> The "h4_recv.h" header contains a duplicate h4_recv_buf() that is nearly
>> but not quite identical to the h4_recv_buf() in hci_h4.c.
>>
>> This duplicated header was added in commit 07eb96a5a7b0 ("Bluetooth:
>> bpa10x: Use separate h4_recv_buf helper"). I wasn't able to find any
>> explanation for duplicating the code in the discussion:
>>
>> https://lore.kernel.org/all/20180320181855.37297-1-marcel@holtmann.org/
>> https://lore.kernel.org/all/20180324091954.73229-2-marcel@holtmann.org/
>>
>> Unfortunately, in the years since, several other drivers have come to
>> also rely on this duplicated function, probably by accident. This is, at
>> the very least, *extremely* confusing. It's also caused real issues when
>> it's become out-of-sync, see the following:
>>
>> ef564119ba83 ("Bluetooth: hci_h4: Add support for ISO packets")
>> 61b27cdf025b ("Bluetooth: hci_h4: Add support for ISO packets in h4_recv.h")
>>
>> This is the full diff between the two implementations today:
>>
>> --- orig.c
>> +++ copy.c
>> @@ -1,117 +1,100 @@
>> {
>> - struct hci_uart *hu = hci_get_drvdata(hdev);
>> - u8 alignment = hu->alignment ? hu->alignment : 1;
>> -
>> /* Check for error from previous call */
>> if (IS_ERR(skb))
>> skb = NULL;
>>
>> while (count) {
>> int i, len;
>>
>> - /* remove padding bytes from buffer */
>> - for (; hu->padding && count > 0; hu->padding--) {
>> - count--;
>> - buffer++;
>> - }
>> - if (!count)
>> - break;
>> -
>> if (!skb) {
>> for (i = 0; i < pkts_count; i++) {
>> if (buffer[0] != (&pkts[i])->type)
>> continue;
>>
>> skb = bt_skb_alloc((&pkts[i])->maxlen,
>> GFP_ATOMIC);
>> if (!skb)
>> return ERR_PTR(-ENOMEM);
>>
>> hci_skb_pkt_type(skb) = (&pkts[i])->type;
>> hci_skb_expect(skb) = (&pkts[i])->hlen;
>> break;
>> }
>>
>> /* Check for invalid packet type */
>> if (!skb)
>> return ERR_PTR(-EILSEQ);
>>
>> count -= 1;
>> buffer += 1;
>> }
>>
>> len = min_t(uint, hci_skb_expect(skb) - skb->len, count);
>> skb_put_data(skb, buffer, len);
>>
>> count -= len;
>> buffer += len;
>>
>> /* Check for partial packet */
>> if (skb->len < hci_skb_expect(skb))
>> continue;
>>
>> for (i = 0; i < pkts_count; i++) {
>> if (hci_skb_pkt_type(skb) == (&pkts[i])->type)
>> break;
>> }
>>
>> if (i >= pkts_count) {
>> kfree_skb(skb);
>> return ERR_PTR(-EILSEQ);
>> }
>>
>> if (skb->len == (&pkts[i])->hlen) {
>> u16 dlen;
>>
>> switch ((&pkts[i])->lsize) {
>> case 0:
>> /* No variable data length */
>> dlen = 0;
>> break;
>> case 1:
>> /* Single octet variable length */
>> dlen = skb->data[(&pkts[i])->loff];
>> hci_skb_expect(skb) += dlen;
>>
>> if (skb_tailroom(skb) < dlen) {
>> kfree_skb(skb);
>> return ERR_PTR(-EMSGSIZE);
>> }
>> break;
>> case 2:
>> /* Double octet variable length */
>> dlen = get_unaligned_le16(skb->data +
>> (&pkts[i])->loff);
>> hci_skb_expect(skb) += dlen;
>>
>> if (skb_tailroom(skb) < dlen) {
>> kfree_skb(skb);
>> return ERR_PTR(-EMSGSIZE);
>> }
>> break;
>> default:
>> /* Unsupported variable length */
>> kfree_skb(skb);
>> return ERR_PTR(-EILSEQ);
>> }
>>
>> if (!dlen) {
>> - hu->padding = (skb->len + 1) % alignment;
>> - hu->padding = (alignment - hu->padding) % alignment;
>> -
>> /* No more data, complete frame */
>> (&pkts[i])->recv(hdev, skb);
>> skb = NULL;
>> }
>> } else {
>> - hu->padding = (skb->len + 1) % alignment;
>> - hu->padding = (alignment - hu->padding) % alignment;
>> -
>> /* Complete frame */
>> (&pkts[i])->recv(hdev, skb);
>> skb = NULL;
>> }
>> }
>>
>> return skb;
>> }
>> -EXPORT_SYMBOL_GPL(h4_recv_buf)
>>
>> As I read this: If alignment is one, and padding is zero, padding
>> remains zero throughout the loop. So it seems to me that the two
>> functions behave strictly identically in that case. All the duplicated
>> defines are also identical, as is the duplicated h4_recv_pkt structure
>> declaration.
>>
>> All four drivers which use the duplicated function use the default
>> alignment of one, and the default padding of zero. I therefore conclude
>> the duplicate function may be safely replaced with the core one.
>>
>> I raised this in an RFC a few months ago, and didn't get much interest:
>>
>> https://lore.kernel.org/all/CABBYNZ+ONkYtq2fR-8PtL3X-vetvJ0BdP4MTw9cNpjLDzG3HUQ@mail.gmail.com/
>>
>> ...but I'm still wary I've missed something, and I'd really appreciate
>> more eyeballs on it.
>>
>> I tested this successfully on btnxpuart a few months ago, but
>> unfortunately I no longer have access to that hardware.
>
> Great analysis. Thank you for your time writing this up.
>
>> Cc: Marcel Holtmann <marcel@holtmann.org>
>> Signed-off-by: Calvin Owens <calvin@wbinvd.org>
>> ---
>> drivers/bluetooth/bpa10x.c | 2 +-
>> drivers/bluetooth/btmtksdio.c | 2 +-
>> drivers/bluetooth/btmtkuart.c | 2 +-
>> drivers/bluetooth/btnxpuart.c | 2 +-
>> drivers/bluetooth/h4_recv.h | 153 ----------------------------------
>> 5 files changed, 4 insertions(+), 157 deletions(-)
>> delete mode 100644 drivers/bluetooth/h4_recv.h
>>
>> diff --git a/drivers/bluetooth/bpa10x.c b/drivers/bluetooth/bpa10x.c
>> index 8b43dfc755de..b7ba667a3d09 100644
>> --- a/drivers/bluetooth/bpa10x.c
>> +++ b/drivers/bluetooth/bpa10x.c
>> @@ -20,7 +20,7 @@
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> -#include "h4_recv.h"
>> +#include "hci_uart.h"
>> #define VERSION "0.11"
>> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
>> index 4fc673640bfc..50abefba6d04 100644
>> --- a/drivers/bluetooth/btmtksdio.c
>> +++ b/drivers/bluetooth/btmtksdio.c
>> @@ -29,7 +29,7 @@
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> -#include "h4_recv.h"
>> +#include "hci_uart.h"
>> #include "btmtk.h"
>> #define VERSION "0.1"
>> diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
>> index 76995cfcd534..d9b90ea2ad38 100644
>> --- a/drivers/bluetooth/btmtkuart.c
>> +++ b/drivers/bluetooth/btmtkuart.c
>> @@ -27,7 +27,7 @@
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> -#include "h4_recv.h"
>> +#include "hci_uart.h"
>> #include "btmtk.h"
>> #define VERSION "0.2"
>> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
>> index 76e7f857fb7d..d5153fed0518 100644
>> --- a/drivers/bluetooth/btnxpuart.c
>> +++ b/drivers/bluetooth/btnxpuart.c
>> @@ -24,7 +24,7 @@
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> -#include "h4_recv.h"
>> +#include "hci_uart.h"
>> #define MANUFACTURER_NXP 37
>> diff --git a/drivers/bluetooth/h4_recv.h b/drivers/bluetooth/h4_recv.h
>> deleted file mode 100644
>> index 28cf2d8c2d48..000000000000
>> --- a/drivers/bluetooth/h4_recv.h
>> +++ /dev/null
>> @@ -1,153 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0-or-later */
>> -/*
>> - *
>> - * Generic Bluetooth HCI UART driver
>> - *
>> - * Copyright (C) 2015-2018 Intel Corporation
>> - */
>> -
>> -#include <linux/unaligned.h>
>> -
>> -struct h4_recv_pkt {
>> - u8 type; /* Packet type */
>> - u8 hlen; /* Header length */
>> - u8 loff; /* Data length offset in header */
>> - u8 lsize; /* Data length field size */
>> - u16 maxlen; /* Max overall packet length */
>> - int (*recv)(struct hci_dev *hdev, struct sk_buff *skb);
>> -};
>> -
>> -#define H4_RECV_ACL \
>> - .type = HCI_ACLDATA_PKT, \
>> - .hlen = HCI_ACL_HDR_SIZE, \
>> - .loff = 2, \
>> - .lsize = 2, \
>> - .maxlen = HCI_MAX_FRAME_SIZE \
>> -
>> -#define H4_RECV_SCO \
>> - .type = HCI_SCODATA_PKT, \
>> - .hlen = HCI_SCO_HDR_SIZE, \
>> - .loff = 2, \
>> - .lsize = 1, \
>> - .maxlen = HCI_MAX_SCO_SIZE
>> -
>> -#define H4_RECV_EVENT \
>> - .type = HCI_EVENT_PKT, \
>> - .hlen = HCI_EVENT_HDR_SIZE, \
>> - .loff = 1, \
>> - .lsize = 1, \
>> - .maxlen = HCI_MAX_EVENT_SIZE
>> -
>> -#define H4_RECV_ISO \
>> - .type = HCI_ISODATA_PKT, \
>> - .hlen = HCI_ISO_HDR_SIZE, \
>> - .loff = 2, \
>> - .lsize = 2, \
>> - .maxlen = HCI_MAX_FRAME_SIZE
>> -
>> -static inline struct sk_buff *h4_recv_buf(struct hci_dev *hdev,
>> - struct sk_buff *skb,
>> - const unsigned char *buffer,
>> - int count,
>> - const struct h4_recv_pkt *pkts,
>> - int pkts_count)
>> -{
>> - /* Check for error from previous call */
>> - if (IS_ERR(skb))
>> - skb = NULL;
>> -
>> - while (count) {
>> - int i, len;
>> -
>> - if (!skb) {
>> - for (i = 0; i < pkts_count; i++) {
>> - if (buffer[0] != (&pkts[i])->type)
>> - continue;
>> -
>> - skb = bt_skb_alloc((&pkts[i])->maxlen,
>> - GFP_ATOMIC);
>> - if (!skb)
>> - return ERR_PTR(-ENOMEM);
>> -
>> - hci_skb_pkt_type(skb) = (&pkts[i])->type;
>> - hci_skb_expect(skb) = (&pkts[i])->hlen;
>> - break;
>> - }
>> -
>> - /* Check for invalid packet type */
>> - if (!skb)
>> - return ERR_PTR(-EILSEQ);
>> -
>> - count -= 1;
>> - buffer += 1;
>> - }
>> -
>> - len = min_t(uint, hci_skb_expect(skb) - skb->len, count);
>> - skb_put_data(skb, buffer, len);
>> -
>> - count -= len;
>> - buffer += len;
>> -
>> - /* Check for partial packet */
>> - if (skb->len < hci_skb_expect(skb))
>> - continue;
>> -
>> - for (i = 0; i < pkts_count; i++) {
>> - if (hci_skb_pkt_type(skb) == (&pkts[i])->type)
>> - break;
>> - }
>> -
>> - if (i >= pkts_count) {
>> - kfree_skb(skb);
>> - return ERR_PTR(-EILSEQ);
>> - }
>> -
>> - if (skb->len == (&pkts[i])->hlen) {
>> - u16 dlen;
>> -
>> - switch ((&pkts[i])->lsize) {
>> - case 0:
>> - /* No variable data length */
>> - dlen = 0;
>> - break;
>> - case 1:
>> - /* Single octet variable length */
>> - dlen = skb->data[(&pkts[i])->loff];
>> - hci_skb_expect(skb) += dlen;
>> -
>> - if (skb_tailroom(skb) < dlen) {
>> - kfree_skb(skb);
>> - return ERR_PTR(-EMSGSIZE);
>> - }
>> - break;
>> - case 2:
>> - /* Double octet variable length */
>> - dlen = get_unaligned_le16(skb->data +
>> - (&pkts[i])->loff);
>> - hci_skb_expect(skb) += dlen;
>> -
>> - if (skb_tailroom(skb) < dlen) {
>> - kfree_skb(skb);
>> - return ERR_PTR(-EMSGSIZE);
>> - }
>> - break;
>> - default:
>> - /* Unsupported variable length */
>> - kfree_skb(skb);
>> - return ERR_PTR(-EILSEQ);
>> - }
>> -
>> - if (!dlen) {
>> - /* No more data, complete frame */
>> - (&pkts[i])->recv(hdev, skb);
>> - skb = NULL;
>> - }
>> - } else {
>> - /* Complete frame */
>> - (&pkts[i])->recv(hdev, skb);
>> - skb = NULL;
>> - }
>> - }
>> -
>> - return skb;
>> -}
>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
>
>
> Kind regards,
>
> Paul
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: remove duplicate h4_recv_buf() in header
2025-08-26 4:11 [PATCH] Bluetooth: remove duplicate h4_recv_buf() in header Calvin Owens
2025-08-26 4:38 ` bluez.test.bot
2025-08-26 5:40 ` [PATCH] " Paul Menzel
@ 2025-08-29 16:40 ` patchwork-bot+bluetooth
2 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+bluetooth @ 2025-08-29 16:40 UTC (permalink / raw)
To: Calvin Owens
Cc: linux-kernel, marcel, luiz.dentz, matthias.bgg,
angelogioacchino.delregno, sean.wang, amitkumar.karwar,
neeraj.sanjaykale, linux-bluetooth, linux-arm-kernel,
linux-mediatek, johan.hedberg
Hello:
This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
On Mon, 25 Aug 2025 21:11:08 -0700 you wrote:
> The "h4_recv.h" header contains a duplicate h4_recv_buf() that is nearly
> but not quite identical to the h4_recv_buf() in hci_h4.c.
>
> This duplicated header was added in commit 07eb96a5a7b0 ("Bluetooth:
> bpa10x: Use separate h4_recv_buf helper"). I wasn't able to find any
> explanation for duplicating the code in the discussion:
>
> [...]
Here is the summary with links:
- Bluetooth: remove duplicate h4_recv_buf() in header
https://git.kernel.org/bluetooth/bluetooth-next/c/0e272fc7e17d
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-08-29 16:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-26 4:11 [PATCH] Bluetooth: remove duplicate h4_recv_buf() in header Calvin Owens
2025-08-26 4:38 ` bluez.test.bot
2025-08-26 5:40 ` [PATCH] " Paul Menzel
2025-08-26 5:44 ` Paul Menzel
2025-08-29 16:40 ` patchwork-bot+bluetooth
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).