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;
MIME-Version: 1.0
References: <20210831115637.6713-1-kiran.k@intel.com> <20210831115637.6713-12-kiran.k@intel.com>
In-Reply-To: <20210831115637.6713-12-kiran.k@intel.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 1 Sep 2021 16:54:08 -0700
Message-ID: <CABBYNZL3=z6CWj9EAiGnOkcaEmgs1Mq7kc5-fT4Mjk2HCUPsFA@mail.gmail.com>
Subject: Re: [PATCH v13 12/12] Bluetooth: Allow usb to auto-suspend when SCO
 use non-HCI transport
To:     Kiran K <kiran.k@intel.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Chethan T N <chethan.tumkur.narayan@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Kiran,

On Tue, Aug 31, 2021 at 4:54 AM Kiran K <kiran.k@intel.com> wrote:
>
> From: Chethan T N <chethan.tumkur.narayan@intel.com>
>
> 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.

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