drain_workqueue() allows only chained work to be queued while draining.
This is inconvenient because work can be queued from contexts other than
work items already executing on the workqueue.
Use flush_workqueue() instead to allow work to be queued from any
context.
Link: https://lore.kernel.org/r/20261019122555.328060-3-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
schedule_error_callback() requires a lock to be held by the caller.
However, __fw_send_request() calls the function without holding any lock
when retrieving the transaction label fails, as no lock is needed in
this case. context_safe() is used to suppress the precondition check in
compiler-based context analysis, but it does not affect lockdep checks.
As a result, lockdep reports a violation of the precondition.
Acquire the lock before calling the function instead of using
context_unsafe().
Link: https://lore.kernel.org/r/20261019122555.328060-2-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Current implementation switches the underlying driver of a card instance
to the dummy driver in the kernel stack when removing the card. The dummy
driver simply invokes callbacks when sending request and response
packets.
Recent changes for the 1394 OHCI driver and core transaction layer ensure
that callbacks are invoked in process context from a workqueue. This
guarantees a consistent execution context for callbacks. However, card
removel is currently an exception.
Invoke packet callbacks from a workqueue during card removal.
Link: https://lore.kernel.org/r/20261018132955.391090-3-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Current implementation requires that the transaction not be on the
pending list before its error callback is scheduled. A WARN_ON() in the
callback function detects violations of this precondition, but the check
belongs in the scheduling helper.
Move the WARN_ON() to schedule_error_callback().
Link: https://lore.kernel.org/r/20261018063056.329226-3-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
When a card is being removed, all pending transactions are canceled by
calling fw_cancel_pending_transactions(). This function moves the entries
for unfinished transactions to a local list so that their callbacks can
be invoked without holding the transaction lock. However, this complicates
the management of pending transactions.
Use workqueue to invoke error callbacks for all pending transactions.
Link: https://lore.kernel.org/r/20261018063056.329226-2-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
The error callback work is scheduled only when the transaction instance
is no longer on the pending list. Therefore, iterating over the list to
cancel the work is pointless.
Remove the code that cancels the work while iterating over the pending
list. Once scheduled, the work can run and invoke the error callback
without being cancelled.
Link: https://lore.kernel.org/r/20261017153339.267337-2-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
When removing a card instance, the underlying driver is replaced to handle
closing operations. Without proper memory reordering, other CPUs may
observe the driver before it is fully initialized.
Use a release/acquire pair to ensure that the driver's initialization is
visible before the driver is accessed for close operations.
Link: https://lore.kernel.org/r/20261017081720.186688-1-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
The spinlocks in the fw_card structure protect transaction-related state
from concurrent access. They are acquired mostly from process context.
The exceptions are split_transaction_timeout_callback(), which runs in
softirq context for the timer wheel, and __fw_send_request(), which may
also be called from interrupt context.
Use spin_lock() in split_transaction_timeout_callback() and
spin_lock_irqsave() in the other functions. The latter preserves and
restores the caller's interrupt state, as the callers are not guaranteed
to have interrupts enabled.
Link: https://lore.kernel.org/r/20261012015739.530731-1-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
When preparing the request subaction for an asynchronous transaction,
the node_id field in the fw_card structure is accessed under the
card-level spinlock. However, the lock is held across a call to a helper
function, unnecessarily extending the critical section.
Narrow the critical section to cover only the access to node_id.
Link: https://lore.kernel.org/r/20261011063146.497455-7-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
The transaction lock protects the list of pending transactions and the
transaction label.
Add context analysis annotations for the transaction lock. Compiler-based
context analysis has difficulty tracking the locking scope when the lock
is accessed through an indirect reference (e.g. t->card->lock), making
some functions difficult to annotate. Use lockdep assertions to cover
these cases.
Link: https://lore.kernel.org/r/20261011063146.497455-4-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
The kernel is transitioning from Sparse-based context analysis to
compier-based context analysis, which requires LLVM/Clang 23 or later.
Individual compilation units can opt in to the analysis through their
Makefiles.
Enable context analysis for the transaction layer.
Link: https://lore.kernel.org/r/20261011063146.497455-3-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
The current implementation invokes the transaction callback in the
context where transaction cancellation is requested. This is
inconvenient because the context can affect the transaction callback.
Invoke the transaction callback from a workqueue instead. Use the
workqueue for error handling.
Link: https://lore.kernel.org/r/20261007093235.175742-5-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
The current implementation invokes the packet callback in the context
where the packet cancellation is requested. This is inconvenient because
the context can affect the packet callback.
Invoke the packet callback from a workqueue instead. Use the workqueue
for local AT packets.
Link: https://lore.kernel.org/r/20261007093235.175742-3-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
The return value from close_transaction() is useful only in
fw_cancel_transaction(). The other caller,
transmit_complete_callback(), simply discards it.
Additionally, the pending transaction queue should be enumerated as soon
as possible to prevent the issued transaction from completing.
Expand close_transaction() into its callers, and remove it.
In the find_and_pop_transaction_entry() macro, a local variable provides
the evaluation result, however transmit_complete_callback() uses the same
local variable for another purpose. Add an underscore prefix to the one
in the macro to avoid the unexpected result.
Link: https://lore.kernel.org/r/20261007093235.175742-2-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
fw_core_handle_request() is now called in process context, so GFP_KERNEL
is available. The fw_request structure has a trailing flexible array
member of quadlets to store packet data. Use kzalloc_flex() to allocate
the structure together with storage for the flexible array member.
Link: https://lore.kernel.org/r/20260928125718.50974-2-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
The transition state of an iso_resource_auto client resource has no
effect on the client, so the client-level lock is not suitable for
protecting it.
At present, this state is the only member that requires mutual
exclusion. Adding a separate lock for it would be excessive.
Change the type of the transition state to atomic_t and use an atomic
compare-and-swap operation to preserve consistency.
Link: https://lore.kernel.org/r/20260927230724.119305-2-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
During packet cancellation, the work item for the AT request context is
disabled. As a result, packets for request subactions are not processed
by the work item even if they finished, since the work item can not be
scheduled from hardIRQ handling.
Run the work item immediately after re-enabling it.
Link: https://lore.kernel.org/r/20260927044407.65497-3-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
When fw_card_driver.cancel_packet() returns a non-zero value, the request
subaction of the transaction has already finished. It means that the
packet for the request subaction has already received an ack code and a
timestamp, so there is no need to check them.
Remove the unnecessary branch for the ack code and timestamp.
Link: https://lore.kernel.org/r/20260927044407.65497-2-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
The GCC and Clang attribute '__counted_by_ptr' associates a pointer
field with an integer field holding its element count. This enables
runtime bounds checking by KASAN and UBSAN to prevent out-of-bounds
accesses to the pointer.
In 'struct fw_device' (defined in 'include/linux/firewire.h'), the
field 'config_rom' is a pointer to the device's Configuration ROM data,
and its associated element count is stored in 'config_rom_length'.
Additionally, update the existing KUnit test in
'drivers/firewire/device-attribute-test.c' where 'config_rom_length'
was incorrectly initialized with the byte size
('sizeof(simple_avc_config_rom)') instead of the element count
('ARRAY_SIZE(simple_avc_config_rom)'). This ensures the
'__counted_by_ptr' bounds-checking annotation does not trigger any
false-positive panics or compile/runtime checks.
Cc: codemender-patching+linux@google.com
Assisted-by: LLM
Signed-off-by: Bill Wendling <morbo@google.com>
Link: https://lore.kernel.org/r/20260925212552.125652-1-morbo@google.com
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
The fw_iso_resource_auto object can outlive the operation that created
it since hte main operation for it is done in a work item. This work and
release paths access members of the client structure, therefore the
client structure must remain valid for the lifetime of an
fw_iso_resource_auto object.
Hold a reference to the client structure for the lifetime of the
fw_iso_resource_auto object.
Link: https://lore.kernel.org/r/20260923172328.277232-2-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Some error paths of __fw_send_request() invoke the transaction callback
in the caller's context. Additionally, when a split transaction times
out, the callback is invoked in softIRQ context by the timer wheel.
These are the only cases where the callback is not guaranteed to be
invoked in process context.
Use a workqueue to invoke the callback in these cases. This may introduce
additional delay when a split transaction times out, but the default
timeout is 2 seconds, so the additional delay should be negligible.
Link: https://lore.kernel.org/r/20260921031931.287429-3-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
In the cdev layer, the list of phy packet receivers is currently
protected by a spinlock. This requires GFP_ATOMIC for memory allocation
when enumerating the list in fw_cdev_handle_phy_packet(). However, the
function is guaranteed to be invoked in process context, so GFP_KERNEL
can be used by switching from the spinlock to a mutex.
Link: https://lore.kernel.org/r/20260920083659.191855-3-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Current implementation uses the spinlock in the card structure to
serialize accesses to the event list. The dequeue_event() function yields
the CPU while waiting for the condition to become true, but checks the
list without acquiring the spinlock. It acquires the spinlock when
operating the list, however it assumes that the list has at least one
entry.
This can cause problems when multiple threads execute read(2) system calls
concurrently, since the list can become empty after rescheduling but before
the list operation, depending on the scheduling order of the two threads.
Use a wait_event variant that reacquires the lock after being
rescheduled, before checking the condition.
Link: https://lore.kernel.org/r/20260920104745.212753-1-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
The bus reset event structure exposed through the UAPI has a different
size depending on the target architecture. Extra care is required when
copying it to/from userspace.
Add a KUnit test for the structure to ensure that it has the expected
size and offsets.
Link: https://lore.kernel.org/r/20260920071501.77785-6-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
There is a race condition involving the bus_reset_closure member of the
fw_cdev_event_bus_reset structure, which was addressed by commit
93b37905f7 ("firewire: cdev: prevent race between first get_info ioctl
and bus reset event queuing").
Add code comments and annotations to document it.
Link: https://lore.kernel.org/r/20260920071501.77785-5-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
A non-zero return value from copy_to_user() is handled by returning
-EFAULT. Using scoped_guard() simplifies this handling, since it
eliminates the need for a local variable to store the return value and
check it after the locking primitive is released.
Refactor the code to remove the local variable.
Link: https://lore.kernel.org/r/20260920071501.77785-4-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Takashi Sakamoto