Re: [PATCHv2] Bluetooth: new sockopt to enter active state when sending data

[Marcel Holtmann <marcel@holtmann.org>]

Hi Andrei,

> >> Patch adds new socket option to enter active state when sending
> >> data. Modified version of Fabien Chevalier patch (Sep/2008).
> >> Discussions:
> >> http://www.spinics.net/lists/bluez-devel/msg00567.html
> >> http://www.spinics.net/lists/linux-bluetooth/msg03765.html
> >>
> >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> >> ---
> >>  include/net/bluetooth/hci_core.h |    1 +
> >>  include/net/bluetooth/l2cap.h    |    2 ++
> >>  net/bluetooth/hci_conn.c         |    7 ++++++-
> >>  net/bluetooth/l2cap.c            |   15 +++++++++++++++
> >>  4 files changed, 24 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> index e42f6ed..477a492 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -178,6 +178,7 @@ struct hci_conn {
> >>       __u8             auth_type;
> >>       __u8             sec_level;
> >>       __u8             power_save;
> >> +     __u8             force_active;
> >>       __u16            disc_timeout;
> >>       unsigned long    pend;
> >>
> >> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> >> index 7c695bf..540cca7 100644
> >> --- a/include/net/bluetooth/l2cap.h
> >> +++ b/include/net/bluetooth/l2cap.h
> >> @@ -74,6 +74,8 @@ struct l2cap_conninfo {
> >>  #define L2CAP_LM_RELIABLE    0x0010
> >>  #define L2CAP_LM_SECURE              0x0020
> >>
> >> +#define L2CAP_FORCE_ACTIVE_MODE      0x04
> >> +
> >>  /* L2CAP command codes */
> >>  #define L2CAP_COMMAND_REJ    0x01
> >>  #define L2CAP_CONN_REQ               0x02
> >> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> >> index 800b6b9..11fc44a 100644
> >> --- a/net/bluetooth/hci_conn.c
> >> +++ b/net/bluetooth/hci_conn.c
> >> @@ -214,6 +214,8 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
> >>       conn->auth_type = HCI_AT_GENERAL_BONDING;
> >>
> >>       conn->power_save = 1;
> >> +     /* Do not enter active state by default */
> >> +     conn->force_active = 0;
> >>       conn->disc_timeout = HCI_DISCONN_TIMEOUT;
> >>
> >>       switch (type) {
> >> @@ -505,7 +507,10 @@ void hci_conn_enter_active_mode(struct hci_conn *conn)
> >>       if (test_bit(HCI_RAW, &hdev->flags))
> >>               return;
> >>
> >> -     if (conn->mode != HCI_CM_SNIFF || !conn->power_save)
> >> +     if (conn->mode != HCI_CM_SNIFF)
> >> +             goto timer;
> >> +
> >> +     if (!conn->power_save && !conn->force_active)
> >>               goto timer;
> >>
> >>       if (!test_and_set_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend)) {
> >> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >> index cf3c407..5328e82 100644
> >> --- a/net/bluetooth/l2cap.c
> >> +++ b/net/bluetooth/l2cap.c
> >> @@ -1909,6 +1909,15 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
> >>               l2cap_pi(sk)->force_reliable = (opt & L2CAP_LM_RELIABLE);
> >>               break;
> >>
> >> +     case L2CAP_FORCE_ACTIVE_MODE:
> >> +             if (get_user(opt, (u32 __user *) optval)) {
> >> +                     err = -EFAULT;
> >> +                     break;
> >> +             }
> >> +
> >> +             l2cap_pi(sk)->conn->hcon->force_active = opt;
> >> +             break;
> >> +
> >>       default:
> >>               err = -ENOPROTOOPT;
> >>               break;
> >> @@ -2058,6 +2067,12 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, char __us
> >>
> >>               break;
> >>
> >> +     case L2CAP_FORCE_ACTIVE_MODE:
> >> +             if (put_user(l2cap_pi(sk)->conn->hcon->force_active,
> >> +                                     (u32 __user *) optval))
> >> +                     err = -EFAULT;
> >> +             break;
> >> +
> >>       default:
> >>               err = -ENOPROTOOPT;
> >>               break;
> >
> > I think that I said this before. Why do you think that adding a new
> > socket option to something called _old is a good idea? I made it pretty
> > clear in the code that it is an OLD interface.
> >
> > I want this on the SOL_BLUETOOTH level with a proper new socket option.
> > And we might even consider doing it like BT_SECURITY where create the
> > struct to potentially add other flags later.
> 
> This can be done.
> 
> > So why not doing BT_POWER or something like that.
> >
> > And in addition you need to store these values on a per L2CAP or RFCOMM
> > socket. You can not just go ahead and mess inside hci_conn via an L2CAP
> > socket option.
> 
> I felt that Sniff Mode is a property of ACL link and not L2CAP or RFCOMM socket.
> 
> There is already code in the same functions which use the same
> technique to access
> following hci_conn fields:
> 
> l2cap_pi(sk)->conn->hcon->handle
> l2cap_pi(sk)->conn->hcon->dev_class

that is for pure read-only access. If multiple L2CAP sockets are open
you have to be smart about the policy you put onto your ACL link.

Regards

Marcel