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: BUU4OoTw9V7aENiCwsKFqMuBf4iCvZZu0u+bmJfuaLMc7ajwoxC9+yIxgW6mHjrgUqYTxu8hMj
 P++pIwcBftEw==
IronPort-SDR: Kb0gC9ePRH3ynWECeNkKnyrdN0aH71tyuK+qF+ucM4rnkLldslZQ9jjt61N1ttS/SB+n0rDYOK
 +LNwbENvc+Bw==
From:   "K, Kiran" <kiran.k@intel.com>
To:     Marcel Holtmann <marcel@holtmann.org>
CC:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        "Srivatsa, Ravishankar" <ravishankar.srivatsa@intel.com>,
        "Tumkur Narayan, Chethan" <chethan.tumkur.narayan@intel.com>
Subject: RE: [PATCH v8 5/9] Bluetooth: Add BT_CODEC option for getsockopt for
 SCO socket
Thread-Topic: [PATCH v8 5/9] Bluetooth: Add BT_CODEC option for getsockopt for
 SCO socket
Thread-Index: AQHXS9IEdU2cPFWGyU6pRlqFOl/QaqsCc74AgAevPcA=
Date:   Tue, 8 Jun 2021 12:01:43 +0000
Message-ID: <DM8PR11MB5573537BBBC5EFEDAC5B6DCEF5379@DM8PR11MB5573.namprd11.prod.outlook.com>
References: <20210518104232.5431-1-kiran.k@intel.com>
 <20210518104232.5431-5-kiran.k@intel.com>
 <C925CD9C-0CD3-493B-8A5B-42025D2655E6@holtmann.org>
In-Reply-To: <C925CD9C-0CD3-493B-8A5B-42025D2655E6@holtmann.org>
Accept-Language: en-US
Content-Language: en-US
dlp-reaction: no-action
dlp-version: 11.5.1.3
dlp-product: dlpe-windows
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?cc/1f651osEaytWlHHpAGske9F3exLZMomK9Xl4253ewrF7XYdanSQjicec0?=
 =?us-ascii?Q?SeFCubAS32OASGWG5mpzr0vkg9JIOEYJhMUcaKQy509sUclYZxadQOs6nvOe?=
 =?us-ascii?Q?dUCqTbknXDLeEeWYWX7Qqyrg4QIJfYege8EobgKluUYQJDpNxy6RUjhFXVw9?=
 =?us-ascii?Q?tn3CqIbJsfRYrcfmhr6NXLa+EgRQZJbvJGCjCTIOWcIsWKRMeVTrQKab3HK/?=
 =?us-ascii?Q?RkAMeKgTwtNkL0HsJX9YnclAG9YLHttiQ9fDI/tLFOZHiKRLEAjhu5K6WUaf?=
 =?us-ascii?Q?zZEaN9JRZFEgZqOQ3TeNyXXZrrWuOD0dAqw7xmVq7bmDkizw/HEFEypbNqTQ?=
 =?us-ascii?Q?N7cn/EZS5vahdPgr91jwgYKDOHM4wO75F3a3oweqWLduPjWk+mPzuGRbkN9a?=
 =?us-ascii?Q?6OdMrYCxwjSCnlWusqFddzdUPGF40VH7INHg8l5T5sTYq4TUUg2Xo2jhk4k7?=
 =?us-ascii?Q?GBDs6gBtnl+ZUNerZwDSUA+QxQR2AXK6ntbDXLWxQOcSuVMS+m62cGhFVPtZ?=
 =?us-ascii?Q?/yhJEdcTQcPtn+QBJtWIH203FZsdq2xgj5Vj+om6SK4rLAXhQx9d2m6OcEjw?=
 =?us-ascii?Q?jTZrtirFQELQ6Cif9p+VaYZY2RzAfw0tjk/kgtUoKODPsljJ8/Oy0tNGMhkQ?=
 =?us-ascii?Q?9tao4jRrnVwsOAVQBPZVn6SR2dyKBOBPlInhLdtF+kUGv13Jmrv7pfdpyWpi?=
 =?us-ascii?Q?t/OgnvkfYKiSH5rvCchn1XcixFfxbH5S9TkO8nG79tWIdHDBAAqcD0mJSYZy?=
 =?us-ascii?Q?e2Xb7IgM7KYri+/MTAS6/HewOzq2KDyorL5fJYAkJvQfONGH68d/7+Wtcdo7?=
 =?us-ascii?Q?ay9F1IYe1Q5+8v0PySEjNiVUoZ4cT0q9TiLFhxqkm/NEBDgWMtM7LVu73RBl?=
 =?us-ascii?Q?uCRcyT1yAAoWpAEN0Kyv9h/nfCGwI0G+zPTsjTD+W/tvxnNwPJzWhgQGAOZR?=
 =?us-ascii?Q?XmgIwgpjY7irYNIXzEfFslxYp2KduqE+U7n1U18JikrJYxohkeDmGtMB78e+?=
 =?us-ascii?Q?0n+TMylox85zBxHa5zI6xrsxcMU+1ikQsLLGDxspvwwFip+TmTmFU17Faf/n?=
 =?us-ascii?Q?NXNqQ92rBNffBC45PjfBdCLV49XV6D3jvotR6AcmEdnE4mhJ/O7JiLciDYpH?=
 =?us-ascii?Q?q/53LwpbSqUnHGz7m3bN+Gz0WVle4w9XqCTmhi6/3TD+XbCvNql/gK56Qvra?=
 =?us-ascii?Q?4wI+CsfHPTK4zpOxi/GSOgPjD55W0GdiBBru0MldEUqCChalVqBlXso7w9r0?=
 =?us-ascii?Q?nRVjAN4+M3HbgxB+7LYZAoUVdICVkMoQAZIheFAQiDI5/A3zjcwOkGToDhPa?=
 =?us-ascii?Q?NwfC9Z8e7rgUz3eMSMgWTUQf?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM8PR11MB5573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 331c9a38-eefd-4d48-1d9a-08d92a752eca
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Jun 2021 12:01:43.3626
 (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: IX3+VPiyQRfoEDwRAQTRlg3mebsL/tlPsJOpE3qL1V6D0IP6bOBYYxyr0PopwJE3R/XD/65NZfKirSAbvq/THA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR11MB5653
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-bluetooth.vger.kernel.org>
X-Mailing-List: linux-bluetooth@vger.kernel.org

Hi Marcel,

> >
> > #define BT_SCM_PKT_STATUS	0x03
> >
> > +#define BT_CODEC	19
> > +
> > +struct	bt_codec_caps {
> > +	__u8	len;
> > +	__u8	data[];
> > +} __packed;
> > +
> > +struct bt_codec {
> > +	__u8	id;
> > +	__le16	cid;
> > +	__le16	vid;
> > +	__u8	data_path;
> > +	__u8	num_caps;
> > +} __packed;
> > +
> > +struct bt_codecs {
> > +	__u8		num_codecs;
> > +	struct bt_codec	codecs[];
> > +} __packed;
> > +
>=20
> what is encapsulating what here?

bt_codec encapsulates caps.
bt_codecs  is aggregation of bt_codec.

>=20
> > __printf(1, 2)
> > void bt_info(const char *fmt, ...);
> > __printf(1, 2)
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 731d48ca873a..9658600ae5de 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -2626,6 +2626,10 @@ static inline struct hci_sco_hdr
> *hci_sco_hdr(const struct sk_buff *skb)
> > #define hci_iso_data_len(h)		((h) & 0x3fff)
> > #define hci_iso_data_flags(h)		((h) >> 14)
> >
> > +/* codec transport types */
> > +#define TRANSPORT_BREDR		0x00
> > +#define TRANSPORT_SCO_ESCO	0x01
> > +
>=20
> This is confusing. SCO_ESCO is also BREDR.

Ack. I will remove the suffix BREDR.

>=20
> > /* le24 support */
> > static inline void hci_cpu_to_le24(__u32 val, __u8 dst[3]) { diff
> > --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index
> > 3bd41563f118..d66293d1cba4 100644
> > --- a/net/bluetooth/sco.c
> > +++ b/net/bluetooth/sco.c
> > @@ -948,6 +948,11 @@ static int sco_sock_getsockopt(struct socket *sock=
,
> int level, int optname,
> > 	struct bt_voice voice;
> > 	u32 phys;
> > 	int pkt_status;
> > +	struct codec_list *c;
> > +	u8 num_codecs, i, __user *ptr;
> > +	struct hci_dev *hdev;
> > +	struct hci_codec_caps *caps;
> > +	__u8	data_path;
> >
> > 	BT_DBG("sk %p", sk);
> >
> > @@ -1012,6 +1017,110 @@ static int sco_sock_getsockopt(struct socket
> *sock, int level, int optname,
> > 			err =3D -EFAULT;
> > 		break;
> >
> > +	case BT_CODEC:
> > +		num_codecs =3D 0;
> > +		len =3D 0;
> > +
> > +		hdev =3D hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src,
> > +BDADDR_BREDR);
> > +
>=20
> Remove extra empty line.

Ack

>=20
> > +		if (!hdev) {
> > +			err =3D -EBADFD;
> > +			break;
> > +		}
> > +
> > +		if
> (!test_bit(HCI_QUIRK_HFP_OFFLOAD_CODECS_SUPPORTED, &hdev->quirks))
> {
> > +			err =3D -EOPNOTSUPP;
> > +			break;
> > +		}
> > +
> > +		if (!hdev->get_data_path_id) {
> > +			err =3D -EOPNOTSUPP;
> > +			break;
> > +		}
>=20
> See it is pointless to check a quirk, you can use the callback for it.
>=20

Ack

> > +
> > +		hci_dev_lock(hdev);
> > +		list_for_each_entry(c, &hdev->local_codecs, list) {
> > +			if (c->transport !=3D TRANSPORT_SCO_ESCO)
> > +				continue;
> > +			num_codecs++;
> > +			for (i =3D 0, caps =3D c->caps; i < c->num_caps; i++) {
> > +				len +=3D 1 + caps->len;
> > +				caps =3D (void *)&caps->data[caps->len];
> > +			}
> > +			len +=3D sizeof(struct bt_codec);
> > +		}
> > +		hci_dev_unlock(hdev);
> > +
> > +		if (len > 255) {
> > +			err =3D -ENOMEM;
> > +			break;
>=20
> I think this is the wrong error code. You need to indicate that size is w=
rong.
> ENOMEM really means that memory allocation failed.

I will change error code to ENOBUFS
>=20

Thanks,
Kiran