From 82b523f369c9f279c98d04475283283e01030405 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Thu, 21 Aug 2025 09:30:14 +0900 Subject: [PATCH 01/43] firewire: ohci: remove obsolete debug logging for IRQ events A commit 0d8914165dd1 ("firewire: ohci: add tracepoints event for hardIRQ event") added "firewire_ohci:irqs" event in v6.11, which can provide equivalent information to the existing debug logging. This commit removes the logging. Link: https://lore.kernel.org/r/20250821003017.186752-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 34 +--------------------------------- 1 file changed, 1 insertion(+), 33 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 5d8301b0f3aa..d3ed43f4d0c3 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -395,50 +395,18 @@ MODULE_PARM_DESC(quirks, "Chip quirks (default = 0" #define OHCI_PARAM_DEBUG_AT_AR 1 #define OHCI_PARAM_DEBUG_SELFIDS 2 -#define OHCI_PARAM_DEBUG_IRQS 4 static int param_debug; module_param_named(debug, param_debug, int, 0644); MODULE_PARM_DESC(debug, "Verbose logging, deprecated in v6.11 kernel or later. (default = 0" ", AT/AR events = " __stringify(OHCI_PARAM_DEBUG_AT_AR) ", self-IDs = " __stringify(OHCI_PARAM_DEBUG_SELFIDS) - ", IRQs = " __stringify(OHCI_PARAM_DEBUG_IRQS) ", or a combination, or all = -1)"); static bool param_remote_dma; module_param_named(remote_dma, param_remote_dma, bool, 0444); MODULE_PARM_DESC(remote_dma, "Enable unfiltered remote DMA (default = N)"); -static void log_irqs(struct fw_ohci *ohci, u32 evt) -{ - if (likely(!(param_debug & OHCI_PARAM_DEBUG_IRQS))) - return; - - ohci_notice(ohci, "IRQ %08x%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n", evt, - evt & OHCI1394_selfIDComplete ? " selfID" : "", - evt & OHCI1394_RQPkt ? " AR_req" : "", - evt & OHCI1394_RSPkt ? " AR_resp" : "", - evt & OHCI1394_reqTxComplete ? " AT_req" : "", - evt & OHCI1394_respTxComplete ? " AT_resp" : "", - evt & OHCI1394_isochRx ? " IR" : "", - evt & OHCI1394_isochTx ? " IT" : "", - evt & OHCI1394_postedWriteErr ? " postedWriteErr" : "", - evt & OHCI1394_cycleTooLong ? " cycleTooLong" : "", - evt & OHCI1394_cycle64Seconds ? " cycle64Seconds" : "", - evt & OHCI1394_cycleInconsistent ? " cycleInconsistent" : "", - evt & OHCI1394_regAccessFail ? " regAccessFail" : "", - evt & OHCI1394_unrecoverableError ? " unrecoverableError" : "", - evt & OHCI1394_busReset ? " busReset" : "", - evt & ~(OHCI1394_selfIDComplete | OHCI1394_RQPkt | - OHCI1394_RSPkt | OHCI1394_reqTxComplete | - OHCI1394_respTxComplete | OHCI1394_isochRx | - OHCI1394_isochTx | OHCI1394_postedWriteErr | - OHCI1394_cycleTooLong | OHCI1394_cycle64Seconds | - OHCI1394_cycleInconsistent | - OHCI1394_regAccessFail | OHCI1394_busReset) - ? " ?" : ""); -} - static void log_selfids(struct fw_ohci *ohci, int generation, int self_id_count) { static const char *const speed[] = { @@ -2226,7 +2194,7 @@ static irqreturn_t irq_handler(int irq, void *data) reg_write(ohci, OHCI1394_IntEventClear, event & ~(OHCI1394_busReset | OHCI1394_postedWriteErr)); trace_irqs(ohci->card.index, event); - log_irqs(ohci, event); + // The flag is masked again at bus_reset_work() scheduled by selfID event. if (event & OHCI1394_busReset) reg_write(ohci, OHCI1394_IntMaskClear, OHCI1394_busReset); From c579f1fe08cc34c7f0af8df44d2badfee53eb7e7 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Thu, 21 Aug 2025 09:30:15 +0900 Subject: [PATCH 02/43] firewire: ohci: remove obsolete debug logging for selfID sequence A commit 677ceae19073 ("firewire: core: add tracepoints event for self_id_sequence") added the "firewire:self_id_sequence" event in v6.11. A commit 526e21a2aa6f ("firewire: ohci: add tracepoints event for data of Self-ID DMA") added the "firewire_ohci:self_id_complete" event in v6.12. These tracepoints replace the equivalent debug logging. This commit removes the logging. Link: https://lore.kernel.org/r/20250821003017.186752-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 72 ----------------------------------------- 1 file changed, 72 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index d3ed43f4d0c3..8cecdf4c6572 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -394,87 +394,17 @@ MODULE_PARM_DESC(quirks, "Chip quirks (default = 0" ")"); #define OHCI_PARAM_DEBUG_AT_AR 1 -#define OHCI_PARAM_DEBUG_SELFIDS 2 static int param_debug; module_param_named(debug, param_debug, int, 0644); MODULE_PARM_DESC(debug, "Verbose logging, deprecated in v6.11 kernel or later. (default = 0" ", AT/AR events = " __stringify(OHCI_PARAM_DEBUG_AT_AR) - ", self-IDs = " __stringify(OHCI_PARAM_DEBUG_SELFIDS) ", or a combination, or all = -1)"); static bool param_remote_dma; module_param_named(remote_dma, param_remote_dma, bool, 0444); MODULE_PARM_DESC(remote_dma, "Enable unfiltered remote DMA (default = N)"); -static void log_selfids(struct fw_ohci *ohci, int generation, int self_id_count) -{ - static const char *const speed[] = { - [0] = "S100", [1] = "S200", [2] = "S400", [3] = "beta", - }; - static const char *const power[] = { - [0] = "+0W", [1] = "+15W", [2] = "+30W", [3] = "+45W", - [4] = "-3W", [5] = " ?W", [6] = "-3..-6W", [7] = "-3..-10W", - }; - static const char port[] = { - [PHY_PACKET_SELF_ID_PORT_STATUS_NONE] = '.', - [PHY_PACKET_SELF_ID_PORT_STATUS_NCONN] = '-', - [PHY_PACKET_SELF_ID_PORT_STATUS_PARENT] = 'p', - [PHY_PACKET_SELF_ID_PORT_STATUS_CHILD] = 'c', - }; - struct self_id_sequence_enumerator enumerator = { - .cursor = ohci->self_id_buffer, - .quadlet_count = self_id_count, - }; - - if (likely(!(param_debug & OHCI_PARAM_DEBUG_SELFIDS))) - return; - - ohci_notice(ohci, "%d selfIDs, generation %d, local node ID %04x\n", - self_id_count, generation, ohci->node_id); - - while (enumerator.quadlet_count > 0) { - unsigned int quadlet_count; - unsigned int port_index; - const u32 *s; - int i; - - s = self_id_sequence_enumerator_next(&enumerator, &quadlet_count); - if (IS_ERR(s)) - break; - - ohci_notice(ohci, - "selfID 0: %08x, phy %d [%c%c%c] %s gc=%d %s %s%s%s\n", - *s, - phy_packet_self_id_get_phy_id(*s), - port[self_id_sequence_get_port_status(s, quadlet_count, 0)], - port[self_id_sequence_get_port_status(s, quadlet_count, 1)], - port[self_id_sequence_get_port_status(s, quadlet_count, 2)], - speed[*s >> 14 & 3], *s >> 16 & 63, - power[*s >> 8 & 7], *s >> 22 & 1 ? "L" : "", - *s >> 11 & 1 ? "c" : "", *s & 2 ? "i" : ""); - - port_index = 3; - for (i = 1; i < quadlet_count; ++i) { - ohci_notice(ohci, - "selfID n: %08x, phy %d [%c%c%c%c%c%c%c%c]\n", - s[i], - phy_packet_self_id_get_phy_id(s[i]), - port[self_id_sequence_get_port_status(s, quadlet_count, port_index)], - port[self_id_sequence_get_port_status(s, quadlet_count, port_index + 1)], - port[self_id_sequence_get_port_status(s, quadlet_count, port_index + 2)], - port[self_id_sequence_get_port_status(s, quadlet_count, port_index + 3)], - port[self_id_sequence_get_port_status(s, quadlet_count, port_index + 4)], - port[self_id_sequence_get_port_status(s, quadlet_count, port_index + 5)], - port[self_id_sequence_get_port_status(s, quadlet_count, port_index + 6)], - port[self_id_sequence_get_port_status(s, quadlet_count, port_index + 7)] - ); - - port_index += 8; - } - } -} - static const char *evts[] = { [0x00] = "evt_no_status", [0x01] = "-reserved-", [0x02] = "evt_long_packet", [0x03] = "evt_missing_ack", @@ -2163,8 +2093,6 @@ static void bus_reset_work(struct work_struct *work) if (free_rom) dmam_free_coherent(ohci->card.device, CONFIG_ROM_SIZE, free_rom, free_rom_bus); - log_selfids(ohci, generation, self_id_count); - fw_core_handle_bus_reset(&ohci->card, ohci->node_id, generation, self_id_count, ohci->self_id_buffer, ohci->csr_state_setclear_abdicate); From 6354cc95193696f1698e01ef6884f0fd9d613a6a Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Thu, 21 Aug 2025 09:30:16 +0900 Subject: [PATCH 03/43] firewire: ohci: remove obsolete debug logging for AT/AR results Between v6.11 and v6.12, a set of tracepoints was added to record asynchronous communication events: - firewire:async_phy_inbound - firewire:async_phy_outbound_initiate - firewire:async_phy_outbound_complete - firewire:async_response_inbound - firewire:async_response_outbound_initiate - firewire:async_response_outbound_complete - firewire:async_request_inbound - firewire:async_request_outbound_initiate - firewire:async_request_outbound_complete These tracepoints cover the functionality of the existing debug logging. This commit removes the logging. Link: https://lore.kernel.org/r/20250821003017.186752-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 132 ++++++---------------------------------- 1 file changed, 19 insertions(+), 113 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 8cecdf4c6572..ae7f75fade8d 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -393,123 +393,15 @@ MODULE_PARM_DESC(quirks, "Chip quirks (default = 0" ", IR wake unreliable = " __stringify(QUIRK_IR_WAKE) ")"); -#define OHCI_PARAM_DEBUG_AT_AR 1 - static int param_debug; module_param_named(debug, param_debug, int, 0644); MODULE_PARM_DESC(debug, "Verbose logging, deprecated in v6.11 kernel or later. (default = 0" - ", AT/AR events = " __stringify(OHCI_PARAM_DEBUG_AT_AR) ", or a combination, or all = -1)"); static bool param_remote_dma; module_param_named(remote_dma, param_remote_dma, bool, 0444); MODULE_PARM_DESC(remote_dma, "Enable unfiltered remote DMA (default = N)"); -static const char *evts[] = { - [0x00] = "evt_no_status", [0x01] = "-reserved-", - [0x02] = "evt_long_packet", [0x03] = "evt_missing_ack", - [0x04] = "evt_underrun", [0x05] = "evt_overrun", - [0x06] = "evt_descriptor_read", [0x07] = "evt_data_read", - [0x08] = "evt_data_write", [0x09] = "evt_bus_reset", - [0x0a] = "evt_timeout", [0x0b] = "evt_tcode_err", - [0x0c] = "-reserved-", [0x0d] = "-reserved-", - [0x0e] = "evt_unknown", [0x0f] = "evt_flushed", - [0x10] = "-reserved-", [0x11] = "ack_complete", - [0x12] = "ack_pending ", [0x13] = "-reserved-", - [0x14] = "ack_busy_X", [0x15] = "ack_busy_A", - [0x16] = "ack_busy_B", [0x17] = "-reserved-", - [0x18] = "-reserved-", [0x19] = "-reserved-", - [0x1a] = "-reserved-", [0x1b] = "ack_tardy", - [0x1c] = "-reserved-", [0x1d] = "ack_data_error", - [0x1e] = "ack_type_error", [0x1f] = "-reserved-", - [0x20] = "pending/cancelled", -}; - -static void log_ar_at_event(struct fw_ohci *ohci, - char dir, int speed, u32 *header, int evt) -{ - static const char *const tcodes[] = { - [TCODE_WRITE_QUADLET_REQUEST] = "QW req", - [TCODE_WRITE_BLOCK_REQUEST] = "BW req", - [TCODE_WRITE_RESPONSE] = "W resp", - [0x3] = "-reserved-", - [TCODE_READ_QUADLET_REQUEST] = "QR req", - [TCODE_READ_BLOCK_REQUEST] = "BR req", - [TCODE_READ_QUADLET_RESPONSE] = "QR resp", - [TCODE_READ_BLOCK_RESPONSE] = "BR resp", - [TCODE_CYCLE_START] = "cycle start", - [TCODE_LOCK_REQUEST] = "Lk req", - [TCODE_STREAM_DATA] = "async stream packet", - [TCODE_LOCK_RESPONSE] = "Lk resp", - [0xc] = "-reserved-", - [0xd] = "-reserved-", - [TCODE_LINK_INTERNAL] = "link internal", - [0xf] = "-reserved-", - }; - int tcode = async_header_get_tcode(header); - char specific[12]; - - if (likely(!(param_debug & OHCI_PARAM_DEBUG_AT_AR))) - return; - - if (unlikely(evt >= ARRAY_SIZE(evts))) - evt = 0x1f; - - if (evt == OHCI1394_evt_bus_reset) { - ohci_notice(ohci, "A%c evt_bus_reset, generation %d\n", - dir, (header[2] >> 16) & 0xff); - return; - } - - switch (tcode) { - case TCODE_WRITE_QUADLET_REQUEST: - case TCODE_READ_QUADLET_RESPONSE: - case TCODE_CYCLE_START: - snprintf(specific, sizeof(specific), " = %08x", - be32_to_cpu((__force __be32)header[3])); - break; - case TCODE_WRITE_BLOCK_REQUEST: - case TCODE_READ_BLOCK_REQUEST: - case TCODE_READ_BLOCK_RESPONSE: - case TCODE_LOCK_REQUEST: - case TCODE_LOCK_RESPONSE: - snprintf(specific, sizeof(specific), " %x,%x", - async_header_get_data_length(header), - async_header_get_extended_tcode(header)); - break; - default: - specific[0] = '\0'; - } - - switch (tcode) { - case TCODE_STREAM_DATA: - ohci_notice(ohci, "A%c %s, %s\n", - dir, evts[evt], tcodes[tcode]); - break; - case TCODE_LINK_INTERNAL: - ohci_notice(ohci, "A%c %s, PHY %08x %08x\n", - dir, evts[evt], header[1], header[2]); - break; - case TCODE_WRITE_QUADLET_REQUEST: - case TCODE_WRITE_BLOCK_REQUEST: - case TCODE_READ_QUADLET_REQUEST: - case TCODE_READ_BLOCK_REQUEST: - case TCODE_LOCK_REQUEST: - ohci_notice(ohci, - "A%c spd %x tl %02x, %04x -> %04x, %s, %s, %012llx%s\n", - dir, speed, async_header_get_tlabel(header), - async_header_get_source(header), async_header_get_destination(header), - evts[evt], tcodes[tcode], async_header_get_offset(header), specific); - break; - default: - ohci_notice(ohci, - "A%c spd %x tl %02x, %04x -> %04x, %s, %s%s\n", - dir, speed, async_header_get_tlabel(header), - async_header_get_source(header), async_header_get_destination(header), - evts[evt], tcodes[tcode], specific); - } -} - static inline void reg_write(const struct fw_ohci *ohci, int offset, u32 data) { writel(data, ohci->registers + offset); @@ -855,8 +747,6 @@ static __le32 *handle_ar_packet(struct ar_context *ctx, __le32 *buffer) p.timestamp = status & 0xffff; p.generation = ohci->request_generation; - log_ar_at_event(ohci, 'R', p.speed, p.header, evt); - /* * Several controllers, notably from NEC and VIA, forget to * write ack_complete status at PHY packet reception. @@ -1464,8 +1354,6 @@ static int handle_at_packet(struct context *context, evt = le16_to_cpu(last->transfer_status) & 0x1f; packet->timestamp = le16_to_cpu(last->res_count); - log_ar_at_event(ohci, 'T', packet->speed, packet->header, evt); - switch (evt) { case OHCI1394_evt_timeout: /* Async response transmit timed out. */ @@ -1670,6 +1558,25 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet static void detect_dead_context(struct fw_ohci *ohci, const char *name, unsigned int regs) { + static const char *const evts[] = { + [0x00] = "evt_no_status", [0x01] = "-reserved-", + [0x02] = "evt_long_packet", [0x03] = "evt_missing_ack", + [0x04] = "evt_underrun", [0x05] = "evt_overrun", + [0x06] = "evt_descriptor_read", [0x07] = "evt_data_read", + [0x08] = "evt_data_write", [0x09] = "evt_bus_reset", + [0x0a] = "evt_timeout", [0x0b] = "evt_tcode_err", + [0x0c] = "-reserved-", [0x0d] = "-reserved-", + [0x0e] = "evt_unknown", [0x0f] = "evt_flushed", + [0x10] = "-reserved-", [0x11] = "ack_complete", + [0x12] = "ack_pending ", [0x13] = "-reserved-", + [0x14] = "ack_busy_X", [0x15] = "ack_busy_A", + [0x16] = "ack_busy_B", [0x17] = "-reserved-", + [0x18] = "-reserved-", [0x19] = "-reserved-", + [0x1a] = "-reserved-", [0x1b] = "ack_tardy", + [0x1c] = "-reserved-", [0x1d] = "ack_data_error", + [0x1e] = "ack_type_error", [0x1f] = "-reserved-", + [0x20] = "pending/cancelled", + }; u32 ctl; ctl = reg_read(ohci, CONTROL_SET(regs)); @@ -2601,7 +2508,6 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet) dma_unmap_single(ohci->card.device, packet->payload_bus, packet->payload_length, DMA_TO_DEVICE); - log_ar_at_event(ohci, 'T', packet->speed, packet->header, 0x20); driver_data->packet = NULL; packet->ack = RCODE_CANCELLED; From 8748368c3d92f7bdef67c90d3f62ab92083b3677 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Thu, 21 Aug 2025 09:30:17 +0900 Subject: [PATCH 04/43] firewire: ohci: remove obsolete module-level debug parameter The module-level debug parameter was added in v2.6.26 by a commit ad3c0fe8b8d16 ("firewire: debug interrupt events"). Its functionality has long been superseded by tracepoints. This commit removes the module parameter, bye. Link: https://lore.kernel.org/r/20250821003017.186752-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index ae7f75fade8d..c8c5d598e3c8 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -393,11 +393,6 @@ MODULE_PARM_DESC(quirks, "Chip quirks (default = 0" ", IR wake unreliable = " __stringify(QUIRK_IR_WAKE) ")"); -static int param_debug; -module_param_named(debug, param_debug, int, 0644); -MODULE_PARM_DESC(debug, "Verbose logging, deprecated in v6.11 kernel or later. (default = 0" - ", or a combination, or all = -1)"); - static bool param_remote_dma; module_param_named(remote_dma, param_remote_dma, bool, 0444); MODULE_PARM_DESC(remote_dma, "Enable unfiltered remote DMA (default = N)"); @@ -2017,11 +2012,6 @@ static irqreturn_t irq_handler(int irq, void *data) if (!event || !~event) return IRQ_NONE; - if (unlikely(param_debug > 0)) { - dev_notice_ratelimited(ohci->card.device, - "The debug parameter is superseded by tracepoints events, and deprecated."); - } - /* * busReset and postedWriteErr events must not be cleared yet * (OHCI 1.1 clauses 7.2.3.2 and 13.2.8.1) From 696968262aeee51e1c0529c3c060ddd180702e02 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 23 Aug 2025 12:09:52 +0900 Subject: [PATCH 05/43] firewire: ohci: move self_id_complete tracepoint after validating register The value of OHCI1394_SelfIDCount register includes an error-indicating bit. It is safer to place the tracepoint probe after validating the register value. Link: https://lore.kernel.org/r/20250823030954.268412-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index c8c5d598e3c8..b3a187e4cba7 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1863,6 +1863,9 @@ static void bus_reset_work(struct work_struct *work) ohci_notice(ohci, "self ID receive error\n"); return; } + + trace_self_id_complete(ohci->card.index, reg, ohci->self_id, has_be_header_quirk(ohci)); + /* * The count in the SelfIDCount register is the number of * bytes in the self ID receive buffer. Since we also receive @@ -2024,15 +2027,8 @@ static irqreturn_t irq_handler(int irq, void *data) if (event & OHCI1394_busReset) reg_write(ohci, OHCI1394_IntMaskClear, OHCI1394_busReset); - if (event & OHCI1394_selfIDComplete) { - if (trace_self_id_complete_enabled()) { - u32 reg = reg_read(ohci, OHCI1394_SelfIDCount); - - trace_self_id_complete(ohci->card.index, reg, ohci->self_id, - has_be_header_quirk(ohci)); - } + if (event & OHCI1394_selfIDComplete) queue_work(selfid_workqueue, &ohci->bus_reset_work); - } if (event & OHCI1394_RQPkt) queue_work(ohci->card.async_wq, &ohci->ar_request_ctx.work); From 61efd0e1afb24f8a71cff80f7600ff7556378c53 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 23 Aug 2025 12:09:53 +0900 Subject: [PATCH 06/43] firewire: ohci: use threaded IRQ handler to handle SelfIDComplete event The first step maintaining the bus topology is to handle SelfIDComplete event. This event occurs after initiating bus reset when 1394 OHCI link layer is enabled, or when the bus topology changes (e.g. when a device is added). Because enumeration of the selfID sequence can take some time, it should be processed in a bottom half. Currently, this is done in a module-local workqueue with the WQ_MEM_RECLAIM flag, to allow invocation during memory reclaim paths. A threaded IRQ handler is a preferable alternative, as it eliminates the need to manage workqueue attributes manually. Although SelfIDComplete events are not so frequent in normal usage, handling them correctly is critical for proper bus topology management. This commit switches SelfIDComplete handling to a threaded IRQ handler. Link: https://lore.kernel.org/r/20250823030954.268412-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index b3a187e4cba7..5b16286280e0 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -760,7 +760,7 @@ static __le32 *handle_ar_packet(struct ar_context *ctx, __le32 *buffer) * * Alas some chips sometimes emit bus reset packets with a * wrong generation. We set the correct generation for these - * at a slightly incorrect time (in bus_reset_work). + * at a slightly incorrect time (in handle_selfid_complete_event). */ if (evt == OHCI1394_evt_bus_reset) { if (!(ohci->quirks & QUIRK_RESET_PACKET)) @@ -1830,9 +1830,9 @@ static int find_and_insert_self_id(struct fw_ohci *ohci, int self_id_count) return self_id_count; } -static void bus_reset_work(struct work_struct *work) +static irqreturn_t handle_selfid_complete_event(int irq, void *data) { - struct fw_ohci *ohci = from_work(ohci, work, bus_reset_work); + struct fw_ohci *ohci = data; int self_id_count, generation, new_generation, i, j; u32 reg, quadlet; void *free_rom = NULL; @@ -1843,11 +1843,11 @@ static void bus_reset_work(struct work_struct *work) if (!(reg & OHCI1394_NodeID_idValid)) { ohci_notice(ohci, "node ID not valid, new bus reset in progress\n"); - return; + goto end; } if ((reg & OHCI1394_NodeID_nodeNumber) == 63) { ohci_notice(ohci, "malconfigured bus\n"); - return; + goto end; } ohci->node_id = reg & (OHCI1394_NodeID_busNumber | OHCI1394_NodeID_nodeNumber); @@ -1861,7 +1861,7 @@ static void bus_reset_work(struct work_struct *work) reg = reg_read(ohci, OHCI1394_SelfIDCount); if (ohci1394_self_id_count_is_error(reg)) { ohci_notice(ohci, "self ID receive error\n"); - return; + goto end; } trace_self_id_complete(ohci->card.index, reg, ohci->self_id, has_be_header_quirk(ohci)); @@ -1876,7 +1876,7 @@ static void bus_reset_work(struct work_struct *work) if (self_id_count > 252) { ohci_notice(ohci, "bad selfIDSize (%08x)\n", reg); - return; + goto end; } quadlet = cond_le32_to_cpu(ohci->self_id[0], has_be_header_quirk(ohci)); @@ -1903,7 +1903,7 @@ static void bus_reset_work(struct work_struct *work) ohci_notice(ohci, "bad self ID %d/%d (%08x != ~%08x)\n", j, self_id_count, id, id2); - return; + goto end; } ohci->self_id_buffer[j] = id; } @@ -1913,13 +1913,13 @@ static void bus_reset_work(struct work_struct *work) if (self_id_count < 0) { ohci_notice(ohci, "could not construct local self ID\n"); - return; + goto end; } } if (self_id_count == 0) { ohci_notice(ohci, "no self IDs\n"); - return; + goto end; } rmb(); @@ -1941,7 +1941,7 @@ static void bus_reset_work(struct work_struct *work) new_generation = ohci1394_self_id_count_get_generation(reg); if (new_generation != generation) { ohci_notice(ohci, "new bus reset, discarding self ids\n"); - return; + goto end; } // FIXME: Document how the locking works. @@ -2002,6 +2002,8 @@ static void bus_reset_work(struct work_struct *work) self_id_count, ohci->self_id_buffer, ohci->csr_state_setclear_abdicate); ohci->csr_state_setclear_abdicate = false; +end: + return IRQ_HANDLED; } static irqreturn_t irq_handler(int irq, void *data) @@ -2023,13 +2025,10 @@ static irqreturn_t irq_handler(int irq, void *data) event & ~(OHCI1394_busReset | OHCI1394_postedWriteErr)); trace_irqs(ohci->card.index, event); - // The flag is masked again at bus_reset_work() scheduled by selfID event. + // The flag is masked again at handle_selfid_complete_event() scheduled by selfID event. if (event & OHCI1394_busReset) reg_write(ohci, OHCI1394_IntMaskClear, OHCI1394_busReset); - if (event & OHCI1394_selfIDComplete) - queue_work(selfid_workqueue, &ohci->bus_reset_work); - if (event & OHCI1394_RQPkt) queue_work(ohci->card.async_wq, &ohci->ar_request_ctx.work); @@ -2100,7 +2099,10 @@ static irqreturn_t irq_handler(int irq, void *data) } else flush_writes(ohci); - return IRQ_HANDLED; + if (event & OHCI1394_selfIDComplete) + return IRQ_WAKE_THREAD; + else + return IRQ_HANDLED; } static int software_reset(struct fw_ohci *ohci) @@ -2413,7 +2415,7 @@ static int ohci_set_config_rom(struct fw_card *card, * then set up the real values for the two registers. * * We use ohci->lock to avoid racing with the code that sets - * ohci->next_config_rom to NULL (see bus_reset_work). + * ohci->next_config_rom to NULL (see handle_selfid_complete_event). */ next_config_rom = dmam_alloc_coherent(ohci->card.device, CONFIG_ROM_SIZE, @@ -3620,7 +3622,9 @@ static int pci_probe(struct pci_dev *dev, goto fail_msi; } - err = request_threaded_irq(irq, irq_handler, NULL, + // IRQF_ONESHOT is not applied so that any events are handled in the hardIRQ handler during + // invoking the threaded IRQ handler for SelfIDComplete event. + err = request_threaded_irq(irq, irq_handler, handle_selfid_complete_event, pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED, ohci_driver_name, ohci); if (err < 0) { From a901f493d06631091bf1f644fdbb5cb4f566645d Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 23 Aug 2025 12:09:54 +0900 Subject: [PATCH 07/43] firewire: ohci: remove module-local workqueue Now module-local workqueue has been replaced by a threaded IRQ handler. This commit removes the workqueue and the associated work item accordingly. Link: https://lore.kernel.org/r/20250823030954.268412-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 5b16286280e0..40851b120615 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -228,13 +228,10 @@ struct fw_ohci { __le32 *self_id; dma_addr_t self_id_bus; - struct work_struct bus_reset_work; u32 self_id_buffer[512]; }; -static struct workqueue_struct *selfid_workqueue; - static inline struct fw_ohci *fw_ohci(struct fw_card *card) { return container_of(card, struct fw_ohci, card); @@ -3512,8 +3509,6 @@ static int pci_probe(struct pci_dev *dev, spin_lock_init(&ohci->lock); mutex_init(&ohci->phy_reg_mutex); - INIT_WORK(&ohci->bus_reset_work, bus_reset_work); - if (!(pci_resource_flags(dev, 0) & IORESOURCE_MEM) || pci_resource_len(dev, 0) < OHCI1394_REGISTER_SIZE) { ohci_err(ohci, "invalid MMIO resource\n"); @@ -3668,7 +3663,6 @@ static void pci_remove(struct pci_dev *dev) reg_write(ohci, OHCI1394_IntMaskClear, ~0); flush_writes(ohci); } - cancel_work_sync(&ohci->bus_reset_work); fw_core_remove_card(&ohci->card); /* @@ -3741,17 +3735,12 @@ static struct pci_driver fw_ohci_pci_driver = { static int __init fw_ohci_init(void) { - selfid_workqueue = alloc_workqueue(KBUILD_MODNAME, WQ_MEM_RECLAIM, 0); - if (!selfid_workqueue) - return -ENOMEM; - return pci_register_driver(&fw_ohci_pci_driver); } static void __exit fw_ohci_cleanup(void) { pci_unregister_driver(&fw_ohci_pci_driver); - destroy_workqueue(selfid_workqueue); } module_init(fw_ohci_init); From ada2e4091d20735537fab67d970ffdbb8e9f7672 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 8 Sep 2025 10:20:58 +0900 Subject: [PATCH 08/43] firewire: ohci: use kcalloc() variant for array allocation When allocating the list of isochronous context structure, a kzalloc() variant of managed device API is used. In this case, a kcalloc() variant is available. This commit replaces these lines with devm_kcalloc(). Link: https://lore.kernel.org/r/20250908012108.514698-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 40851b120615..030aed5453a1 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3482,7 +3482,6 @@ static int pci_probe(struct pci_dev *dev, u32 bus_options, max_receive, link_speed, version; u64 guid; int i, flags, irq, err; - size_t size; if (dev->vendor == PCI_VENDOR_ID_PINNACLE_SYSTEMS) { dev_err(&dev->dev, "Pinnacle MovieBoard is not yet supported\n"); @@ -3576,8 +3575,7 @@ static int pci_probe(struct pci_dev *dev, reg_write(ohci, OHCI1394_IsoRecvIntMaskClear, ~0); ohci->ir_context_mask = ohci->ir_context_support; ohci->n_ir = hweight32(ohci->ir_context_mask); - size = sizeof(struct iso_context) * ohci->n_ir; - ohci->ir_context_list = devm_kzalloc(&dev->dev, size, GFP_KERNEL); + ohci->ir_context_list = devm_kcalloc(&dev->dev, ohci->n_ir, sizeof(struct iso_context), GFP_KERNEL); if (!ohci->ir_context_list) return -ENOMEM; @@ -3591,8 +3589,7 @@ static int pci_probe(struct pci_dev *dev, reg_write(ohci, OHCI1394_IsoXmitIntMaskClear, ~0); ohci->it_context_mask = ohci->it_context_support; ohci->n_it = hweight32(ohci->it_context_mask); - size = sizeof(struct iso_context) * ohci->n_it; - ohci->it_context_list = devm_kzalloc(&dev->dev, size, GFP_KERNEL); + ohci->it_context_list = devm_kcalloc(&dev->dev, ohci->n_it, sizeof(struct iso_context), GFP_KERNEL); if (!ohci->it_context_list) return -ENOMEM; From c908e072b6932e2d40645f6916c9b4bfe8f3f12f Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 8 Sep 2025 10:20:59 +0900 Subject: [PATCH 09/43] firewire: core: utilize cleanup function to release workqueue in error path The helper macro, retain_and_null_ptr(), introduced by a commit 092d00ead733 ("cleanup: Provide retain_and_null_ptr()") in v6.16 kernel, is useful in the error path to release the part of structure member. This commit uses the relatively new function. Link: https://lore.kernel.org/r/20250908012108.514698-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 40 ++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index aae774e7a5c3..d128c7a8bf5f 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -570,9 +570,13 @@ void fw_card_initialize(struct fw_card *card, } EXPORT_SYMBOL(fw_card_initialize); +DEFINE_FREE(workqueue_destroy, struct workqueue_struct *, if (_T) destroy_workqueue(_T)) + int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid, unsigned int supported_isoc_contexts) { + struct workqueue_struct *isoc_wq __free(workqueue_destroy) = NULL; + struct workqueue_struct *async_wq __free(workqueue_destroy) = NULL; int ret; // This workqueue should be: @@ -587,10 +591,10 @@ int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid, // * == WQ_SYSFS Parameters are available via sysfs. // * max_active == n_it + n_ir A hardIRQ could notify events for multiple isochronous // contexts if they are scheduled to the same cycle. - card->isoc_wq = alloc_workqueue("firewire-isoc-card%u", - WQ_UNBOUND | WQ_FREEZABLE | WQ_HIGHPRI | WQ_SYSFS, - supported_isoc_contexts, card->index); - if (!card->isoc_wq) + isoc_wq = alloc_workqueue("firewire-isoc-card%u", + WQ_UNBOUND | WQ_FREEZABLE | WQ_HIGHPRI | WQ_SYSFS, + supported_isoc_contexts, card->index); + if (!isoc_wq) return -ENOMEM; // This workqueue should be: @@ -602,14 +606,14 @@ int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid, // * == WQ_SYSFS Parameters are available via sysfs. // * max_active == 4 A hardIRQ could notify events for a pair of requests and // response AR/AT contexts. - card->async_wq = alloc_workqueue("firewire-async-card%u", - WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI | WQ_SYSFS, - 4, card->index); - if (!card->async_wq) { - ret = -ENOMEM; - goto err_isoc; - } + async_wq = alloc_workqueue("firewire-async-card%u", + WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI | WQ_SYSFS, + 4, card->index); + if (!async_wq) + return -ENOMEM; + card->isoc_wq = isoc_wq; + card->async_wq = async_wq; card->max_receive = max_receive; card->link_speed = link_speed; card->guid = guid; @@ -617,18 +621,18 @@ int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid, scoped_guard(mutex, &card_mutex) { generate_config_rom(card, tmp_config_rom); ret = card->driver->enable(card, tmp_config_rom, config_rom_length); - if (ret < 0) - goto err_async; + if (ret < 0) { + card->isoc_wq = NULL; + card->async_wq = NULL; + return ret; + } + retain_and_null_ptr(isoc_wq); + retain_and_null_ptr(async_wq); list_add_tail(&card->link, &card_list); } return 0; -err_async: - destroy_workqueue(card->async_wq); -err_isoc: - destroy_workqueue(card->isoc_wq); - return ret; } EXPORT_SYMBOL(fw_card_add); From cbb13dceec65b6021a4f403d4fa7b237569a1007 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 8 Sep 2025 10:21:00 +0900 Subject: [PATCH 10/43] firewire: ohci: use return value from fw_node_get() The programming pattern, referring after increasing reference count, is supported by fw_node_get(). This commit simplify the programming pattern. Link: https://lore.kernel.org/r/20250908012108.514698-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index d128c7a8bf5f..41902dcc10a0 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -306,8 +306,7 @@ static void bm_work(struct work_struct *work) generation = card->generation; - root_node = card->root_node; - fw_node_get(root_node); + root_node = fw_node_get(card->root_node); root_device = root_node->data; root_device_is_running = root_device && atomic_read(&root_device->state) == FW_DEVICE_RUNNING; From a2bbb8602dc29a8a3fe4f0377c7b820fba384697 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 8 Sep 2025 10:21:01 +0900 Subject: [PATCH 11/43] firewire: core: add helper functions to access to fw_device data in fw_node structure The data mbmer in fw_node structure is an opaque pointer, while nowadays it is just used to refer to fw_device associated with the fw_node. This commit redefines the opaque pointer to a pointer to fw_device structure, and adds some helper functions to set/get it. Link: https://lore.kernel.org/r/20250908012108.514698-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 4 ++-- drivers/firewire/core-device.c | 18 +++++++++--------- drivers/firewire/core.h | 14 ++++++++++++-- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 41902dcc10a0..4a4210cda571 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -307,12 +307,12 @@ static void bm_work(struct work_struct *work) generation = card->generation; root_node = fw_node_get(card->root_node); - root_device = root_node->data; + root_device = fw_node_get_device(root_node); root_device_is_running = root_device && atomic_read(&root_device->state) == FW_DEVICE_RUNNING; root_device_is_cmc = root_device && root_device->cmc; - irm_device = card->irm_node->data; + irm_device = fw_node_get_device(card->irm_node); irm_is_1394_1995_only = irm_device && irm_device->config_rom && (irm_device->config_rom[2] & 0x000000f0) == 0; diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index aeacd4cfd694..6a04a0014694 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -887,7 +887,7 @@ static void fw_device_release(struct device *dev) * bus manager work looks at this node. */ scoped_guard(spinlock_irqsave, &card->lock) - device->node->data = NULL; + fw_node_set_device(device->node, NULL); fw_node_put(device->node); kfree(device->config_rom); @@ -1007,7 +1007,7 @@ static void fw_device_init(struct work_struct *work) int ret; /* - * All failure paths here set node->data to NULL, so that we + * All failure paths here call fw_node_set_device(node, NULL), so that we * don't try to do device_for_each_child() on a kfree()'d * device. */ @@ -1051,9 +1051,9 @@ static void fw_device_init(struct work_struct *work) struct fw_node *obsolete_node = reused->node; device->node = obsolete_node; - device->node->data = device; + fw_node_set_device(device->node, device); reused->node = current_node; - reused->node->data = reused; + fw_node_set_device(reused->node, reused); reused->max_speed = device->max_speed; reused->node_id = current_node->node_id; @@ -1292,7 +1292,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event) * FW_NODE_UPDATED callbacks can update the node_id * and generation for the device. */ - node->data = device; + fw_node_set_device(node, device); /* * Many devices are slow to respond after bus resets, @@ -1307,7 +1307,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event) case FW_NODE_INITIATED_RESET: case FW_NODE_LINK_ON: - device = node->data; + device = fw_node_get_device(node); if (device == NULL) goto create; @@ -1324,7 +1324,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event) break; case FW_NODE_UPDATED: - device = node->data; + device = fw_node_get_device(node); if (device == NULL) break; @@ -1339,7 +1339,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event) case FW_NODE_DESTROYED: case FW_NODE_LINK_OFF: - if (!node->data) + if (!fw_node_get_device(node)) break; /* @@ -1354,7 +1354,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event) * the device in shutdown state to have that code fail * to create the device. */ - device = node->data; + device = fw_node_get_device(node); if (atomic_xchg(&device->state, FW_DEVICE_GONE) == FW_DEVICE_RUNNING) { device->workfn = fw_device_shutdown; diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index 9b298af1cac0..083e39034c37 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -194,8 +194,8 @@ struct fw_node { /* For serializing node topology into a list. */ struct list_head link; - /* Upper layer specific data. */ - void *data; + // The device when already associated, else NULL. + struct fw_device *device; struct fw_node *ports[] __counted_by(port_count); }; @@ -219,6 +219,16 @@ static inline void fw_node_put(struct fw_node *node) kref_put(&node->kref, release_node); } +static inline struct fw_device *fw_node_get_device(struct fw_node *node) +{ + return node->device; +} + +static inline void fw_node_set_device(struct fw_node *node, struct fw_device *device) +{ + node->device = device; +} + void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, int self_id_count, u32 *self_ids, bool bm_abdicate); void fw_destroy_nodes(struct fw_card *card); From 25feb1a96e21ae37790c109ea2c675061ba11794 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 8 Sep 2025 10:21:02 +0900 Subject: [PATCH 12/43] firewire: core: use cleanup function in bm_work In "bm_work" function, the references to fw_card and fw_node are released at last. This is achieved by using goto statements. For this case, the kernel cleanup framework is available. This commit uses the framework to remove these statements. Link: https://lore.kernel.org/r/20250908012108.514698-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 4a4210cda571..5bd89ddf5018 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -280,14 +280,17 @@ void fw_schedule_bm_work(struct fw_card *card, unsigned long delay) fw_card_put(card); } +DEFINE_FREE(node_unref, struct fw_node *, if (_T) fw_node_put(_T)) +DEFINE_FREE(card_unref, struct fw_card *, if (_T) fw_card_put(_T)) + static void bm_work(struct work_struct *work) { static const char gap_count_table[] = { 63, 5, 7, 8, 10, 13, 16, 18, 21, 24, 26, 29, 32, 35, 37, 40 }; - struct fw_card *card = from_work(card, work, bm_work.work); + struct fw_card *card __free(card_unref) = from_work(card, work, bm_work.work); struct fw_device *root_device, *irm_device; - struct fw_node *root_node; + struct fw_node *root_node __free(node_unref) = NULL; int root_id, new_root_id, irm_id, bm_id, local_id; int gap_count, generation, grace, rcode; bool do_reset = false; @@ -297,11 +300,13 @@ static void bm_work(struct work_struct *work) bool keep_this_irm; __be32 transaction_data[2]; + lockdep_assert_held(&card->lock); + spin_lock_irq(&card->lock); if (card->local_node == NULL) { spin_unlock_irq(&card->lock); - goto out_put_card; + return; } generation = card->generation; @@ -366,9 +371,9 @@ static void bm_work(struct work_struct *work) CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, transaction_data, 8); + // Another bus reset, BM work has been rescheduled. if (rcode == RCODE_GENERATION) - /* Another bus reset, BM work has been rescheduled. */ - goto out; + return; bm_id = be32_to_cpu(transaction_data[0]); @@ -382,8 +387,7 @@ static void bm_work(struct work_struct *work) /* Somebody else is BM. Only act as IRM. */ if (local_id == irm_id) allocate_broadcast_channel(card, generation); - - goto out; + return; } if (rcode == RCODE_SEND_ERROR) { @@ -393,7 +397,7 @@ static void bm_work(struct work_struct *work) * that the problem has gone away by then. */ fw_schedule_bm_work(card, DIV_ROUND_UP(HZ, 8)); - goto out; + return; } spin_lock_irq(&card->lock); @@ -417,7 +421,7 @@ static void bm_work(struct work_struct *work) */ spin_unlock_irq(&card->lock); fw_schedule_bm_work(card, DIV_ROUND_UP(HZ, 8)); - goto out; + return; } /* @@ -455,7 +459,7 @@ static void bm_work(struct work_struct *work) * and let's try again once that's done. */ spin_unlock_irq(&card->lock); - goto out; + return; } else if (root_device_is_cmc) { /* * We will send out a force root packet for this @@ -512,7 +516,7 @@ static void bm_work(struct work_struct *work) */ reset_bus(card, card->gap_count != 0); /* Will allocate broadcast channel after the reset. */ - goto out; + return; } if (root_device_is_cmc) { @@ -525,16 +529,11 @@ static void bm_work(struct work_struct *work) CSR_REGISTER_BASE + CSR_STATE_SET, transaction_data, 4); if (rcode == RCODE_GENERATION) - goto out; + return; } if (local_id == irm_id) allocate_broadcast_channel(card, generation); - - out: - fw_node_put(root_node); - out_put_card: - fw_card_put(card); } void fw_card_initialize(struct fw_card *card, From b70a5f33381f7815f4a579f7b9de33f276c9d8f9 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 8 Sep 2025 10:21:03 +0900 Subject: [PATCH 13/43] firewire: ohci: localize transaction data and rcode per condition branch The function local variable, transaction_data, in bm_work function is conditionally used. In the case, the branch-level variable is sometimes useful. This commit uses this idea. Link: https://lore.kernel.org/r/20250908012108.514698-7-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 5bd89ddf5018..b98797e4f1d4 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -292,13 +292,12 @@ static void bm_work(struct work_struct *work) struct fw_device *root_device, *irm_device; struct fw_node *root_node __free(node_unref) = NULL; int root_id, new_root_id, irm_id, bm_id, local_id; - int gap_count, generation, grace, rcode; + int gap_count, generation, grace; bool do_reset = false; bool root_device_is_running; bool root_device_is_cmc; bool irm_is_1394_1995_only; bool keep_this_irm; - __be32 transaction_data[2]; lockdep_assert_held(&card->lock); @@ -346,6 +345,11 @@ static void bm_work(struct work_struct *work) * gap count. That could well save a reset in the * next generation. */ + __be32 data[2] = { + cpu_to_be32(0x3f), + cpu_to_be32(local_id), + }; + int rcode; if (!card->irm_node->link_on) { new_root_id = local_id; @@ -361,21 +365,18 @@ static void bm_work(struct work_struct *work) goto pick_me; } - transaction_data[0] = cpu_to_be32(0x3f); - transaction_data[1] = cpu_to_be32(local_id); - spin_unlock_irq(&card->lock); rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, irm_id, generation, SCODE_100, CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, - transaction_data, 8); + data, sizeof(data)); // Another bus reset, BM work has been rescheduled. if (rcode == RCODE_GENERATION) return; - bm_id = be32_to_cpu(transaction_data[0]); + bm_id = be32_to_cpu(data[0]); scoped_guard(spinlock_irq, &card->lock) { if (rcode == RCODE_COMPLETE && generation == card->generation) @@ -523,11 +524,11 @@ static void bm_work(struct work_struct *work) /* * Make sure that the cycle master sends cycle start packets. */ - transaction_data[0] = cpu_to_be32(CSR_STATE_BIT_CMSTR); - rcode = fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST, + __be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR); + int rcode = fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST, root_id, generation, SCODE_100, CSR_REGISTER_BASE + CSR_STATE_SET, - transaction_data, 4); + &data, sizeof(data)); if (rcode == RCODE_GENERATION) return; } From 7dc12e84eff7f934e2456a858ad23d3743c69578 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 8 Sep 2025 10:21:04 +0900 Subject: [PATCH 14/43] firewire: core: code refactoring to evaluate transaction result to CSR_BUS_MANAGER_ID The call of bm_work should be done after acquiring spin lock of fw_card. For asynchronous transaction, the lock should be released temporarily due to event waiting. A commit 27310d561622 ("firewire: core: use guard macro to maintain properties of fw_card") applied scoped_guard() to the bm_work function, however it looks hard to follow to the control flow. This commit refactors the spin lock acquisition after the transaction. Link: https://lore.kernel.org/r/20250908012108.514698-8-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index b98797e4f1d4..e1a7a151b109 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -291,7 +291,7 @@ static void bm_work(struct work_struct *work) struct fw_card *card __free(card_unref) = from_work(card, work, bm_work.work); struct fw_device *root_device, *irm_device; struct fw_node *root_node __free(node_unref) = NULL; - int root_id, new_root_id, irm_id, bm_id, local_id; + int root_id, new_root_id, irm_id, local_id; int gap_count, generation, grace; bool do_reset = false; bool root_device_is_running; @@ -376,19 +376,22 @@ static void bm_work(struct work_struct *work) if (rcode == RCODE_GENERATION) return; - bm_id = be32_to_cpu(data[0]); + spin_lock_irq(&card->lock); - scoped_guard(spinlock_irq, &card->lock) { - if (rcode == RCODE_COMPLETE && generation == card->generation) - card->bm_node_id = - bm_id == 0x3f ? local_id : 0xffc0 | bm_id; - } + if (rcode == RCODE_COMPLETE) { + int bm_id = be32_to_cpu(data[0]); - if (rcode == RCODE_COMPLETE && bm_id != 0x3f) { - /* Somebody else is BM. Only act as IRM. */ - if (local_id == irm_id) - allocate_broadcast_channel(card, generation); - return; + if (generation == card->generation) + card->bm_node_id = bm_id == 0x3f ? local_id : 0xffc0 | bm_id; + + if (bm_id != 0x3f) { + spin_unlock_irq(&card->lock); + + // Somebody else is BM. Only act as IRM. + if (local_id == irm_id) + allocate_broadcast_channel(card, generation); + return; + } } if (rcode == RCODE_SEND_ERROR) { @@ -397,12 +400,11 @@ static void bm_work(struct work_struct *work) * some local problem. Let's try again later and hope * that the problem has gone away by then. */ + spin_unlock_irq(&card->lock); fw_schedule_bm_work(card, DIV_ROUND_UP(HZ, 8)); return; } - spin_lock_irq(&card->lock); - if (rcode != RCODE_COMPLETE && !keep_this_irm) { /* * The lock request failed, maybe the IRM From 8c2d2fcd6b7934c7d694148b542760144a2a1b5a Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 8 Sep 2025 10:21:05 +0900 Subject: [PATCH 15/43] firewire: core: refer fw_card member to initiate bus reset under acquiring lock The gap_count member of fw_card structure is referred when initiate bus reset. This reference is done out of acquiring lock. This is not good. This commit takes the reference within the acquiring lock, with additional code refactoring. Link: https://lore.kernel.org/r/20250908012108.514698-9-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 52 ++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index e1a7a151b109..630e229c9cc2 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -292,7 +292,7 @@ static void bm_work(struct work_struct *work) struct fw_device *root_device, *irm_device; struct fw_node *root_node __free(node_unref) = NULL; int root_id, new_root_id, irm_id, local_id; - int gap_count, generation, grace; + int expected_gap_count, generation, grace; bool do_reset = false; bool root_device_is_running; bool root_device_is_cmc; @@ -485,9 +485,9 @@ static void bm_work(struct work_struct *work) */ if (!card->beta_repeaters_present && root_node->max_hops < ARRAY_SIZE(gap_count_table)) - gap_count = gap_count_table[root_node->max_hops]; + expected_gap_count = gap_count_table[root_node->max_hops]; else - gap_count = 63; + expected_gap_count = 63; /* * Finally, figure out if we should do a reset or not. If we have @@ -495,16 +495,17 @@ static void bm_work(struct work_struct *work) * have either a new root or a new gap count setting, let's do it. */ - if (card->bm_retries++ < 5 && - (card->gap_count != gap_count || new_root_id != root_id)) + if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id)) do_reset = true; - spin_unlock_irq(&card->lock); - if (do_reset) { + int card_gap_count = card->gap_count; + + spin_unlock_irq(&card->lock); + fw_notice(card, "phy config: new root=%x, gap_count=%d\n", - new_root_id, gap_count); - fw_send_phy_config(card, new_root_id, generation, gap_count); + new_root_id, expected_gap_count); + fw_send_phy_config(card, new_root_id, generation, expected_gap_count); /* * Where possible, use a short bus reset to minimize * disruption to isochronous transfers. But in the event @@ -517,26 +518,25 @@ static void bm_work(struct work_struct *work) * may treat it as two, causing a gap count inconsistency * again. Using a long bus reset prevents this. */ - reset_bus(card, card->gap_count != 0); + reset_bus(card, card_gap_count != 0); /* Will allocate broadcast channel after the reset. */ - return; - } + } else { + spin_unlock_irq(&card->lock); - if (root_device_is_cmc) { - /* - * Make sure that the cycle master sends cycle start packets. - */ - __be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR); - int rcode = fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST, - root_id, generation, SCODE_100, - CSR_REGISTER_BASE + CSR_STATE_SET, - &data, sizeof(data)); - if (rcode == RCODE_GENERATION) - return; - } + if (root_device_is_cmc) { + // Make sure that the cycle master sends cycle start packets. + __be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR); + int rcode = fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST, + root_id, generation, SCODE_100, + CSR_REGISTER_BASE + CSR_STATE_SET, + &data, sizeof(data)); + if (rcode == RCODE_GENERATION) + return; + } - if (local_id == irm_id) - allocate_broadcast_channel(card, generation); + if (local_id == irm_id) + allocate_broadcast_channel(card, generation); + } } void fw_card_initialize(struct fw_card *card, From ca17601b15d12bc8c435a4068fa2f907501d9305 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 8 Sep 2025 10:21:06 +0900 Subject: [PATCH 16/43] firewire: core: code refactoring to detect both IEEE 1394:1995 IRM and Canon MV5i The detection of IEEE 1394:1995 and Canon MV5i is just required within some of the condition branches. In this case, these check can be capsulated within these branches. This commit refactors the checks. Link: https://lore.kernel.org/r/20250908012108.514698-10-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 630e229c9cc2..99aa98f195ba 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -289,15 +289,13 @@ static void bm_work(struct work_struct *work) 63, 5, 7, 8, 10, 13, 16, 18, 21, 24, 26, 29, 32, 35, 37, 40 }; struct fw_card *card __free(card_unref) = from_work(card, work, bm_work.work); - struct fw_device *root_device, *irm_device; + struct fw_device *root_device; struct fw_node *root_node __free(node_unref) = NULL; int root_id, new_root_id, irm_id, local_id; int expected_gap_count, generation, grace; bool do_reset = false; bool root_device_is_running; bool root_device_is_cmc; - bool irm_is_1394_1995_only; - bool keep_this_irm; lockdep_assert_held(&card->lock); @@ -316,14 +314,6 @@ static void bm_work(struct work_struct *work) atomic_read(&root_device->state) == FW_DEVICE_RUNNING; root_device_is_cmc = root_device && root_device->cmc; - irm_device = fw_node_get_device(card->irm_node); - irm_is_1394_1995_only = irm_device && irm_device->config_rom && - (irm_device->config_rom[2] & 0x000000f0) == 0; - - /* Canon MV5i works unreliably if it is not root node. */ - keep_this_irm = irm_device && irm_device->config_rom && - irm_device->config_rom[3] >> 8 == CANON_OUI; - root_id = root_node->node_id; irm_id = card->irm_node->node_id; local_id = card->local_node->node_id; @@ -349,6 +339,9 @@ static void bm_work(struct work_struct *work) cpu_to_be32(0x3f), cpu_to_be32(local_id), }; + struct fw_device *irm_device = fw_node_get_device(card->irm_node); + bool irm_is_1394_1995_only = false; + bool keep_this_irm = false; int rcode; if (!card->irm_node->link_on) { @@ -358,6 +351,13 @@ static void bm_work(struct work_struct *work) goto pick_me; } + if (irm_device && irm_device->config_rom) { + irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0; + + // Canon MV5i works unreliably if it is not root node. + keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI; + } + if (irm_is_1394_1995_only && !keep_this_irm) { new_root_id = local_id; fw_notice(card, "%s, making local node (%02x) root\n", From cae2d92cdcae3f2f4510feb631661e86a26da55e Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 8 Sep 2025 10:21:07 +0900 Subject: [PATCH 17/43] firewire: core: code refactoring to investigate root node for bus manager In the middle of bm_work function, both the value of gap_count and the state of root node are investigated. Current implementation is not a good shape since the investigation is aligned to be flat. This commit refactors the investigation with two large branches. Link: https://lore.kernel.org/r/20250908012108.514698-11-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 56 +++++++++++++++++------------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 99aa98f195ba..b430a70a7eeb 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -294,7 +294,6 @@ static void bm_work(struct work_struct *work) int root_id, new_root_id, irm_id, local_id; int expected_gap_count, generation, grace; bool do_reset = false; - bool root_device_is_running; bool root_device_is_cmc; lockdep_assert_held(&card->lock); @@ -310,8 +309,6 @@ static void bm_work(struct work_struct *work) root_node = fw_node_get(card->root_node); root_device = fw_node_get_device(root_node); - root_device_is_running = root_device && - atomic_read(&root_device->state) == FW_DEVICE_RUNNING; root_device_is_cmc = root_device && root_device->cmc; root_id = root_node->node_id; @@ -450,34 +447,35 @@ static void bm_work(struct work_struct *work) * is inconsistent, so bypass the 5-reset limit. */ card->bm_retries = 0; - } else if (root_device == NULL) { - /* - * Either link_on is false, or we failed to read the - * config rom. In either case, pick another root. - */ - new_root_id = local_id; - } else if (!root_device_is_running) { - /* - * If we haven't probed this device yet, bail out now - * and let's try again once that's done. - */ - spin_unlock_irq(&card->lock); - return; - } else if (root_device_is_cmc) { - /* - * We will send out a force root packet for this - * node as part of the gap count optimization. - */ - new_root_id = root_id; } else { - /* - * Current root has an active link layer and we - * successfully read the config rom, but it's not - * cycle master capable. - */ - new_root_id = local_id; - } + // Now investigate root node. + struct fw_device *root_device = fw_node_get_device(root_node); + if (root_device == NULL) { + // Either link_on is false, or we failed to read the + // config rom. In either case, pick another root. + new_root_id = local_id; + } else { + bool root_device_is_running = + atomic_read(&root_device->state) == FW_DEVICE_RUNNING; + + if (!root_device_is_running) { + // If we haven't probed this device yet, bail out now + // and let's try again once that's done. + spin_unlock_irq(&card->lock); + return; + } else if (root_device->cmc) { + // We will send out a force root packet for this + // node as part of the gap count optimization. + new_root_id = root_id; + } else { + // Current root has an active link layer and we + // successfully read the config rom, but it's not + // cycle master capable. + new_root_id = local_id; + } + } + } pick_me: /* * Pick a gap count from 1394a table E-1. The table doesn't cover From a4bac55d99d37976209e2fc2c32bd3dfc86b0447 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 8 Sep 2025 10:21:08 +0900 Subject: [PATCH 18/43] firewire: core: code refactoring whether root node is cycle master capable The check of cycle master capability in root node is currently just in a condition branch. In this case, the required variable should be within the branch. This commit is just for the purpose. Link: https://lore.kernel.org/r/20250908012108.514698-12-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index b430a70a7eeb..474d8066e090 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -289,12 +289,10 @@ static void bm_work(struct work_struct *work) 63, 5, 7, 8, 10, 13, 16, 18, 21, 24, 26, 29, 32, 35, 37, 40 }; struct fw_card *card __free(card_unref) = from_work(card, work, bm_work.work); - struct fw_device *root_device; struct fw_node *root_node __free(node_unref) = NULL; int root_id, new_root_id, irm_id, local_id; int expected_gap_count, generation, grace; bool do_reset = false; - bool root_device_is_cmc; lockdep_assert_held(&card->lock); @@ -308,8 +306,6 @@ static void bm_work(struct work_struct *work) generation = card->generation; root_node = fw_node_get(card->root_node); - root_device = fw_node_get_device(root_node); - root_device_is_cmc = root_device && root_device->cmc; root_id = root_node->node_id; irm_id = card->irm_node->node_id; @@ -519,9 +515,11 @@ static void bm_work(struct work_struct *work) reset_bus(card, card_gap_count != 0); /* Will allocate broadcast channel after the reset. */ } else { + struct fw_device *root_device = fw_node_get_device(root_node); + spin_unlock_irq(&card->lock); - if (root_device_is_cmc) { + if (root_device && root_device->cmc) { // Make sure that the cycle master sends cycle start packets. __be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR); int rcode = fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST, From 136d8a6f73fee0686d163dca91fdffb35e25f092 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Fri, 12 Sep 2025 07:13:11 +0900 Subject: [PATCH 19/43] firewire: core: remove useless lockdep_assert_held() The bm_work work item should be scheduled after holding fw_card reference counting. At a commit 25feb1a96e21 ("firewire: core: use cleanup function in bm_work"), I misinterpreted it as fw_card spinlock and inserted lockdep_assert_hold() wrongly. This commit removes the useless line. Fixes: 25feb1a96e21 ("firewire: core: use cleanup function in bm_work") Link: https://lore.kernel.org/r/20250911221312.678076-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 474d8066e090..32cf6b3344cd 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -294,8 +294,6 @@ static void bm_work(struct work_struct *work) int expected_gap_count, generation, grace; bool do_reset = false; - lockdep_assert_held(&card->lock); - spin_lock_irq(&card->lock); if (card->local_node == NULL) { From 91bf158f8cdf6fd344d3035a13ac746d5846de33 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 13 Sep 2025 19:57:36 +0900 Subject: [PATCH 20/43] firewire: core: use macro expression for gap count mismatch The gap_count field is assigned to zero when mismatch is detected. In such case, the macro expression is preferable since it is easy to understand the situation. This commit applies the idea. Link: https://lore.kernel.org/r/20250913105737.778038-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 2 +- drivers/firewire/core-topology.c | 2 +- drivers/firewire/core.h | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 32cf6b3344cd..bf2e7f55b83e 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -425,7 +425,7 @@ static void bm_work(struct work_struct *work) */ card->bm_generation = generation; - if (card->gap_count == 0) { + if (card->gap_count == GAP_COUNT_MISMATCHED) { /* * If self IDs have inconsistent gap counts, do a * bus reset ASAP. The config rom read might never diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index 74a6aa7d8cc9..5f8fb1201d80 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -241,7 +241,7 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self // If PHYs report different gap counts, set an invalid count which will force a gap // count reconfiguration and a reset. if (phy_packet_self_id_zero_get_gap_count(self_id_sequence[0]) != gap_count) - gap_count = 0; + gap_count = GAP_COUNT_MISMATCHED; update_hop_count(node); diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index 083e39034c37..79eb57fd5812 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -27,6 +27,9 @@ struct fw_packet; /* -card */ +// This is the arbitrary value we use to indicate a mismatched gap count. +#define GAP_COUNT_MISMATCHED 0 + extern __printf(2, 3) void fw_err(const struct fw_card *card, const char *fmt, ...); extern __printf(2, 3) From 2ba08d1bad79cc8d9c82f529adc01f27118e0ca7 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 13 Sep 2025 19:57:37 +0900 Subject: [PATCH 21/43] firewire: core: use macro expression for not-registered state of BUS_MANAGER_ID The value of BUS_MANAGER_ID register has 0x3f when no node_id is registered. Current implementation uses hard-coded numeric literal but in the case the macro expression is preferable since it is easy to distinguish the state from node ID mask. This commit applies the idea. Link: https://lore.kernel.org/r/20250913105737.778038-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 12 ++++++++---- drivers/firewire/core.h | 3 +++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index bf2e7f55b83e..adb90161c4c6 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -327,7 +327,7 @@ static void bm_work(struct work_struct *work) * next generation. */ __be32 data[2] = { - cpu_to_be32(0x3f), + cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED), cpu_to_be32(local_id), }; struct fw_device *irm_device = fw_node_get_device(card->irm_node); @@ -372,10 +372,14 @@ static void bm_work(struct work_struct *work) if (rcode == RCODE_COMPLETE) { int bm_id = be32_to_cpu(data[0]); - if (generation == card->generation) - card->bm_node_id = bm_id == 0x3f ? local_id : 0xffc0 | bm_id; + if (generation == card->generation) { + if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) + card->bm_node_id = 0xffc0 & bm_id; + else + card->bm_node_id = local_id; + } - if (bm_id != 0x3f) { + if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) { spin_unlock_irq(&card->lock); // Somebody else is BM. Only act as IRM. diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index 79eb57fd5812..9e68ebf0673d 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -170,6 +170,9 @@ static inline void fw_iso_context_init_work(struct fw_iso_context *ctx, work_fun /* -topology */ +// The initial value of BUS_MANAGER_ID register, to express nothing registered. +#define BUS_MANAGER_ID_NOT_REGISTERED 0x3f + enum { FW_NODE_CREATED, FW_NODE_UPDATED, From 379b870c28c6a615a101df7986eba70fea99eff7 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 15 Sep 2025 11:42:31 +0900 Subject: [PATCH 22/43] firewire: core: use helper macros instead of direct access to HZ There are some macros available to convert usecs, msecs, and secs into jiffies count. Link: https://lore.kernel.org/r/20250915024232.851955-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 17 +++++++---------- drivers/firewire/core-cdev.c | 4 ++-- drivers/firewire/core-device.c | 6 +++--- drivers/firewire/core-transaction.c | 4 ++-- drivers/firewire/core.h | 2 ++ 5 files changed, 16 insertions(+), 17 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index adb90161c4c6..2541e8bb4b75 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -229,8 +229,7 @@ void fw_schedule_bus_reset(struct fw_card *card, bool delayed, bool short_reset) /* Use an arbitrary short delay to combine multiple reset requests. */ fw_card_get(card); - if (!queue_delayed_work(fw_workqueue, &card->br_work, - delayed ? DIV_ROUND_UP(HZ, 100) : 0)) + if (!queue_delayed_work(fw_workqueue, &card->br_work, delayed ? msecs_to_jiffies(10) : 0)) fw_card_put(card); } EXPORT_SYMBOL(fw_schedule_bus_reset); @@ -241,10 +240,10 @@ static void br_work(struct work_struct *work) /* Delay for 2s after last reset per IEEE 1394 clause 8.2.1. */ if (card->reset_jiffies != 0 && - time_before64(get_jiffies_64(), card->reset_jiffies + 2 * HZ)) { + time_before64(get_jiffies_64(), card->reset_jiffies + secs_to_jiffies(2))) { trace_bus_reset_postpone(card->index, card->generation, card->br_short); - if (!queue_delayed_work(fw_workqueue, &card->br_work, 2 * HZ)) + if (!queue_delayed_work(fw_workqueue, &card->br_work, secs_to_jiffies(2))) fw_card_put(card); return; } @@ -309,8 +308,7 @@ static void bm_work(struct work_struct *work) irm_id = card->irm_node->node_id; local_id = card->local_node->node_id; - grace = time_after64(get_jiffies_64(), - card->reset_jiffies + DIV_ROUND_UP(HZ, 8)); + grace = time_after64(get_jiffies_64(), card->reset_jiffies + msecs_to_jiffies(125)); if ((is_next_generation(generation, card->bm_generation) && !card->bm_abdicate) || @@ -396,7 +394,7 @@ static void bm_work(struct work_struct *work) * that the problem has gone away by then. */ spin_unlock_irq(&card->lock); - fw_schedule_bm_work(card, DIV_ROUND_UP(HZ, 8)); + fw_schedule_bm_work(card, msecs_to_jiffies(125)); return; } @@ -418,7 +416,7 @@ static void bm_work(struct work_struct *work) * bus reset is less than 125ms ago. Reschedule this job. */ spin_unlock_irq(&card->lock); - fw_schedule_bm_work(card, DIV_ROUND_UP(HZ, 8)); + fw_schedule_bm_work(card, msecs_to_jiffies(125)); return; } @@ -551,8 +549,7 @@ void fw_card_initialize(struct fw_card *card, card->split_timeout_hi = DEFAULT_SPLIT_TIMEOUT / 8000; card->split_timeout_lo = (DEFAULT_SPLIT_TIMEOUT % 8000) << 19; card->split_timeout_cycles = DEFAULT_SPLIT_TIMEOUT; - card->split_timeout_jiffies = - DIV_ROUND_UP(DEFAULT_SPLIT_TIMEOUT * HZ, 8000); + card->split_timeout_jiffies = isoc_cycles_to_jiffies(DEFAULT_SPLIT_TIMEOUT); card->color = 0; card->broadcast_channel = BROADCAST_CHANNEL_INITIAL; diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 78b10c6ef7fe..9e90c79becdb 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -1324,8 +1324,8 @@ static void iso_resource_work(struct work_struct *work) todo = r->todo; // Allow 1000ms grace period for other reallocations. if (todo == ISO_RES_ALLOC && - time_before64(get_jiffies_64(), client->device->card->reset_jiffies + HZ)) { - schedule_iso_resource(r, DIV_ROUND_UP(HZ, 3)); + time_before64(get_jiffies_64(), client->device->card->reset_jiffies + secs_to_jiffies(1))) { + schedule_iso_resource(r, msecs_to_jiffies(333)); skip = true; } else { // We could be called twice within the same generation. diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index 6a04a0014694..7d5821cd9b37 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -847,9 +847,9 @@ static void fw_schedule_device_work(struct fw_device *device, */ #define MAX_RETRIES 10 -#define RETRY_DELAY (3 * HZ) -#define INITIAL_DELAY (HZ / 2) -#define SHUTDOWN_DELAY (2 * HZ) +#define RETRY_DELAY secs_to_jiffies(3) +#define INITIAL_DELAY msecs_to_jiffies(500) +#define SHUTDOWN_DELAY secs_to_jiffies(2) static void fw_device_shutdown(struct work_struct *work) { diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 1d1c2d8f85ae..623e1d9bd107 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -458,7 +458,7 @@ static struct fw_packet phy_config_packet = { void fw_send_phy_config(struct fw_card *card, int node_id, int generation, int gap_count) { - long timeout = DIV_ROUND_UP(HZ, 10); + long timeout = msecs_to_jiffies(100); u32 data = 0; phy_packet_set_packet_identifier(&data, PHY_PACKET_PACKET_IDENTIFIER_PHY_CONFIG); @@ -1220,7 +1220,7 @@ static void update_split_timeout(struct fw_card *card) cycles = clamp(cycles, 800u, 3u * 8000u); card->split_timeout_cycles = cycles; - card->split_timeout_jiffies = DIV_ROUND_UP(cycles * HZ, 8000); + card->split_timeout_jiffies = isoc_cycles_to_jiffies(cycles); } static void handle_registers(struct fw_card *card, struct fw_request *request, diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index 9e68ebf0673d..7f2ca93406ce 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -30,6 +30,8 @@ struct fw_packet; // This is the arbitrary value we use to indicate a mismatched gap count. #define GAP_COUNT_MISMATCHED 0 +#define isoc_cycles_to_jiffies(cycles) usecs_to_jiffies(cycles * USEC_PER_SEC / 8000) + extern __printf(2, 3) void fw_err(const struct fw_card *card, const char *fmt, ...); extern __printf(2, 3) From 931383f161c066ac5fda12035540498931739842 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 15 Sep 2025 11:42:32 +0900 Subject: [PATCH 23/43] firewire: core: use helper macro to compare against current jiffies The pattern of calling either time_before64() or time_after64() with get_jiffies_64() can be replaced with the corresponding helper macros. Link: https://lore.kernel.org/r/20250915024232.851955-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 4 ++-- drivers/firewire/core-cdev.c | 2 +- drivers/firewire/core-device.c | 3 +-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 2541e8bb4b75..b5e01a711145 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -240,7 +240,7 @@ static void br_work(struct work_struct *work) /* Delay for 2s after last reset per IEEE 1394 clause 8.2.1. */ if (card->reset_jiffies != 0 && - time_before64(get_jiffies_64(), card->reset_jiffies + secs_to_jiffies(2))) { + time_is_after_jiffies64(card->reset_jiffies + secs_to_jiffies(2))) { trace_bus_reset_postpone(card->index, card->generation, card->br_short); if (!queue_delayed_work(fw_workqueue, &card->br_work, secs_to_jiffies(2))) @@ -308,7 +308,7 @@ static void bm_work(struct work_struct *work) irm_id = card->irm_node->node_id; local_id = card->local_node->node_id; - grace = time_after64(get_jiffies_64(), card->reset_jiffies + msecs_to_jiffies(125)); + grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125)); if ((is_next_generation(generation, card->bm_generation) && !card->bm_abdicate) || diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 9e90c79becdb..1be8f5eb3e17 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -1324,7 +1324,7 @@ static void iso_resource_work(struct work_struct *work) todo = r->todo; // Allow 1000ms grace period for other reallocations. if (todo == ISO_RES_ALLOC && - time_before64(get_jiffies_64(), client->device->card->reset_jiffies + secs_to_jiffies(1))) { + time_is_after_jiffies64(client->device->card->reset_jiffies + secs_to_jiffies(1))) { schedule_iso_resource(r, msecs_to_jiffies(333)); skip = true; } else { diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index 7d5821cd9b37..457a0da024a7 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -855,8 +855,7 @@ static void fw_device_shutdown(struct work_struct *work) { struct fw_device *device = from_work(device, work, work.work); - if (time_before64(get_jiffies_64(), - device->card->reset_jiffies + SHUTDOWN_DELAY) + if (time_is_after_jiffies64(device->card->reset_jiffies + SHUTDOWN_DELAY) && !list_empty(&device->card->link)) { fw_schedule_device_work(device, SHUTDOWN_DELAY); return; From 80c5b023a7d6ae41bd79aadece4cb1fc62e95a08 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 16 Sep 2025 08:47:42 +0900 Subject: [PATCH 24/43] firewire: core: use scoped_guard() to manage critical section to update topology At present, guard() macro is used for the critical section to update topology. It is inconvenient to add the other critical sections into the function. This commit uses scoped_guard() macro instead. Link: https://lore.kernel.org/r/20250915234747.915922-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-topology.c | 66 +++++++++++++++----------------- 1 file changed, 30 insertions(+), 36 deletions(-) diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index 5f8fb1201d80..17aaf14cab0b 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -458,46 +458,40 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, trace_bus_reset_handle(card->index, generation, node_id, bm_abdicate, self_ids, self_id_count); - guard(spinlock_irqsave)(&card->lock); + scoped_guard(spinlock, &card->lock) { + // If the selfID buffer is not the immediate successor of the + // previously processed one, we cannot reliably compare the + // old and new topologies. + if (!is_next_generation(generation, card->generation) && card->local_node != NULL) { + fw_destroy_nodes(card); + card->bm_retries = 0; + } + card->broadcast_channel_allocated = card->broadcast_channel_auto_allocated; + card->node_id = node_id; + // Update node_id before generation to prevent anybody from using + // a stale node_id together with a current generation. + smp_wmb(); + card->generation = generation; + card->reset_jiffies = get_jiffies_64(); + card->bm_node_id = 0xffff; + card->bm_abdicate = bm_abdicate; + fw_schedule_bm_work(card, 0); - /* - * If the selfID buffer is not the immediate successor of the - * previously processed one, we cannot reliably compare the - * old and new topologies. - */ - if (!is_next_generation(generation, card->generation) && - card->local_node != NULL) { - fw_destroy_nodes(card); - card->bm_retries = 0; - } + local_node = build_tree(card, self_ids, self_id_count, generation); - card->broadcast_channel_allocated = card->broadcast_channel_auto_allocated; - card->node_id = node_id; - /* - * Update node_id before generation to prevent anybody from using - * a stale node_id together with a current generation. - */ - smp_wmb(); - card->generation = generation; - card->reset_jiffies = get_jiffies_64(); - card->bm_node_id = 0xffff; - card->bm_abdicate = bm_abdicate; - fw_schedule_bm_work(card, 0); + update_topology_map(card, self_ids, self_id_count); - local_node = build_tree(card, self_ids, self_id_count, generation); + card->color++; - update_topology_map(card, self_ids, self_id_count); - - card->color++; - - if (local_node == NULL) { - fw_err(card, "topology build failed\n"); - /* FIXME: We need to issue a bus reset in this case. */ - } else if (card->local_node == NULL) { - card->local_node = local_node; - for_each_fw_node(card, local_node, report_found_node); - } else { - update_tree(card, local_node); + if (local_node == NULL) { + fw_err(card, "topology build failed\n"); + // FIXME: We need to issue a bus reset in this case. + } else if (card->local_node == NULL) { + card->local_node = local_node; + for_each_fw_node(card, local_node, report_found_node); + } else { + update_tree(card, local_node); + } } } EXPORT_SYMBOL(fw_core_handle_bus_reset); From 07c446e35b89bc8774792f8036e595cffdf5b162 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 16 Sep 2025 08:47:43 +0900 Subject: [PATCH 25/43] firewire: core: maintain phy packet receivers locally in cdev layer The list of receivers for phy packet is used only by cdev layer, while it is maintained as a member of fw_card structure. This commit maintains the list locally in cdev layer. Link: https://lore.kernel.org/r/20250915234747.915922-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 1 - drivers/firewire/core-cdev.c | 27 ++++++++++++++++++++------- include/linux/firewire.h | 2 -- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index b5e01a711145..616abb836ef3 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -556,7 +556,6 @@ void fw_card_initialize(struct fw_card *card, kref_init(&card->kref); init_completion(&card->done); INIT_LIST_HEAD(&card->transaction_list); - INIT_LIST_HEAD(&card->phy_receiver_list); spin_lock_init(&card->lock); card->local_node = NULL; diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 1be8f5eb3e17..112b33099610 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -47,6 +47,9 @@ #define FW_CDEV_VERSION_AUTO_FLUSH_ISO_OVERFLOW 5 #define FW_CDEV_VERSION_EVENT_ASYNC_TSTAMP 6 +static DEFINE_SPINLOCK(phy_receiver_list_lock); +static LIST_HEAD(phy_receiver_list); + struct client { u32 version; struct fw_device *device; @@ -1669,15 +1672,16 @@ static int ioctl_send_phy_packet(struct client *client, union ioctl_arg *arg) static int ioctl_receive_phy_packets(struct client *client, union ioctl_arg *arg) { struct fw_cdev_receive_phy_packets *a = &arg->receive_phy_packets; - struct fw_card *card = client->device->card; /* Access policy: Allow this ioctl only on local nodes' device files. */ if (!client->device->is_local) return -ENOSYS; - guard(spinlock_irq)(&card->lock); + // NOTE: This can be without irq when we can guarantee that __fw_send_request() for local + // destination never runs in any type of IRQ context. + scoped_guard(spinlock_irq, &phy_receiver_list_lock) + list_move_tail(&client->phy_receiver_link, &phy_receiver_list); - list_move_tail(&client->phy_receiver_link, &card->phy_receiver_list); client->phy_receiver_closure = a->closure; return 0; @@ -1687,10 +1691,17 @@ void fw_cdev_handle_phy_packet(struct fw_card *card, struct fw_packet *p) { struct client *client; - guard(spinlock_irqsave)(&card->lock); + // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for local + // destination never runs in any type of IRQ context. + guard(spinlock_irqsave)(&phy_receiver_list_lock); - list_for_each_entry(client, &card->phy_receiver_list, phy_receiver_link) { - struct inbound_phy_packet_event *e = kmalloc(sizeof(*e) + 8, GFP_ATOMIC); + list_for_each_entry(client, &phy_receiver_list, phy_receiver_link) { + struct inbound_phy_packet_event *e; + + if (client->device->card != card) + continue; + + e = kmalloc(sizeof(*e) + 8, GFP_ATOMIC); if (e == NULL) break; @@ -1857,7 +1868,9 @@ static int fw_device_op_release(struct inode *inode, struct file *file) struct client_resource *resource; unsigned long index; - scoped_guard(spinlock_irq, &client->device->card->lock) + // NOTE: This can be without irq when we can guarantee that __fw_send_request() for local + // destination never runs in any type of IRQ context. + scoped_guard(spinlock_irq, &phy_receiver_list_lock) list_del(&client->phy_receiver_link); scoped_guard(mutex, &client->device->client_list_mutex) diff --git a/include/linux/firewire.h b/include/linux/firewire.h index d38c6e538e5c..f3260aacf730 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -115,8 +115,6 @@ struct fw_card { int index; struct list_head link; - struct list_head phy_receiver_list; - struct delayed_work br_work; /* bus reset job */ bool br_short; From 7d138cb269dbd2fa9b0da89a9c10503d1cf269d5 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 16 Sep 2025 08:47:44 +0900 Subject: [PATCH 26/43] firewire: core: use spin lock specific to topology map At present, the operation for read transaction to topology map register is not protected by any kind of lock primitives. This causes a potential problem to result in the mixed content of topology map. This commit adds and uses spin lock specific to topology map. Link: https://lore.kernel.org/r/20250915234747.915922-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-topology.c | 22 ++++++++++++++-------- drivers/firewire/core-transaction.c | 6 +++++- include/linux/firewire.h | 6 +++++- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index 17aaf14cab0b..c62cf93f3f65 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -435,20 +435,22 @@ static void update_tree(struct fw_card *card, struct fw_node *root) } } -static void update_topology_map(struct fw_card *card, - u32 *self_ids, int self_id_count) +static void update_topology_map(__be32 *buffer, size_t buffer_size, int root_node_id, + const u32 *self_ids, int self_id_count) { - int node_count = (card->root_node->node_id & 0x3f) + 1; - __be32 *map = card->topology_map; + __be32 *map = buffer; + int node_count = (root_node_id & 0x3f) + 1; + + memset(map, 0, buffer_size); *map++ = cpu_to_be32((self_id_count + 2) << 16); - *map++ = cpu_to_be32(be32_to_cpu(card->topology_map[1]) + 1); + *map++ = cpu_to_be32(be32_to_cpu(buffer[1]) + 1); *map++ = cpu_to_be32((node_count << 16) | self_id_count); while (self_id_count--) *map++ = cpu_to_be32p(self_ids++); - fw_compute_block_crc(card->topology_map); + fw_compute_block_crc(buffer); } void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, @@ -479,8 +481,6 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, local_node = build_tree(card, self_ids, self_id_count, generation); - update_topology_map(card, self_ids, self_id_count); - card->color++; if (local_node == NULL) { @@ -493,5 +493,11 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, update_tree(card, local_node); } } + + // Just used by transaction layer. + scoped_guard(spinlock, &card->topology_map.lock) { + update_topology_map(card->topology_map.buffer, sizeof(card->topology_map.buffer), + card->root_node->node_id, self_ids, self_id_count); + } } EXPORT_SYMBOL(fw_core_handle_bus_reset); diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 623e1d9bd107..8edffafd21c1 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -1196,7 +1196,11 @@ static void handle_topology_map(struct fw_card *card, struct fw_request *request } start = (offset - topology_map_region.start) / 4; - memcpy(payload, &card->topology_map[start], length); + + // NOTE: This can be without irqsave when we can guarantee that fw_send_request() for local + // destination never runs in any type of IRQ context. + scoped_guard(spinlock_irqsave, &card->topology_map.lock) + memcpy(payload, &card->topology_map.buffer[start], length); fw_send_response(card, request, RCODE_COMPLETE); } diff --git a/include/linux/firewire.h b/include/linux/firewire.h index f3260aacf730..aeb71c39e57e 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -129,7 +129,11 @@ struct fw_card { bool broadcast_channel_allocated; u32 broadcast_channel; - __be32 topology_map[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4]; + + struct { + __be32 buffer[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4]; + spinlock_t lock; + } topology_map; __be32 maint_utility_register; From 420bd7068cbfaea0a857472dd631dc48311e2a8f Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 16 Sep 2025 08:47:45 +0900 Subject: [PATCH 27/43] firewire: core: use spin lock specific to transaction The list of instance for asynchronous transaction to wait for response subaction is maintained as a member of fw_card structure. The card-wide spinlock is used at present for any operation over the list, however it is not necessarily suited for the purpose. This commit adds and uses the spin lock specific to maintain the list. Link: https://lore.kernel.org/r/20250915234747.915922-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 12 ++++-- drivers/firewire/core-transaction.c | 59 ++++++++++++++++++----------- include/linux/firewire.h | 10 +++-- 3 files changed, 51 insertions(+), 30 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 616abb836ef3..39f95007305f 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -544,8 +544,12 @@ void fw_card_initialize(struct fw_card *card, card->index = atomic_inc_return(&index); card->driver = driver; card->device = device; - card->current_tlabel = 0; - card->tlabel_mask = 0; + + card->transactions.current_tlabel = 0; + card->transactions.tlabel_mask = 0; + INIT_LIST_HEAD(&card->transactions.list); + spin_lock_init(&card->transactions.lock); + card->split_timeout_hi = DEFAULT_SPLIT_TIMEOUT / 8000; card->split_timeout_lo = (DEFAULT_SPLIT_TIMEOUT % 8000) << 19; card->split_timeout_cycles = DEFAULT_SPLIT_TIMEOUT; @@ -555,7 +559,7 @@ void fw_card_initialize(struct fw_card *card, kref_init(&card->kref); init_completion(&card->done); - INIT_LIST_HEAD(&card->transaction_list); + spin_lock_init(&card->lock); card->local_node = NULL; @@ -772,7 +776,7 @@ void fw_core_remove_card(struct fw_card *card) destroy_workqueue(card->isoc_wq); destroy_workqueue(card->async_wq); - WARN_ON(!list_empty(&card->transaction_list)); + WARN_ON(!list_empty(&card->transactions.list)); } EXPORT_SYMBOL(fw_core_remove_card); diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 8edffafd21c1..5366d8a781ac 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -49,12 +49,14 @@ static int close_transaction(struct fw_transaction *transaction, struct fw_card { struct fw_transaction *t = NULL, *iter; - scoped_guard(spinlock_irqsave, &card->lock) { - list_for_each_entry(iter, &card->transaction_list, link) { + // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for + // local destination never runs in any type of IRQ context. + scoped_guard(spinlock_irqsave, &card->transactions.lock) { + list_for_each_entry(iter, &card->transactions.list, link) { if (iter == transaction) { if (try_cancel_split_timeout(iter)) { list_del_init(&iter->link); - card->tlabel_mask &= ~(1ULL << iter->tlabel); + card->transactions.tlabel_mask &= ~(1ULL << iter->tlabel); t = iter; } break; @@ -117,11 +119,11 @@ static void split_transaction_timeout_callback(struct timer_list *timer) struct fw_transaction *t = timer_container_of(t, timer, split_timeout_timer); struct fw_card *card = t->card; - scoped_guard(spinlock_irqsave, &card->lock) { + scoped_guard(spinlock_irqsave, &card->transactions.lock) { if (list_empty(&t->link)) return; list_del(&t->link); - card->tlabel_mask &= ~(1ULL << t->tlabel); + card->transactions.tlabel_mask &= ~(1ULL << t->tlabel); } if (!t->with_tstamp) { @@ -259,18 +261,21 @@ static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel, } static int allocate_tlabel(struct fw_card *card) +__must_hold(&card->transactions_lock) { int tlabel; - tlabel = card->current_tlabel; - while (card->tlabel_mask & (1ULL << tlabel)) { + lockdep_assert_held(&card->transactions.lock); + + tlabel = card->transactions.current_tlabel; + while (card->transactions.tlabel_mask & (1ULL << tlabel)) { tlabel = (tlabel + 1) & 0x3f; - if (tlabel == card->current_tlabel) + if (tlabel == card->transactions.current_tlabel) return -EBUSY; } - card->current_tlabel = (tlabel + 1) & 0x3f; - card->tlabel_mask |= 1ULL << tlabel; + card->transactions.current_tlabel = (tlabel + 1) & 0x3f; + card->transactions.tlabel_mask |= 1ULL << tlabel; return tlabel; } @@ -331,7 +336,6 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode void *payload, size_t length, union fw_transaction_callback callback, bool with_tstamp, void *callback_data) { - unsigned long flags; int tlabel; /* @@ -339,11 +343,11 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode * the list while holding the card spinlock. */ - spin_lock_irqsave(&card->lock, flags); - - tlabel = allocate_tlabel(card); + // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for + // local destination never runs in any type of IRQ context. + scoped_guard(spinlock_irqsave, &card->transactions.lock) + tlabel = allocate_tlabel(card); if (tlabel < 0) { - spin_unlock_irqrestore(&card->lock, flags); if (!with_tstamp) { callback.without_tstamp(card, RCODE_SEND_ERROR, NULL, 0, callback_data); } else { @@ -368,15 +372,22 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode t->callback = callback; t->with_tstamp = with_tstamp; t->callback_data = callback_data; - - fw_fill_request(&t->packet, tcode, t->tlabel, destination_id, card->node_id, generation, - speed, offset, payload, length); t->packet.callback = transmit_complete_callback; - list_add_tail(&t->link, &card->transaction_list); + // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for + // local destination never runs in any type of IRQ context. + scoped_guard(spinlock_irqsave, &card->lock) { + // The node_id field of fw_card can be updated when handling SelfIDComplete. + fw_fill_request(&t->packet, tcode, t->tlabel, destination_id, card->node_id, + generation, speed, offset, payload, length); + } - spin_unlock_irqrestore(&card->lock, flags); + // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for + // local destination never runs in any type of IRQ context. + scoped_guard(spinlock_irqsave, &card->transactions.lock) + list_add_tail(&t->link, &card->transactions.list); + // Safe with no lock, since the index field of fw_card is immutable once assigned. trace_async_request_outbound_initiate((uintptr_t)t, card->index, generation, speed, t->packet.header, payload, tcode_is_read_request(tcode) ? 0 : length / 4); @@ -1111,12 +1122,14 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p) break; } - scoped_guard(spinlock_irqsave, &card->lock) { - list_for_each_entry(iter, &card->transaction_list, link) { + // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for + // local destination never runs in any type of IRQ context. + scoped_guard(spinlock_irqsave, &card->transactions.lock) { + list_for_each_entry(iter, &card->transactions.list, link) { if (iter->node_id == source && iter->tlabel == tlabel) { if (try_cancel_split_timeout(iter)) { list_del_init(&iter->link); - card->tlabel_mask &= ~(1ULL << iter->tlabel); + card->transactions.tlabel_mask &= ~(1ULL << iter->tlabel); t = iter; } break; diff --git a/include/linux/firewire.h b/include/linux/firewire.h index aeb71c39e57e..8d6801cf2fca 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -88,11 +88,15 @@ struct fw_card { int node_id; int generation; - int current_tlabel; - u64 tlabel_mask; - struct list_head transaction_list; u64 reset_jiffies; + struct { + int current_tlabel; + u64 tlabel_mask; + struct list_head list; + spinlock_t lock; + } transactions; + u32 split_timeout_hi; u32 split_timeout_lo; unsigned int split_timeout_cycles; From b5725cfa4120a4d234ab112aad151d731531d093 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 16 Sep 2025 08:47:46 +0900 Subject: [PATCH 28/43] firewire: core: use spin lock specific to timer for split transaction At present the parameters to compute timeout time for split transaction is protected by card-wide spin lock, while it is not necessarily convenient in a point to narrower critical section. This commit adds and uses another spin lock specific for the purpose. Link: https://lore.kernel.org/r/20250915234747.915922-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 10 +++-- drivers/firewire/core-transaction.c | 63 +++++++++++++++++++---------- include/linux/firewire.h | 15 ++++--- 3 files changed, 57 insertions(+), 31 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 39f95007305f..96495392a1f6 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -550,10 +550,12 @@ void fw_card_initialize(struct fw_card *card, INIT_LIST_HEAD(&card->transactions.list); spin_lock_init(&card->transactions.lock); - card->split_timeout_hi = DEFAULT_SPLIT_TIMEOUT / 8000; - card->split_timeout_lo = (DEFAULT_SPLIT_TIMEOUT % 8000) << 19; - card->split_timeout_cycles = DEFAULT_SPLIT_TIMEOUT; - card->split_timeout_jiffies = isoc_cycles_to_jiffies(DEFAULT_SPLIT_TIMEOUT); + card->split_timeout.hi = DEFAULT_SPLIT_TIMEOUT / 8000; + card->split_timeout.lo = (DEFAULT_SPLIT_TIMEOUT % 8000) << 19; + card->split_timeout.cycles = DEFAULT_SPLIT_TIMEOUT; + card->split_timeout.jiffies = isoc_cycles_to_jiffies(DEFAULT_SPLIT_TIMEOUT); + spin_lock_init(&card->split_timeout.lock); + card->color = 0; card->broadcast_channel = BROADCAST_CHANNEL_INITIAL; diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 5366d8a781ac..dd3656a0c1ff 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -137,14 +137,18 @@ static void split_transaction_timeout_callback(struct timer_list *timer) static void start_split_transaction_timeout(struct fw_transaction *t, struct fw_card *card) { - guard(spinlock_irqsave)(&card->lock); + unsigned long delta; if (list_empty(&t->link) || WARN_ON(t->is_split_transaction)) return; t->is_split_transaction = true; - mod_timer(&t->split_timeout_timer, - jiffies + card->split_timeout_jiffies); + + // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for + // local destination never runs in any type of IRQ context. + scoped_guard(spinlock_irqsave, &card->split_timeout.lock) + delta = card->split_timeout.jiffies; + mod_timer(&t->split_timeout_timer, jiffies + delta); } static u32 compute_split_timeout_timestamp(struct fw_card *card, u32 request_timestamp); @@ -164,8 +168,12 @@ static void transmit_complete_callback(struct fw_packet *packet, break; case ACK_PENDING: { - t->split_timeout_cycle = - compute_split_timeout_timestamp(card, packet->timestamp) & 0xffff; + // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for + // local destination never runs in any type of IRQ context. + scoped_guard(spinlock_irqsave, &card->split_timeout.lock) { + t->split_timeout_cycle = + compute_split_timeout_timestamp(card, packet->timestamp) & 0xffff; + } start_split_transaction_timeout(t, card); break; } @@ -790,11 +798,14 @@ EXPORT_SYMBOL(fw_fill_response); static u32 compute_split_timeout_timestamp(struct fw_card *card, u32 request_timestamp) +__must_hold(&card->split_timeout.lock) { unsigned int cycles; u32 timestamp; - cycles = card->split_timeout_cycles; + lockdep_assert_held(&card->split_timeout.lock); + + cycles = card->split_timeout.cycles; cycles += request_timestamp & 0x1fff; timestamp = request_timestamp & ~0x1fff; @@ -845,9 +856,12 @@ static struct fw_request *allocate_request(struct fw_card *card, return NULL; kref_init(&request->kref); + // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for + // local destination never runs in any type of IRQ context. + scoped_guard(spinlock_irqsave, &card->split_timeout.lock) + request->response.timestamp = compute_split_timeout_timestamp(card, p->timestamp); + request->response.speed = p->speed; - request->response.timestamp = - compute_split_timeout_timestamp(card, p->timestamp); request->response.generation = p->generation; request->response.ack = 0; request->response.callback = free_response_callback; @@ -1228,16 +1242,17 @@ static const struct fw_address_region registers_region = .end = CSR_REGISTER_BASE | CSR_CONFIG_ROM, }; static void update_split_timeout(struct fw_card *card) +__must_hold(&card->split_timeout.lock) { unsigned int cycles; - cycles = card->split_timeout_hi * 8000 + (card->split_timeout_lo >> 19); + cycles = card->split_timeout.hi * 8000 + (card->split_timeout.lo >> 19); /* minimum per IEEE 1394, maximum which doesn't overflow OHCI */ cycles = clamp(cycles, 800u, 3u * 8000u); - card->split_timeout_cycles = cycles; - card->split_timeout_jiffies = isoc_cycles_to_jiffies(cycles); + card->split_timeout.cycles = cycles; + card->split_timeout.jiffies = isoc_cycles_to_jiffies(cycles); } static void handle_registers(struct fw_card *card, struct fw_request *request, @@ -1287,12 +1302,15 @@ static void handle_registers(struct fw_card *card, struct fw_request *request, case CSR_SPLIT_TIMEOUT_HI: if (tcode == TCODE_READ_QUADLET_REQUEST) { - *data = cpu_to_be32(card->split_timeout_hi); + *data = cpu_to_be32(card->split_timeout.hi); } else if (tcode == TCODE_WRITE_QUADLET_REQUEST) { - guard(spinlock_irqsave)(&card->lock); - - card->split_timeout_hi = be32_to_cpu(*data) & 7; - update_split_timeout(card); + // NOTE: This can be without irqsave when we can guarantee that + // __fw_send_request() for local destination never runs in any type of IRQ + // context. + scoped_guard(spinlock_irqsave, &card->split_timeout.lock) { + card->split_timeout.hi = be32_to_cpu(*data) & 7; + update_split_timeout(card); + } } else { rcode = RCODE_TYPE_ERROR; } @@ -1300,12 +1318,15 @@ static void handle_registers(struct fw_card *card, struct fw_request *request, case CSR_SPLIT_TIMEOUT_LO: if (tcode == TCODE_READ_QUADLET_REQUEST) { - *data = cpu_to_be32(card->split_timeout_lo); + *data = cpu_to_be32(card->split_timeout.lo); } else if (tcode == TCODE_WRITE_QUADLET_REQUEST) { - guard(spinlock_irqsave)(&card->lock); - - card->split_timeout_lo = be32_to_cpu(*data) & 0xfff80000; - update_split_timeout(card); + // NOTE: This can be without irqsave when we can guarantee that + // __fw_send_request() for local destination never runs in any type of IRQ + // context. + scoped_guard(spinlock_irqsave, &card->split_timeout.lock) { + card->split_timeout.lo = be32_to_cpu(*data) & 0xfff80000; + update_split_timeout(card); + } } else { rcode = RCODE_TYPE_ERROR; } diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 8d6801cf2fca..6d208769d456 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -97,18 +97,21 @@ struct fw_card { spinlock_t lock; } transactions; - u32 split_timeout_hi; - u32 split_timeout_lo; - unsigned int split_timeout_cycles; - unsigned int split_timeout_jiffies; + struct { + u32 hi; + u32 lo; + unsigned int cycles; + unsigned int jiffies; + spinlock_t lock; + } split_timeout; unsigned long long guid; unsigned max_receive; int link_speed; int config_rom_generation; - spinlock_t lock; /* Take this lock when handling the lists in - * this struct. */ + spinlock_t lock; + struct fw_node *local_node; struct fw_node *root_node; struct fw_node *irm_node; From e0cda0dd12e08ecb8d26b8d78dc63e67e7069510 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 16 Sep 2025 08:47:47 +0900 Subject: [PATCH 29/43] firewire: core: annotate fw_destroy_nodes with must-hold-lock The function, fw_destroy_nodes(), is used widely within firewire-core module. It has a prerequisite condition that struct fw_card.lock must be hold in advance. This commit adds annotation for it. Link: https://lore.kernel.org/r/20250915234747.915922-7-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-topology.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index c62cf93f3f65..8fa0772ee723 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -325,9 +325,11 @@ static void report_found_node(struct fw_card *card, card->bm_retries = 0; } -/* Must be called with card->lock held */ void fw_destroy_nodes(struct fw_card *card) +__must_hold(&card->lock) { + lockdep_assert_held(&card->lock); + card->color++; if (card->local_node != NULL) for_each_fw_node(card, card->local_node, report_lost_node); From b460b317b21d5106f3ebd34bd51b851ad355a70a Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 17 Sep 2025 09:03:45 +0900 Subject: [PATCH 30/43] firewire: core: schedule bm_work item outside of spin lock Before (re)building topology tree, fw_core_handle_bus_reset() schedules a work item under acquiring fw_card spin lock. The work item invokes bm_work() which acquires the spin lock at first, then can be stalled to wait until the building tree finishes. This is inconvenient. This commit moves the timing to schedule the work item after releasing the spin lock. Link: https://lore.kernel.org/r/20250917000347.52369-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-topology.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index 8fa0772ee723..2f73bcd5696f 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -479,7 +479,6 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, card->reset_jiffies = get_jiffies_64(); card->bm_node_id = 0xffff; card->bm_abdicate = bm_abdicate; - fw_schedule_bm_work(card, 0); local_node = build_tree(card, self_ids, self_id_count, generation); @@ -496,6 +495,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, } } + fw_schedule_bm_work(card, 0); + // Just used by transaction layer. scoped_guard(spinlock, &card->topology_map.lock) { update_topology_map(card->topology_map.buffer, sizeof(card->topology_map.buffer), From abe7159125702c734e851bc0c52b51cd446298a5 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 17 Sep 2025 09:03:46 +0900 Subject: [PATCH 31/43] firewire: core: disable bus management work temporarily during updating topology When processing selfID sequence, bus topology tree is (re)built, and some members of fw_card are determined. Once determined, the members are valid during the bus generation. The above operations are in the critical section of fw_card spin lock. Before building the bus topology, a work item is scheduled for bus manager work. The bm_work() function is invoked by the work item. The function tries to acquire the spin lock, then can be stalled until the bus topology building finishes. The bus manager should work once the members of fw_card are determined. This commit suppresses the above situation by disabling the work item during processing selfID sequence. Link: https://lore.kernel.org/r/20250917000347.52369-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-topology.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index 2f73bcd5696f..90b988035a2a 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -460,8 +460,14 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, { struct fw_node *local_node; + might_sleep(); + trace_bus_reset_handle(card->index, generation, node_id, bm_abdicate, self_ids, self_id_count); + // Disable bus management work during updating the cache of bus topology, since the work + // accesses to some members of fw_card. + disable_delayed_work_sync(&card->bm_work); + scoped_guard(spinlock, &card->lock) { // If the selfID buffer is not the immediate successor of the // previously processed one, we cannot reliably compare the @@ -495,6 +501,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, } } + enable_delayed_work(&card->bm_work); + fw_schedule_bm_work(card, 0); // Just used by transaction layer. From 582310376d6e9a8d261b682178713cdc4b251af6 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 17 Sep 2025 09:03:47 +0900 Subject: [PATCH 32/43] firewire: core: shrink critical section of fw_card spinlock in bm_work Now fw_core_handle_bus_reset() and bm_work() are serialized. Some members of fw_card are free to access in bm_work() This commit shrinks critical section of fw_card spinlock in bm_work() Link: https://lore.kernel.org/r/20250917000347.52369-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 96495392a1f6..4fcd5ce4b2ce 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -293,12 +293,8 @@ static void bm_work(struct work_struct *work) int expected_gap_count, generation, grace; bool do_reset = false; - spin_lock_irq(&card->lock); - - if (card->local_node == NULL) { - spin_unlock_irq(&card->lock); + if (card->local_node == NULL) return; - } generation = card->generation; @@ -354,8 +350,6 @@ static void bm_work(struct work_struct *work) goto pick_me; } - spin_unlock_irq(&card->lock); - rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, irm_id, generation, SCODE_100, CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, @@ -365,21 +359,20 @@ static void bm_work(struct work_struct *work) if (rcode == RCODE_GENERATION) return; - spin_lock_irq(&card->lock); - if (rcode == RCODE_COMPLETE) { int bm_id = be32_to_cpu(data[0]); if (generation == card->generation) { - if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) - card->bm_node_id = 0xffc0 & bm_id; - else - card->bm_node_id = local_id; + // Used by cdev layer for "struct fw_cdev_event_bus_reset". + scoped_guard(spinlock, &card->lock) { + if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) + card->bm_node_id = 0xffc0 & bm_id; + else + card->bm_node_id = local_id; + } } if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) { - spin_unlock_irq(&card->lock); - // Somebody else is BM. Only act as IRM. if (local_id == irm_id) allocate_broadcast_channel(card, generation); @@ -393,7 +386,6 @@ static void bm_work(struct work_struct *work) * some local problem. Let's try again later and hope * that the problem has gone away by then. */ - spin_unlock_irq(&card->lock); fw_schedule_bm_work(card, msecs_to_jiffies(125)); return; } @@ -415,7 +407,6 @@ static void bm_work(struct work_struct *work) * We weren't BM in the last generation, and the last * bus reset is less than 125ms ago. Reschedule this job. */ - spin_unlock_irq(&card->lock); fw_schedule_bm_work(card, msecs_to_jiffies(125)); return; } @@ -458,7 +449,6 @@ static void bm_work(struct work_struct *work) if (!root_device_is_running) { // If we haven't probed this device yet, bail out now // and let's try again once that's done. - spin_unlock_irq(&card->lock); return; } else if (root_device->cmc) { // We will send out a force root packet for this @@ -495,8 +485,6 @@ static void bm_work(struct work_struct *work) if (do_reset) { int card_gap_count = card->gap_count; - spin_unlock_irq(&card->lock); - fw_notice(card, "phy config: new root=%x, gap_count=%d\n", new_root_id, expected_gap_count); fw_send_phy_config(card, new_root_id, generation, expected_gap_count); @@ -517,8 +505,6 @@ static void bm_work(struct work_struct *work) } else { struct fw_device *root_device = fw_node_get_device(root_node); - spin_unlock_irq(&card->lock); - if (root_device && root_device->cmc) { // Make sure that the cycle master sends cycle start packets. __be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR); From e6d2338b6f3e522872f3a14fcc5e5de2f58bf23b Mon Sep 17 00:00:00 2001 From: Thorsten Blum Date: Tue, 16 Sep 2025 14:21:45 +0200 Subject: [PATCH 33/43] firewire: core: use struct_size and flex_array_size in ioctl_add_descriptor Use struct_size() to determine the memory needed for a new 'struct descriptor_resource' and flex_array_size() to calculate the number of bytes to copy from userspace. This removes the hardcoded size (4 bytes) for the 'u32 data[]' entries. No functional changes intended. Signed-off-by: Thorsten Blum Link: https://lore.kernel.org/r/20250916122143.2459993-3-thorsten.blum@linux.dev Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 112b33099610..991316200618 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -940,11 +940,12 @@ static int ioctl_add_descriptor(struct client *client, union ioctl_arg *arg) if (a->length > 256) return -EINVAL; - r = kmalloc(sizeof(*r) + a->length * 4, GFP_KERNEL); + r = kmalloc(struct_size(r, data, a->length), GFP_KERNEL); if (r == NULL) return -ENOMEM; - if (copy_from_user(r->data, u64_to_uptr(a->data), a->length * 4)) { + if (copy_from_user(r->data, u64_to_uptr(a->data), + flex_array_size(r, data, a->length))) { ret = -EFAULT; goto failed; } From 182edc05b087f58245d5481e2d4adb231a45a903 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Fri, 19 Sep 2025 08:54:43 +0900 Subject: [PATCH 34/43] firewire: core: remove useless generation check Two functions, fw_core_handle_bus_reset() and bm_work(), are serialized by a commit 3d91fd440cc7 ("firewire: core: disable bus management work temporarily during updating topology"). Therefore the generation member of fw_card is immutable in bm_work(). This commit removes useless generation check in bm_work(). Link: https://lore.kernel.org/r/20250918235448.129705-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 4fcd5ce4b2ce..ef00125fb01a 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -362,14 +362,12 @@ static void bm_work(struct work_struct *work) if (rcode == RCODE_COMPLETE) { int bm_id = be32_to_cpu(data[0]); - if (generation == card->generation) { - // Used by cdev layer for "struct fw_cdev_event_bus_reset". - scoped_guard(spinlock, &card->lock) { - if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) - card->bm_node_id = 0xffc0 & bm_id; - else - card->bm_node_id = local_id; - } + // Used by cdev layer for "struct fw_cdev_event_bus_reset". + scoped_guard(spinlock, &card->lock) { + if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) + card->bm_node_id = 0xffc0 & bm_id; + else + card->bm_node_id = local_id; } if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) { From 52561ebfae9dc9871d6ca2c9e72a4a4e246c4476 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Fri, 19 Sep 2025 08:54:44 +0900 Subject: [PATCH 35/43] firewire: core: use switch statement to evaluate transaction result to CSR_BUS_MANAGER_ID The result of the lock transaction to swap bus manager on isochronous resource manager looks like an ad-hoc style. It is hard to read. This commit uses switch statement to evaluate the result. Link: https://lore.kernel.org/r/20250918235448.129705-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 50 +++++++++++++++++------------------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index ef00125fb01a..e9bf8d93f5b7 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -355,11 +355,18 @@ static void bm_work(struct work_struct *work) CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, data, sizeof(data)); - // Another bus reset, BM work has been rescheduled. - if (rcode == RCODE_GENERATION) + switch (rcode) { + case RCODE_GENERATION: + // Another bus reset, BM work has been rescheduled. return; - - if (rcode == RCODE_COMPLETE) { + case RCODE_SEND_ERROR: + // We have been unable to send the lock request due to + // some local problem. Let's try again later and hope + // that the problem has gone away by then. + fw_schedule_bm_work(card, msecs_to_jiffies(125)); + return; + case RCODE_COMPLETE: + { int bm_id = be32_to_cpu(data[0]); // Used by cdev layer for "struct fw_cdev_event_bus_reset". @@ -376,29 +383,20 @@ static void bm_work(struct work_struct *work) allocate_broadcast_channel(card, generation); return; } + break; } - - if (rcode == RCODE_SEND_ERROR) { - /* - * We have been unable to send the lock request due to - * some local problem. Let's try again later and hope - * that the problem has gone away by then. - */ - fw_schedule_bm_work(card, msecs_to_jiffies(125)); - return; - } - - if (rcode != RCODE_COMPLETE && !keep_this_irm) { - /* - * The lock request failed, maybe the IRM - * isn't really IRM capable after all. Let's - * do a bus reset and pick the local node as - * root, and thus, IRM. - */ - new_root_id = local_id; - fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n", - fw_rcode_string(rcode), new_root_id); - goto pick_me; + default: + if (!keep_this_irm) { + // The lock request failed, maybe the IRM + // isn't really IRM capable after all. Let's + // do a bus reset and pick the local node as + // root, and thus, IRM. + new_root_id = local_id; + fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n", + fw_rcode_string(rcode), new_root_id); + goto pick_me; + } + break; } } else if (card->bm_generation != generation) { /* From e31b990cafd49a8c56eac55094c1a783f5826b47 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Fri, 19 Sep 2025 08:54:45 +0900 Subject: [PATCH 36/43] firewire: core: code refactoring for the case of generation mismatch Current implementation stores the bus generation at which the bus manager contending procedure finishes. The condition for the procedure is the mismatch of the stored generation against current bus generation. This commit refactors the code for the contending procedure. Two existing branches are put into a new branch to detect the generation mismatch, thus the most of change is indentation. Link: https://lore.kernel.org/r/20250918235448.129705-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 194 +++++++++++++++++------------------ 1 file changed, 96 insertions(+), 98 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index e9bf8d93f5b7..02058af705cc 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -290,7 +290,7 @@ static void bm_work(struct work_struct *work) struct fw_card *card __free(card_unref) = from_work(card, work, bm_work.work); struct fw_node *root_node __free(node_unref) = NULL; int root_id, new_root_id, irm_id, local_id; - int expected_gap_count, generation, grace; + int expected_gap_count, generation; bool do_reset = false; if (card->local_node == NULL) @@ -304,107 +304,107 @@ static void bm_work(struct work_struct *work) irm_id = card->irm_node->node_id; local_id = card->local_node->node_id; - grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125)); + if (card->bm_generation != generation) { + bool grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125)); - if ((is_next_generation(generation, card->bm_generation) && - !card->bm_abdicate) || - (card->bm_generation != generation && grace)) { - /* - * This first step is to figure out who is IRM and - * then try to become bus manager. If the IRM is not - * well defined (e.g. does not have an active link - * layer or does not responds to our lock request, we - * will have to do a little vigilante bus management. - * In that case, we do a goto into the gap count logic - * so that when we do the reset, we still optimize the - * gap count. That could well save a reset in the - * next generation. - */ - __be32 data[2] = { - cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED), - cpu_to_be32(local_id), - }; - struct fw_device *irm_device = fw_node_get_device(card->irm_node); - bool irm_is_1394_1995_only = false; - bool keep_this_irm = false; - int rcode; + if (grace || + (is_next_generation(generation, card->bm_generation) && !card->bm_abdicate)) { + // This first step is to figure out who is IRM and + // then try to become bus manager. If the IRM is not + // well defined (e.g. does not have an active link + // layer or does not responds to our lock request, we + // will have to do a little vigilante bus management. + // In that case, we do a goto into the gap count logic + // so that when we do the reset, we still optimize the + // gap count. That could well save a reset in the + // next generation. + __be32 data[2] = { + cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED), + cpu_to_be32(local_id), + }; + struct fw_device *irm_device = fw_node_get_device(card->irm_node); + bool irm_is_1394_1995_only = false; + bool keep_this_irm = false; + int rcode; - if (!card->irm_node->link_on) { - new_root_id = local_id; - fw_notice(card, "%s, making local node (%02x) root\n", - "IRM has link off", new_root_id); - goto pick_me; - } - - if (irm_device && irm_device->config_rom) { - irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0; - - // Canon MV5i works unreliably if it is not root node. - keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI; - } - - if (irm_is_1394_1995_only && !keep_this_irm) { - new_root_id = local_id; - fw_notice(card, "%s, making local node (%02x) root\n", - "IRM is not 1394a compliant", new_root_id); - goto pick_me; - } - - rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, - irm_id, generation, SCODE_100, - CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, - data, sizeof(data)); - - switch (rcode) { - case RCODE_GENERATION: - // Another bus reset, BM work has been rescheduled. - return; - case RCODE_SEND_ERROR: - // We have been unable to send the lock request due to - // some local problem. Let's try again later and hope - // that the problem has gone away by then. - fw_schedule_bm_work(card, msecs_to_jiffies(125)); - return; - case RCODE_COMPLETE: - { - int bm_id = be32_to_cpu(data[0]); - - // Used by cdev layer for "struct fw_cdev_event_bus_reset". - scoped_guard(spinlock, &card->lock) { - if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) - card->bm_node_id = 0xffc0 & bm_id; - else - card->bm_node_id = local_id; - } - - if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) { - // Somebody else is BM. Only act as IRM. - if (local_id == irm_id) - allocate_broadcast_channel(card, generation); - return; - } - break; - } - default: - if (!keep_this_irm) { - // The lock request failed, maybe the IRM - // isn't really IRM capable after all. Let's - // do a bus reset and pick the local node as - // root, and thus, IRM. + if (!card->irm_node->link_on) { new_root_id = local_id; - fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n", - fw_rcode_string(rcode), new_root_id); + fw_notice(card, "%s, making local node (%02x) root\n", + "IRM has link off", new_root_id); goto pick_me; } - break; + + if (irm_device && irm_device->config_rom) { + irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0; + + // Canon MV5i works unreliably if it is not root node. + keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI; + } + + if (irm_is_1394_1995_only && !keep_this_irm) { + new_root_id = local_id; + fw_notice(card, "%s, making local node (%02x) root\n", + "IRM is not 1394a compliant", new_root_id); + goto pick_me; + } + + rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, + irm_id, generation, SCODE_100, + CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, + data, sizeof(data)); + + switch (rcode) { + case RCODE_GENERATION: + // Another bus reset, BM work has been rescheduled. + return; + case RCODE_SEND_ERROR: + // We have been unable to send the lock request due to + // some local problem. Let's try again later and hope + // that the problem has gone away by then. + fw_schedule_bm_work(card, msecs_to_jiffies(125)); + return; + case RCODE_COMPLETE: + { + int bm_id = be32_to_cpu(data[0]); + + // Used by cdev layer for "struct fw_cdev_event_bus_reset". + scoped_guard(spinlock, &card->lock) { + if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) + card->bm_node_id = 0xffc0 & bm_id; + else + card->bm_node_id = local_id; + } + + if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) { + // Somebody else is BM. Only act as IRM. + if (local_id == irm_id) + allocate_broadcast_channel(card, generation); + return; + } + break; + } + default: + if (!keep_this_irm) { + // The lock request failed, maybe the IRM + // isn't really IRM capable after all. Let's + // do a bus reset and pick the local node as + // root, and thus, IRM. + new_root_id = local_id; + fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n", + fw_rcode_string(rcode), new_root_id); + goto pick_me; + } + break; + } + + // A node contends for bus manager in this generation. + card->bm_generation = generation; + } else { + // We weren't BM in the last generation, and the last + // bus reset is less than 125ms ago. Reschedule this job. + fw_schedule_bm_work(card, msecs_to_jiffies(125)); + return; } - } else if (card->bm_generation != generation) { - /* - * We weren't BM in the last generation, and the last - * bus reset is less than 125ms ago. Reschedule this job. - */ - fw_schedule_bm_work(card, msecs_to_jiffies(125)); - return; } /* @@ -412,8 +412,6 @@ static void bm_work(struct work_struct *work) * make sure we have an active cycle master and do gap count * optimization. */ - card->bm_generation = generation; - if (card->gap_count == GAP_COUNT_MISMATCHED) { /* * If self IDs have inconsistent gap counts, do a From 4ff62194d3739c8c7c2d2f365cbc069753387d79 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Fri, 19 Sep 2025 08:54:46 +0900 Subject: [PATCH 37/43] firewire: core: code refactoring to split contention procedure for bus manager The precedure to contend for bus manager has much code. It is better to split it into a helper function. This commit refactors in the point. Link: https://lore.kernel.org/r/20250918235448.129705-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 223 ++++++++++++++++++++--------------- 1 file changed, 127 insertions(+), 96 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 02058af705cc..6268b595d4fa 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -279,6 +279,102 @@ void fw_schedule_bm_work(struct fw_card *card, unsigned long delay) fw_card_put(card); } +enum bm_contention_outcome { + // The bus management contention window is not expired. + BM_CONTENTION_OUTCOME_WITHIN_WINDOW = 0, + // The IRM node has link off. + BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF, + // The IRM node complies IEEE 1394:1994 only. + BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY, + // Another bus reset, BM work has been rescheduled. + BM_CONTENTION_OUTCOME_AT_NEW_GENERATION, + // We have been unable to send the lock request to IRM node due to some local problem. + BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION, + // The lock request failed, maybe the IRM isn't really IRM capable after all. + BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM, + // Somebody else is BM. + BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM, + // The local node succeeds after contending for bus manager. + BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM, +}; + +static enum bm_contention_outcome contend_for_bm(struct fw_card *card) +{ + int generation = card->generation; + int local_id = card->local_node->node_id; + __be32 data[2] = { + cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED), + cpu_to_be32(local_id), + }; + bool grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125)); + bool irm_is_1394_1995_only = false; + bool keep_this_irm = false; + struct fw_node *irm_node; + struct fw_device *irm_device; + int rcode; + + if (!grace) { + if (!is_next_generation(generation, card->bm_generation) || card->bm_abdicate) + return BM_CONTENTION_OUTCOME_WITHIN_WINDOW; + } + + irm_node = card->irm_node; + if (!irm_node->link_on) { + fw_notice(card, "IRM has link off, making local node (%02x) root\n", local_id); + return BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF; + } + + irm_device = fw_node_get_device(irm_node); + if (irm_device && irm_device->config_rom) { + irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0; + + // Canon MV5i works unreliably if it is not root node. + keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI; + } + + if (irm_is_1394_1995_only && !keep_this_irm) { + fw_notice(card, "IRM is not 1394a compliant, making local node (%02x) root\n", + local_id); + return BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY; + } + + rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, irm_node->node_id, generation, + SCODE_100, CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, data, + sizeof(data)); + + switch (rcode) { + case RCODE_GENERATION: + return BM_CONTENTION_OUTCOME_AT_NEW_GENERATION; + case RCODE_SEND_ERROR: + return BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION; + case RCODE_COMPLETE: + { + int bm_id = be32_to_cpu(data[0]); + + // Used by cdev layer for "struct fw_cdev_event_bus_reset". + scoped_guard(spinlock, &card->lock) { + if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) + card->bm_node_id = 0xffc0 & bm_id; + else + card->bm_node_id = local_id; + } + + if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) + return BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM; + else + return BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM; + } + default: + if (!keep_this_irm) { + fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n", + fw_rcode_string(rcode), local_id); + return BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY; + } else { + return BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM; + } + } +} + DEFINE_FREE(node_unref, struct fw_node *, if (_T) fw_node_put(_T)) DEFINE_FREE(card_unref, struct fw_card *, if (_T) fw_card_put(_T)) @@ -305,105 +401,40 @@ static void bm_work(struct work_struct *work) local_id = card->local_node->node_id; if (card->bm_generation != generation) { - bool grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125)); + enum bm_contention_outcome result = contend_for_bm(card); - if (grace || - (is_next_generation(generation, card->bm_generation) && !card->bm_abdicate)) { - // This first step is to figure out who is IRM and - // then try to become bus manager. If the IRM is not - // well defined (e.g. does not have an active link - // layer or does not responds to our lock request, we - // will have to do a little vigilante bus management. - // In that case, we do a goto into the gap count logic - // so that when we do the reset, we still optimize the - // gap count. That could well save a reset in the - // next generation. - __be32 data[2] = { - cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED), - cpu_to_be32(local_id), - }; - struct fw_device *irm_device = fw_node_get_device(card->irm_node); - bool irm_is_1394_1995_only = false; - bool keep_this_irm = false; - int rcode; - - if (!card->irm_node->link_on) { - new_root_id = local_id; - fw_notice(card, "%s, making local node (%02x) root\n", - "IRM has link off", new_root_id); - goto pick_me; - } - - if (irm_device && irm_device->config_rom) { - irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0; - - // Canon MV5i works unreliably if it is not root node. - keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI; - } - - if (irm_is_1394_1995_only && !keep_this_irm) { - new_root_id = local_id; - fw_notice(card, "%s, making local node (%02x) root\n", - "IRM is not 1394a compliant", new_root_id); - goto pick_me; - } - - rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, - irm_id, generation, SCODE_100, - CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, - data, sizeof(data)); - - switch (rcode) { - case RCODE_GENERATION: - // Another bus reset, BM work has been rescheduled. - return; - case RCODE_SEND_ERROR: - // We have been unable to send the lock request due to - // some local problem. Let's try again later and hope - // that the problem has gone away by then. - fw_schedule_bm_work(card, msecs_to_jiffies(125)); - return; - case RCODE_COMPLETE: - { - int bm_id = be32_to_cpu(data[0]); - - // Used by cdev layer for "struct fw_cdev_event_bus_reset". - scoped_guard(spinlock, &card->lock) { - if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) - card->bm_node_id = 0xffc0 & bm_id; - else - card->bm_node_id = local_id; - } - - if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) { - // Somebody else is BM. Only act as IRM. - if (local_id == irm_id) - allocate_broadcast_channel(card, generation); - return; - } - break; - } - default: - if (!keep_this_irm) { - // The lock request failed, maybe the IRM - // isn't really IRM capable after all. Let's - // do a bus reset and pick the local node as - // root, and thus, IRM. - new_root_id = local_id; - fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n", - fw_rcode_string(rcode), new_root_id); - goto pick_me; - } - break; - } - - // A node contends for bus manager in this generation. - card->bm_generation = generation; - } else { - // We weren't BM in the last generation, and the last - // bus reset is less than 125ms ago. Reschedule this job. + switch (result) { + case BM_CONTENTION_OUTCOME_WITHIN_WINDOW: fw_schedule_bm_work(card, msecs_to_jiffies(125)); return; + case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF: + new_root_id = local_id; + goto pick_me; + case BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY: + new_root_id = local_id; + goto pick_me; + case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION: + // BM work has been rescheduled. + return; + case BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION: + // Let's try again later and hope that the local problem has gone away by + // then. + fw_schedule_bm_work(card, msecs_to_jiffies(125)); + return; + case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM: + // Let's do a bus reset and pick the local node as root, and thus, IRM. + new_root_id = local_id; + goto pick_me; + case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM: + if (local_id == irm_id) { + // Only acts as IRM. + allocate_broadcast_channel(card, generation); + } + fallthrough; + case BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM: + default: + card->bm_generation = generation; + break; } } From 9192c37929ff26eae7ab4e82ddb4af54ed73c6e2 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Fri, 19 Sep 2025 08:54:47 +0900 Subject: [PATCH 38/43] firewire: core; eliminate pick_me goto label This commit uses condition statements instead of pick_me goto label. Link: https://lore.kernel.org/r/20250918235448.129705-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 99 ++++++++++++++++++------------------ 1 file changed, 50 insertions(+), 49 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 6268b595d4fa..58d1f58a4a0f 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -388,6 +388,7 @@ static void bm_work(struct work_struct *work) int root_id, new_root_id, irm_id, local_id; int expected_gap_count, generation; bool do_reset = false; + bool stand_for_root = false; if (card->local_node == NULL) return; @@ -408,11 +409,11 @@ static void bm_work(struct work_struct *work) fw_schedule_bm_work(card, msecs_to_jiffies(125)); return; case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF: - new_root_id = local_id; - goto pick_me; + stand_for_root = true; + break; case BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY: - new_root_id = local_id; - goto pick_me; + stand_for_root = true; + break; case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION: // BM work has been rescheduled. return; @@ -423,8 +424,8 @@ static void bm_work(struct work_struct *work) return; case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM: // Let's do a bus reset and pick the local node as root, and thus, IRM. - new_root_id = local_id; - goto pick_me; + stand_for_root = true; + break; case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM: if (local_id == irm_id) { // Only acts as IRM. @@ -438,56 +439,56 @@ static void bm_work(struct work_struct *work) } } - /* - * We're bus manager for this generation, so next step is to - * make sure we have an active cycle master and do gap count - * optimization. - */ - if (card->gap_count == GAP_COUNT_MISMATCHED) { - /* - * If self IDs have inconsistent gap counts, do a - * bus reset ASAP. The config rom read might never - * complete, so don't wait for it. However, still - * send a PHY configuration packet prior to the - * bus reset. The PHY configuration packet might - * fail, but 1394-2008 8.4.5.2 explicitly permits - * it in this case, so it should be safe to try. - */ - new_root_id = local_id; - /* - * We must always send a bus reset if the gap count - * is inconsistent, so bypass the 5-reset limit. - */ - card->bm_retries = 0; - } else { - // Now investigate root node. - struct fw_device *root_device = fw_node_get_device(root_node); + // We're bus manager for this generation, so next step is to make sure we have an active + // cycle master and do gap count optimization. + if (!stand_for_root) { + if (card->gap_count == GAP_COUNT_MISMATCHED) { + // If self IDs have inconsistent gap counts, do a + // bus reset ASAP. The config rom read might never + // complete, so don't wait for it. However, still + // send a PHY configuration packet prior to the + // bus reset. The PHY configuration packet might + // fail, but 1394-2008 8.4.5.2 explicitly permits + // it in this case, so it should be safe to try. + stand_for_root = true; - if (root_device == NULL) { - // Either link_on is false, or we failed to read the - // config rom. In either case, pick another root. - new_root_id = local_id; + // We must always send a bus reset if the gap count + // is inconsistent, so bypass the 5-reset limit. + card->bm_retries = 0; } else { - bool root_device_is_running = - atomic_read(&root_device->state) == FW_DEVICE_RUNNING; + // Now investigate root node. + struct fw_device *root_device = fw_node_get_device(root_node); - if (!root_device_is_running) { - // If we haven't probed this device yet, bail out now - // and let's try again once that's done. - return; - } else if (root_device->cmc) { - // We will send out a force root packet for this - // node as part of the gap count optimization. - new_root_id = root_id; + if (root_device == NULL) { + // Either link_on is false, or we failed to read the + // config rom. In either case, pick another root. + stand_for_root = true; } else { - // Current root has an active link layer and we - // successfully read the config rom, but it's not - // cycle master capable. - new_root_id = local_id; + bool root_device_is_running = + atomic_read(&root_device->state) == FW_DEVICE_RUNNING; + + if (!root_device_is_running) { + // If we haven't probed this device yet, bail out now + // and let's try again once that's done. + return; + } else if (!root_device->cmc) { + // Current root has an active link layer and we + // successfully read the config rom, but it's not + // cycle master capable. + stand_for_root = true; + } } } } - pick_me: + + if (stand_for_root) { + new_root_id = local_id; + } else { + // We will send out a force root packet for this node as part of the gap count + // optimization on behalf of the node. + new_root_id = root_id; + } + /* * Pick a gap count from 1394a table E-1. The table doesn't cover * the typically much larger 1394b beta repeater delays though. From 19e73f65940d3d3357c637f3d7e19a59305a748f Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Fri, 19 Sep 2025 08:54:48 +0900 Subject: [PATCH 39/43] firewire: core: minor code refactoring to delete useless local variable The do_reset local variable has less merit. Let's remove it. Link: https://lore.kernel.org/r/20250918235448.129705-7-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 58d1f58a4a0f..4a5459696093 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -387,7 +387,6 @@ static void bm_work(struct work_struct *work) struct fw_node *root_node __free(node_unref) = NULL; int root_id, new_root_id, irm_id, local_id; int expected_gap_count, generation; - bool do_reset = false; bool stand_for_root = false; if (card->local_node == NULL) @@ -499,16 +498,10 @@ static void bm_work(struct work_struct *work) else expected_gap_count = 63; - /* - * Finally, figure out if we should do a reset or not. If we have - * done less than 5 resets with the same physical topology and we - * have either a new root or a new gap count setting, let's do it. - */ - - if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id)) - do_reset = true; - - if (do_reset) { + // Finally, figure out if we should do a reset or not. If we have done less than 5 resets + // with the same physical topology and we have either a new root or a new gap count + // setting, let's do it. + if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id)) { int card_gap_count = card->gap_count; fw_notice(card, "phy config: new root=%x, gap_count=%d\n", From 8ec6a8ec23b9529d6203cab50a22fab3a5fd0d80 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 24 Sep 2025 22:11:40 +0900 Subject: [PATCH 40/43] firewire: core: suppress overflow warning when computing jiffies from isochronous cycle The multiplication by USEC_PER_SEC (=1000000L) may trigger an overflow warning with 32 bit storage. In the case of the subsystem the input value ranges between 800 and 16000, thus the result always fits within 32 bit storage. This commit suppresses the warning by using widening conversion to 64 bit storage before multiplication, then using narrowing conversion to 32 bit storage. Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202509170136.b5ZHaNAV-lkp@intel.com/ Fixes: 379b870c28c6 ("firewire: core: use helper macros instead of direct access to HZ") Link: https://lore.kernel.org/r/20250924131140.261686-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index 7f2ca93406ce..2dd715a580ac 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -30,7 +30,7 @@ struct fw_packet; // This is the arbitrary value we use to indicate a mismatched gap count. #define GAP_COUNT_MISMATCHED 0 -#define isoc_cycles_to_jiffies(cycles) usecs_to_jiffies(cycles * USEC_PER_SEC / 8000) +#define isoc_cycles_to_jiffies(cycles) usecs_to_jiffies((u32)((u64)(cycles) * USEC_PER_SEC / 8000)) extern __printf(2, 3) void fw_err(const struct fw_card *card, const char *fmt, ...); From a6176b7b2a025f050740887238a7fbd3916ab6b5 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 24 Sep 2025 22:18:22 +0900 Subject: [PATCH 41/43] Revert "firewire: core: shrink critical section of fw_card spinlock in bm_work" This reverts commit 582310376d6e9a8d261b682178713cdc4b251af6. The bus manager work has the race condition against fw_destroy_nodes() called by fw_core_remove_card(). The acquition of spin lock of fw_card is left as is again. Link: https://lore.kernel.org/r/20250924131823.262136-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 38 ++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 4a5459696093..e5e0174a0335 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -299,6 +299,7 @@ enum bm_contention_outcome { }; static enum bm_contention_outcome contend_for_bm(struct fw_card *card) +__must_hold(&card->lock) { int generation = card->generation; int local_id = card->local_node->node_id; @@ -311,8 +312,11 @@ static enum bm_contention_outcome contend_for_bm(struct fw_card *card) bool keep_this_irm = false; struct fw_node *irm_node; struct fw_device *irm_device; + int irm_node_id; int rcode; + lockdep_assert_held(&card->lock); + if (!grace) { if (!is_next_generation(generation, card->bm_generation) || card->bm_abdicate) return BM_CONTENTION_OUTCOME_WITHIN_WINDOW; @@ -338,10 +342,16 @@ static enum bm_contention_outcome contend_for_bm(struct fw_card *card) return BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY; } - rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, irm_node->node_id, generation, + irm_node_id = irm_node->node_id; + + spin_unlock_irq(&card->lock); + + rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, irm_node_id, generation, SCODE_100, CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, data, sizeof(data)); + spin_lock_irq(&card->lock); + switch (rcode) { case RCODE_GENERATION: return BM_CONTENTION_OUTCOME_AT_NEW_GENERATION; @@ -352,12 +362,10 @@ static enum bm_contention_outcome contend_for_bm(struct fw_card *card) int bm_id = be32_to_cpu(data[0]); // Used by cdev layer for "struct fw_cdev_event_bus_reset". - scoped_guard(spinlock, &card->lock) { - if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) - card->bm_node_id = 0xffc0 & bm_id; - else - card->bm_node_id = local_id; - } + if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) + card->bm_node_id = 0xffc0 & bm_id; + else + card->bm_node_id = local_id; if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) return BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM; @@ -389,8 +397,12 @@ static void bm_work(struct work_struct *work) int expected_gap_count, generation; bool stand_for_root = false; - if (card->local_node == NULL) + spin_lock_irq(&card->lock); + + if (card->local_node == NULL) { + spin_unlock_irq(&card->lock); return; + } generation = card->generation; @@ -405,6 +417,7 @@ static void bm_work(struct work_struct *work) switch (result) { case BM_CONTENTION_OUTCOME_WITHIN_WINDOW: + spin_unlock_irq(&card->lock); fw_schedule_bm_work(card, msecs_to_jiffies(125)); return; case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF: @@ -415,10 +428,12 @@ static void bm_work(struct work_struct *work) break; case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION: // BM work has been rescheduled. + spin_unlock_irq(&card->lock); return; case BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION: // Let's try again later and hope that the local problem has gone away by // then. + spin_unlock_irq(&card->lock); fw_schedule_bm_work(card, msecs_to_jiffies(125)); return; case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM: @@ -428,7 +443,9 @@ static void bm_work(struct work_struct *work) case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM: if (local_id == irm_id) { // Only acts as IRM. + spin_unlock_irq(&card->lock); allocate_broadcast_channel(card, generation); + spin_lock_irq(&card->lock); } fallthrough; case BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM: @@ -469,6 +486,7 @@ static void bm_work(struct work_struct *work) if (!root_device_is_running) { // If we haven't probed this device yet, bail out now // and let's try again once that's done. + spin_unlock_irq(&card->lock); return; } else if (!root_device->cmc) { // Current root has an active link layer and we @@ -504,6 +522,8 @@ static void bm_work(struct work_struct *work) if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id)) { int card_gap_count = card->gap_count; + spin_unlock_irq(&card->lock); + fw_notice(card, "phy config: new root=%x, gap_count=%d\n", new_root_id, expected_gap_count); fw_send_phy_config(card, new_root_id, generation, expected_gap_count); @@ -524,6 +544,8 @@ static void bm_work(struct work_struct *work) } else { struct fw_device *root_device = fw_node_get_device(root_node); + spin_unlock_irq(&card->lock); + if (root_device && root_device->cmc) { // Make sure that the cycle master sends cycle start packets. __be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR); From e216c49b3ebb0729c50870ebfb8b798376dc1edf Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 24 Sep 2025 22:18:23 +0900 Subject: [PATCH 42/43] Revert "firewire: core: disable bus management work temporarily during updating topology" This reverts commit abe7159125702c734e851bc0c52b51cd446298a5. The bus manager work item acquires the spin lock of fw_card again, thus no need to serialize it against fw_core_handle_bus_reset(). Link: https://lore.kernel.org/r/20250924131823.262136-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-topology.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index 90b988035a2a..2f73bcd5696f 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -460,14 +460,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, { struct fw_node *local_node; - might_sleep(); - trace_bus_reset_handle(card->index, generation, node_id, bm_abdicate, self_ids, self_id_count); - // Disable bus management work during updating the cache of bus topology, since the work - // accesses to some members of fw_card. - disable_delayed_work_sync(&card->bm_work); - scoped_guard(spinlock, &card->lock) { // If the selfID buffer is not the immediate successor of the // previously processed one, we cannot reliably compare the @@ -501,8 +495,6 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, } } - enable_delayed_work(&card->bm_work); - fw_schedule_bm_work(card, 0); // Just used by transaction layer. From 40d4c761200b796a44bf2c7675ae09c87b17d4af Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 28 Sep 2025 10:19:10 +0900 Subject: [PATCH 43/43] firewire: core: fix undefined reference error in ARM EABI For ARM EABI, GCC generates a reference to __aeabi_uldivmod when compiling a division of 64-bit integer with 32-bit integer. This function is not available in Linux kernel. In such cases, helper macros are defined in include/linux/math64.h. This commit replaces the division with div_u64(). Fixes: 8ec6a8ec23b9 ("firewire: core: suppress overflow warning when computing jiffies from isochronous cycle") Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202509270428.FZaO2PPq-lkp@intel.com/ Link: https://lore.kernel.org/r/20250928011910.581475-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index 2dd715a580ac..e67395ce26b5 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -30,7 +30,7 @@ struct fw_packet; // This is the arbitrary value we use to indicate a mismatched gap count. #define GAP_COUNT_MISMATCHED 0 -#define isoc_cycles_to_jiffies(cycles) usecs_to_jiffies((u32)((u64)(cycles) * USEC_PER_SEC / 8000)) +#define isoc_cycles_to_jiffies(cycles) usecs_to_jiffies((u32)div_u64((u64)cycles * USEC_PER_SEC, 8000)) extern __printf(2, 3) void fw_err(const struct fw_card *card, const char *fmt, ...);