diff mbox series

[RFC] Bluetooth: hci_core: Introduce hci_recv_event_data

Message ID 20220204211236.2690926-2-luiz.dentz@gmail.com
State Superseded
Headers show
Series [RFC] Bluetooth: hci_core: Introduce hci_recv_event_data | expand

Commit Message

Luiz Augusto von Dentz Feb. 4, 2022, 9:12 p.m. UTC 
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This introduces hci_recv_event_data to make it simpler to access the
contents of last received event rather than having to pass its contents
to the likes of *_ind/*_cfm callbacks.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_core.c         | 32 ++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        |  3 +++
 3 files changed, 37 insertions(+)

Comments

Marcel Holtmann Feb. 7, 2022, 3:53 p.m. UTC | #1 
Hi Luiz,

> This introduces hci_recv_event_data to make it simpler to access the
> contents of last received event rather than having to pass its contents
> to the likes of *_ind/*_cfm callbacks.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/hci_core.h |  2 ++
> net/bluetooth/hci_core.c         | 32 ++++++++++++++++++++++++++++++++
> net/bluetooth/hci_event.c        |  3 +++
> 3 files changed, 37 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index f5caff1ddb29..c454913794bf 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -516,6 +516,7 @@ struct hci_dev {
> 	struct sk_buff_head	cmd_q;
> 
> 	struct sk_buff		*sent_cmd;
> +	struct sk_buff		*recv_event;
> 
> 	struct mutex		req_lock;
> 	wait_queue_head_t	req_wait_q;
> @@ -1727,6 +1728,7 @@ void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
> void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
> 
> void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
> +void *hci_recv_event_data(struct hci_dev *hdev, __u8 event);
> 
> u32 hci_conn_get_phy(struct hci_conn *conn);
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index b4782a6c1025..5d1167b67a47 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2740,6 +2740,7 @@ void hci_release_dev(struct hci_dev *hdev)
> 
> 	ida_simple_remove(&hci_index_ida, hdev->id);
> 	kfree_skb(hdev->sent_cmd);
> +	kfree_skb(hdev->recv_event);
> 	kfree(hdev);
> }
> EXPORT_SYMBOL(hci_release_dev);
> @@ -3024,6 +3025,37 @@ void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
> 	return hdev->sent_cmd->data + HCI_COMMAND_HDR_SIZE;
> }
> 
> +/* Get data from last received event */
> +void *hci_recv_event_data(struct hci_dev *hdev, __u8 event)
> +{
> +	struct hci_event_hdr *hdr;
> +	int offset;
> +
> +	if (!hdev->recv_event)
> +		return NULL;
> +
> +	hdr = (void *) hdev->recv_event->data;
> +	offset = sizeof(hdr);
> +
> +	if (hdr->evt != event) {
> +		/* In case of LE metaevent check the subevent match */
> +		if (hdr->evt == HCI_EV_LE_META) {
> +			struct hci_ev_le_meta *ev;
> +
> +			ev = (void *) hdev->recv_event->data + offset;
> +			offset += sizeof(*ev);
> +			if (ev->subevent == event)
> +				goto found;
> +		}
> +		return NULL;
> +	}
> +
> +found:
> +	bt_dev_dbg(hdev, "event 0x%2.2x", event);
> +
> +	return hdev->recv_event->data + offset;
> +}
> +
> /* Send ACL data */
> static void hci_add_acl_hdr(struct sk_buff *skb, __u16 handle, __u16 flags)
> {
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 63b925921c87..613050bd80cc 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -6898,6 +6898,9 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
> 		goto done;
> 	}
> 
> +	kfree_skb(hdev->recv_event);
> +	hdev->recv_event = skb_clone(skb, GFP_KERNEL);
> +

fill me into why this a good idea. We end up creating clones of an SKB. Is this a good idea?

Regards

Marcel
Luiz Augusto von Dentz Feb. 8, 2022, 12:16 a.m. UTC | #2 
Hi Marcel,

On Mon, Feb 7, 2022 at 7:53 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This introduces hci_recv_event_data to make it simpler to access the
> > contents of last received event rather than having to pass its contents
> > to the likes of *_ind/*_cfm callbacks.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/hci_core.h |  2 ++
> > net/bluetooth/hci_core.c         | 32 ++++++++++++++++++++++++++++++++
> > net/bluetooth/hci_event.c        |  3 +++
> > 3 files changed, 37 insertions(+)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index f5caff1ddb29..c454913794bf 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -516,6 +516,7 @@ struct hci_dev {
> >       struct sk_buff_head     cmd_q;
> >
> >       struct sk_buff          *sent_cmd;
> > +     struct sk_buff          *recv_event;
> >
> >       struct mutex            req_lock;
> >       wait_queue_head_t       req_wait_q;
> > @@ -1727,6 +1728,7 @@ void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
> > void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
> >
> > void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
> > +void *hci_recv_event_data(struct hci_dev *hdev, __u8 event);
> >
> > u32 hci_conn_get_phy(struct hci_conn *conn);
> >
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index b4782a6c1025..5d1167b67a47 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -2740,6 +2740,7 @@ void hci_release_dev(struct hci_dev *hdev)
> >
> >       ida_simple_remove(&hci_index_ida, hdev->id);
> >       kfree_skb(hdev->sent_cmd);
> > +     kfree_skb(hdev->recv_event);
> >       kfree(hdev);
> > }
> > EXPORT_SYMBOL(hci_release_dev);
> > @@ -3024,6 +3025,37 @@ void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
> >       return hdev->sent_cmd->data + HCI_COMMAND_HDR_SIZE;
> > }
> >
> > +/* Get data from last received event */
> > +void *hci_recv_event_data(struct hci_dev *hdev, __u8 event)
> > +{
> > +     struct hci_event_hdr *hdr;
> > +     int offset;
> > +
> > +     if (!hdev->recv_event)
> > +             return NULL;
> > +
> > +     hdr = (void *) hdev->recv_event->data;
> > +     offset = sizeof(hdr);
> > +
> > +     if (hdr->evt != event) {
> > +             /* In case of LE metaevent check the subevent match */
> > +             if (hdr->evt == HCI_EV_LE_META) {
> > +                     struct hci_ev_le_meta *ev;
> > +
> > +                     ev = (void *) hdev->recv_event->data + offset;
> > +                     offset += sizeof(*ev);
> > +                     if (ev->subevent == event)
> > +                             goto found;
> > +             }
> > +             return NULL;
> > +     }
> > +
> > +found:
> > +     bt_dev_dbg(hdev, "event 0x%2.2x", event);
> > +
> > +     return hdev->recv_event->data + offset;
> > +}
> > +
> > /* Send ACL data */
> > static void hci_add_acl_hdr(struct sk_buff *skb, __u16 handle, __u16 flags)
> > {
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 63b925921c87..613050bd80cc 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -6898,6 +6898,9 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
> >               goto done;
> >       }
> >
> > +     kfree_skb(hdev->recv_event);
> > +     hdev->recv_event = skb_clone(skb, GFP_KERNEL);
> > +
>
> fill me into why this a good idea. We end up creating clones of an SKB. Is this a good idea?

It is limited to just the last event, Id like to introduce since in
most cases _ind/_cfm callbacks are done with use of an hci_conn but in
case of BIS we don't really have a connection until we have created a
BIG Sync so I end up doing the following:

https://gist.github.com/Vudentz/2dd08291a0ae0df7fff1034653b01e97#file-diff-L279
https://gist.github.com/Vudentz/2dd08291a0ae0df7fff1034653b01e97#file-diff-L337

So that saves us the trouble of having to add or change the existing
callbacks to sockets, so the socket themselves can check the last
event data instead if it really needs to.

> Regards
>
> Marcel
>
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index f5caff1ddb29..c454913794bf 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -516,6 +516,7 @@  struct hci_dev {
 	struct sk_buff_head	cmd_q;
 
 	struct sk_buff		*sent_cmd;
+	struct sk_buff		*recv_event;
 
 	struct mutex		req_lock;
 	wait_queue_head_t	req_wait_q;
@@ -1727,6 +1728,7 @@  void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
 void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
 
 void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
+void *hci_recv_event_data(struct hci_dev *hdev, __u8 event);
 
 u32 hci_conn_get_phy(struct hci_conn *conn);
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b4782a6c1025..5d1167b67a47 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2740,6 +2740,7 @@  void hci_release_dev(struct hci_dev *hdev)
 
 	ida_simple_remove(&hci_index_ida, hdev->id);
 	kfree_skb(hdev->sent_cmd);
+	kfree_skb(hdev->recv_event);
 	kfree(hdev);
 }
 EXPORT_SYMBOL(hci_release_dev);
@@ -3024,6 +3025,37 @@  void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
 	return hdev->sent_cmd->data + HCI_COMMAND_HDR_SIZE;
 }
 
+/* Get data from last received event */
+void *hci_recv_event_data(struct hci_dev *hdev, __u8 event)
+{
+	struct hci_event_hdr *hdr;
+	int offset;
+
+	if (!hdev->recv_event)
+		return NULL;
+
+	hdr = (void *) hdev->recv_event->data;
+	offset = sizeof(hdr);
+
+	if (hdr->evt != event) {
+		/* In case of LE metaevent check the subevent match */
+		if (hdr->evt == HCI_EV_LE_META) {
+			struct hci_ev_le_meta *ev;
+
+			ev = (void *) hdev->recv_event->data + offset;
+			offset += sizeof(*ev);
+			if (ev->subevent == event)
+				goto found;
+		}
+		return NULL;
+	}
+
+found:
+	bt_dev_dbg(hdev, "event 0x%2.2x", event);
+
+	return hdev->recv_event->data + offset;
+}
+
 /* Send ACL data */
 static void hci_add_acl_hdr(struct sk_buff *skb, __u16 handle, __u16 flags)
 {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 63b925921c87..613050bd80cc 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -6898,6 +6898,9 @@  void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 		goto done;
 	}
 
+	kfree_skb(hdev->recv_event);
+	hdev->recv_event = skb_clone(skb, GFP_KERNEL);
+
 	event = hdr->evt;
 	if (!event) {
 		bt_dev_warn(hdev, "Received unexpected HCI Event 0x%2.2x",