RE: [PATCH v13 12/12] Bluetooth: Allow usb to auto-suspend when SCO use non-HCI transport

Hi Luiz,

-----Original Message-----
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Sent: Friday, September 3, 2021 4:07 AM
To: Tumkur Narayan, Chethan <redacted>
Cc: K, Kiran <redacted>; linux-bluetooth@vger.kernel.org; Pierres,
Arnaud [off-list ref]
Subject: Re: [PATCH v13 12/12] Bluetooth: Allow usb to auto-suspend when SCO
use non-HCI transport

Hi Chethan,

On Wed, Sep 1, 2021 at 8:52 PM Tumkur Narayan, Chethan
[off-list ref] wrote:

quoted

Hi Luiz,

quoted

-----Original Message-----
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Sent: Thursday, September 2, 2021 5:24 AM
To: K, Kiran <redacted>
Cc: linux-bluetooth@vger.kernel.org; Tumkur Narayan, Chethan
[off-list ref]
Subject: Re: [PATCH v13 12/12] Bluetooth: Allow usb to auto-suspend
when SCO use non-HCI transport

Hi Kiran,

On Tue, Aug 31, 2021 at 4:54 AM Kiran K [off-list ref] wrote:

quoted

From: Chethan T N <redacted>

Currently usb tranport is not allowed to suspend when SCO over HCI
tranport is active.

This patch shall enable the usb tranport to suspend when SCO link
use non-HCI transport

Signed-off-by: Chethan T N <redacted>
Signed-off-by: Kiran K <redacted>
---

Notes:
    changes in v13:
    - suspend usb in HFP offload use case

 drivers/bluetooth/btintel.c       |  2 +-
 include/net/bluetooth/bluetooth.h |  4 ++++
 net/bluetooth/hci_event.c         | 20 +++++++++++---------
 net/bluetooth/sco.c               |  2 +-
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/btintel.c

b/drivers/bluetooth/btintel.c index 6091b691ddc2..2d64e289cf6e
100644

--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c

@@ -2215,7 +2215,7 @@ static int

btintel_get_codec_config_data(struct
hci_dev *hdev,  static int btintel_get_data_path_id(struct hci_dev
*hdev, __u8 *data_path_id)  {
        /* Intel uses 1 as data path id for all the usecases */
-       *data_path_id = 1;
+       *data_path_id = BT_SCO_PCM_PATH;
        return 0;
 }

diff --git a/include/net/bluetooth/bluetooth.h

b/include/net/bluetooth/bluetooth.h
index c1fa90fb7502..9e2745863b33 100644

--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h

@@ -177,6 +177,10 @@ struct bt_codecs {
 #define CODING_FORMAT_TRANSPARENT      0x03
 #define CODING_FORMAT_MSBC             0x05

+/* Audio data transport path used for SCO */ #define
+BT_SCO_HCI_PATH
+0x00 #define BT_SCO_PCM_PATH 0x01

If this is in fact vendor specific perhaps you should be declared in
btintel.h not here.

This is defined the Host Controller Interface assigned numbers, page no.3

"Transport Layer"-
https://btprodspecificationrefs.blob.core.windows.net/assigned-
numbers/Assigned%20Number%20Types/Host%20Controller%20Interface.pdf.
So defined in bluetooth.h, let me know if you think otherwise.

BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 4, Part E page 2221
Data_Path_ID:
0x01 to 0xFE Logical channel number; the meaning is vendor-specific.


BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 4, Part E page 2022
Input_Data_Path:
0x01 to 0xFE Logical_Channel_Number. The meaning of the logical channels will
be vendor specific.
Output_Data_Path:
0x01 to 0xFE Logical_Channel_Number. The meaning of the logical channels will
be vendor specific.

Ack, will remove/move these #defines accordingly.

quoted

quoted

quoted

+
 __printf(1, 2)
 void bt_info(const char *fmt, ...);  __printf(1, 2) diff --git
a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index
b48e24629fa4..7ff11cba89cf 100644

--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c

@@ -4516,15 +4516,17 @@ static void

hci_sync_conn_complete_evt(struct hci_dev *hdev,

        bt_dev_dbg(hdev, "SCO connected with air mode: %02x",
ev->air_mode);

-       switch (ev->air_mode) {
-       case 0x02:
-               if (hdev->notify)
-                       hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
-               break;
-       case 0x03:
-               if (hdev->notify)
-                       hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
-               break;
+       if (conn->codec.data_path == BT_SCO_HCI_PATH) {
+               switch (ev->air_mode) {
+               case 0x02:
+                       if (hdev->notify)
+                               hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
+                       break;
+               case 0x03:
+                       if (hdev->notify)
+                               hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
+                       break;
+               }

Hmm I think we might need to notify the driver to enable PCM routing
so the likes of btusb can call
usb_disable_endpoint/usb_enable_endpoint for example since in theory
userspace may choose to switch from offload and vice-versa, note
without calling usb_disable_endpoint there might not be much power
saving after all since the endpoint will remain active or do we
actually have a good explanation why it shall not be called when
using PCM routing? Note that usb_set_interface will call
usb_enable_interface that will then call usb_enable_endpoint so
perhaps we need to call usb_disable_interface, either way we can't assume

the platform will be restricted to only use one or the other.

quoted

Ack, Does it make sense to define and notify events

"HCI_NOTIFY_DISABLE_SCO_USB_INTF ",
"HCI_NOTIFY_ENABLE_SCO_USB_INTF " accordingly and handle this in btusb
driver by disabling/enabling the ISOC endpoint respectively. That will serve the
purpose or switch between software to hardware.

Or perhaps we should switch to notify the actual data path, in fact there could
be situations where we have both hardware offload and software based if we
were dealing with multiple connections in which case we would need to check if
there is any connection using HCI routing before disabling it.

Actually, there are no API's from USB core exposed to disable the interface or endpoints to any device driver. However as discussed setting the altsetting to 0 to the ISOC endpoint would be feasible and by doing so no memory and bandwidth shall be allocated for the interface. 

Likewise when it comes to switching from offload to non-offload or vice versa  the same logic of resetting the ISOC endpoint to altsetting 0 makes reliable on SCO disconnection and also I checked the flow where on every SCO disconnection in the clean up procedure "HCI_NOTIFY_DISABLE_SCO" shall be notify to the driver that shall reset ISOC interface with altsetting 0.  Having said that no additional handling of disabling the ISOC interface would be required. 

I shall  re-work and send the updated patch.

quoted

quoted

quoted

        }

        hci_connect_cfm(conn, ev->status); diff --git
a/net/bluetooth/sco.c b/net/bluetooth/sco.c index
004bce2b5eca..f35c12ca6aa5 100644

--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c

@@ -506,7 +506,7 @@ static struct sock *sco_sock_alloc(struct net

*net,

struct socket *sock,

quoted

        sco_pi(sk)->codec.id = CODING_FORMAT_CVSD;
        sco_pi(sk)->codec.cid = 0xffff;
        sco_pi(sk)->codec.vid = 0xffff;
-       sco_pi(sk)->codec.data_path = 0x00;
+       sco_pi(sk)->codec.data_path = BT_SCO_HCI_PATH;

        bt_sock_link(&sco_sk_list, sk);
        return sk;
--
2.17.1

--
Luiz Augusto von Dentz

--
Luiz Augusto von Dentz