Message-ID: <CABBYNZKJSpQcY+k8pczPgNYEoF+OE6enZFE5=Qu_HeWDkcfZEg@mail.gmail.com>
Date: Mon, 13 May 2024 22:01:12 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net, linux-bluetooth@...r.kernel.org, 
	netdev@...r.kernel.org, Pauli Virtanen <pav@....fi>
Subject: Re: pull request: bluetooth-next 2024-05-10

Hi Willem,

On Mon, May 13, 2024 at 9:32 PM Willem de Bruijn
<willemdebruijn.kernel@...il.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 '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 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 / 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.
>
> 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, 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 the
> 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

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.

-- 
Luiz Augusto von Dentz

Powered by blists - more mailing lists