* [PATCH net v7 1/3] nfc: nci: uart: serialize driver selection
2026-11-04 6:20 [PATCH net v7 0/3] nfc: order NCI UART and firmware teardown Shubham Antil
@ 2026-11-04 6:20 ` Shubham Antil
2026-11-07 3:23 ` netdev-bot+sashiko
2026-11-04 6:20 ` [PATCH net v7 2/3] nfc: nci: uart: quiesce callbacks during teardown Shubham Antil
2026-11-04 6:20 ` [PATCH net v7 3/3] nfc: nfcmrvl: stop firmware work before unregistering Shubham Antil
2 siblings, 1 reply; 7+ messages in thread
From: Shubham Antil @ 2026-11-04 6:20 UTC (permalink / raw)
To: netdev; +Cc: oe-linux-nfc, david, horms, gio
NCIUARTSETDRIVER assumes that tty->disc_data remains NULL until a
transport has been selected. Two concurrent callers can therefore both
allocate and register an NCI device for the same line discipline.
The transport registry can also be changed while a caller is copying a
template from it, before the transport module has been pinned.
Allocate one UART context when the line discipline is opened and claim it
atomically during driver selection. Protect the transport registry with a
mutex, take the module reference while holding that mutex, and copy only
the immutable template fields. Keep TTY callbacks disabled until the
transport has finished opening, and unwind the context so a failed
selection can be retried. Use acquire/release bit operations for the
selection claim so a retry observes the completed unwind. Treat the driver
number as unsigned before using it to index the registry.
Discovered during an AI-assisted source audit using OpenAI Codex,
gpt-5.6-sol. Patch development and test planning used the same assistant.
Confirmed using an unmodified kernel, a pty-backed user-space NCI
controller, and two concurrent NCIUARTSETDRIVER ioctls; no error injection
was used.
With the fix applied, the reproducer passed 100 cycles under KASAN, UBSAN,
and lockdep, and 50 cycles under strict-mode KCSAN. No in-tree selftest
covers the NCI UART line-discipline path.
Fixes: 9961127d4bce ("NFC: nci: add generic uart support")
Signed-off-by: Shubham Antil <shubham@octane.security>
---
include/net/nfc/nci_core.h | 1 +
net/nfc/nci/uart.c | 142 ++++++++++++++++++++++++++-----------
2 files changed, 100 insertions(+), 43 deletions(-)
diff --git a/include/net/nfc/nci_core.h b/include/net/nfc/nci_core.h
index 664d5058e..2f542f5c9 100644
--- a/include/net/nfc/nci_core.h
+++ b/include/net/nfc/nci_core.h
@@ -454,6 +454,7 @@ struct nci_uart {
spinlock_t rx_lock;
struct work_struct write_work;
struct tty_struct *tty;
+ unsigned long flags;
unsigned long tx_state;
struct sk_buff_head tx_q;
struct sk_buff *tx_skb;
diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
index aa20e8603..529c8cf88 100644
--- a/net/nfc/nci/uart.c
+++ b/net/nfc/nci/uart.c
@@ -13,6 +13,7 @@
#include <linux/kernel.h>
#include <linux/init.h>
+#include <linux/mutex.h>
#include <linux/types.h>
#include <linux/fcntl.h>
#include <linux/interrupt.h>
@@ -34,7 +35,12 @@
#define NCI_UART_SENDING 1
#define NCI_UART_TX_WAKEUP 2
+/* UART flags */
+#define NCI_UART_DRIVER_SET 0
+#define NCI_UART_READY 1
+
static struct nci_uart *nci_uart_drivers[NCI_UART_DRIVER_MAX];
+static DEFINE_MUTEX(nci_uart_drivers_lock);
static inline struct sk_buff *nci_uart_dequeue(struct nci_uart *nu)
{
@@ -102,43 +108,82 @@ static void nci_uart_write_work(struct work_struct *work)
clear_bit(NCI_UART_SENDING, &nu->tx_state);
}
-static int nci_uart_set_driver(struct tty_struct *tty, unsigned int driver)
+static int nci_uart_set_driver(struct nci_uart *nu, unsigned int driver)
{
- struct nci_uart *nu = NULL;
+ struct nci_uart *template;
int ret;
if (driver >= NCI_UART_DRIVER_MAX)
return -EINVAL;
- if (!nci_uart_drivers[driver])
- return -ENOENT;
+ if (test_and_set_bit_lock(NCI_UART_DRIVER_SET, &nu->flags))
+ return -EBUSY;
+
+ mutex_lock(&nci_uart_drivers_lock);
+ template = nci_uart_drivers[driver];
+ if (!template || !try_module_get(template->owner)) {
+ ret = -ENOENT;
+ goto err_unlock;
+ }
+
+ nu->owner = template->owner;
+ nu->ops = template->ops;
+ nu->name = template->name;
+ nu->driver = template->driver;
+ mutex_unlock(&nci_uart_drivers_lock);
+
+ ret = nu->ops.open(nu);
+ if (ret)
+ goto err_module;
+
+ /* Publish transport fields before callbacks observe READY. */
+ smp_mb__before_atomic();
+ set_bit(NCI_UART_READY, &nu->flags);
+
+ return 0;
+
+err_module:
+ cancel_work_sync(&nu->write_work);
+ kfree_skb(nu->tx_skb);
+ nu->tx_skb = NULL;
+ kfree_skb(nu->rx_skb);
+ nu->rx_skb = NULL;
+ skb_queue_purge(&nu->tx_q);
+ clear_bit(NCI_UART_SENDING, &nu->tx_state);
+ clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+ module_put(nu->owner);
+ nu->owner = NULL;
+ goto err_clear;
+err_unlock:
+ mutex_unlock(&nci_uart_drivers_lock);
+err_clear:
+ clear_bit_unlock(NCI_UART_DRIVER_SET, &nu->flags);
+ return ret;
+}
+
+/* ------ LDISC part ------ */
+
+static int nci_uart_tty_alloc(struct tty_struct *tty)
+{
+ struct nci_uart *nu = kzalloc_obj(*nu);
- nu = kzalloc_obj(*nu);
if (!nu)
return -ENOMEM;
- memcpy(nu, nci_uart_drivers[driver], sizeof(struct nci_uart));
nu->tty = tty;
skb_queue_head_init(&nu->tx_q);
INIT_WORK(&nu->write_work, nci_uart_write_work);
spin_lock_init(&nu->rx_lock);
- ret = nu->ops.open(nu);
- if (ret) {
- kfree(nu);
- return ret;
- } else if (!try_module_get(nu->owner)) {
- nu->ops.close(nu);
- kfree(nu);
- return -ENOENT;
- }
tty->disc_data = nu;
+ tty->receive_room = 65536;
+
+ /* Flush any pending characters in the driver */
+ tty_driver_flush_buffer(tty);
return 0;
}
-/* ------ LDISC part ------ */
-
/* nci_uart_tty_open
*
* Called when line discipline changed to NCI_UART.
@@ -156,13 +201,7 @@ static int nci_uart_tty_open(struct tty_struct *tty)
if (!tty->ops->write)
return -EOPNOTSUPP;
- tty->disc_data = NULL;
- tty->receive_room = 65536;
-
- /* Flush any pending characters in the driver */
- tty_driver_flush_buffer(tty);
-
- return 0;
+ return nci_uart_tty_alloc(tty);
}
/* nci_uart_tty_close()
@@ -180,14 +219,17 @@ static void nci_uart_tty_close(struct tty_struct *tty)
if (!nu)
return;
- kfree_skb(nu->tx_skb);
- kfree_skb(nu->rx_skb);
+ if (test_bit(NCI_UART_DRIVER_SET, &nu->flags)) {
+ kfree_skb(nu->tx_skb);
+ kfree_skb(nu->rx_skb);
- skb_queue_purge(&nu->tx_q);
+ skb_queue_purge(&nu->tx_q);
+
+ nu->ops.close(nu);
+ module_put(nu->owner);
+ }
- nu->ops.close(nu);
nu->tty = NULL;
- module_put(nu->owner);
cancel_work_sync(&nu->write_work);
@@ -211,7 +253,8 @@ static void nci_uart_tty_wakeup(struct tty_struct *tty)
clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
- if (tty != nu->tty)
+ if (tty != nu->tty ||
+ !test_bit_acquire(NCI_UART_READY, &nu->flags))
return;
nci_uart_tx_wakeup(nu);
@@ -300,7 +343,8 @@ static void nci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
{
struct nci_uart *nu = tty->disc_data;
- if (!nu || tty != nu->tty)
+ if (!nu || tty != nu->tty ||
+ !test_bit_acquire(NCI_UART_READY, &nu->flags))
return;
spin_lock(&nu->rx_lock);
@@ -328,13 +372,12 @@ static int nci_uart_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
struct nci_uart *nu = tty->disc_data;
int err = 0;
+ if (!nu)
+ return -EBADF;
+
switch (cmd) {
case NCIUARTSETDRIVER:
- if (!nu)
- return nci_uart_set_driver(tty, (unsigned int)arg);
- else
- return -EBUSY;
- break;
+ return nci_uart_set_driver(nu, (unsigned int)arg);
default:
err = n_tty_ioctl_helper(tty, cmd, arg);
break;
@@ -370,33 +413,46 @@ static int nci_uart_send(struct nci_uart *nu, struct sk_buff *skb)
int nci_uart_register(struct nci_uart *nu)
{
+ int ret = 0;
+
if (!nu || !nu->ops.open ||
- !nu->ops.recv || !nu->ops.close)
+ !nu->ops.recv || !nu->ops.close ||
+ (unsigned int)nu->driver >= NCI_UART_DRIVER_MAX)
return -EINVAL;
- /* Set the send callback */
- nu->ops.send = nci_uart_send;
-
/* Add this driver in the driver list */
+ mutex_lock(&nci_uart_drivers_lock);
if (nci_uart_drivers[nu->driver]) {
pr_err("driver %d is already registered\n", nu->driver);
- return -EBUSY;
+ ret = -EBUSY;
+ goto unlock;
}
+
+ /* Set the send callback */
+ nu->ops.send = nci_uart_send;
nci_uart_drivers[nu->driver] = nu;
pr_info("NCI uart driver '%s [%d]' registered\n", nu->name, nu->driver);
- return 0;
+unlock:
+ mutex_unlock(&nci_uart_drivers_lock);
+ return ret;
}
EXPORT_SYMBOL_GPL(nci_uart_register);
void nci_uart_unregister(struct nci_uart *nu)
{
+ mutex_lock(&nci_uart_drivers_lock);
+
pr_info("NCI uart driver '%s [%d]' unregistered\n", nu->name,
nu->driver);
/* Remove this driver from the driver list */
- nci_uart_drivers[nu->driver] = NULL;
+ if ((unsigned int)nu->driver < NCI_UART_DRIVER_MAX &&
+ nci_uart_drivers[nu->driver] == nu)
+ nci_uart_drivers[nu->driver] = NULL;
+
+ mutex_unlock(&nci_uart_drivers_lock);
}
EXPORT_SYMBOL_GPL(nci_uart_unregister);
--
2.54.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net v7 1/3] nfc: nci: uart: serialize driver selection
2026-11-04 6:20 ` [PATCH net v7 1/3] nfc: nci: uart: serialize driver selection Shubham Antil
@ 2026-11-07 3:23 ` netdev-bot+sashiko
0 siblings, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-11-07 3:23 UTC (permalink / raw)
To: shubham; +Cc: netdev, oe-linux-nfc, david, horms, gio, kuba
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message understates what the bug does.
--
nfc: nci: uart: serialize driver selection
This commit allocates the NCI UART context when the line discipline is
opened and claims it atomically in NCIUARTSETDRIVER, so two concurrent
callers can no longer both register a device. It also puts the transport
registry under a mutex and pins the transport module before calling into
it.
> NCIUARTSETDRIVER assumes that tty->disc_data remains NULL until a
> transport has been selected. Two concurrent callers can therefore both
> allocate and register an NCI device for the same line discipline.
>
> The transport registry can also be changed while a caller is copying a
> template from it, before the transport module has been pinned.
[Severity: Low]
This isn't a bug in the code, but could the commit message say what these
races lead to? As written, it reads like a duplicate registration. The old
code looks like it allowed memory-safety problems.
In the old nci_uart_set_driver(), both racing callers allocate a context
and call ops.open(). For nfcmrvl, that path is nfcmrvl_nci_uart_open() ->
nfcmrvl_nci_register_dev() -> nci_register_device(), so each caller
registers its own NFC device. Both then do:
tty->disc_data = nu;
The last write wins. nci_uart_tty_close() only tears down tty->disc_data,
so the losing nu and its NFC device are never closed and are leaked.
That leaked device still has nu->tty pointing at a tty that is freed
later. Bringing it up afterwards would go nci_uart_send() ->
nci_uart_write_work() -> tty->ops->write() on the freed tty_struct.
The old code also read the registry twice without a lock:
if (!nci_uart_drivers[driver])
return -ENOENT;
...
memcpy(nu, nci_uart_drivers[driver], sizeof(struct nci_uart));
If nci_uart_unregister() runs between the two reads, the memcpy() reads
through a NULL pointer.
It also ran ops.open(), which is code in the transport module, before
pinning that module:
ret = nu->ops.open(nu);
if (ret) {
kfree(nu);
return ret;
} else if (!try_module_get(nu->owner)) {
The reproducer used a pty, and NCIUARTSETDRIVER has no capability check.
Would it make sense to name the leak, the use-after-free, the NULL
dereference and the call into an unloading module explicitly?
Step 9 of Documentation/process/coding-assistants.rst asks AI-assisted
reports to check Documentation/process/threat-model.rst and classify the
bug as a vulnerability or a regular bug. Should the commit message include
that classification?
Does this also need a Cc: stable@vger.kernel.org tag? According to
Documentation/process/maintainer-netdev.rst, netdev follows the standard
stable rules for fixes.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104062026.80599-1-shubham%40octane.security
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net v7 2/3] nfc: nci: uart: quiesce callbacks during teardown
2026-11-04 6:20 [PATCH net v7 0/3] nfc: order NCI UART and firmware teardown Shubham Antil
2026-11-04 6:20 ` [PATCH net v7 1/3] nfc: nci: uart: serialize driver selection Shubham Antil
@ 2026-11-04 6:20 ` Shubham Antil
2026-11-07 3:23 ` netdev-bot+sashiko
2026-11-04 6:20 ` [PATCH net v7 3/3] nfc: nfcmrvl: stop firmware work before unregistering Shubham Antil
2 siblings, 1 reply; 7+ messages in thread
From: Shubham Antil @ 2026-11-04 6:20 UTC (permalink / raw)
To: netdev; +Cc: oe-linux-nfc, david, horms, gio
nci_uart_tty_close() releases transport state before ensuring that the
UART write worker and receive callback have stopped using it. A write
wakeup can also race the worker's final TX_WAKEUP test and SENDING handoff.
The Marvell open path has a related publication window. It registers the
NCI device before publishing the UART back-pointers and enabling receive
callbacks, so an early device-open can use unset transport state or lose
the controller response.
Serialize transmit scheduling and the worker's final state transition.
Once quiescing starts, block new work and cancel the worker synchronously.
Balance tx_start() with tx_done() if teardown catches an active transmit
burst, and make readiness publication restart an already queued frame.
Publish the Marvell UART state before NCI registration exposes the device
and roll the state back if registration fails. On close, detach TTY
callback admission first but keep transmit available while the transport
unregisters, so the existing close-time CORE_RESET can still be sent.
Then stop generic callbacks, halt the controller when a reset GPIO is
available, and withdraw the UART back-pointers before releasing private
state. Apply the same barriers to the failed-open path.
Discovered during an AI-assisted source audit using OpenAI Codex,
gpt-5.6-sol. Patch development and test planning used the same assistant.
Confirmed with repeated NCI UART registration, device-open, and hangup
cycles using an unmodified kernel and a pty-backed user-space controller;
no error injection was used.
With the fix applied, the registration/publication reproducer passed 20
cycles under KASAN, UBSAN, and lockdep, and 5 cycles under strict-mode
KCSAN. Lifecycle testing observed all four expected CORE_RESET commands.
No physical Marvell controller was available, and no in-tree selftest
covers this UART path.
Fixes: 9961127d4bce ("NFC: nci: add generic uart support")
Fixes: e097dc624f78 ("NFC: nfcmrvl: add UART driver")
Signed-off-by: Shubham Antil <shubham@octane.security>
---
drivers/nfc/nfcmrvl/main.c | 15 ++++-
drivers/nfc/nfcmrvl/nfcmrvl.h | 2 +
drivers/nfc/nfcmrvl/uart.c | 28 +++++++--
include/net/nfc/nci_core.h | 5 ++
net/nfc/nci/uart.c | 115 ++++++++++++++++++++++++++++------
5 files changed, 139 insertions(+), 26 deletions(-)
diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 6efa83219..81c760534 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -154,19 +154,24 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
nci_set_drvdata(priv->ndev, priv);
+ /* Ensure that the controller is powered off before exposing it. */
+ nfcmrvl_chip_halt(priv);
+
+ if (priv->if_ops->nci_ready)
+ priv->if_ops->nci_ready(priv);
+
rc = nci_register_device(priv->ndev);
if (rc) {
nfc_err(dev, "nci_register_device failed %d\n", rc);
goto error_fw_dnld_deinit;
}
- /* Ensure that controller is powered off */
- nfcmrvl_chip_halt(priv);
-
nfc_info(dev, "registered with nci successfully\n");
return priv;
error_fw_dnld_deinit:
+ if (priv->if_ops->nci_quiesce)
+ priv->if_ops->nci_quiesce(priv);
nfcmrvl_fw_dnld_deinit(priv);
error_free_dev:
nci_free_device(priv->ndev);
@@ -181,6 +186,9 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
struct nci_dev *ndev = priv->ndev;
nci_unregister_device(ndev);
+ if (priv->if_ops->nci_quiesce)
+ priv->if_ops->nci_quiesce(priv);
+
if (priv->ndev->nfc_dev->fw_download_in_progress)
nfcmrvl_fw_dnld_abort(priv);
@@ -241,6 +249,7 @@ void nfcmrvl_chip_halt(struct nfcmrvl_private *priv)
if (priv->config.reset_gpio)
gpiod_set_value(priv->config.reset_gpio, 1);
}
+EXPORT_SYMBOL_GPL(nfcmrvl_chip_halt);
int nfcmrvl_parse_dt(struct device_node *node,
struct nfcmrvl_platform_data *pdata)
diff --git a/drivers/nfc/nfcmrvl/nfcmrvl.h b/drivers/nfc/nfcmrvl/nfcmrvl.h
index 62fa77d58..5c2a5382c 100644
--- a/drivers/nfc/nfcmrvl/nfcmrvl.h
+++ b/drivers/nfc/nfcmrvl/nfcmrvl.h
@@ -112,6 +112,8 @@ struct nfcmrvl_if_ops {
int (*nci_open) (struct nfcmrvl_private *priv);
int (*nci_close) (struct nfcmrvl_private *priv);
int (*nci_send) (struct nfcmrvl_private *priv, struct sk_buff *skb);
+ void (*nci_ready)(struct nfcmrvl_private *priv);
+ void (*nci_quiesce)(struct nfcmrvl_private *priv);
void (*nci_update_config)(struct nfcmrvl_private *priv,
const void *param);
};
diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 9aedd1687..b7f99cdb4 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -44,6 +44,27 @@ static int nfcmrvl_uart_nci_send(struct nfcmrvl_private *priv,
return nu->ops.send(nu, skb);
}
+static void nfcmrvl_uart_nci_ready(struct nfcmrvl_private *priv)
+{
+ struct nci_uart *nu = priv->drv_data;
+
+ priv->support_fw_dnld = true;
+ nu->drv_data = priv;
+ nu->ndev = priv->ndev;
+ nci_uart_set_ready(nu);
+}
+
+static void nfcmrvl_uart_nci_quiesce(struct nfcmrvl_private *priv)
+{
+ struct nci_uart *nu = priv->drv_data;
+
+ nci_uart_rx_stop(nu);
+ nci_uart_tx_stop(nu);
+ nfcmrvl_chip_halt(priv);
+ nu->drv_data = NULL;
+ nu->ndev = NULL;
+}
+
static void nfcmrvl_uart_nci_update_config(struct nfcmrvl_private *priv,
const void *param)
{
@@ -58,6 +79,8 @@ static const struct nfcmrvl_if_ops uart_ops = {
.nci_open = nfcmrvl_uart_nci_open,
.nci_close = nfcmrvl_uart_nci_close,
.nci_send = nfcmrvl_uart_nci_send,
+ .nci_ready = nfcmrvl_uart_nci_ready,
+ .nci_quiesce = nfcmrvl_uart_nci_quiesce,
.nci_update_config = nfcmrvl_uart_nci_update_config
};
@@ -136,11 +159,6 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
if (IS_ERR(priv))
return PTR_ERR(priv);
- priv->support_fw_dnld = true;
-
- nu->drv_data = priv;
- nu->ndev = priv->ndev;
-
return 0;
}
diff --git a/include/net/nfc/nci_core.h b/include/net/nfc/nci_core.h
index 2f542f5c9..6b938c2b5 100644
--- a/include/net/nfc/nci_core.h
+++ b/include/net/nfc/nci_core.h
@@ -436,6 +436,7 @@ struct nci_uart;
struct nci_uart_ops {
int (*open)(struct nci_uart *nci_uart);
+ /* Quiesce callbacks before releasing transport-private state. */
void (*close)(struct nci_uart *nci_uart);
int (*recv)(struct nci_uart *nci_uart, struct sk_buff *skb);
int (*send)(struct nci_uart *nci_uart, struct sk_buff *skb);
@@ -452,6 +453,7 @@ struct nci_uart {
/* Dynamic data */
struct nci_dev *ndev;
spinlock_t rx_lock;
+ spinlock_t tx_lock; /* protects tx_state */
struct work_struct write_work;
struct tty_struct *tty;
unsigned long flags;
@@ -466,5 +468,8 @@ struct nci_uart {
int nci_uart_register(struct nci_uart *nu);
void nci_uart_unregister(struct nci_uart *nu);
void nci_uart_set_config(struct nci_uart *nu, int baudrate, int flow_ctrl);
+void nci_uart_set_ready(struct nci_uart *nu);
+void nci_uart_rx_stop(struct nci_uart *nu);
+void nci_uart_tx_stop(struct nci_uart *nu);
#endif /* __NCI_CORE_H */
diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
index 529c8cf88..63671ea38 100644
--- a/net/nfc/nci/uart.c
+++ b/net/nfc/nci/uart.c
@@ -34,6 +34,7 @@
/* TX states */
#define NCI_UART_SENDING 1
#define NCI_UART_TX_WAKEUP 2
+#define NCI_UART_CLOSING 3
/* UART flags */
#define NCI_UART_DRIVER_SET 0
@@ -64,24 +65,61 @@ static inline int nci_uart_queue_empty(struct nci_uart *nu)
static int nci_uart_tx_wakeup(struct nci_uart *nu)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&nu->tx_lock, flags);
+ if (test_bit(NCI_UART_CLOSING, &nu->tx_state) ||
+ !test_bit_acquire(NCI_UART_READY, &nu->flags))
+ goto unlock;
+
if (test_and_set_bit(NCI_UART_SENDING, &nu->tx_state)) {
set_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
- return 0;
+ goto unlock;
}
schedule_work(&nu->write_work);
+unlock:
+ spin_unlock_irqrestore(&nu->tx_lock, flags);
+
return 0;
}
+void nci_uart_tx_stop(struct nci_uart *nu)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&nu->tx_lock, flags);
+ set_bit(NCI_UART_CLOSING, &nu->tx_state);
+ spin_unlock_irqrestore(&nu->tx_lock, flags);
+
+ cancel_work_sync(&nu->write_work);
+ clear_bit(TTY_DO_WRITE_WAKEUP, &nu->tty->flags);
+
+ spin_lock_irqsave(&nu->tx_lock, flags);
+ clear_bit(NCI_UART_SENDING, &nu->tx_state);
+ clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+ spin_unlock_irqrestore(&nu->tx_lock, flags);
+}
+EXPORT_SYMBOL_GPL(nci_uart_tx_stop);
+
static void nci_uart_write_work(struct work_struct *work)
{
struct nci_uart *nu = container_of(work, struct nci_uart, write_work);
struct tty_struct *tty = nu->tty;
struct sk_buff *skb;
+ unsigned long flags;
restart:
+ spin_lock_irqsave(&nu->tx_lock, flags);
+ if (test_bit(NCI_UART_CLOSING, &nu->tx_state)) {
+ clear_bit(NCI_UART_SENDING, &nu->tx_state);
+ clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+ spin_unlock_irqrestore(&nu->tx_lock, flags);
+ return;
+ }
clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+ spin_unlock_irqrestore(&nu->tx_lock, flags);
if (nu->ops.tx_start)
nu->ops.tx_start(nu);
@@ -99,14 +137,55 @@ static void nci_uart_write_work(struct work_struct *work)
kfree_skb(skb);
}
- if (test_bit(NCI_UART_TX_WAKEUP, &nu->tx_state))
+ spin_lock_irqsave(&nu->tx_lock, flags);
+ if (test_bit(NCI_UART_CLOSING, &nu->tx_state)) {
+ clear_bit(NCI_UART_SENDING, &nu->tx_state);
+ clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+ spin_unlock_irqrestore(&nu->tx_lock, flags);
+ if (nu->ops.tx_done)
+ nu->ops.tx_done(nu);
+ return;
+ }
+ if (test_and_clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state)) {
+ spin_unlock_irqrestore(&nu->tx_lock, flags);
goto restart;
+ }
+ spin_unlock_irqrestore(&nu->tx_lock, flags);
if (nu->ops.tx_done && nci_uart_queue_empty(nu))
nu->ops.tx_done(nu);
+ spin_lock_irqsave(&nu->tx_lock, flags);
+ if (!test_bit(NCI_UART_CLOSING, &nu->tx_state) &&
+ test_and_clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state)) {
+ spin_unlock_irqrestore(&nu->tx_lock, flags);
+ goto restart;
+ }
clear_bit(NCI_UART_SENDING, &nu->tx_state);
+ clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+ spin_unlock_irqrestore(&nu->tx_lock, flags);
+}
+
+void nci_uart_set_ready(struct nci_uart *nu)
+{
+ /* Publish transport fields before callbacks observe READY. */
+ smp_mb__before_atomic();
+ set_bit(NCI_UART_READY, &nu->flags);
+
+ if (!nci_uart_queue_empty(nu))
+ nci_uart_tx_wakeup(nu);
}
+EXPORT_SYMBOL_GPL(nci_uart_set_ready);
+
+void nci_uart_rx_stop(struct nci_uart *nu)
+{
+ spin_lock(&nu->rx_lock);
+ clear_bit(NCI_UART_READY, &nu->flags);
+ kfree_skb(nu->rx_skb);
+ nu->rx_skb = NULL;
+ spin_unlock(&nu->rx_lock);
+}
+EXPORT_SYMBOL_GPL(nci_uart_rx_stop);
static int nci_uart_set_driver(struct nci_uart *nu, unsigned int driver)
{
@@ -136,21 +215,17 @@ static int nci_uart_set_driver(struct nci_uart *nu, unsigned int driver)
if (ret)
goto err_module;
- /* Publish transport fields before callbacks observe READY. */
- smp_mb__before_atomic();
- set_bit(NCI_UART_READY, &nu->flags);
+ nci_uart_set_ready(nu);
return 0;
err_module:
- cancel_work_sync(&nu->write_work);
+ nci_uart_rx_stop(nu);
+ nci_uart_tx_stop(nu);
kfree_skb(nu->tx_skb);
nu->tx_skb = NULL;
- kfree_skb(nu->rx_skb);
- nu->rx_skb = NULL;
skb_queue_purge(&nu->tx_q);
- clear_bit(NCI_UART_SENDING, &nu->tx_state);
- clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+ clear_bit(NCI_UART_CLOSING, &nu->tx_state);
module_put(nu->owner);
nu->owner = NULL;
goto err_clear;
@@ -173,6 +248,7 @@ static int nci_uart_tty_alloc(struct tty_struct *tty)
nu->tty = tty;
skb_queue_head_init(&nu->tx_q);
INIT_WORK(&nu->write_work, nci_uart_write_work);
+ spin_lock_init(&nu->tx_lock);
spin_lock_init(&nu->rx_lock);
tty->disc_data = nu;
@@ -213,26 +289,25 @@ static void nci_uart_tty_close(struct tty_struct *tty)
{
struct nci_uart *nu = tty->disc_data;
- /* Detach from the tty */
- tty->disc_data = NULL;
-
if (!nu)
return;
+ tty->disc_data = NULL;
+
if (test_bit(NCI_UART_DRIVER_SET, &nu->flags)) {
- kfree_skb(nu->tx_skb);
- kfree_skb(nu->rx_skb);
+ nu->ops.close(nu);
+ nci_uart_rx_stop(nu);
+ nci_uart_tx_stop(nu);
+
+ kfree_skb(nu->tx_skb);
skb_queue_purge(&nu->tx_q);
- nu->ops.close(nu);
module_put(nu->owner);
}
nu->tty = NULL;
- cancel_work_sync(&nu->write_work);
-
kfree(nu);
}
@@ -348,6 +423,10 @@ static void nci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
return;
spin_lock(&nu->rx_lock);
+ if (!test_bit(NCI_UART_READY, &nu->flags)) {
+ spin_unlock(&nu->rx_lock);
+ return;
+ }
nci_uart_default_recv_buf(nu, data, count);
spin_unlock(&nu->rx_lock);
--
2.54.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net v7 2/3] nfc: nci: uart: quiesce callbacks during teardown
2026-11-04 6:20 ` [PATCH net v7 2/3] nfc: nci: uart: quiesce callbacks during teardown Shubham Antil
@ 2026-11-07 3:23 ` netdev-bot+sashiko
0 siblings, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-11-07 3:23 UTC (permalink / raw)
To: shubham; +Cc: netdev, oe-linux-nfc, david, horms, gio, kuba
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 · Low: 3
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] The commit message says teardown will "Balance tx_start() with
tx_done() if teardown catches an active transmit burst".
- [Low] The commit message says nci_uart_tty_close() frees transport
state "before ensuring that the UART write worker and receive callback…
- [Low] The new field comment in include/net/nfc/nci_core.h says
`spinlock_t tx_lock; /* protects tx_state */`.
- [Low] nci_uart_set_ready() (net/nfc/nci/uart.c) calls
nci_uart_queue_empty() with no lock.
--
nfc: nci: uart: quiesce callbacks during teardown
This patch adds a tx_lock and a CLOSING tx state to the generic NCI UART
layer. Teardown can then stop the write worker and receive callbacks before
transport state is freed. It also publishes the Marvell UART back-pointers
before nci_register_device(), and adds nci_ready/nci_quiesce hooks that roll
them back on failure and on unregister.
> nci_uart_tty_close() releases transport state before ensuring that the
> UART write worker and receive callback have stopped using it.
[Severity: Low]
Is the receive callback part of this accurate for the close path?
tty_ldisc_close() runs with the ldisc semaphore held for write:
drivers/tty/tty_ldisc.c:tty_ldisc_close() {
lockdep_assert_held_write(&tty->ldisc_sem);
...
}
tty_port_default_receive_buf() only delivers data after it gets an ldisc
reference:
ld = tty_ldisc_ref(tty);
if (!ld)
return 0;
tty_wakeup() also goes through tty_ldisc_ref(). So nci_uart_tty_receive()
and nci_uart_tty_wakeup() can't run at the same time as
nci_uart_tty_close(). During close, the only concurrent user of tx_skb,
rx_skb and tx_q appears to be nu->write_work.
The same applies here:
> and roll the state back if registration fails. On close, detach TTY
> callback admission first but keep transmit available while the transport
The old code also cleared tty->disc_data before it used nu.
The new READY recheck under rx_lock in nci_uart_tty_receive() and the
nci_uart_rx_stop() calls look like they are needed for the failed-open path
instead. On that path, NCIUARTSETDRIVER runs under an ldisc read reference,
nfcmrvl_uart_nci_ready() publishes READY, and nci_register_device() then
fails. nfcmrvl_uart_nci_quiesce() rolls back while receive_buf can still
run.
Could the commit message say which race actually exists in close?
> diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
> index 529c8cf8861af..63671ea38c67b 100644
> --- a/net/nfc/nci/uart.c
> +++ b/net/nfc/nci/uart.c
[ ... ]
> @@ -64,24 +65,61 @@ static inline int nci_uart_queue_empty(struct nci_uart *nu)
[ ... ]
> +void nci_uart_tx_stop(struct nci_uart *nu)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&nu->tx_lock, flags);
> + set_bit(NCI_UART_CLOSING, &nu->tx_state);
> + spin_unlock_irqrestore(&nu->tx_lock, flags);
> +
> + cancel_work_sync(&nu->write_work);
> + clear_bit(TTY_DO_WRITE_WAKEUP, &nu->tty->flags);
> +
> + spin_lock_irqsave(&nu->tx_lock, flags);
> + clear_bit(NCI_UART_SENDING, &nu->tx_state);
> + clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
> + spin_unlock_irqrestore(&nu->tx_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(nci_uart_tx_stop);
> +
> static void nci_uart_write_work(struct work_struct *work)
> {
> struct nci_uart *nu = container_of(work, struct nci_uart, write_work);
> struct tty_struct *tty = nu->tty;
> struct sk_buff *skb;
> + unsigned long flags;
>
> restart:
> + spin_lock_irqsave(&nu->tx_lock, flags);
> + if (test_bit(NCI_UART_CLOSING, &nu->tx_state)) {
> + clear_bit(NCI_UART_SENDING, &nu->tx_state);
> + clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
> + spin_unlock_irqrestore(&nu->tx_lock, flags);
> + return;
> + }
> clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
> + spin_unlock_irqrestore(&nu->tx_lock, flags);
>
> if (nu->ops.tx_start)
> nu->ops.tx_start(nu);
> @@ -99,14 +137,55 @@ static void nci_uart_write_work(struct work_struct *work)
> kfree_skb(skb);
> }
>
> - if (test_bit(NCI_UART_TX_WAKEUP, &nu->tx_state))
> + spin_lock_irqsave(&nu->tx_lock, flags);
> + if (test_bit(NCI_UART_CLOSING, &nu->tx_state)) {
> + clear_bit(NCI_UART_SENDING, &nu->tx_state);
> + clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
> + spin_unlock_irqrestore(&nu->tx_lock, flags);
> + if (nu->ops.tx_done)
> + nu->ops.tx_done(nu);
> + return;
> + }
> + if (test_and_clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state)) {
> + spin_unlock_irqrestore(&nu->tx_lock, flags);
> goto restart;
> + }
> + spin_unlock_irqrestore(&nu->tx_lock, flags);
>
> if (nu->ops.tx_done && nci_uart_queue_empty(nu))
> nu->ops.tx_done(nu);
[Severity: High]
The commit message says teardown will "Balance tx_start() with tx_done() if
teardown catches an active transmit burst". Can nci_uart_write_work() still
exit after tx_start() without ever calling tx_done()?
Consider this sequence:
write_work nfcmrvl_uart_nci_quiesce()
restart: CLOSING clear
ops.tx_start()
break_ctl(tty, 0)
write loop
test_and_clear_bit(TX_WAKEUP) true
spin_unlock_irqrestore()
nci_uart_tx_stop()
set_bit(NCI_UART_CLOSING)
cancel_work_sync()
goto restart
restart: CLOSING set
clear SENDING, TX_WAKEUP
return
The CLOSING exit at restart: returns without calling ops.tx_done(), even
though tx_start() already ran earlier in the same burst. Only the CLOSING
branch after the write loop calls tx_done().
TX_WAKEUP can be set during the write loop by the close-time CORE_RESET,
sent via nci_close_device()->__nci_request()->nci_uart_send(). If the
closing task has a signal pending,
wait_for_completion_interruptible_timeout() returns early. Teardown then
reaches nci_uart_tx_stop() almost immediately.
There is a second path. If tty->ops->write() writes only part of the skb,
nu->tx_skb stays set. nci_uart_queue_empty() then returns false, so the
tx_done() call above is skipped, and SENDING is cleared while the worker
waits for a write wakeup. If teardown runs at that point, nci_uart_tx_stop()
cancels an idle work item and never calls tx_done() either.
For nfcmrvl with break_control, tx_start and tx_done look like this:
drivers/nfc/nfcmrvl/uart.c:nfcmrvl_nci_uart_tx_start() {
...
/* Remove BREAK to wake up the NFCC */
if (priv->config.break_control && nu->tty->ops->break_ctl) {
nu->tty->ops->break_ctl(nu->tty, 0);
...
}
drivers/nfc/nfcmrvl/uart.c:nfcmrvl_nci_uart_tx_done() {
...
if (priv->config.break_control && nu->tty->ops->break_ctl) {
nu->tty->ops->break_ctl(nu->tty, -1);
...
}
So in both cases BREAK is left removed at teardown. Before this patch, in
the first path, cancel_work_sync() let the running worker finish and reach
tx_done().
The later patch in this series ("nfc: nfcmrvl: stop firmware work before
unregistering") doesn't touch net/nfc/nci/uart.c, so both paths are still
present at the end of the series.
Would it work to set a local started flag when tx_start() runs, and call
tx_done() from every CLOSING exit when it is set? nci_uart_tx_stop() could
also call tx_done() when a burst is still pending.
[ ... ]
> +void nci_uart_set_ready(struct nci_uart *nu)
> +{
> + /* Publish transport fields before callbacks observe READY. */
> + smp_mb__before_atomic();
> + set_bit(NCI_UART_READY, &nu->flags);
> +
> + if (!nci_uart_queue_empty(nu))
> + nci_uart_tx_wakeup(nu);
> }
[Severity: Low]
Is it safe to call nci_uart_queue_empty() here without tx_lock?
nci_uart_set_driver() calls nci_uart_set_ready() a second time after
nu->ops.open() returns. For nfcmrvl, by then nfcmrvl_nci_register_dev() has
already set READY through nfcmrvl_uart_nci_ready() and exposed the device
through nci_register_device().
Userspace can bring the device up in that window:
NFC_CMD_DEV_UP->nci_open_device()->CORE_RESET->nci_uart_send()
The write worker then makes plain stores to nu->tx_skb in
nci_uart_dequeue() and on partial writes, while this unlocked read of
nu->tx_skb runs. Before this patch, nci_uart_queue_empty() was only called
from the worker. Would KCSAN report this as a data race?
The handoff that restarts an already queued frame also seems to lack a full
barrier. This side does set_bit(READY) and then loads the queue. The sender
does skb_queue_tail() and then test_bit_acquire(READY) under tx_lock. That
is a store-buffering pattern the LKMM does not forbid. With nfcmrvl,
though, nothing can be queued before READY is first published.
[ ... ]
> @@ -136,21 +215,17 @@ static int nci_uart_set_driver(struct nci_uart *nu, unsigned int driver)
[ ... ]
> err_module:
> - cancel_work_sync(&nu->write_work);
> + nci_uart_rx_stop(nu);
> + nci_uart_tx_stop(nu);
> kfree_skb(nu->tx_skb);
> nu->tx_skb = NULL;
> - kfree_skb(nu->rx_skb);
> - nu->rx_skb = NULL;
> skb_queue_purge(&nu->tx_q);
> - clear_bit(NCI_UART_SENDING, &nu->tx_state);
> - clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
> + clear_bit(NCI_UART_CLOSING, &nu->tx_state);
[Severity: Low]
Is the new field comment in include/net/nfc/nci_core.h accurate?
spinlock_t tx_lock; /* protects tx_state */
This clear_bit() of NCI_UART_CLOSING runs without tx_lock. Every other
tx_state access the patch adds holds tx_lock.
It is safe today only because nci_uart_rx_stop() has already cleared
NCI_UART_READY, and nci_uart_tx_wakeup() checks CLOSING || !READY under the
lock.
Should this clear take tx_lock, or should the comment be narrowed?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104062026.80599-1-shubham%40octane.security
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net v7 3/3] nfc: nfcmrvl: stop firmware work before unregistering
2026-11-04 6:20 [PATCH net v7 0/3] nfc: order NCI UART and firmware teardown Shubham Antil
2026-11-04 6:20 ` [PATCH net v7 1/3] nfc: nci: uart: serialize driver selection Shubham Antil
2026-11-04 6:20 ` [PATCH net v7 2/3] nfc: nci: uart: quiesce callbacks during teardown Shubham Antil
@ 2026-11-04 6:20 ` Shubham Antil
2026-11-07 3:23 ` netdev-bot+sashiko
2 siblings, 1 reply; 7+ messages in thread
From: Shubham Antil @ 2026-11-04 6:20 UTC (permalink / raw)
To: netdev; +Cc: oe-linux-nfc, david, horms, gio
nfcmrvl_nci_unregister_dev() destroys the NCI workqueues before draining
the firmware receive worker. That worker can still submit NCI commands,
while the later abort can release firmware state that it is processing.
The firmware timeout can also complete the same download in parallel with
the receive worker.
Add active and stopping states protected by the firmware receive lock.
Once completion or teardown starts, reject new firmware receives and wait
for active receive entry points. Shut down the firmware timer, cancel the
receive and timeout workers, and purge their queue before releasing
firmware state. Perform this shutdown before unregistering the NCI device
so its command workqueue remains available until firmware work has ended.
Drain the NCI command timer, command worker, and command queue as one
dependency chain. Delete the timer before and after cancelling command
work so a detached timer callback cannot queue work which rearms the timer.
This also removes command skbs which cannot run after command admission is
closed.
Queue timeout completion on the firmware workqueue and claim completion
under the receive lock so firmware state is released once. Hold received
frames until the initial reset pulse has finished, preventing an error path
from asserting reset while the start path later deasserts it. Report
firmware completion with the positive errno value expected by the NFC API.
Reject a new download while the current one is active or still completing,
so its firmware pointers and state cannot be replaced during asynchronous
processing or cleanup.
Initialize the firmware timer while setting up the receive workqueue.
This makes teardown valid even when a download was never started.
Serialize the stopping transition with firmware start through the NFC
device lock. Use non-synchronous timeout-work cancellation during
completion because
completion can run from the timeout work itself.
Discovered during an AI-assisted source audit using OpenAI Codex,
gpt-5.6-sol. Patch development and test planning used the same assistant.
Confirmed with an unmodified kernel and a pty-backed user-space controller
exercising firmware start, timeout, completion, and device teardown; no
error injection was used.
With the fix applied, the reproducer passed 500 firmware teardown cycles
and all five timer-boundary modes under KASAN, UBSAN, and lockdep, plus 200
cycles under strict-mode KCSAN. No physical Marvell controller was
available, and no in-tree selftest covers this firmware path.
Fixes: 3194c6870158 ("NFC: nfcmrvl: add firmware download support")
Signed-off-by: Shubham Antil <shubham@octane.security>
---
drivers/nfc/nfcmrvl/fw_dnld.c | 243 +++++++++++++++++++++++++---------
drivers/nfc/nfcmrvl/fw_dnld.h | 11 ++
drivers/nfc/nfcmrvl/main.c | 5 +-
3 files changed, 192 insertions(+), 67 deletions(-)
diff --git a/drivers/nfc/nfcmrvl/fw_dnld.c b/drivers/nfc/nfcmrvl/fw_dnld.c
index 8b9d52573..6e8608655 100644
--- a/drivers/nfc/nfcmrvl/fw_dnld.c
+++ b/drivers/nfc/nfcmrvl/fw_dnld.c
@@ -90,22 +90,50 @@ static struct sk_buff *alloc_lc_skb(struct nfcmrvl_private *priv, uint8_t plen)
return skb;
}
-static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
+static void fw_dnld_stop_cmds(struct nci_dev *ndev)
{
- if (priv->fw_dnld.fw) {
- release_firmware(priv->fw_dnld.fw);
- priv->fw_dnld.fw = NULL;
- priv->fw_dnld.header = NULL;
- priv->fw_dnld.binary_config = NULL;
- }
-
- atomic_set(&priv->ndev->cmd_cnt, 0);
-
- if (timer_pending(&priv->ndev->cmd_timer))
- timer_delete_sync(&priv->ndev->cmd_timer);
+ /*
+ * The timer callback can queue cmd_work after a workqueue flush, while
+ * cmd_work can rearm the timer. Drain both directions before purging
+ * commands which belong to the firmware state machine.
+ */
+ timer_delete_sync(&ndev->cmd_timer);
+ atomic_set(&ndev->cmd_cnt, 0);
+ cancel_work_sync(&ndev->cmd_work);
+ timer_delete_sync(&ndev->cmd_timer);
+ skb_queue_purge(&ndev->cmd_q);
+}
- if (timer_pending(&priv->fw_dnld.timer))
- timer_delete_sync(&priv->fw_dnld.timer);
+static void fw_dnld_over(struct nfcmrvl_private *priv, int error)
+{
+ struct nfcmrvl_fw_dnld *fw_dnld = &priv->fw_dnld;
+ char name[NFC_FIRMWARE_NAME_MAXSIZE + 1];
+ u32 result = error < 0 ? -error : error;
+ unsigned long flags;
+
+ spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+ if (!fw_dnld->active) {
+ spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+ return;
+ }
+ fw_dnld->active = false;
+ fw_dnld->completing = true;
+ spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+
+ strscpy(name, fw_dnld->name);
+ wait_event(fw_dnld->rx_wait, !READ_ONCE(fw_dnld->rx_active));
+ timer_delete_sync(&fw_dnld->timer);
+ /* Timeout and receive work share a single-thread workqueue. */
+ cancel_work(&fw_dnld->timeout_work);
+ fw_dnld_stop_cmds(priv->ndev);
+ skb_queue_purge(&fw_dnld->rx_q);
+
+ if (fw_dnld->fw) {
+ release_firmware(fw_dnld->fw);
+ fw_dnld->fw = NULL;
+ fw_dnld->header = NULL;
+ fw_dnld->binary_config = NULL;
+ }
nfc_info(priv->dev, "FW loading over (%d)]\n", error);
@@ -114,7 +142,11 @@ static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
nfcmrvl_chip_halt(priv);
}
- nfc_fw_download_done(priv->ndev->nfc_dev, priv->fw_dnld.name, error);
+ nfc_fw_download_done(priv->ndev->nfc_dev, name, result);
+
+ spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+ fw_dnld->completing = false;
+ spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
}
static void fw_dnld_timeout(struct timer_list *t)
@@ -122,6 +154,18 @@ static void fw_dnld_timeout(struct timer_list *t)
struct nfcmrvl_private *priv = timer_container_of(priv, t,
fw_dnld.timer);
+ queue_work(priv->fw_dnld.rx_wq, &priv->fw_dnld.timeout_work);
+}
+
+static void fw_dnld_timeout_work(struct work_struct *work)
+{
+ struct nfcmrvl_fw_dnld *fw_dnld = container_of(work,
+ struct nfcmrvl_fw_dnld,
+ timeout_work);
+ struct nfcmrvl_private *priv = container_of(fw_dnld,
+ struct nfcmrvl_private,
+ fw_dnld);
+
nfc_err(priv->dev, "FW loading timeout");
priv->fw_dnld.state = STATE_RESET;
fw_dnld_over(priv, -ETIMEDOUT);
@@ -395,62 +439,74 @@ static int process_state_boot(struct nfcmrvl_private *priv,
static void fw_dnld_rx_work(struct work_struct *work)
{
- int ret;
- struct sk_buff *skb;
struct nfcmrvl_fw_dnld *fw_dnld = container_of(work,
struct nfcmrvl_fw_dnld,
rx_work);
struct nfcmrvl_private *priv = container_of(fw_dnld,
struct nfcmrvl_private,
fw_dnld);
+ struct sk_buff *skb;
+ unsigned long flags;
+ int ret;
- while ((skb = skb_dequeue(&fw_dnld->rx_q))) {
- nfc_send_to_raw_sock(priv->ndev->nfc_dev, skb,
- RAW_PAYLOAD_NCI, NFC_DIRECTION_RX);
- switch (fw_dnld->state) {
- case STATE_RESET:
- ret = process_state_reset(priv, skb);
- break;
- case STATE_INIT:
- ret = process_state_init(priv, skb);
- break;
- case STATE_SET_REF_CLOCK:
- ret = process_state_set_ref_clock(priv, skb);
- break;
- case STATE_SET_HI_CONFIG:
- ret = process_state_set_hi_config(priv, skb);
- break;
- case STATE_OPEN_LC:
- ret = process_state_open_lc(priv, skb);
- break;
- case STATE_FW_DNLD:
- ret = process_state_fw_dnld(priv, skb);
- break;
- case STATE_CLOSE_LC:
- ret = process_state_close_lc(priv, skb);
- break;
- case STATE_BOOT:
- ret = process_state_boot(priv, skb);
- break;
- default:
- ret = -EFAULT;
- }
+ skb = skb_dequeue(&fw_dnld->rx_q);
+ if (!skb)
+ return;
- kfree_skb(skb);
+ nfc_send_to_raw_sock(priv->ndev->nfc_dev, skb,
+ RAW_PAYLOAD_NCI, NFC_DIRECTION_RX);
+ switch (fw_dnld->state) {
+ case STATE_RESET:
+ ret = process_state_reset(priv, skb);
+ break;
+ case STATE_INIT:
+ ret = process_state_init(priv, skb);
+ break;
+ case STATE_SET_REF_CLOCK:
+ ret = process_state_set_ref_clock(priv, skb);
+ break;
+ case STATE_SET_HI_CONFIG:
+ ret = process_state_set_hi_config(priv, skb);
+ break;
+ case STATE_OPEN_LC:
+ ret = process_state_open_lc(priv, skb);
+ break;
+ case STATE_FW_DNLD:
+ ret = process_state_fw_dnld(priv, skb);
+ break;
+ case STATE_CLOSE_LC:
+ ret = process_state_close_lc(priv, skb);
+ break;
+ case STATE_BOOT:
+ ret = process_state_boot(priv, skb);
+ break;
+ default:
+ ret = -EFAULT;
+ }
- if (ret != 0) {
- nfc_err(priv->dev, "FW loading error");
- fw_dnld_over(priv, ret);
- break;
- }
+ kfree_skb(skb);
+
+ if (ret) {
+ nfc_err(priv->dev, "FW loading error");
+ fw_dnld_over(priv, ret);
}
+
+ spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+ if (!fw_dnld->stopping && !fw_dnld->starting && fw_dnld->active &&
+ !skb_queue_empty(&fw_dnld->rx_q))
+ queue_work(fw_dnld->rx_wq, &fw_dnld->rx_work);
+ spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
}
int nfcmrvl_fw_dnld_init(struct nfcmrvl_private *priv)
{
char name[32];
+ spin_lock_init(&priv->fw_dnld.rx_lock);
+ init_waitqueue_head(&priv->fw_dnld.rx_wait);
INIT_WORK(&priv->fw_dnld.rx_work, fw_dnld_rx_work);
+ INIT_WORK(&priv->fw_dnld.timeout_work, fw_dnld_timeout_work);
+ timer_setup(&priv->fw_dnld.timer, fw_dnld_timeout, 0);
snprintf(name, sizeof(name), "%s_nfcmrvl_fw_dnld_rx_wq",
dev_name(&priv->ndev->nfc_dev->dev));
priv->fw_dnld.rx_wq = create_singlethread_workqueue(name);
@@ -462,26 +518,64 @@ int nfcmrvl_fw_dnld_init(struct nfcmrvl_private *priv)
void nfcmrvl_fw_dnld_deinit(struct nfcmrvl_private *priv)
{
+ nfcmrvl_fw_dnld_abort(priv);
destroy_workqueue(priv->fw_dnld.rx_wq);
}
void nfcmrvl_fw_dnld_recv_frame(struct nfcmrvl_private *priv,
struct sk_buff *skb)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&priv->fw_dnld.rx_lock, flags);
+ if (priv->fw_dnld.stopping || !priv->fw_dnld.active) {
+ spin_unlock_irqrestore(&priv->fw_dnld.rx_lock, flags);
+ kfree_skb(skb);
+ return;
+ }
+ priv->fw_dnld.rx_active++;
+ spin_unlock_irqrestore(&priv->fw_dnld.rx_lock, flags);
+
/* Discard command timer */
- if (timer_pending(&priv->ndev->cmd_timer))
- timer_delete_sync(&priv->ndev->cmd_timer);
+ timer_delete_sync(&priv->ndev->cmd_timer);
/* Allow next command */
atomic_set(&priv->ndev->cmd_cnt, 1);
- /* Queue and trigger rx work */
- skb_queue_tail(&priv->fw_dnld.rx_q, skb);
- queue_work(priv->fw_dnld.rx_wq, &priv->fw_dnld.rx_work);
+ spin_lock_irqsave(&priv->fw_dnld.rx_lock, flags);
+ if (!priv->fw_dnld.stopping && priv->fw_dnld.active) {
+ /* Queue the frame, but do not process it during the reset pulse. */
+ skb_queue_tail(&priv->fw_dnld.rx_q, skb);
+ if (!priv->fw_dnld.starting)
+ queue_work(priv->fw_dnld.rx_wq,
+ &priv->fw_dnld.rx_work);
+ skb = NULL;
+ }
+ priv->fw_dnld.rx_active--;
+ if (!priv->fw_dnld.rx_active &&
+ (priv->fw_dnld.stopping || !priv->fw_dnld.active))
+ wake_up_all(&priv->fw_dnld.rx_wait);
+ spin_unlock_irqrestore(&priv->fw_dnld.rx_lock, flags);
+ kfree_skb(skb);
}
void nfcmrvl_fw_dnld_abort(struct nfcmrvl_private *priv)
{
+ struct nfcmrvl_fw_dnld *fw_dnld = &priv->fw_dnld;
+ unsigned long flags;
+
+ device_lock(&priv->ndev->nfc_dev->dev);
+ spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+ fw_dnld->stopping = true;
+ spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+ device_unlock(&priv->ndev->nfc_dev->dev);
+
+ wait_event(fw_dnld->rx_wait, !READ_ONCE(fw_dnld->rx_active));
+ timer_shutdown_sync(&fw_dnld->timer);
+ cancel_work_sync(&fw_dnld->rx_work);
+ cancel_work_sync(&fw_dnld->timeout_work);
+ skb_queue_purge(&fw_dnld->rx_q);
+
fw_dnld_over(priv, -EHOSTDOWN);
}
@@ -489,6 +583,7 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
{
struct nfcmrvl_private *priv = nci_get_drvdata(ndev);
struct nfcmrvl_fw_dnld *fw_dnld = &priv->fw_dnld;
+ unsigned long flags;
int res;
if (!priv->support_fw_dnld)
@@ -497,6 +592,14 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
if (!firmware_name || !firmware_name[0])
return -EINVAL;
+ spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+ if (fw_dnld->stopping || fw_dnld->active || fw_dnld->completing) {
+ res = fw_dnld->stopping ? -ENODEV : -EBUSY;
+ spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+ return res;
+ }
+ spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+
strscpy(fw_dnld->name, firmware_name);
/*
@@ -520,6 +623,7 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
firmware_name, fw_dnld->header->magic,
fw_dnld->header->phy);
release_firmware(fw_dnld->fw);
+ fw_dnld->fw = NULL;
fw_dnld->header = NULL;
return -EINVAL;
}
@@ -532,11 +636,6 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
fw_dnld->binary_config = &fw_dnld->header->firmware;
}
- /* Configure a timer for timeout */
- timer_setup(&priv->fw_dnld.timer, fw_dnld_timeout, 0);
- mod_timer(&priv->fw_dnld.timer,
- jiffies + msecs_to_jiffies(FW_DNLD_TIMEOUT));
-
/* Ronfigure HI to be sure that it is the bootrom values */
priv->if_ops->nci_update_config(priv,
&fw_dnld->header->bootrom.config);
@@ -546,8 +645,24 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
/* First, reset the chip */
priv->fw_dnld.state = STATE_RESET;
+
+ spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+ fw_dnld->active = true;
+ fw_dnld->starting = true;
+ spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+
nfcmrvl_chip_reset(priv);
+ spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+ fw_dnld->starting = false;
+ if (fw_dnld->active) {
+ mod_timer(&fw_dnld->timer,
+ jiffies + msecs_to_jiffies(FW_DNLD_TIMEOUT));
+ if (!skb_queue_empty(&fw_dnld->rx_q))
+ queue_work(fw_dnld->rx_wq, &fw_dnld->rx_work);
+ }
+ spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+
/* Now wait for CORE_RESET_NTF or timeout */
return 0;
diff --git a/drivers/nfc/nfcmrvl/fw_dnld.h b/drivers/nfc/nfcmrvl/fw_dnld.h
index 7c4d91b01..a8ed7bfcc 100644
--- a/drivers/nfc/nfcmrvl/fw_dnld.h
+++ b/drivers/nfc/nfcmrvl/fw_dnld.h
@@ -8,6 +8,8 @@
#ifndef __NFCMRVL_FW_DNLD_H__
#define __NFCMRVL_FW_DNLD_H__
+#include <linux/spinlock.h>
+#include <linux/wait.h>
#include <linux/workqueue.h>
#define NFCMRVL_FW_MAGIC 0x88888888
@@ -72,7 +74,16 @@ struct nfcmrvl_fw_dnld {
struct workqueue_struct *rx_wq;
struct work_struct rx_work;
+ struct work_struct timeout_work;
struct sk_buff_head rx_q;
+ /* Protects stopping, starting, active, completing, and rx_active. */
+ spinlock_t rx_lock;
+ wait_queue_head_t rx_wait;
+ unsigned int rx_active;
+ bool stopping;
+ bool starting;
+ bool active;
+ bool completing;
struct timer_list timer;
};
diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 81c760534..02ea3356b 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -185,13 +185,12 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
{
struct nci_dev *ndev = priv->ndev;
+ nfcmrvl_fw_dnld_abort(priv);
nci_unregister_device(ndev);
+
if (priv->if_ops->nci_quiesce)
priv->if_ops->nci_quiesce(priv);
- if (priv->ndev->nfc_dev->fw_download_in_progress)
- nfcmrvl_fw_dnld_abort(priv);
-
nfcmrvl_fw_dnld_deinit(priv);
nci_free_device(ndev);
--
2.54.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net v7 3/3] nfc: nfcmrvl: stop firmware work before unregistering
2026-11-04 6:20 ` [PATCH net v7 3/3] nfc: nfcmrvl: stop firmware work before unregistering Shubham Antil
@ 2026-11-07 3:23 ` netdev-bot+sashiko
0 siblings, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-11-07 3:23 UTC (permalink / raw)
To: shubham; +Cc: netdev, oe-linux-nfc, david, horms, gio, kuba
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 0 (1) · Medium: 2 · Low: 2 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The new -EBUSY return in nfcmrvl_fw_dnld_start()
(fw_dnld.c:595-600) leaves the NFC core's fw_download_in_progress flag…
- [Medium] The reset-pulse hold-off is enforced only when work is queued,
not inside the worker.
- [Low] The commit carries a Fixes: tag but bundles several changes
beyond the teardown UAF fix, and some of them are not in the commit…
- [Low] fw_dnld_over() calls nfc_fw_download_done() (fw_dnld.c:145)
before it clears fw_dnld->completing under rx_lock (lines 147-149).
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the firmware-
download state machine the patch reworks trusts a 16-bit chunk length…
- [Low] Frames that arrive while nfcmrvl_chip_reset() holds reset
asserted are queued in rx_q (nfcmrvl_fw_dnld_recv_frame(),…
--
nfc: nfcmrvl: stop firmware work before unregistering
This patch adds active, stopping, starting and completing state, protected
by a new rx_lock, to the nfcmrvl firmware download path. It moves timeout
completion onto the firmware workqueue and drains the firmware and NCI
command machinery before nci_unregister_device(). It also rejects
concurrent downloads, holds frames received during the reset pulse, and
reports completion with a positive errno.
> diff --git a/drivers/nfc/nfcmrvl/fw_dnld.c b/drivers/nfc/nfcmrvl/fw_dnld.c
> index 8b9d5257320dc..6e8608655710d 100644
> --- a/drivers/nfc/nfcmrvl/fw_dnld.c
> +++ b/drivers/nfc/nfcmrvl/fw_dnld.c
> @@ -90,22 +90,50 @@ static struct sk_buff *alloc_lc_skb(struct nfcmrvl_private *priv, uint8_t plen)
> return skb;
> }
[Severity: High]
This isn't a bug introduced by this patch, but this patch reworks the
firmware receive path, and that path trusts the chunk length the
controller sends.
In process_state_fw_dnld(), SUBSTATE_WAIT_COMMAND only checks len against
its complement before storing it:
len = get_unaligned_le16(skb->data);
...
priv->fw_dnld.chunk_len = len;
so chunk_len can be as large as 0xFFFF. SUBSTATE_WAIT_ACK_CREDIT then does:
out_skb = alloc_lc_skb(priv, priv->fw_dnld.chunk_len);
...
skb_put_data(out_skb,
((uint8_t *)priv->fw_dnld.fw->data) + priv->fw_dnld.offset,
priv->fw_dnld.chunk_len);
alloc_lc_skb() takes uint8_t plen, so the skb is sized for
chunk_len & 0xff, but skb_put_data() copies the full chunk_len. Can a
controller that sends a chunk_len above 255 trigger skb_over_panic() here?
Separately, SUBSTATE_WAIT_DATA_CREDIT does:
priv->fw_dnld.offset += priv->fw_dnld.chunk_len;
Neither offset nor binary_config->offset from the firmware header is ever
checked against fw->size. Can repeated chunks of 255 bytes or less read
past the end of fw->data?
Those bytes go out through nci_send_frame() and nfc_send_to_raw_sock().
On UART the controller can be a pty attached with N_NCI, which has no
capability check. A CAP_NET_ADMIN user still has to start the download
with NFC_CMD_FW_DOWNLOAD.
[ ... ]
> @@ -114,7 +142,11 @@ static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
> nfcmrvl_chip_halt(priv);
> }
>
> - nfc_fw_download_done(priv->ndev->nfc_dev, priv->fw_dnld.name, error);
> + nfc_fw_download_done(priv->ndev->nfc_dev, name, result);
> +
> + spin_lock_irqsave(&fw_dnld->rx_lock, flags);
> + fw_dnld->completing = false;
> + spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
> }
[Severity: Low]
Can a new download get a spurious -EBUSY because of this ordering?
nfc_fw_download_done() clears dev->fw_download_in_progress and multicasts
the genl download-done event before completing is cleared. fw_dnld_over()
does not hold device_lock. An NFC_CMD_FW_DOWNLOAD sent in response to that
event can therefore reach nfcmrvl_fw_dnld_start() while completing is
still true:
if (fw_dnld->stopping || fw_dnld->active || fw_dnld->completing) {
res = fw_dnld->stopping ? -ENODEV : -EBUSY;
Userspace has been told the download finished, but the next request fails
with -EBUSY. Swapping the two steps doesn't look safe either: the old
completion could then clear fw_download_in_progress for the new download.
[ ... ]
> @@ -395,62 +439,74 @@ static int process_state_boot(struct nfcmrvl_private *priv,
>
> static void fw_dnld_rx_work(struct work_struct *work)
> {
[ ... ]
> + skb = skb_dequeue(&fw_dnld->rx_q);
> + if (!skb)
> + return;
>
> - kfree_skb(skb);
> + nfc_send_to_raw_sock(priv->ndev->nfc_dev, skb,
> + RAW_PAYLOAD_NCI, NFC_DIRECTION_RX);
> + switch (fw_dnld->state) {
> + case STATE_RESET:
> + ret = process_state_reset(priv, skb);
> + break;
[Severity: Medium]
Should this worker check starting, active and stopping under rx_lock
before it dequeues?
The reset-pulse hold-off is only enforced where work is queued: in
nfcmrvl_fw_dnld_recv_frame() and in the requeue at the end of this
function. A pending rx_work left over from an earlier download can still
run. fw_dnld_over() cancels timeout_work and purges rx_q, but it never
cancels rx_work:
cancel_work(&fw_dnld->timeout_work);
fw_dnld_stop_cmds(priv->ndev);
skb_queue_purge(&fw_dnld->rx_q);
rx_work can be left pending in two ways:
- recv_frame queues it behind timeout_work on the ordered rx_wq.
- recv_frame requeues it while rx_work itself is inside fw_dnld_over(),
called from process_state_boot() or an error.
If userspace then starts a new download:
nfcmrvl_fw_dnld_start()
active = true; starting = true;
nfcmrvl_chip_reset()
reset asserted, usleep_range(5000, 10000)
frame arrives, recv_frame queues it without queue_work()
stale rx_work runs fw_dnld_rx_work()
skb_dequeue() -> process_state_reset() returns -EINVAL
fw_dnld_over() -> nfcmrvl_chip_halt()
reset deasserted
active == false, so no timer is armed, and start returns 0
Isn't this the case the commit message says it prevents, where an error
path asserts reset and the start path later deasserts it?
Would either of these close it:
- checking the flags in fw_dnld_rx_work() before skb_dequeue(), or
- calling cancel_work_sync(&fw_dnld->rx_work) in
nfcmrvl_fw_dnld_start() before setting starting?
[ ... ]
> @@ -497,6 +592,14 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
> if (!firmware_name || !firmware_name[0])
> return -EINVAL;
>
> + spin_lock_irqsave(&fw_dnld->rx_lock, flags);
> + if (fw_dnld->stopping || fw_dnld->active || fw_dnld->completing) {
> + res = fw_dnld->stopping ? -ENODEV : -EBUSY;
> + spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
> + return res;
> + }
> + spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
[Severity: Medium]
What happens to the NFC core's fw_download_in_progress flag when this
returns -EBUSY for a second download?
nfc_fw_download() does not check for a download that is already running,
and it clears the flag on any driver error:
net/nfc/core.c:nfc_fw_download() {
...
dev->fw_download_in_progress = true;
rc = dev->ops->fw_download(dev, firmware_name);
if (rc)
dev->fw_download_in_progress = false;
...
}
So a second NFC_CMD_FW_DOWNLOAD sent while download A is active gets
-EBUSY. The core then records that no download is in progress, even
though A's state machine and timer are still live.
nfcmrvl_nci_recv_frame() decides where frames go based only on that
flag:
if (priv->ndev->nfc_dev->fw_download_in_progress) {
nfcmrvl_fw_dnld_recv_frame(priv, skb);
return 0;
}
With the flag cleared, A's frames are dropped and A stalls until the
timeout. On UART, nfcmrvl_nci_uart_tx_start() and
nfcmrvl_nci_uart_tx_done() also stop skipping the BREAK handling partway
through the download.
nfc_dev_up() rejects only on fw_download_in_progress, so the NCI device
can be brought up while A is still active. Later, A's timeout_work reaches
fw_dnld_over() and does two things to the running device:
- fw_dnld_stop_cmds() sets cmd_cnt to 0, cancels cmd_work and purges
cmd_q.
- nfcmrvl_chip_halt() asserts reset underneath it.
Before this patch the second request returned 0 and the flag stayed set.
[ ... ]
> @@ -546,8 +645,24 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
>
> /* First, reset the chip */
> priv->fw_dnld.state = STATE_RESET;
> +
> + spin_lock_irqsave(&fw_dnld->rx_lock, flags);
> + fw_dnld->active = true;
> + fw_dnld->starting = true;
> + spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
> +
> nfcmrvl_chip_reset(priv);
>
> + spin_lock_irqsave(&fw_dnld->rx_lock, flags);
> + fw_dnld->starting = false;
> + if (fw_dnld->active) {
> + mod_timer(&fw_dnld->timer,
> + jiffies + msecs_to_jiffies(FW_DNLD_TIMEOUT));
> + if (!skb_queue_empty(&fw_dnld->rx_q))
> + queue_work(fw_dnld->rx_wq, &fw_dnld->rx_work);
> + }
> + spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
[Severity: Low]
This is a pre-existing issue, but should frames that arrive while reset is
asserted go to the STATE_RESET handler at all?
nfcmrvl_fw_dnld_recv_frame() keeps frames that arrive during the pulse in
nfcmrvl_chip_reset() in rx_q, and they are processed here once the pulse
ends. A frame received while reset is held is unlikely to be the
post-reset CORE_RESET_NTF. If it doesn't match, process_state_reset()
returns -EINVAL and fw_dnld_over() halts the chip.
Before this patch, the same frames were processed in STATE_RESET straight
away. Purging them may not be right either: the real CORE_RESET_NTF could
arrive between the reset deassert and the spin_lock_irqsave() above.
[ ... ]
> diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
> index 81c760534cf68..02ea3356b2abb 100644
> --- a/drivers/nfc/nfcmrvl/main.c
> +++ b/drivers/nfc/nfcmrvl/main.c
> @@ -185,13 +185,12 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
> {
> struct nci_dev *ndev = priv->ndev;
>
> + nfcmrvl_fw_dnld_abort(priv);
> nci_unregister_device(ndev);
> +
> if (priv->if_ops->nci_quiesce)
> priv->if_ops->nci_quiesce(priv);
>
> - if (priv->ndev->nfc_dev->fw_download_in_progress)
> - nfcmrvl_fw_dnld_abort(priv);
> -
> nfcmrvl_fw_dnld_deinit(priv);
[Severity: Low]
This isn't a bug, but nfcmrvl_fw_dnld_abort() now runs unconditionally
here, and nfcmrvl_fw_dnld_deinit() calls it a second time a few lines
later:
void nfcmrvl_fw_dnld_deinit(struct nfcmrvl_private *priv)
{
nfcmrvl_fw_dnld_abort(priv);
destroy_workqueue(priv->fw_dnld.rx_wq);
}
The deinit call also runs on the nfcmrvl_nci_register_dev() error path,
where no download was ever started. Are both calls needed?
The commit message also doesn't mention two other changes in this patch:
- fw_dnld_rx_work() no longer drains rx_q in a loop. It processes one
skb per run and requeues itself.
- The bad magic/phy path in nfcmrvl_fw_dnld_start() now also clears
fw_dnld->fw.
The message does describe these changes, which land in the same patch:
- a positive errno in the download status userspace sees
- -EBUSY for a second download
- holding frames during the reset pulse
- moving timeout completion to a workqueue
That is a lot of behaviour change in one Fixes-tagged patch. Could some of
it be split out so the teardown fix can be reviewed and backported on its
own?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104062026.80599-1-shubham%40octane.security
^ permalink raw reply [flat|nested] 7+ messages in thread