| Message ID | 20200318215019.21494-4-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Marcel Holtmann |
| Headers | show |
| Series | Enable use of L2CAP_MODE_EXT_FLOWCTL | expand |
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 | 11 ++++ > net/bluetooth/l2cap_sock.c | 96 +++++++++++++++++++++++++++++++ > 2 files changed, 107 insertions(+) > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > index 1576353a2773..34191e34bfdc 100644 > --- a/include/net/bluetooth/bluetooth.h > +++ b/include/net/bluetooth/bluetooth.h > @@ -139,6 +139,17 @@ 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_RETRANS 0x01 > +#define BT_MODE_FLOWCTL 0x02 > +#define BT_MODE_ERTM 0x03 > +#define BT_MODE_STREAMING 0x04 > +#define BT_MODE_EXT_FLOWCTL 0x05 > + > +#define BT_MODE_LE_FLOWCTL 0x80 > + what I would do is just this: BASIC 0x00 ERTM 0x01 STREAMING 0x02 LE_FLOWCTL 0x03 EXT_FLOWCTL 0x04 Trying to cling onto some old L2CAP definition from the 2.1 days is not helpful. I would really make a clean cut here. This way we can also cleanly check the available modes per selected socket and have either setsockopt or connect fail appropriately. > __printf(1, 2) > void bt_info(const char *fmt, ...); > __printf(1, 2) > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c > index 117ba20ea194..f2bb376c699f 100644 > --- a/net/bluetooth/l2cap_sock.c > +++ b/net/bluetooth/l2cap_sock.c > @@ -500,6 +500,25 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, > return err; > } > > +static u8 l2cap_get_mode(struct l2cap_chan *chan) > +{ > + switch (chan->mode) { > + case L2CAP_MODE_BASIC: > + case L2CAP_MODE_RETRANS: > + case L2CAP_MODE_FLOWCTL: > + case L2CAP_MODE_ERTM: > + case L2CAP_MODE_STREAMING: > + /* Mode above are the same on both old and new sockopt */ > + return chan->mode; > + case L2CAP_MODE_LE_FLOWCTL: > + return BT_MODE_FLOWCTL; > + case L2CAP_MODE_EXT_FLOWCTL: > + return BT_MODE_EXT_FLOWCTL; > + } > + > + return chan->mode; > +} > + Don’t bother with this. Keep the old socket and new socket independent code. I also want to add Kconfig option later that will allow us to disable the old socket options once we have SOL_L2CAP requirement eradicated. Regards Marcel
Hi Marcel, On Thu, Mar 19, 2020 at 5:58 PM 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 | 11 ++++ > > net/bluetooth/l2cap_sock.c | 96 +++++++++++++++++++++++++++++++ > > 2 files changed, 107 insertions(+) > > > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > > index 1576353a2773..34191e34bfdc 100644 > > --- a/include/net/bluetooth/bluetooth.h > > +++ b/include/net/bluetooth/bluetooth.h > > @@ -139,6 +139,17 @@ 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_RETRANS 0x01 > > +#define BT_MODE_FLOWCTL 0x02 > > +#define BT_MODE_ERTM 0x03 > > +#define BT_MODE_STREAMING 0x04 > > +#define BT_MODE_EXT_FLOWCTL 0x05 > > + > > +#define BT_MODE_LE_FLOWCTL 0x80 > > + > > what I would do is just this: > > BASIC 0x00 > ERTM 0x01 > STREAMING 0x02 > LE_FLOWCTL 0x03 > EXT_FLOWCTL 0x04 > > Trying to cling onto some old L2CAP definition from the 2.1 days is not helpful. I would really make a clean cut here. Just to confirm, that means we will not going to support the old flowctl and retransmit modes from BR/EDR with BT_MODE? > This way we can also cleanly check the available modes per selected socket and have either setsockopt or connect fail appropriately. > > > __printf(1, 2) > > void bt_info(const char *fmt, ...); > > __printf(1, 2) > > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c > > index 117ba20ea194..f2bb376c699f 100644 > > --- a/net/bluetooth/l2cap_sock.c > > +++ b/net/bluetooth/l2cap_sock.c > > @@ -500,6 +500,25 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, > > return err; > > } > > > > +static u8 l2cap_get_mode(struct l2cap_chan *chan) > > +{ > > + switch (chan->mode) { > > + case L2CAP_MODE_BASIC: > > + case L2CAP_MODE_RETRANS: > > + case L2CAP_MODE_FLOWCTL: > > + case L2CAP_MODE_ERTM: > > + case L2CAP_MODE_STREAMING: > > + /* Mode above are the same on both old and new sockopt */ > > + return chan->mode; > > + case L2CAP_MODE_LE_FLOWCTL: > > + return BT_MODE_FLOWCTL; > > + case L2CAP_MODE_EXT_FLOWCTL: > > + return BT_MODE_EXT_FLOWCTL; > > + } > > + > > + return chan->mode; > > +} > > + > > Don’t bother with this. Keep the old socket and new socket independent code. I also want to add Kconfig option later that will allow us to disable the old socket options once we have SOL_L2CAP requirement eradicated. The reason I had the defines intermixed was that application would be able to use the old sockopt to set it and then use BT_MODE to read it, in which case I may need to store the actual socket mode separately from the chan->mode and then perhaps fail if application attempt to read it with BT_MODE if was not set using it. > Regards > > Marcel >
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 | 11 ++++ >>> net/bluetooth/l2cap_sock.c | 96 +++++++++++++++++++++++++++++++ >>> 2 files changed, 107 insertions(+) >>> >>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h >>> index 1576353a2773..34191e34bfdc 100644 >>> --- a/include/net/bluetooth/bluetooth.h >>> +++ b/include/net/bluetooth/bluetooth.h >>> @@ -139,6 +139,17 @@ 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_RETRANS 0x01 >>> +#define BT_MODE_FLOWCTL 0x02 >>> +#define BT_MODE_ERTM 0x03 >>> +#define BT_MODE_STREAMING 0x04 >>> +#define BT_MODE_EXT_FLOWCTL 0x05 >>> + >>> +#define BT_MODE_LE_FLOWCTL 0x80 >>> + >> >> what I would do is just this: >> >> BASIC 0x00 >> ERTM 0x01 >> STREAMING 0x02 >> LE_FLOWCTL 0x03 >> EXT_FLOWCTL 0x04 >> >> Trying to cling onto some old L2CAP definition from the 2.1 days is not helpful. I would really make a clean cut here. > > Just to confirm, that means we will not going to support the old > flowctl and retransmit modes from BR/EDR with BT_MODE? we currently don’t support these two modes, nor are there any specs that require it. >> This way we can also cleanly check the available modes per selected socket and have either setsockopt or connect fail appropriately. >> >>> __printf(1, 2) >>> void bt_info(const char *fmt, ...); >>> __printf(1, 2) >>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c >>> index 117ba20ea194..f2bb376c699f 100644 >>> --- a/net/bluetooth/l2cap_sock.c >>> +++ b/net/bluetooth/l2cap_sock.c >>> @@ -500,6 +500,25 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, >>> return err; >>> } >>> >>> +static u8 l2cap_get_mode(struct l2cap_chan *chan) >>> +{ >>> + switch (chan->mode) { >>> + case L2CAP_MODE_BASIC: >>> + case L2CAP_MODE_RETRANS: >>> + case L2CAP_MODE_FLOWCTL: >>> + case L2CAP_MODE_ERTM: >>> + case L2CAP_MODE_STREAMING: >>> + /* Mode above are the same on both old and new sockopt */ >>> + return chan->mode; >>> + case L2CAP_MODE_LE_FLOWCTL: >>> + return BT_MODE_FLOWCTL; >>> + case L2CAP_MODE_EXT_FLOWCTL: >>> + return BT_MODE_EXT_FLOWCTL; >>> + } >>> + >>> + return chan->mode; >>> +} >>> + >> >> Don’t bother with this. Keep the old socket and new socket independent code. I also want to add Kconfig option later that will allow us to disable the old socket options once we have SOL_L2CAP requirement eradicated. > > The reason I had the defines intermixed was that application would be > able to use the old sockopt to set it and then use BT_MODE to read it, > in which case I may need to store the actual socket mode separately > from the chan->mode and then perhaps fail if application attempt to > read it with BT_MODE if was not set using it. If someone used L2CAP_OTIONS, then just lets fail BT_MODE read and write. Regards Marcel
Hi Marcel. On Mon, Mar 23, 2020 at 10:42 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 | 11 ++++ > >>> net/bluetooth/l2cap_sock.c | 96 +++++++++++++++++++++++++++++++ > >>> 2 files changed, 107 insertions(+) > >>> > >>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > >>> index 1576353a2773..34191e34bfdc 100644 > >>> --- a/include/net/bluetooth/bluetooth.h > >>> +++ b/include/net/bluetooth/bluetooth.h > >>> @@ -139,6 +139,17 @@ 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_RETRANS 0x01 > >>> +#define BT_MODE_FLOWCTL 0x02 > >>> +#define BT_MODE_ERTM 0x03 > >>> +#define BT_MODE_STREAMING 0x04 > >>> +#define BT_MODE_EXT_FLOWCTL 0x05 > >>> + > >>> +#define BT_MODE_LE_FLOWCTL 0x80 > >>> + > >> > >> what I would do is just this: > >> > >> BASIC 0x00 > >> ERTM 0x01 > >> STREAMING 0x02 > >> LE_FLOWCTL 0x03 > >> EXT_FLOWCTL 0x04 > >> > >> Trying to cling onto some old L2CAP definition from the 2.1 days is not helpful. I would really make a clean cut here. > > > > Just to confirm, that means we will not going to support the old > > flowctl and retransmit modes from BR/EDR with BT_MODE? > > we currently don’t support these two modes, nor are there any specs that require it. Right, it seems we didn't validate these modes in L2CAP_OPTION though, I left that unchanged in the last set. > >> This way we can also cleanly check the available modes per selected socket and have either setsockopt or connect fail appropriately. > >> > >>> __printf(1, 2) > >>> void bt_info(const char *fmt, ...); > >>> __printf(1, 2) > >>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c > >>> index 117ba20ea194..f2bb376c699f 100644 > >>> --- a/net/bluetooth/l2cap_sock.c > >>> +++ b/net/bluetooth/l2cap_sock.c > >>> @@ -500,6 +500,25 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, > >>> return err; > >>> } > >>> > >>> +static u8 l2cap_get_mode(struct l2cap_chan *chan) > >>> +{ > >>> + switch (chan->mode) { > >>> + case L2CAP_MODE_BASIC: > >>> + case L2CAP_MODE_RETRANS: > >>> + case L2CAP_MODE_FLOWCTL: > >>> + case L2CAP_MODE_ERTM: > >>> + case L2CAP_MODE_STREAMING: > >>> + /* Mode above are the same on both old and new sockopt */ > >>> + return chan->mode; > >>> + case L2CAP_MODE_LE_FLOWCTL: > >>> + return BT_MODE_FLOWCTL; > >>> + case L2CAP_MODE_EXT_FLOWCTL: > >>> + return BT_MODE_EXT_FLOWCTL; > >>> + } > >>> + > >>> + return chan->mode; > >>> +} > >>> + > >> > >> Don’t bother with this. Keep the old socket and new socket independent code. I also want to add Kconfig option later that will allow us to disable the old socket options once we have SOL_L2CAP requirement eradicated. > > > > The reason I had the defines intermixed was that application would be > > able to use the old sockopt to set it and then use BT_MODE to read it, > > in which case I may need to store the actual socket mode separately > > from the chan->mode and then perhaps fail if application attempt to > > read it with BT_MODE if was not set using it. > > If someone used L2CAP_OTIONS, then just lets fail BT_MODE read and write. Right, Ive made it fail with lastest change, I can probably have a flag to make it exclusively thogh with one or another, I will send an update shortly. > Regards > > Marcel >
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h index 1576353a2773..34191e34bfdc 100644 --- a/include/net/bluetooth/bluetooth.h +++ b/include/net/bluetooth/bluetooth.h @@ -139,6 +139,17 @@ 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_RETRANS 0x01 +#define BT_MODE_FLOWCTL 0x02 +#define BT_MODE_ERTM 0x03 +#define BT_MODE_STREAMING 0x04 +#define BT_MODE_EXT_FLOWCTL 0x05 + +#define BT_MODE_LE_FLOWCTL 0x80 + __printf(1, 2) void bt_info(const char *fmt, ...); __printf(1, 2) diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c index 117ba20ea194..f2bb376c699f 100644 --- a/net/bluetooth/l2cap_sock.c +++ b/net/bluetooth/l2cap_sock.c @@ -500,6 +500,25 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, return err; } +static u8 l2cap_get_mode(struct l2cap_chan *chan) +{ + switch (chan->mode) { + case L2CAP_MODE_BASIC: + case L2CAP_MODE_RETRANS: + case L2CAP_MODE_FLOWCTL: + case L2CAP_MODE_ERTM: + case L2CAP_MODE_STREAMING: + /* Mode above are the same on both old and new sockopt */ + return chan->mode; + case L2CAP_MODE_LE_FLOWCTL: + return BT_MODE_FLOWCTL; + case L2CAP_MODE_EXT_FLOWCTL: + return BT_MODE_EXT_FLOWCTL; + } + + return chan->mode; +} + static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, char __user *optval, int __user *optlen) { @@ -508,6 +527,7 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, struct bt_security sec; struct bt_power pwr; u32 phys; + u8 mode; int len, err = 0; BT_DBG("sk %p", sk); @@ -624,6 +644,23 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, err = -EFAULT; break; + case BT_MODE: + if (!enable_ecred) { + err = -ENOPROTOOPT; + break; + } + + if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) { + err = -EINVAL; + break; + } + + mode = l2cap_get_mode(chan); + + if (put_user(mode, (u8 __user *) optval)) + err = -EFAULT; + break; + default: err = -ENOPROTOOPT; break; @@ -763,6 +800,36 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, return err; } +static int l2cap_set_mode(struct l2cap_chan *chan, u8 mode) +{ + switch (mode) { + case BT_MODE_BASIC: + clear_bit(CONF_STATE2_DEVICE, &chan->conf_state); + break; + case BT_MODE_FLOWCTL: + if (bdaddr_type_is_le(chan->src_type)) + mode = L2CAP_MODE_LE_FLOWCTL; + break; + case BT_MODE_EXT_FLOWCTL: + /* TODO: Add support for ECRED PDUs to BR/EDR */ + if (!bdaddr_type_is_le(chan->src_type)) + return -EINVAL; + mode = L2CAP_MODE_EXT_FLOWCTL; + break; + case BT_MODE_ERTM: + case BT_MODE_STREAMING: + if (!disable_ertm || bdaddr_type_is_le(chan->src_type)) + break; + /* fall through */ + default: + return -EINVAL; + } + + chan->mode = mode; + + return 0; +} + static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, char __user *optval, unsigned int optlen) { @@ -968,6 +1035,35 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, break; + case BT_MODE: + if (!enable_ecred) { + err = -ENOPROTOOPT; + break; + } + + if (sk->sk_state != BT_BOUND) { + err = -EINVAL; + break; + } + + if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) { + err = -EINVAL; + break; + } + + if (get_user(opt, (u8 __user *) optval)) { + err = -EFAULT; + break; + } + + err = l2cap_set_mode(chan, opt); + if (err) + break; + + BT_DBG("mode 0x%2.2x", chan->mode); + + break; + default: err = -ENOPROTOOPT; break;