diff mbox series

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

Message ID 2b612e2c1f4fa697b47b5dd9b72f1949d7c206f5.1742324401.git.calvin@wbinvd.org (mailing list archive)
State New
Headers show
Series [QUESTION] bluetooth: Remove duplicated h4_recv_buf() in header | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/SubjectPrefix fail "Bluetooth: " prefix is not specified in the subject
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS
tedd_an/BuildKernel32 success BuildKernel32 PASS
tedd_an/TestRunnerSetup success TestRunnerSetup PASS
tedd_an/TestRunner_l2cap-tester success TestRunner PASS
tedd_an/TestRunner_iso-tester success TestRunner PASS
tedd_an/TestRunner_bnep-tester success TestRunner PASS
tedd_an/TestRunner_mgmt-tester fail TestRunner_mgmt-tester: Total: 490, Passed: 484 (98.8%), Failed: 2, Not Run: 4
tedd_an/TestRunner_rfcomm-tester success TestRunner PASS
tedd_an/TestRunner_sco-tester success TestRunner PASS
tedd_an/TestRunner_ioctl-tester success TestRunner PASS
tedd_an/TestRunner_mesh-tester success TestRunner PASS
tedd_an/TestRunner_smp-tester success TestRunner PASS
tedd_an/TestRunner_userchan-tester success TestRunner PASS

Commit Message

Calvin Owens March 18, 2025, 7:31 p.m. UTC 
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?

Thanks,
Calvin

---8<---
From: Calvin Owens <calvin@wbinvd.org>
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@holtmann.org/
    https://lore.kernel.org/all/20180324091954.73229-2-marcel@holtmann.org/

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@holtmann.org>
Signed-off-by: Calvin Owens <calvin@wbinvd.org>
---
 drivers/bluetooth/btmtksdio.c | 2 +-
 drivers/bluetooth/btmtkuart.c | 2 +-
 drivers/bluetooth/btnxpuart.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Comments

bluez.test.bot@gmail.com March 18, 2025, 7:53 p.m. UTC | #1 
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=945301

---Test result---

Test Summary:
CheckPatch                    PENDING   0.38 seconds
GitLint                       PENDING   0.23 seconds
SubjectPrefix                 FAIL      0.35 seconds
BuildKernel                   PASS      24.02 seconds
CheckAllWarning               PASS      26.34 seconds
CheckSparse                   PASS      29.85 seconds
BuildKernel32                 PASS      23.54 seconds
TestRunnerSetup               PASS      423.74 seconds
TestRunner_l2cap-tester       PASS      22.97 seconds
TestRunner_iso-tester         PASS      28.73 seconds
TestRunner_bnep-tester        PASS      4.73 seconds
TestRunner_mgmt-tester        FAIL      120.62 seconds
TestRunner_rfcomm-tester      PASS      7.74 seconds
TestRunner_sco-tester         PASS      12.18 seconds
TestRunner_ioctl-tester       PASS      8.13 seconds
TestRunner_mesh-tester        PASS      5.90 seconds
TestRunner_smp-tester         PASS      7.12 seconds
TestRunner_userchan-tester    PASS      4.91 seconds
IncrementalBuild              PENDING   0.94 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 484 (98.8%), Failed: 2, Not Run: 4

Failed Test Cases
LL Privacy - Set Flags 1 (Add to RL)                 Failed       0.134 seconds
LL Privacy - Set Flags 2 (Enable RL)                 Failed       0.151 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz March 19, 2025, 5:13 p.m. UTC | #2 
Hi Calvin,

On Tue, Mar 18, 2025 at 3:31 PM Calvin Owens <calvin@wbinvd.org> 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@wbinvd.org>
> 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@holtmann.org/
>     https://lore.kernel.org/all/20180324091954.73229-2-marcel@holtmann.org/
>
> 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@holtmann.org>
> Signed-off-by: Calvin Owens <calvin@wbinvd.org>
> ---
>  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
>
diff mbox series

Patch

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