pull request: bluetooth-next 2024-05-10

pull request: bluetooth-next 2024-05-10

[Luiz Augusto von Dentz]

The following changes since commit f8beae078c82abde57fed4a5be0bbc3579b59ad0:

  Merge tag 'gtp-24-05-07' of git://git.kernel.org/pub/scm/linux/kernel/git/pablo/gtp Pablo neira Ayuso says: (2024-05-10 13:59:27 +0100)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git tags/for-net-next-2024-05-10

for you to fetch changes up to 75f819bdf9cafb0f1458e24c05d24eec17b2f597:

  Bluetooth: btintel: Fix compiler warning for multi_v7_defconfig config (2024-05-10 17:04:15 -0400)

----------------------------------------------------------------
bluetooth-next pull request for net-next:

 - Add support MediaTek MT7921S SDIO
 - Various fixes for -Wflex-array-member-not-at-end and -Wfamnae
 - Add USB HW IDs for MT7921/MT7922/MT7925
 - Add support for Intel BlazarI and Filmore Peak2 (BE201)
 - Add initial support for Intel PCIe driver
 - Remove HCI_AMP support
 - Add TX timestamping support

----------------------------------------------------------------
Archie Pusaka (1):
      Bluetooth: Populate hci_set_hw_info for Intel and Realtek

Chen-Yu Tsai (1):
      dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

Dan Carpenter (1):
      Bluetooth: qca: Fix error code in qca_read_fw_build_info()

Gustavo A. R. Silva (6):
      Bluetooth: L2CAP: Avoid -Wflex-array-member-not-at-end warnings
      Bluetooth: hci_conn, hci_sync: Use __counted_by() to avoid -Wfamnae warnings
      Bluetooth: hci_conn: Use __counted_by() to avoid -Wfamnae warning
      Bluetooth: hci_conn: Use struct_size() in hci_le_big_create_sync()
      Bluetooth: hci_sync: Use cmd->num_cis instead of magic number
      Bluetooth: hci_conn: Use __counted_by() and avoid -Wfamnae warning

Hans de Goede (1):
      Bluetooth: hci_bcm: Limit bcm43455 baudrate to 2000000

Ian W MORRISON (1):
      Bluetooth: Add support for MediaTek MT7922 device

Iulia Tanasescu (2):
      Bluetooth: ISO: Make iso_get_sock_listen generic
      Bluetooth: ISO: Handle PA sync when no BIGInfo reports are generated

Jiande Lu (2):
      Bluetooth: btusb: Add USB HW IDs for MT7921/MT7922/MT7925
      Bluetooth: btusb: Sort usb_device_id table by the ID

Johan Hovold (3):
      Bluetooth: qca: drop bogus edl header checks
      Bluetooth: qca: drop bogus module version
      Bluetooth: qca: clean up defines

Kiran K (8):
      Bluetooth: btintel: Define macros for image types
      Bluetooth: btintel: Add support to download intermediate loader
      Bluetooth: btintel: Add support for BlazarI
      Bluetooth: btintel: Add support for Filmore Peak2 (BE201)
      Bluetooth: btintel: Export few static functions
      Bluetooth: btintel_pcie: Add *setup* function to download firmware
      Bluetooth: btintel_pcie: Fix compiler warnings
      Bluetooth: btintel: Fix compiler warning for multi_v7_defconfig config

Luiz Augusto von Dentz (3):
      Bluetooth: Add proper definitions for scan interval and window
      Bluetooth: hci_event: Set DISCOVERY_FINDING on SCAN_ENABLED
      Bluetooth: HCI: Remove HCI_AMP support

Mahesh Talewad (1):
      LE Create Connection command timeout increased to 20 secs

Marek Vasut (1):
      dt-bindings: net: broadcom-bluetooth: Add CYW43439 DT binding

Pauli Virtanen (5):
      Bluetooth: add support for skb TX timestamping
      Bluetooth: ISO: add TX timestamping
      Bluetooth: L2CAP: add TX timestamping
      Bluetooth: SCO: add TX timestamping
      Bluetooth: add experimental BT_POLL_ERRQUEUE socket option

Peter Tsao (1):
      Bluetooth: btusb: Fix the patch for MT7920 the affected to MT7921

Sebastian Urban (1):
      Bluetooth: compute LE flow credits based on recvbuf space

Sungwoo Kim (1):
      Bluetooth: L2CAP: Fix div-by-zero in l2cap_le_flowctl_init()

Tedd Ho-Jeong An (1):
      Bluetooth: btintel_pcie: Add support for PCIe transport

Uri Arev (2):
      Bluetooth: hci_intel: Fix multiple issues reported by checkpatch.pl
      Bluetooth: ath3k: Fix multiple issues reported by checkpatch.pl

Uwe Kleine-König (3):
      Bluetooth: btqcomsmd: Convert to platform remove callback returning void
      Bluetooth: hci_bcm: Convert to platform remove callback returning void
      Bluetooth: hci_intel: Convert to platform remove callback returning void

Zijun Hu (4):
      Bluetooth: btusb: Correct timeout macro argument used to receive control message
      Bluetooth: hci_conn: Remove a redundant check for HFP offload
      Bluetooth: Remove 3 repeated macro definitions
      Bluetooth: qca: Support downloading board id specific NVM for WCN7850

 .../net/bluetooth/mediatek,mt7921s-bluetooth.yaml  |   55 +
 .../bindings/net/broadcom-bluetooth.yaml           |   33 +-
 MAINTAINERS                                        |    1 +
 drivers/bluetooth/Kconfig                          |   11 +
 drivers/bluetooth/Makefile                         |    1 +
 drivers/bluetooth/ath3k.c                          |   25 +-
 drivers/bluetooth/btintel.c                        |   88 +-
 drivers/bluetooth/btintel.h                        |   51 +-
 drivers/bluetooth/btintel_pcie.c                   | 1358 ++++++++++++++++++++
 drivers/bluetooth/btintel_pcie.h                   |  430 +++++++
 drivers/bluetooth/btmrvl_main.c                    |    9 -
 drivers/bluetooth/btqca.c                          |   47 +-
 drivers/bluetooth/btqca.h                          |   60 +-
 drivers/bluetooth/btqcomsmd.c                      |    6 +-
 drivers/bluetooth/btrsi.c                          |    1 -
 drivers/bluetooth/btrtl.c                          |    7 +
 drivers/bluetooth/btsdio.c                         |    8 -
 drivers/bluetooth/btusb.c                          |   55 +-
 drivers/bluetooth/hci_bcm.c                        |    8 +-
 drivers/bluetooth/hci_bcm4377.c                    |    1 -
 drivers/bluetooth/hci_intel.c                      |   25 +-
 drivers/bluetooth/hci_ldisc.c                      |    6 -
 drivers/bluetooth/hci_serdev.c                     |    5 -
 drivers/bluetooth/hci_uart.h                       |    1 -
 drivers/bluetooth/hci_vhci.c                       |   10 +-
 drivers/bluetooth/virtio_bt.c                      |    2 -
 include/net/bluetooth/bluetooth.h                  |   15 +-
 include/net/bluetooth/hci.h                        |  136 +-
 include/net/bluetooth/hci_core.h                   |   80 +-
 include/net/bluetooth/l2cap.h                      |   36 +-
 include/uapi/linux/virtio_bt.h                     |    1 -
 net/bluetooth/6lowpan.c                            |    2 +-
 net/bluetooth/af_bluetooth.c                       |  119 +-
 net/bluetooth/hci_conn.c                           |  261 +++-
 net/bluetooth/hci_core.c                           |  178 +--
 net/bluetooth/hci_event.c                          |  244 +---
 net/bluetooth/hci_request.h                        |    4 -
 net/bluetooth/hci_sock.c                           |    5 +-
 net/bluetooth/hci_sync.c                           |  196 +--
 net/bluetooth/iso.c                                |  183 +--
 net/bluetooth/l2cap_core.c                         |  151 ++-
 net/bluetooth/l2cap_sock.c                         |  114 +-
 net/bluetooth/mgmt.c                               |  149 ++-
 net/bluetooth/sco.c                                |   33 +-
 net/bluetooth/smp.c                                |    2 +-
 45 files changed, 3046 insertions(+), 1167 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
 create mode 100644 drivers/bluetooth/btintel_pcie.c
 create mode 100644 drivers/bluetooth/btintel_pcie.h

Re: pull request: bluetooth-next 2024-05-10

[Jakub Kicinski]

On Fri, 10 May 2024 17:14:28 -0400 Luiz Augusto von Dentz wrote:
> The following changes since commit f8beae078c82abde57fed4a5be0bbc3579b59ad0:
> 
>   Merge tag 'gtp-24-05-07' of git://git.kernel.org/pub/scm/linux/kernel/git/pablo/gtp Pablo neira Ayuso says: (2024-05-10 13:59:27 +0100)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git tags/for-net-next-2024-05-10
> 
> for you to fetch changes up to 75f819bdf9cafb0f1458e24c05d24eec17b2f597:
> 
>   Bluetooth: btintel: Fix compiler warning for multi_v7_defconfig config (2024-05-10 17:04:15 -0400)
> 
> ----------------------------------------------------------------
> bluetooth-next pull request for net-next:
> 
>  - Add support MediaTek MT7921S SDIO
>  - Various fixes for -Wflex-array-member-not-at-end and -Wfamnae
>  - Add USB HW IDs for MT7921/MT7922/MT7925
>  - Add support for Intel BlazarI and Filmore Peak2 (BE201)
>  - Add initial support for Intel PCIe driver
>  - Remove HCI_AMP support
>  - Add TX timestamping support

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?

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.
   How does it compare to stamping in the driver in terms of accuracy?
 - the "experimental" BT_POLL_ERRQUEUE, how does the user space look?
   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?

Would be great to get more info and/or second opinion, because it's not
sufficiently "obviously right" to me to pull right away :(

Re: pull request: bluetooth-next 2024-05-10

[Luiz Augusto von Dentz]

Hi Jakub,

On Mon, May 13, 2024 at 5:26 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 10 May 2024 17:14:28 -0400 Luiz Augusto von Dentz wrote:
> > The following changes since commit f8beae078c82abde57fed4a5be0bbc3579b59ad0:
> >
> >   Merge tag 'gtp-24-05-07' of git://git.kernel.org/pub/scm/linux/kernel/git/pablo/gtp Pablo neira Ayuso says: (2024-05-10 13:59:27 +0100)
> >
> > are available in the Git repository at:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git tags/for-net-next-2024-05-10
> >
> > for you to fetch changes up to 75f819bdf9cafb0f1458e24c05d24eec17b2f597:
> >
> >   Bluetooth: btintel: Fix compiler warning for multi_v7_defconfig config (2024-05-10 17:04:15 -0400)
> >
> > ----------------------------------------------------------------
> > bluetooth-next pull request for net-next:
> >
> >  - Add support MediaTek MT7921S SDIO
> >  - Various fixes for -Wflex-array-member-not-at-end and -Wfamnae
> >  - Add USB HW IDs for MT7921/MT7922/MT7925
> >  - Add support for Intel BlazarI and Filmore Peak2 (BE201)
> >  - Add initial support for Intel PCIe driver
> >  - Remove HCI_AMP support
> >  - Add TX timestamping support
>
> 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?

>    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.

> 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.

-- 
Luiz Augusto von Dentz

Re: pull request: bluetooth-next 2024-05-10

[Jakub Kicinski]

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.

Re: pull request: bluetooth-next 2024-05-10

[Willem de Bruijn]

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.

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?

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.

Re: pull request: bluetooth-next 2024-05-10

[Luiz Augusto von Dentz]

Hi Willem,

On Mon, May 13, 2024 at 9:32 PM 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 '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

Re: pull request: bluetooth-next 2024-05-10

[Willem de Bruijn]

Luiz Augusto von Dentz wrote:
> Hi Willem,
> 
> On Mon, May 13, 2024 at 9:32 PM 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 '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

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 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

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.


> -- 
> Luiz Augusto von Dentz

Re: pull request: bluetooth-next 2024-05-10

[Luiz Augusto von Dentz]

Hi Willem,

On Mon, May 13, 2024 at 10:09 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Luiz Augusto von Dentz wrote:
> > Hi Willem,
> >
> > On Mon, May 13, 2024 at 9:32 PM 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 '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
>
> 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 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
>
> 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.

-- 
Luiz Augusto von Dentz

Re: pull request: bluetooth-next 2024-05-10

[Willem de Bruijn]

> > > > 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
> >
> > 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.

Perhaps this is because poll is level triggered. Maybe epoll in edge
triggered mode would avoid re-waking for the same outstanding events.

Re: pull request: bluetooth-next 2024-05-10

[Pauli Virtanen]

Hi all,

Thanks for the comments, I guess the original series should have been
Cc'd more widely to get them earlier.

ma, 2024-05-13 kello 22:09 -0400, Willem de Bruijn kirjoitti:
> Luiz Augusto von Dentz wrote:
> > Hi Willem,
> > 
> > On Mon, May 13, 2024 at 9:32 PM 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 '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
> 
> 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.

Ok, I was also under the impression TX_HARDWARE was only for actual HW
timestamps. TSTAMP_ACK appeared to not really match semantics either,
so TSTAMP_SND it then was.


The general timestamping flow here was:

sendmsg() from user generates skbs to net/bluetooth side queue
|
* wait in net/bluetooth side queue until HW has free packet slot
|
* send to driver (-> SCM_TSTAMP_SCHED*)
|
* driver (usu. ASAP) queues to transport e.g. USB
|
* transport tx complete, skb freed
|
* packet waits in hardware-side buffers (usu. the largest delay)
|
* packet completion report from HW (-> SCM_TSTAMP_SND*)
|
* for one packet type, HW timestamp for last tx packet can queried

The packet completion report does not imply the packet was received.

From the above, I gather SCHED* should be SND, and SND* should be
TXCOMPLETION. Then I'm not sure when we should generate SCHED, if at
all, unless it's done more or less in sendmsg() when it generates the
skbs.

Possibly the SND timestamp could also be generated on driver side if
one wants to have it taken at transport tx completion. I don't
immediately know what is the use case would be though, as the packet
may still have to wait on HW side before it goes over the air.

For the use case here, we want to know the total latency, so the
completion timestamp is the interesting one. In the audio use case, in
normal operation there is a free HW slot and packets do not wait in
net/bluetooth queues but end up in HW buffers ASAP (fast, maybe < 1
ms), and then wait a much longer time (usu. 5-50 ms) in the HW buffers
before it reports completion.

> > 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
> 
> 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.

This can be done on bluetoothd side, the ugly part is just the wakeup
on every TX timestamp, which is every ~10ms in these use cases if every
packet is stamped. EPOLLET probably would indeed avoid busy looping on
the same timestamp though.

In the first round of this patchset, this was handled on bluetoothd
side without kernel additions, with rate limiting the polling. If
POLL_ERRQUEUE sounds like a bad idea, maybe we can go back to that.

-- 
Pauli Virtanen

Re: pull request: bluetooth-next 2024-05-10

[Willem de Bruijn]

Pauli Virtanen wrote:
> Hi all,
> 
> Thanks for the comments, I guess the original series should have been
> Cc'd more widely to get them earlier.
> 
> ma, 2024-05-13 kello 22:09 -0400, Willem de Bruijn kirjoitti:
> > Luiz Augusto von Dentz wrote:
> > > Hi Willem,
> > > 
> > > On Mon, May 13, 2024 at 9:32 PM 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 '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
> > 
> > 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.
> 
> Ok, I was also under the impression TX_HARDWARE was only for actual HW
> timestamps. TSTAMP_ACK appeared to not really match semantics either,
> so TSTAMP_SND it then was.
> 
> 
> The general timestamping flow here was:
> 
> sendmsg() from user generates skbs to net/bluetooth side queue
> |
> * wait in net/bluetooth side queue until HW has free packet slot
> |
> * send to driver (-> SCM_TSTAMP_SCHED*)
> |
> * driver (usu. ASAP) queues to transport e.g. USB
> |
> * transport tx complete, skb freed
> |
> * packet waits in hardware-side buffers (usu. the largest delay)
> |
> * packet completion report from HW (-> SCM_TSTAMP_SND*)
> |
> * for one packet type, HW timestamp for last tx packet can queried
> 
> The packet completion report does not imply the packet was received.
> 
> From the above, I gather SCHED* should be SND, and SND* should be
> TXCOMPLETION. Then I'm not sure when we should generate SCHED, if at
> all, unless it's done more or less in sendmsg() when it generates the
> skbs.

Agreed. Missing SCHED if packets do not go through software queuing
should be fine. Inversely, with multiple qdiscs processes see multiple
SCHED events.

> Possibly the SND timestamp could also be generated on driver side if
> one wants to have it taken at transport tx completion. I don't
> immediately know what is the use case would be though, as the packet
> may still have to wait on HW side before it goes over the air.
> 
> For the use case here, we want to know the total latency, so the
> completion timestamp is the interesting one. In the audio use case, in
> normal operation there is a free HW slot and packets do not wait in
> net/bluetooth queues but end up in HW buffers ASAP (fast, maybe < 1
> ms), and then wait a much longer time (usu. 5-50 ms) in the HW buffers
> before it reports completion.
> 
> > > 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
> > 
> > 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.
> 
> This can be done on bluetoothd side, the ugly part is just the wakeup
> on every TX timestamp, which is every ~10ms in these use cases if every
> packet is stamped. EPOLLET probably would indeed avoid busy looping on
> the same timestamp though.
> 
> In the first round of this patchset, this was handled on bluetoothd
> side without kernel additions, with rate limiting the polling. If
> POLL_ERRQUEUE sounds like a bad idea, maybe we can go back to that.

We have prior art to avoid having timestamp completions on
MSG_ERRQUEUE set sk_err and so block normal processing.

Additional work on opting out of timestamp/zerocopy wake-ups sounds
reasonable to me.

Re: pull request: bluetooth-next 2024-05-10

[Luiz Augusto von Dentz]

Hi Pauli,

On Tue, May 14, 2024 at 2:40 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Pauli Virtanen wrote:
> > Hi all,
> >
> > Thanks for the comments, I guess the original series should have been
> > Cc'd more widely to get them earlier.
> >
> > ma, 2024-05-13 kello 22:09 -0400, Willem de Bruijn kirjoitti:
> > > Luiz Augusto von Dentz wrote:
> > > > Hi Willem,
> > > >
> > > > On Mon, May 13, 2024 at 9:32 PM 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 '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
> > >
> > > 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.
> >
> > Ok, I was also under the impression TX_HARDWARE was only for actual HW
> > timestamps. TSTAMP_ACK appeared to not really match semantics either,
> > so TSTAMP_SND it then was.
> >
> >
> > The general timestamping flow here was:
> >
> > sendmsg() from user generates skbs to net/bluetooth side queue
> > |
> > * wait in net/bluetooth side queue until HW has free packet slot
> > |
> > * send to driver (-> SCM_TSTAMP_SCHED*)
> > |
> > * driver (usu. ASAP) queues to transport e.g. USB
> > |
> > * transport tx complete, skb freed
> > |
> > * packet waits in hardware-side buffers (usu. the largest delay)
> > |
> > * packet completion report from HW (-> SCM_TSTAMP_SND*)
> > |
> > * for one packet type, HW timestamp for last tx packet can queried
> >
> > The packet completion report does not imply the packet was received.
> >
> > From the above, I gather SCHED* should be SND, and SND* should be
> > TXCOMPLETION. Then I'm not sure when we should generate SCHED, if at
> > all, unless it's done more or less in sendmsg() when it generates the
> > skbs.
>
> Agreed. Missing SCHED if packets do not go through software queuing
> should be fine. Inversely, with multiple qdiscs processes see multiple
> SCHED events.
>
> > Possibly the SND timestamp could also be generated on driver side if
> > one wants to have it taken at transport tx completion. I don't
> > immediately know what is the use case would be though, as the packet
> > may still have to wait on HW side before it goes over the air.
> >
> > For the use case here, we want to know the total latency, so the
> > completion timestamp is the interesting one. In the audio use case, in
> > normal operation there is a free HW slot and packets do not wait in
> > net/bluetooth queues but end up in HW buffers ASAP (fast, maybe < 1
> > ms), and then wait a much longer time (usu. 5-50 ms) in the HW buffers
> > before it reports completion.
> >
> > > > 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
> > >
> > > 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.
> >
> > This can be done on bluetoothd side, the ugly part is just the wakeup
> > on every TX timestamp, which is every ~10ms in these use cases if every
> > packet is stamped. EPOLLET probably would indeed avoid busy looping on
> > the same timestamp though.
> >
> > In the first round of this patchset, this was handled on bluetoothd
> > side without kernel additions, with rate limiting the polling. If
> > POLL_ERRQUEUE sounds like a bad idea, maybe we can go back to that.
>
> We have prior art to avoid having timestamp completions on
> MSG_ERRQUEUE set sk_err and so block normal processing.
>
> Additional work on opting out of timestamp/zerocopy wake-ups sounds
> reasonable to me.

Id like to follow-up on this, do you have any plans to continue on
this? In addition to synchronization Id also like to experiment
sending on TX complete kind of logic, instead of time based, or at
least I don't have a better way to attempt to synchronize the TX so I
assume if we keep feeding the controller whenever it has buffers
available it should be able to not miss any interval.

-- 
Luiz Augusto von Dentz

Re: pull request: bluetooth-next 2024-05-10

[Pauli Virtanen]

Hi Luiz,

ke, 2025-02-05 kello 15:44 -0500, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Tue, May 14, 2024 at 2:40 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> > 
> > Pauli Virtanen wrote:
> > > Hi all,
> > > 
> > > Thanks for the comments, I guess the original series should have been
> > > Cc'd more widely to get them earlier.
> > > 
> > > ma, 2024-05-13 kello 22:09 -0400, Willem de Bruijn kirjoitti:
> > > > Luiz Augusto von Dentz wrote:
> > > > > Hi Willem,
> > > > > 
> > > > > On Mon, May 13, 2024 at 9:32 PM 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 '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
> > > > 
> > > > 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.
> > > 
> > > Ok, I was also under the impression TX_HARDWARE was only for actual HW
> > > timestamps. TSTAMP_ACK appeared to not really match semantics either,
> > > so TSTAMP_SND it then was.
> > > 
> > > 
> > > The general timestamping flow here was:
> > > 
> > > sendmsg() from user generates skbs to net/bluetooth side queue
> > > > 
> > > * wait in net/bluetooth side queue until HW has free packet slot
> > > > 
> > > * send to driver (-> SCM_TSTAMP_SCHED*)
> > > > 
> > > * driver (usu. ASAP) queues to transport e.g. USB
> > > > 
> > > * transport tx complete, skb freed
> > > > 
> > > * packet waits in hardware-side buffers (usu. the largest delay)
> > > > 
> > > * packet completion report from HW (-> SCM_TSTAMP_SND*)
> > > > 
> > > * for one packet type, HW timestamp for last tx packet can queried
> > > 
> > > The packet completion report does not imply the packet was received.
> > > 
> > > From the above, I gather SCHED* should be SND, and SND* should be
> > > TXCOMPLETION. Then I'm not sure when we should generate SCHED, if at
> > > all, unless it's done more or less in sendmsg() when it generates the
> > > skbs.
> > 
> > Agreed. Missing SCHED if packets do not go through software queuing
> > should be fine. Inversely, with multiple qdiscs processes see multiple
> > SCHED events.
> > 
> > > Possibly the SND timestamp could also be generated on driver side if
> > > one wants to have it taken at transport tx completion. I don't
> > > immediately know what is the use case would be though, as the packet
> > > may still have to wait on HW side before it goes over the air.
> > > 
> > > For the use case here, we want to know the total latency, so the
> > > completion timestamp is the interesting one. In the audio use case, in
> > > normal operation there is a free HW slot and packets do not wait in
> > > net/bluetooth queues but end up in HW buffers ASAP (fast, maybe < 1
> > > ms), and then wait a much longer time (usu. 5-50 ms) in the HW buffers
> > > before it reports completion.
> > > 
> > > > > 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
> > > > 
> > > > 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.
> > > 
> > > This can be done on bluetoothd side, the ugly part is just the wakeup
> > > on every TX timestamp, which is every ~10ms in these use cases if every
> > > packet is stamped. EPOLLET probably would indeed avoid busy looping on
> > > the same timestamp though.
> > > 
> > > In the first round of this patchset, this was handled on bluetoothd
> > > side without kernel additions, with rate limiting the polling. If
> > > POLL_ERRQUEUE sounds like a bad idea, maybe we can go back to that.
> > 
> > We have prior art to avoid having timestamp completions on
> > MSG_ERRQUEUE set sk_err and so block normal processing.
> > 
> > Additional work on opting out of timestamp/zerocopy wake-ups sounds
> > reasonable to me.
> 
> Id like to follow-up on this, do you have any plans to continue on
> this? 

So this is on TODO list, there's an unsent revised patchset adding
SCM_TSTAMP_COMPLETION, taking above comments into account. I can try to
find some time to rebase it and send if useful.

I deprioritized this, as some Intel firmware changes significantly
reduced the ISO desync issue, but now it's not clear if the TX
completion timestamps can be used for ISO synchronization. IIRC, at
least how I used them previously doesn't work now.

The desync issue on Intel HW still exists (not good for production
use...), but it's hard to trigger, so I don't know currently if TX
timestamps can help with it. Someone would need to spend more time on
this.

TX timestamps would still be useful for flow control.

> In addition to synchronization Id also like to experiment
> sending on TX complete kind of logic, instead of time based, or at
> least I don't have a better way to attempt to synchronize the TX so I
> assume if we keep feeding the controller whenever it has buffers
> available it should be able to not miss any interval.

AFAICS, for synchronization (only) guidance in the specification is
(Version 6.0 | Vol 6, Part G Page 3709)

"""When an HCI ISO Data packet sent by the Host does not contain a
Time_Stamp or the Time_Stamp value is not based on the Controller's
clock, the Controller should determine the isochronous event to be used
to transmit the SDU contained in that packet based on the time of
arrival of that packet."""

which I'm interpreting that Host should queue synchronized packets for
different CIS to HCI at the same time. But since this seems
implementation-defined, I don't really know what Intel firmware is
expecting the Host to do, so maybe pull on completion works (at least
until user app misses a wakeup).

-- 
Pauli Virtanen

Re: pull request: bluetooth-next 2024-05-10

[Luiz Augusto von Dentz]

Hi Pauli,

On Thu, Feb 6, 2025 at 12:13 PM Pauli Virtanen <pav@iki.fi> wrote:
> AFAICS, for synchronization (only) guidance in the specification is
> (Version 6.0 | Vol 6, Part G Page 3709)
>
> """When an HCI ISO Data packet sent by the Host does not contain a
> Time_Stamp or the Time_Stamp value is not based on the Controller's
> clock, the Controller should determine the isochronous event to be used
> to transmit the SDU contained in that packet based on the time of
> arrival of that packet."""
>
> which I'm interpreting that Host should queue synchronized packets for
> different CIS to HCI at the same time. But since this seems
> implementation-defined, I don't really know what Intel firmware is
> expecting the Host to do, so maybe pull on completion works (at least
> until user app misses a wakeup).

Yeah, I think this lack the clarity on how the Controller determine
what packet got the what event, in theory the buffer count acts as the
queue, the queue is then used as jitter meaning there will be some
latency but I think that is sort of unavoidable with the way packets
are transmitted over HCI.

With this in mind I think one of the problems is that when we have
multiple connections we probably need to load balances the usage of
the controller buffers, each connection needs at least 2 buffers since
with just 1 it is possible to miss events due to the transport/bus not
being fast enough to notify number of packets complete event and then
the stack to react in time to send the next packet, so we need to have
at least 1 packet queued ahead of time.

Once we can notify the TX complete we can perhaps use it as a trigger
to send the next packet, instead of trying to do it time based like
bluetoothctl is doing nowadays, which imo is simpler and should result
in a better utilization of the controller buffers.

-- 
Luiz Augusto von Dentz

Re: pull request: bluetooth-next 2024-05-10

[Pauli Virtanen]

Hi Luiz,

pe, 2025-02-07 kello 12:34 -0500, Luiz Augusto von Dentz kirjoitti:
> On Thu, Feb 6, 2025 at 12:13 PM Pauli Virtanen <pav@iki.fi> wrote:
> > AFAICS, for synchronization (only) guidance in the specification is
> > (Version 6.0 | Vol 6, Part G Page 3709)
> > 
> > """When an HCI ISO Data packet sent by the Host does not contain a
> > Time_Stamp or the Time_Stamp value is not based on the Controller's
> > clock, the Controller should determine the isochronous event to be used
> > to transmit the SDU contained in that packet based on the time of
> > arrival of that packet."""
> > 
> > which I'm interpreting that Host should queue synchronized packets for
> > different CIS to HCI at the same time. But since this seems
> > implementation-defined, I don't really know what Intel firmware is
> > expecting the Host to do, so maybe pull on completion works (at least
> > until user app misses a wakeup).
> 
> Yeah, I think this lack the clarity on how the Controller determine
> what packet got the what event, in theory the buffer count acts as the
> queue, the queue is then used as jitter meaning there will be some
> latency but I think that is sort of unavoidable with the way packets
> are transmitted over HCI.
> 
> With this in mind I think one of the problems is that when we have
> multiple connections we probably need to load balances the usage of
> the controller buffers, each connection needs at least 2 buffers since
> with just 1 it is possible to miss events due to the transport/bus not
> being fast enough to notify number of packets complete event and then
> the stack to react in time to send the next packet, so we need to have
> at least 1 packet queued ahead of time.
> 
> Once we can notify the TX complete we can perhaps use it as a trigger
> to send the next packet, instead of trying to do it time based like
> bluetoothctl is doing nowadays, which imo is simpler and should result
> in a better utilization of the controller buffers.

Another thing to consider is that it's probably more or less guaranteed
userspace can miss the deadlines sometimes, and packets for some of the
CIS are late. (Maybe kernel can also miss deadlines, I don't know if
there's any guaranteed wakeup for the the HCI worker tasks.)

The scheduling should somehow guarantee that underrun only produces
temporary audio glitch, and not persistent stream desynchronization.

So maybe it is still necessary to wait for completion for all
synchronized CIS, and send new packet for all of them at once?

Or, maybe controllers now implement LE Read ISO TX Sync, and it can be
used to figure out the timestamping. IIUC, not straightforward either,
you'd need to deal with syncing packet sequence numbers with ISOAL and
clock drift...

-- 
Pauli Virtanen

Re: pull request: bluetooth-next 2024-05-10

[Luiz Augusto von Dentz]

Hi Jakub,

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

Re: pull request: bluetooth-next 2024-05-10

[Jakub Kicinski]

On Mon, 13 May 2024 22:12:04 -0400 Luiz Augusto von Dentz wrote:
> > > 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.

Sounds like the best way forward, thanks!
Could you drop them and resend the PR?

It's going to take me until noon pacific to write up the PR text
for all of net-next, to give you a sense of when our PR will come
out.

Re: pull request: bluetooth-next 2024-05-10

[Luiz Augusto von Dentz]

Hi Jakub,

On Tue, May 14, 2024 at 10:34 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 13 May 2024 22:12:04 -0400 Luiz Augusto von Dentz wrote:
> > > > 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.
>
> Sounds like the best way forward, thanks!
> Could you drop them and resend the PR?
>
> It's going to take me until noon pacific to write up the PR text
> for all of net-next, to give you a sense of when our PR will come
> out.

Perfect, will also make sure to include the btintel_pcie fix for the
warning and drop the entire set implementing support SO_TIMESTAMPING.



-- 
Luiz Augusto von Dentz

Re: pull request: bluetooth-next 2024-05-10

[Luiz Augusto von Dentz]

Hi Jakub,

On Mon, May 13, 2024 at 6:43 PM Jakub Kicinski <kuba@kernel.org> 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.

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/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?

This is more or less explained on the cover letter:

https://lore.kernel.org/linux-bluetooth/713b1d0333eb2f12e63bc8a7b8f423e1240abae0.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.

-- 
Luiz Augusto von Dentz