Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\))
Subject: Re: [PATCH v13 12/12] Bluetooth: Allow usb to auto-suspend when SCO
 use non-HCI transport
From:   Marcel Holtmann <marcel@holtmann.org>
In-Reply-To: <CABBYNZJGF+VzeouaJXf7_T1smtgEqwyUimsxV8ajUZURMGBxfg@mail.gmail.com>
Date:   Fri, 10 Sep 2021 09:35:11 +0200
Cc:     Kiran K <kiran.k@intel.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Chethan T N <chethan.tumkur.narayan@intel.com>
Content-Transfer-Encoding: 8BIT
Message-Id: <B092F950-4328-4609-8A24-8E48B8272EE8@holtmann.org>
References: <20210831115637.6713-1-kiran.k@intel.com>
 <20210831115637.6713-12-kiran.k@intel.com>
 <CABBYNZL3=z6CWj9EAiGnOkcaEmgs1Mq7kc5-fT4Mjk2HCUPsFA@mail.gmail.com>
 <135FFE98-549B-4C02-9AC3-AD7F18EC0564@holtmann.org>
 <CABBYNZJGF+VzeouaJXf7_T1smtgEqwyUimsxV8ajUZURMGBxfg@mail.gmail.com>
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Precedence: bulk

Hi Luiz,

>>>> 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 <chethan.tumkur.narayan@intel.com>
>>>> Signed-off-by: Kiran K <kiran.k@intel.com>
>>>> ---
>>>> 
>>>> 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.
>>> 
>>>> +
>>>> __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 software to hardware 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.
>> 
>> actually for the Intel hardware we shouldn’t do this at all. We should switch to vendor specific SCO over bulk endpoints and not claim the ISOC endpoints at all.
> 
> Yep, but I guess that requires switching to SCO over bulk then which
> perhaps needs more changes, not sure if we should pursue that or go
> with all the way with H4 mode like we did in Zephyr, anyway for the
> purpose of offload I would be fine skipping the SCO over bulk since we
> are already at v13 of these.

actually SCO over bulk is rather simple. You jus send one extra HCI command at init (just need to figure out if it survices HCI_Reset) and then you only need to look up the handle if it is SCO or ACL.

Regards

Marcel