Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4581-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240510211431.1728667-1-luiz.dentz@gmail.com>
 <20240513142641.0d721b18@kernel.org> <CABBYNZKn5YBRjj+RT_TVDtjOBS6V_H7BQmFMufQj-cOTC=RXDA@mail.gmail.com>
 <20240513154332.16e4e259@kernel.org>
In-Reply-To: <20240513154332.16e4e259@kernel.org>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 13 May 2024 21:34:36 -0400
Message-ID: <CABBYNZJqwGo6VmTsSHFveRLy7pYQ0PJ+4X_aaL9r5if0dyqf6A@mail.gmail.com>
Subject: Re: pull request: bluetooth-next 2024-05-10
To: Jakub Kicinski <kuba@kernel.org>
Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>, davem@davemloft.net, 
	linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org, 
	Pauli Virtanen <pav@iki.fi>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Jakub,

On Mon, May 13, 2024 at 6:43=E2=80=AFPM Jakub Kicinski <kuba@kernel.org> wr=
ote:
>
> On Mon, 13 May 2024 18:09:31 -0400 Luiz Augusto von Dentz wrote:
> > > There is one more warning in the Intel driver:
> > >
> > > drivers/bluetooth/btintel_pcie.c:673:33: warning: symbol 'causes_list=
'
> > > was not declared. Should it be static?
> >
> > We have a fix for that but I was hoping to have it in before the merge
> > window and then have the fix merged later.
> >
> > > It'd also be great to get an ACK from someone familiar with the socke=
t
> > > time stamping (Willem?) I'm not sure there's sufficient detail in the
> > > commit message to explain the choices to:
> > >  - change the definition of SCHED / SEND to mean queued / completed,
> > >    while for Ethernet they mean queued to qdisc, queued to HW.
> >
> > hmm I thought this was hardware specific, it obviously won't work
> > exactly as Ethernet since it is a completely different protocol stack,
> > or are you suggesting we need other definitions for things like TX
> > completed?
>
> I don't know anything about queuing in BT, in terms of timestamping
> the SEND - SCHED difference is supposed to indicate the level of
> host delay or host congestion. If the queuing in BT happens mostly in
> the device HW queue then it may make sense to generate SCHED when
> handing over to the driver. OTOH if the devices can coalesce or delay
> completions the completion timeout may be less accurate than stamping
> before submitting to HW... I'm looking for the analysis that the choices
> were well thought thru.

I guess you want to know if is SCHED is done at enqueing (before
submitting to the driver) or dequeing (after it has been submitted),
right now it is the former, the said the driver should normally just
submit the packets immediately since we do have events from HCI to
informing when a buffer has been freed, so that tells HW queue
situation, so the driver doesn't have any queueing.

> > >    How does it compare to stamping in the driver in terms of accuracy=
?
> >
> > @Pauli any input here?
> >
> > >  - the "experimental" BT_POLL_ERRQUEUE, how does the user space look?
> >
> > There are test cases in BlueZ:
> >
> > https://github.com/bluez/bluez/commit/141f66411ca488e26bdd64e6f858ffa19=
0395d23
> >
> > >    What is the "upper layer"? What does it mean for kernel uAPI to be
> > >    "experimental"? When does the "upper layer" get to run and how doe=
s
> > >    it know that there are time stamps on the error queue?
> >
> > The socketopt only gets enabled with use of MGMT Set Experimental
> > Feature Command:
> >
> > https://github.com/bluez/bluez/blob/master/doc/mgmt-api.txt#L3205
> >
> > Anyway you can see on the tests how we are using it.
>
> Either I didn't grok the test or it doesn't answer my question.
> What is the lower layer that we want to "protect" from POLLERR?

This is more or less explained on the cover letter:

https://lore.kernel.org/linux-bluetooth/713b1d0333eb2f12e63bc8a7b8f423e1240=
abae0.camel@iki.fi/T/

The problem with POLLERR is that there are 2 processes (bluetoothd and
pipewire) monitoring the same file descriptor, so it will keep waking
up both processes to process the errqueue while only one is really
reading those events (pipewire), bluetoothd is only really monitoring
the sockets to know if there is a disconnection but it can't really
process the errqueue otherwise pipewire would compete reading from the
same queue. Anyway I wasn't sure  this was the best approach thus we
decided to go with something experimental until we have a better
understanding how all of this should work.

--=20
Luiz Augusto von Dentz