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

[Neeraj sanjay kale <neeraj.sanjaykale@nxp.com>]

Hi Ilpo,

I have resolved most of your comments in v8 patch, and I have few things to discuss regarding the v6 patch.

> 
> > > > +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.
> 
> So the .crc member should be __be32 then?
> 
I disagree with using __be32.
I have simplified this part of the code in v8 patch, please do check it out.
So the CRC part of the data structure will remain __le32, and will be sent over UART to the chip in Little Endian format.
It's just that the FW expects the CRC to be byte-swapped. 
Technically it is big endian format, but you may think of it as a "+1 level" of encryption (although it isn't).
So defining this structure member as __be32 can create more questions than answers, leading to more confusion.
If it helps, I have also added a small comment in there to signify that the FW  expects CRC in byte swapped method.

> > > > +     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.
> 
> Eh, sizeof(*nxp_cmd7) is 16 bytes!?! Are you sure that req_len given to
> serdev_device_write_buf() is not larger than 16 bytes?
> 
I have now replaced req_len with sizeof(<struct>).
There is also a check in the beginning of the function to return if req_len is not 16 bytes.

> > > > +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.
> 
> Okay, it seems your changes had a side-effect of addressing this.
> 
> > > > +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!
> 
> Okay, makes sense now and the logic is also clearer now. However, the brace
> blocks you added into those cases in bxp_enqueue() you should try to
> remove. I realize you do it to avoid name collisions because you reused
> param in each but they introduced these ugly constructs:
>         case XX:
>                 {
>                         ...
>                         goto free_skb;
>                 }
>                 break;
> 
Modified in v8.

Thanks again for the review comments!

Neeraj.