Re: [PATCH v5 3/3] Bluetooth: Add BT_MODE socket option

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Marcel,

On Tue, Mar 24, 2020 at 1:44 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This adds BT_MODE socket option which can be used to set L2CAP modes,
> > including modes only supported over LE which were not supported using
> > the L2CAP_OPTIONS.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/bluetooth.h |   8 ++
> > include/net/bluetooth/l2cap.h     |   6 ++
> > net/bluetooth/l2cap_sock.c        | 124 ++++++++++++++++++++++++++++++
> > 3 files changed, 138 insertions(+)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 1576353a2773..3fa7b1e3c5d9 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -139,6 +139,14 @@ struct bt_voice {
> > #define BT_PHY_LE_CODED_TX    0x00002000
> > #define BT_PHY_LE_CODED_RX    0x00004000
> >
> > +#define BT_MODE                      15
> > +
> > +#define BT_MODE_BASIC                0x00
> > +#define BT_MODE_ERTM         0x01
> > +#define BT_MODE_STREAMING    0x02
> > +#define BT_MODE_LE_FLOWCTL   0x03
> > +#define BT_MODE_EXT_FLOWCTL  0x04
> > +
> > __printf(1, 2)
> > void bt_info(const char *fmt, ...);
> > __printf(1, 2)
> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > index dada14d0622c..56f727ba23bd 100644
> > --- a/include/net/bluetooth/l2cap.h
> > +++ b/include/net/bluetooth/l2cap.h
> > @@ -720,9 +720,15 @@ struct l2cap_user {
> > /* ----- L2CAP socket info ----- */
> > #define l2cap_pi(sk) ((struct l2cap_pinfo *) sk)
> >
> > +#define L2CAP_PI_OPTION_UNSET                0x00
> > +#define L2CAP_PI_OPTION_LEGACY               0x01
> > +#define L2CAP_PI_OPTION_BT_MODE              0x02
> > +
> > struct l2cap_pinfo {
> >       struct bt_sock          bt;
> >       struct l2cap_chan       *chan;
> > +     u8                      option;
> > +     u8                      bt_mode;
> >       struct sk_buff          *rx_busy_skb;
> > };
>
> why do you want to store bt_mode here. Whatever we have in l2cap_chan should be plenty.

Ive thought it would be cleaner to mess with types that comes from the
spec itself so l2cap_chan would continue to use them.

> I also looked at l2cap_sock_setsockopt_old and if you use L2CAP_OPTIONS and want to read BT_MODE, then everything should be fine. Same as setting BT_MODE (except EXT_FLOWCTL) and then reading L2CAP_OPTIONS is fine as well. We can all translate this properly and with have EINVAL return errors for not supported / disabled modes.
>
> So the only time L2CAP_OPTIONS read should fail is if you use BT_MODE with EXT_FLOWCTL as mode. So you can just check the mode set in l2cap_chan. And we start using our new mode definition there and then convert it for L2CAP_OPTIONS.

Sure, I thought it would be more efficient to not have conversions
back and forth but Im fine either way.

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz