Re: [PATCH net v6 1/6] nfc: nci: skip controller reset while unregistering

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 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 patch skips CORE_RESET_CMD for every caller of
  nci_unregister_device(), not only for transports that are detaching.
- [Medium] The NCI_UNREG check only removes the CORE_RESET that close
  sends itself.
- [Medium] The new NCI_UNREG gate cannot affect a reset (or init) request
  that is already in flight when line discipline close starts.
- [Low] On the unregister path, nci_close_device() no longer sets
  NCI_INIT, but it still calls ndev->ops->close().

--

nfc: nci: skip controller reset while unregistering

This patch makes nci_close_device() skip sending CORE_RESET (and skip
setting NCI_INIT) when NCI_UNREG is set. The motivation is that on line
discipline close the transport can no longer deliver the response, so close
waits for the full reset timeout.

> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> index 73e3a96470ace..8ee33505e93ad 100644
> --- a/net/nfc/nci/core.c
> +++ b/net/nfc/nci/core.c
> @@ -582,13 +582,15 @@ static int nci_close_device(struct nci_dev *ndev)
>  	/* Flush TX wq, RX wq flush can't be under the lock */
>  	flush_workqueue(ndev->tx_wq);
>  
> -	/* Reset device */
> +	/* Reset device unless it is being unregistered */
>  	skb_queue_purge(&ndev->cmd_q);
> -	atomic_set(&ndev->cmd_cnt, 1);
>  
> -	set_bit(NCI_INIT, &ndev->flags);
> -	__nci_request(ndev, nci_reset_req, (void *)0,
> -		      msecs_to_jiffies(NCI_RESET_TIMEOUT));
> +	if (!test_bit(NCI_UNREG, &ndev->flags)) {
> +		atomic_set(&ndev->cmd_cnt, 1);
> +		set_bit(NCI_INIT, &ndev->flags);
> +		__nci_request(ndev, nci_reset_req, (void *)0,
> +			      msecs_to_jiffies(NCI_RESET_TIMEOUT));
> +	}

[Severity: Medium]
This skips the reset for every caller of nci_unregister_device(), not only
for transports that are detaching. Is that intended for drivers whose bus
still works during remove() and whose ops->close() does nothing?

For nfcmrvl over I2C/SPI, nfcmrvl_nci_close() only calls the per-bus close
function, and that function is empty:

drivers/nfc/nfcmrvl/i2c.c:nfcmrvl_i2c_nci_close() {
	return 0;
}

FDP is the same:

drivers/nfc/fdp/fdp.c:fdp_nci_close() {
	return 0;
}

nfcmrvl_nci_unregister_dev() also never calls nfcmrvl_chip_halt(). The
I2C/SPI IRQs are devm-managed, so they are still live during remove().

Before this patch, the CORE_RESET went out over the live bus with NCI_INIT
set. nci_recv_frame() accepted the response, and the NFCC went back to
reset.

With this change, suppose the device is up when nfcmrvl_i2c_remove(),
nfcmrvl_spi_remove() or fdp_nci_remove() runs. Can the controller then stay
in its last RF state (polling with the field on, listen mode, or an active
interface) after the kernel has cleared all of its state?

The commit message says:

  Keep the reset for a normal device-down operation, where the transport
  remains attached and can deliver the response.

For these drivers, the transport is also still attached during unregister.

Later patches in the series don't seem to change this:

- nci_close_device() stays the same.
- nfcmrvl_fw_dnld_abort() returns early when no download is active.
- The new nci_quiesce hook exists only for UART.

[Severity: Low]
On the unregister path, test_and_clear_bit() at the top of
nci_close_device() has already cleared NCI_UP. Now NCI_INIT is not set
either. But ndev->ops->close(ndev) still runs right after this block, and
for ST NCI that callback sends a request:

st_nci_close()
  ndlc_close()
    nci_prop_cmd(ndlc->ndev, ST_NCI_CORE_PROP, ...)
      __nci_request(..., msecs_to_jiffies(NCI_CMD_TIMEOUT))

The reply comes in through llt_ndlc_rcv_queue() -> nci_recv_frame(), which
drops it:

	if (!ndev || (!test_bit(NCI_UP, &ndev->flags) &&
	    !test_bit(NCI_INIT, &ndev->flags))) {

Will the mode-set command in ndlc_close() now always time out after 5
seconds and log a pr_err during unregister?

The impact is limited. st_nci_remove() already calls ndlc_close() directly
before nci_unregister_device(), and that call disables the IRQ, so the old
CORE_RESET also timed out. The stall moves from the reset to the mode-set
command, and total unbind time stays about the same.

[Severity: Medium]
Does this cover a reset or init request that is already running when line
discipline close starts?

nfc_dev_down() takes device_lock and calls nci_dev_down() ->
nci_close_device(). NCI_UNREG isn't set yet, so __nci_request(nci_reset_req)
runs and waits for up to NCI_RESET_TIMEOUT. nfc_dev_up() ->
nci_open_device() does the same with its reset and init requests.

Meanwhile, with the ldisc write lock held:

nci_uart_tty_close()
  tty->disc_data = NULL;
  nu->ops.close(nu)
    nfcmrvl_nci_unregister_dev()
      nci_unregister_device()
        nfc_unregister_rfkill()
          device_lock(&dev->dev);   <- blocks behind nfc_dev_down()

This block happens before set_bit(NCI_UNREG) and before nci_req_cancel() in
nci_close_device().

The pending response can't arrive, because disc_data is NULL and the ldisc
lock is held for writing. So the waiter runs out the full 5000 ms timeout,
and ldisc close waits along with it.

This is the "close waits for the full timeout" symptom from the commit
message, reached through a race. The rest of the series doesn't seem to
change it either. The later UART quiesce patch stops rx/tx before ops.close,
which only makes sure the response never arrives.

Would it work better to mark the device as unregistering and cancel pending
requests before serializing on device_lock? Another option is to make
in-flight requests abort when the transport detaches.

[Severity: Medium]
A related question about requests already in flight. nci_req_cancel() runs
only once, at the top of nci_close_device(). It only completes a request if
one is pending at that exact moment:

net/nfc/nci/core.c:nci_req_cancel() {
	if (ndev->req_status == NCI_REQ_PEND) {
	...
}

nci_open_device() checks NCI_UNREG only on entry:

	if (test_bit(NCI_UNREG, &ndev->flags)) {

After that check, its __nci_request() calls (reset/init, 5000 ms timeouts)
wait out their full timeouts while device_lock is held.

Say NFC_CMD_DEV_UP is waiting there when nci_uart_tty_close() starts.
Unregister then blocks in device_lock() in nfc_unregister_rfkill() until
the timeout expires.

Doesn't that still leave ldisc close stalled for NCI_RESET_TIMEOUT or
NCI_INIT_TIMEOUT, even with the new NCI_UNREG check?

This overlaps with the device_lock ordering question above. Other nfc core
ops take device_lock and check shutting_down, so no new request should be
able to start after nfc_unregister_rfkill().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021032443.44412-1-shubham%40octane.security