On Mon, Mar 13, 2023 at 08:10:28PM +0530, Neeraj Sanjay Kale wrote:
> This adds a driver based on serdev driver for the NXP BT serial protocol
> based on running H:4, which can enable the built-in Bluetooth device
> inside an NXP BT chip.
>
> This driver has Power Save feature that will put the chip into sleep state
> whenever there is no activity for 2000ms, and will be woken up when any
> activity is to be initiated over UART.
>
> This driver enables the power save feature by default by sending the vendor
> specific commands to the chip during setup.
>
> During setup, the driver checks if a FW is already running on the chip
> by waiting for the bootloader signature, and downloads device specific FW
> file into the chip over UART if bootloader signature is received..
>
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
Hi,
some minor feedback from my side.
...
> +/* for legacy chipsets with V1 bootloader */
> +static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> + struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> + struct btnxpuart_data *nxp_data = nxpdev->nxp_data;
> + struct v1_data_req *req;
> + u32 requested_len;
> +
> + if (!process_boot_signature(nxpdev))
> + goto ret;
> +
> + req = (struct v1_data_req *)skb_pull_data(skb, sizeof(struct v1_data_req));
> + if (!req)
> + goto ret;
> +
> + if ((req->len ^ req->len_comp) != 0xffff) {
> + bt_dev_dbg(hdev, "ERR: Send NAK");
> + nxp_send_ack(NXP_NAK_V1, hdev);
> + goto ret;
> + }
> + nxp_send_ack(NXP_ACK_V1, hdev);
> +
> + if (nxp_data->fw_dnld_use_high_baudrate) {
> + 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 (nxp_request_firmware(hdev, nxp_data->fw_name))
> + goto ret;
> +
> + requested_len = req->len;
> + if (requested_len == 0) {
> + bt_dev_dbg(hdev, "FW Downloaded Successfully: %zu bytes", nxpdev->fw->size);
> + clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
> + wake_up_interruptible(&nxpdev->fw_dnld_done_wait_q);
> + goto ret;
> + }
> + if (requested_len & 0x01) {
> + /* The CRC did not match at the other end.
> + * Simply send the same bytes again.
> + */
> + requested_len = nxpdev->fw_v1_sent_bytes;
> + bt_dev_dbg(hdev, "CRC error. Resend %d bytes of FW.", requested_len);
> + } else {
> + nxpdev->fw_dnld_v1_offset += nxpdev->fw_v1_sent_bytes;
> +
> + /* The FW bin file is made up of many blocks of
> + * 16 byte header and payload data chunks. If the
> + * FW has requested a header, read the payload length
> + * info from the header, before sending the header.
> + * In the next iteration, the FW should request the
> + * payload data chunk, which should be equal to the
> + * payload length read from header. If there is a
> + * mismatch, clearly the driver and FW are out of sync,
> + * and we need to re-send the previous header again.
> + */
> + if (requested_len == nxpdev->fw_v1_expected_len) {
> + if (requested_len == HDR_LEN)
> + nxpdev->fw_v1_expected_len = nxp_get_data_len(nxpdev->fw->data +
> + nxpdev->fw_dnld_v1_offset);
> + else
> + nxpdev->fw_v1_expected_len = HDR_LEN;
> + } else if (requested_len == HDR_LEN) {
> + /* FW download out of sync. Send previous chunk again */
> + nxpdev->fw_dnld_v1_offset -= nxpdev->fw_v1_sent_bytes;
> + nxpdev->fw_v1_expected_len = HDR_LEN;
> + }
> + }
> +
> + if (nxpdev->fw_dnld_v1_offset + requested_len <= nxpdev->fw->size)
> + serdev_device_write_buf(nxpdev->serdev,
> + nxpdev->fw->data + nxpdev->fw_dnld_v1_offset,
> + requested_len);
> + nxpdev->fw_v1_sent_bytes = requested_len;
> +
> +ret:
nit: I think it would be more intuitive to call this label free_skb.
Likewise elsewhere.
> + kfree_skb(skb);
> + return 0;
> +}
...
> +static int nxp_set_ind_reset(struct hci_dev *hdev, void *data)
> +{
> + struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> + struct sk_buff *skb;
> + u8 *status;
> + u8 pcmd = 0;
> + int err;
> +
> + skb = nxp_drv_send_cmd(hdev, HCI_NXP_IND_RESET, 1, &pcmd);
> + if (IS_ERR(skb))
> + return PTR_ERR(skb);
> +
> + status = skb_pull_data(skb, 1);
> + if (status) {
> + if (*status == 0) {
> + set_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
> + err = nxp_download_firmware(hdev);
> + if (err < 0)
> + return err;
Does this leak skb?
> + serdev_device_set_baudrate(nxpdev->serdev, nxpdev->fw_init_baudrate);
> + nxpdev->current_baudrate = nxpdev->fw_init_baudrate;
> + if (nxpdev->current_baudrate != HCI_NXP_SEC_BAUDRATE) {
> + nxpdev->new_baudrate = HCI_NXP_SEC_BAUDRATE;
> + nxp_set_baudrate_cmd(hdev, NULL);
> + }
> + hci_cmd_sync_queue(hdev, send_wakeup_method_cmd, NULL, NULL);
> + hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL);
> + }
> + }
> + 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;
> + struct psmode_cmd_payload ps_parm;
> + struct wakeup_cmd_payload wakeup_parm;
> + __le32 baudrate_parm;
> +
> + /* 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.
> + * In case the payload for any command does not match expected payload
> + * length, let the firmware and user space program handle it, or throw
> + * an error.
> + */
> + if (bt_cb(skb)->pkt_type == HCI_COMMAND_PKT && !psdata->driver_sent_cmd) {
> + hdr = (struct hci_command_hdr *)skb->data;
> + if (hdr->plen != (skb->len - HCI_COMMAND_HDR_SIZE))
> + goto send_skb;
> +
> + switch (__le16_to_cpu(hdr->opcode)) {
> + case HCI_NXP_AUTO_SLEEP_MODE:
> + if (hdr->plen == sizeof(ps_parm)) {
> + memcpy(&ps_parm, skb->data + HCI_COMMAND_HDR_SIZE, hdr->plen);
> + if (ps_parm.ps_cmd == BT_PS_ENABLE)
> + psdata->target_ps_mode = PS_MODE_ENABLE;
> + else if (ps_parm.ps_cmd == BT_PS_DISABLE)
> + psdata->target_ps_mode = PS_MODE_DISABLE;
> + psdata->c2h_ps_interval = __le16_to_cpu(ps_parm.c2h_ps_interval);
> + hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL);
> + goto free_skb;
> + }
> + break;
> + case HCI_NXP_WAKEUP_METHOD:
> + if (hdr->plen == sizeof(wakeup_parm)) {
> + memcpy(&wakeup_parm, skb->data + HCI_COMMAND_HDR_SIZE, hdr->plen);
> + psdata->c2h_wakeupmode = wakeup_parm.c2h_wakeupmode;
> + psdata->c2h_wakeup_gpio = wakeup_parm.c2h_wakeup_gpio;
> + psdata->h2c_wakeup_gpio = wakeup_parm.h2c_wakeup_gpio;
> + switch (wakeup_parm.h2c_wakeupmode) {
> + case BT_CTRL_WAKEUP_METHOD_DSR:
> + psdata->h2c_wakeupmode = WAKEUP_METHOD_DTR;
> + break;
> + case BT_CTRL_WAKEUP_METHOD_BREAK:
> + default:
> + psdata->h2c_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 == sizeof(baudrate_parm)) {
> + memcpy(&baudrate_parm, skb->data + HCI_COMMAND_HDR_SIZE, hdr->plen);
> + nxpdev->new_baudrate = __le32_to_cpu(baudrate_parm);
> + 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;
> + }
> + }
...
> +send_skb:
> + /* 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;
nit: I think it would be nicer to simply return 0 here.
And remove the ret label entirely.
> +}
...
