| Message ID | 20200813164059.v1.2.I03247d3813c6dcbcdbeab26d068f9fd765edb1f5@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | To support the HFP WBS, a chip vendor may choose a particular | expand |
Hi Joseph, On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > It is desirable to expose the wideband speech packet length via > a socket option to the user space so that the user space can set > the value correctly in configuring the sco connection. > > Reviewed-by: Alain Michaud <alainm@chromium.org> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > --- > > include/net/bluetooth/bluetooth.h | 2 ++ > net/bluetooth/sco.c | 8 ++++++++ > 2 files changed, 10 insertions(+) > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > index 9125effbf4483d..922cc03143def4 100644 > --- a/include/net/bluetooth/bluetooth.h > +++ b/include/net/bluetooth/bluetooth.h > @@ -153,6 +153,8 @@ struct bt_voice { > > #define BT_SCM_PKT_STATUS 0x03 > > +#define BT_SCO_PKT_LEN 17 > + > __printf(1, 2) > void bt_info(const char *fmt, ...); > __printf(1, 2) > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > index dcf7f96ff417e6..97e4e7c7b8cf62 100644 > --- a/net/bluetooth/sco.c > +++ b/net/bluetooth/sco.c > @@ -67,6 +67,7 @@ struct sco_pinfo { > __u32 flags; > __u16 setting; > __u8 cmsg_mask; > + __u32 pkt_len; > struct sco_conn *conn; > }; > > @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) > sco_sock_set_timer(sk, sk->sk_sndtimeo); > } > > + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; > + > done: > hci_dev_unlock(hdev); > hci_dev_put(hdev); > @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, > err = -EFAULT; > break; > > + case BT_SCO_PKT_LEN: > + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) > + err = -EFAULT; > + break; Couldn't we expose this via BT_SNDMTU/BT_RCVMTU? > default: > err = -ENOPROTOOPT; > break; > -- > 2.28.0.236.gb10cc79966-goog >
On Friday 14 August 2020 12:56:05 Luiz Augusto von Dentz wrote: > Hi Joseph, > > On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > > > It is desirable to expose the wideband speech packet length via > > a socket option to the user space so that the user space can set > > the value correctly in configuring the sco connection. > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > > --- > > > > include/net/bluetooth/bluetooth.h | 2 ++ > > net/bluetooth/sco.c | 8 ++++++++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > > index 9125effbf4483d..922cc03143def4 100644 > > --- a/include/net/bluetooth/bluetooth.h > > +++ b/include/net/bluetooth/bluetooth.h > > @@ -153,6 +153,8 @@ struct bt_voice { > > > > #define BT_SCM_PKT_STATUS 0x03 > > > > +#define BT_SCO_PKT_LEN 17 > > + > > __printf(1, 2) > > void bt_info(const char *fmt, ...); > > __printf(1, 2) > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > index dcf7f96ff417e6..97e4e7c7b8cf62 100644 > > --- a/net/bluetooth/sco.c > > +++ b/net/bluetooth/sco.c > > @@ -67,6 +67,7 @@ struct sco_pinfo { > > __u32 flags; > > __u16 setting; > > __u8 cmsg_mask; > > + __u32 pkt_len; > > struct sco_conn *conn; > > }; > > > > @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) > > sco_sock_set_timer(sk, sk->sk_sndtimeo); > > } > > > > + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; > > + > > done: > > hci_dev_unlock(hdev); > > hci_dev_put(hdev); > > @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, > > err = -EFAULT; > > break; > > > > + case BT_SCO_PKT_LEN: > > + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) > > + err = -EFAULT; > > + break; > > Couldn't we expose this via BT_SNDMTU/BT_RCVMTU? Hello! There is already SCO_OPTIONS sock option, uses struct sco_options and contains 'mtu' member. I think that instead of adding new sock option, existing SCO_OPTIONS option should be used. > > default: > > err = -ENOPROTOOPT; > > break; > > -- > > 2.28.0.236.gb10cc79966-goog > > > > > -- > Luiz Augusto von Dentz
Hi Pali, On Wed, Aug 19, 2020 at 7:37 AM Pali Rohár <pali@kernel.org> wrote: > > On Friday 14 August 2020 12:56:05 Luiz Augusto von Dentz wrote: > > Hi Joseph, > > > > On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > > > > > It is desirable to expose the wideband speech packet length via > > > a socket option to the user space so that the user space can set > > > the value correctly in configuring the sco connection. > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > > > --- > > > > > > include/net/bluetooth/bluetooth.h | 2 ++ > > > net/bluetooth/sco.c | 8 ++++++++ > > > 2 files changed, 10 insertions(+) > > > > > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > > > index 9125effbf4483d..922cc03143def4 100644 > > > --- a/include/net/bluetooth/bluetooth.h > > > +++ b/include/net/bluetooth/bluetooth.h > > > @@ -153,6 +153,8 @@ struct bt_voice { > > > > > > #define BT_SCM_PKT_STATUS 0x03 > > > > > > +#define BT_SCO_PKT_LEN 17 > > > + > > > __printf(1, 2) > > > void bt_info(const char *fmt, ...); > > > __printf(1, 2) > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > > index dcf7f96ff417e6..97e4e7c7b8cf62 100644 > > > --- a/net/bluetooth/sco.c > > > +++ b/net/bluetooth/sco.c > > > @@ -67,6 +67,7 @@ struct sco_pinfo { > > > __u32 flags; > > > __u16 setting; > > > __u8 cmsg_mask; > > > + __u32 pkt_len; > > > struct sco_conn *conn; > > > }; > > > > > > @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) > > > sco_sock_set_timer(sk, sk->sk_sndtimeo); > > > } > > > > > > + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; > > > + > > > done: > > > hci_dev_unlock(hdev); > > > hci_dev_put(hdev); > > > @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, > > > err = -EFAULT; > > > break; > > > > > > + case BT_SCO_PKT_LEN: > > > + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) > > > + err = -EFAULT; > > > + break; > > > > Couldn't we expose this via BT_SNDMTU/BT_RCVMTU? > > Hello! > > There is already SCO_OPTIONS sock option, uses struct sco_options and > contains 'mtu' member. > > I think that instead of adding new sock option, existing SCO_OPTIONS > option should be used. We are moving away from type specific options to so options like BT_SNDMTU/BT_RCVMTU should be supported in all socket types. > > > > default: > > > err = -ENOPROTOOPT; > > > break; > > > -- > > > 2.28.0.236.gb10cc79966-goog > > > > > > > > > -- > > Luiz Augusto von Dentz
On Wednesday 19 August 2020 11:21:00 Luiz Augusto von Dentz wrote: > Hi Pali, > > On Wed, Aug 19, 2020 at 7:37 AM Pali Rohár <pali@kernel.org> wrote: > > > > On Friday 14 August 2020 12:56:05 Luiz Augusto von Dentz wrote: > > > Hi Joseph, > > > > > > On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > > > > > > > It is desirable to expose the wideband speech packet length via > > > > a socket option to the user space so that the user space can set > > > > the value correctly in configuring the sco connection. > > > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > > > > --- > > > > > > > > include/net/bluetooth/bluetooth.h | 2 ++ > > > > net/bluetooth/sco.c | 8 ++++++++ > > > > 2 files changed, 10 insertions(+) > > > > > > > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > > > > index 9125effbf4483d..922cc03143def4 100644 > > > > --- a/include/net/bluetooth/bluetooth.h > > > > +++ b/include/net/bluetooth/bluetooth.h > > > > @@ -153,6 +153,8 @@ struct bt_voice { > > > > > > > > #define BT_SCM_PKT_STATUS 0x03 > > > > > > > > +#define BT_SCO_PKT_LEN 17 > > > > + > > > > __printf(1, 2) > > > > void bt_info(const char *fmt, ...); > > > > __printf(1, 2) > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > > > index dcf7f96ff417e6..97e4e7c7b8cf62 100644 > > > > --- a/net/bluetooth/sco.c > > > > +++ b/net/bluetooth/sco.c > > > > @@ -67,6 +67,7 @@ struct sco_pinfo { > > > > __u32 flags; > > > > __u16 setting; > > > > __u8 cmsg_mask; > > > > + __u32 pkt_len; > > > > struct sco_conn *conn; > > > > }; > > > > > > > > @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) > > > > sco_sock_set_timer(sk, sk->sk_sndtimeo); > > > > } > > > > > > > > + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; > > > > + > > > > done: > > > > hci_dev_unlock(hdev); > > > > hci_dev_put(hdev); > > > > @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, > > > > err = -EFAULT; > > > > break; > > > > > > > > + case BT_SCO_PKT_LEN: > > > > + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) > > > > + err = -EFAULT; > > > > + break; > > > > > > Couldn't we expose this via BT_SNDMTU/BT_RCVMTU? > > > > Hello! > > > > There is already SCO_OPTIONS sock option, uses struct sco_options and > > contains 'mtu' member. > > > > I think that instead of adding new sock option, existing SCO_OPTIONS > > option should be used. > > We are moving away from type specific options to so options like > BT_SNDMTU/BT_RCVMTU should be supported in all socket types. Yes, this make sense. But I guess that SCO_OPTIONS should be provided for backward compatibility as it is already used by lot of userspace applications. So for me it looks like that BT_SNDMTU/BT_RCVMTU should return same value as SCO_OPTIONS. > > > > > > default: > > > > err = -ENOPROTOOPT; > > > > break; > > > > -- > > > > 2.28.0.236.gb10cc79966-goog > > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz
On Wed, Aug 19, 2020 at 11:23 AM Pali Rohár <pali@kernel.org> wrote: > > On Wednesday 19 August 2020 11:21:00 Luiz Augusto von Dentz wrote: > > Hi Pali, > > > > On Wed, Aug 19, 2020 at 7:37 AM Pali Rohár <pali@kernel.org> wrote: > > > > > > On Friday 14 August 2020 12:56:05 Luiz Augusto von Dentz wrote: > > > > Hi Joseph, > > > > > > > > On Thu, Aug 13, 2020 at 1:42 AM Joseph Hwang <josephsih@chromium.org> wrote: > > > > > > > > > > It is desirable to expose the wideband speech packet length via > > > > > a socket option to the user space so that the user space can set > > > > > the value correctly in configuring the sco connection. > > > > > > > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > > > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > > > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > > > > > --- > > > > > > > > > > include/net/bluetooth/bluetooth.h | 2 ++ > > > > > net/bluetooth/sco.c | 8 ++++++++ > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > > > > > index 9125effbf4483d..922cc03143def4 100644 > > > > > --- a/include/net/bluetooth/bluetooth.h > > > > > +++ b/include/net/bluetooth/bluetooth.h > > > > > @@ -153,6 +153,8 @@ struct bt_voice { > > > > > > > > > > #define BT_SCM_PKT_STATUS 0x03 > > > > > > > > > > +#define BT_SCO_PKT_LEN 17 > > > > > + > > > > > __printf(1, 2) > > > > > void bt_info(const char *fmt, ...); > > > > > __printf(1, 2) > > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > > > > index dcf7f96ff417e6..97e4e7c7b8cf62 100644 > > > > > --- a/net/bluetooth/sco.c > > > > > +++ b/net/bluetooth/sco.c > > > > > @@ -67,6 +67,7 @@ struct sco_pinfo { > > > > > __u32 flags; > > > > > __u16 setting; > > > > > __u8 cmsg_mask; > > > > > + __u32 pkt_len; > > > > > struct sco_conn *conn; > > > > > }; > > > > > > > > > > @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) > > > > > sco_sock_set_timer(sk, sk->sk_sndtimeo); > > > > > } > > > > > > > > > > + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; > > > > > + > > > > > done: > > > > > hci_dev_unlock(hdev); > > > > > hci_dev_put(hdev); > > > > > @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, > > > > > err = -EFAULT; > > > > > break; > > > > > > > > > > + case BT_SCO_PKT_LEN: > > > > > + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) > > > > > + err = -EFAULT; > > > > > + break; > > > > > > > > Couldn't we expose this via BT_SNDMTU/BT_RCVMTU? > > > > > > Hello! > > > > > > There is already SCO_OPTIONS sock option, uses struct sco_options and > > > contains 'mtu' member. > > > > > > I think that instead of adding new sock option, existing SCO_OPTIONS > > > option should be used. > > > > We are moving away from type specific options to so options like > > BT_SNDMTU/BT_RCVMTU should be supported in all socket types. > > Yes, this make sense. > > But I guess that SCO_OPTIONS should be provided for backward > compatibility as it is already used by lot of userspace applications. > > So for me it looks like that BT_SNDMTU/BT_RCVMTU should return same > value as SCO_OPTIONS. Yep, luckily we can do this here because SCO MTU is symmetric. > > > > > > > > default: > > > > > err = -ENOPROTOOPT; > > > > > break; > > > > > -- > > > > > 2.28.0.236.gb10cc79966-goog > > > > > > > > > > > > > > > > > -- > > > > Luiz Augusto von Dentz > > > > > > > > -- > > Luiz Augusto von Dentz
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h index 9125effbf4483d..922cc03143def4 100644 --- a/include/net/bluetooth/bluetooth.h +++ b/include/net/bluetooth/bluetooth.h @@ -153,6 +153,8 @@ struct bt_voice { #define BT_SCM_PKT_STATUS 0x03 +#define BT_SCO_PKT_LEN 17 + __printf(1, 2) void bt_info(const char *fmt, ...); __printf(1, 2) diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index dcf7f96ff417e6..97e4e7c7b8cf62 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -67,6 +67,7 @@ struct sco_pinfo { __u32 flags; __u16 setting; __u8 cmsg_mask; + __u32 pkt_len; struct sco_conn *conn; }; @@ -267,6 +268,8 @@ static int sco_connect(struct sock *sk) sco_sock_set_timer(sk, sk->sk_sndtimeo); } + sco_pi(sk)->pkt_len = hdev->sco_pkt_len; + done: hci_dev_unlock(hdev); hci_dev_put(hdev); @@ -1001,6 +1004,11 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, err = -EFAULT; break; + case BT_SCO_PKT_LEN: + if (put_user(sco_pi(sk)->pkt_len, (u32 __user *)optval)) + err = -EFAULT; + break; + default: err = -ENOPROTOOPT; break;