Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4585-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> <6642bf28469d6_203b4c294bc@willemb.c.googlers.com.notmuch>
 <CABBYNZKJSpQcY+k8pczPgNYEoF+OE6enZFE5=Qu_HeWDkcfZEg@mail.gmail.com> <6642c7f3427b5_20539c2949a@willemb.c.googlers.com.notmuch>
In-Reply-To: <6642c7f3427b5_20539c2949a@willemb.c.googlers.com.notmuch>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 13 May 2024 22:17:00 -0400
Message-ID: <CABBYNZKdbvyev+BV=CMGrzWPECJraP4OVJeysQYV=EFLKf_WVw@mail.gmail.com>
Subject: Re: pull request: bluetooth-next 2024-05-10
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>, 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 Willem,

On Mon, May 13, 2024 at 10:09=E2=80=AFPM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Luiz Augusto von Dentz wrote:
> > Hi Willem,
> >
> > On Mon, May 13, 2024 at 9:32=E2=80=AFPM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jakub Kicinski wrote:
> > > > 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 'cause=
s_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=
 socket
> > > > > > 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 / compl=
eted,
> > > > > >    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 s=
tack,
> > > > > or are you suggesting we need other definitions for things like T=
X
> > > > > 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 del=
ay
> > > > completions the completion timeout may be less accurate than stampi=
ng
> > > > before submitting to HW... I'm looking for the analysis that the ch=
oices
> > > > were well thought thru.
> > >
> > > SCM_TSTAMP_SND is taken before an skb is passed to the device.
> > > This matches request SOF_TIMESTAMPING_TX_SOFTWARE.
> > >
> > > A timestamp returned on transmit completion is requested as
> > > SOF_TIMESTAMPING_TX_HARDWARE. We do not have a type for a software
> > > timestamp taken at tx completion cleaning. If anything, I would think
> > > it would be a passes as a hardware timestamp.
> >
> > In that case I think we probably misinterpret it, at least I though
> > that TX_HARDWARE would really be a hardware generated timestamp using
> > it own clock
>
> It normally is. It is just read from the tx descriptor on completion.
>
> We really don't have a good model for a software timestamp taken at
> completion processing.
>
> It may be worthwhile more broadly, especially for devices that do not
> support true hardware timestamps.
>
> Perhaps we should add an SCM_TSTAMP_TXCOMPLETION for this case. And a
> new SOF_TIMESTAMPING option to go with it. Similar to what we did for
> SCM_STAMP_SCHED.
>
> > if you are saying that TX_HARDWARE is just marking the
> > TX completion of the packet at the host then we can definitely align
> > with the current exception, that said we do have a command to actually
> > read out the actual timestamp from the BT controller, that is usually
> > more precise since some of the connection do require usec precision
> > which is something that can get skew by the processing of HCI events
> > themselves, well I guess we use that if the controller supports it and
> > if it doesn't then we do based on the host timestamp when processing
> > the HCI event indicating the completion of the transmission.
> >
> > > Returning SCHED when queuing to a device and SND later on receiving
> > > completions seems like not following SO_TIMESTAMPING convention to me=
.
> > > But I don't fully know the HCI model.
> > >
> > > As for the "experimental" BT_POLL_ERRQUEUE. This is an addition to th=
e
> > > ABI, right? So immutable. Is it fair to call that experimental?
> >
> > I guess you are referring to the fact that sockopt ID reserved to
> > BT_POLL_ERRQUEUE cannot be reused anymore even if we drop its usage in
> > the future, yes that is correct, but we can actually return
> > ENOPROTOOPT as it current does:
> >
> >         if (!bt_poll_errqueue_enabled())
> >             return -ENOPROTOOPT
>
> I see. Once applications rely on a feature, it can be hard to actually
> deprecate. But in this case it may be possible.
>
> > Anyway I would be really happy to drop it so we don't have to worry
> > about it later.
> >
> > > It might be safer to only suppress the sk_error_report in
> > > sock_queue_err_skb. Or at least in bt_sock_poll to check the type of
> > > all outstanding errors and only suppress if all are timestamps.
> >
> > Or perhaps we could actually do that via poll/epoll directly? Not that
> > it would make it much simpler since the library tends to wrap the
> > usage of poll/epoll but POLLERR meaning both errors or errqueue events
> > is sort of the problem we are trying to figure out how to process them
> > separately.
>
> The process would still be awoken, of course. If bluetoothd can just
> be modified to ignore the reports, that would indeed be easiest from
> a kernel PoV.

@Pauli Virtanen tried that but apparently it would keep waking up the
process until the errqueue is fully read, maybe we are missing
something, or glib is not really doing a good job wrt to poll/epoll
handling.

--=20
Luiz Augusto von Dentz