| Age | Commit message (Collapse) | Author |
|---|
|
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>
|
|
instance
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>
|
|
The dummy driver used during card removal contains mutable variables.
Use a designated initializer instead of a static constant assignment.
Link: https://lore.kernel.org/r/20261018132955.391090-2-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
error callbacks
The precondition for schedule_error_callback() involves accessing the
pending transaction list. This access should be protected by the
transaction lock.
Use compiler-based context analysis to verify that the lock is held when
accessing the list.
Link: https://lore.kernel.org/r/20261018063056.329226-4-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 transaction layer has some code that can be replaced with the
existing helper function for scheduling error callbacks.
Link: https://lore.kernel.org/r/20261017153339.267337-3-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>
|
|
card removal
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 core provides helper functions and structures for handling
transaction requests to local address regions.
Add kerneldoc comments for these symbols.
Link: https://lore.kernel.org/r/20261013111509.612518-3-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
The core provides helper functions and structures for initiating
asynchronous transactions.
Add kerneldoc comments for these symbols.
Link: https://lore.kernel.org/r/20261013111509.612518-2-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>
|
|
topology map lock
The spinlock protects the topology map from concurrent access.
Add context analysis annotations and lockdep checks for this lock.
Link: https://lore.kernel.org/r/20261011063146.497455-6-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
split timeout lock
The spinlock protects parameters related to split transaction timeouts.
Add context analysis annotations and lockdep checks for this lock.
Link: https://lore.kernel.org/r/20261009121048.282982-5-o-takashi@sakamocchi.jp
Link: https://lore.kernel.org/r/20261011063146.497455-5-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
transactions lock
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>
|
|
Some functions are now guaranteed to run in process context.
Add might_sleep() calls to these functions to document and check this
requirement.
Link: https://lore.kernel.org/r/20261011063146.497455-2-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
fw_cancel_transaction() has no kerneldoc even though it is exposed.
Add kerneldoc for it.
Link: https://lore.kernel.org/r/20261007093235.175742-6-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 uses a different path for local packets than
for remote packets, and local packets cannot be cancelled once they have
been queued.
Allow local packets to be cancelled.
Link: https://lore.kernel.org/r/20261007093235.175742-4-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>
|
|
Some event structures have a trailing flexible array member of bytes to
store event-specific data. Use kzalloc_flex() to allocate the structure
together with storage for the flexible array member.
Link: https://lore.kernel.org/r/20260928125718.50974-4-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
Some event structures have a trailing flexible array member of quadlets
to store event-specific data. Use kzalloc_flex() to allocate the
structure together with storage for the flexible array member.
Link: https://lore.kernel.org/r/20260928125718.50974-3-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>
|
|
All execution paths in the cdev layer now run in process context.
Use a mutex instead of a spinlock for client-level locking.
Link: https://lore.kernel.org/r/20260927230724.119305-3-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>
|
|
transaction request packets
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>
|
|
Arrange the local variables to avoid a goto statement when copying events
to the userspace buffer.
Link: https://lore.kernel.org/r/20260927034343.28076-5-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
xchg() is useful for exchanging a pointer and returning the old value,
even when atomicity is not required.
Link: https://lore.kernel.org/r/20260927034343.28076-4-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
available
The in_shutdown member of client structure is set only when releasing
file descriptor, thus no need to wake up the client itself.
Link: https://lore.kernel.org/r/20260927034343.28076-3-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
Some accesses to fw_device members unnecessarily use the client-level
lock. Remove the locking from these accesses.
Link: https://lore.kernel.org/r/20260927034343.28076-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 client reference leaks when the pending work is cancelled because an
additional reference was taken when scheduling the work.
The reference held by the fw_iso_resource_auto object already ensures
that the client structure remains valid for the lifetime of the object.
Therefore, the additional reference taken when scheduling the work is
unnecessary.
Remove the additional reference.
Reported-by: Dingisoul <dingiso.kernel@gmail.com>
Link: https://sourceforge.net/p/linux1394/mailman/message/59317811/
Link: https://lore.kernel.org/r/20260923172328.277232-3-o-takashi@sakamocchi.jp
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 invoke the transaction callback in process context using
a workqueue. All transaction callbacks are now invoked in process context.
Update kerneldoc to reflect this behavior.
Link: https://lore.kernel.org/r/20260921031931.287429-4-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>
|
|
Two types of transaction callbacks are defined. There are many places
where the transaction callback is invoked with conditional branches.
Add a helper function to simplify invoking the callback.
Link: https://lore.kernel.org/r/20260921031931.287429-2-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
The implementation of the client_resource structure now requires a release
callback.
Add an argument to add_client_resource() for the release callback.
Link: https://lore.kernel.org/r/20260920083659.191855-6-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
The add_client_resource() function has an argument for GFP flags. The
argument was originally used to distinguish atomic calls. However, all
callers now use GFP_KERNEL.
Drop the unused argument and always allocate with GFP_KERNEL.
Link: https://lore.kernel.org/r/20260920083659.191855-5-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
|
|
The callback for the address handler is now guaranteed to be invoked in
process context. In the cdev layer, GFP_ATOMIC is still used for memory
allocation in the address handler.
Use GFP_KERNEL instead.
Link: https://lore.kernel.org/r/20260920083659.191855-4-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>
|
|
The callback for the address handler is now invoked in process context.
Update the kerneldoc for the address handler accordingly.
Link: https://lore.kernel.org/r/20260920083659.191855-2-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>
|
|
bus_reset_closure
There is a race condition involving the bus_reset_closure member of the
fw_cdev_event_bus_reset structure, which was addressed by commit
93b37905f70 ("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>
|
