From: Marcel Holtmann <marcel@holtmann.org>
To: Kiran K <kiran.k@intel.com>
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH v9 05/10] Bluetooth: Add BT_CODEC option for getsockopt for SCO socket
Date: Tue, 15 Jun 2021 21:37:20 +0200 [thread overview]
Message-ID: <D0153D80-4C8C-47BF-9455-865D96649268@holtmann.org> (raw)
In-Reply-To: <20210608122455.19583-5-kiran.k@intel.com>
Hi Kiran,
> Add BT_CODEC option for getsockopt systemcall over SCO socket
> to expose the codecs supported by controller
>
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> * changes on v9:
> - fix typos,review comments, remove quirk
>
> include/net/bluetooth/bluetooth.h | 20 ++++++
> include/net/bluetooth/hci.h | 4 ++
> net/bluetooth/sco.c | 111 +++++++++++++++++++++++++++++-
> 3 files changed, 134 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 9125effbf448..1840756958ce 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -153,6 +153,26 @@ struct bt_voice {
>
> #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;
> +
> __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 45bd9af4ce61..31a5ac8918fc 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -2619,6 +2619,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_ACL 0x00
> +#define TRANSPORT_SCO_ESCO 0x01
> +
> /* 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 d9a4e88dacbb..98d5e24e5680 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -944,10 +944,15 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
> char __user *optval, int __user *optlen)
> {
> struct sock *sk = sock->sk;
> - int len, err = 0;
> + int len, err = 0, buf_len;
> 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;
> + buf_len = 0;
> +
> + hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src, BDADDR_BREDR);
> + if (!hdev) {
> + err = -EBADFD;
> + break;
> + }
> +
> + if (!hdev->get_data_path) {
> + err = -EOPNOTSUPP;
> + break;
> + }
> +
> + hci_dev_lock(hdev);
> + /* find total buffer size required to copy codec + caps */
please check for simple style mistakes like double spaces. Also I would put the comment above the hci_dev_lock().
> + 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++) {
> + buf_len += 1 + caps->len;
> + caps = (void *)&caps->data[caps->len];
> + }
> + buf_len += sizeof(struct bt_codec);
> + }
> + hci_dev_unlock(hdev);
> +
> + buf_len += sizeof(struct bt_codecs);
> + if (buf_len > len) {
> + err = -ENOBUFS;
> + break;
> + }
> + ptr = optval;
> +
> + if (put_user(num_codecs, ptr)) {
> + err = -EFAULT;
> + break;
> + }
> + ptr += sizeof(num_codecs);
> +
And this is missing comment as well.
> + hci_dev_lock(hdev);
> + list_for_each_entry(c, &hdev->local_codecs, list) {
> + if (c->transport != TRANSPORT_SCO_ESCO)
> + continue;
> +
> + if (put_user(c->id, ptr)) {
> + err = -EFAULT;
> + goto unlock;
> + }
> + ptr += sizeof(c->id);
> +
> + if (put_user(c->cid, ptr)) {
> + err = -EFAULT;
> + goto unlock;
> + }
> + ptr += sizeof(c->cid);
> +
> + if (put_user(c->vid, ptr)) {
> + err = -EFAULT;
> + goto unlock;
> + }
> + ptr += sizeof(c->vid);
> +
> + err = hdev->get_data_path(hdev);
> + if (err < 0) {
> + err = -EFAULT;
> + goto unlock;
> + }
Using the variable name err is really bad here. It is also not an EFAULT type of error. They are really specific. I really don’t get why not prepare the data in advance and have a single put_user call.
> +
> + data_path = (__u8)err;
> + if (put_user(data_path, ptr)) {
> + err = -EFAULT;
> + goto unlock;
> + }
> + ptr += sizeof(data_path);
> +
> + if (put_user(c->num_caps, ptr)) {
> + err = -EFAULT;
> + goto unlock;
> + }
> + ptr += sizeof(c->num_caps);
> +
> + len = 0;
> + for (i = 0, caps = c->caps; i < c->num_caps; i++) {
> + len += 1 + caps->len;
> + caps = (void *)&caps->data[caps->len];
> + }
> +
> + if (len && copy_to_user(ptr, c->caps, len)) {
> + err = -EFAULT;
> + goto unlock;
> + }
> + ptr += len;
> + }
> +
> + if (put_user(buf_len, optlen))
> + err = -EFAULT;
> +unlock:
> + hci_dev_unlock(hdev);
Jumping from within a for-loop is nothing something that I actually like. It is better you break out of via break.
Regards
Marcel
next prev parent reply other threads:[~2021-06-15 19:37 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-08 12:24 [PATCH v9 01/10] Bluetooth: enumerate local supported codec and cache details Kiran K
2021-06-08 12:24 ` [PATCH v9 02/10] Bluetooth: Add support for Read Local Supported Codecs V2 Kiran K
2021-06-08 12:24 ` [PATCH v9 03/10] Bluetooth: Add a callback function to retireve data path Kiran K
2021-06-15 19:26 ` Marcel Holtmann
2021-06-16 2:56 ` K, Kiran
2021-06-16 5:17 ` Marcel Holtmann
2021-06-08 12:24 ` [PATCH v9 04/10] Bluetooth: btintel: set get_data_path callback Kiran K
2021-06-15 19:30 ` Marcel Holtmann
2021-06-16 3:02 ` K, Kiran
2021-06-08 12:24 ` [PATCH v9 05/10] Bluetooth: Add BT_CODEC option for getsockopt for SCO socket Kiran K
2021-06-15 19:37 ` Marcel Holtmann [this message]
2021-06-08 12:24 ` [PATCH v9 06/10] Bluetooth: Add a callback function to set data path Kiran K
2021-06-15 19:37 ` Marcel Holtmann
2021-06-08 12:24 ` [PATCH v9 07/10] Bluetooth: btintel: define callback " Kiran K
2021-06-15 19:39 ` Marcel Holtmann
2021-06-16 3:10 ` K, Kiran
2021-06-16 5:18 ` Marcel Holtmann
2021-06-17 7:53 ` K, Kiran
2021-06-17 10:19 ` Marcel Holtmann
2021-06-23 3:21 ` K, Kiran
2021-06-08 12:24 ` [PATCH v9 08/10] Bluetooth: Add BT_CODEC option for setsockopt over SCO Kiran K
2021-06-08 12:24 ` [PATCH v9 09/10] Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command Kiran K
2021-06-08 12:24 ` [PATCH v9 10/10] Bluetooth: Add support for msbc coding format Kiran K
2021-06-15 19:43 ` Marcel Holtmann
2021-06-23 3:24 ` K, Kiran
2021-06-08 13:39 ` [v9,01/10] Bluetooth: enumerate local supported codec and cache details bluez.test.bot
2021-06-08 18:49 ` [PATCH v9 01/10] " kernel test robot
2021-06-08 18:49 ` kernel test robot
2021-06-15 19:25 ` Marcel Holtmann
2021-06-16 2:51 ` K, Kiran
2021-06-16 5:15 ` Marcel Holtmann
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=D0153D80-4C8C-47BF-9455-865D96649268@holtmann.org \
--to=marcel@holtmann.org \
--cc=kiran.k@intel.com \
--cc=linux-bluetooth@vger.kernel.org \
/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.