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

["Tumkur Narayan, Chethan" <chethan.tumkur.narayan@intel.com>]

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 <chethan.tumkur.narayan@intel.com>
> Cc: K, Kiran <kiran.k@intel.com>; linux-bluetooth@vger.kernel.org; Pierres,
> Arnaud <arnaud.pierres@intel.com>
> 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
> <chethan.tumkur.narayan@intel.com> wrote:
> >
> > Hi Luiz,
> >
> > > -----Original Message-----
> > > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> > > Sent: Thursday, September 2, 2021 5:24 AM
> > > To: K, Kiran <kiran.k@intel.com>
> > > Cc: linux-bluetooth@vger.kernel.org; Tumkur Narayan, Chethan
> > > <chethan.tumkur.narayan@intel.com>
> > > 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 <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.
> > 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.
> > >
> > > > +
> > > >  __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.
> > 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.
> > >
> > > >         }
> > > >
> > > >         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
> 
> 
> 
> --
> Luiz Augusto von Dentz