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
next prev parent 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.