Hi Marcel,
On Fri, Dec 3, 2021 at 1:40 PM Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
>
> Hi Luiz,
>
> > This introduces mgmt_alloc_skb and mgmt_send_event_skb which are
> > convenient when building MGMT events that have variable length as the
> > likes of skb_put_data can be used to insert portion directly on the skb
> > instead of having to first build an intermediate buffer just to be
> > copied over the skb.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> > ---
> > net/bluetooth/mgmt_util.c | 54 +++++++++++++++++++++++++++------------
> > net/bluetooth/mgmt_util.h | 3 +++
> > 2 files changed, 40 insertions(+), 17 deletions(-)
> >
> > diff --git a/net/bluetooth/mgmt_util.c b/net/bluetooth/mgmt_util.c
> > index 83875f2a0604..4774e993d774 100644
> > --- a/net/bluetooth/mgmt_util.c
> > +++ b/net/bluetooth/mgmt_util.c
> > @@ -56,40 +56,60 @@ static struct sk_buff *create_monitor_ctrl_event(__le16 index, u32 cookie,
> > return skb;
> > }
> >
> > -int mgmt_send_event(u16 event, struct hci_dev *hdev, unsigned short channel,
> > - void *data, u16 data_len, int flag, struct sock *skip_sk)
> > +struct sk_buff *mgmt_alloc_skb(unsigned int size)
> > {
> > struct sk_buff *skb;
> > +
> > + skb = alloc_skb(sizeof(struct mgmt_hdr) + size, GFP_KERNEL);
> > + if (skb)
> > + skb_reserve(skb, sizeof(struct mgmt_hdr));
> > +
> > + return skb;
> > +}
> > +
> > +int mgmt_send_event_skb(u16 event, struct hci_dev *hdev, unsigned short channel,
> > + struct sk_buff *skb, int flag, struct sock *skip_sk)
> > +{
> > struct mgmt_hdr *hdr;
> > + int len = skb->len;
> >
> > - skb = alloc_skb(sizeof(*hdr) + data_len, GFP_KERNEL);
> > - if (!skb)
> > - return -ENOMEM;
> > + /* Time stamp */
> > + __net_timestamp(skb);
> >
> > - hdr = skb_put(skb, sizeof(*hdr));
> > + /* Send just the data, without headers, to the monitor */
> > + if (channel == HCI_CHANNEL_CONTROL)
> > + hci_send_monitor_ctrl_event(hdev, event, skb->data, skb->len,
> > + skb_get_ktime(skb), flag, skip_sk);
> > +
> > + hdr = skb_push(skb, sizeof(*hdr));
> > hdr->opcode = cpu_to_le16(event);
> > if (hdev)
> > hdr->index = cpu_to_le16(hdev->id);
> > else
> > hdr->index = cpu_to_le16(MGMT_INDEX_NONE);
> > - hdr->len = cpu_to_le16(data_len);
> > -
> > - if (data)
> > - skb_put_data(skb, data, data_len);
> > -
> > - /* Time stamp */
> > - __net_timestamp(skb);
> > + hdr->len = cpu_to_le16(len);
> >
> > hci_send_to_channel(channel, skb, flag, skip_sk);
> >
> > - if (channel == HCI_CHANNEL_CONTROL)
> > - hci_send_monitor_ctrl_event(hdev, event, data, data_len,
> > - skb_get_ktime(skb), flag, skip_sk);
> > -
> > kfree_skb(skb);
> > return 0;
> > }
> >
> > +int mgmt_send_event(u16 event, struct hci_dev *hdev, unsigned short channel,
> > + void *data, u16 data_len, int flag, struct sock *skip_sk)
> > +{
> > + struct sk_buff *skb;
> > +
> > + skb = mgmt_alloc_skb(data_len);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + if (data)
> > + skb_put_data(skb, data, data_len);
> > +
> > + return mgmt_send_event_skb(event, hdev, channel, skb, flag, skip_sk);
> > +}
> > +
> > int mgmt_cmd_status(struct sock *sk, u16 index, u16 cmd, u8 status)
> > {
> > struct sk_buff *skb, *mskb;
> > diff --git a/net/bluetooth/mgmt_util.h b/net/bluetooth/mgmt_util.h
> > index 63b965eaaaac..9f8692d4ce90 100644
> > --- a/net/bluetooth/mgmt_util.h
> > +++ b/net/bluetooth/mgmt_util.h
> > @@ -32,6 +32,9 @@ struct mgmt_pending_cmd {
> > int (*cmd_complete)(struct mgmt_pending_cmd *cmd, u8 status);
> > };
> >
> > +struct sk_buff *mgmt_alloc_skb(unsigned int size);
> > +int mgmt_send_event_skb(u16 event, struct hci_dev *hdev, unsigned short channel,
> > + struct sk_buff *skb, int flag, struct sock *skip_sk);
> > int mgmt_send_event(u16 event, struct hci_dev *hdev, unsigned short channel,
> > void *data, u16 data_len, int flag, struct sock *skip_sk);
> > int mgmt_cmd_status(struct sock *sk, u16 index, u16 cmd, u8 status);
>
> this altogether doesn’t create nicer to read code. What about doing this:
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 2f31e571f34c..fd0a114a69c6 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -399,6 +399,7 @@ struct bt_skb_cb {
> struct l2cap_ctrl l2cap;
> struct sco_ctrl sco;
> struct hci_ctrl hci;
> + struct mgmt_ctrl mgmt;
> };
> };
>
> And then adding hci_dev and event to mgmt_ctrl. So you can do something like:
>
> struct sk_buff *mgmt_alloc_skb(struct hci_dev *hdev, u16 event, unsigned int size);
So you want bt_cb to be used when sending? We could as well push the
mgmt_header as well and then update the length at send since the
header is already at the start in skb->data.
> Then the send_event becomes easier to read. Right now it is total mess with the allocated skb parameter in the middle. It should be first and center to make sure that is the one that matter.
>
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
