| Message ID | 20210831115637.6713-1-kiran.k@intel.com |
|---|---|
| State | Accepted |
| Commit | 8961987f3f5fa2f2618e72304d013c8dd5e604a6 |
| Headers | show |
| Series | [v13,01/12] Bluetooth: Enumerate local supported codec and cache details | expand |
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 >
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. > > > + > > __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. > > > } > > > > 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
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. Regards Marcel
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. > > > > > + > > > __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. > > > > > } > > > > > > 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
Hi Marcel, On Thu, Sep 2, 2021 at 2:37 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > 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. > Regards > > Marcel > -- Luiz Augusto von Dentz
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
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
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index bb6b7398f490..13a93da6b76c 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -1308,6 +1308,28 @@ struct hci_rp_read_data_block_size { } __packed; #define HCI_OP_READ_LOCAL_CODECS 0x100b +struct hci_std_codecs { + __u8 num; + __u8 codec[]; +} __packed; + +struct hci_vnd_codec { + /* company id */ + __le16 cid; + /* vendor codec id */ + __le16 vid; +} __packed; + +struct hci_vnd_codecs { + __u8 num; + struct hci_vnd_codec codec[]; +} __packed; + +struct hci_rp_read_local_supported_codecs { + __u8 status; + struct hci_std_codecs std_codecs; + struct hci_vnd_codecs vnd_codecs; +} __packed; #define HCI_OP_READ_LOCAL_PAIRING_OPTS 0x100c struct hci_rp_read_local_pairing_opts { @@ -1316,6 +1338,25 @@ struct hci_rp_read_local_pairing_opts { __u8 max_key_size; } __packed; +#define HCI_OP_READ_LOCAL_CODEC_CAPS 0x100e +struct hci_op_read_local_codec_caps { + __u8 id; + __le16 cid; + __le16 vid; + __u8 transport; + __u8 direction; +} __packed; + +struct hci_codec_caps { + __u8 len; + __u8 data[]; +} __packed; + +struct hci_rp_read_local_codec_caps { + __u8 status; + __u8 num_caps; +} __packed; + #define HCI_OP_READ_PAGE_SCAN_ACTIVITY 0x0c1b struct hci_rp_read_page_scan_activity { __u8 status; diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 24ad6ebd00e2..585cbe7ff67d 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -131,6 +131,17 @@ struct bdaddr_list { u8 bdaddr_type; }; +struct codec_list { + struct list_head list; + u8 id; + __u16 cid; + __u16 vid; + u8 transport; + u8 num_caps; + u32 len; + struct hci_codec_caps caps[]; +}; + struct bdaddr_list_with_irk { struct list_head list; bdaddr_t bdaddr; @@ -536,6 +547,7 @@ struct hci_dev { struct list_head pend_le_conns; struct list_head pend_le_reports; struct list_head blocked_keys; + struct list_head local_codecs; struct hci_dev_stats stat; @@ -1870,4 +1882,9 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr, #define SCO_AIRMODE_CVSD 0x0000 #define SCO_AIRMODE_TRANSP 0x0003 +#define LOCAL_CODEC_ACL_MASK BIT(0) +#define LOCAL_CODEC_SCO_MASK BIT(1) + +#define TRANSPORT_TYPE_MAX 0x04 + #endif /* __HCI_CORE_H */ diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile index cc0995301f93..f3e439d98b85 100644 --- a/net/bluetooth/Makefile +++ b/net/bluetooth/Makefile @@ -14,7 +14,7 @@ bluetooth_6lowpan-y := 6lowpan.o bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \ hci_sock.o hci_sysfs.o l2cap_core.o l2cap_sock.o smp.o lib.o \ - ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o + ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o hci_codec.o bluetooth-$(CONFIG_BT_BREDR) += sco.o bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o diff --git a/net/bluetooth/hci_codec.c b/net/bluetooth/hci_codec.c new file mode 100644 index 000000000000..f86ca6ba5814 --- /dev/null +++ b/net/bluetooth/hci_codec.c @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* Copyright (C) 2021 Intel Corporation */ + +#include <net/bluetooth/bluetooth.h> +#include <net/bluetooth/hci_core.h> +#include "hci_codec.h" + +static int hci_codec_list_add(struct list_head *list, + struct hci_op_read_local_codec_caps *sent, + struct hci_rp_read_local_codec_caps *rp, + void *caps, + __u32 len) +{ + struct codec_list *entry; + + entry = kzalloc(sizeof(*entry) + len, GFP_KERNEL); + if (!entry) + return -ENOMEM; + + entry->id = sent->id; + if (sent->id == 0xFF) { + entry->cid = __le16_to_cpu(sent->cid); + entry->vid = __le16_to_cpu(sent->vid); + } + entry->transport = sent->transport; + entry->len = len; + entry->num_caps = rp->num_caps; + if (rp->num_caps) + memcpy(entry->caps, caps, len); + list_add(&entry->list, list); + + return 0; +} + +void hci_codec_list_clear(struct list_head *codec_list) +{ + struct codec_list *c, *n; + + list_for_each_entry_safe(c, n, codec_list, list) { + list_del(&c->list); + kfree(c); + } +} + +static void hci_read_codec_capabilities(struct hci_dev *hdev, __u8 transport, + struct hci_op_read_local_codec_caps + *cmd) +{ + __u8 i; + + for (i = 0; i < TRANSPORT_TYPE_MAX; i++) { + if (transport & BIT(i)) { + struct hci_rp_read_local_codec_caps *rp; + struct hci_codec_caps *caps; + struct sk_buff *skb; + __u8 j; + __u32 len; + + cmd->transport = i; + skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_CODEC_CAPS, + sizeof(*cmd), cmd, + HCI_CMD_TIMEOUT); + if (IS_ERR(skb)) { + bt_dev_err(hdev, "Failed to read codec capabilities (%ld)", + PTR_ERR(skb)); + continue; + } + + if (skb->len < sizeof(*rp)) + goto error; + + rp = (void *)skb->data; + + if (rp->status) + goto error; + + if (!rp->num_caps) { + len = 0; + /* this codec doesn't have capabilities */ + goto skip_caps_parse; + } + + skb_pull(skb, sizeof(*rp)); + + for (j = 0, len = 0; j < rp->num_caps; j++) { + caps = (void *)skb->data; + if (skb->len < sizeof(*caps)) + goto error; + if (skb->len < caps->len) + goto error; + len += sizeof(caps->len) + caps->len; + skb_pull(skb, sizeof(caps->len) + caps->len); + } + +skip_caps_parse: + hci_dev_lock(hdev); + hci_codec_list_add(&hdev->local_codecs, cmd, rp, + (__u8 *)rp + sizeof(*rp), len); + hci_dev_unlock(hdev); +error: + kfree_skb(skb); + } + } +} + +void hci_read_supported_codecs(struct hci_dev *hdev) +{ + struct sk_buff *skb; + struct hci_rp_read_local_supported_codecs *rp; + struct hci_std_codecs *std_codecs; + struct hci_vnd_codecs *vnd_codecs; + struct hci_op_read_local_codec_caps caps; + __u8 i; + + skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_CODECS, 0, NULL, + HCI_CMD_TIMEOUT); + + if (IS_ERR(skb)) { + bt_dev_err(hdev, "Failed to read local supported codecs (%ld)", + PTR_ERR(skb)); + return; + } + + if (skb->len < sizeof(*rp)) + goto error; + + rp = (void *)skb->data; + + if (rp->status) + goto error; + + skb_pull(skb, sizeof(rp->status)); + + std_codecs = (void *)skb->data; + + /* validate codecs length before accessing */ + if (skb->len < flex_array_size(std_codecs, codec, std_codecs->num) + + sizeof(std_codecs->num)) + goto error; + + /* enumerate codec capabilities of standard codecs */ + memset(&caps, 0, sizeof(caps)); + for (i = 0; i < std_codecs->num; i++) { + caps.id = std_codecs->codec[i]; + caps.direction = 0x00; + hci_read_codec_capabilities(hdev, LOCAL_CODEC_ACL_MASK, &caps); + } + + skb_pull(skb, flex_array_size(std_codecs, codec, std_codecs->num) + + sizeof(std_codecs->num)); + + vnd_codecs = (void *)skb->data; + + /* validate vendor codecs length before accessing */ + if (skb->len < + flex_array_size(vnd_codecs, codec, vnd_codecs->num) + + sizeof(vnd_codecs->num)) + goto error; + + /* enumerate vendor codec capabilities */ + for (i = 0; i < vnd_codecs->num; i++) { + caps.id = 0xFF; + caps.cid = vnd_codecs->codec[i].cid; + caps.vid = vnd_codecs->codec[i].vid; + caps.direction = 0x00; + hci_read_codec_capabilities(hdev, LOCAL_CODEC_ACL_MASK, &caps); + } + +error: + kfree_skb(skb); +} diff --git a/net/bluetooth/hci_codec.h b/net/bluetooth/hci_codec.h new file mode 100644 index 000000000000..efb0df634ac6 --- /dev/null +++ b/net/bluetooth/hci_codec.h @@ -0,0 +1,6 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +/* Copyright (C) 2014 Intel Corporation */ + +void hci_read_supported_codecs(struct hci_dev *hdev); +void hci_codec_list_clear(struct list_head *codec_list); diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index f3a18d16b81f..a80d813ef743 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -45,6 +45,7 @@ #include "leds.h" #include "msft.h" #include "aosp.h" +#include "hci_codec.h" static void hci_rx_work(struct work_struct *work); static void hci_cmd_work(struct work_struct *work); @@ -838,10 +839,6 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt) if (hdev->commands[22] & 0x04) hci_set_event_mask_page_2(req); - /* Read local codec list if the HCI command is supported */ - if (hdev->commands[29] & 0x20) - hci_req_add(req, HCI_OP_READ_LOCAL_CODECS, 0, NULL); - /* Read local pairing options if the HCI command is supported */ if (hdev->commands[41] & 0x08) hci_req_add(req, HCI_OP_READ_LOCAL_PAIRING_OPTS, 0, NULL); @@ -937,6 +934,10 @@ static int __hci_init(struct hci_dev *hdev) if (err < 0) return err; + /* Read local codec list if the HCI command is supported */ + if (hdev->commands[29] & 0x20) + hci_read_supported_codecs(hdev); + /* This function is only called when the controller is actually in * configured state. When the controller is marked as unconfigured, * this initialization procedure is not run. @@ -1848,6 +1849,7 @@ int hci_dev_do_close(struct hci_dev *hdev) memset(hdev->eir, 0, sizeof(hdev->eir)); memset(hdev->dev_class, 0, sizeof(hdev->dev_class)); bacpy(&hdev->random_addr, BDADDR_ANY); + hci_codec_list_clear(&hdev->local_codecs); hci_req_sync_unlock(hdev); @@ -3848,6 +3850,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) INIT_LIST_HEAD(&hdev->adv_instances); INIT_LIST_HEAD(&hdev->blocked_keys); + INIT_LIST_HEAD(&hdev->local_codecs); INIT_WORK(&hdev->rx_work, hci_rx_work); INIT_WORK(&hdev->cmd_work, hci_cmd_work); INIT_WORK(&hdev->tx_work, hci_tx_work);