Hi Luiz,
Thanks for the comments. I will make the suggested changes in next patch set - v5.
> > +static void hci_read_supported_codecs(struct hci_dev *hdev) {
> > + struct sk_buff *skb;
> > + __u8 num_codecs;
> > +
> > + skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_CODECS, 0, NULL,
> > + HCI_CMD_TIMEOUT);
> > +
> > + if (IS_ERR(skb)) {
> > + bt_dev_err(hdev, "Failed to read local supported codecs (%ld)",
> > + PTR_ERR(skb));
> > + return;
> > + }
> > +
> > + if (skb->len < 1 || skb->data[0])
> > + goto error;
> > +
> > + skb_pull(skb, 1);
>
> We better use a skb_pull with a sizeof of the struct we expect, here you are
> probably reading the status but for someone else might not understand what
> you doing here.
>
Ack
> > +
> > + if (skb->len < 1)
> > + goto error;
> > +
> > + /* enumerate standard codecs */
> > + num_codecs = skb->data[0];
> > +
> > + skb_pull(skb, 1);
>
> Ditto, use skb_pull(sbk, sizeof(num_codecs)).
>
> > +
> > + if (num_codecs && skb->len < num_codecs)
> > + goto error;
>
> The check above might be easier if we use flex_array_size so we perform the
> check for the entire array, and then on the while loop you just access each
> element by index like Im doing in the patch bellow:
>
> https://patchwork.kernel.org/project/bluetooth/patch/20210419171257.386
> 5181-11-luiz.dentz@xxxxxxxxx/
Ack
>
> > + while (num_codecs--) {
> > + hci_read_codec_capabilities(hdev, skb->data,
> LOCAL_CODEC_ACL,
> > + false);
> > + skb_pull(skb, 1);
> > + }
>
> Same thing here.
>
> > + /* enumerate vendor specific codecs */
> > + if (skb->len < 1)
> > + goto error;
> > +
> > + num_codecs = skb->data[0];
> > +
> > + skb_pull(skb, 1);
> > +
> > + if (num_codecs && skb->len < (num_codecs * 4))
> > + goto error;
> > +
> > + while (num_codecs--) {
> > + hci_read_codec_capabilities(hdev, skb->data,
> LOCAL_CODEC_ACL,
> > + true);
> > + skb_pull(skb, 4);
> > + }
>
> Btw, the code for reading vendor and standard seems exactly the same so
> perhaps it is worth moving these lines above under another function e.g.
> hci_codec_list_parse which can then take a boolean saying is vendor or not
> and from there call hci_read_codec_capabilities.
>
Ack
> > +error:
> > + kfree_skb(skb);
> > +}
> > +
> > +static void hci_init5_req(struct hci_dev *hdev) {
> > + /* Read local codec list if the HCI command is supported */
> > + if (hdev->commands[29] & 0x20)
> > + hci_read_supported_codecs(hdev); }
> > +
> > static int __hci_init(struct hci_dev *hdev) {
> > int err;
> > @@ -937,6 +1040,8 @@ static int __hci_init(struct hci_dev *hdev)
> > if (err < 0)
> > return err;
> >
> > + hci_init5_req(hdev);
> > +
> > /* This function is only called when the controller is actually in
> > * configured state. When the controller is marked as unconfigured,
> > * this initialization procedure is not run.
> > @@ -3559,6 +3664,35 @@ void hci_conn_params_clear_disabled(struct
> hci_dev *hdev)
> > BT_DBG("All LE disabled connection parameters were removed");
> > }
> >
> > +int hci_codec_list_add(struct list_head *list, struct
> hci_rp_read_local_codec_caps *rp,
> > + __u32 len,
> > + struct hci_op_read_local_codec_caps *sent) {
> > + struct codec_list *entry;
> > +
> > + entry = kzalloc(sizeof(*entry) + len, GFP_KERNEL);
> > + if (!entry)
> > + return -ENOMEM;
> > +
> > + memcpy(entry->codec_id, sent->codec_id, 5);
> > + entry->transport = sent->transport;
> > + entry->num_caps = rp->num_caps;
> > + if (rp->num_caps)
> > + memcpy(entry->caps, rp->caps, len);
> > + list_add(&entry->list, list);
> > +
> > + return 0;
> > +}
> > +
> > +void hci_codec_list_clear(struct list_head *codec_list) {
> > + struct codec_list *c, *n;
> > +
> > + list_for_each_entry_safe(c, n, codec_list, list) {
> > + list_del(&c->list);
> > + kfree(c);
> > + }
> > +}
> > /* This function requires the caller holds hdev->lock */ static void
> > hci_conn_params_clear_all(struct hci_dev *hdev) { @@ -3818,6 +3952,7
> > @@ struct hci_dev *hci_alloc_dev(void)
> > INIT_LIST_HEAD(&hdev->conn_hash.list);
> > INIT_LIST_HEAD(&hdev->adv_instances);
> > INIT_LIST_HEAD(&hdev->blocked_keys);
> > + INIT_LIST_HEAD(&hdev->local_codecs);
> >
> > INIT_WORK(&hdev->rx_work, hci_rx_work);
> > INIT_WORK(&hdev->cmd_work, hci_cmd_work); @@ -4036,6 +4171,7
> > @@ void hci_unregister_dev(struct hci_dev *hdev)
> > hci_conn_params_clear_all(hdev);
> > hci_discovery_filter_clear(hdev);
> > hci_blocked_keys_clear(hdev);
> > + hci_codec_list_clear(&hdev->local_codecs);
> > hci_dev_unlock(hdev);
> >
> > hci_dev_put(hdev);
> > --
> > 2.17.1
> >
>
>
> --
> Luiz Augusto von Dentz
Thanks,
Kiran
