Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191129121042.3311-1-sathish.narasimman@intel.com> <D31F831A-84EE-4678-A1C6-421BEB8D1244@holtmann.org>
In-Reply-To: <D31F831A-84EE-4678-A1C6-421BEB8D1244@holtmann.org>
From:   Sathish Narasimman <nsathish41@gmail.com>
Date:   Thu, 2 Apr 2020 13:25:30 +0530
Message-ID: <CAOVXEJ+1AJm7krA-nFwK50aWnkM+zT+Y7H+Z4pahzO7ayP=9=w@mail.gmail.com>
Subject: Re: [PATCH v5 1/2] bluetooth: add support to notify SCO air mode
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     BlueZ devel list <linux-bluetooth@vger.kernel.org>,
        Sathish Narsimman <sathish.narasimman@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Marcel

On Sat, Jan 4, 2020 at 3:38 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Satish,
>
> > notifying HCI_NOTIFY_CONN_ADD for SCO connection is generic in
> > case of msbc audio. To differnetiate SCO air mode audio introducing
> > HCI_NOTIFY_SCO_CVSD and HCI_NOTIFY_SCO_TRASP.
> >
> > Signed-off-by: Sathish Narsimman <sathish.narasimman@intel.com>
> > ---
> > include/net/bluetooth/hci.h      |  3 +++
> > include/net/bluetooth/hci_core.h |  3 +++
> > net/bluetooth/hci_conn.c         | 19 +++++++++++++++----
> > net/bluetooth/hci_event.c        |  9 +++++++++
> > 4 files changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 5bc1e30dedde..bfaaef1aa4d6 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -52,6 +52,9 @@
> > #define HCI_NOTIFY_CONN_ADD           1
> > #define HCI_NOTIFY_CONN_DEL           2
> > #define HCI_NOTIFY_VOICE_SETTING      3
> > +#define HCI_NOTIFY_ENABLE_SCO_CVSD   4
> > +#define HCI_NOTIFY_ENABLE_SCO_TRANSP 5
> > +#define HCI_NOTIFY_DISABLE_SCO               6
> >
> > /* HCI bus types */
> > #define HCI_VIRTUAL   0
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index b689aceb636b..9258e19f8cc5 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -1604,4 +1604,7 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
> > #define SCO_AIRMODE_CVSD       0x0000
> > #define SCO_AIRMODE_TRANSP     0x0003
> >
> > +#define SCO_CODED_CVSD         0x02
> > +#define SCO_CODED_TRANSP       0x03
> > +
>
> why is this needed. And more important, what is wrong with SCO_AIRMODE_CVSD above?
>
AFAIK the above is voice settings param (core spec 5.0, Vol 2, Part E,
6.12) which is used to set the sco pkt type.
This patch is to notify the btusb layer. when we receive
sync_conn_complete_evt based on the AIR_MODE we receive
SCO_CODED_CVSD and SCO_CODED_TRANSP are used to choose alt_setting.
air_mode value we receive are 1 byte
whereas voice setting param is 2 bytes and are different for CVSD

> > #endif /* __HCI_CORE_H */
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 87691404d0c6..097a179418d6 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -122,8 +122,13 @@ static void hci_conn_cleanup(struct hci_conn *conn)
> >
> >       hci_conn_hash_del(hdev, conn);
> >
> > -     if (hdev->notify)
> > -             hdev->notify(hdev, HCI_NOTIFY_CONN_DEL);
> > +     if (conn->type == SCO_LINK || conn->type == ESCO_LINK) {
> > +             if (hdev->notify)
> > +                     hdev->notify(hdev, HCI_NOTIFY_DISABLE_SCO);
> > +     } else {
> > +             if (hdev->notify)
> > +                     hdev->notify(hdev, HCI_NOTIFY_CONN_DEL);
> > +     }
> >
> >       hci_conn_del_sysfs(conn);
> >
> > @@ -561,8 +566,14 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
> >       hci_dev_hold(hdev);
> >
> >       hci_conn_hash_add(hdev, conn);
> > -     if (hdev->notify)
> > -             hdev->notify(hdev, HCI_NOTIFY_CONN_ADD);
> > +
> > +     /* As HCI_NOTIFY_ENABLE_SCO_CVSD & HCI_NOTIFY_ENABLE_SCO_TRANSP is
> > +      * used to notify SCO connections during sync_conn_complete event
> > +      */
> > +     if (conn->type != SCO_LINK && conn->type != ESCO_LINK) {
> > +             if (hdev->notify)
> > +                     hdev->notify(hdev, HCI_NOTIFY_CONN_ADD);
> > +     }
> >
> >       hci_conn_init_sysfs(conn);
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index c1d3a303d97f..ecad696db139 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -4231,6 +4231,15 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
> >               break;
> >       }
> >
> > +     BT_DBG("sco connected with air mode: %02x", ev->air_mode);
> > +     if (ev->air_mode == SCO_CODED_CVSD) {
> > +             if (hdev->notify)
> > +                     hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> > +     } else if (ev->air_mode == SCO_CODED_TRANSP) {
> > +             if (hdev->notify)
> > +                     hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> > +     }
> > +
>
> You are however forgetting the old hci_conn_complete for legacy SCO setup.
Will fix this and update in next version
>
> Regards
>
> Marcel
>