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

[Neeraj sanjay kale]

Hi Ilpo,

Thank you for reviewing this patch. I have resolved most of your review comments in v7 patch, and I have some clarification inline below:

> > +
> > +struct ps_data {
> > +     u8    ps_mode;
> > +     u8    cur_psmode;
> > +     u8    ps_state;
> 
> I tried to follow the code around these variables but its turned out too hard
> for me... Should ps_mode perhaps be renamed to target_psmode?
> 
Renamed to target_psmode since it makes more sense. I have also added a comment for few confusing variables.

> Should nxp_enqueue() check if ps_mode was changed or not before calling
> hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL); ?
No. If it does, then the user space tool (hcitool) stays in waiting state for a response.
Our intension here in nxp_enqueue() is to simple parse the vendor commands coming from user space (only), and cache
few important contents of the command parameters in driver, and pass on the command, as it is, to the FW.
When FW replies with a  success response, we let the cached values in driver take effect.

> > +static void nxp_send_ack(u8 ack, struct hci_dev *hdev) {
> > +     struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> > +     u8 ack_nak[2];
> > +
> > +     if (ack == NXP_ACK_V1 || ack == NXP_NAK_V1) {
> > +             ack_nak[0] = ack;
> > +             serdev_device_write_buf(nxpdev->serdev, ack_nak, 1);
> > +     } else if (ack == NXP_ACK_V3) {
> > +             ack_nak[0] = ack;
> > +             ack_nak[1] = crc8(crc8_table, ack_nak, 1, 0xff);
> > +             serdev_device_write_buf(nxpdev->serdev, ack_nak, 2);
> > +     }
> 
> This could be done as:
> 
>         int len = 1;
> 
>         ack_nak[0] = ack;
>         if (ack == NXP_ACK_V3) {
>                 len = 2;
>                 ack_nak[1] = crc8(crc8_table, ack_nak, 1, 0xff);
>         }
>         serdev_device_write_buf(nxpdev->serdev, ack_nak, len);
> 
> or calculate the len with NXP_ACK_V3 ? 2 : 1 on the func call line.
I have replaced this code with what you initially suggested here.

> > +
> > +static bool nxp_fw_change_baudrate(struct hci_dev *hdev, u16 req_len)
> > +{
> > +     struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> > +     struct nxp_bootloader_cmd nxp_cmd5;
> > +     struct uart_config uart_config;
> > +
> > +     if (req_len == sizeof(nxp_cmd5)) {
> > +             nxp_cmd5.header = __cpu_to_le32(5);
> > +             nxp_cmd5.arg = 0;
> > +             nxp_cmd5.payload_len = __cpu_to_le32(sizeof(uart_config));
> > +             nxp_cmd5.crc = swab32(crc32_be(0UL, (char *)&nxp_cmd5,
> > +                                            sizeof(nxp_cmd5) - 4));
> 
> swab32(crc32_be(...)) seems and odd construct instead of __cpu_to_le32().
Earlier I had tried using __cpu_to_le32() but that did not work. The FW expects a swapped
CRC value for it's header and payload data.

> 
> > +     serdev_device_write_buf(nxpdev->serdev, (u8 *)&nxp_cmd7,
> > + req_len);
> 
> Is it safe to assume req_len is small enough to not leak stack content?
The chip requests chunk of FW data which is never more than 2048 bytes at a time.

> 
> > +static bool nxp_check_boot_sign(struct btnxpuart_dev *nxpdev) {
> > +     int ret;
> > +
> > +     serdev_device_set_baudrate(nxpdev->serdev,
> HCI_NXP_PRI_BAUDRATE);
> > +     serdev_device_set_flow_control(nxpdev->serdev, 0);
> > +     set_bit(BTNXPUART_CHECK_BOOT_SIGNATURE, &nxpdev->tx_state);
> > +
> > +     ret = wait_event_interruptible_timeout(nxpdev-
> >check_boot_sign_wait_q,
> > +                                            !test_bit(BTNXPUART_CHECK_BOOT_SIGNATURE,
> > +                                                      &nxpdev->tx_state),
> > +                                            msecs_to_jiffies(1000));
> > +     if (ret == 0)
> > +             return false;
> > +     else
> > +             return true;
> 
> How does does this handle -ERESTARTSYS? But this runs in nxp_setup() so is
> that even relevant (I don't know).
This function is waits for 1 second and checks if it is receiving any bootloader signatures
over UART. If yes, it means FW download is needed. If no, it means FW is already present
on the chip, and we skip FW download.


> > +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[MAX_USER_PARAMS];
> > +
> > +     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) {
> 
> Should this !psdata->driver_sent_cmd do something else than end up into a
> place labelled send_skb. Maybe return early (or free skb + return)?
> There's a comment elsewhere stating: "set flag to prevent re-sending
> command in nxp_enqueue."
I'm sorry if the comment was misleading. This flag is set to prevent nxp_enqueue() from
Parsing the command parameters again, and calling hci_cmd_sync_queue() again.
The commands sent from user space, as well as the commands sent by __hci_cmd_sync(),
both endup in nxp_enqueue().
Hope this helps!

> 
> > +             if (!skb->len || skb->len > (MAX_USER_PARAMS +
> HCI_COMMAND_HDR_SIZE))
> > +                     goto send_skb;
> > +
> > +             hdr = (struct hci_command_hdr *)skb->data;
> > +             if (hdr->plen != (skb->len - HCI_COMMAND_HDR_SIZE))
> > +                     goto send_skb;
> 
> Are these two gotos happily sending invalid skbs?
As mentioned earlier, in nxp_enqueue, we simply want to catch few vendor command parameters and cache them
till the FW replies with a success/failure, and forward those commands to the FW as is. Incase there are any invalid
skbs, the FW will send an error response to the user space program.
Freeing any skbs in driver will result in user space tools like hcitool to wait indefinitely for a response.

> 
> > +             memcpy(param, skb->data + HCI_COMMAND_HDR_SIZE, hdr-
> >plen);
> > +             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) {
> > +                             psdata->c2h_wakeupmode = param[0];
> > +                             psdata->c2h_wakeup_gpio = param[1];
> > +                             switch (param[2]) {
> > +                             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 == 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;
> 
> Okay to end up into place labelled send_skb?
Yes, other vendor commands can be send directly to the chip, as we do not need to cache any info from them.

> > +
> > +     if (hci_register_dev(hdev) < 0) {
> > +             dev_err(&serdev->dev, "Can't register HCI device\n");
> > +             hci_free_dev(hdev);
> > +             return -ENODEV;
> > +     }
> > +
> > +     if (!ps_init_work(hdev))
> > +             ps_init_timer(hdev);
> 
> Eh, shouldn't this fail the probe on error?
> 
> But then, why is psdata not embedded into btnxpuart_dev but separately
> allocated?
Initially I planned to keep psdata as a separate data structure, and handle it using ps_* functions.
This was to keep the btnxpuart_dev from getting too complex.
Anyways I have now embedded a psdata data structure inside btnxpuart_dev.

> 
> > +     crc8_populate_msb(crc8_table, POLYNOMIAL8);
> 
> Is it okay to populate after HCI device is registered?
Moved this and a couple of other function calls before registering HCI device.
 
> --
>  i.

Please let me know if you have any concerns or further review comments.

Thank you,
Neeraj