Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2542-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1710440392.git.pav@iki.fi> <134027f3cbaeb7095d080c27cd4b1053d2eb560e.1710440392.git.pav@iki.fi>
In-Reply-To: <134027f3cbaeb7095d080c27cd4b1053d2eb560e.1710440392.git.pav@iki.fi>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Thu, 14 Mar 2024 23:18:32 -0400
Message-ID: <CABBYNZLWbwTGOuZeYvFou=+98_XNdYVu-hWiqrUFvZvuU50_yw@mail.gmail.com>
Subject: Re: [PATCH v2 5/5] Bluetooth: add BT_NO_ERRQUEUE_POLL socket option
To: Pauli Virtanen <pav@iki.fi>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Pauli,

On Thu, Mar 14, 2024 at 2:23=E2=80=AFPM Pauli Virtanen <pav@iki.fi> wrote:
>
> Add socket option to disable POLLERR on non-empty socket error queue.
>
> Applications can use this for sleeping socket error POLLERR waits, also
> when using TX timestamping. This is useful when multiple layers of the
> stack are using the same socket.

So the idea here is that bluetoothd would set BT_NO_ERRQUEUE_POLL to
not be awakened by timestamps? I wonder why this works on per
fd/socket level while SO_TIMESTAMPING does apply to all sockets
though, or perhaps that is not really related to SO_TIMESTAMPING but
errqueue being the same to all cloned sockets? I'm a little hesitant
to start introducing new socket options if the whole point was to
avoid doing so with the use of SO_TIMESTAMPING.


> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>  include/net/bluetooth/bluetooth.h |  9 +++-
>  net/bluetooth/af_bluetooth.c      | 72 ++++++++++++++++++++++++++++++-
>  net/bluetooth/iso.c               |  8 ++--
>  net/bluetooth/l2cap_sock.c        |  8 ++--
>  net/bluetooth/sco.c               |  8 ++--
>  5 files changed, 91 insertions(+), 14 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bl=
uetooth.h
> index 9280e72093ee..dcee5384d715 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -240,6 +240,8 @@ struct bt_codecs {
>
>  #define BT_ISO_BASE            20
>
> +#define BT_NO_ERRQUEUE_POLL    21
> +
>  __printf(1, 2)
>  void bt_info(const char *fmt, ...);
>  __printf(1, 2)
> @@ -387,7 +389,8 @@ struct bt_sock {
>  enum {
>         BT_SK_DEFER_SETUP,
>         BT_SK_SUSPEND,
> -       BT_SK_PKT_STATUS
> +       BT_SK_PKT_STATUS,
> +       BT_SK_NO_ERRQUEUE_POLL
>  };
>
>  struct bt_sock_list {
> @@ -410,6 +413,10 @@ int  bt_sock_stream_recvmsg(struct socket *sock, str=
uct msghdr *msg,
>                             size_t len, int flags);
>  __poll_t bt_sock_poll(struct file *file, struct socket *sock, poll_table=
 *wait);
>  int  bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long =
arg);
> +int bt_sock_setsockopt(struct socket *sock, int level, int optname,
> +                      sockptr_t optval, unsigned int optlen);
> +int bt_sock_getsockopt(struct socket *sock, int level, int optname,
> +                      char __user *optval, int __user *optlen);
>  int  bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)=
;
>  int  bt_sock_wait_ready(struct sock *sk, unsigned int msg_flags);
>
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 67604ccec2f4..997197aa7b48 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -500,6 +500,12 @@ static inline __poll_t bt_accept_poll(struct sock *p=
arent)
>         return 0;
>  }
>
> +static bool bt_sock_error_queue_poll(struct sock *sk)
> +{
> +       return !test_bit(BT_SK_NO_ERRQUEUE_POLL, &bt_sk(sk)->flags) &&
> +               !skb_queue_empty_lockless(&sk->sk_error_queue);
> +}
> +
>  __poll_t bt_sock_poll(struct file *file, struct socket *sock,
>                       poll_table *wait)
>  {
> @@ -511,7 +517,7 @@ __poll_t bt_sock_poll(struct file *file, struct socke=
t *sock,
>         if (sk->sk_state =3D=3D BT_LISTEN)
>                 return bt_accept_poll(sk);
>
> -       if (sk->sk_err || !skb_queue_empty_lockless(&sk->sk_error_queue))
> +       if (sk->sk_err || bt_sock_error_queue_poll(sk))
>                 mask |=3D EPOLLERR |
>                         (sock_flag(sk, SOCK_SELECT_ERR_QUEUE) ? EPOLLPRI =
: 0);
>
> @@ -582,6 +588,70 @@ int bt_sock_ioctl(struct socket *sock, unsigned int =
cmd, unsigned long arg)
>  }
>  EXPORT_SYMBOL(bt_sock_ioctl);
>
> +int bt_sock_setsockopt(struct socket *sock, int level, int optname,
> +                      sockptr_t optval, unsigned int optlen)
> +{
> +       struct sock *sk =3D sock->sk;
> +       int err =3D 0;
> +       u32 opt;
> +
> +       if (level !=3D SOL_BLUETOOTH)
> +               return -ENOPROTOOPT;
> +
> +       lock_sock(sk);
> +
> +       switch (optname) {
> +       case BT_NO_ERRQUEUE_POLL:
> +               if (copy_from_sockptr(&opt, optval, sizeof(opt))) {
> +                       err =3D -EFAULT;
> +                       break;
> +               }
> +
> +               if (opt)
> +                       set_bit(BT_SK_NO_ERRQUEUE_POLL, &bt_sk(sk)->flags=
);
> +               else
> +                       clear_bit(BT_SK_NO_ERRQUEUE_POLL, &bt_sk(sk)->fla=
gs);
> +               break;
> +
> +       default:
> +               err =3D -ENOPROTOOPT;
> +               break;
> +       }
> +
> +       release_sock(sk);
> +       return err;
> +}
> +EXPORT_SYMBOL(bt_sock_setsockopt);
> +
> +int bt_sock_getsockopt(struct socket *sock, int level, int optname,
> +                      char __user *optval, int __user *optlen)
> +{
> +       struct sock *sk =3D sock->sk;
> +       int err =3D 0;
> +       u32 opt;
> +
> +       if (level !=3D SOL_BLUETOOTH)
> +               return -ENOPROTOOPT;
> +
> +       lock_sock(sk);
> +
> +       switch (optname) {
> +       case BT_NO_ERRQUEUE_POLL:
> +               opt =3D test_bit(BT_SK_NO_ERRQUEUE_POLL, &bt_sk(sk)->flag=
s);
> +               if (put_user(opt, (u32 __user *)optval))
> +                       err =3D -EFAULT;
> +               break;
> +
> +       default:
> +               err =3D -ENOPROTOOPT;
> +               break;
> +       }
> +
> +       release_sock(sk);
> +       return err;
> +}
> +EXPORT_SYMBOL(bt_sock_getsockopt);
> +
>  /* This function expects the sk lock to be held when called */
>  int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
>  {
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index a77ab9df7994..6f5af549a8cc 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -1596,8 +1596,8 @@ static int iso_sock_setsockopt(struct socket *sock,=
 int level, int optname,
>                 break;
>
>         default:
> -               err =3D -ENOPROTOOPT;
> -               break;
> +               release_sock(sk);
> +               return bt_sock_setsockopt(sock, level, optname, optval, o=
ptlen);
>         }
>
>         release_sock(sk);
> @@ -1667,8 +1667,8 @@ static int iso_sock_getsockopt(struct socket *sock,=
 int level, int optname,
>                 break;
>
>         default:
> -               err =3D -ENOPROTOOPT;
> -               break;
> +               release_sock(sk);
> +               return bt_sock_getsockopt(sock, level, optname, optval, o=
ptlen);
>         }
>
>         release_sock(sk);
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 9a9f933a748b..06277ce1fd6b 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -697,8 +697,8 @@ static int l2cap_sock_getsockopt(struct socket *sock,=
 int level, int optname,
>                 break;
>
>         default:
> -               err =3D -ENOPROTOOPT;
> -               break;
> +               release_sock(sk);
> +               return bt_sock_getsockopt(sock, level, optname, optval, o=
ptlen);
>         }
>
>         release_sock(sk);
> @@ -1102,8 +1102,8 @@ static int l2cap_sock_setsockopt(struct socket *soc=
k, int level, int optname,
>                 break;
>
>         default:
> -               err =3D -ENOPROTOOPT;
> -               break;
> +               release_sock(sk);
> +               return bt_sock_setsockopt(sock, level, optname, optval, o=
ptlen);
>         }
>
>         release_sock(sk);
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index b3c2af7c7d67..b8b1b314aed4 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -968,8 +968,8 @@ static int sco_sock_setsockopt(struct socket *sock, i=
nt level, int optname,
>                 break;
>
>         default:
> -               err =3D -ENOPROTOOPT;
> -               break;
> +               release_sock(sk);
> +               return bt_sock_setsockopt(sock, level, optname, optval, o=
ptlen);
>         }
>
>         release_sock(sk);
> @@ -1211,8 +1211,8 @@ static int sco_sock_getsockopt(struct socket *sock,=
 int level, int optname,
>                 break;
>
>         default:
> -               err =3D -ENOPROTOOPT;
> -               break;
> +               release_sock(sk);
> +               return bt_sock_getsockopt(sock, level, optname, optval, o=
ptlen);
>         }
>
>         release_sock(sk);
> --
> 2.44.0
>
>


--=20
Luiz Augusto von Dentz