linux-kernel - Re: [PATCH v1] Bluetooth: btnxpuart: Add support for IW624 chipset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZNUt9V2WCS/ZCavl@livingston.pivistrello.it>
Date:   Thu, 10 Aug 2023 20:35:33 +0200
From:   Francesco Dolcini <francesco@...cini.it>
To:     Neeraj sanjay kale <neeraj.sanjaykale@....com>
Cc:     Francesco Dolcini <francesco@...cini.it>,
        "marcel@...tmann.org" <marcel@...tmann.org>,
        "johan.hedberg@...il.com" <johan.hedberg@...il.com>,
        "luiz.dentz@...il.com" <luiz.dentz@...il.com>,
        Amitkumar Karwar <amitkumar.karwar@....com>,
        Rohit Fule <rohit.fule@....com>,
        Sherry Sun <sherry.sun@....com>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] Bluetooth: btnxpuart: Add support for IW624 chipset

On Thu, Aug 10, 2023 at 06:02:32PM +0000, Neeraj sanjay kale wrote:
> Hi Francesco
> 
> Thank you for reviewing this patch.
> 
> > > --- a/drivers/bluetooth/btnxpuart.c
> > > +++ b/drivers/bluetooth/btnxpuart.c
> > ...
> > > @@ -547,7 +553,7 @@ static int nxp_download_firmware(struct hci_dev
> > *hdev)
> > >       serdev_device_set_flow_control(nxpdev->serdev, false);
> > >       nxpdev->current_baudrate = HCI_NXP_PRI_BAUDRATE;
> > >
> > > -     /* Wait till FW is downloaded and CTS becomes low */
> > > +     /* Wait till FW is downloaded */
> > >       err = wait_event_interruptible_timeout(nxpdev->fw_dnld_done_wait_q,
> > >                                              !test_bit(BTNXPUART_FW_DOWNLOADING,
> > >
> > > &nxpdev->tx_state), @@ -558,16 +564,11 @@ static int
> > nxp_download_firmware(struct hci_dev *hdev)
> > >       }
> > >
> > >       serdev_device_set_flow_control(nxpdev->serdev, true);
> > > -     err = serdev_device_wait_for_cts(nxpdev->serdev, 1, 60000);
> > > -     if (err < 0) {
> > > -             bt_dev_err(hdev, "CTS is still high. FW Download failed.");
> > > -             return err;
> > > -     }
> > this seems like an unrelated change, and it's moving from a 60secs timeout
> > polling CTS to nothing.
> > 
> > What's the reason for this? Should be this a separate commit with a proper
> > explanation?
> > 
> While working on integrating IW624 in btnxpuart driver, I observed that the
> first reset command was getting timed out, after FW download was complete 2
> out of 10 times. On further timing analysis, I noticed that this wait for CTS
> code did not actually help much, since CTS is already low after FW download,
> and becomes high after few more milli-seconds, and then low again after FW is
> initialized.  So it was either adding a "wait for CTS high" followed by "wait
> for CTS low", or simply increasing the sleep delay from 1000msec to 1200msec.
> I chose the later as it seemed more cleaner, and did the job perfectly, and
> tested all previously supported chipsets to make sure nothing is broke.  But
> you are right, I should add an explanation for this change in the commit
> message in the v2 patch.

This should be a separate commit, and probably it should have a fixes tag,
since this is solving a bug. I recently noted some bugs around this, I just did
not have the time to reproduce on the latest mainline kernel to report those.

One more question on this, what about the use case in which a combo firmware
is used and no firmware is loaded here? Will this use case be affected?

Francesco