linux1394-devel

[PATCH] firewire: core: fix __must_hold() annotation

[Takashi S. <o-t...@sa...>]

The variable name passed to __must_hold() annotation is invalid.

This commit fixes it.

Fixes: 420bd7068cbf ("firewire: core: use spin lock specific to transaction")
Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index dd3656a0c1ff..c65f491c54d0 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -269,7 +269,7 @@ static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel,
 }
 
 static int allocate_tlabel(struct fw_card *card)
-__must_hold(&card->transactions_lock)
+__must_hold(&card->transactions.lock)
 {
 	int tlabel;
 

base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
-- 
2.51.0

Re: [PATCH 0/4] firewire: ohci: remove obsolete module-level debug parameter

[Adam G. <ad...@po...>]

On Thu, Aug 21, 2025 at 09:30:13AM +0900, Takashi Sakamoto wrote:
> The "firewire-ohci" module has long provided a "debug" parameter that
> enabled debug logging by calling printk() from hardIRQ context.
> 
> Between v6.11 and v6.12, a series of tracepoints events have been added as
> a more suitable alternative. Since v6.12, a commit cd7023729877
> ("firewire: ohci: deprecate debug parameter") has already marked the
> parameter as deprecated.
> 
> This series removes the parameter, as its functionality is now fully
> covered by tracepoints.

Hi Takashi,

Now that the "debug" parameter has been removed, can you provide 
instructions for using tracepoints? For example, what is the new 
procedure instead of adding "debug=7" to the module command line? What 
is the equivalent to 
"echo -1 > /sys/module/firewire_ohci/parameters/debug"?

-- Adam

Re: [PATCH] firewire: core: use cleanup function to release cached configuration ROM

[Takashi S. <o-t...@sa...>]

On Mon, Oct 20, 2025 at 08:58:10PM +0900, Takashi Sakamoto wrote:
> When returning from read_config_rom() function, the allocated buffer and
> the previous buffer for configuration ROM should be released. The cleanup
> function is useful in the case.
> 
> This commit uses the cleanup function to remove goto statements.
> 
> Signed-off-by: Takashi Sakamoto <o-t...@sa...>
> ---
>  drivers/firewire/core-device.c | 34 ++++++++++++----------------------
>  1 file changed, 12 insertions(+), 22 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto

[PATCH] firewire: core: use cleanup function to release cached configuration ROM

[Takashi S. <o-t...@sa...>]

When returning from read_config_rom() function, the allocated buffer and
the previous buffer for configuration ROM should be released. The cleanup
function is useful in the case.

This commit uses the cleanup function to remove goto statements.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-device.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 1674de477852..9b0080397154 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -653,8 +653,8 @@ static int read_rom(struct fw_device *device, int generation, int speed, int ind
 static int read_config_rom(struct fw_device *device, int generation)
 {
 	struct fw_card *card = device->card;
-	const u32 *old_rom, *new_rom;
-	u32 *rom, *stack;
+	const u32 *new_rom, *old_rom __free(kfree) = NULL;
+	u32 *stack, *rom __free(kfree) = NULL;
 	u32 sp, key;
 	int i, end, length, ret, speed;
 	int quirks;
@@ -673,7 +673,7 @@ static int read_config_rom(struct fw_device *device, int generation)
 	for (i = 0; i < 5; i++) {
 		ret = read_rom(device, generation, speed, i, &rom[i]);
 		if (ret != RCODE_COMPLETE)
-			goto out;
+			return ret;
 		/*
 		 * As per IEEE1212 7.2, during initialization, devices can
 		 * reply with a 0 for the first quadlet of the config
@@ -682,10 +682,8 @@ static int read_config_rom(struct fw_device *device, int generation)
 		 * harddisk).  In that case we just fail, and the
 		 * retry mechanism will try again later.
 		 */
-		if (i == 0 && rom[i] == 0) {
-			ret = RCODE_BUSY;
-			goto out;
-		}
+		if (i == 0 && rom[i] == 0)
+			return RCODE_BUSY;
 	}
 
 	quirks = detect_quirks_by_bus_information_block(rom);
@@ -712,15 +710,13 @@ static int read_config_rom(struct fw_device *device, int generation)
 		 */
 		key = stack[--sp];
 		i = key & 0xffffff;
-		if (WARN_ON(i >= MAX_CONFIG_ROM_SIZE)) {
-			ret = -ENXIO;
-			goto out;
-		}
+		if (WARN_ON(i >= MAX_CONFIG_ROM_SIZE))
+			return -ENXIO;
 
 		/* Read header quadlet for the block to get the length. */
 		ret = read_rom(device, generation, speed, i, &rom[i]);
 		if (ret != RCODE_COMPLETE)
-			goto out;
+			return ret;
 		end = i + (rom[i] >> 16) + 1;
 		if (end > MAX_CONFIG_ROM_SIZE) {
 			/*
@@ -744,7 +740,7 @@ static int read_config_rom(struct fw_device *device, int generation)
 		for (; i < end; i++) {
 			ret = read_rom(device, generation, speed, i, &rom[i]);
 			if (ret != RCODE_COMPLETE)
-				goto out;
+				return ret;
 
 			if ((key >> 30) != 3 || (rom[i] >> 30) < 2)
 				continue;
@@ -804,25 +800,19 @@ static int read_config_rom(struct fw_device *device, int generation)
 
 	old_rom = device->config_rom;
 	new_rom = kmemdup(rom, length * 4, GFP_KERNEL);
-	if (new_rom == NULL) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (new_rom == NULL)
+		return -ENOMEM;
 
 	scoped_guard(rwsem_write, &fw_device_rwsem) {
 		device->config_rom = new_rom;
 		device->config_rom_length = length;
 	}
 
-	kfree(old_rom);
-	ret = RCODE_COMPLETE;
 	device->max_rec	= rom[2] >> 12 & 0xf;
 	device->cmc	= rom[2] >> 30 & 1;
 	device->irmc	= rom[2] >> 31 & 1;
- out:
-	kfree(rom);
 
-	return ret;
+	return RCODE_COMPLETE;
 }
 
 static void fw_unit_release(struct device *dev)

base-commit: dbd0cf204fe6ba7ba226153d1d90369019b90164
-- 
2.51.0

Re: [PATCH 0/4] firewire: core: handle TASCAM FW-1884/FW-1804/FW-1082 quirk

[Takashi S. <o-t...@sa...>]

On Sat, Oct 18, 2025 at 12:55:28PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> In 2003, TEAC Corporation had released FW-1884/FW-1804/FW-1082 in its
> TASCAM brand. These devices are already supported by a driver in ALSA
> firewire stack, but they have an interoperability issue related to
> the speed of asynchronous transactions and isochronous transmissions.
> When operating at the speed described in configuration ROM, they are
> too lazy to respond, and eventually frozen.
> 
> The most likely cause of this issue is a mismatch in the gap count
> between the initiators and receivers. Theoretically, this can be
> resolved by transmitting phy configuration packets to optimize gap count.
> Nevertheless, this approach has proven ineffective, suggesting that the
> device firmware may contain a bug causing the issue.
> 
> From my experience, these devices operate more reliably at lower
> transaction and transmission speeds, which provides a practical
> mitigation.
> 
> This patch series addresses the interoperability issue. The core function
> of Linux FireWire subsystem is changed to read the entire configuration
> ROM at the lowest speed (S100), and to identify these devices based on its
> contents. Once identified, their maximum speed is limited to S200. The
> ALSA driver then performs asynchronous requests and isochronous
> transmission at that speed to prevent device freezes.
> 
> Takashi Sakamoto (4):
>   firewire: core: code refactoring to compute transaction speed
>   firewire: core: determine transaction speed after detecting quirks
>   firewire: core: handle device quirk of TASCAM FW-1884/FW-1804/FW-1082
>   ALSA: firewire-tascam: reserve resources for transferred isochronous
>     packets at S400
> 
>  drivers/firewire/core-device.c        | 86 +++++++++++++++------------
>  include/linux/firewire.h              |  3 +
>  sound/firewire/tascam/tascam-stream.c | 21 +++----
>  3 files changed, 63 insertions(+), 47 deletions(-)
> 
> 
> base-commit: 15f9610fc96ac6fd2844e63f7bf5a0b08e1c31c8

Applied to for-next branch.

To sound subsystem maintainer, I'll send the 4th patch to mainline as a
part of firewire subsystem updates in next merge window.


Regards

Takashi Sakamoto
`

[PATCH 3/4] firewire: core: handle device quirk of TASCAM FW-1884/FW-1804/FW-1082

[Takashi S. <o-t...@sa...>]

TASCAM FW-1884/FW-1804/FW-1082 is too lazy to repspond to asynchronous
request at S400. The asynchronous transaction often results in timeout.
This is a problematic quirk.

This commit adds support for the quirk. When identifying the new quirk
flag, then the transaction speed is configured at S200.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-device.c | 18 +++++++++++++++++-
 include/linux/firewire.h       |  3 +++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 6a5740ed4934..1674de477852 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -571,6 +571,14 @@ static const struct entry_match motu_audio_express_matches[] = {
 	{ 8, 0x17104800 },
 };
 
+static const struct entry_match tascam_fw_series_matches[] = {
+	{ 1, 0x0300022e },
+	{ 3, 0x8d000006 },
+	{ 4, 0xd1000001 },
+	{ 6, 0x1200022e },
+	{ 8, 0xd4000004 },
+};
+
 static int detect_quirks_by_root_directory(const u32 *root_directory, unsigned int length)
 {
 	static const struct {
@@ -583,6 +591,11 @@ static int detect_quirks_by_root_directory(const u32 *root_directory, unsigned i
 			.matches = motu_audio_express_matches,
 			.match_count = ARRAY_SIZE(motu_audio_express_matches),
 		},
+		{
+			.quirk = FW_DEVICE_QUIRK_UNSTABLE_AT_S400,
+			.matches = tascam_fw_series_matches,
+			.match_count = ARRAY_SIZE(tascam_fw_series_matches),
+		},
 	};
 	int quirks = 0;
 	int i;
@@ -761,7 +774,10 @@ static int read_config_rom(struct fw_device *device, int generation)
 	// Just prevent from torn writing/reading.
 	WRITE_ONCE(device->quirks, quirks);
 
-	speed = device->node->max_speed;
+	if (unlikely(quirks & FW_DEVICE_QUIRK_UNSTABLE_AT_S400))
+		speed = SCODE_200;
+	else
+		speed = device->node->max_speed;
 
 	// Determine the speed of
 	//   - devices with link speed less than PHY speed,
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index f1d8734c0ec6..6143b7d28eac 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -179,6 +179,9 @@ enum fw_device_quirk {
 
 	// MOTU Audio Express transfers acknowledge packet with 0x10 for pending state.
 	FW_DEVICE_QUIRK_ACK_PACKET_WITH_INVALID_PENDING_CODE = BIT(2),
+
+	// TASCAM FW-1082/FW-1804/FW-1884 often freezes when receiving S400 packets.
+	FW_DEVICE_QUIRK_UNSTABLE_AT_S400 = BIT(3),
 };
 
 enum fw_device_state {
-- 
2.51.0

[PATCH 1/4] firewire: core: code refactoring to compute transaction speed

[Takashi S. <o-t...@sa...>]

This commit refactors the helper function to read the content of
configuration ROM with the passed speed.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-device.c | 35 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 33ce4cd357ed..c698d4ced7d7 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -605,8 +605,7 @@ static int detect_quirks_by_root_directory(const u32 *root_directory, unsigned i
 	return quirks;
 }
 
-static int read_rom(struct fw_device *device,
-		    int generation, int index, u32 *data)
+static int read_rom(struct fw_device *device, int generation, int speed, int index, u32 *data)
 {
 	u64 offset = (CSR_REGISTER_BASE | CSR_CONFIG_ROM) + index * 4;
 	int i, rcode;
@@ -617,7 +616,7 @@ static int read_rom(struct fw_device *device,
 	for (i = 10; i < 100; i += 10) {
 		rcode = fw_run_transaction(device->card,
 				TCODE_READ_QUADLET_REQUEST, device->node_id,
-				generation, device->max_speed, offset, data, 4);
+				generation, speed, offset, data, 4);
 		if (rcode != RCODE_BUSY)
 			break;
 		msleep(i);
@@ -644,7 +643,7 @@ static int read_config_rom(struct fw_device *device, int generation)
 	const u32 *old_rom, *new_rom;
 	u32 *rom, *stack;
 	u32 sp, key;
-	int i, end, length, ret;
+	int i, end, length, ret, speed;
 	int quirks;
 
 	rom = kmalloc(sizeof(*rom) * MAX_CONFIG_ROM_SIZE +
@@ -655,11 +654,11 @@ static int read_config_rom(struct fw_device *device, int generation)
 	stack = &rom[MAX_CONFIG_ROM_SIZE];
 	memset(rom, 0, sizeof(*rom) * MAX_CONFIG_ROM_SIZE);
 
-	device->max_speed = SCODE_100;
+	speed = SCODE_100;
 
 	/* First read the bus info block. */
 	for (i = 0; i < 5; i++) {
-		ret = read_rom(device, generation, i, &rom[i]);
+		ret = read_rom(device, generation, speed, i, &rom[i]);
 		if (ret != RCODE_COMPLETE)
 			goto out;
 		/*
@@ -681,7 +680,7 @@ static int read_config_rom(struct fw_device *device, int generation)
 	// Just prevent from torn writing/reading.
 	WRITE_ONCE(device->quirks, quirks);
 
-	device->max_speed = device->node->max_speed;
+	speed = device->node->max_speed;
 
 	/*
 	 * Determine the speed of
@@ -692,20 +691,18 @@ static int read_config_rom(struct fw_device *device, int generation)
 	 * because some buggy firmwares set it lower than necessary and because
 	 * 1394-1995 nodes do not have the field.
 	 */
-	if ((rom[2] & 0x7) < device->max_speed ||
-	    device->max_speed == SCODE_BETA ||
-	    card->beta_repeaters_present) {
+	if ((rom[2] & 0x7) < speed || speed == SCODE_BETA || card->beta_repeaters_present) {
 		u32 dummy;
 
 		/* for S1600 and S3200 */
-		if (device->max_speed == SCODE_BETA)
-			device->max_speed = card->link_speed;
+		if (speed == SCODE_BETA)
+			speed = card->link_speed;
 
-		while (device->max_speed > SCODE_100) {
-			if (read_rom(device, generation, 0, &dummy) ==
+		while (speed > SCODE_100) {
+			if (read_rom(device, generation, speed, 0, &dummy) ==
 			    RCODE_COMPLETE)
 				break;
-			device->max_speed--;
+			--speed;
 		}
 	}
 
@@ -734,7 +731,7 @@ static int read_config_rom(struct fw_device *device, int generation)
 		}
 
 		/* Read header quadlet for the block to get the length. */
-		ret = read_rom(device, generation, i, &rom[i]);
+		ret = read_rom(device, generation, speed, i, &rom[i]);
 		if (ret != RCODE_COMPLETE)
 			goto out;
 		end = i + (rom[i] >> 16) + 1;
@@ -758,7 +755,7 @@ static int read_config_rom(struct fw_device *device, int generation)
 		 * it references another block, and push it in that case.
 		 */
 		for (; i < end; i++) {
-			ret = read_rom(device, generation, i, &rom[i]);
+			ret = read_rom(device, generation, speed, i, &rom[i]);
 			if (ret != RCODE_COMPLETE)
 				goto out;
 
@@ -785,6 +782,8 @@ static int read_config_rom(struct fw_device *device, int generation)
 			length = i;
 	}
 
+	device->max_speed = speed;
+
 	quirks |= detect_quirks_by_root_directory(rom + ROOT_DIR_OFFSET, length - ROOT_DIR_OFFSET);
 
 	// Just prevent from torn writing/reading.
@@ -1234,7 +1233,7 @@ static int reread_config_rom(struct fw_device *device, int generation,
 	int i, rcode;
 
 	for (i = 0; i < 6; i++) {
-		rcode = read_rom(device, generation, i, &q);
+		rcode = read_rom(device, generation, device->max_speed, i, &q);
 		if (rcode != RCODE_COMPLETE)
 			return rcode;
 
-- 
2.51.0

[PATCH 4/4] ALSA: firewire-tascam: reserve resources for transferred isochronous packets at S400

[Takashi S. <o-t...@sa...>]

TASCAM FW-1884/FW-1804/FW-1082 have a quirk that they often freeze when
receiving isochronous packets at S400. This behaviour is suppressed by a
new quirk flag added in Linux FireWire core to restrict maximum speed.
Consequently both of the asynchronous transactions and isochronous
transmissions are done at S200. However, the device still transfers
isochronous packet at S400, and the way to indicate the transmission
speed is not cleared yet.

This commit correctly reserves isochronous resources for the transferred
packet stream at S400. As a beneficial side effect, the pair of
isochronous transmissions for FW-1884 fits within the bandwidth capacity
of the bus.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 sound/firewire/tascam/tascam-stream.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/sound/firewire/tascam/tascam-stream.c b/sound/firewire/tascam/tascam-stream.c
index 9c8fddd7dee1..4ecd151a46c1 100644
--- a/sound/firewire/tascam/tascam-stream.c
+++ b/sound/firewire/tascam/tascam-stream.c
@@ -282,20 +282,22 @@ static int keep_resources(struct snd_tscm *tscm, unsigned int rate,
 			  struct amdtp_stream *stream)
 {
 	struct fw_iso_resources *resources;
+	int speed;
 	int err;
 
-	if (stream == &tscm->tx_stream)
+	if (stream == &tscm->tx_stream) {
 		resources = &tscm->tx_resources;
-	else
+		speed = fw_parent_device(tscm->unit)->max_speed;
+	} else {
 		resources = &tscm->rx_resources;
+		speed = SCODE_400;
+	}
 
 	err = amdtp_tscm_set_parameters(stream, rate);
 	if (err < 0)
 		return err;
 
-	return fw_iso_resources_allocate(resources,
-				amdtp_stream_get_max_payload(stream),
-				fw_parent_device(tscm->unit)->max_speed);
+	return fw_iso_resources_allocate(resources, amdtp_stream_get_max_payload(stream), speed);
 }
 
 static int init_stream(struct snd_tscm *tscm, struct amdtp_stream *s)
@@ -455,7 +457,6 @@ int snd_tscm_stream_start_duplex(struct snd_tscm *tscm, unsigned int rate)
 	}
 
 	if (!amdtp_stream_running(&tscm->rx_stream)) {
-		int spd = fw_parent_device(tscm->unit)->max_speed;
 		unsigned int tx_init_skip_cycles;
 
 		err = set_stream_formats(tscm, rate);
@@ -466,13 +467,13 @@ int snd_tscm_stream_start_duplex(struct snd_tscm *tscm, unsigned int rate)
 		if (err < 0)
 			goto error;
 
-		err = amdtp_domain_add_stream(&tscm->domain, &tscm->rx_stream,
-					      tscm->rx_resources.channel, spd);
+		err = amdtp_domain_add_stream(&tscm->domain, &tscm->rx_stream, tscm->rx_resources.channel,
+					      fw_parent_device(tscm->unit)->max_speed);
 		if (err < 0)
 			goto error;
 
-		err = amdtp_domain_add_stream(&tscm->domain, &tscm->tx_stream,
-					      tscm->tx_resources.channel, spd);
+		err = amdtp_domain_add_stream(&tscm->domain, &tscm->tx_stream, tscm->tx_resources.channel,
+					      SCODE_400);
 		if (err < 0)
 			goto error;
 
-- 
2.51.0

[PATCH 0/4] firewire: core: handle TASCAM FW-1884/FW-1804/FW-1082 quirk

[Takashi S. <o-t...@sa...>]

Hi,

In 2003, TEAC Corporation had released FW-1884/FW-1804/FW-1082 in its
TASCAM brand. These devices are already supported by a driver in ALSA
firewire stack, but they have an interoperability issue related to
the speed of asynchronous transactions and isochronous transmissions.
When operating at the speed described in configuration ROM, they are
too lazy to respond, and eventually frozen.

The most likely cause of this issue is a mismatch in the gap count
between the initiators and receivers. Theoretically, this can be
resolved by transmitting phy configuration packets to optimize gap count.
Nevertheless, this approach has proven ineffective, suggesting that the
device firmware may contain a bug causing the issue.

>From my experience, these devices operate more reliably at lower
transaction and transmission speeds, which provides a practical
mitigation.

This patch series addresses the interoperability issue. The core function
of Linux FireWire subsystem is changed to read the entire configuration
ROM at the lowest speed (S100), and to identify these devices based on its
contents. Once identified, their maximum speed is limited to S200. The
ALSA driver then performs asynchronous requests and isochronous
transmission at that speed to prevent device freezes.

Takashi Sakamoto (4):
  firewire: core: code refactoring to compute transaction speed
  firewire: core: determine transaction speed after detecting quirks
  firewire: core: handle device quirk of TASCAM FW-1884/FW-1804/FW-1082
  ALSA: firewire-tascam: reserve resources for transferred isochronous
    packets at S400

 drivers/firewire/core-device.c        | 86 +++++++++++++++------------
 include/linux/firewire.h              |  3 +
 sound/firewire/tascam/tascam-stream.c | 21 +++----
 3 files changed, 63 insertions(+), 47 deletions(-)


base-commit: 15f9610fc96ac6fd2844e63f7bf5a0b08e1c31c8
-- 
2.51.0

[PATCH 2/4] firewire: core: determine transaction speed after detecting quirks

[Takashi S. <o-t...@sa...>]

Current implementation determines the maximum transaction speed supported
by the target device after reading bus information block of configuration
ROM. The read operations for root directory block are then performed at
the determined speed. However, some devices have quirks that cause issues
when transactions are performed at the determined speed.

In the first place, all devices are required to support the lowest speed
(S100) and must respond successfully to any read request within the
configuration ROM space. Therefore it is safe to postpone speed
determination until the entire configuration ROM has been read.

This commit moves the speed determination after reading root directory.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-device.c | 53 ++++++++++++++++------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index c698d4ced7d7..6a5740ed4934 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -680,32 +680,6 @@ static int read_config_rom(struct fw_device *device, int generation)
 	// Just prevent from torn writing/reading.
 	WRITE_ONCE(device->quirks, quirks);
 
-	speed = device->node->max_speed;
-
-	/*
-	 * Determine the speed of
-	 *   - devices with link speed less than PHY speed,
-	 *   - devices with 1394b PHY (unless only connected to 1394a PHYs),
-	 *   - all devices if there are 1394b repeaters.
-	 * Note, we cannot use the bus info block's link_spd as starting point
-	 * because some buggy firmwares set it lower than necessary and because
-	 * 1394-1995 nodes do not have the field.
-	 */
-	if ((rom[2] & 0x7) < speed || speed == SCODE_BETA || card->beta_repeaters_present) {
-		u32 dummy;
-
-		/* for S1600 and S3200 */
-		if (speed == SCODE_BETA)
-			speed = card->link_speed;
-
-		while (speed > SCODE_100) {
-			if (read_rom(device, generation, speed, 0, &dummy) ==
-			    RCODE_COMPLETE)
-				break;
-			--speed;
-		}
-	}
-
 	/*
 	 * Now parse the config rom.  The config rom is a recursive
 	 * directory structure so we parse it using a stack of
@@ -782,13 +756,36 @@ static int read_config_rom(struct fw_device *device, int generation)
 			length = i;
 	}
 
-	device->max_speed = speed;
-
 	quirks |= detect_quirks_by_root_directory(rom + ROOT_DIR_OFFSET, length - ROOT_DIR_OFFSET);
 
 	// Just prevent from torn writing/reading.
 	WRITE_ONCE(device->quirks, quirks);
 
+	speed = device->node->max_speed;
+
+	// Determine the speed of
+	//   - devices with link speed less than PHY speed,
+	//   - devices with 1394b PHY (unless only connected to 1394a PHYs),
+	//   - all devices if there are 1394b repeaters.
+	// Note, we cannot use the bus info block's link_spd as starting point because some buggy
+	// firmwares set it lower than necessary and because 1394-1995 nodes do not have the field.
+	if ((rom[2] & 0x7) < speed || speed == SCODE_BETA || card->beta_repeaters_present) {
+		u32 dummy;
+
+		// for S1600 and S3200.
+		if (speed == SCODE_BETA)
+			speed = card->link_speed;
+
+		while (speed > SCODE_100) {
+			if (read_rom(device, generation, speed, 0, &dummy) ==
+			    RCODE_COMPLETE)
+				break;
+			--speed;
+		}
+	}
+
+	device->max_speed = speed;
+
 	old_rom = device->config_rom;
 	new_rom = kmemdup(rom, length * 4, GFP_KERNEL);
 	if (new_rom == NULL) {
-- 
2.51.0

Re: [PATCH 0/2] firewire: core: add device quirk detection

[Takashi S. <o-t...@sa...>]

On Mon, Oct 13, 2025 at 11:03:09PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> In the history of this subsystem, we have experienced some device-specific
> quirks. For example:
> 
> * afa1282a35d3 ("firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder").
> * a509e43ff338 ("firewire: core: fix unstable I/O with Canon camcorder").
> * 3a93d082bacf ("ALSA: firewire-motu: add support for MOTU Audio Express")
> 
> However, there is no common mechanism to handle such quirks. This patchset
> adds a consistent approach for detecting and managing device-specific
> quirks within the subsystem.
> 
> Takashi Sakamoto (2):
>   firewire: core: detect device quirk when reading configuration ROM
>   firewire: core: handle device quirk of MOTu Audio Express
> 
>  drivers/firewire/core-card.c   | 21 +++------
>  drivers/firewire/core-device.c | 78 +++++++++++++++++++++++++++++++++-
>  drivers/firewire/ohci.c        | 29 +++++++++++++
>  include/linux/firewire.h       | 14 ++++++
>  4 files changed, 126 insertions(+), 16 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto

[PATCH 2/2] firewire: core: handle device quirk of MOTU Audio Express

[Takashi S. <o-t...@sa...>]

A commit 3a93d082bacf ("ALSA: firewire-motu: add support for MOTU Audio
Express") describes a quirk of MOTU Audio Express. The device returns
acknowledge packet with 0x10 as the pending state of any types of
asynchronous request transaction. It is completely out of specification.

This commit implements handling for that device-specific quirk. The quirk
is detected after reading the root directory of configuration ROM. When
processing the acknowledge code in 1394 OHCI AT context event handler,
firewire-ohci module seeks the device instance of destination node by
traversing device hierarchy. If the device has the quirk, the acknowledge
code is replaced with the standard code.

The 1394 OHCI AT context events occur for outgoing asynchronous request
packets. The device traversal is safe since no new request initiators
exist after the fw_card_instance has been invalidated.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-device.c | 53 ++++++++++++++++++++++++++++++++++
 drivers/firewire/ohci.c        | 29 +++++++++++++++++++
 include/linux/firewire.h       |  3 ++
 3 files changed, 85 insertions(+)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 9bab2d594b89..33ce4cd357ed 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -557,6 +557,54 @@ static int detect_quirks_by_bus_information_block(const u32 *bus_information_blo
 	return quirks;
 }
 
+struct entry_match {
+	unsigned int index;
+	u32 value;
+};
+
+static const struct entry_match motu_audio_express_matches[] = {
+	{ 1, 0x030001f2 },
+	{ 3, 0xd1000002 },
+	{ 4, 0x8d000005 },
+	{ 6, 0x120001f2 },
+	{ 7, 0x13000033 },
+	{ 8, 0x17104800 },
+};
+
+static int detect_quirks_by_root_directory(const u32 *root_directory, unsigned int length)
+{
+	static const struct {
+		enum fw_device_quirk quirk;
+		const struct entry_match *matches;
+		unsigned int match_count;
+	} *entry, entries[] = {
+		{
+			.quirk = FW_DEVICE_QUIRK_ACK_PACKET_WITH_INVALID_PENDING_CODE,
+			.matches = motu_audio_express_matches,
+			.match_count = ARRAY_SIZE(motu_audio_express_matches),
+		},
+	};
+	int quirks = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(entries); ++i) {
+		int j;
+
+		entry = entries + i;
+		for (j = 0; j < entry->match_count; ++j) {
+			unsigned int index = entry->matches[j].index;
+			unsigned int value = entry->matches[j].value;
+
+			if ((length < index) || (root_directory[index] != value))
+				break;
+		}
+		if (j == entry->match_count)
+			quirks |= entry->quirk;
+	}
+
+	return quirks;
+}
+
 static int read_rom(struct fw_device *device,
 		    int generation, int index, u32 *data)
 {
@@ -737,6 +785,11 @@ static int read_config_rom(struct fw_device *device, int generation)
 			length = i;
 	}
 
+	quirks |= detect_quirks_by_root_directory(rom + ROOT_DIR_OFFSET, length - ROOT_DIR_OFFSET);
+
+	// Just prevent from torn writing/reading.
+	WRITE_ONCE(device->quirks, quirks);
+
 	old_rom = device->config_rom;
 	new_rom = kmemdup(rom, length * 4, GFP_KERNEL);
 	if (new_rom == NULL) {
diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 030aed5453a1..757dd9c64b1c 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1319,6 +1319,14 @@ static void at_context_flush(struct at_context *ctx)
 	enable_work(&ctx->work);
 }
 
+static int find_fw_device(struct device *dev, const void *data)
+{
+	struct fw_device *device = fw_device(dev);
+	const u32 *params = data;
+
+	return (device->generation == params[0]) && (device->node_id == params[1]);
+}
+
 static int handle_at_packet(struct context *context,
 			    struct descriptor *d,
 			    struct descriptor *last)
@@ -1390,6 +1398,27 @@ static int handle_at_packet(struct context *context,
 		fallthrough;
 
 	default:
+		if (unlikely(evt == 0x10)) {
+			u32 params[2] = {
+				packet->generation,
+				async_header_get_destination(packet->header),
+			};
+			struct device *dev;
+
+			fw_card_get(&ohci->card);
+			dev = device_find_child(ohci->card.device, (const void *)params, find_fw_device);
+			fw_card_put(&ohci->card);
+			if (dev) {
+				struct fw_device *device = fw_device(dev);
+				int quirks = READ_ONCE(device->quirks);
+
+				put_device(dev);
+				if (quirks & FW_DEVICE_QUIRK_ACK_PACKET_WITH_INVALID_PENDING_CODE) {
+					packet->ack = ACK_PENDING;
+					break;
+				}
+			}
+		}
 		packet->ack = RCODE_SEND_ERROR;
 		break;
 	}
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 161829cfcc00..f1d8734c0ec6 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -176,6 +176,9 @@ enum fw_device_quirk {
 
 	// See a509e43ff338 ("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.
+	FW_DEVICE_QUIRK_ACK_PACKET_WITH_INVALID_PENDING_CODE = BIT(2),
 };
 
 enum fw_device_state {
-- 
2.51.0

[PATCH 1/2] firewire: core: detect device quirk when reading configuration ROM

[Takashi S. <o-t...@sa...>]

Every time the bus manager runs, the cached configuration ROM content of
the IRM device is investigated to detect device-specific quirks. This
detection can be performed in advance when reading the configuration ROM.

This commit adds device quirk flags to the fw_device structure, and
initializes them after reading the bus information block of the
configuration ROM. The quirk flags are immutable once the configuration
ROM has been read. Although they are likely accessed concurrently only by
the bus manager, this commit ensures safe access by preventing torn writes
and reads using the WRITE_ONCE()/READ_ONCE() macros.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-card.c   | 21 +++++++--------------
 drivers/firewire/core-device.c | 25 +++++++++++++++++++++++--
 include/linux/firewire.h       | 11 +++++++++++
 3 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index e5e0174a0335..6979d6a88ae2 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -86,8 +86,6 @@ static size_t config_rom_length = 1 + 4 + 1 + 1;
  */
 #define DEFAULT_SPLIT_TIMEOUT	(2 * 8000)
 
-#define CANON_OUI		0x000085
-
 static void generate_config_rom(struct fw_card *card, __be32 *config_rom)
 {
 	struct fw_descriptor *desc;
@@ -308,11 +306,9 @@ __must_hold(&card->lock)
 		cpu_to_be32(local_id),
 	};
 	bool grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125));
-	bool irm_is_1394_1995_only = false;
-	bool keep_this_irm = false;
 	struct fw_node *irm_node;
 	struct fw_device *irm_device;
-	int irm_node_id;
+	int irm_node_id, irm_device_quirks = 0;
 	int rcode;
 
 	lockdep_assert_held(&card->lock);
@@ -328,15 +324,12 @@ __must_hold(&card->lock)
 		return BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF;
 	}
 
+	// NOTE: It is likely that the quirk detection for IRM device has not done yet.
 	irm_device = fw_node_get_device(irm_node);
-	if (irm_device && irm_device->config_rom) {
-		irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0;
-
-		// Canon MV5i works unreliably if it is not root node.
-		keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI;
-	}
-
-	if (irm_is_1394_1995_only && !keep_this_irm) {
+	if (irm_device)
+		irm_device_quirks = READ_ONCE(irm_device->quirks);
+	if ((irm_device_quirks & FW_DEVICE_QUIRK_IRM_IS_1394_1995_ONLY) &&
+	    !(irm_device_quirks & FW_DEVICE_QUIRK_IRM_IGNORES_BUS_MANAGER)) {
 		fw_notice(card, "IRM is not 1394a compliant, making local node (%02x) root\n",
 			  local_id);
 		return BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY;
@@ -373,7 +366,7 @@ __must_hold(&card->lock)
 			return BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM;
 	}
 	default:
-		if (!keep_this_irm) {
+		if (!(irm_device_quirks & FW_DEVICE_QUIRK_IRM_IGNORES_BUS_MANAGER)) {
 			fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
 				  fw_rcode_string(rcode), local_id);
 			return BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY;
diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 457a0da024a7..9bab2d594b89 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -542,6 +542,21 @@ static struct device_attribute fw_device_attributes[] = {
 	__ATTR_NULL,
 };
 
+#define CANON_OUI		0x000085
+
+static int detect_quirks_by_bus_information_block(const u32 *bus_information_block)
+{
+	int quirks = 0;
+
+	if ((bus_information_block[2] & 0x000000f0) == 0)
+		quirks |= FW_DEVICE_QUIRK_IRM_IS_1394_1995_ONLY;
+
+	if ((bus_information_block[3] >> 8) == CANON_OUI)
+		quirks |= FW_DEVICE_QUIRK_IRM_IGNORES_BUS_MANAGER;
+
+	return quirks;
+}
+
 static int read_rom(struct fw_device *device,
 		    int generation, int index, u32 *data)
 {
@@ -582,6 +597,7 @@ static int read_config_rom(struct fw_device *device, int generation)
 	u32 *rom, *stack;
 	u32 sp, key;
 	int i, end, length, ret;
+	int quirks;
 
 	rom = kmalloc(sizeof(*rom) * MAX_CONFIG_ROM_SIZE +
 		      sizeof(*stack) * MAX_CONFIG_ROM_SIZE, GFP_KERNEL);
@@ -612,6 +628,11 @@ static int read_config_rom(struct fw_device *device, int generation)
 		}
 	}
 
+	quirks = detect_quirks_by_bus_information_block(rom);
+
+	// Just prevent from torn writing/reading.
+	WRITE_ONCE(device->quirks, quirks);
+
 	device->max_speed = device->node->max_speed;
 
 	/*
@@ -1122,10 +1143,10 @@ static void fw_device_init(struct work_struct *work)
 		device->workfn = fw_device_shutdown;
 		fw_schedule_device_work(device, SHUTDOWN_DELAY);
 	} else {
-		fw_notice(card, "created device %s: GUID %08x%08x, S%d00\n",
+		fw_notice(card, "created device %s: GUID %08x%08x, S%d00, quirks %08x\n",
 			  dev_name(&device->device),
 			  device->config_rom[3], device->config_rom[4],
-			  1 << device->max_speed);
+			  1 << device->max_speed, device->quirks);
 		device->config_rom_retries = 0;
 
 		set_broadcast_channel(device, device->generation);
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 6d208769d456..161829cfcc00 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -170,6 +170,14 @@ struct fw_attribute_group {
 	struct attribute *attrs[13];
 };
 
+enum fw_device_quirk {
+	// See afa1282a35d3 ("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").
+	FW_DEVICE_QUIRK_IRM_IGNORES_BUS_MANAGER = BIT(1),
+};
+
 enum fw_device_state {
 	FW_DEVICE_INITIALIZING,
 	FW_DEVICE_RUNNING,
@@ -203,6 +211,9 @@ struct fw_device {
 	struct fw_card *card;
 	struct device device;
 
+	// A set of enum fw_device_quirk.
+	int quirks;
+
 	struct mutex client_list_mutex;
 	struct list_head client_list;
 
-- 
2.51.0

[PATCH 0/2] firewire: core: add device quirk detection

[Takashi S. <o-t...@sa...>]

Hi,

In the history of this subsystem, we have experienced some device-specific
quirks. For example:

* afa1282a35d3 ("firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder").
* a509e43ff338 ("firewire: core: fix unstable I/O with Canon camcorder").
* 3a93d082bacf ("ALSA: firewire-motu: add support for MOTU Audio Express")

However, there is no common mechanism to handle such quirks. This patchset
adds a consistent approach for detecting and managing device-specific
quirks within the subsystem.

Takashi Sakamoto (2):
  firewire: core: detect device quirk when reading configuration ROM
  firewire: core: handle device quirk of MOTu Audio Express

 drivers/firewire/core-card.c   | 21 +++------
 drivers/firewire/core-device.c | 78 +++++++++++++++++++++++++++++++++-
 drivers/firewire/ohci.c        | 29 +++++++++++++
 include/linux/firewire.h       | 14 ++++++
 4 files changed, 126 insertions(+), 16 deletions(-)


base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
-- 
2.51.0

[GIT PULL] firewire updates for v6.18 kernel

[Takashi S. <o-t...@sa...>]

Hi Linus,

Please accept the changes for FireWire subsystem to your tree.

This update may appear to include many changes, but most of them are
code refactoring. Except for the removal of firewire-ohci module
parameter, there are only a few notable changes.


The following changes since commit c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9:

  Linux 6.17-rc2 (2025-08-17 15:22:10 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git tags/firewire-updates-6.18

for you to fetch changes up to 40d4c761200b796a44bf2c7675ae09c87b17d4af:

  firewire: core: fix undefined reference error in ARM EABI (2025-09-28 10:20:30 +0900)

----------------------------------------------------------------
firewire updates for v6.18

This update includes the following changes:

- Removal of the deprecated debug parameter from firewire-ohci module
- Replacement of the module-local workqueue in 1394 OHCI PCI driver with
  a companion IRQ thread
- Refactoring of bus management code
- Additional minor code cleanup

The existing tracepoints serve as an alternative to the removed debug
parameter. The use of IRQ thread is experimental, as it handles 1394 OHCI
SelfIDComplete event only. It may be replaced in the future releases with
another approach; e.g. by providing workqueue from core functionality

----------------------------------------------------------------
Takashi Sakamoto (42):
      firewire: ohci: remove obsolete debug logging for IRQ events
      firewire: ohci: remove obsolete debug logging for selfID sequence
      firewire: ohci: remove obsolete debug logging for AT/AR results
      firewire: ohci: remove obsolete module-level debug parameter
      firewire: ohci: move self_id_complete tracepoint after validating register
      firewire: ohci: use threaded IRQ handler to handle SelfIDComplete event
      firewire: ohci: remove module-local workqueue
      firewire: ohci: use kcalloc() variant for array allocation
      firewire: core: utilize cleanup function to release workqueue in error path
      firewire: ohci: use return value from fw_node_get()
      firewire: core: add helper functions to access to fw_device data in fw_node structure
      firewire: core: use cleanup function in bm_work
      firewire: ohci: localize transaction data and rcode per condition branch
      firewire: core: code refactoring to evaluate transaction result to CSR_BUS_MANAGER_ID
      firewire: core: refer fw_card member to initiate bus reset under acquiring lock
      firewire: core: code refactoring to detect both IEEE 1394:1995 IRM and Canon MV5i
      firewire: core: code refactoring to investigate root node for bus manager
      firewire: core: code refactoring whether root node is cycle master capable
      firewire: core: remove useless lockdep_assert_held()
      firewire: core: use macro expression for gap count mismatch
      firewire: core: use macro expression for not-registered state of BUS_MANAGER_ID
      firewire: core: use helper macros instead of direct access to HZ
      firewire: core: use helper macro to compare against current jiffies
      firewire: core: use scoped_guard() to manage critical section to update topology
      firewire: core: maintain phy packet receivers locally in cdev layer
      firewire: core: use spin lock specific to topology map
      firewire: core: use spin lock specific to transaction
      firewire: core: use spin lock specific to timer for split transaction
      firewire: core: annotate fw_destroy_nodes with must-hold-lock
      firewire: core: schedule bm_work item outside of spin lock
      firewire: core: disable bus management work temporarily during updating topology
      firewire: core: shrink critical section of fw_card spinlock in bm_work
      firewire: core: remove useless generation check
      firewire: core: use switch statement to evaluate transaction result to CSR_BUS_MANAGER_ID
      firewire: core: code refactoring for the case of generation mismatch
      firewire: core: code refactoring to split contention procedure for bus manager
      firewire: core; eliminate pick_me goto label
      firewire: core: minor code refactoring to delete useless local variable
      firewire: core: suppress overflow warning when computing jiffies from isochronous cycle
      Revert "firewire: core: shrink critical section of fw_card spinlock in bm_work"
      Revert "firewire: core: disable bus management work temporarily during updating topology"
      firewire: core: fix undefined reference error in ARM EABI

Thorsten Blum (1):
      firewire: core: use struct_size and flex_array_size in ioctl_add_descriptor

 drivers/firewire/core-card.c        | 490 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------------------------------------
 drivers/firewire/core-cdev.c        |  36 ++++++++++-----
 drivers/firewire/core-device.c      |  27 ++++++-----
 drivers/firewire/core-topology.c    |  91 +++++++++++++++++++------------------
 drivers/firewire/core-transaction.c | 130 ++++++++++++++++++++++++++++++++++-------------------
 drivers/firewire/core.h             |  22 ++++++++-
 drivers/firewire/ohci.c             | 316 +++++++++++++++++++-------------------------------------------------------------------------------------------------------------
 include/linux/firewire.h            |  33 +++++++++-----
 8 files changed, 518 insertions(+), 627 deletions(-)


Regards

Takashi Sakamoto

[PATCH] firewire: core: fix undefined reference error in ARM EABI

[Takashi S. <o-t...@sa...>]

For ARM EABI, GCC generates a reference to __aeabi_uldivmod when compiling
a division of 64-bit integer with 32-bit integer. This function is not
available in Linux kernel. In such cases, helper macros are defined in
include/linux/math64.h.

This commit replaces the division with div_u64().

Fixes: 8ec6a8ec23b9 ("firewire: core: suppress overflow warning when computing jiffies from isochronous cycle")
Reported-by: kernel test robot <lk...@in...>
Closes: https://lore.kernel.org/oe-kbuild-all/202...@in.../
Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h
index 2dd715a580ac..e67395ce26b5 100644
--- a/drivers/firewire/core.h
+++ b/drivers/firewire/core.h
@@ -30,7 +30,7 @@ struct fw_packet;
 // This is the arbitrary value we use to indicate a mismatched gap count.
 #define GAP_COUNT_MISMATCHED	0
 
-#define isoc_cycles_to_jiffies(cycles)	usecs_to_jiffies((u32)((u64)(cycles) * USEC_PER_SEC / 8000))
+#define isoc_cycles_to_jiffies(cycles) usecs_to_jiffies((u32)div_u64((u64)cycles * USEC_PER_SEC, 8000))
 
 extern __printf(2, 3)
 void fw_err(const struct fw_card *card, const char *fmt, ...);
-- 
2.48.1

Re: [PATCH v2 0/2] firewire: core: revert "serialize topology building and bus manager work"

[Takashi S. <o-t...@sa...>]

On Wed, Sep 24, 2025 at 10:18:21PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> The patchset that serialized bm_work() and fw_core_handle_bus_reset()
> was merged without sufficient consideration of the race condition during
> fw_card removal.
> 
> This patchset reverts some commits and restores the acquisition of the
> fw_card spin lock.
> 
> [1] https://lore.kernel.org/lkml/202...@sa.../
> 
> Changes from v1:
> * Fulfill cover-letter title
> 
> Takashi Sakamoto (2):
>   Revert "firewire: core: shrink critical section of fw_card spinlock in
>     bm_work"
>   Revert "firewire: core: disable bus management work temporarily during
>     updating topology"
> 
>  drivers/firewire/core-card.c     | 38 +++++++++++++++++++++++++-------
>  drivers/firewire/core-topology.c |  8 -------
>  2 files changed, 30 insertions(+), 16 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto

Re: [PATCH] firewire: core: suppress overflow warning when computing jiffies from isochronous cycle

[Takashi S. <o-t...@sa...>]

On Wed, Sep 24, 2025 at 10:11:40PM +0900, Takashi Sakamoto wrote:
> The multiplication by USEC_PER_SEC (=1000000L) may trigger an overflow
> warning with 32 bit storage. In the case of the subsystem the input value
> ranges between 800 and 16000, thus the result always fits within 32 bit
> storage.
> 
> This commit suppresses the warning by using widening conversion to 64 bit
> storage before multiplication, then using narrowing conversion to 32 bit
> storage.
> 
> Reported-by: kernel test robot <lk...@in...>
> Closes: https://lore.kernel.org/oe-kbuild-all/202...@in.../
> Fixes: 379b870c28c6 ("firewire: core: use helper macros instead of direct access to HZ")
> Signed-off-by: Takashi Sakamoto <o-t...@sa...>
> ---
>  drivers/firewire/core.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to for-next branch.


Regards

Takashi Sakamoto

[PATCH v2 0/2] firewire: core: revert "serialize topology building and bus manager work"

[Takashi S. <o-t...@sa...>]

Hi,

The patchset that serialized bm_work() and fw_core_handle_bus_reset()
was merged without sufficient consideration of the race condition during
fw_card removal.

This patchset reverts some commits and restores the acquisition of the
fw_card spin lock.

[1] https://lore.kernel.org/lkml/202...@sa.../

Changes from v1:
* Fulfill cover-letter title

Takashi Sakamoto (2):
  Revert "firewire: core: shrink critical section of fw_card spinlock in
    bm_work"
  Revert "firewire: core: disable bus management work temporarily during
    updating topology"

 drivers/firewire/core-card.c     | 38 +++++++++++++++++++++++++-------
 drivers/firewire/core-topology.c |  8 -------
 2 files changed, 30 insertions(+), 16 deletions(-)


base-commit: 19e73f65940d3d3357c637f3d7e19a59305a748f
-- 
2.48.1

[PATCH v2 2/2] Revert "firewire: core: disable bus management work temporarily during updating topology"

[Takashi S. <o-t...@sa...>]

This reverts commit abe7159125702c734e851bc0c52b51cd446298a5.

The bus manager work item acquires the spin lock of fw_card again, thus
no need to serialize it against fw_core_handle_bus_reset().

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-topology.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index 90b988035a2a..2f73bcd5696f 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -460,14 +460,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
 {
 	struct fw_node *local_node;
 
-	might_sleep();
-
 	trace_bus_reset_handle(card->index, generation, node_id, bm_abdicate, self_ids, self_id_count);
 
-	// Disable bus management work during updating the cache of bus topology, since the work
-	// accesses to some members of fw_card.
-	disable_delayed_work_sync(&card->bm_work);
-
 	scoped_guard(spinlock, &card->lock) {
 		// If the selfID buffer is not the immediate successor of the
 		// previously processed one, we cannot reliably compare the
@@ -501,8 +495,6 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
 		}
 	}
 
-	enable_delayed_work(&card->bm_work);
-
 	fw_schedule_bm_work(card, 0);
 
 	// Just used by transaction layer.
-- 
2.48.1

[PATCH v2 1/2] Revert "firewire: core: shrink critical section of fw_card spinlock in bm_work"

[Takashi S. <o-t...@sa...>]

This reverts commit 582310376d6e9a8d261b682178713cdc4b251af6.

The bus manager work has the race condition against fw_destroy_nodes()
called by fw_core_remove_card(). The acquition of spin lock of fw_card
is left as is again.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-card.c | 38 ++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 4a5459696093..e5e0174a0335 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -299,6 +299,7 @@ enum bm_contention_outcome {
 };
 
 static enum bm_contention_outcome contend_for_bm(struct fw_card *card)
+__must_hold(&card->lock)
 {
 	int generation = card->generation;
 	int local_id = card->local_node->node_id;
@@ -311,8 +312,11 @@ static enum bm_contention_outcome contend_for_bm(struct fw_card *card)
 	bool keep_this_irm = false;
 	struct fw_node *irm_node;
 	struct fw_device *irm_device;
+	int irm_node_id;
 	int rcode;
 
+	lockdep_assert_held(&card->lock);
+
 	if (!grace) {
 		if (!is_next_generation(generation, card->bm_generation) || card->bm_abdicate)
 			return BM_CONTENTION_OUTCOME_WITHIN_WINDOW;
@@ -338,10 +342,16 @@ static enum bm_contention_outcome contend_for_bm(struct fw_card *card)
 		return BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY;
 	}
 
-	rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, irm_node->node_id, generation,
+	irm_node_id = irm_node->node_id;
+
+	spin_unlock_irq(&card->lock);
+
+	rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, irm_node_id, generation,
 				   SCODE_100, CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, data,
 				   sizeof(data));
 
+	spin_lock_irq(&card->lock);
+
 	switch (rcode) {
 	case RCODE_GENERATION:
 		return BM_CONTENTION_OUTCOME_AT_NEW_GENERATION;
@@ -352,12 +362,10 @@ static enum bm_contention_outcome contend_for_bm(struct fw_card *card)
 		int bm_id = be32_to_cpu(data[0]);
 
 		// Used by cdev layer for "struct fw_cdev_event_bus_reset".
-		scoped_guard(spinlock, &card->lock) {
-			if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED)
-				card->bm_node_id = 0xffc0 & bm_id;
-			else
-				card->bm_node_id = local_id;
-		}
+		if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED)
+			card->bm_node_id = 0xffc0 & bm_id;
+		else
+			card->bm_node_id = local_id;
 
 		if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED)
 			return BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM;
@@ -389,8 +397,12 @@ static void bm_work(struct work_struct *work)
 	int expected_gap_count, generation;
 	bool stand_for_root = false;
 
-	if (card->local_node == NULL)
+	spin_lock_irq(&card->lock);
+
+	if (card->local_node == NULL) {
+		spin_unlock_irq(&card->lock);
 		return;
+	}
 
 	generation = card->generation;
 
@@ -405,6 +417,7 @@ static void bm_work(struct work_struct *work)
 
 		switch (result) {
 		case BM_CONTENTION_OUTCOME_WITHIN_WINDOW:
+			spin_unlock_irq(&card->lock);
 			fw_schedule_bm_work(card, msecs_to_jiffies(125));
 			return;
 		case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF:
@@ -415,10 +428,12 @@ static void bm_work(struct work_struct *work)
 			break;
 		case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION:
 			// BM work has been rescheduled.
+			spin_unlock_irq(&card->lock);
 			return;
 		case BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION:
 			// Let's try again later and hope that the local problem has gone away by
 			// then.
+			spin_unlock_irq(&card->lock);
 			fw_schedule_bm_work(card, msecs_to_jiffies(125));
 			return;
 		case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM:
@@ -428,7 +443,9 @@ static void bm_work(struct work_struct *work)
 		case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM:
 			if (local_id == irm_id) {
 				// Only acts as IRM.
+				spin_unlock_irq(&card->lock);
 				allocate_broadcast_channel(card, generation);
+				spin_lock_irq(&card->lock);
 			}
 			fallthrough;
 		case BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM:
@@ -469,6 +486,7 @@ static void bm_work(struct work_struct *work)
 				if (!root_device_is_running) {
 					// If we haven't probed this device yet, bail out now
 					// and let's try again once that's done.
+					spin_unlock_irq(&card->lock);
 					return;
 				} else if (!root_device->cmc) {
 					// Current root has an active link layer and we
@@ -504,6 +522,8 @@ static void bm_work(struct work_struct *work)
 	if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id)) {
 		int card_gap_count = card->gap_count;
 
+		spin_unlock_irq(&card->lock);
+
 		fw_notice(card, "phy config: new root=%x, gap_count=%d\n",
 			  new_root_id, expected_gap_count);
 		fw_send_phy_config(card, new_root_id, generation, expected_gap_count);
@@ -524,6 +544,8 @@ static void bm_work(struct work_struct *work)
 	} else {
 		struct fw_device *root_device = fw_node_get_device(root_node);
 
+		spin_unlock_irq(&card->lock);
+
 		if (root_device && root_device->cmc) {
 			// Make sure that the cycle master sends cycle start packets.
 			__be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR);
-- 
2.48.1

[PATCH] firewire: core: suppress overflow warning when computing jiffies from isochronous cycle

[Takashi S. <o-t...@sa...>]

The multiplication by USEC_PER_SEC (=1000000L) may trigger an overflow
warning with 32 bit storage. In the case of the subsystem the input value
ranges between 800 and 16000, thus the result always fits within 32 bit
storage.

This commit suppresses the warning by using widening conversion to 64 bit
storage before multiplication, then using narrowing conversion to 32 bit
storage.

Reported-by: kernel test robot <lk...@in...>
Closes: https://lore.kernel.org/oe-kbuild-all/202...@in.../
Fixes: 379b870c28c6 ("firewire: core: use helper macros instead of direct access to HZ")
Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h
index 7f2ca93406ce..2dd715a580ac 100644
--- a/drivers/firewire/core.h
+++ b/drivers/firewire/core.h
@@ -30,7 +30,7 @@ struct fw_packet;
 // This is the arbitrary value we use to indicate a mismatched gap count.
 #define GAP_COUNT_MISMATCHED	0
 
-#define isoc_cycles_to_jiffies(cycles)	usecs_to_jiffies(cycles * USEC_PER_SEC / 8000)
+#define isoc_cycles_to_jiffies(cycles)	usecs_to_jiffies((u32)((u64)(cycles) * USEC_PER_SEC / 8000))
 
 extern __printf(2, 3)
 void fw_err(const struct fw_card *card, const char *fmt, ...);

base-commit: 19e73f65940d3d3357c637f3d7e19a59305a748f
-- 
2.48.1

[PATCH 1/2] Revert "firewire: core: shrink critical section of fw_card spinlock in bm_work"

[Takashi S. <o-t...@sa...>]

This reverts commit 582310376d6e9a8d261b682178713cdc4b251af6.

The bus manager work has the race condition against fw_destroy_nodes()
called by fw_core_remove_card(). The acquisition of spin lock of fw_card
is left as is again.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-card.c | 38 ++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 4a5459696093..e5e0174a0335 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -299,6 +299,7 @@ enum bm_contention_outcome {
 };
 
 static enum bm_contention_outcome contend_for_bm(struct fw_card *card)
+__must_hold(&card->lock)
 {
 	int generation = card->generation;
 	int local_id = card->local_node->node_id;
@@ -311,8 +312,11 @@ static enum bm_contention_outcome contend_for_bm(struct fw_card *card)
 	bool keep_this_irm = false;
 	struct fw_node *irm_node;
 	struct fw_device *irm_device;
+	int irm_node_id;
 	int rcode;
 
+	lockdep_assert_held(&card->lock);
+
 	if (!grace) {
 		if (!is_next_generation(generation, card->bm_generation) || card->bm_abdicate)
 			return BM_CONTENTION_OUTCOME_WITHIN_WINDOW;
@@ -338,10 +342,16 @@ static enum bm_contention_outcome contend_for_bm(struct fw_card *card)
 		return BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY;
 	}
 
-	rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, irm_node->node_id, generation,
+	irm_node_id = irm_node->node_id;
+
+	spin_unlock_irq(&card->lock);
+
+	rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, irm_node_id, generation,
 				   SCODE_100, CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, data,
 				   sizeof(data));
 
+	spin_lock_irq(&card->lock);
+
 	switch (rcode) {
 	case RCODE_GENERATION:
 		return BM_CONTENTION_OUTCOME_AT_NEW_GENERATION;
@@ -352,12 +362,10 @@ static enum bm_contention_outcome contend_for_bm(struct fw_card *card)
 		int bm_id = be32_to_cpu(data[0]);
 
 		// Used by cdev layer for "struct fw_cdev_event_bus_reset".
-		scoped_guard(spinlock, &card->lock) {
-			if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED)
-				card->bm_node_id = 0xffc0 & bm_id;
-			else
-				card->bm_node_id = local_id;
-		}
+		if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED)
+			card->bm_node_id = 0xffc0 & bm_id;
+		else
+			card->bm_node_id = local_id;
 
 		if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED)
 			return BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM;
@@ -389,8 +397,12 @@ static void bm_work(struct work_struct *work)
 	int expected_gap_count, generation;
 	bool stand_for_root = false;
 
-	if (card->local_node == NULL)
+	spin_lock_irq(&card->lock);
+
+	if (card->local_node == NULL) {
+		spin_unlock_irq(&card->lock);
 		return;
+	}
 
 	generation = card->generation;
 
@@ -405,6 +417,7 @@ static void bm_work(struct work_struct *work)
 
 		switch (result) {
 		case BM_CONTENTION_OUTCOME_WITHIN_WINDOW:
+			spin_unlock_irq(&card->lock);
 			fw_schedule_bm_work(card, msecs_to_jiffies(125));
 			return;
 		case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF:
@@ -415,10 +428,12 @@ static void bm_work(struct work_struct *work)
 			break;
 		case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION:
 			// BM work has been rescheduled.
+			spin_unlock_irq(&card->lock);
 			return;
 		case BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION:
 			// Let's try again later and hope that the local problem has gone away by
 			// then.
+			spin_unlock_irq(&card->lock);
 			fw_schedule_bm_work(card, msecs_to_jiffies(125));
 			return;
 		case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM:
@@ -428,7 +443,9 @@ static void bm_work(struct work_struct *work)
 		case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM:
 			if (local_id == irm_id) {
 				// Only acts as IRM.
+				spin_unlock_irq(&card->lock);
 				allocate_broadcast_channel(card, generation);
+				spin_lock_irq(&card->lock);
 			}
 			fallthrough;
 		case BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM:
@@ -469,6 +486,7 @@ static void bm_work(struct work_struct *work)
 				if (!root_device_is_running) {
 					// If we haven't probed this device yet, bail out now
 					// and let's try again once that's done.
+					spin_unlock_irq(&card->lock);
 					return;
 				} else if (!root_device->cmc) {
 					// Current root has an active link layer and we
@@ -504,6 +522,8 @@ static void bm_work(struct work_struct *work)
 	if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id)) {
 		int card_gap_count = card->gap_count;
 
+		spin_unlock_irq(&card->lock);
+
 		fw_notice(card, "phy config: new root=%x, gap_count=%d\n",
 			  new_root_id, expected_gap_count);
 		fw_send_phy_config(card, new_root_id, generation, expected_gap_count);
@@ -524,6 +544,8 @@ static void bm_work(struct work_struct *work)
 	} else {
 		struct fw_device *root_device = fw_node_get_device(root_node);
 
+		spin_unlock_irq(&card->lock);
+
 		if (root_device && root_device->cmc) {
 			// Make sure that the cycle master sends cycle start packets.
 			__be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR);
-- 
2.48.1

[PATCH 0/2] firewire: core:

[Takashi S. <o-t...@sa...>]

Hi,

The patchset that serialized bm_work() and fw_core_handle_bus_reset()
was merged without sufficient consideration of the race condition during
fw_card removal.

This patchset reverts some commits and restores the acquisition of the
fw_card spin lock.

[1] https://lore.kernel.org/lkml/202...@sa.../

Takashi Sakamoto (2):
  Revert "firewire: core: shrink critical section of fw_card spinlock in
    bm_work"
  Revert "firewire: core: disable bus management work temporarily during
    updating topology"

 drivers/firewire/core-card.c     | 38 +++++++++++++++++++++++++-------
 drivers/firewire/core-topology.c |  8 -------
 2 files changed, 30 insertions(+), 16 deletions(-)


base-commit: 19e73f65940d3d3357c637f3d7e19a59305a748f
-- 
2.48.1

[PATCH 2/2] Revert "firewire: core: disable bus management work temporarily during updating topology"

[Takashi S. <o-t...@sa...>]

This reverts commit abe7159125702c734e851bc0c52b51cd446298a5.

The bus manager work item acquires the spin lock of fw_card again, thus
no need to serialize it against fw_core_handle_bus_reset().

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-topology.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index 90b988035a2a..2f73bcd5696f 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -460,14 +460,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
 {
 	struct fw_node *local_node;
 
-	might_sleep();
-
 	trace_bus_reset_handle(card->index, generation, node_id, bm_abdicate, self_ids, self_id_count);
 
-	// Disable bus management work during updating the cache of bus topology, since the work
-	// accesses to some members of fw_card.
-	disable_delayed_work_sync(&card->bm_work);
-
 	scoped_guard(spinlock, &card->lock) {
 		// If the selfID buffer is not the immediate successor of the
 		// previously processed one, we cannot reliably compare the
@@ -501,8 +495,6 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
 		}
 	}
 
-	enable_delayed_work(&card->bm_work);
-
 	fw_schedule_bm_work(card, 0);
 
 	// Just used by transaction layer.
-- 
2.48.1