| Message ID | 20210724140331.3465-1-penguin-kernel@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: use helper function for monitor's open/close notifications | expand |
Hi Tetsuo, On Sat, Jul 24, 2021 at 7:03 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > hci_sock.c has many > hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, HCI_SOCK_TRUSTED, NULL); > calls. Use helper functions and replace skb with sk. > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > net/bluetooth/hci_sock.c | 96 ++++++++++++++++------------------------ > 1 file changed, 37 insertions(+), 59 deletions(-) > > diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c > index 786a06a232fd..fc2336855dab 100644 > --- a/net/bluetooth/hci_sock.c > +++ b/net/bluetooth/hci_sock.c > @@ -295,6 +295,11 @@ void hci_send_to_channel(unsigned short channel, struct sk_buff *skb, > read_unlock(&hci_sk_list.lock); > } > > +static void __hci_send_to_monitor(struct sk_buff *skb) > +{ We can probably have the checks of NULL skb added directly here and perhaps kfree_skb as well since it seems it is always a copy that is sent here, the hci_send_to_monitor don't have __ prefix so I wonder why you have chosen to use it? > + hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, HCI_SOCK_TRUSTED, NULL); > +} > + > /* Send frame to monitor socket */ > void hci_send_to_monitor(struct hci_dev *hdev, struct sk_buff *skb) > { > @@ -350,8 +355,7 @@ void hci_send_to_monitor(struct hci_dev *hdev, struct sk_buff *skb) > hdr->index = cpu_to_le16(hdev->id); > hdr->len = cpu_to_le16(skb->len); > > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb_copy, > - HCI_SOCK_TRUSTED, NULL); > + __hci_send_to_monitor(skb_copy); > kfree_skb(skb_copy); > } > > @@ -545,6 +549,16 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk) > return skb; > } > > +static void hci_monitor_ctrl_open(struct sock *sk) > +{ > + struct sk_buff *skb = create_monitor_ctrl_open(sk); > + > + if (skb) { > + __hci_send_to_monitor(skb); > + kfree_skb(skb); > + } > +} > + > static struct sk_buff *create_monitor_ctrl_close(struct sock *sk) > { > struct hci_mon_hdr *hdr; > @@ -583,6 +597,16 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk) > return skb; > } > > +static void hci_monitor_ctrl_close(struct sock *sk) > +{ > + struct sk_buff *skb = create_monitor_ctrl_close(sk); > + > + if (skb) { > + __hci_send_to_monitor(skb); > + kfree_skb(skb); > + } > +} > + > static struct sk_buff *create_monitor_ctrl_command(struct sock *sk, u16 index, > u16 opcode, u16 len, > const void *buf) > @@ -741,8 +765,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event) > /* Send event to monitor */ > skb = create_monitor_event(hdev, event); > if (skb) { > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, > - HCI_SOCK_TRUSTED, NULL); > + __hci_send_to_monitor(skb); > kfree_skb(skb); > } > } > @@ -859,7 +882,6 @@ static int hci_sock_release(struct socket *sock) > { > struct sock *sk = sock->sk; > struct hci_dev *hdev; > - struct sk_buff *skb; > > BT_DBG("sock %p sk %p", sock, sk); > > @@ -876,12 +898,7 @@ static int hci_sock_release(struct socket *sock) > case HCI_CHANNEL_USER: > case HCI_CHANNEL_CONTROL: > /* Send event to monitor */ > - skb = create_monitor_ctrl_close(sk); > - if (skb) { > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, > - HCI_SOCK_TRUSTED, NULL); > - kfree_skb(skb); > - } > + hci_monitor_ctrl_close(sk); > > hci_sock_free_cookie(sk); > break; > @@ -1021,18 +1038,11 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd, > * of a given socket. > */ > if (hci_sock_gen_cookie(sk)) { > - struct sk_buff *skb; > - > if (capable(CAP_NET_ADMIN)) > hci_sock_set_flag(sk, HCI_SOCK_TRUSTED); > > /* Send event to monitor */ > - skb = create_monitor_ctrl_open(sk); > - if (skb) { > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, > - HCI_SOCK_TRUSTED, NULL); > - kfree_skb(skb); > - } > + hci_monitor_ctrl_open(sk); > } > > release_sock(sk); > @@ -1114,7 +1124,6 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, > struct sockaddr_hci haddr; > struct sock *sk = sock->sk; > struct hci_dev *hdev = NULL; > - struct sk_buff *skb; > int len, err = 0; > > BT_DBG("sock %p sk %p", sock, sk); > @@ -1162,12 +1171,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, > * notification. Send a close notification first to > * allow the state transition to bounded. > */ > - skb = create_monitor_ctrl_close(sk); > - if (skb) { > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, > - HCI_SOCK_TRUSTED, NULL); > - kfree_skb(skb); > - } > + hci_monitor_ctrl_close(sk); > } > > if (capable(CAP_NET_ADMIN)) > @@ -1176,12 +1180,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, > hci_pi(sk)->hdev = hdev; > > /* Send event to monitor */ > - skb = create_monitor_ctrl_open(sk); > - if (skb) { > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, > - HCI_SOCK_TRUSTED, NULL); > - kfree_skb(skb); > - } > + hci_monitor_ctrl_open(sk); > break; > > case HCI_CHANNEL_USER: > @@ -1251,12 +1250,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, > * a user channel socket. For a clean transition, send > * the close notification first. > */ > - skb = create_monitor_ctrl_close(sk); > - if (skb) { > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, > - HCI_SOCK_TRUSTED, NULL); > - kfree_skb(skb); > - } > + hci_monitor_ctrl_close(sk); > } > > /* The user channel is restricted to CAP_NET_ADMIN > @@ -1267,12 +1261,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, > hci_pi(sk)->hdev = hdev; > > /* Send event to monitor */ > - skb = create_monitor_ctrl_open(sk); > - if (skb) { > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, > - HCI_SOCK_TRUSTED, NULL); > - kfree_skb(skb); > - } > + hci_monitor_ctrl_open(sk); > > atomic_inc(&hdev->promisc); > break; > @@ -1359,21 +1348,11 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, > * allow for a clean transition, send the > * close notification first. > */ > - skb = create_monitor_ctrl_close(sk); > - if (skb) { > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, > - HCI_SOCK_TRUSTED, NULL); > - kfree_skb(skb); > - } > + hci_monitor_ctrl_close(sk); > } > > /* Send event to monitor */ > - skb = create_monitor_ctrl_open(sk); > - if (skb) { > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, > - HCI_SOCK_TRUSTED, NULL); > - kfree_skb(skb); > - } > + hci_monitor_ctrl_open(sk); > > hci_sock_set_flag(sk, HCI_MGMT_INDEX_EVENTS); > hci_sock_set_flag(sk, HCI_MGMT_UNCONF_INDEX_EVENTS); > @@ -1559,8 +1538,7 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, > skb = create_monitor_ctrl_command(sk, index, opcode, len, > buf + sizeof(*hdr)); > if (skb) { > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, > - HCI_SOCK_TRUSTED, NULL); > + __hci_send_to_monitor(skb); > kfree_skb(skb); > } > } > @@ -1715,7 +1693,7 @@ static int hci_logging_frame(struct sock *sk, struct msghdr *msg, int len) > > hdr->opcode = cpu_to_le16(HCI_MON_USER_LOGGING); > > - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, HCI_SOCK_TRUSTED, NULL); > + __hci_send_to_monitor(skb); > err = len; > > if (hdev) > -- > 2.18.4 >
On 2021/07/27 2:40, Luiz Augusto von Dentz wrote: >> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c >> index 786a06a232fd..fc2336855dab 100644 >> --- a/net/bluetooth/hci_sock.c >> +++ b/net/bluetooth/hci_sock.c >> @@ -295,6 +295,11 @@ void hci_send_to_channel(unsigned short channel, struct sk_buff *skb, >> read_unlock(&hci_sk_list.lock); >> } >> >> +static void __hci_send_to_monitor(struct sk_buff *skb) >> +{ > > We can probably have the checks of NULL skb added directly here and > perhaps kfree_skb as well since it seems it is always a copy that is > sent here, The NULL skb check is in hci_monitor_ctrl_open() and hci_monitor_ctrl_close(). The purpose of __hci_send_to_monitor() is to hide common arguments. > the hci_send_to_monitor don't have __ prefix so I wonder > why you have chosen to use it? Only to avoid name conflict with hci_send_to_monitor(). I thought that the __ prefix is fine because hci_send_to_monitor() also calls this function. Please suggest whatever name you want to use. > >> + hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, HCI_SOCK_TRUSTED, NULL); >> +} >> + >> /* Send frame to monitor socket */ >> void hci_send_to_monitor(struct hci_dev *hdev, struct sk_buff *skb) >> {
Hi Tetsuo, On Mon, Jul 26, 2021 at 1:40 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2021/07/27 2:40, Luiz Augusto von Dentz wrote: > >> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c > >> index 786a06a232fd..fc2336855dab 100644 > >> --- a/net/bluetooth/hci_sock.c > >> +++ b/net/bluetooth/hci_sock.c > >> @@ -295,6 +295,11 @@ void hci_send_to_channel(unsigned short channel, struct sk_buff *skb, > >> read_unlock(&hci_sk_list.lock); > >> } > >> > >> +static void __hci_send_to_monitor(struct sk_buff *skb) > >> +{ > > > > We can probably have the checks of NULL skb added directly here and > > perhaps kfree_skb as well since it seems it is always a copy that is > > sent here, > > The NULL skb check is in hci_monitor_ctrl_open() and hci_monitor_ctrl_close(). > The purpose of __hci_send_to_monitor() is to hide common arguments. All instances that call into it do seem to have the NULL check and kfree, in fact hci_monitor_ctrl_open and hci_monitor_ctrl_close do exactly the same thing: + if (skb) { + __hci_send_to_monitor(skb); + kfree_skb(skb); + } Perhap we can call it hci_send_ctrl_to_monitor: static void hci_send_ctrl_to_monitor(struct sk_buff *skb) { if (!skb) return; hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, HCI_SOCK_TRUSTED, NULL); kfree_skb(skb); } > > > the hci_send_to_monitor don't have __ prefix so I wonder > > why you have chosen to use it? > > Only to avoid name conflict with hci_send_to_monitor(). I thought that > the __ prefix is fine because hci_send_to_monitor() also calls this function. > Please suggest whatever name you want to use. > > > > >> + hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, HCI_SOCK_TRUSTED, NULL); > >> +} > >> + > >> /* Send frame to monitor socket */ > >> void hci_send_to_monitor(struct hci_dev *hdev, struct sk_buff *skb) > >> { >
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=520663 ---Test result--- Test Summary: CheckPatch PASS 0.49 seconds GitLint PASS 0.10 seconds BuildKernel PASS 512.83 seconds TestRunner: Setup PASS 339.19 seconds TestRunner: l2cap-tester PASS 2.59 seconds TestRunner: bnep-tester PASS 1.94 seconds TestRunner: mgmt-tester PASS 30.52 seconds TestRunner: rfcomm-tester PASS 2.06 seconds TestRunner: sco-tester PASS 2.01 seconds TestRunner: smp-tester FAIL 2.04 seconds TestRunner: userchan-tester PASS 1.93 seconds Details ############################## Test: CheckPatch - PASS - 0.49 seconds Run checkpatch.pl script with rule in .checkpatch.conf ############################## Test: GitLint - PASS - 0.10 seconds Run gitlint with rule in .gitlint ############################## Test: BuildKernel - PASS - 512.83 seconds Build Kernel with minimal configuration supports Bluetooth ############################## Test: TestRunner: Setup - PASS - 339.19 seconds Setup environment for running Test Runner ############################## Test: TestRunner: l2cap-tester - PASS - 2.59 seconds Run test-runner with l2cap-tester Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: bnep-tester - PASS - 1.94 seconds Run test-runner with bnep-tester Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: mgmt-tester - PASS - 30.52 seconds Run test-runner with mgmt-tester Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3 ############################## Test: TestRunner: rfcomm-tester - PASS - 2.06 seconds Run test-runner with rfcomm-tester Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: sco-tester - PASS - 2.01 seconds Run test-runner with sco-tester Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: smp-tester - FAIL - 2.04 seconds Run test-runner with smp-tester Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0 Failed Test Cases SMP Client - SC Request 2 Failed 0.019 seconds ############################## Test: TestRunner: userchan-tester - PASS - 1.93 seconds Run test-runner with userchan-tester Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0 --- Regards, Linux Bluetooth
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c index 786a06a232fd..fc2336855dab 100644 --- a/net/bluetooth/hci_sock.c +++ b/net/bluetooth/hci_sock.c @@ -295,6 +295,11 @@ void hci_send_to_channel(unsigned short channel, struct sk_buff *skb, read_unlock(&hci_sk_list.lock); } +static void __hci_send_to_monitor(struct sk_buff *skb) +{ + hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, HCI_SOCK_TRUSTED, NULL); +} + /* Send frame to monitor socket */ void hci_send_to_monitor(struct hci_dev *hdev, struct sk_buff *skb) { @@ -350,8 +355,7 @@ void hci_send_to_monitor(struct hci_dev *hdev, struct sk_buff *skb) hdr->index = cpu_to_le16(hdev->id); hdr->len = cpu_to_le16(skb->len); - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb_copy, - HCI_SOCK_TRUSTED, NULL); + __hci_send_to_monitor(skb_copy); kfree_skb(skb_copy); } @@ -545,6 +549,16 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk) return skb; } +static void hci_monitor_ctrl_open(struct sock *sk) +{ + struct sk_buff *skb = create_monitor_ctrl_open(sk); + + if (skb) { + __hci_send_to_monitor(skb); + kfree_skb(skb); + } +} + static struct sk_buff *create_monitor_ctrl_close(struct sock *sk) { struct hci_mon_hdr *hdr; @@ -583,6 +597,16 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk) return skb; } +static void hci_monitor_ctrl_close(struct sock *sk) +{ + struct sk_buff *skb = create_monitor_ctrl_close(sk); + + if (skb) { + __hci_send_to_monitor(skb); + kfree_skb(skb); + } +} + static struct sk_buff *create_monitor_ctrl_command(struct sock *sk, u16 index, u16 opcode, u16 len, const void *buf) @@ -741,8 +765,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event) /* Send event to monitor */ skb = create_monitor_event(hdev, event); if (skb) { - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, - HCI_SOCK_TRUSTED, NULL); + __hci_send_to_monitor(skb); kfree_skb(skb); } } @@ -859,7 +882,6 @@ static int hci_sock_release(struct socket *sock) { struct sock *sk = sock->sk; struct hci_dev *hdev; - struct sk_buff *skb; BT_DBG("sock %p sk %p", sock, sk); @@ -876,12 +898,7 @@ static int hci_sock_release(struct socket *sock) case HCI_CHANNEL_USER: case HCI_CHANNEL_CONTROL: /* Send event to monitor */ - skb = create_monitor_ctrl_close(sk); - if (skb) { - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, - HCI_SOCK_TRUSTED, NULL); - kfree_skb(skb); - } + hci_monitor_ctrl_close(sk); hci_sock_free_cookie(sk); break; @@ -1021,18 +1038,11 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd, * of a given socket. */ if (hci_sock_gen_cookie(sk)) { - struct sk_buff *skb; - if (capable(CAP_NET_ADMIN)) hci_sock_set_flag(sk, HCI_SOCK_TRUSTED); /* Send event to monitor */ - skb = create_monitor_ctrl_open(sk); - if (skb) { - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, - HCI_SOCK_TRUSTED, NULL); - kfree_skb(skb); - } + hci_monitor_ctrl_open(sk); } release_sock(sk); @@ -1114,7 +1124,6 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, struct sockaddr_hci haddr; struct sock *sk = sock->sk; struct hci_dev *hdev = NULL; - struct sk_buff *skb; int len, err = 0; BT_DBG("sock %p sk %p", sock, sk); @@ -1162,12 +1171,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, * notification. Send a close notification first to * allow the state transition to bounded. */ - skb = create_monitor_ctrl_close(sk); - if (skb) { - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, - HCI_SOCK_TRUSTED, NULL); - kfree_skb(skb); - } + hci_monitor_ctrl_close(sk); } if (capable(CAP_NET_ADMIN)) @@ -1176,12 +1180,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, hci_pi(sk)->hdev = hdev; /* Send event to monitor */ - skb = create_monitor_ctrl_open(sk); - if (skb) { - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, - HCI_SOCK_TRUSTED, NULL); - kfree_skb(skb); - } + hci_monitor_ctrl_open(sk); break; case HCI_CHANNEL_USER: @@ -1251,12 +1250,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, * a user channel socket. For a clean transition, send * the close notification first. */ - skb = create_monitor_ctrl_close(sk); - if (skb) { - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, - HCI_SOCK_TRUSTED, NULL); - kfree_skb(skb); - } + hci_monitor_ctrl_close(sk); } /* The user channel is restricted to CAP_NET_ADMIN @@ -1267,12 +1261,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, hci_pi(sk)->hdev = hdev; /* Send event to monitor */ - skb = create_monitor_ctrl_open(sk); - if (skb) { - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, - HCI_SOCK_TRUSTED, NULL); - kfree_skb(skb); - } + hci_monitor_ctrl_open(sk); atomic_inc(&hdev->promisc); break; @@ -1359,21 +1348,11 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, * allow for a clean transition, send the * close notification first. */ - skb = create_monitor_ctrl_close(sk); - if (skb) { - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, - HCI_SOCK_TRUSTED, NULL); - kfree_skb(skb); - } + hci_monitor_ctrl_close(sk); } /* Send event to monitor */ - skb = create_monitor_ctrl_open(sk); - if (skb) { - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, - HCI_SOCK_TRUSTED, NULL); - kfree_skb(skb); - } + hci_monitor_ctrl_open(sk); hci_sock_set_flag(sk, HCI_MGMT_INDEX_EVENTS); hci_sock_set_flag(sk, HCI_MGMT_UNCONF_INDEX_EVENTS); @@ -1559,8 +1538,7 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, skb = create_monitor_ctrl_command(sk, index, opcode, len, buf + sizeof(*hdr)); if (skb) { - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, - HCI_SOCK_TRUSTED, NULL); + __hci_send_to_monitor(skb); kfree_skb(skb); } } @@ -1715,7 +1693,7 @@ static int hci_logging_frame(struct sock *sk, struct msghdr *msg, int len) hdr->opcode = cpu_to_le16(HCI_MON_USER_LOGGING); - hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, HCI_SOCK_TRUSTED, NULL); + __hci_send_to_monitor(skb); err = len; if (hdev)
hci_sock.c has many hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, HCI_SOCK_TRUSTED, NULL); calls. Use helper functions and replace skb with sk. Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- net/bluetooth/hci_sock.c | 96 ++++++++++++++++------------------------ 1 file changed, 37 insertions(+), 59 deletions(-)