All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Neeraj sanjay kale <neeraj.sanjaykale@nxp.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
Date: Tue, 7 Mar 2023 13:43:35 +0200 (EET)	[thread overview]
Message-ID: <48e776a1-7526-5b77-568b-322d4555a138@linux.intel.com> (raw)
In-Reply-To: <AM9PR04MB86037CDF6A032963405AF0CEE7B69@AM9PR04MB8603.eurprd04.prod.outlook.com>

On Mon, 6 Mar 2023, Neeraj sanjay kale wrote:

> 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:

Further discussion below + I sent a few against v7.

 
> > > +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?

> > > +     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?

> > > +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;

-- 
 i.


  reply	other threads:[~2023-03-07 11:48 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-01 15:45 [PATCH v6 0/3] Add support for NXP bluetooth chipsets Neeraj Sanjay Kale
2023-03-01 15:45 ` [PATCH v6 1/3] serdev: Add method to assert break signal over tty UART port Neeraj Sanjay Kale
2023-03-01 16:38   ` Add support for NXP bluetooth chipsets bluez.test.bot
2023-03-01 15:45 ` [PATCH v6 2/3] dt-bindings: net: bluetooth: Add NXP bluetooth support Neeraj Sanjay Kale
2023-03-01 15:45 ` [PATCH v6 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets Neeraj Sanjay Kale
2023-03-02 14:43   ` Ilpo Järvinen
2023-03-06 17:43     ` Neeraj sanjay kale
2023-03-07 11:43       ` Ilpo Järvinen [this message]
2023-03-10 18:33         ` Neeraj sanjay kale
2023-03-13  9:08           ` Ilpo Järvinen
2023-03-13 14:17             ` [EXT] " Neeraj sanjay kale
2023-03-06 14:56   ` Francesco Dolcini
2023-03-10 12:09     ` [EXT] " Neeraj sanjay kale
2023-03-10 13:00       ` Francesco Dolcini
2023-03-10 13:51         ` Neeraj sanjay kale

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=48e776a1-7526-5b77-568b-322d4555a138@linux.intel.com \
    --to=ilpo.jarvinen@linux.intel.com \
    --cc=alok.a.tiwari@oracle.com \
    --cc=amitkumar.karwar@nxp.com \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=edumazet@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdanton@sina.com \
    --cc=jirislaby@kernel.org \
    --cc=johan.hedberg@gmail.com \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=kuba@kernel.org \
    --cc=leon@kernel.org \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=luiz.dentz@gmail.com \
    --cc=marcel@holtmann.org \
    --cc=neeraj.sanjaykale@nxp.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=robh+dt@kernel.org \
    --cc=rohit.fule@nxp.com \
    --cc=sherry.sun@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.