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;
IronPort-SDR: rZOWGcXWCvve2WY+Obl6gOz+4Wkw7ROLyF1+bdC48MEZN6Bdmh/D61sMAiOc899cT7TNOyAhop
 jnVyxYDoOFJg==
IronPort-SDR: bT6k7sx4u4pUZ9cs9xsKC5j/rKXIrhJf3SjgXfMwP2Yc7DJcQPnpEYGwyOaqEcib9zTzSxFJ8D
 DbjmQPV1o2wg==
From:   "K, Kiran" <kiran.k@intel.com>
To:     Marcel Holtmann <marcel@holtmann.org>
CC:     "open list:BLUETOOTH DRIVERS" <linux-bluetooth@vger.kernel.org>,
        "Srivatsa, Ravishankar" <ravishankar.srivatsa@intel.com>,
        "Tumkur Narayan, Chethan" <chethan.tumkur.narayan@intel.com>,
        "Von Dentz, Luiz" <luiz.von.dentz@intel.com>
Subject: RE: [PATCH v2 1/3] Bluetooth: add support to enumerate codec
 capabilities
Thread-Topic: [PATCH v2 1/3] Bluetooth: add support to enumerate codec
 capabilities
Thread-Index: AQHXL3ZHw8qxP5Rt8k67snZMAKMkIaqy04uAgA3PI7A=
Date:   Thu, 22 Apr 2021 14:08:32 +0000
Message-ID: <DM8PR11MB55730591C18EFDD9FD603717F5469@DM8PR11MB5573.namprd11.prod.outlook.com>
References: <20210412083538.18600-1-kiran.k@intel.com>
 <3434AC21-8C36-435D-9D0F-3597FE434D54@holtmann.org>
In-Reply-To: <3434AC21-8C36-435D-9D0F-3597FE434D54@holtmann.org>
Accept-Language: en-US
Content-Language: en-US
dlp-reaction: no-action
dlp-version: 11.5.1.3
dlp-product: dlpe-windows
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 2d85313d-cc78-4827-6161-08d905981cc7
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Apr 2021 14:08:32.4855
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 9qCjPCP133YLdEpBHzQjSeQGSjRpBUkd7+PEBCdD8ZKDgkOCVea8svIDpA6b3ftZyQ9YZX2zfpZjuRRHUiMCCA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR11MB5670
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-bluetooth.vger.kernel.org>
X-Mailing-List: linux-bluetooth@vger.kernel.org

Hi Marcel,

Thanks for the comments. I will send out an updated version of patchset.

> -----Original Message-----
> From: Marcel Holtmann <marcel@holtmann.org>
> Sent: Wednesday, April 14, 2021 12:45 AM
> To: K, Kiran <kiran.k@intel.com>
> Cc: open list:BLUETOOTH DRIVERS <linux-bluetooth@vger.kernel.org>;
> Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; Tumkur Narayan,
> Chethan <chethan.tumkur.narayan@intel.com>; Von Dentz, Luiz
> <luiz.von.dentz@intel.com>
> Subject: Re: [PATCH v2 1/3] Bluetooth: add support to enumerate codec
> capabilities
>=20
> Hi Kiran,
>=20
> > 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>
> > ---
> > * changes in v2
> >  add skb->len check before accessing event data
> >
> > include/net/bluetooth/hci.h |  7 ++++
> > net/bluetooth/hci_event.c   | 68
> +++++++++++++++++++++++++++++++++++++
> > 2 files changed, 75 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 5e99968939ce..a4b905a76c1b 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -887,6 +887,70 @@ 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;
> > +	struct hci_op_read_local_codec_caps caps;
> > +
> > +	if (skb->len < sizeof(caps))
> > +		return;
> > +
> > +	bt_dev_dbg(hdev, "status 0x%2.2x", skb->data[0]);
> > +
> > +	if (skb->data[0])
> > +		return;
> > +
> > +	/* enumerate standard codecs */
> > +	skb_pull(skb, 1);
> > +
> > +	if (skb->len < 1)
> > +		return;
> > +
> > +	num_codecs =3D skb->data[0];
> > +
> > +	bt_dev_dbg(hdev, "Number of standard codecs: %u", num_codecs);
> > +
> > +	skb_pull(skb, 1);
> > +
> > +	if (skb->len < num_codecs)
> > +		return;
> > +
> > +	while (num_codecs--) {
> > +		caps.codec_id[0] =3D skb->data[0];
> > +		caps.transport =3D 0x00;
> > +		caps.direction =3D 0x00;
> > +
> > +		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_CODEC_CAPS,
> sizeof(caps),
> > +			     &caps);
> > +
> > +		skb_pull(skb, 1);
> > +	}
> > +
> > +	/* enumerate vendor specific codecs */
> > +	if (skb->len < 1)
> > +		return;
> > +
> > +	num_codecs =3D skb->data[0];
> > +	skb_pull(skb, 1);
> > +
> > +	bt_dev_dbg(hdev, "Number of vendor specific codecs: %u",
> > +num_codecs);
> > +
> > +	if (skb->len < (num_codecs * 4))
> > +		return;
> > +
> > +	while (num_codecs--) {
> > +		caps.codec_id[0] =3D 0xFF;
> > +		memcpy(&caps.codec_id[1], skb->data, 4);
> > +		caps.transport =3D 0x00;
> > +		caps.direction =3D 0x00;
> > +
> > +		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_CODEC_CAPS,
> sizeof(caps),
> > +			     &caps);
> > +		skb_pull(skb, 4);
> > +	}
>=20
> instead of sending hci_send_cmd here, I rather do this in a separate init
> stage. Since you want to cache the codec values anyway, start doing it no=
w.
>=20
> Regards
>=20
> Marcel

Regards,
Kiran