Re: [PATCH net] nfc: st21nfca: complete a pending SE APDU request only once

[Simon Horman <horms@kernel.org>]

On Sat, Oct 31, 2026 at 04:44:28PM -0300, Joas Antonio wrote:
> Hi Simon,
> 
> Thanks, you're right.
> 
> The two completion paths are serialised against each other:
> st21nfca_apdu_reader_event_received() does timer_delete_sync() and
> cancel_work_sync() before it looks at bwi_active, and the timer is only
> re-armed from the same HCI rx work (EVT_WTX_REQUEST). But
> st21nfca_hci_se_io() runs from the netlink path under device_lock()
> only (nfc_se_io() in net/nfc/netlink.c), which neither the rx work nor
> the timeout work takes. So:
> 
> - bwi_active, cb and cb_context are plain fields written by
>   st21nfca_hci_se_io() while the rx work or the timeout work may be
>   reading them;
> - a second SE_IO issued while one is pending overwrites cb and
>   cb_context, so a completion running at the same time can pair one
>   request's callback with the other's context, and the first context is
>   never completed. That is not new with this patch, but the bwi_active
>   check does not cover it either;
> - st21nfca_hci_se_io() sets bwi_active only after mod_timer().
> 
> For v2 I plan to add a spinlock to se_info that covers bwi_active, cb
> and cb_context: st21nfca_hci_se_io() fails with -EBUSY (freeing
> cb_context, as on the existing error path) if a request is already
> pending, otherwise records it and sets bwi_active before arming the
> timer; the timeout work and the EVT_TRANSMIT_DATA handler take the
> pending request out under the lock (test and clear bwi_active, copy cb
> and cb_context) and call the callback after dropping it, so exactly one
> of them completes it.
> 
> Does that sound reasonable? I'll rerun the emulator tests on it before
> sending.

Thanks Joas,

I am wary of scope creep. But as some locking does seem necessary
for using bwi_active to be non-racy in this patch, then yes, I agree
this is a good approach.

You may want to consider two patches in a patch-set: one to add the lock,
and another being v2 of this patch, if the changes seem to be distict even
if one is dependent on the other.

...