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: <20210409122405.29850-1-kiran.k@intel.com> <20210409122405.29850-2-kiran.k@intel.com>
In-Reply-To: <20210409122405.29850-2-kiran.k@intel.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 9 Apr 2021 10:02:00 -0700
Message-ID: <CABBYNZLm5jqbzDVNLRG4G1bSgK8AwCPmz_gjGZ92Oo2EZxfpvQ@mail.gmail.com>
Subject: Re: [PATCH v1 1/3] Bluetooth: add support to enumerate codec capabilities
To:     Kiran K <kiran.k@intel.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        "Srivatsa, Ravishankar" <ravishankar.srivatsa@intel.com>,
        Chethan T N <chethan.tumkur.narayan@intel.com>,
        Luiz Augusto Von Dentz <luiz.von.dentz@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Kiran,

On Fri, Apr 9, 2021 at 5:21 AM Kiran K <kiran.k@intel.com> wrote:
>
> add support to enumerate local supported codec capabilities
>
> < HCI Command: Read Local Suppor.. (0x04|0x000e) plen 7
>         Codec: mSBC (0x05)
>         Logical Transport Type: 0x00
>         Direction: Input (Host to Controller) (0x00)
> > HCI Event: Command Complete (0x0e) plen 12
>       Read Local Supported Codec Capabilities (0x04|0x000e) ncmd 1
>         Status: Success (0x00)
>         Number of codec capabilities: 1
>          Capabilities #0:
>         00 00 11 15 02 33
>
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Signed-off-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
>  include/net/bluetooth/hci.h |  7 +++++
>  net/bluetooth/hci_event.c   | 56 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index ea4ae551c426..e3f7771fe84f 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1314,6 +1314,13 @@ 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    codec_id[5];
> +       __u8    transport;
> +       __u8    direction;
> +} __packed;
> +
>  #define HCI_OP_READ_PAGE_SCAN_ACTIVITY 0x0c1b
>  struct hci_rp_read_page_scan_activity {
>         __u8     status;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 016b2999f219..ceed5a5d332b 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -887,6 +887,58 @@ static void hci_cc_read_data_block_size(struct hci_dev *hdev,
>                hdev->block_cnt, hdev->block_len);
>  }
>
> +static void hci_cc_read_local_codecs(struct hci_dev *hdev,
> +                                    struct sk_buff *skb)
> +{
> +       __u8 num_codecs;
> +       __u8 *ptr;
> +       struct hci_op_read_local_codec_caps caps;
> +
> +       bt_dev_dbg(hdev, "status 0x%2.2x", skb->data[0]);
> +
> +       if (skb->data[0])
> +               return;
> +
> +       /* enumerate standard codecs */
> +       skb_pull(skb, 1);

After each skb_pull check the expected length against skb->len.

> +       num_codecs = skb->data[0];
> +
> +       bt_dev_dbg(hdev, "Number of standard codecs: %u", num_codecs);
> +
> +       skb_pull(skb, 1);

Ditto.

> +       ptr = (__u8 *)skb->data;
> +
> +       skb_pull(skb, num_codecs);
> +
> +       while (num_codecs--) {
> +               caps.codec_id[0] = *ptr++;

Lets just use skb_pull to advance on the codecs ids, that way we can
properly check the remaining length with use of skb->len.

> +               caps.transport = 0x00;
> +               caps.direction = 0x00;
> +
> +               hci_send_cmd(hdev, HCI_OP_READ_LOCAL_CODEC_CAPS, sizeof(caps),
> +                            &caps);
> +       }
> +
> +       /* enumerate vendor specific codecs */
> +       num_codecs = skb->data[0];
> +       skb_pull(skb, 1);
> +
> +       bt_dev_dbg(hdev, "Number of vendor specific codecs: %u", num_codecs);
> +
> +       ptr = (__u8 *)skb->data;
> +
> +       while (num_codecs--) {
> +               caps.codec_id[0] = 0xFF;
> +               memcpy(&caps.codec_id[1], ptr, 4);
> +               ptr += 4;
> +               caps.transport = 0x00;
> +               caps.direction = 0x00;
> +
> +               hci_send_cmd(hdev, HCI_OP_READ_LOCAL_CODEC_CAPS, sizeof(caps),
> +                            &caps);
> +       }
> +}
> +
>  static void hci_cc_read_clock(struct hci_dev *hdev, struct sk_buff *skb)
>  {
>         struct hci_rp_read_clock *rp = (void *) skb->data;
> @@ -3437,6 +3489,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
>                 hci_cc_read_data_block_size(hdev, skb);
>                 break;
>
> +       case HCI_OP_READ_LOCAL_CODECS:
> +               hci_cc_read_local_codecs(hdev, skb);
> +               break;
> +
>         case HCI_OP_READ_FLOW_CONTROL_MODE:
>                 hci_cc_read_flow_control_mode(hdev, skb);
>                 break;
> --
> 2.17.1
>


-- 
Luiz Augusto von Dentz