mirrors/linux
1
0
Fork 1
mirror of https://github.com/torvalds/linux.git synced 2025-11-03 10:10:33 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions

Bluetooth: remove duplicate h4_recv_buf() in header

Browse source 
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 07eb96a5a7 ("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:

    ef564119ba ("Bluetooth: hci_h4: Add support for ISO packets")
    61b27cdf02 ("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>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This commit is contained in:
Calvin Owens 2025-08-25 21:11:08 -07:00 committed by Luiz Augusto von Dentz
parent 7722d6fb54
commit 93f06f8f0d
5 changed files with 4 additions and 157 deletions
repo.diff.show_diff_stats Download patch file Download diff file Expand all files Collapse all files

2
drivers/bluetooth/bpa10x.c
View file

@ -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"

2
drivers/bluetooth/btmtksdio.c
View file

@ -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"

2
drivers/bluetooth/btmtkuart.c
View file

@ -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"

2
drivers/bluetooth/btnxpuart.c
View file

@ -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

153
drivers/bluetooth/h4_recv.h
View file

@ -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;
}