Hi Calvin,
On Tue, Mar 18, 2025 at 3:31 PM Calvin Owens <calvin@xxxxxxxxxx> wrote:
>
> Hello all,
>
> In the course of other debugging, I've discovered that h4_recv_buf() is
> almost completely duplicated in an inline header definiton, and I don't
> understand why.
>
> I'd like to clean this up: see the patch below for an explanation.
>
> Does anybody who was around at the time remember any more details about
> this? Or failing that, perhaps somebody with access to the bpa10x
> hardware can test it with the core function?
Don't think Ive been involved with kernel internals for this long to
give you an answer, but I assume it was due to alignment differences
in the drivers, but it looks like you could only find bpa10x, which
seems to be a usb driver for a single model (Tektronix BPA 100/105
(Digianswer)) and a quick google seem to refer to a protocol analyzer
for Bluetooth 2.1, so it is not really a regular Bluetooth controller.
> Thanks,
> Calvin
>
> ---8<---
> From: Calvin Owens <calvin@xxxxxxxxxx>
> Subject: [PATCH] bluetooth: Remove duplicated h4_recv_buf() in header
>
> 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"). Unfortunately, there was no
> discussion about it on the list at the time:
>
> https://lore.kernel.org/all/20180320181855.37297-1-marcel@xxxxxxxxxxxx/
> https://lore.kernel.org/all/20180324091954.73229-2-marcel@xxxxxxxxxxxx/
>
> This is the diff between the two implementations as they exist today:
>
> --- /home/calvinow/orig.c 2025-03-10 14:43:18.383882623 -0700
> +++ /home/calvinow/copy.c 2025-03-10 14:42:57.109953576 -0700
> @@ -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;
> }
>
> It seems fairly obvious from the above that, if alignment is one,
> hu->padding is always zero, and in that case the two functions behave
> strictly identically.
>
> Since that is the case for every driver except hci_nokia, clean this up
> and let them use the core function. I've done some light testing on
> btnxpuart, so far everything seems to work.
>
> I would love to eliminate the duplicate function entirely, but I don't
> have access to hardware for bpa10x. Since bpa10x breaking was the
> original justification for the change, I've left it there for now. I'm
> hoping somebody else can shed more light on this.
>
> Cc: Marcel Holtmann <marcel@xxxxxxxxxxxx>
> Signed-off-by: Calvin Owens <calvin@xxxxxxxxxx>
> ---
> drivers/bluetooth/btmtksdio.c | 2 +-
> drivers/bluetooth/btmtkuart.c | 2 +-
> drivers/bluetooth/btnxpuart.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
> index bd5464bde174..47d1073fb4a4 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 c97e260fcb0c..aeb111a0f242 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 aa5ec1d444a9..e6db563088cb 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -21,7 +21,7 @@
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
>
> -#include "h4_recv.h"
> +#include "hci_uart.h"
>
> #define MANUFACTURER_NXP 37
>
> --
> 2.47.2
>
--
Luiz Augusto von Dentz
