[PATCH v2] sco:Add support for BT_PKT_STATUS CMSG data

[PATCH v2] sco:Add support for BT_PKT_STATUS CMSG data

[Alain Michaud]

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>

---

 include/net/bluetooth/bluetooth.h |  8 ++++++++
 include/net/bluetooth/sco.h       |  3 +++
 net/bluetooth/af_bluetooth.c      |  3 +++
 net/bluetooth/hci_core.c          |  1 +
 net/bluetooth/sco.c               | 28 ++++++++++++++++++++++++++++
 5 files changed, 43 insertions(+)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 3fa7b1e3c5d9..85e6c5754448 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -147,6 +147,8 @@ struct bt_voice {
 #define BT_MODE_LE_FLOWCTL	0x03
 #define BT_MODE_EXT_FLOWCTL	0x04
 
+#define BT_PKT_STATUS          16
+
 __printf(1, 2)
 void bt_info(const char *fmt, ...);
 __printf(1, 2)
@@ -275,6 +277,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 +327,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 +357,7 @@ struct bt_skb_cb {
 	u8 incoming:1;
 	union {
 		struct l2cap_ctrl l2cap;
+		struct sco_ctrl sco;
 		struct hci_ctrl hci;
 	};
 };
diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
index f40ddb4264fc..7f0d7bdc53f6 100644
--- a/include/net/bluetooth/sco.h
+++ b/include/net/bluetooth/sco.h
@@ -46,4 +46,7 @@ struct sco_conninfo {
 	__u8  dev_class[3];
 };
 
+/* CMSG flags */
+#define SCO_CMSG_PKT_STATUS	0x0001
+
 #endif /* __SCO_H */
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..3d7b973e5b7b 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;
+	__u32           cmsg_mask;
 	struct sco_conn	*conn;
 };
 
@@ -449,6 +450,19 @@ 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)
+{
+	__u32 mask = sco_pi(sk)->cmsg_mask;
+
+	if (mask & SCO_CMSG_PKT_STATUS) {
+		int pkt_status = bt_cb(skb)->sco.pkt_status;
+
+		put_cmsg(msg, SOL_BLUETOOTH, BT_PKT_STATUS, sizeof(pkt_status),
+			 &pkt_status);
+	}
+}
+
 static void sco_sock_init(struct sock *sk, struct sock *parent)
 {
 	BT_DBG("sk %p", sk);
@@ -457,6 +471,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;
 	}
 }
 
@@ -846,6 +862,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(opt, (int __user *)optval)) {
+			err = -EFAULT;
+			break;
+		}
+
+		if (opt)
+			sco_pi(sk)->cmsg_mask |= SCO_CMSG_PKT_STATUS;
+		else
+			sco_pi(sk)->cmsg_mask &= ~SCO_CMSG_PKT_STATUS;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
-- 
2.27.0.rc0.183.gde8f92d652-goog

Re: [PATCH v2] sco:Add support for BT_PKT_STATUS CMSG data

[Marcel Holtmann]

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>
> 
> ---
> 
> include/net/bluetooth/bluetooth.h |  8 ++++++++
> include/net/bluetooth/sco.h       |  3 +++
> net/bluetooth/af_bluetooth.c      |  3 +++
> net/bluetooth/hci_core.c          |  1 +
> net/bluetooth/sco.c               | 28 ++++++++++++++++++++++++++++
> 5 files changed, 43 insertions(+)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 3fa7b1e3c5d9..85e6c5754448 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -147,6 +147,8 @@ struct bt_voice {
> #define BT_MODE_LE_FLOWCTL	0x03
> #define BT_MODE_EXT_FLOWCTL	0x04
> 
> +#define BT_PKT_STATUS          16
> +

we need to agree on if this is an int or u8 value. Otherwise this looks good.

Regards

Marcel

Re: [PATCH v2] sco:Add support for BT_PKT_STATUS CMSG data

[Alain Michaud]

On Thu, May 28, 2020 at 4:26 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> 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>
> >
> > ---
> >
> > include/net/bluetooth/bluetooth.h |  8 ++++++++
> > include/net/bluetooth/sco.h       |  3 +++
> > net/bluetooth/af_bluetooth.c      |  3 +++
> > net/bluetooth/hci_core.c          |  1 +
> > net/bluetooth/sco.c               | 28 ++++++++++++++++++++++++++++
> > 5 files changed, 43 insertions(+)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 3fa7b1e3c5d9..85e6c5754448 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -147,6 +147,8 @@ struct bt_voice {
> > #define BT_MODE_LE_FLOWCTL    0x03
> > #define BT_MODE_EXT_FLOWCTL   0x04
> >
> > +#define BT_PKT_STATUS          16
> > +
>
> we need to agree on if this is an int or u8 value. Otherwise this looks good.
Since doing anything beyond a u8 value would require a core spec
change and likely a new definition for what the packet flags are, I
think it is likely safe to keep as u8.  Do you have a different
perspective?
>
> Regards
>
> Marcel
>

[PATCH v2] sco:Add support for BT_PKT_STATUS CMSG data

[Alain Michaud]

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>

---

 include/net/bluetooth/bluetooth.h |  8 +++++++
 include/net/bluetooth/sco.h       |  3 +++
 net/bluetooth/af_bluetooth.c      |  3 +++
 net/bluetooth/hci_core.c          |  1 +
 net/bluetooth/sco.c               | 35 +++++++++++++++++++++++++++++++
 5 files changed, 50 insertions(+)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 3fa7b1e3c5d9..85e6c5754448 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -147,6 +147,8 @@ struct bt_voice {
 #define BT_MODE_LE_FLOWCTL	0x03
 #define BT_MODE_EXT_FLOWCTL	0x04
 
+#define BT_PKT_STATUS          16
+
 __printf(1, 2)
 void bt_info(const char *fmt, ...);
 __printf(1, 2)
@@ -275,6 +277,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 +327,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 +357,7 @@ struct bt_skb_cb {
 	u8 incoming:1;
 	union {
 		struct l2cap_ctrl l2cap;
+		struct sco_ctrl sco;
 		struct hci_ctrl hci;
 	};
 };
diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
index f40ddb4264fc..7f0d7bdc53f6 100644
--- a/include/net/bluetooth/sco.h
+++ b/include/net/bluetooth/sco.h
@@ -46,4 +46,7 @@ struct sco_conninfo {
 	__u8  dev_class[3];
 };
 
+/* CMSG flags */
+#define SCO_CMSG_PKT_STATUS	0x0001
+
 #endif /* __SCO_H */
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..f6b54853e547 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;
+	__u32           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 (sco_pi(sk)->cmsg_mask & SCO_CMSG_PKT_STATUS)
+		put_cmsg(msg, SOL_BLUETOOTH, BT_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;
 	}
 }
 
@@ -846,6 +858,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(opt, (u32 __user *)optval)) {
+			err = -EFAULT;
+			break;
+		}
+
+		if (opt)
+			sco_pi(sk)->cmsg_mask |= SCO_CMSG_PKT_STATUS;
+		else
+			sco_pi(sk)->cmsg_mask &= ~SCO_CMSG_PKT_STATUS;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
@@ -923,6 +947,7 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
 	int len, err = 0;
 	struct bt_voice voice;
 	u32 phys;
+	u32 pkt_status;
 
 	BT_DBG("sk %p", sk);
 
@@ -969,6 +994,16 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
 			err = -EFAULT;
 		break;
 
+	case BT_PKT_STATUS:
+		if (sco_pi(sk)->cmsg_mask & SCO_CMSG_PKT_STATUS)
+			pkt_status = 1;
+		else
+			pkt_status = 0;
+
+		if (put_user(pkt_status, (u32 __user *)optval))
+			err = -EFAULT;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
-- 
2.27.0.rc2.251.g90737beb825-goog

Re: [PATCH v2] sco:Add support for BT_PKT_STATUS CMSG data

[Marcel Holtmann]

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>
> 
> ---
> 
> include/net/bluetooth/bluetooth.h |  8 +++++++
> include/net/bluetooth/sco.h       |  3 +++
> net/bluetooth/af_bluetooth.c      |  3 +++
> net/bluetooth/hci_core.c          |  1 +
> net/bluetooth/sco.c               | 35 +++++++++++++++++++++++++++++++
> 5 files changed, 50 insertions(+)

I already had this patch in my tree for testing, but then realized that I gave you a bad example on how to handle this.

So we should not use the same constants for CMSG and socket options. Fundamentally the socket semantics use SO_foo for enabling an aux data transmission and they use SCM_bar for the CMSG portion.

> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 3fa7b1e3c5d9..85e6c5754448 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -147,6 +147,8 @@ struct bt_voice {
> #define BT_MODE_LE_FLOWCTL	0x03
> #define BT_MODE_EXT_FLOWCTL	0x04
> 
> +#define BT_PKT_STATUS          16
> +
> __printf(1, 2)
> void bt_info(const char *fmt, ...);
> __printf(1, 2)
> @@ -275,6 +277,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 +327,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 +357,7 @@ struct bt_skb_cb {
> 	u8 incoming:1;
> 	union {
> 		struct l2cap_ctrl l2cap;
> +		struct sco_ctrl sco;
> 		struct hci_ctrl hci;
> 	};
> };
> diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
> index f40ddb4264fc..7f0d7bdc53f6 100644
> --- a/include/net/bluetooth/sco.h
> +++ b/include/net/bluetooth/sco.h
> @@ -46,4 +46,7 @@ struct sco_conninfo {
> 	__u8  dev_class[3];
> };
> 
> +/* CMSG flags */
> +#define SCO_CMSG_PKT_STATUS	0x0001
> +
> #endif /* __SCO_H */
> 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..f6b54853e547 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;
> +	__u32           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 (sco_pi(sk)->cmsg_mask & SCO_CMSG_PKT_STATUS)
> +		put_cmsg(msg, SOL_BLUETOOTH, BT_PKT_STATUS,

This needs an extra BT_SCM_PKT_STATUS constant.

> +			 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;
> 	}
> }
> 
> @@ -846,6 +858,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(opt, (u32 __user *)optval)) {
> +			err = -EFAULT;
> +			break;
> +		}
> +
> +		if (opt)
> +			sco_pi(sk)->cmsg_mask |= SCO_CMSG_PKT_STATUS;
> +		else
> +			sco_pi(sk)->cmsg_mask &= ~SCO_CMSG_PKT_STATUS;
> +		break;
> +
> 	default:
> 		err = -ENOPROTOOPT;
> 		break;
> @@ -923,6 +947,7 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
> 	int len, err = 0;
> 	struct bt_voice voice;
> 	u32 phys;
> +	u32 pkt_status;

Lets follow the example of SO_TIMESTAMP and use int here and convert it to a bool meaning. Since the BT_PKT_STATUS really is just suppose to enable / disable sending the packet status.

> 
> 	BT_DBG("sk %p", sk);
> 
> @@ -969,6 +994,16 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
> 			err = -EFAULT;
> 		break;
> 
> +	case BT_PKT_STATUS:
> +		if (sco_pi(sk)->cmsg_mask & SCO_CMSG_PKT_STATUS)
> +			pkt_status = 1;
> +		else
> +			pkt_status = 0;
> +
> +		if (put_user(pkt_status, (u32 __user *)optval))
> +			err = -EFAULT;
> +		break;
> +
> 	default:
> 		err = -ENOPROTOOPT;
> 		break;

Regards

Marcel

Re: [PATCH v2] sco:Add support for BT_PKT_STATUS CMSG data

[Alain Michaud]

Resending in plaintext.


On Wed, Jun 10, 2020 at 9:33 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> Hi Marcel
>
> On Wed, Jun 10, 2020 at 4:25 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>>
>> 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>
>> >
>> > ---
>> >
>> > include/net/bluetooth/bluetooth.h |  8 +++++++
>> > include/net/bluetooth/sco.h       |  3 +++
>> > net/bluetooth/af_bluetooth.c      |  3 +++
>> > net/bluetooth/hci_core.c          |  1 +
>> > net/bluetooth/sco.c               | 35 +++++++++++++++++++++++++++++++
>> > 5 files changed, 50 insertions(+)
>>
>> I already had this patch in my tree for testing, but then realized that I gave you a bad example on how to handle this.
>>
>> So we should not use the same constants for CMSG and socket options. Fundamentally the socket semantics use SO_foo for enabling an aux data transmission and they use SCM_bar for the CMSG portion.
>>
>> >
>> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> > index 3fa7b1e3c5d9..85e6c5754448 100644
>> > --- a/include/net/bluetooth/bluetooth.h
>> > +++ b/include/net/bluetooth/bluetooth.h
>> > @@ -147,6 +147,8 @@ struct bt_voice {
>> > #define BT_MODE_LE_FLOWCTL    0x03
>> > #define BT_MODE_EXT_FLOWCTL   0x04
>> >
>> > +#define BT_PKT_STATUS          16
>> > +
>> > __printf(1, 2)
>> > void bt_info(const char *fmt, ...);
>> > __printf(1, 2)
>> > @@ -275,6 +277,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 +327,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 +357,7 @@ struct bt_skb_cb {
>> >       u8 incoming:1;
>> >       union {
>> >               struct l2cap_ctrl l2cap;
>> > +             struct sco_ctrl sco;
>> >               struct hci_ctrl hci;
>> >       };
>> > };
>> > diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
>> > index f40ddb4264fc..7f0d7bdc53f6 100644
>> > --- a/include/net/bluetooth/sco.h
>> > +++ b/include/net/bluetooth/sco.h
>> > @@ -46,4 +46,7 @@ struct sco_conninfo {
>> >       __u8  dev_class[3];
>> > };
>> >
>> > +/* CMSG flags */
>> > +#define SCO_CMSG_PKT_STATUS  0x0001
>> > +
>> > #endif /* __SCO_H */
>> > 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..f6b54853e547 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;
>> > +     __u32           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 (sco_pi(sk)->cmsg_mask & SCO_CMSG_PKT_STATUS)
>> > +             put_cmsg(msg, SOL_BLUETOOTH, BT_PKT_STATUS,
>>
>> This needs an extra BT_SCM_PKT_STATUS constant.
>
> Since we don't have this name space defined yet, I'm assuming I just start with 01 for this.
>>
>>
>> > +                      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;
>> >       }
>> > }
>> >
>> > @@ -846,6 +858,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(opt, (u32 __user *)optval)) {
>> > +                     err = -EFAULT;
>> > +                     break;
>> > +             }
>> > +
>> > +             if (opt)
>> > +                     sco_pi(sk)->cmsg_mask |= SCO_CMSG_PKT_STATUS;
>> > +             else
>> > +                     sco_pi(sk)->cmsg_mask &= ~SCO_CMSG_PKT_STATUS;
>> > +             break;
>> > +
>> >       default:
>> >               err = -ENOPROTOOPT;
>> >               break;
>> > @@ -923,6 +947,7 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
>> >       int len, err = 0;
>> >       struct bt_voice voice;
>> >       u32 phys;
>> > +     u32 pkt_status;
>>
>> Lets follow the example of SO_TIMESTAMP and use int here and convert it to a bool meaning. Since the BT_PKT_STATUS really is just suppose to enable / disable sending the packet status.
>
> I get that SO_TIMESTAMP uses a signed integer here, but I'm not quite sure what benefit it provides, especially when simply using it as a boolean value.
>>
>>
>> >
>> >       BT_DBG("sk %p", sk);
>> >
>> > @@ -969,6 +994,16 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
>> >                       err = -EFAULT;
>> >               break;
>> >
>> > +     case BT_PKT_STATUS:
>> > +             if (sco_pi(sk)->cmsg_mask & SCO_CMSG_PKT_STATUS)
>> > +                     pkt_status = 1;
>> > +             else
>> > +                     pkt_status = 0;
>> > +
>> > +             if (put_user(pkt_status, (u32 __user *)optval))
>> > +                     err = -EFAULT;
>> > +             break;
>> > +
>> >       default:
>> >               err = -ENOPROTOOPT;
>> >               break;
>>
>> Regards
>>
>> Marcel
>>

Re: [PATCH v2] sco:Add support for BT_PKT_STATUS CMSG data

[Marcel Holtmann]

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>
> > 
> > ---
> > 
> > include/net/bluetooth/bluetooth.h |  8 +++++++
> > include/net/bluetooth/sco.h       |  3 +++
> > net/bluetooth/af_bluetooth.c      |  3 +++
> > net/bluetooth/hci_core.c          |  1 +
> > net/bluetooth/sco.c               | 35 +++++++++++++++++++++++++++++++
> > 5 files changed, 50 insertions(+)
> 
> I already had this patch in my tree for testing, but then realized that I gave you a bad example on how to handle this.
> 
> So we should not use the same constants for CMSG and socket options. Fundamentally the socket semantics use SO_foo for enabling an aux data transmission and they use SCM_bar for the CMSG portion.
> 
> > 
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 3fa7b1e3c5d9..85e6c5754448 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -147,6 +147,8 @@ struct bt_voice {
> > #define BT_MODE_LE_FLOWCTL    0x03
> > #define BT_MODE_EXT_FLOWCTL   0x04
> > 
> > +#define BT_PKT_STATUS          16
> > +
> > __printf(1, 2)
> > void bt_info(const char *fmt, ...);
> > __printf(1, 2)
> > @@ -275,6 +277,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 +327,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 +357,7 @@ struct bt_skb_cb {
> >       u8 incoming:1;
> >       union {
> >               struct l2cap_ctrl l2cap;
> > +             struct sco_ctrl sco;
> >               struct hci_ctrl hci;
> >       };
> > };
> > diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
> > index f40ddb4264fc..7f0d7bdc53f6 100644
> > --- a/include/net/bluetooth/sco.h
> > +++ b/include/net/bluetooth/sco.h
> > @@ -46,4 +46,7 @@ struct sco_conninfo {
> >       __u8  dev_class[3];
> > };
> > 
> > +/* CMSG flags */
> > +#define SCO_CMSG_PKT_STATUS  0x0001
> > +
> > #endif /* __SCO_H */
> > 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..f6b54853e547 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;
> > +     __u32           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 (sco_pi(sk)->cmsg_mask & SCO_CMSG_PKT_STATUS)
> > +             put_cmsg(msg, SOL_BLUETOOTH, BT_PKT_STATUS,
> 
> This needs an extra BT_SCM_PKT_STATUS constant.
> Since we don't have this name space defined yet, I'm assuming I just start with 01 for this. 

hmmm. Since we used to have a few HCI ones, just for the sake of never ever conflicting, I would say start after them.

/* CMSG flags */                                                                 
#define HCI_CMSG_DIR    0x0001                                                   
#define HCI_CMSG_TSTAMP 0x0002

So lets start with 3.

I think even for the socket numbering options we started with 4 to just make sure we never ever come into some weird conflict. Some of this code is 18+ years old ;)

Regards

Marcel