Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4576-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 13 May 2024 15:43:32 -0700
From: Jakub Kicinski <kuba@kernel.org>
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
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>
Subject: Re: pull request: bluetooth-next 2024-05-10
Message-ID: <20240513154332.16e4e259@kernel.org>
In-Reply-To: <CABBYNZKn5YBRjj+RT_TVDtjOBS6V_H7BQmFMufQj-cOTC=RXDA@mail.gmail.com>
References: <20240510211431.1728667-1-luiz.dentz@gmail.com>
	<20240513142641.0d721b18@kernel.org>
	<CABBYNZKn5YBRjj+RT_TVDtjOBS6V_H7BQmFMufQj-cOTC=RXDA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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.

> >    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/141f66411ca488e26bdd64e6f858ffa190395d23
> 
> >    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 does
> >    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?

> > Would be great to get more info and/or second opinion, because it's not
> > sufficiently "obviously right" to me to pull right away :(  
> 
> Well I assumed sockopt starting with SO_ sort of means it applies that
> all socket families, in fact SO_TIMESTAMP already seem to work without
> these changes they just don't generate anything, so in a way we are
> just implementing a missing feature.