Date: Mon, 13 May 2024 22:12:04 -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 Jakub,

On Mon, May 13, 2024 at 10:01 PM Luiz Augusto von Dentz
<luiz.dentz@...il.com> wrote:
>
> 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.

@Jakub Kicinski I'm fine removing these from the pull request, or if
you want to do it yourself, in order not to miss the merge window,
then we can discuss it better and even put you and Willem on CC to
review the upcoming changes.

-- 
Luiz Augusto von Dentz

Powered by blists - more mailing lists