Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1164-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
To: Francesco Dolcini <francesco@dolcini.it>
CC: "marcel@holtmann.org" <marcel@holtmann.org>, "johan.hedberg@gmail.com"
	<johan.hedberg@gmail.com>, "luiz.dentz@gmail.com" <luiz.dentz@gmail.com>,
	Marcel Ziswiler <marcel.ziswiler@toradex.com>, Amitkumar Karwar
	<amitkumar.karwar@nxp.com>, "linux-bluetooth@vger.kernel.org"
	<linux-bluetooth@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, Sherry Sun <sherry.sun@nxp.com>, Rohit Fule
	<rohit.fule@nxp.com>
Subject: Re: [RFC PATCH] Bluetooth: btnxpuart: Fix nxp_setup in case chip is
 powered on late
Thread-Topic: [RFC PATCH] Bluetooth: btnxpuart: Fix nxp_setup in case chip is
 powered on late
Thread-Index: AQHaSTX0VCxQE2mpnEWoNbGuyoXvZg==
Date: Wed, 17 Jan 2024 11:11:44 +0000
Message-ID:
 <AS4PR04MB969292D8951C3C93FD23099AE7722@AS4PR04MB9692.eurprd04.prod.outlook.com>
References: <20240117030501.149114-1-neeraj.sanjaykale@nxp.com>
 <20240117090932.GA3787@francesco-nb>
 <AS4PR04MB9692991FC87A8FF21E455BC8E7722@AS4PR04MB9692.eurprd04.prod.outlook.com>
 <20240117104917.GA6138@francesco-nb>
In-Reply-To: <20240117104917.GA6138@francesco-nb>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?LLkwSzDqpvXTxdH30j+zxz8/0FmM9HTQAkNtTe2Z5vLlpQBZpQ9y1NgB/bu6?=
 =?us-ascii?Q?sGgybg0oZf5+CFyac0dPTLRk22GoUDa3x84Aqmp4VcWqhEGMGU5piL0A3rag?=
 =?us-ascii?Q?1lzIgWrYin8MSmUYJZarLRY9yZMXBRCqzXspG3KkZYcLREnTInzhTMwfBkxL?=
 =?us-ascii?Q?E66ysijczixqN9K6aQ4sDRqUa49UnKvC4CMZBdbSUUDW5RH28v4tez/SeDPq?=
 =?us-ascii?Q?cz3cQz9BW7gt+VF6IpAg+L4VMW+sqMXI020a+0ue5TIp2GkR2CWcKnylF8DF?=
 =?us-ascii?Q?ZKnq8KrBCbjo5CjrIdy2m3Gstpc9OrRirtJkDvQxWyQ8la6neBFhkxjUSBSE?=
 =?us-ascii?Q?BhLRs4gl91BtAC2kZhKu5kGEnuhaQq5iydvYFRoTBN8+yb1wDRMnnUYZb781?=
 =?us-ascii?Q?LlJNbOmScmHDuigu0kccvbhoHfOVSt+jgKSt4TkO6zPvnued2qS7x9GNEO7U?=
 =?us-ascii?Q?s7BjcHp8cRAb9AesM0uqc8kHbGgzqi3SaU3le2If/CEes7HVhcyR4DjTDUqw?=
 =?us-ascii?Q?jW4tx+p/isuvqPJGpqUZapuj7RcXVqE2AYbHSwE9mRyU1uWdDizkImkfrwKC?=
 =?us-ascii?Q?B4eFIHz/vnuh/Pb9rn5XS+EgF6F2Jecnj6KNRxM28R9kjTSilFbmEJERn6Lq?=
 =?us-ascii?Q?s6DDuxEW1aUEw4gMgIMRYOXY1bziZheXotKu3HgWqLBw1cje47Brav2aMPLe?=
 =?us-ascii?Q?JBaqZm2oIr3w0GHb3x2dyzmEQgRwg9rNm7oQGkh80o5F9Lj7X4+J0QTeCpRd?=
 =?us-ascii?Q?moQrKdX9mMRUmTnfTU8rGP5vCmcseDfog6fErVgvt3DgkUukF4CCejIUp7fQ?=
 =?us-ascii?Q?LBstVsKRwix93jpgE57ZXPjXIFpwznwa12Yf/D4KrVPjqiEuAiWa7JdukTsH?=
 =?us-ascii?Q?ZAsCUwp5YfGM2/zp7PtzsQvVIab/7JZLZJIkjxRrE8yEnW+KsVhUd5OrsSdv?=
 =?us-ascii?Q?BP47HjOBf3xmjyTaZ2ujwzbmJnh5NFRclpr2RMg7KACQOh2NxnzZax1afIK/?=
 =?us-ascii?Q?dHfFSzv3nk1nQHDtLoRDXHWg/v+QUh5xsSTX75gOWD+/gY8cepcFyKMZmwwm?=
 =?us-ascii?Q?3C3cGIuvf3NkeDz7GAMpz/kBTsS54+MQ6TmHEx02jb7BCJvcggCg0ov3cUqY?=
 =?us-ascii?Q?2pDo+RIVSHKrg7cEFUOIyxSpgqo3+JWn2q6hPStCk+FSExGyRdWDCIINKJv5?=
 =?us-ascii?Q?bWRo1QF28ARRSl2m6KlejWlf4sT0GLlqxX5CvnSEUQpk2AuGwjE8dic2Yia6?=
 =?us-ascii?Q?8tcX8/9XvqpMjI5oTMjbduXesbemp/2MKZRkK6UUFvKtjlCAeKoFVOf3NeBq?=
 =?us-ascii?Q?VlOjIMt6VwE//6mPY1Bs7mx9RP82yzRk6D1lOgMkBgULsGXU+TIZSMMWVyog?=
 =?us-ascii?Q?Ejo8EmuGHrX980d1JGpH0AehobOJdF0S6j4v+vH5ddeGxkJcNi4/xHp5b9Ki?=
 =?us-ascii?Q?lvmMstDim4cJW6SZytKu0F2ClBh24CAG7a5Cpdlr6rLmWlU9ZUFRL0dhgfB4?=
 =?us-ascii?Q?ByXUEs7rKCFcuiTGmOTBLFZeTg3Cxqb8TCarT1tzaANHYDb3q+1e3WVp5uEy?=
 =?us-ascii?Q?n0PCrm4Wke6inubVWJrT/M+KFfu/QDP6nJAiJYRW?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-bluetooth@vger.kernel.org
List-Id: <linux-bluetooth.vger.kernel.org>
List-Subscribe: <mailto:linux-bluetooth+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-bluetooth+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AS4PR04MB9692.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5e21e4fb-12e7-4f35-a0cc-08dc174d16cf
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Jan 2024 11:11:44.1953
 (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: nuiIeb5em/sy69wlbbAmBTJ4JkKSl4YQNWdmYFErdR9B4qzB6TPcdML61o6cAZ4eAhyqgO4R8/9Zhf2Itcz8LAMNU/xZHDJSzzLh9MlIuEQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM9PR04MB8397

Hi Francesco,

> > > > 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 =3D 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_RETR=
Y_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?
>=20
> Yes, you are right. I was rushing to this suggestion without thinking at =
this
> properly.
>=20
> > 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.
>=20
> 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 suitab=
le
> state (e.g. either you received the bootloader signature, you know the de=
vice
> is powered or that the firmware is loaded and ready?).
>=20
> In other words returning EPROBE_DEFER before calling hci_register_dev()?
>=20
Unfortunately no. To read any bootloader signatures, or read CTS line, we n=
eed serdev device opened, which is done only after hci_register_dev() -> po=
wer_on work -> hci_dev_open()->serdev_open().

Re-scheduling power_on work would open the serdev, check for bootloader sig=
natures and CTS line, and if no chip detected, it will return error, closin=
g serdev device.
=20
> 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 procedur=
e
> is concluded and the firmware is ready. Not sure if you have a way to wai=
t for
> this "firmware ready" state.
After FW is downloaded, no matter if it is BT-only FW or Combo FW downloade=
d by WiFi driver, the FW has a bunch of it's own initializations. Only when=
 FW is ready to accept commands, it pulls the CTS line low.
So driver would still be re-scheduling power_on work at the moment FW is do=
wnloaded, but not ready to accept commands.

We could simplify this by only returning -ENODEV, without the delayed_work =
handling, but then user would have to remove and re-insert the btnxpuart dr=
iver after mwifiex driver is loaded successfully. This may not seam like a =
user-friendly approach.
Please let me know your thoughts on this.

Thanks,
Neeraj