linux-kernel - Re: [RFC PATCH] Bluetooth: btnxpuart: Fix nxp

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: <20240117104917.GA6138@francesco-nb>
Date: Wed, 17 Jan 2024 11:49:17 +0100
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>,
	Marcel Ziswiler <marcel.ziswiler@...adex.com>,
	Amitkumar Karwar <amitkumar.karwar@....com>,
	"linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Sherry Sun <sherry.sun@....com>, Rohit Fule <rohit.fule@....com>
Subject: Re: [RFC PATCH] Bluetooth: btnxpuart: Fix nxp_setup in case chip is
 powered on late

On Wed, Jan 17, 2024 at 09:38:43AM +0000, Neeraj Sanjay Kale wrote:
> > > diff --git a/drivers/bluetooth/btnxpuart.c
> > > b/drivers/bluetooth/btnxpuart.c index 7f88b6f52f26..20a3a5bd5529
> > > 100644
> > > --- a/drivers/bluetooth/btnxpuart.c
> > > +++ b/drivers/bluetooth/btnxpuart.c
> > > @@ -1036,6 +1048,13 @@ static int nxp_setup(struct hci_dev *hdev)
> > >               err = nxp_download_firmware(hdev);
> > >               if (err < 0)
> > >                       return err;
> > > +     } else if (!serdev_device_get_cts(nxpdev->serdev)) {
> > > +             /* CTS is high and no bootloader signatures detected */
> > > +             bt_dev_dbg(hdev, "Controller not detected. Will check again in %d
> > msec",
> > > +                        NXP_SETUP_RETRY_TIME_MS);
> > > +             schedule_delayed_work(&nxpdev->setup_retry_work,
> > > +                                   msecs_to_jiffies(NXP_SETUP_RETRY_TIME_MS));
> > > +             return -ENODEV;
> > why not just
> > 
> > return -EPROBE_DEFER;
> > 
> > and remove everything else, no need for any kind of retry or delayed work if
> > the driver core takes care of it.
> > 
> Wouldn't returning -EPROBE_DEFER make more sense in driver probe context?

Yes, you are right. I was rushing to this suggestion without thinking at this
properly.

> Here, the driver probe registers an hci interface
> (hci_register_dev()), and returns success to kernel.
> 
> The hci_register_dev() queues hdev->power_on work at the end, which
> opens the hci dev, and ultimately calls this setup function.
> 
> In this patch, we are queueing the same work from the delayed
> setup_retry_work().
> 
> Returning -ENODEV (or -EPROBE_DEFER) would only affect hci_dev_open(),
> which is in power_on work context, and not driver probe context.
> 
> Perhaps, we should call it hci_retry_power_on() work or something
> similar?
> 
> Please let me know your thoughts on this.

Do you see any way to get rid of this complexity? Maybe having this
check done during probe, deferring there till we know the device is in a
suitable state (e.g. either you received the bootloader signature, you
know the device is powered or that the firmware is loaded and ready?).

In other words returning EPROBE_DEFER before calling hci_register_dev()?


With that said I still see an issue if the firmware is loaded by the
wifi driver and the BT driver start sending commands before the firware
load procedure is concluded and the firmware is ready. Not sure if you
have a way to wait for this "firmware ready" state.

Francesco