diff mbox series

[v4] sco:Add support for BT_PKT_STATUS CMSG data

Message ID 20200610143122.15453-1-alainm@chromium.org (mailing list archive)
State Superseded
Headers show
Series [v4] sco:Add support for BT_PKT_STATUS CMSG data | expand

Commit Message

Alain Michaud June 10, 2020, 2:31 p.m. UTC 
This change adds support for reporting the BT_PKT_STATUS to the socket
CMSG data to allow the implementation of a packet loss correction on
erronous data received on the SCO socket.

The patch was partially developed by Marcel Holtmann and validated by
Hsin-yu Chao

Signed-off-by: Alain Michaud <alainm@chromium.org>

---

Changes in v4:
- Addressing feedback from Marcel

 include/net/bluetooth/bluetooth.h | 11 ++++++++++
 net/bluetooth/af_bluetooth.c      |  3 +++
 net/bluetooth/hci_core.c          |  1 +
 net/bluetooth/sco.c               | 34 +++++++++++++++++++++++++++++++
 4 files changed, 49 insertions(+)

Comments

Marcel Holtmann June 10, 2020, 3:43 p.m. UTC | #1 
Hi Alain,

> This change adds support for reporting the BT_PKT_STATUS to the socket
> CMSG data to allow the implementation of a packet loss correction on
> erronous data received on the SCO socket.
> 
> The patch was partially developed by Marcel Holtmann and validated by
> Hsin-yu Chao
> 
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> 
> ---
> 
> Changes in v4:
> - Addressing feedback from Marcel
> 
> include/net/bluetooth/bluetooth.h | 11 ++++++++++
> net/bluetooth/af_bluetooth.c      |  3 +++
> net/bluetooth/hci_core.c          |  1 +
> net/bluetooth/sco.c               | 34 +++++++++++++++++++++++++++++++
> 4 files changed, 49 insertions(+)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 3fa7b1e3c5d9..ff7258200efb 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -147,6 +147,11 @@ struct bt_voice {
> #define BT_MODE_LE_FLOWCTL	0x03
> #define BT_MODE_EXT_FLOWCTL	0x04
> 
> +#define BT_PKT_STATUS          16
> +
> +/* CMSG flags */
> +#define BT_CMSG_PKT_STATUS	0x0003
> +

I have the feeling that I confused you more than I made clear on how this should be done.

So the public available constant names should be BT_SCM_PKT_STATUS and we can keep that as u8 here for simplification and to not waste space for each SCO socket message.

> __printf(1, 2)
> void bt_info(const char *fmt, ...);
> __printf(1, 2)
> @@ -275,6 +280,7 @@ struct bt_sock {
> 	struct sock *parent;
> 	unsigned long flags;
> 	void (*skb_msg_name)(struct sk_buff *, void *, int *);
> +	void (*skb_put_cmsg)(struct sk_buff *, struct msghdr *, struct sock *);
> };
> 
> enum {
> @@ -324,6 +330,10 @@ struct l2cap_ctrl {
> 	struct l2cap_chan *chan;
> };
> 
> +struct sco_ctrl {
> +	u8	pkt_status;
> +};
> +
> struct hci_dev;
> 
> typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status, u16 opcode);
> @@ -350,6 +360,7 @@ struct bt_skb_cb {
> 	u8 incoming:1;
> 	union {
> 		struct l2cap_ctrl l2cap;
> +		struct sco_ctrl sco;
> 		struct hci_ctrl hci;
> 	};
> };
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 3fd124927d4d..d0abea8d08cc 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -286,6 +286,9 @@ int bt_sock_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> 		if (msg->msg_name && bt_sk(sk)->skb_msg_name)
> 			bt_sk(sk)->skb_msg_name(skb, msg->msg_name,
> 						&msg->msg_namelen);
> +
> +		if (bt_sk(sk)->skb_put_cmsg)
> +			bt_sk(sk)->skb_put_cmsg(skb, msg, sk);
> 	}
> 
> 	skb_free_datagram(sk, skb);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 51d399273276..7b5e46198d99 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -4549,6 +4549,7 @@ static void hci_scodata_packet(struct hci_dev *hdev, struct sk_buff *skb)
> 
> 	if (conn) {
> 		/* Send to upper protocol */
> +		bt_cb(skb)->sco.pkt_status = flags & 0x03;
> 		sco_recv_scodata(conn, skb);
> 		return;
> 	} else {
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index c8c3d38cdc7b..abcefa00ae11 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -66,6 +66,7 @@ struct sco_pinfo {
> 	bdaddr_t	dst;
> 	__u32		flags;
> 	__u16		setting;
> +	unsigned long	cmsg_mask;

I would keep this as __u32 cmsg_mask similar to what HCI has. And using SCO_CMSG_PKT_STATUS 0x0001 is fine here. This is just internal and it doesn’t make a difference what value is used.

Actually we could reduce this to __u8 cmsg_mask also for HCI since a) it is all internal and b) we don’t need the extra size right now.

> 	struct sco_conn	*conn;
> };
> 
> @@ -449,6 +450,15 @@ static void sco_sock_close(struct sock *sk)
> 	sco_sock_kill(sk);
> }
> 
> +static void sco_skb_put_cmsg(struct sk_buff *skb, struct msghdr *msg,
> +			     struct sock *sk)
> +{
> +	if (test_bit(BT_CMSG_PKT_STATUS, &sco_pi(sk)->cmsg_mask))
> +		put_cmsg(msg, SOL_BLUETOOTH, BT_CMSG_PKT_STATUS,
> +			 sizeof(bt_cb(skb)->sco.pkt_status),
> +			 &bt_cb(skb)->sco.pkt_status);
> +}
> +
> static void sco_sock_init(struct sock *sk, struct sock *parent)
> {
> 	BT_DBG("sk %p", sk);
> @@ -457,6 +467,8 @@ static void sco_sock_init(struct sock *sk, struct sock *parent)
> 		sk->sk_type = parent->sk_type;
> 		bt_sk(sk)->flags = bt_sk(parent)->flags;
> 		security_sk_clone(parent, sk);
> +	} else {
> +		bt_sk(sk)->skb_put_cmsg = sco_skb_put_cmsg;
> 	}
> }
> 
> @@ -797,6 +809,7 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
> 	int len, err = 0;
> 	struct bt_voice voice;
> 	u32 opt;
> +	int pkt_status;
> 
> 	BT_DBG("sk %p", sk);
> 
> @@ -846,6 +859,18 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
> 		sco_pi(sk)->setting = voice.setting;
> 		break;
> 
> +	case BT_PKT_STATUS:
> +		if (get_user(pkt_status, (int __user *)optval)) {
> +			err = -EFAULT;
> +			break;
> +		}
> +
> +		if (pkt_status)
> +			set_bit(BT_CMSG_PKT_STATUS, &sco_pi(sk)->cmsg_mask);
> +		else
> +			clear_bit(BT_CMSG_PKT_STATUS, &sco_pi(sk)->cmsg_mask);
> +		break;
> +
> 	default:
> 		err = -ENOPROTOOPT;
> 		break;
> @@ -923,6 +948,7 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
> 	int len, err = 0;
> 	struct bt_voice voice;
> 	u32 phys;
> +	int pkt_status;
> 
> 	BT_DBG("sk %p", sk);
> 
> @@ -969,6 +995,14 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
> 			err = -EFAULT;
> 		break;
> 
> +	case BT_PKT_STATUS:
> +		pkt_status = test_bit(BT_CMSG_PKT_STATUS,
> +				      &(sco_pi(sk)->cmsg_mask));
> +
> +		if (put_user(pkt_status, (int __user *)optval))
> +			err = -EFAULT;
> +		break;
> +
> 	default:
> 		err = -ENOPROTOOPT;
> 		break;

Regards

Marcel
kernel test robot June 11, 2020, 9:28 a.m. UTC | #2 
Hi Alain,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on next-20200611]
[cannot apply to bluetooth/master v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Alain-Michaud/sco-Add-support-for-BT_PKT_STATUS-CMSG-data/20200610-223512
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: sh-randconfig-s032-20200611 (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-250-g42323db3-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 ARCH=sh CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

   net/bluetooth/sco.c:826:21: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected unsigned int const *__gu_addr @@     got unsigned int [noderef] [usertype] <asn:1> * @@
   net/bluetooth/sco.c:826:21: sparse:     expected unsigned int const *__gu_addr
   net/bluetooth/sco.c:826:21: sparse:     got unsigned int [noderef] [usertype] <asn:1> *
   net/bluetooth/sco.c:826:21: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] <asn:1> * @@     got unsigned int const *__gu_addr @@
   net/bluetooth/sco.c:826:21: sparse:     expected void const volatile [noderef] <asn:1> *
   net/bluetooth/sco.c:826:21: sparse:     got unsigned int const *__gu_addr
>> net/bluetooth/sco.c:863:21: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int const *__gu_addr @@     got int [noderef] <asn:1> * @@
   net/bluetooth/sco.c:863:21: sparse:     expected int const *__gu_addr
>> net/bluetooth/sco.c:863:21: sparse:     got int [noderef] <asn:1> *
   net/bluetooth/sco.c:863:21: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] <asn:1> * @@     got int const *__gu_addr @@
   net/bluetooth/sco.c:863:21: sparse:     expected void const volatile [noderef] <asn:1> *
   net/bluetooth/sco.c:863:21: sparse:     got int const *__gu_addr
   net/bluetooth/sco.c:893:13: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int const *__gu_addr @@     got int [noderef] <asn:1> *optlen @@
   net/bluetooth/sco.c:893:13: sparse:     expected int const *__gu_addr
   net/bluetooth/sco.c:893:13: sparse:     got int [noderef] <asn:1> *optlen
   net/bluetooth/sco.c:893:13: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] <asn:1> * @@     got int const *__gu_addr @@
   net/bluetooth/sco.c:893:13: sparse:     expected void const volatile [noderef] <asn:1> *
   net/bluetooth/sco.c:893:13: sparse:     got int const *__gu_addr
   net/bluetooth/sco.c:958:13: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int const *__gu_addr @@     got int [noderef] <asn:1> *optlen @@
   net/bluetooth/sco.c:958:13: sparse:     expected int const *__gu_addr
   net/bluetooth/sco.c:958:13: sparse:     got int [noderef] <asn:1> *optlen
   net/bluetooth/sco.c:958:13: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] <asn:1> * @@     got int const *__gu_addr @@
   net/bluetooth/sco.c:958:13: sparse:     expected void const volatile [noderef] <asn:1> *
   net/bluetooth/sco.c:958:13: sparse:     got int const *__gu_addr

vim +863 net/bluetooth/sco.c

   804	
   805	static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
   806				       char __user *optval, unsigned int optlen)
   807	{
   808		struct sock *sk = sock->sk;
   809		int len, err = 0;
   810		struct bt_voice voice;
   811		u32 opt;
   812		int pkt_status;
   813	
   814		BT_DBG("sk %p", sk);
   815	
   816		lock_sock(sk);
   817	
   818		switch (optname) {
   819	
   820		case BT_DEFER_SETUP:
   821			if (sk->sk_state != BT_BOUND && sk->sk_state != BT_LISTEN) {
   822				err = -EINVAL;
   823				break;
   824			}
   825	
 > 826			if (get_user(opt, (u32 __user *) optval)) {
   827				err = -EFAULT;
   828				break;
   829			}
   830	
   831			if (opt)
   832				set_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
   833			else
   834				clear_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
   835			break;
   836	
   837		case BT_VOICE:
   838			if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND &&
   839			    sk->sk_state != BT_CONNECT2) {
   840				err = -EINVAL;
   841				break;
   842			}
   843	
   844			voice.setting = sco_pi(sk)->setting;
   845	
   846			len = min_t(unsigned int, sizeof(voice), optlen);
   847			if (copy_from_user((char *)&voice, optval, len)) {
   848				err = -EFAULT;
   849				break;
   850			}
   851	
   852			/* Explicitly check for these values */
   853			if (voice.setting != BT_VOICE_TRANSPARENT &&
   854			    voice.setting != BT_VOICE_CVSD_16BIT) {
   855				err = -EINVAL;
   856				break;
   857			}
   858	
   859			sco_pi(sk)->setting = voice.setting;
   860			break;
   861	
   862		case BT_PKT_STATUS:
 > 863			if (get_user(pkt_status, (int __user *)optval)) {
   864				err = -EFAULT;
   865				break;
   866			}
   867	
   868			if (pkt_status)
   869				set_bit(BT_CMSG_PKT_STATUS, &sco_pi(sk)->cmsg_mask);
   870			else
   871				clear_bit(BT_CMSG_PKT_STATUS, &sco_pi(sk)->cmsg_mask);
   872			break;
   873	
   874		default:
   875			err = -ENOPROTOOPT;
   876			break;
   877		}
   878	
   879		release_sock(sk);
   880		return err;
   881	}
   882	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 3fa7b1e3c5d9..ff7258200efb 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -147,6 +147,11 @@  struct bt_voice {
 #define BT_MODE_LE_FLOWCTL	0x03
 #define BT_MODE_EXT_FLOWCTL	0x04
 
+#define BT_PKT_STATUS          16
+
+/* CMSG flags */
+#define BT_CMSG_PKT_STATUS	0x0003
+
 __printf(1, 2)
 void bt_info(const char *fmt, ...);
 __printf(1, 2)
@@ -275,6 +280,7 @@  struct bt_sock {
 	struct sock *parent;
 	unsigned long flags;
 	void (*skb_msg_name)(struct sk_buff *, void *, int *);
+	void (*skb_put_cmsg)(struct sk_buff *, struct msghdr *, struct sock *);
 };
 
 enum {
@@ -324,6 +330,10 @@  struct l2cap_ctrl {
 	struct l2cap_chan *chan;
 };
 
+struct sco_ctrl {
+	u8	pkt_status;
+};
+
 struct hci_dev;
 
 typedef void (*hci_req_complete_t)(struct hci_dev *hdev, u8 status, u16 opcode);
@@ -350,6 +360,7 @@  struct bt_skb_cb {
 	u8 incoming:1;
 	union {
 		struct l2cap_ctrl l2cap;
+		struct sco_ctrl sco;
 		struct hci_ctrl hci;
 	};
 };
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 3fd124927d4d..d0abea8d08cc 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -286,6 +286,9 @@  int bt_sock_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
 		if (msg->msg_name && bt_sk(sk)->skb_msg_name)
 			bt_sk(sk)->skb_msg_name(skb, msg->msg_name,
 						&msg->msg_namelen);
+
+		if (bt_sk(sk)->skb_put_cmsg)
+			bt_sk(sk)->skb_put_cmsg(skb, msg, sk);
 	}
 
 	skb_free_datagram(sk, skb);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 51d399273276..7b5e46198d99 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -4549,6 +4549,7 @@  static void hci_scodata_packet(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (conn) {
 		/* Send to upper protocol */
+		bt_cb(skb)->sco.pkt_status = flags & 0x03;
 		sco_recv_scodata(conn, skb);
 		return;
 	} else {
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index c8c3d38cdc7b..abcefa00ae11 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -66,6 +66,7 @@  struct sco_pinfo {
 	bdaddr_t	dst;
 	__u32		flags;
 	__u16		setting;
+	unsigned long	cmsg_mask;
 	struct sco_conn	*conn;
 };
 
@@ -449,6 +450,15 @@  static void sco_sock_close(struct sock *sk)
 	sco_sock_kill(sk);
 }
 
+static void sco_skb_put_cmsg(struct sk_buff *skb, struct msghdr *msg,
+			     struct sock *sk)
+{
+	if (test_bit(BT_CMSG_PKT_STATUS, &sco_pi(sk)->cmsg_mask))
+		put_cmsg(msg, SOL_BLUETOOTH, BT_CMSG_PKT_STATUS,
+			 sizeof(bt_cb(skb)->sco.pkt_status),
+			 &bt_cb(skb)->sco.pkt_status);
+}
+
 static void sco_sock_init(struct sock *sk, struct sock *parent)
 {
 	BT_DBG("sk %p", sk);
@@ -457,6 +467,8 @@  static void sco_sock_init(struct sock *sk, struct sock *parent)
 		sk->sk_type = parent->sk_type;
 		bt_sk(sk)->flags = bt_sk(parent)->flags;
 		security_sk_clone(parent, sk);
+	} else {
+		bt_sk(sk)->skb_put_cmsg = sco_skb_put_cmsg;
 	}
 }
 
@@ -797,6 +809,7 @@  static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 	int len, err = 0;
 	struct bt_voice voice;
 	u32 opt;
+	int pkt_status;
 
 	BT_DBG("sk %p", sk);
 
@@ -846,6 +859,18 @@  static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 		sco_pi(sk)->setting = voice.setting;
 		break;
 
+	case BT_PKT_STATUS:
+		if (get_user(pkt_status, (int __user *)optval)) {
+			err = -EFAULT;
+			break;
+		}
+
+		if (pkt_status)
+			set_bit(BT_CMSG_PKT_STATUS, &sco_pi(sk)->cmsg_mask);
+		else
+			clear_bit(BT_CMSG_PKT_STATUS, &sco_pi(sk)->cmsg_mask);
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
@@ -923,6 +948,7 @@  static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
 	int len, err = 0;
 	struct bt_voice voice;
 	u32 phys;
+	int pkt_status;
 
 	BT_DBG("sk %p", sk);
 
@@ -969,6 +995,14 @@  static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
 			err = -EFAULT;
 		break;
 
+	case BT_PKT_STATUS:
+		pkt_status = test_bit(BT_CMSG_PKT_STATUS,
+				      &(sco_pi(sk)->cmsg_mask));
+
+		if (put_user(pkt_status, (int __user *)optval))
+			err = -EFAULT;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;