From:   Neeraj sanjay kale <neeraj.sanjaykale@nxp.com>
To:     =?iso-8859-1?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
CC:     "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "marcel@holtmann.org" <marcel@holtmann.org>,
        "johan.hedberg@gmail.com" <johan.hedberg@gmail.com>,
        "luiz.dentz@gmail.com" <luiz.dentz@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        "alok.a.tiwari@oracle.com" <alok.a.tiwari@oracle.com>,
        "hdanton@sina.com" <hdanton@sina.com>,
        "leon@kernel.org" <leon@kernel.org>,
        Netdev <netdev@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        linux-serial <linux-serial@vger.kernel.org>,
        Amitkumar Karwar <amitkumar.karwar@nxp.com>,
        Rohit Fule <rohit.fule@nxp.com>,
        Sherry Sun <sherry.sun@nxp.com>
Subject: Re: [PATCH v6 3/3] Bluetooth: NXP: Add protocol support for NXP
 Bluetooth chipsets
Thread-Topic: [PATCH v6 3/3] Bluetooth: NXP: Add protocol support for NXP
 Bluetooth chipsets
Thread-Index: AQHZU37Vjd7Ix4B5hkSh358K3L6HrQ==
Date:   Fri, 10 Mar 2023 18:33:39 +0000
Message-ID: <AM9PR04MB8603D2F3E3CDC714BDACECC0E7BA9@AM9PR04MB8603.eurprd04.prod.outlook.com>
References: <20230301154514.3292154-1-neeraj.sanjaykale@nxp.com>
 <20230301154514.3292154-4-neeraj.sanjaykale@nxp.com>
 <73527cb7-6546-6c47-768c-5f4648b6d477@linux.intel.com>
 <AM9PR04MB86037CDF6A032963405AF0CEE7B69@AM9PR04MB8603.eurprd04.prod.outlook.com>
 <48e776a1-7526-5b77-568b-322d4555a138@linux.intel.com>
In-Reply-To: <48e776a1-7526-5b77-568b-322d4555a138@linux.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?ASeMjxcyhH7BMTkdtT/1AHH1Qz4Vo8A1Nojc7Sr4nDCPD4N3XBx4lJ0EHN?=
 =?iso-8859-1?Q?Prn+R828Zt/Q/2T6J8LBuYeCrx6dHJsAIYsVd50YnbkzEij+Qp8JQrC5Km?=
 =?iso-8859-1?Q?kqsgTLJfjlg5mPyPu+TpQ/3uE/2ZoL+2PUOevyiFeipl+gxBwoz6ybiYhN?=
 =?iso-8859-1?Q?TQUBF+xAGwLv4aKFCzdqJCrVbtpJX7AQQ+QqIrT54ywZtWjCSYgGCzQCz+?=
 =?iso-8859-1?Q?NiH5Z7MIKPievthP4RJte0ZrLd04khzV/iEhe8gwFmjszzMdOHH4CsoGX3?=
 =?iso-8859-1?Q?Li81YK3bW8WKcFYaTpGn4vmFeSbfjjD9nlZxyKefYR48Wb88VRvFTC3SHg?=
 =?iso-8859-1?Q?dUdC5ZYHk68fR7EX+bx17WVBbH0lAUaUruq9pWXCt3b9KjflZCg8PHGfR5?=
 =?iso-8859-1?Q?5FaCWsCyYkXI5xLB0hHsmFaK9PS4IT0SJiZSHtJJ2Met2+i0MonYcHIndQ?=
 =?iso-8859-1?Q?701LvseEz0yTpzU9XutOCwnNHI+hcweM0zZbocjfcVxoXKM9jgU/vr8Ikl?=
 =?iso-8859-1?Q?cNx1DELRqtNUh9XHSh+hAHWGAToNpMOgPl3wf2LIfOTbv+Z3uC8HNMYn/G?=
 =?iso-8859-1?Q?NaETe3mVgZ7ARS9QFywgg25bAExs8Lbi5JcSLSEZnoOpAsFg3cFVUmnQZk?=
 =?iso-8859-1?Q?jr+d2GIhklJ0XS8waF4xxY1CYi3ZRZq3VqIwxJSJ0I6wQ1qahUQOs7sleG?=
 =?iso-8859-1?Q?gea8QQTfII34l/uJIf7NScLspA0f364oqWwBUsRh7X4dhNYUqSL92OWVBr?=
 =?iso-8859-1?Q?Ln9zBzIqLTmgrVkSxJB9qy/IufzoUtY8HFag4Xg9ZwriU8oY2pWiv1B53g?=
 =?iso-8859-1?Q?x/byh943DJidUYo/b3A+WMaLQAe4FXeJdTkpAILoDRQKS63XgGOQIxojkM?=
 =?iso-8859-1?Q?ya0X9nPwEVRVMjTeE4AtR7WLaSPDZ6OZ8uuceHrrMeFfRvF8tXxQxl6qvF?=
 =?iso-8859-1?Q?uJnjdyl1rsfx3KJwxWrA9TsvcbBZ/IA4JZ44bW9ZhVKrNv2kzkdTvcRqpQ?=
 =?iso-8859-1?Q?Pbw2qvIJpgcRbL0kfPJkjUehjAg8PL3uYz2crCkH6XkWoWHiKrQa+BKwzC?=
 =?iso-8859-1?Q?QomFiNB/wBOIOsIweHzNrUTA55ap1Ckx6El0pTJ4p5ZRS5QzvxsXyFpIV+?=
 =?iso-8859-1?Q?/I/OD4QR7G6DXA9EFrp4aH1oOyYcJHysNEd76rBYL/vSNpI6f03JbfDWg/?=
 =?iso-8859-1?Q?l3psRrdzKMO82a6oBvu4SdJZfA4iM7Sz0LzO7e+egxDS3cLHkiaAyTx5ls?=
 =?iso-8859-1?Q?PDUezxY/y1Nm38C0F+NcmNZhnqzye3/o1GyxhdeqDq8i7GxXjqtbF77HLn?=
 =?iso-8859-1?Q?1fcPrXabLHx5Nr4RtYeW12v0Fh3TgGfRRfsoX+1RbTT4qcB/msIH210FYw?=
 =?iso-8859-1?Q?Rf3KqY5XWounz9jQKBWHc5ZJ52fDCvPZF9ZfkYvEFKY7wMWMub8Sx3sZ7/?=
 =?iso-8859-1?Q?2Lt06yzyqsLQLbKIiH0p11C52hP76O2ERlYI+L4ymIqbBKe8Kab5sOKPan?=
 =?iso-8859-1?Q?CLs7MRCWezxUrChiYr5Hmr8Zs5OLQWZ6o1t4haASPinTMmLDDin/RRq8EZ?=
 =?iso-8859-1?Q?cdSjYcyW0lIWQYGSjzLOm0cSR1tXO/d3yYUSDDH0jilfmoVDdhNX8/gYWv?=
 =?iso-8859-1?Q?xA4dC4zTLZm6E3PZq5HAXaEVFfIbeIqYgX967fmCHb9FS7TNwvEMnurg?=
 =?iso-8859-1?Q?=3D=3D?=
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AM9PR04MB8603.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8c22357d-fc06-4269-aacc-08db2195f7b8
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Mar 2023 18:33:39.2819
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: IkAnuwgxxUUx9MJ0fvE8vrjfGa9GkGvawpKQ5DDZoTg9ODeFgWz84uprnRQA2Dp4oU7QYQ7R60R2AvkuAhPTpcolIobdOexP16dyuPZJapM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM7PR04MB6997
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Ilpo,

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

>=20
> > > > +static bool nxp_fw_change_baudrate(struct hci_dev *hdev, u16
> > > > +req_len) {
> > > > +     struct btnxpuart_dev *nxpdev =3D hci_get_drvdata(hdev);
> > > > +     struct nxp_bootloader_cmd nxp_cmd5;
> > > > +     struct uart_config uart_config;
> > > > +
> > > > +     if (req_len =3D=3D sizeof(nxp_cmd5)) {
> > > > +             nxp_cmd5.header =3D __cpu_to_le32(5);
> > > > +             nxp_cmd5.arg =3D 0;
> > > > +             nxp_cmd5.payload_len =3D __cpu_to_le32(sizeof(uart_co=
nfig));
> > > > +             nxp_cmd5.crc =3D swab32(crc32_be(0UL, (char *)&nxp_cm=
d5,
> > > > +                                            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.
>=20
> So the .crc member should be __be32 then?
>=20
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.=20
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 conten=
t?
> > The chip requests chunk of FW data which is never more than 2048 bytes
> > at a time.
>=20
> 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?
>=20
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 =3D wait_event_interruptible_timeout(nxpdev-
> > > >check_boot_sign_wait_q,
> > > >
> +                                            !test_bit(BTNXPUART_CHECK_BO=
OT_SIGNATURE,
> > > > +                                                      &nxpdev->tx_=
state),
> > > > +                                            msecs_to_jiffies(1000)=
);
> > > > +     if (ret =3D=3D 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.
>=20
> Okay, it seems your changes had a side-effect of addressing this.
>=20
> > > > +static int nxp_enqueue(struct hci_dev *hdev, struct sk_buff *skb) =
{
> > > > +     struct btnxpuart_dev *nxpdev =3D hci_get_drvdata(hdev);
> > > > +     struct ps_data *psdata =3D 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 =3D=3D 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 + ret=
urn)?
> > > 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!
>=20
> Okay, makes sense now and the logic is also clearer now. However, the bra=
ce
> 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;
>=20
Modified in v8.

Thanks again for the review comments!

Neeraj.