Re: [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets

[Neeraj sanjay kale]

Hi Luiz,

Thank you for reviewing this patch. I have resolved all the comments in V5 patch.

> > +static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +       struct v3_data_req *req = skb_pull_data(skb, sizeof(struct
> v3_data_req));
> > +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> > +
> > +       if (!req || !nxpdev || !nxpdev->fw)
> > +               goto ret;
> > +
> > +       if (!test_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state))
> > +               goto ret;
> > +
> > +       nxp_send_ack(NXP_ACK_V3, hdev);
> > +
> > +       if (!nxpdev->timeout_changed) {
> > +               nxpdev->timeout_changed = nxp_fw_change_timeout(hdev, req-
> >len);
> > +               goto ret;
> > +       }
> > +
> > +       if (!nxpdev->baudrate_changed) {
> > +               nxpdev->baudrate_changed = nxp_fw_change_baudrate(hdev,
> req->len);
> > +               if (nxpdev->baudrate_changed) {
> > +                       serdev_device_set_baudrate(nxpdev->serdev,
> > +                                                  HCI_NXP_SEC_BAUDRATE);
> > +                       serdev_device_set_flow_control(nxpdev->serdev, 1);
> > +                       nxpdev->current_baudrate = HCI_NXP_SEC_BAUDRATE;
> > +               }
> > +               goto ret;
> > +       }
> > +
> > +       if (req->len == 0) {
> > +               bt_dev_info(hdev, "FW Downloaded Successfully: %zu bytes",
> nxpdev->fw->size);
> > +               clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
> > +               wake_up_interruptible(&nxpdev->suspend_wait_q);
> > +               goto ret;
> > +       }
> > +       if (req->error)
> > +               bt_dev_err(hdev, "FW Download received err 0x%02x from chip.
> Resending FW chunk.",
> > +                          req->error);
> > +
> > +       if (req->offset < nxpdev->fw_v3_offset_correction) {
> > +               /* This scenario should ideally never occur.
> > +                * But if it ever does, FW is out of sync and
> > +                * needs a power cycle.
> > +                */
> > +               bt_dev_err(hdev, "Something went wrong during FW download.
> Please power cycle and try again");
> 
> Can't we actually power cycle instead of printing an error?
The NXP chips draw power from the platform's 5V power supply, which is used by WLAN as well as BT sub-system inside the chip. These chips have no mechanism to reset or power-cycle BT only sub-system independently.
> 
> > +               goto ret;
> > +       }
> > +
> > +       serdev_device_write_buf(nxpdev->serdev,
> > +                               nxpdev->fw->data + req->offset - nxpdev-
> >fw_v3_offset_correction,
> > +                               req->len);
> > +
> > +ret:
> > +       kfree_skb(skb);
> > +       return 0;
> > +}
> > +


> > +static int nxp_enqueue(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> > +       struct ps_data *psdata = nxpdev->psdata;
> > +       struct hci_command_hdr *hdr;
> > +       u8 *param;
> > +
> > +       if (!nxpdev || !psdata)
> > +               goto free_skb;
> > +
> > +       /* if vendor commands are received from user space (e.g. hcitool),
> update
> > +        * driver flags accordingly and ask driver to re-send the command to
> FW.
> > +        */
> > +       if (bt_cb(skb)->pkt_type == HCI_COMMAND_PKT && !psdata-
> >driver_sent_cmd) {
> > +               hdr = (struct hci_command_hdr *)skb->data;
> 
> It is not safe to access the contents of skb->data without first
> checking skb->len, I understand you can't use skb_pull_data since that
> changes the packet but Im not so happy with this code either way since
> you appear to be doing this only to support userspace initiating these
> commands but is that really expected or you are just doing this for
> testing purpose? Also why not doing this handling on the command
> complete/command status event as that would be common to both driver
> or userspace initiated?
> 
I have made few changes to handle this issue in a safe way by checking
skb->len and hdr->plen before using the parameters.
We do need to parse a couple of user space vendor commands before forwarding
them to the FW, since the driver needs to update its internal flags and mechanism
accordingly. We do not usually get the parameters while handling command complete
or command status events.
In one of the previous patches I was parsing parameters in nxp_enqueue, and updating
driver flags in ps_check_event_packet() on status success.
https://patchwork.kernel.org/project/bluetooth/patch/1669207413-9637-1-git-send-email-neeraj.sanjaykale@xxxxxxx/

> 
> > +               param = skb->data + HCI_COMMAND_HDR_SIZE;
> > +               switch (__le16_to_cpu(hdr->opcode)) {
> > +               case HCI_NXP_AUTO_SLEEP_MODE:
> > +                       if (hdr->plen >= 1) {
> > +                               if (param[0] == BT_PS_ENABLE)
> > +                                       psdata->ps_mode = PS_MODE_ENABLE;
> > +                               else if (param[0] == BT_PS_DISABLE)
> > +                                       psdata->ps_mode = PS_MODE_DISABLE;
> > +                               hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL);
> > +                               goto free_skb;
> > +                       }
> > +                       break;
> > +               case HCI_NXP_WAKEUP_METHOD:
> > +                       if (hdr->plen >= 4) {
> > +                               switch (param[2]) {
> > +                               case BT_CTRL_WAKEUP_METHOD_DSR:
> > +                                       psdata->wakeupmode = WAKEUP_METHOD_DTR;
> > +                                       break;
> > +                               case BT_CTRL_WAKEUP_METHOD_BREAK:
> > +                               default:
> > +                                       psdata->wakeupmode = WAKEUP_METHOD_BREAK;
> > +                                       break;
> > +                               }
> > +                               hci_cmd_sync_queue(hdev, send_wakeup_method_cmd,
> NULL, NULL);
> > +                               goto free_skb;
> > +                       }
> > +                       break;
> > +               case HCI_NXP_SET_OPER_SPEED:
> > +                       if (hdr->plen == 4) {
> > +                               nxpdev->new_baudrate = *((u32 *)param);
> > +                               hci_cmd_sync_queue(hdev, nxp_set_baudrate_cmd,
> NULL, NULL);
> > +                               goto free_skb;
> > +                       }
> > +                       break;
> > +               case HCI_NXP_IND_RESET:
> > +                       if (hdr->plen == 1) {
> > +                               hci_cmd_sync_queue(hdev, nxp_set_ind_reset, NULL,
> NULL);
> > +                               goto free_skb;
> > +                       }
> > +                       break;
> > +               default:
> > +                       break;
> > +               }
> > +       }
> > +
> > +       /* Prepend skb with frame type */
> > +       memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
> > +       skb_queue_tail(&nxpdev->txq, skb);
> > +
> > +       btnxpuart_tx_wakeup(nxpdev);
> > +ret:
> > +       return 0;
> > +
> > +free_skb:
> > +       kfree_skb(skb);
> > +       goto ret;
> > +}
> > +

Thanks,
Neeraj