Re: [PATCH][QUESTION] bluetooth: Remove duplicated h4_recv_buf() in header

[Luiz Augusto von Dentz]

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