From efc4506664ea4a713483bfc0b34078982ed7670a Mon Sep 17 00:00:00 2001 From: longlong yan Date: Tue, 18 Aug 2026 10:20:34 +0800 Subject: [PATCH 01/88] tools/firewire: nosy-dump: guard close(fd) when reading from input file The file descriptor fd is initialized to -1 and only opened via open() when the --input option is not used. However, close(fd) is called unconditionally at the end of main(), which results in close(-1) and a spurious EBADF error when nosy-dump reads from an input file. Guard the close() call with "if (fd >= 0)" to match the existing conditional cleanup pattern used for the output file handle. Signed-off-by: longlong yan Signed-off-by: Takashi Sakamoto --- tools/firewire/nosy-dump.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/firewire/nosy-dump.c b/tools/firewire/nosy-dump.c index 9a906de3a9ef..9cc8626a7e94 100644 --- a/tools/firewire/nosy-dump.c +++ b/tools/firewire/nosy-dump.c @@ -1014,7 +1014,8 @@ int main(int argc, const char *argv[]) if (output != NULL) fclose(output); - close(fd); + if (fd >= 0) + close(fd); poptFreeContext(con); From 56b41ee056d09da939fa68dd5c87c577134f62f4 Mon Sep 17 00:00:00 2001 From: Triet Hoang Date: Thu, 20 Aug 2026 15:30:34 +0700 Subject: [PATCH 02/88] firewire: ohci: Convert to DEFINE_SIMPLE_DEV_PM_OPS() Convert the deprecated SIMPLE_DEV_PM_OPS to DEFINE_SIMPLE_DEV_PM_OPS and pm_sleep_ptr(). This lets us drop the __maybe_unused annotations from the suspend and resume callbacks, and reduces kernel size in case CONFIG_PM or CONFIG_PM_SLEEP is disabled. Signed-off-by: Triet Hoang Link: https://lore.kernel.org/r/20260820083034.2087217-1-triet.hoang.dev@gmail.com Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 46dd53ecedc7..b7df280bec51 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3761,7 +3761,7 @@ static void pci_remove(struct pci_dev *dev) dev_notice(&dev->dev, "removing fw-ohci device\n"); } -static int __maybe_unused pci_suspend(struct device *dev) +static int pci_suspend(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct fw_ohci *ohci = pci_get_drvdata(pdev); @@ -3773,7 +3773,7 @@ static int __maybe_unused pci_suspend(struct device *dev) } -static int __maybe_unused pci_resume(struct device *dev) +static int pci_resume(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct fw_ohci *ohci = pci_get_drvdata(pdev); @@ -3804,14 +3804,14 @@ static const struct pci_device_id pci_table[] = { MODULE_DEVICE_TABLE(pci, pci_table); -static SIMPLE_DEV_PM_OPS(pci_pm_ops, pci_suspend, pci_resume); +static DEFINE_SIMPLE_DEV_PM_OPS(pci_pm_ops, pci_suspend, pci_resume); static struct pci_driver fw_ohci_pci_driver = { .name = ohci_driver_name, .id_table = pci_table, .probe = pci_probe, .remove = pci_remove, - .driver.pm = &pci_pm_ops, + .driver.pm = pm_sleep_ptr(&pci_pm_ops), }; static int __init fw_ohci_init(void) From 54cc6aed1458f1f325b4a0f74979ee258fa78ce9 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 31 Aug 2026 19:12:53 +0900 Subject: [PATCH 03/88] firewire: core: fix wrong git hash in code comment When refactoring device quirk code, the wrong git hash from my local repository slipped into the common header. Fixes: 5a43dc9f4ee0 ("firewire: core: detect device quirk when reading configuration ROM") Link: https://lore.kernel.org/r/20260831101253.1810143-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- include/linux/firewire.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/firewire.h b/include/linux/firewire.h index fd35a6570cd8..b41af502bd89 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -172,7 +172,7 @@ struct fw_attribute_group { }; enum fw_device_quirk { - // See afa1282a35d3 ("firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder"). + // See 10389536742c ("firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder"). FW_DEVICE_QUIRK_IRM_IS_1394_1995_ONLY = BIT(0), // See a509e43ff338 ("firewire: core: fix unstable I/O with Canon camcorder"). From 53f839d09acb86f977fca79f61982aae4138d2a7 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 11:50:31 +0900 Subject: [PATCH 04/88] firewire: core: fix wrong git hash in code comment When refactoring device quirk code, the wrong git hash from my local repository slipped into the common header. Fixes: 5a43dc9f4ee0 ("firewire: core: detect device quirk when reading configuration ROM") Link: https://lore.kernel.org/lkml/20260901134921.2386739-1-o-takashi@sakamocchi.jp/ Signed-off-by: Takashi Sakamoto --- include/linux/firewire.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/firewire.h b/include/linux/firewire.h index b41af502bd89..7fa1a3fc8ed4 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -175,7 +175,7 @@ enum fw_device_quirk { // See 10389536742c ("firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder"). FW_DEVICE_QUIRK_IRM_IS_1394_1995_ONLY = BIT(0), - // See a509e43ff338 ("firewire: core: fix unstable I/O with Canon camcorder"). + // See 6044565af458 ("firewire: core: fix unstable I/O with Canon camcorder"). FW_DEVICE_QUIRK_IRM_IGNORES_BUS_MANAGER = BIT(1), // MOTU Audio Express transfers acknowledge packet with 0x10 for pending state. From 428ef9ca38545ccab8dc5c38b0fa62b02090e66d Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:22 +0900 Subject: [PATCH 05/88] firewire: core: add KUnit test skeleton for config ROM parser and generator In IEEE 1394, all devices on the bus are required to provide structured data in response to requests to a specific range of their address space. This structured data is called configuration ROM and is defined in IEEE 1212. The core function of this subsystem contains the generator and parser for the configuration ROM. The generator constructs the configuration ROM for the local host node, while the parser interprets the configuration ROM of detected nodes. Both the generator and parser are fundamental to identifying the node capabilities. Add KUnit test skeletons for testing their implementations. Link: https://lore.kernel.org/r/20260901134534.2386577-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/.kunitconfig | 1 + drivers/firewire/Kconfig | 16 ++++++++++++++++ drivers/firewire/config-rom-generator-test.c | 19 +++++++++++++++++++ drivers/firewire/config-rom-parser-test.c | 19 +++++++++++++++++++ drivers/firewire/core-card.c | 4 ++++ drivers/firewire/core-device.c | 4 ++++ 6 files changed, 63 insertions(+) create mode 100644 drivers/firewire/config-rom-generator-test.c create mode 100644 drivers/firewire/config-rom-parser-test.c diff --git a/drivers/firewire/.kunitconfig b/drivers/firewire/.kunitconfig index 7406acb00478..ee9129081cef 100644 --- a/drivers/firewire/.kunitconfig +++ b/drivers/firewire/.kunitconfig @@ -7,3 +7,4 @@ CONFIG_FIREWIRE_KUNIT_PACKET_SERDES_TEST=y CONFIG_FIREWIRE_KUNIT_SELF_ID_SEQUENCE_HELPER_TEST=y CONFIG_FIREWIRE_KUNIT_OHCI_SERDES_TEST=y CONFIG_FIREWIRE_KUNIT_NODE_TREE_TEST=y +CONFIG_FIREWIRE_KUNIT_CONFIG_ROM_PARSER_AND_GENERATOR_TEST=y diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig index b5abe00accc9..7e1d4b2c440b 100644 --- a/drivers/firewire/Kconfig +++ b/drivers/firewire/Kconfig @@ -96,6 +96,22 @@ config FIREWIRE_KUNIT_NODE_TREE_TEST For more information on KUnit and unit tests in general, refer to the KUnit documentation in Documentation/dev-tools/kunit/. +config FIREWIRE_KUNIT_CONFIG_ROM_PARSER_AND_GENERATOR_TEST + tristate "KUnit tests for config ROM parser and generator" if !KUNIT_ALL_TESTS + depends on FIREWIRE && KUNIT + default KUNIT_ALL_TESTS + help + This builds the KUnit tests to check parser and generator for + configuration ROM content defined in IEEE 1212. + + KUnit tests run during boot and output the results to the debug + log in TAP format (https://testanything.org/). Only useful for + kernel devs running KUnit test harness and are not for inclusion + into a production build. + + For more information on KUnit and unit tests in general, refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + config FIREWIRE_OHCI tristate "OHCI-1394 controllers" depends on PCI && FIREWIRE diff --git a/drivers/firewire/config-rom-generator-test.c b/drivers/firewire/config-rom-generator-test.c new file mode 100644 index 000000000000..9a901eef7b0a --- /dev/null +++ b/drivers/firewire/config-rom-generator-test.c @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// config-rom-generator-test.c - An application of Kunit to test configuration ROM generator. +// +// Copyright (c) 2026 Takashi Sakamoto +// +// This file can not be built independently since it is intentionally included in core-card.c. + +#include + +static struct kunit_case config_rom_generator_test_cases[] = { + {} +}; + +static struct kunit_suite config_rom_generator_test_suite = { + .name = "firewire-config-rom-generator", + .test_cases = config_rom_generator_test_cases, +}; +kunit_test_suite(config_rom_generator_test_suite); diff --git a/drivers/firewire/config-rom-parser-test.c b/drivers/firewire/config-rom-parser-test.c new file mode 100644 index 000000000000..632f24e68692 --- /dev/null +++ b/drivers/firewire/config-rom-parser-test.c @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// config-rom-parser-test.c - An application of Kunit to test configuration ROM parser. +// +// Copyright (c) 2026 Takashi Sakamoto +// +// This file can not be built independently since it is intentionally included in core-device.c. + +#include + +static struct kunit_case config_rom_parser_test_cases[] = { + {} +}; + +static struct kunit_suite config_rom_parser_test_suite = { + .name = "firewire-config-rom-parser", + .test_cases = config_rom_parser_test_cases, +}; +kunit_test_suite(config_rom_parser_test_suite); diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 94992791f02e..23749434d900 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -851,3 +851,7 @@ int fw_card_read_cycle_time(struct fw_card *card, u32 *cycle_time) return 0; } EXPORT_SYMBOL_GPL(fw_card_read_cycle_time); + +#ifdef CONFIG_FIREWIRE_KUNIT_CONFIG_ROM_PARSER_AND_GENERATOR_TEST +#include "config-rom-generator-test.c" +#endif diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index cbac66916240..4e79c57263ff 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -1442,3 +1442,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event) #ifdef CONFIG_FIREWIRE_KUNIT_DEVICE_ATTRIBUTE_TEST #include "device-attribute-test.c" #endif + +#ifdef CONFIG_FIREWIRE_KUNIT_CONFIG_ROM_PARSER_AND_GENERATOR_TEST +#include "config-rom-parser-test.c" +#endif From bae885b9bee546cd260736cd9e4e282c74e9baf7 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:23 +0900 Subject: [PATCH 06/88] firewire: core: add test to detect irm-is-1394-1995-only quirk in config ROM parser Some Sony DV cameras have a quirk where their IRM functionality does not comply with IEEE 1394a:2000. This quirk was supported by commit 10389536742c ("firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder") and refactored by commit 5a43dc9f4ee0 ("firewire: core: detect device quirk when reading configuration ROM"). Add a KUnit test to check detection of the quirk. The configuration ROM content is retrieved from the following discussion: https://github.com/systemd/systemd/issues/25029. Link: https://lore.kernel.org/r/20260901134534.2386577-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-parser-test.c | 151 ++++++++++++++++++++++ drivers/firewire/core-transaction.c | 4 + 2 files changed, 155 insertions(+) diff --git a/drivers/firewire/config-rom-parser-test.c b/drivers/firewire/config-rom-parser-test.c index 632f24e68692..131c7aa3b224 100644 --- a/drivers/firewire/config-rom-parser-test.c +++ b/drivers/firewire/config-rom-parser-test.c @@ -7,13 +7,164 @@ // This file can not be built independently since it is intentionally included in core-device.c. #include +#include +#include + +static const u32 sony_dcr_trv310k_config_rom[] = { + 0x0404e552, + 0x31333934, + 0xe0648100, + 0x00008500, + 0x005eb597, + 0x0007cdd0, + 0x03000085, + 0x8100000d, + 0x17000002, + 0x81000010, + 0x0c0083c0, + 0xd8000002, + 0xd1000003, + 0x0001ce96, + 0xd1000001, + 0x0004bbee, + 0x1200a02d, + 0x13010001, + 0x17000002, + 0x81000006, + 0x00046dc8, + 0x00000000, + 0x00000000, + 0x43616e6f, + 0x6e000000, + 0x000621ee, + 0x00000000, + 0x00000000, + 0x4d563569, + 0x204d4300, + 0x00000000, + 0x00000000, +}; + +static const struct parser_test_case { + const char *name; + const u32 *quadlets; + size_t quadlet_length; + int phy_speed_in_self_id; + int expected_speed; + int expected_quirk; + unsigned int expected_max_rec; + bool expected_cmc; + bool expected_irmc; +} parser_test_cases[] = { + { + .name = "detect_irm_is_1394_1995_only_quirk", + .quadlets = sony_dcr_trv310k_config_rom, + .quadlet_length = ARRAY_SIZE(sony_dcr_trv310k_config_rom), + .phy_speed_in_self_id = SCODE_100, + .expected_speed = SCODE_100, + .expected_quirk = FW_DEVICE_QUIRK_IRM_IS_1394_1995_ONLY, + .expected_max_rec = 8, + .expected_cmc = true, + .expected_irmc = true, + }, +}; + +// Define parser_test_gen_params. +KUNIT_ARRAY_PARAM_DESC(parser_test, parser_test_cases, name); + +static int stub_run_regular_transaction(struct fw_card *card, int tcode, int destination_id, + int generation, int speed, unsigned long long offset, + void *payload, size_t length) +{ + struct kunit *test = kunit_get_current_test(); + const struct parser_test_case *param = test->param_value; + + KUNIT_ASSERT_GE(test, offset, CSR_REGISTER_BASE | CSR_CONFIG_ROM); + KUNIT_ASSERT_LT(test, offset, CSR_REGISTER_BASE | CSR_CONFIG_ROM_END); + KUNIT_ASSERT_NOT_NULL(test, payload); + KUNIT_ASSERT_EQ(test, length, 4); + + unsigned int index = (offset - (CSR_REGISTER_BASE | CSR_CONFIG_ROM)) / sizeof(u32); + u32 *quadlet = payload; + + KUNIT_EXPECT_LE(test, speed, param->expected_speed); + KUNIT_EXPECT_LT(test, index, param->quadlet_length); + + *quadlet = cpu_to_be32(param->quadlets[index]); + + return RCODE_COMPLETE; +} + +static void test_parser_with_regular_cases(struct kunit *test) +{ + struct fw_device *device = test->priv; + const struct parser_test_case *param = test->param_value; + + kunit_activate_static_stub(test, fw_run_transaction, stub_run_regular_transaction); + + device->card->link_speed = SCODE_BETA; + device->node->max_speed = param->phy_speed_in_self_id; + + KUNIT_EXPECT_EQ(test, read_config_rom(device, 0), RCODE_COMPLETE); + + KUNIT_EXPECT_EQ(test, device->config_rom_length, param->quadlet_length); + KUNIT_EXPECT_MEMEQ(test, device->config_rom, param->quadlets, param->quadlet_length); + + KUNIT_EXPECT_TRUE(test, device->quirks & param->expected_quirk); + KUNIT_EXPECT_EQ(test, device->max_speed, param->expected_speed); + KUNIT_EXPECT_EQ(test, (unsigned int)device->max_rec, param->expected_max_rec); + KUNIT_EXPECT_EQ(test, (bool)device->cmc, param->expected_cmc); + KUNIT_EXPECT_EQ(test, (bool)device->irmc, param->expected_irmc); + + kunit_deactivate_static_stub(test, fw_run_transaction); +} + +static const struct fw_card_driver dummy_card_driver; + +static int config_rom_parser_test_init(struct kunit *test) +{ + struct fw_device *device; + struct device *dev; + + device = kunit_kzalloc(test, sizeof(*device), GFP_KERNEL); + KUNIT_ASSERT_NOT_NULL(test, device); + + device->node = kunit_kzalloc(test, sizeof(*device->node), GFP_KERNEL); + KUNIT_ASSERT_NOT_NULL(test, device->node); + kref_init(&device->node->kref); + + device->card = kunit_kzalloc(test, sizeof(*device->card), GFP_KERNEL); + KUNIT_ASSERT_NOT_NULL(test, device->card); + + dev = kunit_device_register(test, "dummy-device"); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); + + fw_card_initialize(device->card, &dummy_card_driver, dev); + + test->priv = device; + + return 0; +} + +static void config_rom_parser_test_exit(struct kunit *test) +{ + struct fw_device *device = test->priv; + + kunit_device_unregister(test, device->card->device); + kunit_kfree(test, device->card); + kunit_kfree(test, device->node); + kunit_kfree(test, device); +} static struct kunit_case config_rom_parser_test_cases[] = { + KUNIT_CASE_PARAM(test_parser_with_regular_cases, parser_test_gen_params), {} }; static struct kunit_suite config_rom_parser_test_suite = { .name = "firewire-config-rom-parser", + .init = config_rom_parser_test_init, + .exit = config_rom_parser_test_exit, .test_cases = config_rom_parser_test_cases, }; kunit_test_suite(config_rom_parser_test_suite); diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 016070560c74..2d64ab5b0ab0 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -24,6 +24,7 @@ #include #include #include +#include #include @@ -485,6 +486,9 @@ int fw_run_transaction(struct fw_card *card, int tcode, int destination_id, int generation, int speed, unsigned long long offset, void *payload, size_t length) { + KUNIT_STATIC_STUB_REDIRECT(fw_run_transaction, card, tcode, destination_id, generation, + speed, offset, payload, length); + struct transaction_callback_data d; struct fw_transaction t; From 86ae631356655c5c73107468f25cb1adf659b9e6 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:24 +0900 Subject: [PATCH 07/88] firewire: core: add test to detect irm-ignores-bus-manager quirk in config ROM parser Some Canon DV cameras have a quirk where they malfunction as non-root nodes by ignoring the bus manager. This quirk was supported by commit 6044565af458e ("firewire: core: fix unstable I/O with Canon camcorder") and refactored by commit 5a43dc9f4ee0 ("firewire: core: detect device quirk when reading configuration ROM"). Add a KUnit test to verify that the configuration ROM parser detects the quirk correctly. The configuration ROM content is retrieved from the following discussion: https://bugzilla.redhat.com/show_bug.cgi?id=633260 Link: https://lore.kernel.org/r/20260901134534.2386577-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-parser-test.c | 46 +++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/drivers/firewire/config-rom-parser-test.c b/drivers/firewire/config-rom-parser-test.c index 131c7aa3b224..11c01ef9efd9 100644 --- a/drivers/firewire/config-rom-parser-test.c +++ b/drivers/firewire/config-rom-parser-test.c @@ -45,6 +45,41 @@ static const u32 sony_dcr_trv310k_config_rom[] = { 0x00000000, }; +static const u32 canon_mv5i_mc_config_rom[] = { + 0x0404e552, + 0x31333934, + 0xe0648100, + 0x00008500, + 0x005eb597, + 0x0007cdd0, + 0x03000085, + 0x8100000d, + 0x17000002, + 0x81000010, + 0x0c0083c0, + 0xd8000002, + 0xd1000003, + 0x0001ce96, + 0xd1000001, + 0x0004bbee, + 0x1200a02d, + 0x13010001, + 0x17000002, + 0x81000006, + 0x00046dc8, + 0x00000000, + 0x00000000, + 0x43616e6f, + 0x6e000000, + 0x000621ee, + 0x00000000, + 0x00000000, + 0x4d563569, + 0x204d4300, + 0x00000000, + 0x00000000, +}; + static const struct parser_test_case { const char *name; const u32 *quadlets; @@ -67,6 +102,17 @@ static const struct parser_test_case { .expected_cmc = true, .expected_irmc = true, }, + { + .name = "detect_irm_ignores_bus_manager_quirk", + .quadlets = canon_mv5i_mc_config_rom, + .quadlet_length = ARRAY_SIZE(canon_mv5i_mc_config_rom), + .phy_speed_in_self_id = SCODE_100, + .expected_speed = SCODE_100, + .expected_quirk = FW_DEVICE_QUIRK_IRM_IGNORES_BUS_MANAGER, + .expected_max_rec = 8, + .expected_cmc = true, + .expected_irmc = true, + }, }; // Define parser_test_gen_params. From 502e6d9c496c39fa6f709cd67d8fe115dd97b94c Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:25 +0900 Subject: [PATCH 08/88] firewire: core: add test to detect ack-packet-with-invalid-pending-code quirk in config ROM parser MOTU Audio Express has a quirk where it transfers an invalid pending code when receiving an asynchronous request. This quirk was originally reported by commit 3a93d082bacf ("ALSA: firewire-motu: add support for MOTU Audio Express") and later supported by commit 15f9610fc96a ("firewire: core: handle device quirk of MOTU Audio Express"). Add a KUnit test to verify that the configuration ROM parser detects the quirk correctly. The configuration ROM content is retrieved from the commit mentioned above. Link: https://lore.kernel.org/r/20260901134534.2386577-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-parser-test.c | 31 +++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/drivers/firewire/config-rom-parser-test.c b/drivers/firewire/config-rom-parser-test.c index 11c01ef9efd9..35ebe8a6b611 100644 --- a/drivers/firewire/config-rom-parser-test.c +++ b/drivers/firewire/config-rom-parser-test.c @@ -80,6 +80,26 @@ static const u32 canon_mv5i_mc_config_rom[] = { 0x00000000, }; +static const u32 motu_audioexpress_config_rom[] = { + 0x0410a756, + 0x31333934, + 0x20ff7000, + 0x0001f200, + 0x000a8a7b, + 0x0004ef04, + 0x030001f2, + 0x0c0083c0, + 0xd1000002, + 0x8d000005, + 0x00031680, + 0x120001f2, + 0x13000033, + 0x17104800, + 0x00025ef3, + 0x0001f200, + 0x000a8a7b, +}; + static const struct parser_test_case { const char *name; const u32 *quadlets; @@ -113,6 +133,17 @@ static const struct parser_test_case { .expected_cmc = true, .expected_irmc = true, }, + { + .name = "detect_ack_packet_with_invalid_pending_code_quirk", + .quadlets = motu_audioexpress_config_rom, + .quadlet_length = ARRAY_SIZE(motu_audioexpress_config_rom), + .phy_speed_in_self_id = SCODE_400, + .expected_speed = SCODE_400, + .expected_quirk = FW_DEVICE_QUIRK_ACK_PACKET_WITH_INVALID_PENDING_CODE, + .expected_max_rec = 7, + .expected_cmc = false, + .expected_irmc = false, + }, }; // Define parser_test_gen_params. From 7b9011c209e85cd1593dc0a21be844bfbdced3bb Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:26 +0900 Subject: [PATCH 09/88] firewire: core: add test to detect unstable-at-s400 quirk in config ROM parser TASCAM FireWire series devices have a quirk where they fail to respond to asynchronous transactions at S400 speed. This quirk was supported by commit d52bb3daad3f ("firewire: core: handle device quirk of TASCAM FW-1884/FW-1804/FW-1082"). Add a KUnit test to verify that the configuration ROM parser detects the quirk correctly. The configuration ROM content is retrieved from the author's collection. Link: https://lore.kernel.org/r/20260901134534.2386577-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-parser-test.c | 44 +++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/drivers/firewire/config-rom-parser-test.c b/drivers/firewire/config-rom-parser-test.c index 35ebe8a6b611..d7c58c7fb995 100644 --- a/drivers/firewire/config-rom-parser-test.c +++ b/drivers/firewire/config-rom-parser-test.c @@ -100,6 +100,39 @@ static const u32 motu_audioexpress_config_rom[] = { 0x000a8a7b, }; +static const u32 tascam_fw1884_config_rom[] = { + 0x040f23c0, + 0x31333934, + 0x20ff7002, + 0x00022eff, + 0xfe800000, + 0x0004bccc, + 0x0300022e, + 0x0c0083c0, + 0x8d000006, + 0xd1000001, + 0x000347f5, + 0x1200022e, + 0x13800000, + 0xd4000004, + 0x000289aa, + 0x00022eff, + 0xfe800000, + 0x0002ae47, + 0x81000002, + 0x82000006, + 0x0004a79e, + 0x00000000, + 0x00000000, + 0x54415343, + 0x414d0000, + 0x00045443, + 0x00000000, + 0x00000000, + 0x46572d31, + 0x38383400, +}; + static const struct parser_test_case { const char *name; const u32 *quadlets; @@ -144,6 +177,17 @@ static const struct parser_test_case { .expected_cmc = false, .expected_irmc = false, }, + { + .name = "detect_unstable_at_s400_quirk", + .quadlets = tascam_fw1884_config_rom, + .quadlet_length = ARRAY_SIZE(tascam_fw1884_config_rom), + .phy_speed_in_self_id = SCODE_400, + .expected_speed = SCODE_200, + .expected_quirk = FW_DEVICE_QUIRK_UNSTABLE_AT_S400, + .expected_max_rec = 7, + .expected_cmc = false, + .expected_irmc = false, + }, }; // Define parser_test_gen_params. From 1c4326e7d8305389c5ee7d15127b33da2f3ec98a Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:27 +0900 Subject: [PATCH 10/88] firewire: core: add test to avoid excessive configuration ROM length The configuration ROM parser needs to handle malformed ROM contents safely. In particular, malformed directory and leaf offsets and block lengths should not result in accesses beyond the available configuration ROM data. Add a KUnit test with malformed configuration ROM content to verify that the parser sanitizes entries that would cause accesses beyond the available configuration ROM data. Reported-by: Aleksandr Shabelnikov Link: https://lore.kernel.org/lkml/20250901171547.47065-1-mistermidi@gmail.com/ Link: https://lore.kernel.org/r/20260901134534.2386577-7-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-parser-test.c | 68 ++++++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/config-rom-parser-test.c b/drivers/firewire/config-rom-parser-test.c index d7c58c7fb995..e5c173353057 100644 --- a/drivers/firewire/config-rom-parser-test.c +++ b/drivers/firewire/config-rom-parser-test.c @@ -193,7 +193,7 @@ static const struct parser_test_case { // Define parser_test_gen_params. KUNIT_ARRAY_PARAM_DESC(parser_test, parser_test_cases, name); -static int stub_run_regular_transaction(struct fw_card *card, int tcode, int destination_id, +static int stub_run_transaction_regular(struct fw_card *card, int tcode, int destination_id, int generation, int speed, unsigned long long offset, void *payload, size_t length) { @@ -221,7 +221,7 @@ static void test_parser_with_regular_cases(struct kunit *test) struct fw_device *device = test->priv; const struct parser_test_case *param = test->param_value; - kunit_activate_static_stub(test, fw_run_transaction, stub_run_regular_transaction); + kunit_activate_static_stub(test, fw_run_transaction, stub_run_transaction_regular); device->card->link_speed = SCODE_BETA; device->node->max_speed = param->phy_speed_in_self_id; @@ -240,6 +240,69 @@ static void test_parser_with_regular_cases(struct kunit *test) kunit_deactivate_static_stub(test, fw_run_transaction); } +static int stub_run_transaction_malformed(struct fw_card *card, int tcode, int destination_id, + int generation, int speed, unsigned long long offset, + void *payload, size_t length) +{ + static const u32 config_rom_first_part[] = { + 0x04000000, + 0x31333934, + 0x00008002, + 0x00000000, + 0x00000000, + 0x00030000, + (CSR_VENDOR << 24) | 0x00123456, // Regular entry. + ((CSR_LEAF | CSR_DESCRIPTOR) << 24) | 0x000000f9, // Beyond the upper limit. + ((CSR_DIRECTORY | CSR_UNIT) << 24) | 0x00000001, + 0xffff0000, // Over the upper limit. + }; + struct kunit *test = kunit_get_current_test(); + + KUNIT_ASSERT_GE(test, offset, CSR_REGISTER_BASE | CSR_CONFIG_ROM); + KUNIT_ASSERT_LT(test, offset, CSR_REGISTER_BASE | CSR_CONFIG_ROM_END); + KUNIT_ASSERT_NOT_NULL(test, payload); + KUNIT_ASSERT_EQ(test, length, 4); + + unsigned int index = (offset - (CSR_REGISTER_BASE | CSR_CONFIG_ROM)) / sizeof(u32); + u32 *quadlet = payload; + + if (index < ARRAY_SIZE(config_rom_first_part)) + *quadlet = cpu_to_be32(config_rom_first_part[index]); + else + *quadlet = 0; + + return RCODE_COMPLETE; +} + +static void test_parser_with_overflowed_case(struct kunit *test) +{ + static const u32 corrected_config_rom[] = { + 0x04000000, + 0x31333934, + 0x00008002, + 0x00000000, + 0x00000000, + 0x00030000, + 0x03123456, + 0x00000000, // Sanitized. + 0xd1000001, + 0x00000000, // Sanitized. + }; + struct fw_device *device = test->priv; + + kunit_activate_static_stub(test, fw_run_transaction, stub_run_transaction_malformed); + + device->card->link_speed = SCODE_BETA; + + KUNIT_EXPECT_EQ(test, read_config_rom(device, 0), RCODE_COMPLETE); + + KUNIT_EXPECT_EQ(test, device->config_rom_length, ARRAY_SIZE(corrected_config_rom)); + KUNIT_EXPECT_MEMEQ(test, device->config_rom, corrected_config_rom, + sizeof(corrected_config_rom)); + + kunit_deactivate_static_stub(test, fw_run_transaction); +} + static const struct fw_card_driver dummy_card_driver; static int config_rom_parser_test_init(struct kunit *test) @@ -279,6 +342,7 @@ static void config_rom_parser_test_exit(struct kunit *test) static struct kunit_case config_rom_parser_test_cases[] = { KUNIT_CASE_PARAM(test_parser_with_regular_cases, parser_test_gen_params), + KUNIT_CASE(test_parser_with_overflowed_case), {} }; From bea8f9df4254ef4a254d663dc7229367c0fd4b46 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:28 +0900 Subject: [PATCH 11/88] firewire: core: add test for root directory generation in config ROM generator The core function allows both the unit drivers and the userspace applications to extend the system configuration ROM. Without any extensions, the generator produces a root directory accompanied by two descriptor leaves. Add a KUnit test to verify the configuration ROM generated in this case. Link: https://lore.kernel.org/r/20260901134534.2386577-8-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-generator-test.c | 100 +++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/drivers/firewire/config-rom-generator-test.c b/drivers/firewire/config-rom-generator-test.c index 9a901eef7b0a..02477300c1df 100644 --- a/drivers/firewire/config-rom-generator-test.c +++ b/drivers/firewire/config-rom-generator-test.c @@ -7,13 +7,113 @@ // This file can not be built independently since it is intentionally included in core-card.c. #include +#include + +static const u32 config_rom_bare[] = { + cpu_to_be32(0x0404921b), // bus info + cpu_to_be32(0x31333934), // | + cpu_to_be32(0xf000b223), // | + cpu_to_be32(0x01234567), // | + cpu_to_be32(0x89abcdef), // v + cpu_to_be32(0x00051b70), // root directory + cpu_to_be32(0x0c0083c0), // | + cpu_to_be32(0x03001f11), // | + cpu_to_be32(0x81000003), // | + cpu_to_be32(0x17023901), // | + cpu_to_be32(0x81000008), // v + cpu_to_be32(0x00064cb7), // text descriptor leaf (from root) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x4c696e75), // | + cpu_to_be32(0x78204669), // | + cpu_to_be32(0x72657769), // | + cpu_to_be32(0x72650000), // v + cpu_to_be32(0x0003ff1c), // text descriptor leaf (from root) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x4a756a75), // v Juju is a code name at the development time of this stack. +}; + +static const struct generator_test_case { + const char *const name; + int config_rom_generation; + const __be32 *const expected; + unsigned int quadlet_length; +} generator_test_cases[] = { + { + .name = "bare", + .config_rom_generation = 0, + .expected = config_rom_bare, + .quadlet_length = ARRAY_SIZE(config_rom_bare), + }, +}; + +// Define generator_test_gen_params. +KUNIT_ARRAY_PARAM_DESC(generator_test, generator_test_cases, name); + +struct state_data { + struct fw_card card; + __be32 config_rom[(CSR_CONFIG_ROM_END - CSR_CONFIG_ROM) / sizeof(__be32)]; +}; + +static void test_config_rom_generator(struct kunit *test) +{ + const struct generator_test_case *test_case = test->param_value; + struct state_data *state = test->priv; + struct fw_card *card = &state->card; + __be32 *config_rom = state->config_rom; + + card->config_rom_generation = test_case->config_rom_generation; + card->link_speed = SCODE_800; + card->max_receive = 11; + card->guid = 0x0123456789abcdefULL; + + scoped_guard(mutex, &card_mutex) { + generate_config_rom(card, config_rom); + KUNIT_EXPECT_EQ(test, config_rom_length, test_case->quadlet_length); + } + + KUNIT_EXPECT_MEMEQ(test, config_rom, test_case->expected, + sizeof(*test_case->expected) * test_case->quadlet_length); +} + +static const struct fw_card_driver dummy_card_driver; + +static int config_rom_generator_test_init(struct kunit *test) +{ + struct state_data *state = kunit_kzalloc(test, sizeof(*state), GFP_KERNEL); + + KUNIT_ASSERT_NOT_NULL(test, state); + + struct device *dev = kunit_device_register(test, "dummy-device"); + + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); + + fw_card_initialize(&state->card, &dummy_card_driver, dev); + + test->priv = state; + + return 0; +} + +static void config_rom_generator_test_exit(struct kunit *test) +{ + struct state_data *state = test->priv; + + kunit_device_unregister(test, state->card.device); + kunit_kfree(test, state); +} + static struct kunit_case config_rom_generator_test_cases[] = { + KUNIT_CASE_PARAM(test_config_rom_generator, generator_test_gen_params), {} }; static struct kunit_suite config_rom_generator_test_suite = { .name = "firewire-config-rom-generator", + .init = config_rom_generator_test_init, + .exit = config_rom_generator_test_exit, .test_cases = config_rom_generator_test_cases, }; kunit_test_suite(config_rom_generator_test_suite); From c0c5987f4bddd2b54ac56286d1f33307805710dd Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:29 +0900 Subject: [PATCH 12/88] firewire: core: add test to generate with AV/C unit in config ROM generator There is a standard layout of configuration ROM for AV/C devices. Add a KUnit test to verify that the generator produces the expected configuration ROM when an AV/C unit is registered. Link: https://lore.kernel.org/r/20260901134534.2386577-9-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-generator-test.c | 71 ++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/drivers/firewire/config-rom-generator-test.c b/drivers/firewire/config-rom-generator-test.c index 02477300c1df..c695811a50bb 100644 --- a/drivers/firewire/config-rom-generator-test.c +++ b/drivers/firewire/config-rom-generator-test.c @@ -34,6 +34,59 @@ static const u32 config_rom_bare[] = { cpu_to_be32(0x4a756a75), // v Juju is a code name at the development time of this stack. }; +// Following to Configuration ROM for AV/C Devices 1.0 (Dec. 2000. 1394 Trading Association, +// Document 1999027). +#define UNIT_SPEC_ID_1394TA 0x0000a02d +#define UNIT_SW_VERSION_AVC 0x00010001 +#define UNIT_SW_VERSION_IIDC_0104 0x00000100 + +static const u32 avc_unit_directory_and_leaf[] = { + 0x00040000, // Unit directory consists of below 4 quads. + (CSR_SPECIFIER_ID << 24) | UNIT_SPEC_ID_1394TA, + (CSR_VERSION << 24) | UNIT_SW_VERSION_AVC, + (CSR_MODEL << 24) | 0x00260827, // Today. + ((CSR_LEAF | CSR_DESCRIPTOR) << 24) | 0x00000001, // Point to next quadlet. + 0x00030000, // Text leaf consists of below 3 quads. + 0x00000000, + 0x00000000, + 0x50756900, // Pui is the name of a cat that the author takes care of. +}; + +static const u32 config_rom_with_avc_unit[] = { + cpu_to_be32(0x0404c1e5), // bus info + cpu_to_be32(0x31333934), // | + cpu_to_be32(0xf000b233), // | + cpu_to_be32(0x01234567), // | + cpu_to_be32(0x89abcdef), // v + cpu_to_be32(0x0006a2d2), // root directory + cpu_to_be32(0x0c0083c0), // | + cpu_to_be32(0x03001f11), // | + cpu_to_be32(0x81000004), // | + cpu_to_be32(0x17023901), // | + cpu_to_be32(0x81000009), // | + cpu_to_be32(0xd100000c), // v + cpu_to_be32(0x00064cb7), // text descriptor leaf (from root) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x4c696e75), // | + cpu_to_be32(0x78204669), // | + cpu_to_be32(0x72657769), // | + cpu_to_be32(0x72650000), // v + cpu_to_be32(0x0003ff1c), // text descriptor leaf (from root) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x4a756a75), // v + cpu_to_be32(0x0004227b), // unit directory (from root) + cpu_to_be32(0x1200a02d), // | + cpu_to_be32(0x13010001), // | + cpu_to_be32(0x17260827), // | + cpu_to_be32(0x81000001), // v + cpu_to_be32(0x0003f771), // text descriptor leaf (from unit) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x50756900), // v +}; + static const struct generator_test_case { const char *const name; int config_rom_generation; @@ -46,6 +99,12 @@ static const struct generator_test_case { .expected = config_rom_bare, .quadlet_length = ARRAY_SIZE(config_rom_bare), }, + { + .name = "with_avc_unit", + .config_rom_generation = 1, + .expected = config_rom_with_avc_unit, + .quadlet_length = ARRAY_SIZE(config_rom_with_avc_unit), + }, }; // Define generator_test_gen_params. @@ -58,6 +117,12 @@ struct state_data { static void test_config_rom_generator(struct kunit *test) { + // Use kernel stack since they should be mutable for doubly linked-list. + struct fw_descriptor avc_unit_entry = { + .length = ARRAY_SIZE(avc_unit_directory_and_leaf), + .key = (CSR_DIRECTORY | CSR_UNIT) << 24, + .data = avc_unit_directory_and_leaf, + }; const struct generator_test_case *test_case = test->param_value; struct state_data *state = test->priv; struct fw_card *card = &state->card; @@ -68,6 +133,9 @@ static void test_config_rom_generator(struct kunit *test) card->max_receive = 11; card->guid = 0x0123456789abcdefULL; + if (test_case->expected == config_rom_with_avc_unit) + KUNIT_EXPECT_EQ(test, fw_core_add_descriptor(&avc_unit_entry), 0); + scoped_guard(mutex, &card_mutex) { generate_config_rom(card, config_rom); KUNIT_EXPECT_EQ(test, config_rom_length, test_case->quadlet_length); @@ -75,6 +143,9 @@ static void test_config_rom_generator(struct kunit *test) KUNIT_EXPECT_MEMEQ(test, config_rom, test_case->expected, sizeof(*test_case->expected) * test_case->quadlet_length); + + if (test_case->expected == config_rom_with_avc_unit) + fw_core_remove_descriptor(&avc_unit_entry); } static const struct fw_card_driver dummy_card_driver; From 49e4d0eea6cd0f3c615223af0a3b04fd55db5655 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:30 +0900 Subject: [PATCH 13/88] firewire: core: add test to generate with IIDC unit in config ROM generator There is a standard layout of configuration ROM for IIDC devices. Add a KUnit test to verify that the generator produces the expected configuration ROM when an IIDC unit is registered. Link: https://lore.kernel.org/r/20260901134534.2386577-10-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-generator-test.c | 92 ++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/drivers/firewire/config-rom-generator-test.c b/drivers/firewire/config-rom-generator-test.c index c695811a50bb..6ca5c16b4f0f 100644 --- a/drivers/firewire/config-rom-generator-test.c +++ b/drivers/firewire/config-rom-generator-test.c @@ -87,6 +87,81 @@ static const u32 config_rom_with_avc_unit[] = { cpu_to_be32(0x50756900), // v }; +// Following to 1394-based Digital Camera Specification Version 1.04 (Aug. 1996. 1394 Trading +// Association) +#define IIDC_COMMAND_REGS_BASE 0x00 +#define IIDC_VENDOR_NAME_LEAF 0x01 +#define IIDC_MODEL_NAME_LEAF 0x02 + +static const u32 iidc_unit_directories_and_leafs[] = { + 0x00030000, + (CSR_SPECIFIER_ID << 24) | UNIT_SPEC_ID_1394TA, + (CSR_VERSION << 24) | UNIT_SW_VERSION_IIDC_0104, + ((CSR_DIRECTORY | CSR_DEPENDENT_INFO) << 24) | 0x00000001, + 0x00030000, + ((CSR_OFFSET | IIDC_COMMAND_REGS_BASE) << 24) | 0x00012345, + ((CSR_LEAF | IIDC_VENDOR_NAME_LEAF) << 24) | 0x00000002, + ((CSR_LEAF | IIDC_MODEL_NAME_LEAF) << 24) | 0x00000008, + 0x00060000, // Text leaf consists of below 6 quads. + 0x00000000, + 0x00000000, + 0x4c696e75, // Use the same name in root directory. + 0x78204669, + 0x72657769, + 0x72650000, + 0x00040000, // Text leaf consists of below 4 quads. + 0x00000000, + 0x00000000, + 0x43686566, // Chef Cat is a nick name when inventing IEEE 1394 itself. + 0x20436174, +}; + +static const u32 config_rom_with_iidc_unit[] = { + cpu_to_be32(0x04046a3e), // bus info + cpu_to_be32(0x31333934), // | + cpu_to_be32(0xf000b243), // | + cpu_to_be32(0x01234567), // | + cpu_to_be32(0x89abcdef), // v + cpu_to_be32(0x0006a2d2), // root directory + cpu_to_be32(0x0c0083c0), // | + cpu_to_be32(0x03001f11), // | + cpu_to_be32(0x81000004), // | + cpu_to_be32(0x17023901), // | + cpu_to_be32(0x81000009), // | + cpu_to_be32(0xd100000c), // v + cpu_to_be32(0x00064cb7), // text descriptor leaf (from root) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x4c696e75), // | + cpu_to_be32(0x78204669), // | + cpu_to_be32(0x72657769), // | + cpu_to_be32(0x72650000), // v + cpu_to_be32(0x0003ff1c), // text descriptor leaf (from root) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x4a756a75), // v + cpu_to_be32(0x0003d7fe), // unit directory (from root) + cpu_to_be32(0x1200a02d), // | + cpu_to_be32(0x13000100), // | + cpu_to_be32(0xd4000001), // v + cpu_to_be32(0x0003ea57), // dependent directory (from unit directory) + cpu_to_be32(0x40012345), // | + cpu_to_be32(0x81000002), // | + cpu_to_be32(0x82000008), // v + cpu_to_be32(0x00064cb7), // text descriptor leaf (from dependent directory) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x4c696e75), // | + cpu_to_be32(0x78204669), // | + cpu_to_be32(0x72657769), // | + cpu_to_be32(0x72650000), // v + cpu_to_be32(0x0004a3e9), // text descriptor leaf (from dependent directory) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x43686566), // | + cpu_to_be32(0x20436174), // v +}; + static const struct generator_test_case { const char *const name; int config_rom_generation; @@ -105,6 +180,12 @@ static const struct generator_test_case { .expected = config_rom_with_avc_unit, .quadlet_length = ARRAY_SIZE(config_rom_with_avc_unit), }, + { + .name = "with_iidc_unit", + .config_rom_generation = 2, + .expected = config_rom_with_iidc_unit, + .quadlet_length = ARRAY_SIZE(config_rom_with_iidc_unit), + }, }; // Define generator_test_gen_params. @@ -123,6 +204,11 @@ static void test_config_rom_generator(struct kunit *test) .key = (CSR_DIRECTORY | CSR_UNIT) << 24, .data = avc_unit_directory_and_leaf, }; + struct fw_descriptor iidc_unit_entry = { + .length = ARRAY_SIZE(iidc_unit_directories_and_leafs), + .key = (CSR_DIRECTORY | CSR_UNIT) << 24, + .data = iidc_unit_directories_and_leafs, + }; const struct generator_test_case *test_case = test->param_value; struct state_data *state = test->priv; struct fw_card *card = &state->card; @@ -136,6 +222,9 @@ static void test_config_rom_generator(struct kunit *test) if (test_case->expected == config_rom_with_avc_unit) KUNIT_EXPECT_EQ(test, fw_core_add_descriptor(&avc_unit_entry), 0); + if (test_case->expected == config_rom_with_iidc_unit) + KUNIT_EXPECT_EQ(test, fw_core_add_descriptor(&iidc_unit_entry), 0); + scoped_guard(mutex, &card_mutex) { generate_config_rom(card, config_rom); KUNIT_EXPECT_EQ(test, config_rom_length, test_case->quadlet_length); @@ -144,6 +233,9 @@ static void test_config_rom_generator(struct kunit *test) KUNIT_EXPECT_MEMEQ(test, config_rom, test_case->expected, sizeof(*test_case->expected) * test_case->quadlet_length); + if (test_case->expected == config_rom_with_iidc_unit) + fw_core_remove_descriptor(&iidc_unit_entry); + if (test_case->expected == config_rom_with_avc_unit) fw_core_remove_descriptor(&avc_unit_entry); } From 56db07175f9f8ee4dcbebf12ab17aa73ae30c264 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:31 +0900 Subject: [PATCH 14/88] firewire: core: add test to generate with both AV/C and IIDC units in config ROM generator Add a KUnit test to verify that the generator produces the expected configuration ROM when both AV/C and IIDC units are registered. Link: https://lore.kernel.org/r/20260901134534.2386577-11-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-generator-test.c | 74 ++++++++++++++++++-- 1 file changed, 70 insertions(+), 4 deletions(-) diff --git a/drivers/firewire/config-rom-generator-test.c b/drivers/firewire/config-rom-generator-test.c index 6ca5c16b4f0f..7d76580c2004 100644 --- a/drivers/firewire/config-rom-generator-test.c +++ b/drivers/firewire/config-rom-generator-test.c @@ -162,6 +162,62 @@ static const u32 config_rom_with_iidc_unit[] = { cpu_to_be32(0x20436174), // v }; +static const u32 config_rom_with_avc_and_iidc_units[] = { + cpu_to_be32(0x040439c0), // bus info + cpu_to_be32(0x31333934), // | + cpu_to_be32(0xf000b253), // | + cpu_to_be32(0x01234567), // | + cpu_to_be32(0x89abcdef), // v + cpu_to_be32(0x0007073e), // root directory + cpu_to_be32(0x0c0083c0), // | + cpu_to_be32(0x03001f11), // | + cpu_to_be32(0x81000005), // | + cpu_to_be32(0x17023901), // | + cpu_to_be32(0x8100000a), // | + cpu_to_be32(0xd100000d), // | + cpu_to_be32(0xd1000015), // v + cpu_to_be32(0x00064cb7), // text descriptor leaf (from root) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x4c696e75), // | + cpu_to_be32(0x78204669), // | + cpu_to_be32(0x72657769), // | + cpu_to_be32(0x72650000), // v + cpu_to_be32(0x0003ff1c), // text descriptor leaf (from root) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x4a756a75), // v + cpu_to_be32(0x0004227b), // unit directory (from root) + cpu_to_be32(0x1200a02d), // | + cpu_to_be32(0x13010001), // | + cpu_to_be32(0x17260827), // | + cpu_to_be32(0x81000001), // v + cpu_to_be32(0x0003f771), // text descriptor leaf (from unit) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x50756900), // v + cpu_to_be32(0x0003d7fe), // unit directory (from root) + cpu_to_be32(0x1200a02d), // | + cpu_to_be32(0x13000100), // | + cpu_to_be32(0xd4000001), // v + cpu_to_be32(0x0003ea57), // dependent directory (from unit directory) + cpu_to_be32(0x40012345), // | + cpu_to_be32(0x81000002), // | + cpu_to_be32(0x82000008), // v + cpu_to_be32(0x00064cb7), // text descriptor leaf (from dependent directory) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x4c696e75), // | + cpu_to_be32(0x78204669), // | + cpu_to_be32(0x72657769), // | + cpu_to_be32(0x72650000), // v + cpu_to_be32(0x0004a3e9), // text descriptor leaf (from dependent directory) + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x00000000), // | + cpu_to_be32(0x43686566), // | + cpu_to_be32(0x20436174), // v +}; + static const struct generator_test_case { const char *const name; int config_rom_generation; @@ -186,6 +242,12 @@ static const struct generator_test_case { .expected = config_rom_with_iidc_unit, .quadlet_length = ARRAY_SIZE(config_rom_with_iidc_unit), }, + { + .name = "with_avc_and_iidc_unit", + .config_rom_generation = 3, + .expected = config_rom_with_avc_and_iidc_units, + .quadlet_length = ARRAY_SIZE(config_rom_with_avc_and_iidc_units), + }, }; // Define generator_test_gen_params. @@ -219,10 +281,12 @@ static void test_config_rom_generator(struct kunit *test) card->max_receive = 11; card->guid = 0x0123456789abcdefULL; - if (test_case->expected == config_rom_with_avc_unit) + if (test_case->expected == config_rom_with_avc_unit || + test_case->expected == config_rom_with_avc_and_iidc_units) KUNIT_EXPECT_EQ(test, fw_core_add_descriptor(&avc_unit_entry), 0); - if (test_case->expected == config_rom_with_iidc_unit) + if (test_case->expected == config_rom_with_iidc_unit || + test_case->expected == config_rom_with_avc_and_iidc_units) KUNIT_EXPECT_EQ(test, fw_core_add_descriptor(&iidc_unit_entry), 0); scoped_guard(mutex, &card_mutex) { @@ -233,10 +297,12 @@ static void test_config_rom_generator(struct kunit *test) KUNIT_EXPECT_MEMEQ(test, config_rom, test_case->expected, sizeof(*test_case->expected) * test_case->quadlet_length); - if (test_case->expected == config_rom_with_iidc_unit) + if (test_case->expected == config_rom_with_iidc_unit || + test_case->expected == config_rom_with_avc_and_iidc_units) fw_core_remove_descriptor(&iidc_unit_entry); - if (test_case->expected == config_rom_with_avc_unit) + if (test_case->expected == config_rom_with_avc_unit || + test_case->expected == config_rom_with_avc_and_iidc_units) fw_core_remove_descriptor(&avc_unit_entry); } From 0eefccd9cce8a98abf8260b44d5cd299980a9a35 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:32 +0900 Subject: [PATCH 15/88] firewire: core: add test for invalid length in config ROM generator Although the unit drivers and the userspace applications are allowed to extend the system configuration ROM, they could provide invalid extension data. Add a KUnit test to verify that the generator handles an extension with an invalid length. Link: https://lore.kernel.org/r/20260901134534.2386577-12-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-generator-test.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/firewire/config-rom-generator-test.c b/drivers/firewire/config-rom-generator-test.c index 7d76580c2004..260b3d6e9a27 100644 --- a/drivers/firewire/config-rom-generator-test.c +++ b/drivers/firewire/config-rom-generator-test.c @@ -306,6 +306,16 @@ static void test_config_rom_generator(struct kunit *test) fw_core_remove_descriptor(&avc_unit_entry); } +static void add_descriptor_with_invalid_length(struct kunit *test) +{ + // Use kernel stack since they should be mutable for doubly linked-list. + struct fw_descriptor entry_with_invalid_length = { + .length = 257, + }; + + KUNIT_EXPECT_EQ(test, fw_core_add_descriptor(&entry_with_invalid_length), -EINVAL); +} + static const struct fw_card_driver dummy_card_driver; static int config_rom_generator_test_init(struct kunit *test) @@ -336,6 +346,7 @@ static void config_rom_generator_test_exit(struct kunit *test) static struct kunit_case config_rom_generator_test_cases[] = { KUNIT_CASE_PARAM(test_config_rom_generator, generator_test_gen_params), + KUNIT_CASE(add_descriptor_with_invalid_length), {} }; From eb077c3503777828617b056229b45815e695b3f8 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:33 +0900 Subject: [PATCH 16/88] firewire: core: add invalid block test for config ROM generator Although the unit drivers and the userspace applications are allowed to extend the system configuration ROM, they could provide invalid extension data. Add a KUnit test to verify that the generator handles an invalid block. Link: https://lore.kernel.org/r/20260901134534.2386577-13-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-generator-test.c | 27 ++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/drivers/firewire/config-rom-generator-test.c b/drivers/firewire/config-rom-generator-test.c index 260b3d6e9a27..3114110307b5 100644 --- a/drivers/firewire/config-rom-generator-test.c +++ b/drivers/firewire/config-rom-generator-test.c @@ -316,6 +316,32 @@ static void add_descriptor_with_invalid_length(struct kunit *test) KUNIT_EXPECT_EQ(test, fw_core_add_descriptor(&entry_with_invalid_length), -EINVAL); } +static void add_descriptor_with_invalid_data(struct kunit *test) +{ + // Use vendor directory defined in Annex A of Configuration ROM for AV/C Devices 1.0 (Dec. + // 2000, 1394 Trading Association, TA Document 1999027). + static const u32 invalid_vendor_directory[] = { + 0x00020000, + (CSR_MODEL << 24) | 0x00009402, + ((CSR_LEAF | CSR_DESCRIPTOR) << 24) | 0x00000001, + 0xffff0000, // The length should be 6, invalid. + 0x00000000, + 0x00000000, + 0x436f6e63, + 0x6174204e, + 0x6f746174, + 0x696f6e00, + }; + // Use kernel stack since they should be mutable for doubly linked-list. + struct fw_descriptor entry_with_invalid_data = { + .length = ARRAY_SIZE(invalid_vendor_directory), + .key = (CSR_DIRECTORY | CSR_VENDOR) << 24, + .data = invalid_vendor_directory, + }; + + KUNIT_EXPECT_EQ(test, fw_core_add_descriptor(&entry_with_invalid_data), -EINVAL); +} + static const struct fw_card_driver dummy_card_driver; static int config_rom_generator_test_init(struct kunit *test) @@ -347,6 +373,7 @@ static void config_rom_generator_test_exit(struct kunit *test) static struct kunit_case config_rom_generator_test_cases[] = { KUNIT_CASE_PARAM(test_config_rom_generator, generator_test_gen_params), KUNIT_CASE(add_descriptor_with_invalid_length), + KUNIT_CASE(add_descriptor_with_invalid_data), {} }; From 6899ac01c6ae398daf2acbf78f7a2dd86e3c0af5 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 1 Sep 2026 22:45:34 +0900 Subject: [PATCH 17/88] firewire: core: add test for beyond-boundary case in config ROM generator Although unit drivers and userspace applications are allowed to extend the system configuration ROM, they could provide invalid extension data. Add a KUnit test to verify that the generator does not write beyond the boundary of the configuration ROM when handling an extension. Link: https://lore.kernel.org/r/20260901134534.2386577-14-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-generator-test.c | 21 ++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/drivers/firewire/config-rom-generator-test.c b/drivers/firewire/config-rom-generator-test.c index 3114110307b5..212dbdcd43ba 100644 --- a/drivers/firewire/config-rom-generator-test.c +++ b/drivers/firewire/config-rom-generator-test.c @@ -342,6 +342,26 @@ static void add_descriptor_with_invalid_data(struct kunit *test) KUNIT_EXPECT_EQ(test, fw_core_add_descriptor(&entry_with_invalid_data), -EINVAL); } +static void add_descriptor_beyond_upper_limit(struct kunit *test) +{ + // Use kernel stack since they should be mutable for doubly linked-list. + struct fw_descriptor entry_beyond_upper_limit = { + .length = 255, + .key = (CSR_DIRECTORY | CSR_UNIT) << 24, + .data = NULL, + }; + u32 *data; + + data = kunit_kzalloc(test, entry_beyond_upper_limit.length, GFP_KERNEL); + KUNIT_ASSERT_NOT_NULL(test, data); + + data[0] = (entry_beyond_upper_limit.length - 1) << 16; + entry_beyond_upper_limit.data = data; + KUNIT_EXPECT_EQ(test, fw_core_add_descriptor(&entry_beyond_upper_limit), -EBUSY); + + kunit_kfree(test, data); +} + static const struct fw_card_driver dummy_card_driver; static int config_rom_generator_test_init(struct kunit *test) @@ -374,6 +394,7 @@ static struct kunit_case config_rom_generator_test_cases[] = { KUNIT_CASE_PARAM(test_config_rom_generator, generator_test_gen_params), KUNIT_CASE(add_descriptor_with_invalid_length), KUNIT_CASE(add_descriptor_with_invalid_data), + KUNIT_CASE(add_descriptor_beyond_upper_limit), {} }; From eef77f7c065e35cfac9e782a646c6e1b4b4c67c1 Mon Sep 17 00:00:00 2001 From: longlong yan Date: Wed, 2 Sep 2026 10:39:13 +0800 Subject: [PATCH 18/88] tools/firewire: nosy-dump: fix input file handle leak The input file handle opened via the --input option is leaked on multiple exit paths: 1. When fopen() for the output file fails, the already-opened input handle is not closed before returning. 2. When fread() reaches EOF while reading from the input file, the main loop returns directly, bypassing the cleanup section entirely. 3. On normal exit (e.g., SIGINT), the cleanup section closes output and fd but never closes input. Fix this by closing the input handle on the output-fopen error path and in the cleanup section, and by changing the fread EOF early return to a break so that the cleanup section runs. Signed-off-by: longlong yan Link: https://lore.kernel.org/r/20260902023914.959-1-yanlonglong@kylinos.cn Signed-off-by: Takashi Sakamoto --- tools/firewire/nosy-dump.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tools/firewire/nosy-dump.c b/tools/firewire/nosy-dump.c index 9cc8626a7e94..7ddb50521c29 100644 --- a/tools/firewire/nosy-dump.c +++ b/tools/firewire/nosy-dump.c @@ -947,6 +947,8 @@ int main(int argc, const char *argv[]) output = fopen(option_output, "w"); if (output == NULL) { fprintf(stderr, "Could not open %s, %m\n", option_output); + if (input != NULL) + fclose(input); return -1; } } @@ -973,7 +975,7 @@ int main(int argc, const char *argv[]) while (run) { if (input != NULL) { if (fread(&length, sizeof length, 1, input) != 1) - return 0; + break; fread(buf, 1, length, input); } else { poll(pollfds, 2, -1); @@ -1014,6 +1016,9 @@ int main(int argc, const char *argv[]) if (output != NULL) fclose(output); + if (input != NULL) + fclose(input); + if (fd >= 0) close(fd); From 6393d75914a6ac95a05621db8d9d6a98eafe72f7 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 8 Sep 2026 21:28:40 +0900 Subject: [PATCH 19/88] firewire: core: fix source bus ID detection for AT context in big-endian systems The source bus ID bit in AT DMA context is not detected correctly on big-endian systems because the little-endian data is not converted to CPU endianness. Fix the bug. The helper functions related to this are never used in driver code, so extend the test to check the field instead. Cc: stable@vger.kernel.org # v6.12 Fixes: 8db9d1557122 ("firewire: ohci: add static inline functions to serialize/deserialize data of AT DMA") Link: https://lore.kernel.org/r/20260908122840.2886621-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci-serdes-test.c | 4 ++-- drivers/firewire/ohci.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/firewire/ohci-serdes-test.c b/drivers/firewire/ohci-serdes-test.c index 258f668619ef..87b136e041de 100644 --- a/drivers/firewire/ohci-serdes-test.c +++ b/drivers/firewire/ohci-serdes-test.c @@ -43,7 +43,7 @@ static void test_self_id_receive_buffer_deserialization(struct kunit *test) static void test_at_data_serdes(struct kunit *test) { static const __le32 expected[] = { - cpu_to_le32(0x00020e80), + cpu_to_le32(0x00820e80), cpu_to_le32(0xffc2ffff), cpu_to_le32(0xe0000000), }; @@ -56,7 +56,7 @@ static void test_at_data_serdes(struct kunit *test) unsigned int destination_id = ohci1394_at_data_get_destination_id(expected); u64 destination_offset = ohci1394_at_data_get_destination_offset(expected); - KUNIT_EXPECT_FALSE(test, has_src_bus_id); + KUNIT_EXPECT_TRUE(test, has_src_bus_id); KUNIT_EXPECT_EQ(test, 0x02, speed); KUNIT_EXPECT_EQ(test, 0x03, tlabel); KUNIT_EXPECT_EQ(test, 0x02, retry); diff --git a/drivers/firewire/ohci.h b/drivers/firewire/ohci.h index 218666cfe14a..63ccf2f44504 100644 --- a/drivers/firewire/ohci.h +++ b/drivers/firewire/ohci.h @@ -178,7 +178,7 @@ static inline bool ohci1394_at_data_get_src_bus_id(const __le32 *data) { - return !!((data[0] & OHCI1394_AT_DATA_Q0_srcBusID_MASK) >> OHCI1394_AT_DATA_Q0_srcBusID_SHIFT); + return !!((le32_to_cpu(data[0]) & OHCI1394_AT_DATA_Q0_srcBusID_MASK) >> OHCI1394_AT_DATA_Q0_srcBusID_SHIFT); } static inline void ohci1394_at_data_set_src_bus_id(__le32 *data, bool src_bus_id) From b0ae21b88ae4fd68c197d5ee3b6676162bebb61e Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 8 Sep 2026 21:29:00 +0900 Subject: [PATCH 20/88] firewire: core: fix type of hard-coded image in config ROM generator test The type of the hard-coded configuration ROM image should be __be32 instead of u32. Fixes: bea8f9df4254 ("firewire: core: add test for root directory generation in config ROM generator") Fixes: c0c5987f4bdd ("firewire: core: add test to generate with AV/C unit in config ROM generator") Fixes: 49e4d0eea6cd ("firewire: core: add test to generate with IIDC unit in config ROM generator") Fixes: 56db07175f9f ("firewire: core: add test to generate with both AV/C and IIDC units in config ROM generator") Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202609060630.rC0Zz88P-lkp@intel.com/ Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202609061023.GbWS6Skb-lkp@intel.com/ Link: https://lore.kernel.org/r/20260908122900.2886661-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-generator-test.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/firewire/config-rom-generator-test.c b/drivers/firewire/config-rom-generator-test.c index 212dbdcd43ba..dd2e71213209 100644 --- a/drivers/firewire/config-rom-generator-test.c +++ b/drivers/firewire/config-rom-generator-test.c @@ -9,7 +9,7 @@ #include #include -static const u32 config_rom_bare[] = { +static const __be32 config_rom_bare[] = { cpu_to_be32(0x0404921b), // bus info cpu_to_be32(0x31333934), // | cpu_to_be32(0xf000b223), // | @@ -52,7 +52,7 @@ static const u32 avc_unit_directory_and_leaf[] = { 0x50756900, // Pui is the name of a cat that the author takes care of. }; -static const u32 config_rom_with_avc_unit[] = { +static const __be32 config_rom_with_avc_unit[] = { cpu_to_be32(0x0404c1e5), // bus info cpu_to_be32(0x31333934), // | cpu_to_be32(0xf000b233), // | @@ -116,7 +116,7 @@ static const u32 iidc_unit_directories_and_leafs[] = { 0x20436174, }; -static const u32 config_rom_with_iidc_unit[] = { +static const __be32 config_rom_with_iidc_unit[] = { cpu_to_be32(0x04046a3e), // bus info cpu_to_be32(0x31333934), // | cpu_to_be32(0xf000b243), // | @@ -162,7 +162,7 @@ static const u32 config_rom_with_iidc_unit[] = { cpu_to_be32(0x20436174), // v }; -static const u32 config_rom_with_avc_and_iidc_units[] = { +static const __be32 config_rom_with_avc_and_iidc_units[] = { cpu_to_be32(0x040439c0), // bus info cpu_to_be32(0x31333934), // | cpu_to_be32(0xf000b253), // | From e1f9d641fb9c14331ff62b4db6fe0844beb529ec Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 8 Sep 2026 21:29:29 +0900 Subject: [PATCH 21/88] firewire: core: fix type of transaction data in config ROM parser test The type of transaction data should be __be32 in the stub function for the asynchronous transactions. Fixes: bae885b9bee5 ("firewire: core: add test to detect irm-is-1394-1995-only quirk in config ROM parser") Fixes: 1c4326e7d830 ("firewire: core: add test to avoid excessive configuration ROM length") Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202609060257.8wQo9w9N-lkp@intel.com/ Link: https://lore.kernel.org/r/20260908122929.2886698-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/config-rom-parser-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/config-rom-parser-test.c b/drivers/firewire/config-rom-parser-test.c index e5c173353057..45f1ead326c5 100644 --- a/drivers/firewire/config-rom-parser-test.c +++ b/drivers/firewire/config-rom-parser-test.c @@ -206,7 +206,7 @@ static int stub_run_transaction_regular(struct fw_card *card, int tcode, int des KUNIT_ASSERT_EQ(test, length, 4); unsigned int index = (offset - (CSR_REGISTER_BASE | CSR_CONFIG_ROM)) / sizeof(u32); - u32 *quadlet = payload; + __be32 *quadlet = payload; KUNIT_EXPECT_LE(test, speed, param->expected_speed); KUNIT_EXPECT_LT(test, index, param->quadlet_length); @@ -264,7 +264,7 @@ static int stub_run_transaction_malformed(struct fw_card *card, int tcode, int d KUNIT_ASSERT_EQ(test, length, 4); unsigned int index = (offset - (CSR_REGISTER_BASE | CSR_CONFIG_ROM)) / sizeof(u32); - u32 *quadlet = payload; + __be32 *quadlet = payload; if (index < ARRAY_SIZE(config_rom_first_part)) *quadlet = cpu_to_be32(config_rom_first_part[index]); From 0a6cf2b7adcbd362039e9d282f73aff3f24022ca Mon Sep 17 00:00:00 2001 From: Karl Mehltretter Date: Sat, 12 Sep 2026 00:28:07 +0200 Subject: [PATCH 22/88] firewire: cdev: Fix FW_CDEV_IOC_CREATE_ISO_CONTEXT references in kernel-doc The fw_cdev_event_iso_interrupt and fw_cdev_event_iso_interrupt_mc comments refer to FW_CDEV_CREATE_ISO_CONTEXT, but the ioctl is named FW_CDEV_IOC_CREATE_ISO_CONTEXT. Correct both references. The misspellings were introduced separately with the userspace interface documentation and multichannel reception support. Fixes: 7ada60e36764 ("firewire: Document userspace ioctl interface.") Fixes: 872e330e3880 ("firewire: add isochronous multichannel reception") Assisted-by: LLM Signed-off-by: Karl Mehltretter Link: https://lore.kernel.org/r/20260911222807.7673-1-kmehltretter@gmail.com Signed-off-by: Takashi Sakamoto --- include/uapi/linux/firewire-cdev.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/uapi/linux/firewire-cdev.h b/include/uapi/linux/firewire-cdev.h index 05e3aa8fa8bc..4f83d6babc92 100644 --- a/include/uapi/linux/firewire-cdev.h +++ b/include/uapi/linux/firewire-cdev.h @@ -293,7 +293,7 @@ struct fw_cdev_event_request3 { /** * struct fw_cdev_event_iso_interrupt - Sent when an iso packet was completed * @closure: See &fw_cdev_event_common; - * set by %FW_CDEV_CREATE_ISO_CONTEXT ioctl + * set by %FW_CDEV_IOC_CREATE_ISO_CONTEXT ioctl * @type: See &fw_cdev_event_common; always %FW_CDEV_EVENT_ISO_INTERRUPT * @cycle: Cycle counter of the last completed packet * @header_length: Total length of following headers, in bytes @@ -353,7 +353,7 @@ struct fw_cdev_event_iso_interrupt { /** * struct fw_cdev_event_iso_interrupt_mc - An iso buffer chunk was completed * @closure: See &fw_cdev_event_common; - * set by %FW_CDEV_CREATE_ISO_CONTEXT ioctl + * set by %FW_CDEV_IOC_CREATE_ISO_CONTEXT ioctl * @type: %FW_CDEV_EVENT_ISO_INTERRUPT_MULTICHANNEL * @completed: Offset into the receive buffer; data before this offset is valid * From 00d4d31e4fdfe51312f0184f8d162b8d4de172dc Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Fri, 11 Sep 2026 23:16:02 +0900 Subject: [PATCH 23/88] firewire: core: remove unused link field in fw_packet structure The link field in fw_packet structure has never been used since it was added. Fixes: 3038e353cfaf ("firewire: Add core firewire stack.") Link: https://lore.kernel.org/r/20260911141602.3288806-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- include/linux/firewire.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 7fa1a3fc8ed4..a1e0cb4526c9 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -337,7 +337,6 @@ struct fw_packet { */ fw_packet_callback_t callback; int ack; - struct list_head link; void *driver_data; }; From f3a992ff079a02d415ea1c90f1b32091b5f0367c Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 12 Sep 2026 12:22:19 +0900 Subject: [PATCH 24/88] firewire: ohci: obsolete flushing field in at_context structure The flushing field in the at_context structure is used for the internal branches in handle_at_packet(). This field is enabled in at_context_flush() when either handling the selfIDComplete event in the threadedIRQ handler or calling the .remove callback of the PCI device. In these two flushing cases, the pointer to the work structure retrieved by current_work() should be different from the one in the at_context structure. Replacing the flushing field with a work structure check is preferable since the branching can be self-contained without relying on an external flag. Link: https://lore.kernel.org/r/20260912032219.3379821-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index b7df280bec51..7d797755bbd6 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -162,7 +162,6 @@ struct context { struct at_context { struct context context; struct work_struct work; - bool flushing; }; struct iso_context { @@ -1338,9 +1337,7 @@ static void at_context_flush(struct at_context *ctx) disable_work_sync(&ctx->work); - WRITE_ONCE(ctx->flushing, true); ohci_at_context_work(&ctx->work); - WRITE_ONCE(ctx->flushing, false); enable_work(&ctx->work); } @@ -1362,8 +1359,10 @@ static int handle_at_packet(struct context *context, struct driver_data *driver_data; struct fw_packet *packet; int evt; + // Check whether this is called from at_context_flush(). + bool in_flushing = current_work() != &ctx->work; - if (last->transfer_status == 0 && !READ_ONCE(ctx->flushing)) + if (last->transfer_status == 0 && !in_flushing) /* This descriptor isn't done yet, stop iteration. */ return 0; @@ -1395,7 +1394,7 @@ static int handle_at_packet(struct context *context, break; case OHCI1394_evt_missing_ack: - if (READ_ONCE(ctx->flushing)) + if (in_flushing) packet->ack = RCODE_GENERATION; else { /* @@ -1417,7 +1416,7 @@ static int handle_at_packet(struct context *context, break; case OHCI1394_evt_no_status: - if (READ_ONCE(ctx->flushing)) { + if (in_flushing) { packet->ack = RCODE_GENERATION; break; } From a1cc42e146cb710cc3547225a8ffb46951285a49 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 13 Sep 2026 19:13:52 +0900 Subject: [PATCH 25/88] firewire: ohci: use in_range() macro to detect CSR address ranges The current implementation handles asynchronous packets sent to the two CSR address ranges on the local node specially. To decide whether a packet is destined for either range, the destination offset needs to be checked according to the IEEE 1394 and 1394 OHCI specifications. The current implementation uses both conditional and switch statements to check the offset, which makes the range checks difficult to reuse. Add two helper functions for the range checks using the in_range() macro. Link: https://lore.kernel.org/r/20260913101356.156420-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 7d797755bbd6..c8cb7bdcb4ed 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1534,35 +1534,38 @@ static void handle_local_lock(struct fw_ohci *ohci, fw_core_handle_response(&ohci->card, &response); } +static bool in_config_rom_csr_registers(u64 offset) +{ + return in_range(offset, CSR_CONFIG_ROM, CONFIG_ROM_SIZE); +} + +// 5.5.1 Bus Management CSR Registers. +static bool in_bus_management_csr_registers(u64 offset) +{ + // 0x22c = CSR_MAINT_CONTROL following to CSR_CHANNELS_AVAILABLE_LO. + return in_range(offset, CSR_BUS_MANAGER_ID, 0x22c - CSR_BUS_MANAGER_ID); +} + static void handle_local_request(struct at_context *ctx, struct fw_packet *packet) { struct fw_ohci *ohci = ctx->context.ohci; - u64 offset, csr; if (ctx == &ohci->at_request_ctx) { packet->ack = ACK_PENDING; packet->callback(packet, &ohci->card, packet->ack); } - offset = async_header_get_offset(packet->header); - csr = offset - CSR_REGISTER_BASE; + u64 csr_offset = async_header_get_offset(packet->header) - CSR_REGISTER_BASE; - /* Handle config rom reads. */ - if (csr >= CSR_CONFIG_ROM && csr < CSR_CONFIG_ROM_END) - handle_local_rom(ohci, packet, csr); - else switch (csr) { - case CSR_BUS_MANAGER_ID: - case CSR_BANDWIDTH_AVAILABLE: - case CSR_CHANNELS_AVAILABLE_HI: - case CSR_CHANNELS_AVAILABLE_LO: - handle_local_lock(ohci, packet, csr); - break; - default: + if (in_config_rom_csr_registers(csr_offset)) { + handle_local_rom(ohci, packet, csr_offset); + } else if (in_bus_management_csr_registers(csr_offset)) { + handle_local_lock(ohci, packet, csr_offset); + } else { if (ctx == &ohci->at_request_ctx) fw_core_handle_request(&ohci->card, packet); else fw_core_handle_response(&ohci->card, packet); - break; } if (ctx == &ohci->at_response_ctx) { From 7d8dcdf220de1a8a240de67c53cb4a33107c5a21 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 13 Sep 2026 19:13:53 +0900 Subject: [PATCH 26/88] firewire: ohci: add helper function to detect local AT request/response packets The current implementation handles asynchronous packets sent to the local node specially. To decide whether a packet is destined for the local node, the local node ID and bus generation need to be checked while holding the spinlock, since both can change. Add a helper function annotated to require the spinlock to be held. Link: https://lore.kernel.org/r/20260913101356.156420-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index c8cb7bdcb4ed..290bac5112b7 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1574,6 +1574,15 @@ static void handle_local_request(struct at_context *ctx, struct fw_packet *packe } } +static bool destination_is_local(const struct fw_packet *packet, const struct fw_ohci *ohci) +__must_hold(&ohci->lock) +{ + lockdep_assert_held(&ohci->lock); + + return (async_header_get_destination(packet->header) == ohci->node_id && + ohci->generation == packet->generation); +} + static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet) { struct fw_ohci *ohci = ctx->context.ohci; @@ -1582,8 +1591,7 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet spin_lock_irqsave(&ohci->lock, flags); - if (async_header_get_destination(packet->header) == ohci->node_id && - ohci->generation == packet->generation) { + if (destination_is_local(packet, ohci)) { spin_unlock_irqrestore(&ohci->lock, flags); // Timestamping on behalf of the hardware. From 07457748d53204ff93aa2eb6852ea649df9d349a Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 13 Sep 2026 19:13:54 +0900 Subject: [PATCH 27/88] firewire: ohci: refactor multiple calls to fw_fill_response() The handle_local_rom() function calls fw_fill_response() in each conditional branch. Using local variables for the function parameters allows the function to be called from a single place. Link: https://lore.kernel.org/r/20260913101356.156420-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 290bac5112b7..ec069657ea8b 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1459,7 +1459,8 @@ static void handle_local_rom(struct fw_ohci *ohci, struct fw_packet *packet, u32 csr) { struct fw_packet response; - int tcode, length, i; + int tcode, rcode, length, i; + void *payload = NULL; tcode = async_header_get_tcode(packet->header); if (tcode_is_block_packet(tcode)) @@ -1469,16 +1470,18 @@ static void handle_local_rom(struct fw_ohci *ohci, i = csr - CSR_CONFIG_ROM; if (i + length > CONFIG_ROM_SIZE) { - fw_fill_response(&response, packet->header, - RCODE_ADDRESS_ERROR, NULL, 0); + rcode = RCODE_ADDRESS_ERROR; + length = 0; } else if (!tcode_is_read_request(tcode)) { - fw_fill_response(&response, packet->header, - RCODE_TYPE_ERROR, NULL, 0); + rcode = RCODE_TYPE_ERROR; + length = 0; } else { - fw_fill_response(&response, packet->header, RCODE_COMPLETE, - (void *) ohci->config_rom + i, length); + rcode = RCODE_COMPLETE; + payload = (u8 *)ohci->config_rom + i; } + fw_fill_response(&response, packet->header, rcode, payload, length); + // Timestamping on behalf of the hardware. response.timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); fw_core_handle_response(&ohci->card, &response); From 82b72626be3786bcd9b88f7dc40d1ce020066ac6 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 13 Sep 2026 19:13:55 +0900 Subject: [PATCH 28/88] firewire: ohci: refactor local response packet handling For an asynchronous transaction to one of the two CSR address ranges on the local node, the current implementation generates the packet data for the response subaction internally while handling the request subaction. The handling code is duplicated between the two ranges. Reduce the duplication by moving response handling to the callers. Link: https://lore.kernel.org/r/20260913101356.156420-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 54 ++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index ec069657ea8b..8a0aedf50a92 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1455,10 +1455,9 @@ static int handle_at_packet(struct context *context, static u32 get_cycle_time(struct fw_ohci *ohci); -static void handle_local_rom(struct fw_ohci *ohci, - struct fw_packet *packet, u32 csr) +static void handle_local_rom(struct fw_ohci *ohci, struct fw_packet *packet, u32 csr, + struct fw_packet *response) { - struct fw_packet response; int tcode, rcode, length, i; void *payload = NULL; @@ -1480,17 +1479,12 @@ static void handle_local_rom(struct fw_ohci *ohci, payload = (u8 *)ohci->config_rom + i; } - fw_fill_response(&response, packet->header, rcode, payload, length); - - // Timestamping on behalf of the hardware. - response.timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); - fw_core_handle_response(&ohci->card, &response); + fw_fill_response(response, packet->header, rcode, payload, length); } -static void handle_local_lock(struct fw_ohci *ohci, - struct fw_packet *packet, u32 csr) +static void handle_local_lock(struct fw_ohci *ohci, struct fw_packet *packet, u32 csr, + struct fw_packet *response) { - struct fw_packet response; int tcode, length, ext_tcode, sel, try; __be32 *payload, lock_old; u32 lock_arg, lock_data; @@ -1508,9 +1502,8 @@ static void handle_local_lock(struct fw_ohci *ohci, lock_arg = 0; lock_data = 0; } else { - fw_fill_response(&response, packet->header, - RCODE_TYPE_ERROR, NULL, 0); - goto out; + fw_fill_response(response, packet->header, RCODE_TYPE_ERROR, NULL, 0); + return; } sel = (csr - CSR_BUS_MANAGER_ID) / 4; @@ -1522,19 +1515,13 @@ static void handle_local_lock(struct fw_ohci *ohci, if (reg_read(ohci, OHCI1394_CSRControl) & 0x80000000) { lock_old = cpu_to_be32(reg_read(ohci, OHCI1394_CSRData)); - fw_fill_response(&response, packet->header, - RCODE_COMPLETE, - &lock_old, sizeof(lock_old)); - goto out; + fw_fill_response(response, packet->header, RCODE_COMPLETE, &lock_old, + sizeof(lock_old)); + return; } ohci_err(ohci, "swap not done (CSR lock timeout)\n"); - fw_fill_response(&response, packet->header, RCODE_BUSY, NULL, 0); - - out: - // Timestamping on behalf of the hardware. - response.timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); - fw_core_handle_response(&ohci->card, &response); + fw_fill_response(response, packet->header, RCODE_BUSY, NULL, 0); } static bool in_config_rom_csr_registers(u64 offset) @@ -1560,10 +1547,21 @@ static void handle_local_request(struct at_context *ctx, struct fw_packet *packe u64 csr_offset = async_header_get_offset(packet->header) - CSR_REGISTER_BASE; - if (in_config_rom_csr_registers(csr_offset)) { - handle_local_rom(ohci, packet, csr_offset); - } else if (in_bus_management_csr_registers(csr_offset)) { - handle_local_lock(ohci, packet, csr_offset); + if (in_config_rom_csr_registers(csr_offset) || in_bus_management_csr_registers(csr_offset)) { + struct fw_packet response; + + memset(&response, 0, sizeof(response)); + + if (in_bus_management_csr_registers(csr_offset)) + handle_local_lock(ohci, packet, csr_offset, &response); + else + handle_local_rom(ohci, packet, csr_offset, &response); + + // Timestamping on behalf of the hardware. + response.timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); + + // Finish the transaction immediately. + fw_core_handle_response(&ohci->card, &response); } else { if (ctx == &ohci->at_request_ctx) fw_core_handle_request(&ohci->card, packet); From fbb948d001f77583164b365414823ef217632a78 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 13 Sep 2026 19:13:56 +0900 Subject: [PATCH 29/88] firewire: ohci: refactor handling of local AT request/response packets For an asynchronous transaction to one of the two CSR address ranges on the local node, the current implementation generates the packet data for the response subaction internally while handling the request subaction. Unlike transactions to other addresses, there is no need to submit the new packet for the response subaction. The current implementation handles both subactions in a single helper function with conditional statements. This makes it difficult to see why the two subactions should be handled differently. Split the helper function into two helper functions, one for the request subaction and the other for the response subaction. Link: https://lore.kernel.org/r/20260913101356.156420-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 8a0aedf50a92..bd3e01b2f450 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1536,14 +1536,11 @@ static bool in_bus_management_csr_registers(u64 offset) return in_range(offset, CSR_BUS_MANAGER_ID, 0x22c - CSR_BUS_MANAGER_ID); } -static void handle_local_request(struct at_context *ctx, struct fw_packet *packet) +static void handle_local_at_request_packet(struct fw_ohci *ohci, struct fw_packet *packet) { - struct fw_ohci *ohci = ctx->context.ohci; - - if (ctx == &ohci->at_request_ctx) { - packet->ack = ACK_PENDING; - packet->callback(packet, &ohci->card, packet->ack); - } + // Emulate split transaction. + packet->ack = ACK_PENDING; + packet->callback(packet, &ohci->card, packet->ack); u64 csr_offset = async_header_get_offset(packet->header) - CSR_REGISTER_BASE; @@ -1563,16 +1560,23 @@ static void handle_local_request(struct at_context *ctx, struct fw_packet *packe // Finish the transaction immediately. fw_core_handle_response(&ohci->card, &response); } else { - if (ctx == &ohci->at_request_ctx) - fw_core_handle_request(&ohci->card, packet); - else - fw_core_handle_response(&ohci->card, packet); + fw_core_handle_request(&ohci->card, packet); } +} - if (ctx == &ohci->at_response_ctx) { - packet->ack = ACK_COMPLETE; - packet->callback(packet, &ohci->card, packet->ack); - } +static void handle_local_at_response_packet(struct fw_ohci *ohci, struct fw_packet *packet) +{ + u64 csr_offset = async_header_get_offset(packet->header) - CSR_REGISTER_BASE; + + // The transaction is already finished by handle_local_at_request_packet(). + if (WARN_ON(in_config_rom_csr_registers(csr_offset)) || + WARN_ON(in_bus_management_csr_registers(csr_offset))) + return; + + fw_core_handle_response(&ohci->card, packet); + + packet->ack = ACK_COMPLETE; + packet->callback(packet, &ohci->card, packet->ack); } static bool destination_is_local(const struct fw_packet *packet, const struct fw_ohci *ohci) @@ -1598,7 +1602,10 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet // Timestamping on behalf of the hardware. packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); - handle_local_request(ctx, packet); + if (ctx == &ohci->at_request_ctx) + handle_local_at_request_packet(ohci, packet); + else + handle_local_at_response_packet(ohci, packet); return; } From 5b671ed29a83925d60f61200be10b3ea952e6a7f Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 15 Sep 2026 07:14:34 +0900 Subject: [PATCH 30/88] firewire: core: correct comment about execution context The comment for fw_address_callback_t describes the execution context, but the callback has not been called from RCU read-side critical section since v6.17. Fixes: e884a8a0c573 ("firewire: core: call FCP address handlers outside RCU read-side critical section") Fixes: e8cf6875005b ("firewire: core: call handler for exclusive regions outside RCU read-side critical section") Link: https://lore.kernel.org/r/20260914221434.213633-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- include/linux/firewire.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/linux/firewire.h b/include/linux/firewire.h index a1e0cb4526c9..cbe59a18162e 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -299,8 +299,9 @@ union fw_transaction_callback { }; /* - * This callback handles an inbound request subaction. It is called in - * RCU read-side context, therefore must not sleep. + * This callback handles an inbound request subaction. If the request subaction is initiated from + * the local node (e.g. by unit driver), the execution context depends on the initiator and is + * unspecified. Otherwise, it runs in workqueue context. * * The callback should not initiate outbound request subactions directly. * Otherwise there is a danger of recursion of inbound and outbound From 5fe52c6aa28e0059cbddbd71eaf887948d98c311 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 19 Sep 2026 20:50:13 +0900 Subject: [PATCH 31/88] firewire: ohci: use workqueue to handle error case of AT request/response packet queueing When a packet cannot be queued to an AT context, the packet callback is invoked to report the error to the caller. Since the callback runs in the caller's context, its execution context depends on where the packet was submitted. This makes the callback context inconsistent between successful and failed subactions. Use a workqueue to handle the error cases. Link: https://lore.kernel.org/r/20260919115017.859169-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 7 +-- drivers/firewire/ohci.c | 90 +++++++++++++++++++++++++++++++++--- include/linux/firewire.h | 3 ++ 3 files changed, 91 insertions(+), 9 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 23749434d900..be6f18d67ece 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -643,11 +643,12 @@ int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid, // * == WQ_FREEZABLE The target device would not be available when being freezed. // * == WQ_HIGHPRI High priority to process semi-realtime timestamped data. // * == 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. + // * max_active == 4 + 2 A hardIRQ could notify events for a pair of requests and + // response AR/AT contexts. Additional 2 capacity are for the + // internal handling of local AT request and response packets. async_wq = alloc_workqueue("firewire-async-card%u", WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI | WQ_SYSFS, - 4, card->index); + 6, card->index); if (!async_wq) return -ENOMEM; diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index bd3e01b2f450..45f03095a196 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -164,6 +164,13 @@ struct at_context { struct work_struct work; }; +// The local-to-local transaction is handled by the work item in the following structure. +struct at_local { + struct list_head list; + spinlock_t lock; + struct work_struct work; +}; + struct iso_context { struct fw_iso_context base; struct context context; @@ -216,6 +223,9 @@ struct fw_ohci { struct at_context at_request_ctx; struct at_context at_response_ctx; + struct at_local at_request_local; + struct at_local at_response_local; + u32 it_context_support; u32 it_context_mask; /* unoccupied IT contexts */ struct iso_context *it_context_list; @@ -1579,6 +1589,51 @@ static void handle_local_at_response_packet(struct fw_ohci *ohci, struct fw_pack packet->callback(packet, &ohci->card, packet->ack); } +static void handle_at_local_packets(struct at_local *local, struct fw_ohci *ohci) +{ + struct fw_packet *packet; + + spin_lock(&local->lock); + + while ((packet = list_first_entry_or_null(&local->list, typeof(*packet), link_for_local))) { + list_del(&packet->link_for_local); + spin_unlock(&local->lock); + + if (unlikely(packet->ack != 0)) { + // This case is active when the call of at_context_queue_packet() returns + // error in at_context_transmit(). + packet->callback(packet, &ohci->card, packet->ack); + } + + spin_lock(&local->lock); + } + + spin_unlock(&local->lock); +} + +static void at_request_local_work(struct work_struct *work) +{ + struct at_local *local = from_work(local, work, work); + struct fw_ohci *ohci = container_of(local, struct fw_ohci, at_request_local); + + handle_at_local_packets(local, ohci); +} + +static void at_response_local_work(struct work_struct *work) +{ + struct at_local *local = from_work(local, work, work); + struct fw_ohci *ohci = container_of(local, struct fw_ohci, at_response_local); + + handle_at_local_packets(local, ohci); +} + +static void at_local_init(struct at_local *local, work_func_t func) +{ + spin_lock_init(&local->lock); + INIT_LIST_HEAD(&local->list); + INIT_WORK(&local->work, func); +} + static bool destination_is_local(const struct fw_packet *packet, const struct fw_ohci *ohci) __must_hold(&ohci->lock) { @@ -1588,6 +1643,24 @@ __must_hold(&ohci->lock) ohci->generation == packet->generation); } +static void queue_work_for_at_local_packet(struct at_context *ctx, struct fw_packet *packet, + struct fw_ohci *ohci) +{ + struct at_local *local; + + if (ctx == &ohci->at_request_ctx) + local = &ohci->at_request_local; + else + local = &ohci->at_response_local; + + // Timestamping on behalf of the hardware. + packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); + + scoped_guard(spinlock_irqsave, &local->lock) + list_add_tail(&packet->link_for_local, &local->list); + queue_work(ohci->card.async_wq, &local->work); +} + static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet) { struct fw_ohci *ohci = ctx->context.ohci; @@ -1612,12 +1685,8 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet ret = at_context_queue_packet(ctx, packet); spin_unlock_irqrestore(&ohci->lock, flags); - if (ret < 0) { - // Timestamping on behalf of the hardware. - packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); - - packet->callback(packet, &ohci->card, packet->ack); - } + if (ret < 0) + queue_work_for_at_local_packet(ctx, packet, ohci); } static void detect_dead_context(struct fw_ohci *ohci, @@ -2474,6 +2543,9 @@ static void ohci_disable(struct fw_card *card) flush_work(&ohci->at_request_ctx.work); flush_work(&ohci->at_response_ctx.work); + flush_work(&ohci->at_request_local.work); + flush_work(&ohci->at_response_local.work); + for (i = 0; i < ohci->n_ir; ++i) { if (!(ohci->ir_context_mask & BIT(i))) flush_work(&ohci->ir_context_list[i].base.work); @@ -2485,6 +2557,9 @@ static void ohci_disable(struct fw_card *card) at_context_flush(&ohci->at_request_ctx); at_context_flush(&ohci->at_response_ctx); + + at_request_local_work(&ohci->at_request_local.work); + at_response_local_work(&ohci->at_response_local.work); } static int ohci_set_config_rom(struct fw_card *card, @@ -3684,6 +3759,9 @@ static int pci_probe(struct pci_dev *dev, return err; INIT_WORK(&ohci->at_response_ctx.work, ohci_at_context_work); + at_local_init(&ohci->at_request_local, at_request_local_work); + at_local_init(&ohci->at_response_local, at_response_local_work); + reg_write(ohci, OHCI1394_IsoRecvIntMaskSet, ~0); ohci->ir_context_channels = ~0ULL; ohci->ir_context_support = reg_read(ohci, OHCI1394_IsoRecvIntMaskSet); diff --git a/include/linux/firewire.h b/include/linux/firewire.h index cbe59a18162e..1c71ff69c42f 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -329,6 +329,9 @@ struct fw_packet { bool payload_mapped; u32 timestamp; + // Used to handle the local-to-local packets in the AT request/response contexts. + struct list_head link_for_local; + /* * This callback is called when the packet transmission has completed. * For successful transmission, the status code is the ack received From 8617771f178deabb0a8b1a85b20914eb81458cfb Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 19 Sep 2026 20:50:14 +0900 Subject: [PATCH 32/88] firewire: ohci: use workqueue to handle local AT request/response packets Local-to-local asynchronous transactions are currently handled in the initiator's context. This requires the request handlers to support any context in which the initiator may run. Use workqueue to handle the AT request and response packets targeting local address offsets, so that the request handler always runs in process context. Link: https://lore.kernel.org/r/20260919115017.859169-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 21 +++++++++------------ include/linux/firewire.h | 2 +- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 45f03095a196..dddb08dbb45f 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1546,7 +1546,7 @@ static bool in_bus_management_csr_registers(u64 offset) return in_range(offset, CSR_BUS_MANAGER_ID, 0x22c - CSR_BUS_MANAGER_ID); } -static void handle_local_at_request_packet(struct fw_ohci *ohci, struct fw_packet *packet) +static void handle_at_request_local_packet(struct fw_ohci *ohci, struct fw_packet *packet) { // Emulate split transaction. packet->ack = ACK_PENDING; @@ -1574,7 +1574,7 @@ static void handle_local_at_request_packet(struct fw_ohci *ohci, struct fw_packe } } -static void handle_local_at_response_packet(struct fw_ohci *ohci, struct fw_packet *packet) +static void handle_at_response_local_packet(struct fw_ohci *ohci, struct fw_packet *packet) { u64 csr_offset = async_header_get_offset(packet->header) - CSR_REGISTER_BASE; @@ -1589,7 +1589,8 @@ static void handle_local_at_response_packet(struct fw_ohci *ohci, struct fw_pack packet->callback(packet, &ohci->card, packet->ack); } -static void handle_at_local_packets(struct at_local *local, struct fw_ohci *ohci) +static void handle_at_local_packets(struct at_local *local, struct fw_ohci *ohci, + void (*handle_at_local_packet)(struct fw_ohci *, struct fw_packet *)) { struct fw_packet *packet; @@ -1603,6 +1604,8 @@ static void handle_at_local_packets(struct at_local *local, struct fw_ohci *ohci // This case is active when the call of at_context_queue_packet() returns // error in at_context_transmit(). packet->callback(packet, &ohci->card, packet->ack); + } else { + handle_at_local_packet(ohci, packet); } spin_lock(&local->lock); @@ -1616,7 +1619,7 @@ static void at_request_local_work(struct work_struct *work) struct at_local *local = from_work(local, work, work); struct fw_ohci *ohci = container_of(local, struct fw_ohci, at_request_local); - handle_at_local_packets(local, ohci); + handle_at_local_packets(local, ohci, handle_at_request_local_packet); } static void at_response_local_work(struct work_struct *work) @@ -1624,7 +1627,7 @@ static void at_response_local_work(struct work_struct *work) struct at_local *local = from_work(local, work, work); struct fw_ohci *ohci = container_of(local, struct fw_ohci, at_response_local); - handle_at_local_packets(local, ohci); + handle_at_local_packets(local, ohci, handle_at_response_local_packet); } static void at_local_init(struct at_local *local, work_func_t func) @@ -1672,13 +1675,7 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet if (destination_is_local(packet, ohci)) { spin_unlock_irqrestore(&ohci->lock, flags); - // Timestamping on behalf of the hardware. - packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); - - if (ctx == &ohci->at_request_ctx) - handle_local_at_request_packet(ohci, packet); - else - handle_local_at_response_packet(ohci, packet); + queue_work_for_at_local_packet(ctx, packet, ohci); return; } diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 1c71ff69c42f..2b065f03565d 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -337,7 +337,7 @@ struct fw_packet { * For successful transmission, the status code is the ack received * from the destination. Otherwise it is one of the juju-specific * rcodes: RCODE_SEND_ERROR, _CANCELLED, _BUSY, _GENERATION, _NO_ACK. - * The callback can be called from workqueue and thus must never block. + * The callback is called from a workqueue. It is not preferable to block it so long. */ fw_packet_callback_t callback; int ack; From 137d54d91b20063170e1098ec315e8e18aba8ddc Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 19 Sep 2026 20:50:15 +0900 Subject: [PATCH 33/88] firewire: ohci: refactor branches in at_context_transmit() Refactor the branches to consolidate the handling of local requests and responses, as well as hardware queueing errors. Link: https://lore.kernel.org/r/20260919115017.859169-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index dddb08dbb45f..710640b759e4 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1667,22 +1667,16 @@ static void queue_work_for_at_local_packet(struct at_context *ctx, struct fw_pac static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet) { struct fw_ohci *ohci = ctx->context.ohci; - unsigned long flags; - int ret; + bool use_work = true; - spin_lock_irqsave(&ohci->lock, flags); - - if (destination_is_local(packet, ohci)) { - spin_unlock_irqrestore(&ohci->lock, flags); - - queue_work_for_at_local_packet(ctx, packet, ohci); - return; + scoped_guard(spinlock_irqsave, &ohci->lock) { + if (!destination_is_local(packet, ohci)) { + if (!at_context_queue_packet(ctx, packet)) + use_work = false; + } } - ret = at_context_queue_packet(ctx, packet); - spin_unlock_irqrestore(&ohci->lock, flags); - - if (ret < 0) + if (use_work) queue_work_for_at_local_packet(ctx, packet, ohci); } From d1573b0d907a75420aafa4708faf154743cd097d Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 19 Sep 2026 20:50:16 +0900 Subject: [PATCH 34/88] firewire: core: use spinlock without irqsave for card split_timeout member Asynchronous packet callbacks are now always invoked in process context. Therefore, the split_timeout member is not accessed from IRQ context. Use spin_lock() for the split_timeout member since disabling local IRQs is unnecessary. Link: https://lore.kernel.org/r/20260919115017.859169-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 2d64ab5b0ab0..20d15aec4c40 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -208,9 +208,7 @@ static void transmit_complete_callback(struct fw_packet *packet, { unsigned int delta; - // 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) { + scoped_guard(spinlock, &card->split_timeout.lock) { t->split_timeout_cycle = compute_split_timeout_timestamp(card, packet->timestamp) & 0xffff; delta = card->split_timeout.jiffies; @@ -904,9 +902,7 @@ 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) + scoped_guard(spinlock, &card->split_timeout.lock) request->response.timestamp = compute_split_timeout_timestamp(card, p->timestamp); request->response.speed = p->speed; @@ -1344,10 +1340,7 @@ static void handle_registers(struct fw_card *card, struct fw_request *request, if (tcode == TCODE_READ_QUADLET_REQUEST) { *data = cpu_to_be32(card->split_timeout.hi); } else if (tcode == TCODE_WRITE_QUADLET_REQUEST) { - // 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) { + scoped_guard(spinlock, &card->split_timeout.lock) { card->split_timeout.hi = be32_to_cpu(*data) & 7; update_split_timeout(card); } @@ -1360,10 +1353,7 @@ static void handle_registers(struct fw_card *card, struct fw_request *request, if (tcode == TCODE_READ_QUADLET_REQUEST) { *data = cpu_to_be32(card->split_timeout.lo); } else if (tcode == TCODE_WRITE_QUADLET_REQUEST) { - // 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) { + scoped_guard(spinlock, &card->split_timeout.lock) { card->split_timeout.lo = be32_to_cpu(*data) & 0xfff80000; update_split_timeout(card); } From 00bbf49783b0107f3b69f35ecebf6afbeaae3f1f Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 19 Sep 2026 20:50:17 +0900 Subject: [PATCH 35/88] firewire: core: use spinlock without irqsave for card topology_map member Asynchronous packet callbacks are now always invoked in process context. Therefore, the topology_map member is not accessed from IRQ context. Use spin_lock() for the topology_map member since disabling local IRQs is unnecessary. Link: https://lore.kernel.org/r/20260919115017.859169-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 20d15aec4c40..9dc9f0da0bd8 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -1260,9 +1260,7 @@ static void handle_topology_map(struct fw_card *card, struct fw_request *request start = (offset - topology_map_region.start) / 4; - // 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) + scoped_guard(spinlock, &card->topology_map.lock) memcpy(payload, &card->topology_map.buffer[start], length); fw_send_response(card, request, RCODE_COMPLETE); From b9d9a226589277aad5d82ce8e922b754766f672d Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 20 Sep 2026 16:14:57 +0900 Subject: [PATCH 36/88] firewire: cdev: use memory barrier to access fw_device members The core code uses a read memory barrier when accessing the generation and node_id members of the fw_device structure. It assumes that the latest value of the former is visible to a processor core when the latest value of the latter is visible, since these members are updated in reverse order with a write memory barrier. However, the function that fills the fw_cdev_event_bus_reset structure does not follow this approach. Use a read memory barrier when accessing these members. Link: https://lore.kernel.org/r/20260920071501.77785-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 664952a67a11..3277eec98d93 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -399,18 +399,21 @@ static ssize_t fw_device_op_read(struct file *file, char __user *buffer, static void fill_bus_reset_event(struct fw_cdev_event_bus_reset *event, struct client *client) { - struct fw_card *card = client->device->card; - - guard(spinlock_irq)(&card->lock); - event->closure = client->bus_reset_closure; event->type = FW_CDEV_EVENT_BUS_RESET; + event->generation = client->device->generation; + smp_rmb(); event->node_id = client->device->node_id; - event->local_node_id = card->local_node->node_id; - event->bm_node_id = card->bm_node_id; - event->irm_node_id = card->irm_node->node_id; - event->root_node_id = card->root_node->node_id; + + struct fw_card *card = client->device->card; + + scoped_guard(spinlock_irq, &card->lock) { + event->local_node_id = card->local_node->node_id; + event->bm_node_id = card->bm_node_id; + event->irm_node_id = card->irm_node->node_id; + event->root_node_id = card->root_node->node_id; + } } static void for_each_client(struct fw_device *device, From 19de2f11fc5cf08c4ee572cd5abc02935a953eec Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 20 Sep 2026 16:14:58 +0900 Subject: [PATCH 37/88] firewire: cdev: refactor using scoped_guard() for get_info ioctl call The scope of scoped_guard() corresponds to the code block, making it easier to understand which code is protected by the locking primitive. Link: https://lore.kernel.org/r/20260920071501.77785-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 3277eec98d93..278ea0ee970c 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -491,7 +491,6 @@ union ioctl_arg { static int ioctl_get_info(struct client *client, union ioctl_arg *arg) { struct fw_cdev_get_info *a = &arg->get_info; - struct fw_cdev_event_bus_reset bus_reset; unsigned long ret = 0; client->version = a->version; @@ -511,16 +510,21 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg) a->rom_length = client->device->config_rom_length * 4; } - guard(mutex)(&client->device->client_list_mutex); + scoped_guard(mutex, &client->device->client_list_mutex) { + client->bus_reset_closure = a->bus_reset_closure; - client->bus_reset_closure = a->bus_reset_closure; - if (a->bus_reset != 0) { - fill_bus_reset_event(&bus_reset, client); - /* unaligned size of bus_reset is 36 bytes */ - ret = copy_to_user(u64_to_uptr(a->bus_reset), &bus_reset, 36); + if (a->bus_reset != 0) { + struct fw_cdev_event_bus_reset bus_reset; + + memset(&bus_reset, 0, sizeof(bus_reset)); + fill_bus_reset_event(&bus_reset, client); + + /* unaligned size of bus_reset is 36 bytes */ + ret = copy_to_user(u64_to_uptr(a->bus_reset), &bus_reset, 36); + } + if (ret == 0 && list_empty(&client->link)) + list_add_tail(&client->link, &client->device->client_list); } - if (ret == 0 && list_empty(&client->link)) - list_add_tail(&client->link, &client->device->client_list); return ret ? -EFAULT : 0; } From 1f6877ccef68ce9fed7625a0de384c3576b803ca Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 20 Sep 2026 16:14:59 +0900 Subject: [PATCH 38/88] firewire: cdev: refactor copy_to_user() in get_info ioctl call A non-zero return value from copy_to_user() is handled by returning -EFAULT. Using scoped_guard() simplifies this handling, since it eliminates the need for a local variable to store the return value and check it after the locking primitive is released. Refactor the code to remove the local variable. Link: https://lore.kernel.org/r/20260920071501.77785-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 278ea0ee970c..ed5ac59507cc 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -491,7 +491,6 @@ union ioctl_arg { static int ioctl_get_info(struct client *client, union ioctl_arg *arg) { struct fw_cdev_get_info *a = &arg->get_info; - unsigned long ret = 0; client->version = a->version; a->version = FW_CDEV_KERNEL_VERSION; @@ -499,12 +498,10 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg) scoped_guard(rwsem_read, &fw_device_rwsem) { if (a->rom != 0) { - size_t want = a->rom_length; - size_t have = client->device->config_rom_length * 4; + size_t length = min_t(size_t, a->rom_length, + client->device->config_rom_length * 4); - ret = copy_to_user(u64_to_uptr(a->rom), client->device->config_rom, - min(want, have)); - if (ret != 0) + if (copy_to_user(u64_to_uptr(a->rom), client->device->config_rom, length)) return -EFAULT; } a->rom_length = client->device->config_rom_length * 4; @@ -520,13 +517,15 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg) fill_bus_reset_event(&bus_reset, client); /* unaligned size of bus_reset is 36 bytes */ - ret = copy_to_user(u64_to_uptr(a->bus_reset), &bus_reset, 36); + if (copy_to_user(u64_to_uptr(a->bus_reset), &bus_reset, 36)) + return -EFAULT; } - if (ret == 0 && list_empty(&client->link)) + + if (list_empty(&client->link)) list_add_tail(&client->link, &client->device->client_list); } - return ret ? -EFAULT : 0; + return 0; } static int add_client_resource(struct client *client, struct client_resource *resource, From 259ee8e2556d9be1b8164bd0fafa9cb62ea6b598 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 20 Sep 2026 16:15:00 +0900 Subject: [PATCH 39/88] firewire: cdev: add annotations and comments about race condition of bus_reset_closure There is a race condition involving the bus_reset_closure member of the fw_cdev_event_bus_reset structure, which was addressed by commit 93b37905f70 ("firewire: cdev: prevent race between first get_info ioctl and bus reset event queuing"). Add code comments and annotations to document it. Link: https://lore.kernel.org/r/20260920071501.77785-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index ed5ac59507cc..3d7217620106 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -398,7 +398,12 @@ static ssize_t fw_device_op_read(struct file *file, char __user *buffer, static void fill_bus_reset_event(struct fw_cdev_event_bus_reset *event, struct client *client) +__must_hold(&client->device->client_list_mutex) { + lockdep_assert_held(&client->device->client_list_mutex); + + // This member is related to the above mutex. In detail, see 93b37905f70 ("firewire: cdev: + // prevent race between first get_info ioctl and bus reset event queuing"). event->closure = client->bus_reset_closure; event->type = FW_CDEV_EVENT_BUS_RESET; @@ -508,6 +513,9 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg) } scoped_guard(mutex, &client->device->client_list_mutex) { + // Coordinate concurrent access to this member with bus reset event handling, see + // 93b37905f70 ("firewire: cdev: prevent race between first get_info ioctl and bus + // reset event queuing"). client->bus_reset_closure = a->bus_reset_closure; if (a->bus_reset != 0) { From ab2a823af3d4bc35785fec56103f59796bd94ec1 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 20 Sep 2026 16:15:01 +0900 Subject: [PATCH 40/88] firewire: cdev: add KUnit test for bus reset event UAPI structure The bus reset event structure exposed through the UAPI has a different size depending on the target architecture. Extra care is required when copying it to/from userspace. Add a KUnit test for the structure to ensure that it has the expected size and offsets. Link: https://lore.kernel.org/r/20260920071501.77785-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 5 ++++- drivers/firewire/uapi-test.c | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 3d7217620106..398bebaad064 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -524,7 +524,10 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg) memset(&bus_reset, 0, sizeof(bus_reset)); fill_bus_reset_event(&bus_reset, client); - /* unaligned size of bus_reset is 36 bytes */ + // This structure has 4 bytes of trailing padding under the System V ABI + // on most architectures (due to 8-byte alignment of the long long type), + // except for Intel386 (where long long type is aligned to 4 bytes). In + // either case, the effective length is 36 bytes. if (copy_to_user(u64_to_uptr(a->bus_reset), &bus_reset, 36)) return -EFAULT; } diff --git a/drivers/firewire/uapi-test.c b/drivers/firewire/uapi-test.c index bc3f10a2e516..0bdacb1caeb8 100644 --- a/drivers/firewire/uapi-test.c +++ b/drivers/firewire/uapi-test.c @@ -72,11 +72,31 @@ static void structure_layout_event_phy_packet2(struct kunit *test) KUNIT_EXPECT_EQ(test, 24, offsetof(struct fw_cdev_event_phy_packet2, data)); } +static void structure_layout_event_bus_reset(struct kunit *test) +{ +#if defined(CONFIG_X86_32) + // 4 bytes alignment for aggregate type including 8 bytes storage types. + KUNIT_EXPECT_EQ(test, 36, sizeof(struct fw_cdev_event_bus_reset)); +#else + KUNIT_EXPECT_EQ(test, 40, sizeof(struct fw_cdev_event_bus_reset)); +#endif + + KUNIT_EXPECT_EQ(test, 0, offsetof(struct fw_cdev_event_bus_reset, closure)); + KUNIT_EXPECT_EQ(test, 8, offsetof(struct fw_cdev_event_bus_reset, type)); + KUNIT_EXPECT_EQ(test, 12, offsetof(struct fw_cdev_event_bus_reset, node_id)); + KUNIT_EXPECT_EQ(test, 16, offsetof(struct fw_cdev_event_bus_reset, local_node_id)); + KUNIT_EXPECT_EQ(test, 20, offsetof(struct fw_cdev_event_bus_reset, bm_node_id)); + KUNIT_EXPECT_EQ(test, 24, offsetof(struct fw_cdev_event_bus_reset, irm_node_id)); + KUNIT_EXPECT_EQ(test, 28, offsetof(struct fw_cdev_event_bus_reset, root_node_id)); + KUNIT_EXPECT_EQ(test, 32, offsetof(struct fw_cdev_event_bus_reset, generation)); +} + static struct kunit_case structure_layout_test_cases[] = { KUNIT_CASE(structure_layout_event_response), KUNIT_CASE(structure_layout_event_request3), KUNIT_CASE(structure_layout_event_response2), KUNIT_CASE(structure_layout_event_phy_packet2), + KUNIT_CASE(structure_layout_event_bus_reset), {} }; From e64f2d4de1f344b84c0fa0bebf3102a8fac48b33 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 20 Sep 2026 19:47:45 +0900 Subject: [PATCH 41/88] firewire: core: consolidate lock scopes for event waiting in dequeue function Current implementation uses the spinlock in the card structure to serialize accesses to the event list. The dequeue_event() function yields the CPU while waiting for the condition to become true, but checks the list without acquiring the spinlock. It acquires the spinlock when operating the list, however it assumes that the list has at least one entry. This can cause problems when multiple threads execute read(2) system calls concurrently, since the list can become empty after rescheduling but before the list operation, depending on the scheduling order of the two threads. Use a wait_event variant that reacquires the lock after being rescheduled, before checking the condition. Link: https://lore.kernel.org/r/20260920104745.212753-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 398bebaad064..1b5d75f7e075 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -356,19 +356,27 @@ static int dequeue_event(struct client *client, size_t size, total; int i, ret; - ret = wait_event_interruptible(client->wait, - !list_empty(&client->event_list) || - fw_device_is_shutdown(client->device)); - if (ret < 0) - return ret; + // After the following block, the event pointer above is guaranteed to have a correct value. + { + spin_lock_irq(&client->lock); - if (list_empty(&client->event_list) && - fw_device_is_shutdown(client->device)) - return -ENODEV; + int ret = wait_event_interruptible_lock_irq(client->wait, + !list_empty(&client->event_list) || fw_device_is_shutdown(client->device), + client->lock); + if (ret < 0) { + spin_unlock_irq(&client->lock); + return ret; + } + + if (fw_device_is_shutdown(client->device)) { + spin_unlock_irq(&client->lock); + return -ENODEV; + } - scoped_guard(spinlock_irq, &client->lock) { event = list_first_entry(&client->event_list, struct event, link); list_del(&event->link); + + spin_unlock_irq(&client->lock); } total = 0; From 567a656d929c3d5c07ab70eab4aa62cc76059e6b Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 20 Sep 2026 17:36:55 +0900 Subject: [PATCH 42/88] firewire: core: fulfill kerneldoc for address handler The callback for the address handler is now invoked in process context. Update the kerneldoc for the address handler accordingly. Link: https://lore.kernel.org/r/20260920083659.191855-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 8 +++----- include/linux/firewire.h | 22 +++++++++++++++------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 9dc9f0da0bd8..ae37377ef65a 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -641,16 +641,14 @@ static int put_address_handler(struct fw_address_handler *handler) * * When a request is received that falls within the specified address range, the specified callback * is invoked. The parameters passed to the callback give the details of the particular request. - * The callback is invoked in the workqueue context in most cases. However, if the request is - * initiated by the local node, the callback is invoked in the initiator's context. - * - * To be called in process context. - * Return value: 0 on success, non-zero otherwise. * * The start offset of the handler's address region is determined by * fw_core_add_address_handler() and is returned in handler->offset. * * Address allocations are exclusive, except for the FCP registers. + * + * Context: Process context. + * Returns: 0 on success, non-zero otherwise. */ int fw_core_add_address_handler(struct fw_address_handler *handler, const struct fw_address_region *region) diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 2b065f03565d..4bca5af6793d 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -298,17 +298,25 @@ union fw_transaction_callback { fw_transaction_callback_with_tstamp_t with_tstamp; }; -/* - * This callback handles an inbound request subaction. If the request subaction is initiated from - * the local node (e.g. by unit driver), the execution context depends on the initiator and is - * unspecified. Otherwise, it runs in workqueue context. +/** + * typedef fw_address_callback_t - Function to handle the request of the asynchronous transaction. + * @card: the card instance which receives the request + * @request: the request instance. + * @tcode: the transaction code + * @destination: the destination node ID + * @source: the source node ID + * @generation: the bus generation in which the request was sent + * @offset: the destination offset in source node. + * @data: the request content if available. + * @length: the length of data. + * @callback_data: the data registered with this function. * - * The callback should not initiate outbound request subactions directly. - * Otherwise there is a danger of recursion of inbound and outbound - * transactions from and to the local node. + * This callback handles an inbound request subaction. * * The callback is responsible that fw_send_response() is called on the @request, except for FCP * registers for which the core takes care of that. + * + * Context: Process context. */ typedef void (*fw_address_callback_t)(struct fw_card *card, struct fw_request *request, From a520ce89fcf838acce1c8b2d52f02825d0d37796 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 20 Sep 2026 17:36:56 +0900 Subject: [PATCH 43/88] firewire: cdev: use mutex for phy receiver list In the cdev layer, the list of phy packet receivers is currently protected by a spinlock. This requires GFP_ATOMIC for memory allocation when enumerating the list in fw_cdev_handle_phy_packet(). However, the function is guaranteed to be invoked in process context, so GFP_KERNEL can be used by switching from the spinlock to a mutex. Link: https://lore.kernel.org/r/20260920083659.191855-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 1b5d75f7e075..b211396e592a 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -47,7 +47,7 @@ #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 DEFINE_MUTEX(phy_receiver_list_mutex); static LIST_HEAD(phy_receiver_list); struct client { @@ -1761,9 +1761,7 @@ static int ioctl_receive_phy_packets(struct client *client, union ioctl_arg *arg if (!client->device->is_local) return -ENOSYS; - // 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) + scoped_guard(mutex, &phy_receiver_list_mutex) list_move_tail(&client->phy_receiver_link, &phy_receiver_list); client->phy_receiver_closure = a->closure; @@ -1775,18 +1773,14 @@ void fw_cdev_handle_phy_packet(struct fw_card *card, struct fw_packet *p) { struct client *client; - // 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); + guard(mutex)(&phy_receiver_list_mutex); 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) + struct inbound_phy_packet_event *e = kmalloc(sizeof(*e) + 8, GFP_KERNEL); + if (!e) break; if (client->version < FW_CDEV_VERSION_EVENT_ASYNC_TSTAMP) { @@ -1953,9 +1947,7 @@ static int fw_device_op_release(struct inode *inode, struct file *file) struct client_resource *resource; unsigned long index; - // 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) + scoped_guard(mutex, &phy_receiver_list_mutex) list_del(&client->phy_receiver_link); scoped_guard(mutex, &client->device->client_list_mutex) From 0958dcb1e5e1c036b58e629e35327a4d157c65bc Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 20 Sep 2026 17:36:57 +0900 Subject: [PATCH 44/88] firewire: cdev: use GFP_KERNEL in address handler The callback for the address handler is now guaranteed to be invoked in process context. In the cdev layer, GFP_ATOMIC is still used for memory allocation in the address handler. Use GFP_KERNEL instead. Link: https://lore.kernel.org/r/20260920083659.191855-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index b211396e592a..bff4560fefe9 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -785,8 +785,8 @@ static void handle_request(struct fw_card *card, struct fw_request *request, if (is_fcp) fw_request_get(request); - r = kmalloc_obj(*r, GFP_ATOMIC); - e = kmalloc_obj(*e, GFP_ATOMIC); + r = kmalloc_obj(*r); + e = kmalloc_obj(*e); if (r == NULL || e == NULL) goto failed; @@ -797,7 +797,7 @@ static void handle_request(struct fw_card *card, struct fw_request *request, r->length = length; r->resource.release = release_request; - ret = add_client_resource(handler->client, &r->resource, GFP_ATOMIC); + ret = add_client_resource(handler->client, &r->resource, GFP_KERNEL); if (ret < 0) goto failed; From 4e3f598122d842910b6d1341a1357566a6d85e2e Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 20 Sep 2026 17:36:58 +0900 Subject: [PATCH 45/88] firewire: cdev: refactor add_client_resource() to drop GFP flags argument The add_client_resource() function has an argument for GFP flags. The argument was originally used to distinguish atomic calls. However, all callers now use GFP_KERNEL. Drop the unused argument and always allocate with GFP_KERNEL. Link: https://lore.kernel.org/r/20260920083659.191855-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index bff4560fefe9..216f8b6ec324 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -547,8 +547,7 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg) return 0; } -static int add_client_resource(struct client *client, struct client_resource *resource, - gfp_t gfp_mask) +static int add_client_resource(struct client *client, struct client_resource *resource) { scoped_guard(spinlock_irqsave, &client->lock) { u32 index; @@ -557,13 +556,7 @@ static int add_client_resource(struct client *client, struct client_resource *re if (client->in_shutdown) return -ECANCELED; - if (gfpflags_allow_blocking(gfp_mask)) { - ret = xa_alloc(&client->resource_xa, &index, resource, xa_limit_32b, - GFP_NOWAIT); - } else { - ret = xa_alloc_bh(&client->resource_xa, &index, resource, - xa_limit_32b, GFP_NOWAIT); - } + ret = xa_alloc(&client->resource_xa, &index, resource, xa_limit_32b, GFP_KERNEL); if (ret < 0) return ret; @@ -713,7 +706,7 @@ static int init_request(struct client *client, } e->r.resource.release = release_transaction; - ret = add_client_resource(client, &e->r.resource, GFP_KERNEL); + ret = add_client_resource(client, &e->r.resource); if (ret < 0) goto failed; @@ -797,7 +790,7 @@ static void handle_request(struct fw_card *card, struct fw_request *request, r->length = length; r->resource.release = release_request; - ret = add_client_resource(handler->client, &r->resource, GFP_KERNEL); + ret = add_client_resource(handler->client, &r->resource); if (ret < 0) goto failed; @@ -901,7 +894,7 @@ static int ioctl_allocate(struct client *client, union ioctl_arg *arg) a->offset = r->handler.offset; r->resource.release = release_address_handler; - ret = add_client_resource(client, &r->resource, GFP_KERNEL); + ret = add_client_resource(client, &r->resource); if (ret < 0) { release_address_handler(client, &r->resource); return ret; @@ -1001,7 +994,7 @@ static int ioctl_add_descriptor(struct client *client, union ioctl_arg *arg) goto failed; r->resource.release = release_descriptor; - ret = add_client_resource(client, &r->resource, GFP_KERNEL); + ret = add_client_resource(client, &r->resource); if (ret < 0) { fw_core_remove_descriptor(&r->descriptor); goto failed; @@ -1489,7 +1482,7 @@ static int ioctl_allocate_iso_resource(struct client *client, union ioctl_arg *a e2->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED; r->resource.release = release_iso_resource_auto; - err = add_client_resource(client, &r->resource, GFP_KERNEL); + err = add_client_resource(client, &r->resource); if (err < 0) return err; request->handle = r->resource.handle; From e079effb35100b94dd9d50e9daf7cbc41317717f Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 20 Sep 2026 17:36:59 +0900 Subject: [PATCH 46/88] firewire: cdev: refactor add_client_resource() to have release callback function The implementation of the client_resource structure now requires a release callback. Add an argument to add_client_resource() for the release callback. Link: https://lore.kernel.org/r/20260920083659.191855-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 216f8b6ec324..6a2e28b71ba9 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -547,7 +547,8 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg) return 0; } -static int add_client_resource(struct client *client, struct client_resource *resource) +static int add_client_resource(struct client *client, struct client_resource *resource, + client_resource_release_fn_t release) { scoped_guard(spinlock_irqsave, &client->lock) { u32 index; @@ -561,6 +562,7 @@ static int add_client_resource(struct client *client, struct client_resource *re return ret; resource->handle = index; + resource->release = release; client_get(client); } @@ -705,8 +707,7 @@ static int init_request(struct client *client, goto failed; } - e->r.resource.release = release_transaction; - ret = add_client_resource(client, &e->r.resource); + ret = add_client_resource(client, &e->r.resource, release_transaction); if (ret < 0) goto failed; @@ -789,8 +790,7 @@ static void handle_request(struct fw_card *card, struct fw_request *request, r->data = payload; r->length = length; - r->resource.release = release_request; - ret = add_client_resource(handler->client, &r->resource); + ret = add_client_resource(handler->client, &r->resource, release_request); if (ret < 0) goto failed; @@ -893,8 +893,7 @@ static int ioctl_allocate(struct client *client, union ioctl_arg *arg) } a->offset = r->handler.offset; - r->resource.release = release_address_handler; - ret = add_client_resource(client, &r->resource); + ret = add_client_resource(client, &r->resource, release_address_handler); if (ret < 0) { release_address_handler(client, &r->resource); return ret; @@ -993,8 +992,7 @@ static int ioctl_add_descriptor(struct client *client, union ioctl_arg *arg) if (ret < 0) goto failed; - r->resource.release = release_descriptor; - ret = add_client_resource(client, &r->resource); + ret = add_client_resource(client, &r->resource, release_descriptor); if (ret < 0) { fw_core_remove_descriptor(&r->descriptor); goto failed; @@ -1481,8 +1479,7 @@ static int ioctl_allocate_iso_resource(struct client *client, union ioctl_arg *a e2->iso_resource.closure = request->closure; e2->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED; - r->resource.release = release_iso_resource_auto; - err = add_client_resource(client, &r->resource); + err = add_client_resource(client, &r->resource, release_iso_resource_auto); if (err < 0) return err; request->handle = r->resource.handle; From 4e7cbc6c1421682f4523bd411d1cebed326f7e31 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 21 Sep 2026 12:19:29 +0900 Subject: [PATCH 47/88] firewire: core: refactor invoking transaction callback Two types of transaction callbacks are defined. There are many places where the transaction callback is invoked with conditional branches. Add a helper function to simplify invoking the callback. Link: https://lore.kernel.org/r/20260921031931.287429-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 69 ++++++++++++----------------- 1 file changed, 28 insertions(+), 41 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index ae37377ef65a..00291042f48e 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -52,6 +52,17 @@ static void remove_transaction_entry(struct fw_card *card, struct fw_transaction card->transactions.tlabel_mask &= ~(1ULL << entry->tlabel); } +static void invoke_callback(struct fw_transaction *t, int rcode, u32 response_tstamp, void *data, + size_t data_length) +{ + if (!t->with_tstamp) { + t->callback.without_tstamp(t->card, rcode, data, data_length, t->callback_data); + } else { + t->callback.with_tstamp(t->card, rcode, t->packet.timestamp, response_tstamp, + data, data_length, t->callback_data); + } +} + // Must be called without holding card->transactions.lock. void fw_cancel_pending_transactions(struct fw_card *card) { @@ -69,14 +80,7 @@ void fw_cancel_pending_transactions(struct fw_card *card) list_for_each_entry_safe(t, tmp, &pending_list, link) { list_del(&t->link); - - if (!t->with_tstamp) { - t->callback.without_tstamp(card, RCODE_CANCELLED, NULL, 0, - t->callback_data); - } else { - t->callback.with_tstamp(card, RCODE_CANCELLED, t->packet.timestamp, 0, - NULL, 0, t->callback_data); - } + invoke_callback(t, RCODE_CANCELLED, 0, NULL, 0); } } @@ -112,12 +116,7 @@ static int close_transaction(struct fw_transaction *transaction, struct fw_card return -ENOENT; } - if (!t->with_tstamp) { - t->callback.without_tstamp(card, rcode, NULL, 0, t->callback_data); - } else { - t->callback.with_tstamp(card, rcode, t->packet.timestamp, response_tstamp, NULL, 0, - t->callback_data); - } + invoke_callback(t, rcode, response_tstamp, NULL, 0); return 0; } @@ -170,12 +169,7 @@ static void split_transaction_timeout_callback(struct timer_list *timer) remove_transaction_entry(card, t); } - if (!t->with_tstamp) { - t->callback.without_tstamp(card, RCODE_CANCELLED, NULL, 0, t->callback_data); - } else { - t->callback.with_tstamp(card, RCODE_CANCELLED, t->packet.timestamp, - t->split_timeout_cycle, NULL, 0, t->callback_data); - } + invoke_callback(t, RCODE_CANCELLED, t->split_timeout_cycle, NULL, 0); } // card->transactions.lock should be acquired in advance for the linked list. @@ -389,6 +383,11 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode { int tlabel; + t->card = card; + t->callback = callback; + t->with_tstamp = with_tstamp; + t->callback_data = callback_data; + /* * Allocate tlabel from the bitmap and put the transaction on * the list while holding the card spinlock. @@ -399,30 +398,23 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode scoped_guard(spinlock_irqsave, &card->transactions.lock) tlabel = allocate_tlabel(card); if (tlabel < 0) { - if (!with_tstamp) { - callback.without_tstamp(card, RCODE_SEND_ERROR, NULL, 0, callback_data); - } else { - // Timestamping on behalf of hardware. - u32 curr_cycle_time = 0; - u32 tstamp; + // Timestamping on behalf of hardware. + u32 curr_cycle_time = 0; + u32 tstamp; - (void)fw_card_read_cycle_time(card, &curr_cycle_time); - tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); + (void)fw_card_read_cycle_time(card, &curr_cycle_time); + tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); + + t->packet.timestamp = tstamp; + invoke_callback(t, RCODE_SEND_ERROR, tstamp, NULL, 0); - callback.with_tstamp(card, RCODE_SEND_ERROR, tstamp, tstamp, NULL, 0, - callback_data); - } return; } t->node_id = destination_id; t->tlabel = tlabel; - t->card = card; t->is_split_transaction = false; timer_setup(&t->split_timeout_timer, split_transaction_timeout_callback, 0); - t->callback = callback; - t->with_tstamp = with_tstamp; - t->callback_data = callback_data; t->packet.callback = transmit_complete_callback; // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for @@ -1200,12 +1192,7 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p) */ card->driver->cancel_packet(card, &t->packet); - if (!t->with_tstamp) { - t->callback.without_tstamp(card, rcode, data, data_length, t->callback_data); - } else { - t->callback.with_tstamp(card, rcode, t->packet.timestamp, p->timestamp, data, - data_length, t->callback_data); - } + invoke_callback(t, rcode, p->timestamp, data, data_length); } EXPORT_SYMBOL(fw_core_handle_response); From 382ec2940670c273b297a6803333237d5f4a90f5 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 21 Sep 2026 12:19:30 +0900 Subject: [PATCH 48/88] firewire: core: use workqueue to invoke transaction callback in some error cases Some error paths of __fw_send_request() invoke the transaction callback in the caller's context. Additionally, when a split transaction times out, the callback is invoked in softIRQ context by the timer wheel. These are the only cases where the callback is not guaranteed to be invoked in process context. Use a workqueue to invoke the callback in these cases. This may introduce additional delay when a split transaction times out, but the default timeout is 2 seconds, so the additional delay should be negligible. Link: https://lore.kernel.org/r/20260921031931.287429-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 22 +++++++++++++++++++--- include/linux/firewire.h | 5 +++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 00291042f48e..de80dd160363 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -40,7 +40,7 @@ static int try_cancel_split_timeout(struct fw_transaction *t) { if (t->is_split_transaction) - return timer_delete(&t->split_timeout_timer); + return timer_delete(&t->split_timeout_timer) || disable_work(&t->error_work); else return 1; } @@ -158,6 +158,21 @@ int fw_cancel_transaction(struct fw_card *card, } EXPORT_SYMBOL(fw_cancel_transaction); +static void error_callback_work(struct work_struct *work) +{ + struct fw_transaction *t = from_work(t, work, error_work); + + invoke_callback(t, t->rcode, t->response_timestamp, NULL, 0); +} + +static void schedule_error_callback(struct fw_transaction *t, int rcode, u32 response_timestamp) +{ + t->rcode = rcode; + t->response_timestamp = response_timestamp; + + queue_work(t->card->async_wq, &t->error_work); +} + static void split_transaction_timeout_callback(struct timer_list *timer) { struct fw_transaction *t = timer_container_of(t, timer, split_timeout_timer); @@ -169,7 +184,7 @@ static void split_transaction_timeout_callback(struct timer_list *timer) remove_transaction_entry(card, t); } - invoke_callback(t, RCODE_CANCELLED, t->split_timeout_cycle, NULL, 0); + schedule_error_callback(t, RCODE_CANCELLED, t->split_timeout_cycle); } // card->transactions.lock should be acquired in advance for the linked list. @@ -387,6 +402,7 @@ 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; + INIT_WORK(&t->error_work, error_callback_work); /* * Allocate tlabel from the bitmap and put the transaction on @@ -406,7 +422,7 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); t->packet.timestamp = tstamp; - invoke_callback(t, RCODE_SEND_ERROR, tstamp, NULL, 0); + schedule_error_callback(t, RCODE_SEND_ERROR, tstamp); return; } diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 4bca5af6793d..a0a41a320e46 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -370,6 +370,11 @@ struct fw_transaction { union fw_transaction_callback callback; bool with_tstamp; void *callback_data; + + // For some error cases. + struct work_struct error_work; + int rcode; + u32 response_timestamp; }; struct fw_address_handler { From f95344968c5d3b4c7cbd0e1c80462540f804f278 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 21 Sep 2026 12:19:31 +0900 Subject: [PATCH 49/88] firewire: core: update kerneldoc for fw_send_request() and its variants Some error paths invoke the transaction callback in process context using a workqueue. All transaction callbacks are now invoked in process context. Update kerneldoc to reflect this behavior. Link: https://lore.kernel.org/r/20260921031931.287429-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 4 ++-- include/linux/firewire.h | 10 ++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index de80dd160363..cefa92812361 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -378,8 +378,8 @@ __must_hold(&card->transactions.lock) * * In case of request types without payload, @data is NULL and @length is 0. * - * After the transaction is completed successfully or unsuccessfully, the - * @callback will be called. Among its parameters is the response code which + * After the transaction is completed successfully or unsuccessfully, the @callback will be called + * in process context. Among its parameters is the response code which * is either one of the rcodes per IEEE 1394 or, in case of internal errors, * the firewire-core specific %RCODE_SEND_ERROR. The other firewire-core * specific rcodes (%RCODE_CANCELLED, %RCODE_BUSY, %RCODE_GENERATION, diff --git a/include/linux/firewire.h b/include/linux/firewire.h index a0a41a320e46..91a89e812832 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -427,9 +427,8 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode * A variation of __fw_send_request() to generate callback for response subaction without time * stamp. * - * The callback is invoked in the workqueue context in most cases. However, if an error is detected - * before queueing or the destination address refers to the local node, it is invoked in the - * current context instead. + * After the transaction is completed successfully or unsuccessfully, the @callback will be called + * in process context. */ static inline void fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode, int destination_id, int generation, int speed, @@ -460,9 +459,8 @@ static inline void fw_send_request(struct fw_card *card, struct fw_transaction * * * A variation of __fw_send_request() to generate callback for response subaction with time stamp. * - * The callback is invoked in the workqueue context in most cases. However, if an error is detected - * before queueing or the destination address refers to the local node, it is invoked in the current - * context instead. + * After the transaction is completed successfully or unsuccessfully, the @callback will be called + * in process context. */ static inline void fw_send_request_with_tstamp(struct fw_card *card, struct fw_transaction *t, int tcode, int destination_id, int generation, int speed, unsigned long long offset, From a689a10b519f98cd119c5ed8534a851e2a92f197 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Thu, 24 Sep 2026 02:23:27 +0900 Subject: [PATCH 50/88] firewire: cdev: hold client reference for fw_iso_resource_auto lifetime The fw_iso_resource_auto object can outlive the operation that created it since hte main operation for it is done in a work item. This work and release paths access members of the client structure, therefore the client structure must remain valid for the lifetime of an fw_iso_resource_auto object. Hold a reference to the client structure for the lifetime of the fw_iso_resource_auto object. Link: https://lore.kernel.org/r/20260923172328.277232-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 6a2e28b71ba9..0f1415a5eb55 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -1397,6 +1397,7 @@ static void iso_resource_auto_work(struct work_struct *work) // xarray and prepare for deletion, unless the client is shutting down. scoped_guard(spinlock_irq, &client->lock) { if (!client->in_shutdown && xa_erase(&client->resource_xa, index)) { + // For the incrementation by add_client_resource(). client_put(client); free = true; } @@ -1438,6 +1439,9 @@ static void iso_resource_auto_work(struct work_struct *work) kfree(r->e_alloc); kfree(r->e_dealloc); kfree(r); + + // For the incrementation by ioctl_allocate_iso_resource(). + client_put(client); } out: client_put(client); @@ -1483,6 +1487,7 @@ static int ioctl_allocate_iso_resource(struct client *client, union ioctl_arg *a if (err < 0) return err; request->handle = r->resource.handle; + client_get(client); retain_and_null_ptr(e1); retain_and_null_ptr(e2); From 76ef77d7e6cba02cb7b691e874389e3333ece969 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Thu, 24 Sep 2026 02:23:28 +0900 Subject: [PATCH 51/88] firewire: cdev: fix client refcount leak in iso_resource_auto_work() The client reference leaks when the pending work is cancelled because an additional reference was taken when scheduling the work. The reference held by the fw_iso_resource_auto object already ensures that the client structure remains valid for the lifetime of the object. Therefore, the additional reference taken when scheduling the work is unnecessary. Remove the additional reference. Reported-by: Dingisoul Link: https://sourceforge.net/p/linux1394/mailman/message/59317811/ Link: https://lore.kernel.org/r/20260923172328.277232-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 0f1415a5eb55..eafb99389a69 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -196,9 +196,7 @@ static int is_outbound_transaction_resource(const struct client_resource *resour static void schedule_iso_resource_auto(struct iso_resource_auto *r, unsigned long delay) { - client_get(r->client); - if (!queue_delayed_work(fw_workqueue, &r->work, delay)) - client_put(r->client); + queue_delayed_work(fw_workqueue, &r->work, delay); } /* @@ -1360,13 +1358,13 @@ static void iso_resource_auto_work(struct work_struct *work) // Allow 1000ms grace period for other reallocations. if (time_is_after_jiffies64(reset_jiffies + secs_to_jiffies(1))) { schedule_iso_resource_auto(r, msecs_to_jiffies(333)); - goto out; + return; } break; case ISO_RES_AUTO_REALLOC: // We could be called twice within the same generation. if (resource_generation == current_generation) - goto out; + return; break; case ISO_RES_AUTO_DEALLOC: default: @@ -1388,7 +1386,7 @@ static void iso_resource_auto_work(struct work_struct *work) // Is this generation outdated already? As long as this resource sticks in the // xarray, it will be scheduled again for a newer generation or at shutdown. if (channel == -EAGAIN) - goto out; + return; bool success = channel >= 0 || bandwidth > 0; @@ -1406,7 +1404,7 @@ static void iso_resource_auto_work(struct work_struct *work) if (todo == ISO_RES_AUTO_REALLOC) { if (success) - goto out; + return; // Notify the userspace client of the failure through a deallocation event. e = r->e_dealloc; @@ -1443,8 +1441,6 @@ static void iso_resource_auto_work(struct work_struct *work) // For the incrementation by ioctl_allocate_iso_resource(). client_put(client); } - out: - client_put(client); } static void release_iso_resource_auto(struct client *client, struct client_resource *resource) From a1eae4befca272c238b6e832bbf27f49a5e03825 Mon Sep 17 00:00:00 2001 From: Bill Wendling Date: Fri, 25 Sep 2026 21:09:12 +0000 Subject: [PATCH 52/88] firewire: core: add __counted_by_ptr to struct fw_packet In 'struct fw_packet', the 'payload' field points to a buffer of size 'payload_length' bytes. To enable compiler bounds checking (via KASAN and __builtin_dynamic_object_size), add the '__counted_by_ptr' attribute to the 'payload' pointer, associated with the 'payload_length' count. A thorough analysis of all allocation and initialization points for 'struct fw_packet' was conducted. The pointer and its length are assigned together. Because the count field is always correctly initialized before the pointer is accessed, the '__counted_by_ptr' attribute is fully safe and will not cause runtime false positives or panics. Cc: codemender-patching+linux@google.com Assisted-by: LLM Signed-off-by: Bill Wendling Link: https://lore.kernel.org/r/20260925210912.113052-1-morbo@google.com Signed-off-by: Takashi Sakamoto --- include/linux/firewire.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 91a89e812832..9d5a42724f1a 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -331,7 +331,7 @@ struct fw_packet { int generation; u32 header[4]; size_t header_length; - void *payload; + void *payload __counted_by_ptr(payload_length); size_t payload_length; dma_addr_t payload_bus; bool payload_mapped; From 39e505c38ffcc845ea58259573ff34c6d98e36bb Mon Sep 17 00:00:00 2001 From: Bill Wendling Date: Fri, 25 Sep 2026 21:25:52 +0000 Subject: [PATCH 53/88] firewire: core: add __counted_by_ptr attribute to struct fw_device The GCC and Clang attribute '__counted_by_ptr' associates a pointer field with an integer field holding its element count. This enables runtime bounds checking by KASAN and UBSAN to prevent out-of-bounds accesses to the pointer. In 'struct fw_device' (defined in 'include/linux/firewire.h'), the field 'config_rom' is a pointer to the device's Configuration ROM data, and its associated element count is stored in 'config_rom_length'. Additionally, update the existing KUnit test in 'drivers/firewire/device-attribute-test.c' where 'config_rom_length' was incorrectly initialized with the byte size ('sizeof(simple_avc_config_rom)') instead of the element count ('ARRAY_SIZE(simple_avc_config_rom)'). This ensures the '__counted_by_ptr' bounds-checking annotation does not trigger any false-positive panics or compile/runtime checks. Cc: codemender-patching+linux@google.com Assisted-by: LLM Signed-off-by: Bill Wendling Link: https://lore.kernel.org/r/20260925212552.125652-1-morbo@google.com Signed-off-by: Takashi Sakamoto --- drivers/firewire/device-attribute-test.c | 4 ++-- include/linux/firewire.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/firewire/device-attribute-test.c b/drivers/firewire/device-attribute-test.c index 97478a96d1c9..2c482afca8f1 100644 --- a/drivers/firewire/device-attribute-test.c +++ b/drivers/firewire/device-attribute-test.c @@ -86,7 +86,7 @@ static void device_attr_simple_avc(struct kunit *test) .type = &fw_device_type, }, .config_rom = simple_avc_config_rom, - .config_rom_length = sizeof(simple_avc_config_rom), + .config_rom_length = ARRAY_SIZE(simple_avc_config_rom), }; static const struct fw_unit unit0 = { .device = { @@ -168,7 +168,7 @@ static void device_attr_legacy_avc(struct kunit *test) .type = &fw_device_type, }, .config_rom = legacy_avc_config_rom, - .config_rom_length = sizeof(legacy_avc_config_rom), + .config_rom_length = ARRAY_SIZE(legacy_avc_config_rom), }; static const struct fw_unit unit0 = { .device = { diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 9d5a42724f1a..dbb5c825a1cc 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -224,7 +224,7 @@ struct fw_device { struct mutex client_list_mutex; struct list_head client_list; - const u32 *config_rom; + const u32 *config_rom __counted_by_ptr(config_rom_length); size_t config_rom_length; int config_rom_retries; unsigned is_local:1; From b362e6dd5c990f9cf1f0a84854b84a9524958098 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 27 Sep 2026 12:43:40 +0900 Subject: [PATCH 54/88] firewire: cdev: remove unnecessary client locking for fw_device members Some accesses to fw_device members unnecessarily use the client-level lock. Remove the locking from these accesses. Link: https://lore.kernel.org/r/20260927034343.28076-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index eafb99389a69..5c05cec0f8f8 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -1341,17 +1341,17 @@ static void iso_resource_auto_work(struct work_struct *work) struct iso_resource_auto *r = from_work(r, work, work.work); struct client *client = r->client; unsigned long index = r->resource.handle; - int current_generation, resource_generation, channel, bandwidth, todo; - u64 reset_jiffies; + int channel, bandwidth, todo; bool free; - scoped_guard(spinlock_irq, &client->lock) { - reset_jiffies = client->device->card->reset_jiffies; - current_generation = client->device->generation; - resource_generation = r->generation; - r->generation = current_generation; + u64 reset_jiffies = client->device->card->reset_jiffies; + int current_generation = client->device->generation; + + int resource_generation = r->generation; + r->generation = current_generation; + + scoped_guard(spinlock_irq, &client->lock) todo = r->todo; - } switch (todo) { case ISO_RES_AUTO_ALLOC: @@ -1506,12 +1506,10 @@ static void iso_resource_once_work(struct work_struct *work) struct iso_resource_once *r = from_work(r, work, work); struct client *client = r->client; struct iso_resource_event *e = r->event; - int generation, channel, bandwidth; + int channel; - scoped_guard(spinlock_irq, &client->lock) - generation = client->device->generation; - - bandwidth = r->params.bandwidth; + int generation = client->device->generation; + int bandwidth = r->params.bandwidth; fw_iso_resource_manage(client->device->card, generation, r->params.channels_mask, &channel, &bandwidth, r->todo == ISO_RES_ONCE_ALLOC); From 0f636cfe8d93e1b36168d9524de44b33c42daaaf Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 27 Sep 2026 12:43:41 +0900 Subject: [PATCH 55/88] firewire: cdev: wake up client from queue_event() only when any event is available The in_shutdown member of client structure is set only when releasing file descriptor, thus no need to wake up the client itself. Link: https://lore.kernel.org/r/20260927034343.28076-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 5c05cec0f8f8..d1188c101e84 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -338,13 +338,14 @@ static void queue_event(struct client *client, struct event *event, event->v[1].size = size1; scoped_guard(spinlock_irqsave, &client->lock) { - if (client->in_shutdown) + if (client->in_shutdown) { kfree(event); - else + } else { list_add_tail(&event->link, &client->event_list); - } - wake_up_interruptible(&client->wait); + wake_up_interruptible(&client->wait); + } + } } static int dequeue_event(struct client *client, From 9cc8efc586ea46128df7d73c17dda10f93c79251 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 27 Sep 2026 12:43:42 +0900 Subject: [PATCH 56/88] firewire: cdev: use xchg() to exchange pointer value xchg() is useful for exchanging a pointer and returning the old value, even when atomicity is not required. Link: https://lore.kernel.org/r/20260927034343.28076-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index d1188c101e84..4e594b6029f4 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -1348,8 +1348,7 @@ static void iso_resource_auto_work(struct work_struct *work) u64 reset_jiffies = client->device->card->reset_jiffies; int current_generation = client->device->generation; - int resource_generation = r->generation; - r->generation = current_generation; + int resource_generation = xchg(&r->generation, current_generation); // But no need to be atomic. scoped_guard(spinlock_irq, &client->lock) todo = r->todo; @@ -1379,8 +1378,7 @@ static void iso_resource_auto_work(struct work_struct *work) if (todo == ISO_RES_AUTO_DEALLOC) { free = true; - e = r->e_dealloc; - r->e_dealloc = NULL; + e = xchg(&r->e_dealloc, NULL); // But no need to be atomic. } else { free = false; @@ -1408,8 +1406,7 @@ static void iso_resource_auto_work(struct work_struct *work) return; // Notify the userspace client of the failure through a deallocation event. - e = r->e_dealloc; - r->e_dealloc = NULL; + e = xchg(&r->e_dealloc, NULL); // But no need to be atomic. } else { // Transit from allocation to reallocation, except if the client requested // deallocation in the meantime. @@ -1421,8 +1418,7 @@ static void iso_resource_auto_work(struct work_struct *work) if (channel >= 0) r->params.channels_mask = BIT_ULL(channel); - e = r->e_alloc; - r->e_alloc = NULL; + e = xchg(&r->e_alloc, NULL); // But no need to be atomic. } } From 981a9c4feb7754815d84974fb1a14ba8cdcace57 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 27 Sep 2026 12:43:43 +0900 Subject: [PATCH 57/88] firewire: cdev: refactor event copying to remove goto statement Arrange the local variables to avoid a goto statement when copying events to the userspace buffer. Link: https://lore.kernel.org/r/20260927034343.28076-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 4e594b6029f4..a62df9034f4f 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -348,12 +348,9 @@ static void queue_event(struct client *client, struct event *event, } } -static int dequeue_event(struct client *client, - char __user *buffer, size_t count) +static ssize_t dequeue_event(struct client *client, char __user *buffer, size_t count) { struct event *event; - size_t size, total; - int i, ret; // After the following block, the event pointer above is guaranteed to have a correct value. { @@ -378,18 +375,17 @@ static int dequeue_event(struct client *client, spin_unlock_irq(&client->lock); } - total = 0; - for (i = 0; i < ARRAY_SIZE(event->v) && total < count; i++) { - size = min(event->v[i].size, count - total); - if (copy_to_user(buffer + total, event->v[i].data, size)) { - ret = -EFAULT; - goto out; - } - total += size; - } - ret = total; + ssize_t ret = 0; + + for (int i = 0; i < ARRAY_SIZE(event->v) && ret < count; i++) { + size_t size = min(event->v[i].size, count - ret); + if (copy_to_user(buffer + ret, event->v[i].data, size)) { + ret = -EFAULT; + break; + } + ret += size; + } - out: kfree(event); return ret; From 87a48a22b3809bac2d745cab84713d1631e28e23 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 27 Sep 2026 13:44:06 +0900 Subject: [PATCH 58/88] firewire: core: remove unnecessary branch for timestamping when cancelling transaction request packets When fw_card_driver.cancel_packet() returns a non-zero value, the request subaction of the transaction has already finished. It means that the packet for the request subaction has already received an ack code and a timestamp, so there is no need to check them. Remove the unnecessary branch for the ack code and timestamp. Link: https://lore.kernel.org/r/20260927044407.65497-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index cefa92812361..88c0bfbe98ad 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -139,21 +139,16 @@ int fw_cancel_transaction(struct fw_card *card, if (card->driver->cancel_packet(card, &transaction->packet) == 0) return 0; + u32 curr_cycle_time = 0; + + // Timestamping on behalf of hardware. + (void)fw_card_read_cycle_time(card, &curr_cycle_time); + tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); + /* * If the request packet has already been sent, we need to see * if the transaction is still pending and remove it in that case. */ - - if (transaction->packet.ack == 0) { - // The timestamp is reused since it was just read now. - tstamp = transaction->packet.timestamp; - } else { - u32 curr_cycle_time = 0; - - (void)fw_card_read_cycle_time(card, &curr_cycle_time); - tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); - } - return close_transaction(transaction, card, RCODE_CANCELLED, tstamp); } EXPORT_SYMBOL(fw_cancel_transaction); From c94caec799b7173bfa2eef0f4cac01872d6d62ba Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 27 Sep 2026 13:44:07 +0900 Subject: [PATCH 59/88] firewire: ohci: run work immediately when re-enabled in packet cancellation During packet cancellation, the work item for the AT request context is disabled. As a result, packets for request subactions are not processed by the work item even if they finished, since the work item can not be scheduled from hardIRQ handling. Run the work item immediately after re-enabling it. Link: https://lore.kernel.org/r/20260927044407.65497-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 710640b759e4..df53efec0b0a 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -2676,7 +2676,7 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet) packet->callback(packet, &ohci->card, packet->ack); ret = 0; out: - enable_work(&ctx->work); + enable_and_queue_work(card->async_wq, &ctx->work); return ret; } From 2b69c5fe9eb319058a9b6767ebee4a4b0e8054bb Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 28 Sep 2026 08:07:23 +0900 Subject: [PATCH 60/88] firewire: cdev: use atomic_t for iso_resource_auto todo member The transition state of an iso_resource_auto client resource has no effect on the client, so the client-level lock is not suitable for protecting it. At present, this state is the only member that requires mutual exclusion. Adding a separate lock for it would be excessive. Change the type of the transition state to atomic_t and use an atomic compare-and-swap operation to preserve consistency. Link: https://lore.kernel.org/r/20260927230724.119305-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index a62df9034f4f..d4c72cd20069 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -133,16 +133,17 @@ struct iso_resource_params { s32 bandwidth; }; +enum { + ISO_RES_AUTO_ALLOC, + ISO_RES_AUTO_REALLOC, + ISO_RES_AUTO_DEALLOC, +}; + struct iso_resource_auto { struct client_resource resource; struct client *client; - /* Schedule work and access todo only with client->lock held. */ struct delayed_work work; - enum { - ISO_RES_AUTO_ALLOC, - ISO_RES_AUTO_REALLOC, - ISO_RES_AUTO_DEALLOC, - } todo; + atomic_t todo; // one of ISO_RES_AUTO_XXX. int generation; struct iso_resource_params params; struct iso_resource_event *e_alloc, *e_dealloc; @@ -1338,16 +1339,14 @@ static void iso_resource_auto_work(struct work_struct *work) struct iso_resource_auto *r = from_work(r, work, work.work); struct client *client = r->client; unsigned long index = r->resource.handle; - int channel, bandwidth, todo; + int channel, bandwidth; bool free; u64 reset_jiffies = client->device->card->reset_jiffies; int current_generation = client->device->generation; int resource_generation = xchg(&r->generation, current_generation); // But no need to be atomic. - - scoped_guard(spinlock_irq, &client->lock) - todo = r->todo; + int todo = atomic_read(&r->todo); switch (todo) { case ISO_RES_AUTO_ALLOC: @@ -1404,12 +1403,10 @@ static void iso_resource_auto_work(struct work_struct *work) // Notify the userspace client of the failure through a deallocation event. e = xchg(&r->e_dealloc, NULL); // But no need to be atomic. } else { - // Transit from allocation to reallocation, except if the client requested - // deallocation in the meantime. - scoped_guard(spinlock_irq, &client->lock) { - if (r->todo == ISO_RES_AUTO_ALLOC) - r->todo = ISO_RES_AUTO_REALLOC; - } + // Transit from allocation to reallocation. Use compare-and-swap atomic + // operation because the todo member can be set with ISO_RES_AUTO_DEALLOC + // by release_iso_resource_auto() in parallel. + atomic_cmpxchg_relaxed(&r->todo, ISO_RES_AUTO_ALLOC, ISO_RES_AUTO_REALLOC); if (channel >= 0) r->params.channels_mask = BIT_ULL(channel); @@ -1440,9 +1437,7 @@ static void release_iso_resource_auto(struct client *client, struct client_resou { struct iso_resource_auto *r = to_iso_resource_auto(resource); - guard(spinlock_irq)(&client->lock); - - r->todo = ISO_RES_AUTO_DEALLOC; + atomic_set(&r->todo, ISO_RES_AUTO_DEALLOC); schedule_iso_resource_auto(r, 0); } @@ -1463,7 +1458,7 @@ static int ioctl_allocate_iso_resource(struct client *client, union ioctl_arg *a INIT_DELAYED_WORK(&r->work, iso_resource_auto_work); r->client = client; - r->todo = ISO_RES_AUTO_ALLOC; + atomic_set(&r->todo, ISO_RES_AUTO_ALLOC); r->e_alloc = e1; r->e_dealloc = e2; From 930960ee371ed37f96b6d4c39617a55a680f3698 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 28 Sep 2026 08:07:24 +0900 Subject: [PATCH 61/88] firewire: cdev: use mutex for client locking All execution paths in the cdev layer now run in process context. Use a mutex instead of a spinlock for client-level locking. Link: https://lore.kernel.org/r/20260927230724.119305-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 54 ++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index d4c72cd20069..9c204bd7af74 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -54,7 +54,7 @@ struct client { u32 version; struct fw_device *device; - spinlock_t lock; + struct mutex mutex; bool in_shutdown; struct xarray resource_xa; struct list_head event_list; @@ -315,7 +315,7 @@ static int fw_device_op_open(struct inode *inode, struct file *file) } client->device = device; - spin_lock_init(&client->lock); + mutex_init(&client->mutex); xa_init_flags(&client->resource_xa, XA_FLAGS_ALLOC1 | XA_FLAGS_LOCK_BH); INIT_LIST_HEAD(&client->event_list); init_waitqueue_head(&client->wait); @@ -338,7 +338,7 @@ static void queue_event(struct client *client, struct event *event, event->v[1].data = data1; event->v[1].size = size1; - scoped_guard(spinlock_irqsave, &client->lock) { + scoped_guard(mutex, &client->mutex) { if (client->in_shutdown) { kfree(event); } else { @@ -355,25 +355,31 @@ static ssize_t dequeue_event(struct client *client, char __user *buffer, size_t // After the following block, the event pointer above is guaranteed to have a correct value. { - spin_lock_irq(&client->lock); + mutex_lock(&client->mutex); - int ret = wait_event_interruptible_lock_irq(client->wait, + // This could be replaced with wait_var_event_any_lock() if poll_wait() alternative + // would be introduced. + int ret = ___wait_event(client->wait, !list_empty(&client->event_list) || fw_device_is_shutdown(client->device), - client->lock); + TASK_INTERRUPTIBLE, 0, 0, + mutex_unlock(&client->mutex); + schedule(); + mutex_lock(&client->mutex) + ); if (ret < 0) { - spin_unlock_irq(&client->lock); + mutex_unlock(&client->mutex); return ret; } if (fw_device_is_shutdown(client->device)) { - spin_unlock_irq(&client->lock); + mutex_unlock(&client->mutex); return -ENODEV; } event = list_first_entry(&client->event_list, struct event, link); list_del(&event->link); - spin_unlock_irq(&client->lock); + mutex_unlock(&client->mutex); } ssize_t ret = 0; @@ -451,11 +457,11 @@ static void queue_bus_reset_event(struct client *client) queue_event(client, &e->event, &e->reset, sizeof(e->reset), NULL, 0); - guard(spinlock_irq)(&client->lock); - - xa_for_each(&client->resource_xa, index, resource) { - if (is_iso_resource_auto(resource)) - schedule_iso_resource_auto(to_iso_resource_auto(resource), 0); + scoped_guard(mutex, &client->mutex) { + xa_for_each(&client->resource_xa, index, resource) { + if (is_iso_resource_auto(resource)) + schedule_iso_resource_auto(to_iso_resource_auto(resource), 0); + } } } @@ -546,7 +552,7 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg) static int add_client_resource(struct client *client, struct client_resource *resource, client_resource_release_fn_t release) { - scoped_guard(spinlock_irqsave, &client->lock) { + scoped_guard(mutex, &client->mutex) { u32 index; int ret; @@ -572,7 +578,7 @@ static int release_client_resource(struct client *client, u32 handle, unsigned long index = handle; struct client_resource *resource; - scoped_guard(spinlock_irq, &client->lock) { + scoped_guard(mutex, &client->mutex) { if (client->in_shutdown) return -EINVAL; @@ -605,7 +611,7 @@ static void complete_transaction(struct fw_card *card, int rcode, u32 request_ts struct client *client = e->client; unsigned long index = e->r.resource.handle; - scoped_guard(spinlock_irqsave, &client->lock) { + scoped_guard(mutex, &client->mutex) { xa_erase(&client->resource_xa, index); if (client->in_shutdown) wake_up(&client->tx_flush_wait); @@ -1387,7 +1393,7 @@ static void iso_resource_auto_work(struct work_struct *work) if (!success) { // Allocation or reallocation failure? Pull this resource out of the // xarray and prepare for deletion, unless the client is shutting down. - scoped_guard(spinlock_irq, &client->lock) { + scoped_guard(mutex, &client->mutex) { if (!client->in_shutdown && xa_erase(&client->resource_xa, index)) { // For the incrementation by add_client_resource(). client_put(client); @@ -1907,11 +1913,11 @@ static bool has_outbound_transactions(struct client *client) struct client_resource *resource; unsigned long index; - guard(spinlock_irq)(&client->lock); - - xa_for_each(&client->resource_xa, index, resource) { - if (is_outbound_transaction_resource(resource)) - return true; + scoped_guard(mutex, &client->mutex) { + xa_for_each(&client->resource_xa, index, resource) { + if (is_outbound_transaction_resource(resource)) + return true; + } } return false; @@ -1938,7 +1944,7 @@ static int fw_device_op_release(struct inode *inode, struct file *file) fw_iso_buffer_destroy(&client->buffer, client->device->card); // Freeze client->resource_xa and client->event_list. - scoped_guard(spinlock_irq, &client->lock) + scoped_guard(mutex, &client->mutex) client->in_shutdown = true; wait_event(client->tx_flush_wait, !has_outbound_transactions(client)); From 8fa3077dff05084a140d75fe0f7827adc9f541c8 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 28 Sep 2026 21:57:15 +0900 Subject: [PATCH 62/88] firewire: core: use kzalloc_flex() to allocate structure with quadlet array fw_core_handle_request() is now called in process context, so GFP_KERNEL is available. The fw_request structure has a trailing flexible array member of quadlets to store packet data. Use kzalloc_flex() to allocate the structure together with storage for the flexible array member. Link: https://lore.kernel.org/r/20260928125718.50974-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 88c0bfbe98ad..d3a1467b1239 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -898,8 +898,8 @@ static struct fw_request *allocate_request(struct fw_card *card, return NULL; } - request = kmalloc(sizeof(*request) + length, GFP_ATOMIC); - if (request == NULL) + request = kzalloc_flex(*request, data, DIV_ROUND_UP(length, sizeof(u32))); + if (!request) return NULL; kref_init(&request->kref); From b1790574d374aa7d90aa8200eae1bc45e03c66c8 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 28 Sep 2026 21:57:16 +0900 Subject: [PATCH 63/88] firewire: cdev: use kzalloc_flex() to allocate structure with quadlet array Some event structures have a trailing flexible array member of quadlets to store event-specific data. Use kzalloc_flex() to allocate the structure together with storage for the flexible array member. Link: https://lore.kernel.org/r/20260928125718.50974-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 9c204bd7af74..caabaaf99cb7 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -683,8 +683,8 @@ static int init_request(struct client *client, request->length < 4) return -EINVAL; - e = kmalloc(sizeof(*e) + request->length, GFP_KERNEL); - if (e == NULL) + e = kzalloc_flex(*e, rsp.with_tstamp.data, DIV_ROUND_UP(request->length, sizeof(u32))); + if (!e) return -ENOMEM; e->client = client; @@ -1696,7 +1696,7 @@ static int ioctl_send_phy_packet(struct client *client, union ioctl_arg *arg) if (!client->device->is_local) return -ENOSYS; - e = kzalloc(sizeof(*e) + sizeof(a->data), GFP_KERNEL); + e = kzalloc_flex(*e, phy_packet.with_tstamp.data, ARRAY_SIZE(a->data)); if (e == NULL) return -ENOMEM; @@ -1762,7 +1762,8 @@ void fw_cdev_handle_phy_packet(struct fw_card *card, struct fw_packet *p) if (client->device->card != card) continue; - struct inbound_phy_packet_event *e = kmalloc(sizeof(*e) + 8, GFP_KERNEL); + struct inbound_phy_packet_event *e = + kzalloc_flex(*e, phy_packet.with_tstamp.data, 2); if (!e) break; From 8487a3a876eea696f1eaf4b477d2e9cc336bc436 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 28 Sep 2026 21:57:17 +0900 Subject: [PATCH 64/88] firewire: cdev: use kzalloc_flex() to allocate structure with byte array Some event structures have a trailing flexible array member of bytes to store event-specific data. Use kzalloc_flex() to allocate the structure together with storage for the flexible array member. Link: https://lore.kernel.org/r/20260928125718.50974-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index caabaaf99cb7..60067c5b1973 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -1020,7 +1020,7 @@ static void iso_callback(struct fw_iso_context *context, u32 cycle, struct client *client = data; struct iso_interrupt_event *e; - e = kmalloc(sizeof(*e) + header_length, GFP_KERNEL); + e = kzalloc_flex(*e, interrupt.header, header_length); if (e == NULL) return; From a87cf01800575578c9db83acfb137b261296da52 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 7 Oct 2026 18:32:31 +0900 Subject: [PATCH 65/88] firewire: core: expand close_transaction() into its callers and remove it The return value from close_transaction() is useful only in fw_cancel_transaction(). The other caller, transmit_complete_callback(), simply discards it. Additionally, the pending transaction queue should be enumerated as soon as possible to prevent the issued transaction from completing. Expand close_transaction() into its callers, and remove it. In the find_and_pop_transaction_entry() macro, a local variable provides the evaluation result, however transmit_complete_callback() uses the same local variable for another purpose. Add an underscore prefix to the one in the macro to avoid the unexpected result. Link: https://lore.kernel.org/r/20261007093235.175742-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 80 ++++++++++++++--------------- 1 file changed, 38 insertions(+), 42 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index d3a1467b1239..7a3bf8133ffc 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -87,40 +87,22 @@ void fw_cancel_pending_transactions(struct fw_card *card) // card->transactions.lock must be acquired in advance. #define find_and_pop_transaction_entry(card, condition) \ ({ \ - struct fw_transaction *iter, *t = NULL; \ + struct fw_transaction *iter, *__t = NULL; \ list_for_each_entry(iter, &card->transactions.list, link) { \ if (condition) { \ - t = iter; \ + __t = iter; \ break; \ } \ } \ - if (t) { \ - if (try_cancel_split_timeout(t)) \ - remove_transaction_entry(card, t); \ + if (__t) { \ + if (try_cancel_split_timeout(__t)) \ + remove_transaction_entry(card, __t); \ else \ - t = NULL; \ + __t = NULL; \ } \ - t; \ + __t; \ }) -static int close_transaction(struct fw_transaction *transaction, struct fw_card *card, int rcode, - u32 response_tstamp) -{ - struct fw_transaction *t; - - // 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) { - t = find_and_pop_transaction_entry(card, iter == transaction); - if (!t) - return -ENOENT; - } - - invoke_callback(t, rcode, response_tstamp, NULL, 0); - - return 0; -} - /* * Only valid for transactions that are potentially pending (ie have * been sent). @@ -139,17 +121,25 @@ int fw_cancel_transaction(struct fw_card *card, if (card->driver->cancel_packet(card, &transaction->packet) == 0) return 0; + // If the request packet has already been sent, we need to see if the transaction is still + // pending and remove it in that case (e.g. the split transaction). + // + // 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) { + if (!find_and_pop_transaction_entry(card, iter == transaction)) + return -ENOENT; + } + u32 curr_cycle_time = 0; // Timestamping on behalf of hardware. (void)fw_card_read_cycle_time(card, &curr_cycle_time); tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); - /* - * If the request packet has already been sent, we need to see - * if the transaction is still pending and remove it in that case. - */ - return close_transaction(transaction, card, RCODE_CANCELLED, tstamp); + invoke_callback(transaction, RCODE_CANCELLED, tstamp, NULL, 0); + + return 0; } EXPORT_SYMBOL(fw_cancel_transaction); @@ -205,9 +195,6 @@ static void transmit_complete_callback(struct fw_packet *packet, packet->speed, status, packet->timestamp); switch (status) { - case ACK_COMPLETE: - close_transaction(t, card, RCODE_COMPLETE, packet->timestamp); - break; case ACK_PENDING: { unsigned int delta; @@ -222,27 +209,36 @@ static void transmit_complete_callback(struct fw_packet *packet, // local destination never runs in any type of IRQ context. scoped_guard(spinlock_irqsave, &card->transactions.lock) start_split_transaction_timeout(t, delta); - break; + return; } + case ACK_COMPLETE: + status = RCODE_COMPLETE; + break; case ACK_BUSY_X: case ACK_BUSY_A: case ACK_BUSY_B: - close_transaction(t, card, RCODE_BUSY, packet->timestamp); + status = RCODE_BUSY; break; case ACK_DATA_ERROR: - close_transaction(t, card, RCODE_DATA_ERROR, packet->timestamp); + status = RCODE_DATA_ERROR; break; case ACK_TYPE_ERROR: - close_transaction(t, card, RCODE_TYPE_ERROR, packet->timestamp); + status = RCODE_TYPE_ERROR; break; default: - /* - * In this case the ack is really a juju specific - * rcode, so just forward that to the callback. - */ - close_transaction(t, card, status, packet->timestamp); + // In this case the ack is really a juju specific rcode, so just forward that to + // the callback. break; } + + // 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) { + if (!find_and_pop_transaction_entry(card, iter == t)) + return; + } + + invoke_callback(t, status, packet->timestamp, NULL, 0); } static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel, From 1d2f5118baa804592de725f445bffb907e6efaeb Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 7 Oct 2026 18:32:32 +0900 Subject: [PATCH 66/88] firewire: ohci: schedule callback on workqueue when cancelling AT request packet The current implementation invokes the packet callback in the context where the packet cancellation is requested. This is inconvenient because the context can affect the packet callback. Invoke the packet callback from a workqueue instead. Use the workqueue for local AT packets. Link: https://lore.kernel.org/r/20261007093235.175742-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 8 ++---- drivers/firewire/ohci.c | 43 ++++++++++++++++++++--------- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 7a3bf8133ffc..bf39f82d9fc4 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -112,12 +112,8 @@ int fw_cancel_transaction(struct fw_card *card, { u32 tstamp; - /* - * Cancel the packet transmission if it's still queued. That - * will call the packet transmission callback which cancels - * the transaction. - */ - + // Cancel the packet transmission if it's still queued. That will schedule the packet + // transmission callback which cancels the transaction. if (card->driver->cancel_packet(card, &transaction->packet) == 0) return 0; diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index df53efec0b0a..896d0cb9a895 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1669,6 +1669,9 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet struct fw_ohci *ohci = ctx->context.ohci; bool use_work = true; + // These members are used to cancel the packet. + packet->driver_data = NULL; + scoped_guard(spinlock_irqsave, &ohci->lock) { if (!destination_is_local(packet, ohci)) { if (!at_context_queue_packet(ctx, packet)) @@ -2652,7 +2655,7 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet) { struct fw_ohci *ohci = fw_ohci(card); struct at_context *ctx = &ohci->at_request_ctx; - struct driver_data *driver_data = packet->driver_data; + struct at_local *local = &ohci->at_request_local; int ret = -ENOENT; // Avoid dead lock due to programming mistake. @@ -2660,24 +2663,38 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet) return 0; disable_work_sync(&ctx->work); - if (packet->ack != 0) - goto out; + bool use_local_work = false; - if (packet->payload_mapped) - dma_unmap_single(ohci->card.device, packet->payload_bus, - packet->payload_length, DMA_TO_DEVICE); + if (!packet->ack) { + // NOTE: This function relies on this value, thus it is not thread-safe. + struct driver_data *driver_data = packet->driver_data; - driver_data->packet = NULL; - packet->ack = RCODE_CANCELLED; + if (driver_data) { + if (packet->payload_mapped) { + dma_unmap_single(ohci->card.device, packet->payload_bus, + packet->payload_length, DMA_TO_DEVICE); + } - // Timestamping on behalf of the hardware. - packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); + // Invalidate the packet queued in AT request context. + driver_data->packet = NULL; + + scoped_guard(spinlock_irqsave, &local->lock) + list_add_tail(&packet->link_for_local, &local->list); + use_local_work = true; + } + } - packet->callback(packet, &ohci->card, packet->ack); - ret = 0; - out: enable_and_queue_work(card->async_wq, &ctx->work); + if (use_local_work) { + // Timestamping on behalf of the hardware. + packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); + packet->ack = RCODE_CANCELLED; + queue_work(card->async_wq, &local->work); + + ret = 0; + } + return ret; } From 34dca2ecd4021a26762f16f32fb94f8e591b8fba Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 7 Oct 2026 18:32:33 +0900 Subject: [PATCH 67/88] firewire: ohci: allow AT local packet cancellation The current implementation uses a different path for local packets than for remote packets, and local packets cannot be cancelled once they have been queued. Allow local packets to be cancelled. Link: https://lore.kernel.org/r/20261007093235.175742-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 896d0cb9a895..c06e8d87239c 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1597,7 +1597,7 @@ static void handle_at_local_packets(struct at_local *local, struct fw_ohci *ohci spin_lock(&local->lock); while ((packet = list_first_entry_or_null(&local->list, typeof(*packet), link_for_local))) { - list_del(&packet->link_for_local); + list_del_init(&packet->link_for_local); spin_unlock(&local->lock); if (unlikely(packet->ack != 0)) { @@ -1671,6 +1671,7 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet // These members are used to cancel the packet. packet->driver_data = NULL; + INIT_LIST_HEAD(&packet->link_for_local); scoped_guard(spinlock_irqsave, &ohci->lock) { if (!destination_is_local(packet, ohci)) { @@ -2658,6 +2659,10 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet) struct at_local *local = &ohci->at_request_local; int ret = -ENOENT; + // handle_at_request_local_packet() can reach here by calling fw_core_handle_response(). + if (current_work() != &local->work) + disable_work_sync(&local->work); + // Avoid dead lock due to programming mistake. if (WARN_ON_ONCE(current_work() == &ctx->work)) return 0; @@ -2681,6 +2686,11 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet) scoped_guard(spinlock_irqsave, &local->lock) list_add_tail(&packet->link_for_local, &local->list); use_local_work = true; + } else { + scoped_guard(spinlock_irqsave, &local->lock) { + if (!list_empty(&packet->link_for_local)) + use_local_work = true; + } } } @@ -2690,11 +2700,13 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet) // Timestamping on behalf of the hardware. packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); packet->ack = RCODE_CANCELLED; - queue_work(card->async_wq, &local->work); ret = 0; } + if (current_work() != &local->work) + enable_and_queue_work(card->async_wq, &local->work); + return ret; } From dd1c6b1518346b582a675a6e630a20ccbd90e508 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 7 Oct 2026 18:32:34 +0900 Subject: [PATCH 68/88] firewire: core: schedule callback when cancelling pending transactions The current implementation invokes the transaction callback in the context where transaction cancellation is requested. This is inconvenient because the context can affect the transaction callback. Invoke the transaction callback from a workqueue instead. Use the workqueue for error handling. Link: https://lore.kernel.org/r/20261007093235.175742-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index bf39f82d9fc4..617df7ecb12e 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -110,8 +110,6 @@ void fw_cancel_pending_transactions(struct fw_card *card) int fw_cancel_transaction(struct fw_card *card, struct fw_transaction *transaction) { - u32 tstamp; - // Cancel the packet transmission if it's still queued. That will schedule the packet // transmission callback which cancels the transaction. if (card->driver->cancel_packet(card, &transaction->packet) == 0) @@ -131,9 +129,10 @@ int fw_cancel_transaction(struct fw_card *card, // Timestamping on behalf of hardware. (void)fw_card_read_cycle_time(card, &curr_cycle_time); - tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); + transaction->response_timestamp = cycle_time_to_ohci_tstamp(curr_cycle_time); + transaction->rcode = RCODE_CANCELLED; - invoke_callback(transaction, RCODE_CANCELLED, tstamp, NULL, 0); + enable_and_queue_work(card->async_wq, &transaction->error_work); return 0; } From b381407e03b844d1e836b5a96badb62a0bf159b2 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 7 Oct 2026 18:32:35 +0900 Subject: [PATCH 69/88] firewire: core: fulfill kerneldoc for fw_cancel_transaction() fw_cancel_transaction() has no kerneldoc even though it is exposed. Add kerneldoc for it. Link: https://lore.kernel.org/r/20261007093235.175742-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 617df7ecb12e..770c44f17ec6 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -103,12 +103,20 @@ void fw_cancel_pending_transactions(struct fw_card *card) __t; \ }) -/* - * Only valid for transactions that are potentially pending (ie have - * been sent). +/** + * fw_cancel_transaction: cancel transaction. + * @card: interface where the transaction has been requested + * @transaction: transaction instance to be cancelled + * + * Terminate the transaction by either canceling the request subaction or the response subaction + * of the split transaction, then schedule the transaction callback to notify the cancellation. + * The transaction instance should be kept until the callback will be invoked. + * + * Context: Process context. + * Returns: 0 on success, -ENOENT if the transaction instance is not found in the queue for the + * pending transaction. */ -int fw_cancel_transaction(struct fw_card *card, - struct fw_transaction *transaction) +int fw_cancel_transaction(struct fw_card *card, struct fw_transaction *transaction) { // Cancel the packet transmission if it's still queued. That will schedule the packet // transmission callback which cancels the transaction. From 9136caffae97fc83829629915b64ad34fb74d4e6 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 11 Oct 2026 15:31:41 +0900 Subject: [PATCH 70/88] firewire: core: add might_sleep() checks to transaction layer Some functions are now guaranteed to run in process context. Add might_sleep() calls to these functions to document and check this requirement. Link: https://lore.kernel.org/r/20261011063146.497455-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 770c44f17ec6..92a2722a9aa8 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -118,6 +118,9 @@ void fw_cancel_pending_transactions(struct fw_card *card) */ int fw_cancel_transaction(struct fw_card *card, struct fw_transaction *transaction) { + // Allow the call of struct fw_card_driver.cancel_packet() to wait for any event. + might_sleep(); + // Cancel the packet transmission if it's still queued. That will schedule the packet // transmission callback which cancels the transaction. if (card->driver->cancel_packet(card, &transaction->packet) == 0) @@ -492,6 +495,9 @@ int fw_run_transaction(struct fw_card *card, int tcode, int destination_id, struct transaction_callback_data d; struct fw_transaction t; + // Due to the call of wait_for_completion(). + might_sleep(); + timer_setup_on_stack(&t.split_timeout_timer, NULL, 0); init_completion(&d.done); d.payload = payload; @@ -528,6 +534,9 @@ void fw_send_phy_config(struct fw_card *card, long timeout = msecs_to_jiffies(100); u32 data = 0; + // Due to the call of wait_for_completion_timeout(). + might_sleep(); + phy_packet_set_packet_identifier(&data, PHY_PACKET_PACKET_IDENTIFIER_PHY_CONFIG); if (node_id != FW_PHY_CONFIG_NO_NODE_ID) { @@ -701,6 +710,9 @@ EXPORT_SYMBOL(fw_core_add_address_handler); */ void fw_core_remove_address_handler(struct fw_address_handler *handler) { + // Due to synchronize_rcu(). + might_sleep(); + scoped_guard(spinlock, &address_handler_list_lock) list_del_rcu(&handler->link); @@ -1108,6 +1120,10 @@ void fw_core_handle_request(struct fw_card *card, struct fw_packet *p) unsigned long long offset; unsigned int tcode; + // Allow the call of allocate_request() to perform object allocation with GFP_KERNEL, + // as well as the address handlers to work with mutexes. + might_sleep(); + if (p->ack != ACK_PENDING && p->ack != ACK_COMPLETE) return; @@ -1146,6 +1162,9 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p) size_t data_length; int tcode, tlabel, source, rcode; + // Allow the call of struct fw_card_driver.cancel_packet() to wait for any event. + might_sleep(); + tcode = async_header_get_tcode(p->header); tlabel = async_header_get_tlabel(p->header); source = async_header_get_source(p->header); From 382e671eaca3afa004dd74d4441427ddb9962a52 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 11 Oct 2026 15:31:42 +0900 Subject: [PATCH 71/88] firewire: core: enable context analysis for transaction layer The kernel is transitioning from Sparse-based context analysis to compier-based context analysis, which requires LLVM/Clang 23 or later. Individual compilation units can opt in to the analysis through their Makefiles. Enable context analysis for the transaction layer. Link: https://lore.kernel.org/r/20261011063146.497455-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/firewire/Makefile b/drivers/firewire/Makefile index 1ff550e93a8c..e27fca37b4b2 100644 --- a/drivers/firewire/Makefile +++ b/drivers/firewire/Makefile @@ -3,6 +3,8 @@ # Makefile for the Linux IEEE 1394 implementation # +CONTEXT_ANALYSIS_core-transaction.o := y + firewire-core-y += core-trace.o core-card.o core-cdev.o core-device.o \ core-iso.o core-topology.o core-transaction.o firewire-ohci-y += ohci.o From 15efe5cec615ee71ded805dbbc9fea8e8e020c81 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 11 Oct 2026 15:31:43 +0900 Subject: [PATCH 72/88] firewire: core: use context analysis annotations and lockdep checks for transactions lock The transaction lock protects the list of pending transactions and the transaction label. Add context analysis annotations for the transaction lock. Compiler-based context analysis has difficulty tracking the locking scope when the lock is accessed through an indirect reference (e.g. t->card->lock), making some functions difficult to annotate. Use lockdep assertions to cover these cases. Link: https://lore.kernel.org/r/20261011063146.497455-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 30 +++++++++++++++++++++++++++-- include/linux/firewire.h | 6 +++--- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 92a2722a9aa8..6b461142e30f 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -36,18 +36,24 @@ #define HEADER_DESTINATION_IS_BROADCAST(header) \ ((async_header_get_destination(header) & 0x3f) == 0x3f) -/* returns 0 if the split timeout handler is already running */ +// Returns 0 if the split timeout handler is already running. +// +// card->transactions.lock must be acquired in advance. static int try_cancel_split_timeout(struct fw_transaction *t) { + lockdep_assert_held(&t->card->transactions.lock); + if (t->is_split_transaction) return timer_delete(&t->split_timeout_timer) || disable_work(&t->error_work); else return 1; } -// card->transactions.lock must be acquired in advance. static void remove_transaction_entry(struct fw_card *card, struct fw_transaction *entry) +__must_hold(&card->transactions.lock) { + lockdep_assert_held(&card->transactions.lock); + list_del_init(&entry->link); card->transactions.tlabel_mask &= ~(1ULL << entry->tlabel); } @@ -65,10 +71,13 @@ static void invoke_callback(struct fw_transaction *t, int rcode, u32 response_ts // Must be called without holding card->transactions.lock. void fw_cancel_pending_transactions(struct fw_card *card) +__must_not_hold(&card->transactions.lock) { struct fw_transaction *t, *tmp; LIST_HEAD(pending_list); + lockdep_assert_not_held(&card->transactions.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. scoped_guard(spinlock_irqsave, &card->transactions.lock) { @@ -117,7 +126,10 @@ void fw_cancel_pending_transactions(struct fw_card *card) * pending transaction. */ int fw_cancel_transaction(struct fw_card *card, struct fw_transaction *transaction) +__must_not_hold(&card->transactions.lock) { + lockdep_assert_not_held(&card->transactions.lock); + // Allow the call of struct fw_card_driver.cancel_packet() to wait for any event. might_sleep(); @@ -164,11 +176,14 @@ static void schedule_error_callback(struct fw_transaction *t, int rcode, u32 res queue_work(t->card->async_wq, &t->error_work); } +// card->transactions.lock must be released in advance. 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; + lockdep_assert_not_held(&card->transactions.lock); + scoped_guard(spinlock_irqsave, &card->transactions.lock) { if (list_empty(&t->link)) return; @@ -181,6 +196,8 @@ static void split_transaction_timeout_callback(struct timer_list *timer) // card->transactions.lock should be acquired in advance for the linked list. static void start_split_transaction_timeout(struct fw_transaction *t, unsigned int delta) { + lockdep_assert_held(&t->card->transactions.lock); + if (list_empty(&t->link) || WARN_ON(t->is_split_transaction)) return; @@ -193,10 +210,13 @@ static u32 compute_split_timeout_timestamp(struct fw_card *card, u32 request_tim static void transmit_complete_callback(struct fw_packet *packet, struct fw_card *card, int status) +__must_not_hold(&card->transactions.lock) { struct fw_transaction *t = container_of(packet, struct fw_transaction, packet); + lockdep_assert_not_held(&card->transactions.lock); + trace_async_request_outbound_complete((uintptr_t)t, card->index, packet->generation, packet->speed, status, packet->timestamp); @@ -392,9 +412,12 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode int destination_id, int generation, int speed, unsigned long long offset, void *payload, size_t length, union fw_transaction_callback callback, bool with_tstamp, void *callback_data) +__must_not_hold(&card->transactions.lock) { int tlabel; + lockdep_assert_not_held(&card->transactions.lock); + t->card = card; t->callback = callback; t->with_tstamp = with_tstamp; @@ -1156,12 +1179,15 @@ void fw_core_handle_request(struct fw_card *card, struct fw_packet *p) EXPORT_SYMBOL(fw_core_handle_request); void fw_core_handle_response(struct fw_card *card, struct fw_packet *p) +__must_not_hold(&card->transactions.lock) { struct fw_transaction *t = NULL; u32 *data; size_t data_length; int tcode, tlabel, source, rcode; + lockdep_assert_not_held(&card->transactions.lock); + // Allow the call of struct fw_card_driver.cancel_packet() to wait for any event. might_sleep(); diff --git a/include/linux/firewire.h b/include/linux/firewire.h index dbb5c825a1cc..89db958dea2e 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -92,9 +92,9 @@ struct fw_card { u64 reset_jiffies; struct { - int current_tlabel; - u64 tlabel_mask; - struct list_head list; + int current_tlabel __guarded_by(&lock); + u64 tlabel_mask __guarded_by(&lock); + struct list_head list __guarded_by(&lock); spinlock_t lock; } transactions; From 8540ec31c831419c79ad65be2f515a9d8bbde257 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 11 Oct 2026 15:31:44 +0900 Subject: [PATCH 73/88] firewire: core: use context analysis annotations and lockdep checks for split timeout lock The spinlock protects parameters related to split transaction timeouts. Add context analysis annotations and lockdep checks for this lock. Link: https://lore.kernel.org/r/20261009121048.282982-5-o-takashi@sakamocchi.jp Link: https://lore.kernel.org/r/20261011063146.497455-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 31 ++++++++++++++++++----------- include/linux/firewire.h | 8 ++++---- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 6b461142e30f..4d60f4998c6e 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -210,11 +210,13 @@ static u32 compute_split_timeout_timestamp(struct fw_card *card, u32 request_tim static void transmit_complete_callback(struct fw_packet *packet, struct fw_card *card, int status) +__must_not_hold(&card->split_timeout.lock) __must_not_hold(&card->transactions.lock) { struct fw_transaction *t = container_of(packet, struct fw_transaction, packet); + lockdep_assert_not_held(&card->split_timeout.lock); lockdep_assert_not_held(&card->transactions.lock); trace_async_request_outbound_complete((uintptr_t)t, card->index, packet->generation, @@ -898,11 +900,14 @@ __must_hold(&card->split_timeout.lock) static struct fw_request *allocate_request(struct fw_card *card, struct fw_packet *p) +__must_not_hold(&card->split_timeout.lock) { struct fw_request *request; u32 *data, length; int request_tcode; + lockdep_assert_not_held(&card->split_timeout.lock); + request_tcode = async_header_get_tcode(p->header); switch (request_tcode) { case TCODE_WRITE_QUADLET_REQUEST: @@ -1320,6 +1325,8 @@ __must_hold(&card->split_timeout.lock) { unsigned int cycles; + lockdep_assert_held(&card->split_timeout.lock); + cycles = card->split_timeout.hi * 8000 + (card->split_timeout.lo >> 19); /* minimum per IEEE 1394, maximum which doesn't overflow OHCI */ @@ -1375,28 +1382,28 @@ static void handle_registers(struct fw_card *card, struct fw_request *request, break; case CSR_SPLIT_TIMEOUT_HI: - if (tcode == TCODE_READ_QUADLET_REQUEST) { - *data = cpu_to_be32(card->split_timeout.hi); - } else if (tcode == TCODE_WRITE_QUADLET_REQUEST) { - scoped_guard(spinlock, &card->split_timeout.lock) { + scoped_guard(spinlock, &card->split_timeout.lock) { + if (tcode == TCODE_READ_QUADLET_REQUEST) { + *data = cpu_to_be32(card->split_timeout.hi); + } else if (tcode == TCODE_WRITE_QUADLET_REQUEST) { card->split_timeout.hi = be32_to_cpu(*data) & 7; update_split_timeout(card); + } else { + rcode = RCODE_TYPE_ERROR; } - } else { - rcode = RCODE_TYPE_ERROR; } break; case CSR_SPLIT_TIMEOUT_LO: - if (tcode == TCODE_READ_QUADLET_REQUEST) { - *data = cpu_to_be32(card->split_timeout.lo); - } else if (tcode == TCODE_WRITE_QUADLET_REQUEST) { - scoped_guard(spinlock, &card->split_timeout.lock) { + scoped_guard(spinlock, &card->split_timeout.lock) { + if (tcode == TCODE_READ_QUADLET_REQUEST) { + *data = cpu_to_be32(card->split_timeout.lo); + } else if (tcode == TCODE_WRITE_QUADLET_REQUEST) { card->split_timeout.lo = be32_to_cpu(*data) & 0xfff80000; update_split_timeout(card); + } else { + rcode = RCODE_TYPE_ERROR; } - } else { - rcode = RCODE_TYPE_ERROR; } break; diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 89db958dea2e..a3660ad1a1d4 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -99,10 +99,10 @@ struct fw_card { } transactions; struct { - u32 hi; - u32 lo; - unsigned int cycles; - unsigned int jiffies; + u32 hi __guarded_by(&lock); + u32 lo __guarded_by(&lock); + unsigned int cycles __guarded_by(&lock); + unsigned int jiffies __guarded_by(&lock); spinlock_t lock; } split_timeout; From c2b3fb63263ed5f15d3855e2ca0405d412f836f0 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 11 Oct 2026 15:31:45 +0900 Subject: [PATCH 74/88] firewire: core: use context analysis annotations and lockdep checks for topology map lock The spinlock protects the topology map from concurrent access. Add context analysis annotations and lockdep checks for this lock. Link: https://lore.kernel.org/r/20261011063146.497455-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-topology.c | 3 +++ drivers/firewire/core-transaction.c | 3 +++ include/linux/firewire.h | 2 +- 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index ee6b54f89859..512b7e4473af 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -474,9 +474,12 @@ static void update_topology_map(__be32 *buffer, size_t buffer_size, int root_nod 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) +__must_not_hold(&card->topology_map.lock) { struct fw_node *local_node; + lockdep_assert_not_held(&card->topology_map.lock); + trace_bus_reset_handle(card->index, generation, node_id, bm_abdicate, self_ids, self_id_count); scoped_guard(spinlock, &card->lock) { diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 4d60f4998c6e..ee1679347b36 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -1290,9 +1290,12 @@ static void handle_topology_map(struct fw_card *card, struct fw_request *request int tcode, int destination, int source, int generation, unsigned long long offset, void *payload, size_t length, void *callback_data) +__must_not_hold(&card->topology_map.lock) { int start; + lockdep_assert_not_held(&card->topology_map.lock); + if (!tcode_is_read_request(tcode)) { fw_send_response(card, request, RCODE_TYPE_ERROR); return; diff --git a/include/linux/firewire.h b/include/linux/firewire.h index a3660ad1a1d4..b55c2fdd739d 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -139,7 +139,7 @@ struct fw_card { u32 broadcast_channel; struct { - __be32 buffer[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4]; + __be32 buffer[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4] __guarded_by(&lock); spinlock_t lock; } topology_map; From aea8c084cb93fb250f096143bf038855405456c9 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 11 Oct 2026 15:31:46 +0900 Subject: [PATCH 75/88] firewire: core: narrow the card lock scope when accessing node_id When preparing the request subaction for an asynchronous transaction, the node_id field in the fw_card structure is accessed under the card-level spinlock. However, the lock is held across a call to a helper function, unnecessarily extending the critical section. Narrow the critical section to cover only the access to node_id. Link: https://lore.kernel.org/r/20261011063146.497455-7-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index ee1679347b36..97be9fddadcb 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -455,13 +455,15 @@ __must_not_hold(&card->transactions.lock) timer_setup(&t->split_timeout_timer, split_transaction_timeout_callback, 0); t->packet.callback = transmit_complete_callback; + // The node_id field of fw_card can be updated when handling SelfIDComplete. + int node_id; + // 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); - } + scoped_guard(spinlock_irqsave, &card->lock) + node_id = card->node_id; + fw_fill_request(&t->packet, tcode, t->tlabel, destination_id, node_id, generation, speed, + offset, payload, 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. From a78c645379c5d6a580a4c0d4ffe0502b20441b65 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 12 Oct 2026 10:57:39 +0900 Subject: [PATCH 76/88] firewire: core: use appropriate spinlock variants for transaction-related locks The spinlocks in the fw_card structure protect transaction-related state from concurrent access. They are acquired mostly from process context. The exceptions are split_transaction_timeout_callback(), which runs in softirq context for the timer wheel, and __fw_send_request(), which may also be called from interrupt context. Use spin_lock() in split_transaction_timeout_callback() and spin_lock_irqsave() in the other functions. The latter preserves and restores the caller's interrupt state, as the callers are not guaranteed to have interrupts enabled. Link: https://lore.kernel.org/r/20261012015739.530731-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 97be9fddadcb..e32f3be5b315 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -78,8 +78,6 @@ __must_not_hold(&card->transactions.lock) lockdep_assert_not_held(&card->transactions.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. scoped_guard(spinlock_irqsave, &card->transactions.lock) { list_for_each_entry_safe(t, tmp, &card->transactions.list, link) { if (try_cancel_split_timeout(t)) @@ -140,9 +138,6 @@ __must_not_hold(&card->transactions.lock) // If the request packet has already been sent, we need to see if the transaction is still // pending and remove it in that case (e.g. the split transaction). - // - // 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) { if (!find_and_pop_transaction_entry(card, iter == transaction)) return -ENOENT; @@ -184,7 +179,7 @@ static void split_transaction_timeout_callback(struct timer_list *timer) lockdep_assert_not_held(&card->transactions.lock); - scoped_guard(spinlock_irqsave, &card->transactions.lock) { + scoped_guard(spinlock, &card->transactions.lock) { if (list_empty(&t->link)) return; remove_transaction_entry(card, t); @@ -227,14 +222,12 @@ __must_not_hold(&card->transactions.lock) { unsigned int delta; - scoped_guard(spinlock, &card->split_timeout.lock) { + scoped_guard(spinlock_irqsave, &card->split_timeout.lock) { t->split_timeout_cycle = compute_split_timeout_timestamp(card, packet->timestamp) & 0xffff; delta = 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->transactions.lock) start_split_transaction_timeout(t, delta); return; @@ -259,8 +252,6 @@ __must_not_hold(&card->transactions.lock) break; } - // 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) { if (!find_and_pop_transaction_entry(card, iter == t)) return; @@ -431,8 +422,6 @@ __must_not_hold(&card->transactions.lock) * the list while holding the card spinlock. */ - // 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) { @@ -458,15 +447,11 @@ __must_not_hold(&card->transactions.lock) // The node_id field of fw_card can be updated when handling SelfIDComplete. int node_id; - // 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) node_id = card->node_id; fw_fill_request(&t->packet, tcode, t->tlabel, destination_id, node_id, generation, speed, offset, payload, 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->transactions.lock) list_add_tail(&t->link, &card->transactions.list); @@ -1232,8 +1217,6 @@ __must_not_hold(&card->transactions.lock) break; } - // 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) { t = find_and_pop_transaction_entry(card, iter->node_id == source && iter->tlabel == tlabel); From 7a8db81dbd780c6fb4584deb51b0cecad1baca56 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 13 Oct 2026 20:15:08 +0900 Subject: [PATCH 77/88] firewire: core: add kerneldoc for outbound transaction-related symbols The core provides helper functions and structures for initiating asynchronous transactions. Add kerneldoc comments for these symbols. Link: https://lore.kernel.org/r/20261013111509.612518-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- Documentation/driver-api/firewire.rst | 3 ++ drivers/firewire/core-transaction.c | 2 + include/linux/firewire.h | 61 +++++++++++++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/Documentation/driver-api/firewire.rst b/Documentation/driver-api/firewire.rst index 28a32410f7d2..e1120376a31c 100644 --- a/Documentation/driver-api/firewire.rst +++ b/Documentation/driver-api/firewire.rst @@ -37,6 +37,9 @@ Firewire device probing and sysfs interfaces Firewire core transaction interfaces ==================================== +.. kernel-doc:: include/linux/firewire.h + :identifiers: fw_transaction fw_transaction_callback_t fw_transaction_callback_with_tstamp_t + fw_transaction_callback fw_send_request fw_send_request_with_tstamp .. kernel-doc:: drivers/firewire/core-transaction.c :export: diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index e32f3be5b315..b866c8572392 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -400,6 +400,8 @@ __must_hold(&card->transactions.lock) * than when the request packet actually hits the wire. On the other hand, * transaction completion and hence execution of @callback may happen even * before fw_send_request() returns. + * + * Context: Any context. */ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode, int destination_id, int generation, int speed, unsigned long long offset, diff --git a/include/linux/firewire.h b/include/linux/firewire.h index b55c2fdd739d..f1cb7569eaa0 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -286,13 +286,62 @@ struct fw_request; typedef void (*fw_packet_callback_t)(struct fw_packet *packet, struct fw_card *card, int status); + +/** + * typedef fw_transaction_callback_t - Function to handle the result of outgoing transaction. + * @card: the interface to send the request at + * @rcode: the response code + * @data: the data for the response subaction + * @length: the length of data + * @callback_data: the data registered with this callback + * + * This callback is called from a workqueue in process context after the outgoing transaction is + * completed successfully or unsuccessfully. + * + * Among its parameters is the response code which is either one of the rcodes per IEEE 1394 or, in + * case of internal errors, the firewire-core specific %RCODE_SEND_ERROR. The other firewire-core + * specific rcodes (%RCODE_CANCELLED, %RCODE_BUSY, %RCODE_GENERATION, %RCODE_NO_ACK) denote + * transaction timeout, busy responder, stale request generation, or missing ACK respectively. + * + * Context: Process context. + */ typedef void (*fw_transaction_callback_t)(struct fw_card *card, int rcode, void *data, size_t length, void *callback_data); + +/** + * typedef fw_transaction_callback_with_tstamp_t - Function to handle the result of outgoing + * transaction with hardware time stamps. + * @card: the interface to send the request at + * @rcode: the response code + * @request_tstamp: the time stamp at which the request subaction finished + * @response_tstamp: the time stamp at which the response subaction finished + * @data: the data for the response subaction + * @length: the length of data + * @callback_data: the data registered with this callback + * + * This callback is called from a workqueue in process context after the outgoing transaction is + * completed successfully or unsuccessfully. + * + * Among its parameters is the response code which is either one of the rcodes per IEEE 1394 or, in + * case of internal errors, the firewire-core specific %RCODE_SEND_ERROR. The other firewire-core + * specific rcodes (%RCODE_CANCELLED, %RCODE_BUSY, %RCODE_GENERATION, %RCODE_NO_ACK) denote + * transaction timeout, busy responder, stale request generation, or missing ACK respectively. + * + * The timestamp consists of the low order 3 bits of second field and the full 13 bits of count + * field of isochronous cycle time register. + * + * Context: Process context. + */ typedef void (*fw_transaction_callback_with_tstamp_t)(struct fw_card *card, int rcode, u32 request_tstamp, u32 response_tstamp, void *data, size_t length, void *callback_data); +/** + * union fw_transaction_callback - A union to store the outgoing transaction callback. + * @without_tstamp: Callback without hardware timestamps. + * @with_tstamp: Callback with hardware timestamps. + */ union fw_transaction_callback { fw_transaction_callback_t without_tstamp; fw_transaction_callback_with_tstamp_t with_tstamp; @@ -352,7 +401,15 @@ struct fw_packet { void *driver_data; }; +/** + * struct fw_transaction - A structure to hold outgoing transaction state. + * + * All structure members are for internal use only. Once passed to __fw_send_request(), this + * structure holds the state of an outgoing transaction. It must not be freed until the callback + * reports the transaction result. + */ struct fw_transaction { + /* private: */ int node_id; /* The generation is implied; it is always the current. */ int tlabel; struct list_head link; @@ -429,6 +486,8 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode * * After the transaction is completed successfully or unsuccessfully, the @callback will be called * in process context. + * + * Context: Any context. */ static inline void fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode, int destination_id, int generation, int speed, @@ -461,6 +520,8 @@ static inline void fw_send_request(struct fw_card *card, struct fw_transaction * * * After the transaction is completed successfully or unsuccessfully, the @callback will be called * in process context. + * + * Context: Any context. */ static inline void fw_send_request_with_tstamp(struct fw_card *card, struct fw_transaction *t, int tcode, int destination_id, int generation, int speed, unsigned long long offset, From aa7f13e25c8003b54726b6e1339ee0fa8a2e99a8 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 13 Oct 2026 20:15:09 +0900 Subject: [PATCH 78/88] firewire: core: add kerneldoc for inbound transaction-related symbols The core provides helper functions and structures for handling transaction requests to local address regions. Add kerneldoc comments for these symbols. Link: https://lore.kernel.org/r/20261013111509.612518-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- Documentation/driver-api/firewire.rst | 2 ++ drivers/firewire/core-transaction.c | 37 +++++++++++++++------------ include/linux/firewire.h | 28 +++++++++++++++++--- 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/Documentation/driver-api/firewire.rst b/Documentation/driver-api/firewire.rst index e1120376a31c..695c5f6db045 100644 --- a/Documentation/driver-api/firewire.rst +++ b/Documentation/driver-api/firewire.rst @@ -40,6 +40,8 @@ Firewire core transaction interfaces .. kernel-doc:: include/linux/firewire.h :identifiers: fw_transaction fw_transaction_callback_t fw_transaction_callback_with_tstamp_t fw_transaction_callback fw_send_request fw_send_request_with_tstamp + fw_address_callback_t fw_address_handler fw_address_region + .. kernel-doc:: drivers/firewire/core-transaction.c :export: diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index b866c8572392..5038a10f0717 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -623,6 +623,12 @@ static struct fw_address_handler *lookup_enclosing_address_handler( static DEFINE_SPINLOCK(address_handler_list_lock); static LIST_HEAD(address_handler_list); +/** + * var fw_high_memory_region - An address range for high memory. + * + * Represents an address range within the Middle Address Space defined by the 1394 OHCI + * specification. + */ const struct fw_address_region fw_high_memory_region = { .start = FW_MAX_PHYSICAL_RANGE, .end = 0xffffe0000000ULL, }; EXPORT_SYMBOL(fw_high_memory_region); @@ -658,21 +664,20 @@ static int put_address_handler(struct fw_address_handler *handler) } /** - * fw_core_add_address_handler() - register for incoming requests - * @handler: callback - * @region: region in the IEEE 1212 node space address range + * fw_core_add_address_handler() - register a handler for incoming transaction requests to a + * specific address region. + * @handler: The handler including a callback function for an incoming transaction request. + * @region: The region in the IEEE 1212 node space address range. * - * region->start, ->end, and handler->length have to be quadlet-aligned. + * &fw_address_handler.length, &fw_address_region.start, and &fw_address_region.end are specified + * in bytes and must be multiples of a quadlet. &fw_address_handler.offset is available after + * successful registration. * * When a request is received that falls within the specified address range, the specified callback - * is invoked. The parameters passed to the callback give the details of the particular request. - * - * The start offset of the handler's address region is determined by - * fw_core_add_address_handler() and is returned in handler->offset. + * is invoked. The parameters passed to the callback give the details of the particular request. * * Address allocations are exclusive, except for the FCP registers. * - * Context: Process context. * Returns: 0 on success, non-zero otherwise. */ int fw_core_add_address_handler(struct fw_address_handler *handler, @@ -714,13 +719,13 @@ int fw_core_add_address_handler(struct fw_address_handler *handler, EXPORT_SYMBOL(fw_core_add_address_handler); /** - * fw_core_remove_address_handler() - unregister an address handler - * @handler: callback + * fw_core_remove_address_handler() - unregister an address handler for an incoming transaction + * request. + * @handler: The registered handler. * - * To be called in process context. + * After this function returns, the handler's callback is guaranteed not to be running on any CPU. * - * When fw_core_remove_address_handler() returns, @handler->callback() is - * guaranteed to not run on any CPU anymore. + * Context: Process context. */ void fw_core_remove_address_handler(struct fw_address_handler *handler) { @@ -955,8 +960,8 @@ __must_not_hold(&card->split_timeout.lock) * @request: firewire request data for the transaction. * @rcode: response code to send. * - * Submit a response packet into the asynchronous response transmission queue. The @request - * is going to be released when the transmission successfully finishes later. + * Submit a response packet into the asynchronous response transmission queue. The contents of + * @request are opaque to the caller. The @request is released when the transmission finishes. */ void fw_send_response(struct fw_card *card, struct fw_request *request, int rcode) diff --git a/include/linux/firewire.h b/include/linux/firewire.h index f1cb7569eaa0..9c563fbdbbf0 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -348,7 +348,7 @@ union fw_transaction_callback { }; /** - * typedef fw_address_callback_t - Function to handle the request of the asynchronous transaction. + * typedef fw_address_callback_t - Function to handle the request of the incoming transaction. * @card: the card instance which receives the request * @request: the request instance. * @tcode: the transaction code @@ -362,8 +362,9 @@ union fw_transaction_callback { * * This callback handles an inbound request subaction. * - * The callback is responsible that fw_send_response() is called on the @request, except for FCP - * registers for which the core takes care of that. + * The contents of @request are opaque to the callback. The callback is responsible for calling + * fw_send_response() for @request, except for FCP registers, for which the core handles the + * response. * * Context: Process context. */ @@ -434,18 +435,37 @@ struct fw_transaction { u32 response_timestamp; }; +/** + * struct fw_address_handler - A handler for incoming transaction requests to a registered address + * region. + * @offset: The registered offset, available after successful registration. + * @length: The length of the address region in bytes, a multiple of a quadlet. + * @address_callback: Callback invoked when a transaction request is received for the registered + * address region. + * @callback_data: Data passed to @address_callback. + * + * This structure represents a handler registered to receive incoming asynchronous requests to a + * specific region of the address space on the local node. + */ struct fw_address_handler { u64 offset; u64 length; fw_address_callback_t address_callback; void *callback_data; - // Only for core functions. + /* private: */ struct list_head link; struct kref kref; struct completion done; }; +/** + * struct fw_address_region - An address range. + * @start: The starting offset in bytes, a multiple of a quadlet. + * @end: The ending offset in bytes, a multiple of a quadlet. + * + * Represents a range of addresses in the local node's address space. + */ struct fw_address_region { u64 start; u64 end; From 23b790766adfb140abc09c83fbe625e2c7f35a36 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sat, 17 Oct 2026 17:17:20 +0900 Subject: [PATCH 79/88] firewire: core: use release/acquire pair when replacing underlying driver on card removal When removing a card instance, the underlying driver is replaced to handle closing operations. Without proper memory reordering, other CPUs may observe the driver before it is fully initialized. Use a release/acquire pair to ensure that the driver's initialization is visible before the driver is accessed for close operations. Link: https://lore.kernel.org/r/20261017081720.186688-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 16 ++++++++------- drivers/firewire/core-cdev.c | 2 +- drivers/firewire/core-device.c | 8 ++++---- drivers/firewire/core-iso.c | 32 +++++++++++++++++++++-------- drivers/firewire/core-transaction.c | 20 +++++++++--------- 5 files changed, 47 insertions(+), 31 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index be6f18d67ece..83c29a487d86 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -153,8 +153,8 @@ static void update_config_roms(void) list_for_each_entry (card, &card_list, link) { generate_config_rom(card, tmp_config_rom); - card->driver->set_config_rom(card, tmp_config_rom, - config_rom_length); + smp_load_acquire(&card->driver)->set_config_rom(card, tmp_config_rom, + config_rom_length); } } @@ -231,7 +231,7 @@ static int reset_bus(struct fw_card *card, bool short_reset) trace_bus_reset_initiate(card->index, card->generation, short_reset); - return card->driver->update_phy_reg(card, reg, 0, bit); + return smp_load_acquire(&card->driver)->update_phy_reg(card, reg, 0, bit); } void fw_schedule_bus_reset(struct fw_card *card, bool delayed, bool short_reset) @@ -660,7 +660,8 @@ 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); + ret = smp_load_acquire(&card->driver)->enable(card, tmp_config_rom, + config_rom_length); if (ret < 0) { card->isoc_wq = NULL; card->async_wq = NULL; @@ -804,7 +805,8 @@ void fw_core_remove_card(struct fw_card *card) dummy_driver.free_iso_context = card->driver->free_iso_context; dummy_driver.stop_iso = card->driver->stop_iso; dummy_driver.disable = card->driver->disable; - card->driver = &dummy_driver; + // Ensure the entire contents of the driver are visible to other CPUs. + smp_store_release(&card->driver, &dummy_driver); drain_workqueue(card->isoc_wq); drain_workqueue(card->async_wq); @@ -843,12 +845,12 @@ EXPORT_SYMBOL(fw_core_remove_card); */ int fw_card_read_cycle_time(struct fw_card *card, u32 *cycle_time) { - if (card->driver->read_csr == dummy_read_csr) + if (smp_load_acquire(&card->driver)->read_csr == dummy_read_csr) return -ENODEV; // It's possible to switch to dummy driver between the above and the below. This is the best // effort to return -ENODEV. - *cycle_time = card->driver->read_csr(card, CSR_CYCLE_TIME); + *cycle_time = smp_load_acquire(&card->driver)->read_csr(card, CSR_CYCLE_TIME); return 0; } EXPORT_SYMBOL_GPL(fw_card_read_cycle_time); diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 60067c5b1973..87efd65e6b00 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -1731,7 +1731,7 @@ static int ioctl_send_phy_packet(struct client *client, union ioctl_arg *arg) trace_async_phy_outbound_initiate((uintptr_t)&e->p, card->index, e->p.generation, e->p.header[1], e->p.header[2]); - card->driver->send_request(card, &e->p); + smp_load_acquire(&card->driver)->send_request(card, &e->p); return 0; } diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index 4e79c57263ff..a68100d3da0b 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -261,14 +261,14 @@ EXPORT_SYMBOL(fw_bus_type); int fw_device_enable_phys_dma(struct fw_device *device) { - int generation = device->generation; + struct fw_card *card = device->card; /* device->node_id, accessed below, must not be older than generation */ + int generation = device->generation; smp_rmb(); + int node_id = device->node_id; - return device->card->driver->enable_phys_dma(device->card, - device->node_id, - generation); + return smp_load_acquire(&card->driver)->enable_phys_dma(device->card, node_id, generation); } EXPORT_SYMBOL(fw_device_enable_phys_dma); diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c index d2b40a1a565e..fd6cbbf79d0b 100644 --- a/drivers/firewire/core-iso.c +++ b/drivers/firewire/core-iso.c @@ -144,8 +144,8 @@ struct fw_iso_context *__fw_iso_context_create(struct fw_card *card, int type, i { struct fw_iso_context *ctx; - ctx = card->driver->allocate_iso_context(card, type, channel, header_size, - header_storage_size); + ctx = smp_load_acquire(&card->driver)->allocate_iso_context(card, type, channel, + header_size, header_storage_size); if (IS_ERR(ctx)) return ctx; @@ -173,7 +173,9 @@ void fw_iso_context_destroy(struct fw_iso_context *ctx) trace_isoc_inbound_single_destroy(ctx); trace_isoc_inbound_multiple_destroy(ctx); - ctx->card->driver->free_iso_context(ctx); + struct fw_card *card = ctx->card; + + smp_load_acquire(&card->driver)->free_iso_context(ctx); } EXPORT_SYMBOL(fw_iso_context_destroy); @@ -184,7 +186,9 @@ int fw_iso_context_start(struct fw_iso_context *ctx, trace_isoc_inbound_single_start(ctx, cycle, sync, tags); trace_isoc_inbound_multiple_start(ctx, cycle, sync, tags); - return ctx->card->driver->start_iso(ctx, cycle, sync, tags); + struct fw_card *card = ctx->card; + + return smp_load_acquire(&card->driver)->start_iso(ctx, cycle, sync, tags); } EXPORT_SYMBOL(fw_iso_context_start); @@ -192,7 +196,9 @@ int fw_iso_context_set_channels(struct fw_iso_context *ctx, u64 *channels) { trace_isoc_inbound_multiple_channels(ctx, *channels); - return ctx->card->driver->set_iso_channels(ctx, channels); + struct fw_card *card = ctx->card; + + return smp_load_acquire(&card->driver)->set_iso_channels(ctx, channels); } int fw_iso_context_queue(struct fw_iso_context *ctx, @@ -204,7 +210,9 @@ int fw_iso_context_queue(struct fw_iso_context *ctx, trace_isoc_inbound_single_queue(ctx, payload, packet); trace_isoc_inbound_multiple_queue(ctx, payload, packet); - return ctx->card->driver->queue_iso(ctx, packet, buffer, payload); + struct fw_card *card = ctx->card; + + return smp_load_acquire(&card->driver)->queue_iso(ctx, packet, buffer, payload); } EXPORT_SYMBOL(fw_iso_context_queue); @@ -214,7 +222,9 @@ void fw_iso_context_queue_flush(struct fw_iso_context *ctx) trace_isoc_inbound_single_flush(ctx); trace_isoc_inbound_multiple_flush(ctx); - ctx->card->driver->flush_queue_iso(ctx); + struct fw_card *card = ctx->card; + + smp_load_acquire(&card->driver)->flush_queue_iso(ctx); } EXPORT_SYMBOL(fw_iso_context_queue_flush); @@ -248,7 +258,9 @@ int fw_iso_context_flush_completions(struct fw_iso_context *ctx) disable_work_sync(&ctx->work); - err = ctx->card->driver->flush_iso_completions(ctx); + struct fw_card *card = ctx->card; + + err = smp_load_acquire(&card->driver)->flush_iso_completions(ctx); enable_work(&ctx->work); @@ -270,7 +282,9 @@ int fw_iso_context_stop(struct fw_iso_context *ctx) if (WARN_ON_ONCE(current_work() == &ctx->work)) return 0; - err = ctx->card->driver->stop_iso(ctx); + struct fw_card *card = ctx->card; + + err = smp_load_acquire(&card->driver)->stop_iso(ctx); cancel_work_sync(&ctx->work); diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 5038a10f0717..589dd25b9785 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -133,7 +133,7 @@ __must_not_hold(&card->transactions.lock) // Cancel the packet transmission if it's still queued. That will schedule the packet // transmission callback which cancels the transaction. - if (card->driver->cancel_packet(card, &transaction->packet) == 0) + if (smp_load_acquire(&card->driver)->cancel_packet(card, &transaction->packet) == 0) return 0; // If the request packet has already been sent, we need to see if the transaction is still @@ -462,7 +462,7 @@ __must_not_hold(&card->transactions.lock) t->packet.header, payload, tcode_is_read_request(tcode) ? 0 : length / 4); - card->driver->send_request(card, &t->packet); + smp_load_acquire(&card->driver)->send_request(card, &t->packet); } EXPORT_SYMBOL_GPL(__fw_send_request); @@ -559,7 +559,7 @@ void fw_send_phy_config(struct fw_card *card, } if (gap_count == FW_PHY_CONFIG_CURRENT_GAP_COUNT) { - gap_count = card->driver->read_phy_reg(card, 1); + gap_count = smp_load_acquire(&card->driver)->read_phy_reg(card, 1); if (gap_count < 0) return; @@ -582,7 +582,7 @@ void fw_send_phy_config(struct fw_card *card, phy_config_packet.generation, phy_config_packet.header[1], phy_config_packet.header[2]); - card->driver->send_request(card, &phy_config_packet); + smp_load_acquire(&card->driver)->send_request(card, &phy_config_packet); wait_for_completion_timeout(&phy_config_done, timeout); } @@ -991,7 +991,7 @@ void fw_send_response(struct fw_card *card, request->response.header, data, data ? data_length / 4 : 0); - card->driver->send_response(card, &request->response); + smp_load_acquire(&card->driver)->send_response(card, &request->response); } EXPORT_SYMBOL(fw_send_response); @@ -1242,7 +1242,7 @@ __must_not_hold(&card->transactions.lock) * The response handler may be executed while the request handler * is still pending. Cancel the request handler. */ - card->driver->cancel_packet(card, &t->packet); + smp_load_acquire(&card->driver)->cancel_packet(card, &t->packet); invoke_callback(t, rcode, p->timestamp, data, data_length); } @@ -1361,17 +1361,17 @@ static void handle_registers(struct fw_card *card, struct fw_request *request, case CSR_BUS_TIME: case CSR_BUSY_TIMEOUT: if (tcode == TCODE_READ_QUADLET_REQUEST) - *data = cpu_to_be32(card->driver->read_csr(card, reg)); + *data = cpu_to_be32(smp_load_acquire(&card->driver)->read_csr(card, reg)); else if (tcode == TCODE_WRITE_QUADLET_REQUEST) - card->driver->write_csr(card, reg, be32_to_cpu(*data)); + smp_load_acquire(&card->driver)->write_csr(card, reg, be32_to_cpu(*data)); else rcode = RCODE_TYPE_ERROR; break; case CSR_RESET_START: if (tcode == TCODE_WRITE_QUADLET_REQUEST) - card->driver->write_csr(card, CSR_STATE_CLEAR, - CSR_STATE_BIT_ABDICATE); + smp_load_acquire(&card->driver)->write_csr(card, CSR_STATE_CLEAR, + CSR_STATE_BIT_ABDICATE); else rcode = RCODE_TYPE_ERROR; break; From e7f6c18a8c1e991d52cd49804f1f55902d54e7ba Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 18 Oct 2026 00:33:38 +0900 Subject: [PATCH 80/88] firewire: core: don't disable error work for transaction The error callback work is scheduled only when the transaction instance is no longer on the pending list. Therefore, iterating over the list to cancel the work is pointless. Remove the code that cancels the work while iterating over the pending list. Once scheduled, the work can run and invoke the error callback without being cancelled. Link: https://lore.kernel.org/r/20261017153339.267337-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 589dd25b9785..3f7c4ec4d618 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -44,7 +44,7 @@ static int try_cancel_split_timeout(struct fw_transaction *t) lockdep_assert_held(&t->card->transactions.lock); if (t->is_split_transaction) - return timer_delete(&t->split_timeout_timer) || disable_work(&t->error_work); + return timer_delete(&t->split_timeout_timer); else return 1; } @@ -150,16 +150,22 @@ __must_not_hold(&card->transactions.lock) transaction->response_timestamp = cycle_time_to_ohci_tstamp(curr_cycle_time); transaction->rcode = RCODE_CANCELLED; - enable_and_queue_work(card->async_wq, &transaction->error_work); + queue_work(card->async_wq, &transaction->error_work); return 0; } EXPORT_SYMBOL(fw_cancel_transaction); +// Ensure that the transaction is not on the pending list when scheduling this work. Cancelling +// the work after scheduling makes no sense, as the initiator must be notified of the transaction +// result. static void error_callback_work(struct work_struct *work) { struct fw_transaction *t = from_work(t, work, error_work); + // To detect programming mistakes. + WARN_ON(!list_empty(&t->link)); + invoke_callback(t, t->rcode, t->response_timestamp, NULL, 0); } @@ -418,6 +424,7 @@ __must_not_hold(&card->transactions.lock) t->with_tstamp = with_tstamp; t->callback_data = callback_data; INIT_WORK(&t->error_work, error_callback_work); + INIT_LIST_HEAD(&t->link); /* * Allocate tlabel from the bitmap and put the transaction on From 3e7ed554a85a21a1d4a9bff520f9f2f8b59c7f79 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 18 Oct 2026 00:33:39 +0900 Subject: [PATCH 81/88] firewire: core: reuse helper function for scheduling error callbacks The transaction layer has some code that can be replaced with the existing helper function for scheduling error callbacks. Link: https://lore.kernel.org/r/20261017153339.267337-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 3f7c4ec4d618..08536275be12 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -110,6 +110,14 @@ __must_not_hold(&card->transactions.lock) __t; \ }) +static void schedule_error_callback(struct fw_transaction *t, int rcode, u32 response_timestamp) +{ + t->rcode = rcode; + t->response_timestamp = response_timestamp; + + queue_work(t->card->async_wq, &t->error_work); +} + /** * fw_cancel_transaction: cancel transaction. * @card: interface where the transaction has been requested @@ -147,10 +155,9 @@ __must_not_hold(&card->transactions.lock) // Timestamping on behalf of hardware. (void)fw_card_read_cycle_time(card, &curr_cycle_time); - transaction->response_timestamp = cycle_time_to_ohci_tstamp(curr_cycle_time); - transaction->rcode = RCODE_CANCELLED; - queue_work(card->async_wq, &transaction->error_work); + schedule_error_callback(transaction, RCODE_CANCELLED, + cycle_time_to_ohci_tstamp(curr_cycle_time)); return 0; } @@ -169,14 +176,6 @@ static void error_callback_work(struct work_struct *work) invoke_callback(t, t->rcode, t->response_timestamp, NULL, 0); } -static void schedule_error_callback(struct fw_transaction *t, int rcode, u32 response_timestamp) -{ - t->rcode = rcode; - t->response_timestamp = response_timestamp; - - queue_work(t->card->async_wq, &t->error_work); -} - // card->transactions.lock must be released in advance. static void split_transaction_timeout_callback(struct timer_list *timer) { From 767e87bb6f0f721e0b97640780ff3ab1d413b329 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 18 Oct 2026 15:30:54 +0900 Subject: [PATCH 82/88] firewire: core: use workqueue to cancel all pending transactions on card removal When a card is being removed, all pending transactions are canceled by calling fw_cancel_pending_transactions(). This function moves the entries for unfinished transactions to a local list so that their callbacks can be invoked without holding the transaction lock. However, this complicates the management of pending transactions. Use workqueue to invoke error callbacks for all pending transactions. Link: https://lore.kernel.org/r/20261018063056.329226-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 4 +++- drivers/firewire/core-transaction.c | 37 ++++++++++++----------------- 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 83c29a487d86..a43fc3a278f2 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -811,7 +811,9 @@ void fw_core_remove_card(struct fw_card *card) drain_workqueue(card->isoc_wq); drain_workqueue(card->async_wq); card->driver->disable(card); - fw_cancel_pending_transactions(card); + + scoped_guard(spinlock_irqsave, &card->transactions.lock) + fw_cancel_pending_transactions(card); scoped_guard(spinlock_irqsave, &card->lock) fw_destroy_nodes(card); diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 08536275be12..8e4951631185 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -69,28 +69,6 @@ static void invoke_callback(struct fw_transaction *t, int rcode, u32 response_ts } } -// Must be called without holding card->transactions.lock. -void fw_cancel_pending_transactions(struct fw_card *card) -__must_not_hold(&card->transactions.lock) -{ - struct fw_transaction *t, *tmp; - LIST_HEAD(pending_list); - - lockdep_assert_not_held(&card->transactions.lock); - - scoped_guard(spinlock_irqsave, &card->transactions.lock) { - list_for_each_entry_safe(t, tmp, &card->transactions.list, link) { - if (try_cancel_split_timeout(t)) - list_move(&t->link, &pending_list); - } - } - - list_for_each_entry_safe(t, tmp, &pending_list, link) { - list_del(&t->link); - invoke_callback(t, RCODE_CANCELLED, 0, NULL, 0); - } -} - // card->transactions.lock must be acquired in advance. #define find_and_pop_transaction_entry(card, condition) \ ({ \ @@ -118,6 +96,21 @@ static void schedule_error_callback(struct fw_transaction *t, int rcode, u32 res queue_work(t->card->async_wq, &t->error_work); } +void fw_cancel_pending_transactions(struct fw_card *card) +__must_hold(&card->transactions.lock) +{ + struct fw_transaction *t, *tmp; + + lockdep_assert_held(&card->transactions.lock); + + list_for_each_entry_safe(t, tmp, &card->transactions.list, link) { + if (try_cancel_split_timeout(t)) { + remove_transaction_entry(card, t); + schedule_error_callback(t, RCODE_CANCELLED, 0); + } + } +} + /** * fw_cancel_transaction: cancel transaction. * @card: interface where the transaction has been requested From 31ab33a6e3721902f3dbfbf1fd3116ef4d683810 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 18 Oct 2026 15:30:55 +0900 Subject: [PATCH 83/88] firewire: core: check precondition when scheduling error callbacks Current implementation requires that the transaction not be on the pending list before its error callback is scheduled. A WARN_ON() in the callback function detects violations of this precondition, but the check belongs in the scheduling helper. Move the WARN_ON() to schedule_error_callback(). Link: https://lore.kernel.org/r/20261018063056.329226-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 8e4951631185..71e3a1b07221 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -88,8 +88,14 @@ static void invoke_callback(struct fw_transaction *t, int rcode, u32 response_ts __t; \ }) +// Ensure that the transaction is not on the pending list when scheduling this work. Cancelling +// the work after scheduling makes no sense, as the initiator must be notified of the transaction +// result. static void schedule_error_callback(struct fw_transaction *t, int rcode, u32 response_timestamp) { + // To detect programming mistakes. + WARN_ON(!list_empty(&t->link)); + t->rcode = rcode; t->response_timestamp = response_timestamp; @@ -156,16 +162,10 @@ __must_not_hold(&card->transactions.lock) } EXPORT_SYMBOL(fw_cancel_transaction); -// Ensure that the transaction is not on the pending list when scheduling this work. Cancelling -// the work after scheduling makes no sense, as the initiator must be notified of the transaction -// result. static void error_callback_work(struct work_struct *work) { struct fw_transaction *t = from_work(t, work, error_work); - // To detect programming mistakes. - WARN_ON(!list_empty(&t->link)); - invoke_callback(t, t->rcode, t->response_timestamp, NULL, 0); } From d7a7c63437d3c5db282ebff08f5258e906ae9920 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 18 Oct 2026 15:30:56 +0900 Subject: [PATCH 84/88] firewire: core: use context analysis to check lock state when scheduling error callbacks The precondition for schedule_error_callback() involves accessing the pending transaction list. This access should be protected by the transaction lock. Use compiler-based context analysis to verify that the lock is held when accessing the list. Link: https://lore.kernel.org/r/20261018063056.329226-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 30 ++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 71e3a1b07221..e24fb6b97770 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -91,8 +91,12 @@ static void invoke_callback(struct fw_transaction *t, int rcode, u32 response_ts // Ensure that the transaction is not on the pending list when scheduling this work. Cancelling // the work after scheduling makes no sense, as the initiator must be notified of the transaction // result. -static void schedule_error_callback(struct fw_transaction *t, int rcode, u32 response_timestamp) +static void schedule_error_callback(struct fw_card *card, struct fw_transaction *t, int rcode, + u32 response_timestamp) +__must_hold(&card->transactions.lock) { + lockdep_assert_held(&card->transactions.lock); + // To detect programming mistakes. WARN_ON(!list_empty(&t->link)); @@ -112,7 +116,7 @@ __must_hold(&card->transactions.lock) list_for_each_entry_safe(t, tmp, &card->transactions.list, link) { if (try_cancel_split_timeout(t)) { remove_transaction_entry(card, t); - schedule_error_callback(t, RCODE_CANCELLED, 0); + schedule_error_callback(card, t, RCODE_CANCELLED, 0); } } } @@ -143,20 +147,20 @@ __must_not_hold(&card->transactions.lock) if (smp_load_acquire(&card->driver)->cancel_packet(card, &transaction->packet) == 0) return 0; - // If the request packet has already been sent, we need to see if the transaction is still - // pending and remove it in that case (e.g. the split transaction). - scoped_guard(spinlock_irqsave, &card->transactions.lock) { - if (!find_and_pop_transaction_entry(card, iter == transaction)) - return -ENOENT; - } - u32 curr_cycle_time = 0; // Timestamping on behalf of hardware. (void)fw_card_read_cycle_time(card, &curr_cycle_time); - schedule_error_callback(transaction, RCODE_CANCELLED, - cycle_time_to_ohci_tstamp(curr_cycle_time)); + u32 tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); + + // If the request packet has already been sent, we need to see if the transaction is still + // pending and remove it in that case (e.g. the split transaction). + scoped_guard(spinlock_irqsave, &card->transactions.lock) { + if (!find_and_pop_transaction_entry(card, iter == transaction)) + return -ENOENT; + schedule_error_callback(card, transaction, RCODE_CANCELLED, tstamp); + } return 0; } @@ -181,9 +185,9 @@ static void split_transaction_timeout_callback(struct timer_list *timer) if (list_empty(&t->link)) return; remove_transaction_entry(card, t); + schedule_error_callback(card, t, RCODE_CANCELLED, t->split_timeout_cycle); } - schedule_error_callback(t, RCODE_CANCELLED, t->split_timeout_cycle); } // card->transactions.lock should be acquired in advance for the linked list. @@ -434,7 +438,7 @@ __must_not_hold(&card->transactions.lock) tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); t->packet.timestamp = tstamp; - schedule_error_callback(t, RCODE_SEND_ERROR, tstamp); + context_unsafe(schedule_error_callback(card, t, RCODE_SEND_ERROR, tstamp)); return; } From 2b6748c5c673f1ce9c57c577dbe995952f29c2c9 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 18 Oct 2026 22:29:54 +0900 Subject: [PATCH 85/88] firewire: core: use designated initializer for dummy driver on card removal The dummy driver used during card removal contains mutable variables. Use a designated initializer instead of a static constant assignment. Link: https://lore.kernel.org/r/20261018132955.391090-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 44 ++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index a43fc3a278f2..83be100a1f4b 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -763,23 +763,6 @@ static int dummy_flush_iso_completions(struct fw_iso_context *ctx) return -ENODEV; } -static const struct fw_card_driver dummy_driver_template = { - .read_phy_reg = dummy_read_phy_reg, - .update_phy_reg = dummy_update_phy_reg, - .send_request = dummy_send_request, - .send_response = dummy_send_response, - .cancel_packet = dummy_cancel_packet, - .enable_phys_dma = dummy_enable_phys_dma, - .read_csr = dummy_read_csr, - .write_csr = dummy_write_csr, - .allocate_iso_context = dummy_allocate_iso_context, - .start_iso = dummy_start_iso, - .set_iso_channels = dummy_set_iso_channels, - .queue_iso = dummy_queue_iso, - .flush_queue_iso = dummy_flush_queue_iso, - .flush_iso_completions = dummy_flush_iso_completions, -}; - void fw_card_release(struct kref *kref) { struct fw_card *card = container_of(kref, struct fw_card, kref); @@ -790,7 +773,28 @@ EXPORT_SYMBOL_GPL(fw_card_release); void fw_core_remove_card(struct fw_card *card) { - struct fw_card_driver dummy_driver = dummy_driver_template; + // Switch off most of the card driver interface. + const struct fw_card_driver dummy_driver = { + .enable = NULL, + .disable = card->driver->disable, + .read_phy_reg = dummy_read_phy_reg, + .update_phy_reg = dummy_update_phy_reg, + .set_config_rom = NULL, + .send_request = dummy_send_request, + .send_response = dummy_send_response, + .cancel_packet = dummy_cancel_packet, + .enable_phys_dma = dummy_enable_phys_dma, + .read_csr = dummy_read_csr, + .write_csr = dummy_write_csr, + .allocate_iso_context = dummy_allocate_iso_context, + .free_iso_context = card->driver->free_iso_context, + .start_iso = dummy_start_iso, + .set_iso_channels = dummy_set_iso_channels, + .queue_iso = dummy_queue_iso, + .flush_queue_iso = dummy_flush_queue_iso, + .flush_iso_completions = dummy_flush_iso_completions, + .stop_iso = card->driver->stop_iso, + }; might_sleep(); @@ -801,10 +805,6 @@ void fw_core_remove_card(struct fw_card *card) scoped_guard(mutex, &card_mutex) list_del_init(&card->link); - /* Switch off most of the card driver interface. */ - dummy_driver.free_iso_context = card->driver->free_iso_context; - dummy_driver.stop_iso = card->driver->stop_iso; - dummy_driver.disable = card->driver->disable; // Ensure the entire contents of the driver are visible to other CPUs. smp_store_release(&card->driver, &dummy_driver); From 7d1e256fa1d0d72bcb1185a83e7251331cb6ec21 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 18 Oct 2026 22:29:55 +0900 Subject: [PATCH 86/88] firewire: core: invoke packet callback from workqueue when removing card instance Current implementation switches the underlying driver of a card instance to the dummy driver in the kernel stack when removing the card. The dummy driver simply invokes callbacks when sending request and response packets. Recent changes for the 1394 OHCI driver and core transaction layer ensure that callbacks are invoked in process context from a workqueue. This guarantees a consistent execution context for callbacks. However, card removel is currently an exception. Invoke packet callbacks from a workqueue during card removal. Link: https://lore.kernel.org/r/20261018132955.391090-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 89 +++++++++++++++++++++++++----------- 1 file changed, 63 insertions(+), 26 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 83be100a1f4b..b4d40dc7720f 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -689,6 +689,15 @@ EXPORT_SYMBOL(fw_card_add); * .set_config_rom() is never called since the card is taken out of card_list * before switching to the dummy driver. */ +struct fw_card_driver_closer { + const struct fw_card_driver driver; + + // For shutdown operations. + struct fw_card *card; + struct list_head packet_list; + spinlock_t packet_list_lock; + struct work_struct work; +}; static int dummy_read_phy_reg(struct fw_card *card, int address) { @@ -701,14 +710,31 @@ static int dummy_update_phy_reg(struct fw_card *card, int address, return -ENODEV; } -static void dummy_send_request(struct fw_card *card, struct fw_packet *packet) +static void dummy_packet_callback_work(struct work_struct *work) { - packet->callback(packet, card, RCODE_CANCELLED); + struct fw_card_driver_closer *closer = container_of(work, typeof(*closer), work); + unsigned long flags; + struct fw_packet *p; + + spin_lock_irqsave(&closer->packet_list_lock, flags); + while ((p = list_first_entry_or_null(&closer->packet_list, typeof(*p), link_for_local))) { + list_del(&p->link_for_local); + spin_unlock_irqrestore(&closer->packet_list_lock, flags); + + p->callback(p, closer->card, RCODE_CANCELLED); + + spin_lock_irqsave(&closer->packet_list_lock, flags); + } + spin_unlock_irqrestore(&closer->packet_list_lock, flags); } -static void dummy_send_response(struct fw_card *card, struct fw_packet *packet) +static void dummy_schedule_packet_callback(struct fw_card *card, struct fw_packet *packet) { - packet->callback(packet, card, RCODE_CANCELLED); + struct fw_card_driver_closer *closer = container_of(card->driver, typeof(*closer), driver); + + scoped_guard(spinlock_irqsave, &closer->packet_list_lock) + list_add_tail(&packet->link_for_local, &closer->packet_list); + queue_work(card->async_wq, &closer->work); } static int dummy_cancel_packet(struct fw_card *card, struct fw_packet *packet) @@ -773,27 +799,30 @@ EXPORT_SYMBOL_GPL(fw_card_release); void fw_core_remove_card(struct fw_card *card) { - // Switch off most of the card driver interface. - const struct fw_card_driver dummy_driver = { - .enable = NULL, - .disable = card->driver->disable, - .read_phy_reg = dummy_read_phy_reg, - .update_phy_reg = dummy_update_phy_reg, - .set_config_rom = NULL, - .send_request = dummy_send_request, - .send_response = dummy_send_response, - .cancel_packet = dummy_cancel_packet, - .enable_phys_dma = dummy_enable_phys_dma, - .read_csr = dummy_read_csr, - .write_csr = dummy_write_csr, - .allocate_iso_context = dummy_allocate_iso_context, - .free_iso_context = card->driver->free_iso_context, - .start_iso = dummy_start_iso, - .set_iso_channels = dummy_set_iso_channels, - .queue_iso = dummy_queue_iso, - .flush_queue_iso = dummy_flush_queue_iso, - .flush_iso_completions = dummy_flush_iso_completions, - .stop_iso = card->driver->stop_iso, + struct fw_card_driver_closer closer = { + // Switch off most of the card driver interface. + .driver = { + .enable = NULL, + .disable = card->driver->disable, + .read_phy_reg = dummy_read_phy_reg, + .update_phy_reg = dummy_update_phy_reg, + .set_config_rom = NULL, + .send_request = dummy_schedule_packet_callback, + .send_response = dummy_schedule_packet_callback, + .cancel_packet = dummy_cancel_packet, + .enable_phys_dma = dummy_enable_phys_dma, + .read_csr = dummy_read_csr, + .write_csr = dummy_write_csr, + .allocate_iso_context = dummy_allocate_iso_context, + .free_iso_context = card->driver->free_iso_context, + .start_iso = dummy_start_iso, + .set_iso_channels = dummy_set_iso_channels, + .queue_iso = dummy_queue_iso, + .flush_queue_iso = dummy_flush_queue_iso, + .flush_iso_completions = dummy_flush_iso_completions, + .stop_iso = card->driver->stop_iso, + }, + .card = card, }; might_sleep(); @@ -805,8 +834,12 @@ void fw_core_remove_card(struct fw_card *card) scoped_guard(mutex, &card_mutex) list_del_init(&card->link); + INIT_LIST_HEAD(&closer.packet_list); + spin_lock_init(&closer.packet_list_lock); + INIT_WORK_ONSTACK(&closer.work, dummy_packet_callback_work); + // Ensure the entire contents of the driver are visible to other CPUs. - smp_store_release(&card->driver, &dummy_driver); + smp_store_release(&card->driver, &closer.driver); drain_workqueue(card->isoc_wq); drain_workqueue(card->async_wq); @@ -825,6 +858,10 @@ void fw_core_remove_card(struct fw_card *card) destroy_workqueue(card->isoc_wq); destroy_workqueue(card->async_wq); + disable_work_sync(&closer.work); + destroy_work_on_stack(&closer.work); + + WARN_ON(!list_empty(&closer.packet_list)); WARN_ON(!list_empty(&card->transactions.list)); } EXPORT_SYMBOL(fw_core_remove_card); From 3e3858e479a8fc0c78afb7da8ff65ed053f8ddce Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 19 Oct 2026 21:25:54 +0900 Subject: [PATCH 87/88] firewire: core: adjust context_unsafe() usage for lockdep checks schedule_error_callback() requires a lock to be held by the caller. However, __fw_send_request() calls the function without holding any lock when retrieving the transaction label fails, as no lock is needed in this case. context_safe() is used to suppress the precondition check in compiler-based context analysis, but it does not affect lockdep checks. As a result, lockdep reports a violation of the precondition. Acquire the lock before calling the function instead of using context_unsafe(). Link: https://lore.kernel.org/r/20261019122555.328060-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index e24fb6b97770..0ebe70a4d440 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -438,7 +438,8 @@ __must_not_hold(&card->transactions.lock) tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); t->packet.timestamp = tstamp; - context_unsafe(schedule_error_callback(card, t, RCODE_SEND_ERROR, tstamp)); + scoped_guard(spinlock_irqsave, &card->transactions.lock) + schedule_error_callback(card, t, RCODE_SEND_ERROR, tstamp); return; } From 06d410ba93d33a6c78e917d54e821355492e3ce4 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Mon, 19 Oct 2026 21:25:55 +0900 Subject: [PATCH 88/88] firewire: core: replace drain_workqueue() with flush_workqueue() drain_workqueue() allows only chained work to be queued while draining. This is inconvenient because work can be queued from contexts other than work items already executing on the workqueue. Use flush_workqueue() instead to allow work to be queued from any context. Link: https://lore.kernel.org/r/20261019122555.328060-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-card.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index b4d40dc7720f..b35e8901cc4b 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -841,8 +841,8 @@ void fw_core_remove_card(struct fw_card *card) // Ensure the entire contents of the driver are visible to other CPUs. smp_store_release(&card->driver, &closer.driver); - drain_workqueue(card->isoc_wq); - drain_workqueue(card->async_wq); + flush_workqueue(card->isoc_wq); + flush_workqueue(card->async_wq); card->driver->disable(card); scoped_guard(spinlock_irqsave, &card->transactions.lock)