All of lore.kernel.org
 help / color / mirror / Atom feed
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
Date: Tue, 8 Jun 2021 12:01:43 +0000	[thread overview]
Message-ID: <DM8PR11MB5573537BBBC5EFEDAC5B6DCEF5379@DM8PR11MB5573.namprd11.prod.outlook.com> (raw)
In-Reply-To: <C925CD9C-0CD3-493B-8A5B-42025D2655E6@holtmann.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;
> > +
> 
> what is encapsulating what here?

bt_codec encapsulates caps.
bt_codecs  is aggregation of bt_codec.

> 
> > __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
> > +
> 
> This is confusing. SCO_ESCO is also BREDR.

Ack. I will remove the suffix BREDR.

> 
> > /* 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 = -EFAULT;
> > 		break;
> >
> > +	case BT_CODEC:
> > +		num_codecs = 0;
> > +		len = 0;
> > +
> > +		hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src,
> > +BDADDR_BREDR);
> > +
> 
> Remove extra empty line.

Ack

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

Ack

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

I will change error code to ENOBUFS
> 

Thanks,
Kiran


  reply	other threads:[~2021-06-08 12:01 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-18 10:42 [PATCH v8 1/9] Bluetooth: enumerate local supported codec and cache details Kiran K
2021-05-18 10:42 ` [PATCH v8 2/9] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
2021-06-03 14:24   ` Marcel Holtmann
2021-05-18 10:42 ` [PATCH v8 3/9] Bluetooth: btintel: Add a quirk for hfp offload usecase Kiran K
2021-06-03 14:26   ` Marcel Holtmann
2021-05-18 10:42 ` [PATCH v8 4/9] Bluetooth: btitnel: Add a callback function to retireve data path Kiran K
2021-06-03 14:29   ` Marcel Holtmann
2021-06-08 11:56     ` K, Kiran
2021-05-18 10:42 ` [PATCH v8 5/9] Bluetooth: Add BT_CODEC option for getsockopt for SCO socket Kiran K
2021-06-03 14:35   ` Marcel Holtmann
2021-06-08 12:01     ` K, Kiran [this message]
2021-05-18 10:42 ` [PATCH v8 6/9] Bluetooth: btintel: Add a callback function to configure data path Kiran K
2021-06-03 14:30   ` Marcel Holtmann
2021-05-18 10:42 ` [PATCH v8 7/9] Bluetooth: Add BT_CODEC option for setsockopt over SCO Kiran K
2021-06-03 14:37   ` Marcel Holtmann
2021-05-18 10:42 ` [PATCH v8 8/9] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command Kiran K
2021-06-03 14:39   ` Marcel Holtmann
2021-06-08 12:06     ` K, Kiran
2021-05-18 10:42 ` [PATCH v8 9/9] Bluetooth: Add support for msbc coding format Kiran K
2021-05-18 11:12 ` [v8,1/9] Bluetooth: enumerate local supported codec and cache details bluez.test.bot
2021-06-03 14:23 ` [PATCH v8 1/9] " Marcel Holtmann
2021-06-08 11:54   ` K, Kiran

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM8PR11MB5573537BBBC5EFEDAC5B6DCEF5379@DM8PR11MB5573.namprd11.prod.outlook.com \
    --to=kiran.k@intel.com \
    --cc=chethan.tumkur.narayan@intel.com \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=marcel@holtmann.org \
    --cc=ravishankar.srivatsa@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.