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: Mon, 13 Mar 2023 11:08:33 +0200 (EET)	[thread overview]
Message-ID: <11c7e098-19c8-6961-5369-214bc948bc37@linux.intel.com> (raw)
In-Reply-To: <AM9PR04MB8603D2F3E3CDC714BDACECC0E7BA9@AM9PR04MB8603.eurprd04.prod.outlook.com>

On Fri, 10 Mar 2023, Neeraj sanjay kale wrote:

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

I'd have still put the member as __be32 and commented the swap expectation 
there. But it's not an end of the world even in the current form.

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

Ah, I'd missed that check for some reason.


-- 
 i.


  reply	other threads:[~2023-03-13  9:10 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
2023-03-10 18:33         ` Neeraj sanjay kale
2023-03-13  9:08           ` Ilpo Järvinen [this message]
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=11c7e098-19c8-6961-5369-214bc948bc37@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.