[PATCH v5 00/11] ASoC: renesas: fsi: Fix system hang by adding SPU clock

[PATCH v5 00/11] ASoC: renesas: fsi: Fix system hang by adding SPU clock

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Hi all,

The FSI on r8a7740 requires the SPU clock to be enabled before accessing
its internal registers. Without it, register accesses may hang the system
even when the FSI functional clock is enabled.

Previously, the SPU clock remained enabled because it was left running by
the bootloader. After adding the SPU clock to the device tree, it is
automatically disabled once system initialization completes.

This series adds the missing clocks and aligns their names with those used
by the driver.

Following feedback from Morimoto-san, the driver is also refactored to
improve stability. Clock initialization is moved from the runtime path to
the probe function to simplify the flow and avoid redundant setup.
Additionally, the shutdown sequence is reordered to ensure the stream is
stopped before the hardware is shut down.

The driver currently uses clk_enable()/clk_disable() without matching
clk_prepare()/clk_unprepare() handling. This series adds the missing
prepare/unprepare operations and moves them into startup/shutdown paths,
since clk_prepare() may sleep and therefore must not be called from 
atomic contexts.

The series also fixes a race where in-flight IRQ handlers may continue
accessing registers after the SPU clock has been disabled during shutdown.

Changes in v5:
 - Drop spu_count and rely on the clk core for clock reference counting.
 - Reorder the patch series as suggested by Morimoto-san.
 - Update the probe error handling path to ensure proper cleanup on
   failure.

Changes in v4:
 - use fsi_stream_is_working() for Fixed a race where in-flight IRQ 
   handlers following Morimoto-san's suggestions
 - Handle the return value of fsi_clk_init() to properly support deferred 
   probe, as suggested by Mark.
 - Split the clock refactoring into a devm cleanup patch and a refactor 
   patch, as suggested by Morimoto-san.
 - Update dt-bindings based on feedback from Krzysztof, Rob, and Geert.

Changes in v3:
 - Reordered the patches following Morimoto-san's suggestions
 - Updated the DT bindings based on Geert's feedback and renamed the
   "own" clock to "fck"
 - Added fsi_clk_prepare()/fsi_clk_unprepare() and moved them into
   dai_startup()/dai_shutdown()
 - Fixed a race where in-flight IRQ handlers could continue accessing
   registers after the SPU clock had been disabled

Changes in v2:
 - DT Bindings:
   Define "own" clock and add "spu", "icka/b", "diva/b", "xcka/b" to the 
   clock tree.
   Use YAML anchors and "if" rules to enforce clock-names and r8a7740 
   requirements.
   Relocate allOf block and update example with full 8-clock configuration.

 - DTS:
   Rename "fsi" clock to "own" to match driver implementation.
   Add missing clock names: "icka", "ickb", "diva", "divb", "xcka", "xckb".
 
 - In the driver:
   Refactor clock initialization.
   Reorder shutdown: stop stream before hardware shutdown.
   Move SPU clock enable/disable handling to fsi_hw_startup/shutdown.
v4 links:
   https://lore.kernel.org/all/20260605121955.105661-1-phucduc.bui@gmail.com/
v3 links:
   https://lore.kernel.org/all/20260510084303.122426-1-phucduc.bui@gmail.com/
v2 links: 
   https://lore.kernel.org/all/20260413100700.30995-1-phucduc.bui@gmail.com/
v1 links : 
   https://lore.kernel.org/all/20260403112655.167593-1-phucduc.bui@gmail.com/

Testing:
  - Verified on r8a7740 (Armadillo-800EVA): FSI slave / Codec master mode.
    The system no longer hangs. aplay works correctly, while arecord has 
    some noise in the recorded file (this likely needs further tuning, but
    it is not part of this patch series). 
  - FSI master mode is currently compile-tested only. Full verification
    requires a dedicated HDMI driver (FSIB) or hardware modifications 
    (resoldering board resistors) (FSIA).
  - Youtube video link of the test process (from v3 verification):
    https://youtu.be/w3H4v5djr7M

Best regards,
Phuc

bui duc phuc (11):
  ASoC: dt-bindings: renesas,fsi: add support multiple clocks
  ARM: dts: renesas: r8a7740: Add clocks for FSI
  ASoC: renesas: fsi: Fix trigger stop ordering
  ASoC: renesas: fsi: Move fsi_stream_is_working()
  ASoC: renesas: fsi: Fix register access from in-flight IRQ after
    shutdown
  ASoC: renesas: fsi: Move fsi_clk_init()
  ASoC: renesas: fsi: Use devm_clk_get_optional() for optional clocks
  ASoC: renesas: fsi: refactor clock initialization
  ASoC: renesas: fsi: Add SPU clock support
  ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()
  ASoC: renesas: fsi: Add SPU clock control in hw_startup/shutdown

 .../bindings/sound/renesas,fsi.yaml           |  61 ++++-
 arch/arm/boot/dts/renesas/r8a7740.dtsi        |  12 +-
 sound/soc/renesas/fsi.c                       | 246 ++++++++++++------
 3 files changed, 228 insertions(+), 91 deletions(-)

-- 
2.43.0

[PATCH v5 01/11] ASoC: dt-bindings: renesas,fsi: add support multiple clocks

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

The FSI on r8a7740 requires the SPU bus/bridge clock to be enabled before
accessing its registers. Without this clock, any register access leads to
a system hang as the FSI block sits behind the SPU bus.
Update the binding to support multiple clocks to properly describe the
hardware clock tree, including:
  - SPU bus/bridge clock (spu) for register access.
  - CPG DIV6 clocks (icka/b) as functional clock.
  - FSI dividers (diva/b) for audio clock generation.
  - External clock inputs (xcka/b) provided by the board.
Both sh73a0 and r8a7740 define the SPU DIV6 clock control register at
0xe6150084. The binding therefore documents the clocks supported by the
FSI driver for these variants.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v4:
 - Update dt-bindings based on feedback from Krzysztof, Rob, and Geert.

 .../bindings/sound/renesas,fsi.yaml           | 61 +++++++++++++++++--
 1 file changed, 56 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
index df91991699a7..b966b55ff772 100644
--- a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
+++ b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
@@ -9,9 +9,6 @@ title: Renesas FIFO-buffered Serial Interface (FSI)
 maintainers:
   - Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
 
-allOf:
-  - $ref: dai-common.yaml#
-
 properties:
   $nodename:
     pattern: "^sound@.*"
@@ -38,7 +35,32 @@ properties:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Main FSI module clock
+      - description: |
+          SPU bus/bridge clock. On R8A7740, this clock must be enabled to allow
+          register access as the FSI block is connected behind the SPU bus.
+      - description: CPG DIV6 functional clocks for FSI port A
+      - description: CPG DIV6 functional clocks for FSI port B
+      - description: FSI dividers for port A used for audio clock generation
+      - description: FSI dividers for port B used for audio clock generation
+      - description: External clock inputs for FSI port A provided by the board
+      - description: External clock inputs for FSI port B provided by the board
+
+  clock-names:
+    minItems: 1
+    maxItems: 8
+    items:
+      enum:
+        - fck  # Main FSI module clock
+        - spu  # optional SPU bus/bridge clock
+        - icka # optional CPG DIV6 functional clocks for FSI port A
+        - ickb # optional CPG DIV6 functional clocks for FSI port B
+        - diva # optional FSI dividers for port A used for audio clock generation
+        - divb # optional FSI dividers for port B used for audio clock generation
+        - xcka # optional External clock inputs for FSI port A provided by the board
+        - xckb # optional External clock inputs for FSI port B provided by the board
 
   power-domains:
     maxItems: 1
@@ -69,6 +91,31 @@ required:
 
 unevaluatedProperties: false
 
+allOf:
+  - $ref: dai-common.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,fsi2-r8a7740
+    then:
+      required:
+        - clock-names
+
+      properties:
+        clock-names:
+          minItems: 2
+          uniqueItems: true
+          items:
+            - const: fck
+            - const: spu
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+
 examples:
   - |
     #include <dt-bindings/clock/r8a7740-clock.h>
@@ -77,7 +124,11 @@ examples:
             compatible = "renesas,fsi2-r8a7740", "renesas,sh_fsi2";
             reg = <0xfe1f0000 0x400>;
             interrupts = <GIC_SPI 9 0x4>;
-            clocks = <&mstp3_clks R8A7740_CLK_FSI>;
+            clocks = <&mstp3_clks R8A7740_CLK_FSI>, <&spu_clk>,
+                    <&fsia_clk>, <&fsiack_clk>, <&fsidiva_clk>,
+                    <&fsib_clk>, <&fsibck_clk>, <&fsidivb_clk>;
+            clock-names = "fck", "spu", "icka", "xcka", "diva",
+                         "ickb", "xckb", "divb";
             power-domains = <&pd_a4mp>;
 
             #sound-dai-cells = <1>;
-- 
2.43.0

[PATCH v5 02/11] ARM: dts: renesas: r8a7740: Add clocks for FSI

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Add the SPU bus clock, icka/b functional clocks, and xcka/b external
clock inputs to the FSI device node.
This prepares for subsequent driver changes that explicitly manage the
SPU clock required for FSI register access on the r8a7740.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 arch/arm/boot/dts/renesas/r8a7740.dtsi | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/renesas/r8a7740.dtsi b/arch/arm/boot/dts/renesas/r8a7740.dtsi
index d13ab86c3ab4..6f9d9bbfd159 100644
--- a/arch/arm/boot/dts/renesas/r8a7740.dtsi
+++ b/arch/arm/boot/dts/renesas/r8a7740.dtsi
@@ -393,7 +393,11 @@ sh_fsi2: sound@fe1f0000 {
 		compatible = "renesas,fsi2-r8a7740", "renesas,sh_fsi2";
 		reg = <0xfe1f0000 0x400>;
 		interrupts = <GIC_SPI 9 0x4>;
-		clocks = <&mstp3_clks R8A7740_CLK_FSI>;
+		clocks = <&mstp3_clks R8A7740_CLK_FSI>, <&spu_clk>,
+			<&fsia_clk>, <&fsib_clk>, <&fsiack_clk>,
+			<&fsibck_clk>;
+		clock-names = "fck", "spu", "icka", "ickb", "xcka",
+				"xckb";
 		power-domains = <&pd_a4mp>;
 		status = "disabled";
 	};
@@ -614,6 +618,12 @@ vou_clk: vou@e6150088 {
 				 <0>;
 			#clock-cells = <0>;
 		};
+		fsib_clk: fsib@e6150090 {
+			compatible = "renesas,r8a7740-div6-clock", "renesas,cpg-div6-clock";
+			reg = <0xe6150090 4>;
+			clocks = <&pllc1_div2_clk>, <&fsibck_clk>, <0>, <0>;
+			#clock-cells = <0>;
+		};
 		stpro_clk: stpro@e615009c {
 			compatible = "renesas,r8a7740-div6-clock", "renesas,cpg-div6-clock";
 			reg = <0xe615009c 4>;
-- 
2.43.0

[PATCH v5 03/11] ASoC: renesas: fsi: Fix trigger stop ordering

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Call fsi_stream_stop() before fsi_hw_shutdown(). This matches the existing
order in the suspend path.
This change ensures all register accesses during stream shutdown are fully
completed before disabling the clocks.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v5:
 - Move fsi_hw_shutdown() after fsi_stream_quit() to prepare for
   subsequent patches that use fsi_stream_is_working() to handle
   in-flight IRQ handlers.
Changes in v4:
 - update commit messages 

 sound/soc/renesas/fsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 8cbd7acc26f4..800ac40f9680 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -1586,10 +1586,10 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 			ret = fsi_stream_transfer(io);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
-		if (!ret)
-			ret = fsi_hw_shutdown(fsi, dai->dev);
 		fsi_stream_stop(fsi, io);
 		fsi_stream_quit(fsi, io);
+		if (!ret)
+			ret = fsi_hw_shutdown(fsi, dai->dev);
 		break;
 	}
 
-- 
2.43.0

[PATCH v5 04/11] ASoC: renesas: fsi: Move fsi_stream_is_working()

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Move fsi_stream_is_working() before fsi_count_fifo_err().
This prepares for a subsequent patch that needs to check stream status
when handling in-flight IRQ handlers. No functional changwqes intended.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/renesas/fsi.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 800ac40f9680..0bd0e0c8c5a3 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -442,6 +442,16 @@ static int fsi_sample2frame(struct fsi_priv *fsi, int samples)
 	return samples / fsi->chan_num;
 }
 
+static int fsi_stream_is_working(struct fsi_priv *fsi,
+				 struct fsi_stream *io)
+{
+	struct fsi_master *master = fsi_get_master(fsi);
+
+	guard(spinlock_irqsave)(&master->lock);
+
+	return !!(io->substream && io->substream->runtime);
+}
+
 static int fsi_get_current_fifo_samples(struct fsi_priv *fsi,
 					struct fsi_stream *io)
 {
@@ -488,16 +498,6 @@ static inline struct fsi_stream *fsi_stream_get(struct fsi_priv *fsi,
 	return fsi_is_play(substream) ? &fsi->playback : &fsi->capture;
 }
 
-static int fsi_stream_is_working(struct fsi_priv *fsi,
-				 struct fsi_stream *io)
-{
-	struct fsi_master *master = fsi_get_master(fsi);
-
-	guard(spinlock_irqsave)(&master->lock);
-
-	return !!(io->substream && io->substream->runtime);
-}
-
 static struct fsi_priv *fsi_stream_to_priv(struct fsi_stream *io)
 {
 	return io->priv;
-- 
2.43.0

[PATCH v5 05/11] ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

In-flight IRQs may still be running when the SPU clock is disabled,
leading to register access after shutdown and causing system hangs.

Fix this to use fsi_stream_is_working() when handling in-flight IRQ
handlers. If no streams are active, the handler now returns immediately
to prevent hardware access.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v4:
 - use fsi_stream_is_working instead of running_streams.

 sound/soc/renesas/fsi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 0bd0e0c8c5a3..3e3c6fd7c56b 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -470,6 +470,10 @@ static int fsi_get_current_fifo_samples(struct fsi_priv *fsi,
 
 static void fsi_count_fifo_err(struct fsi_priv *fsi)
 {
+	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
+	    !fsi_stream_is_working(fsi, &fsi->capture))
+		return;
+
 	u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
 	u32 istatus = fsi_reg_read(fsi, DIFF_ST);
 
@@ -681,6 +685,10 @@ static void fsi_irq_clear_status(struct fsi_priv *fsi)
 	u32 data = 0;
 	struct fsi_master *master = fsi_get_master(fsi);
 
+	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
+	    !fsi_stream_is_working(fsi, &fsi->capture))
+		return;
+
 	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->playback));
 	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->capture));
 
-- 
2.43.0

[PATCH v5 06/11] ASoC: renesas: fsi: Move fsi_clk_init()

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Move fsi_clk_init() after set_rate() functions to prepare for subsequent
refactoring.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/renesas/fsi.c | 128 ++++++++++++++++++++--------------------
 1 file changed, 64 insertions(+), 64 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 3e3c6fd7c56b..1df7dc014363 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -717,70 +717,6 @@ static void fsi_spdif_clk_ctrl(struct fsi_priv *fsi, int enable)
 /*
  *		clock function
  */
-static int fsi_clk_init(struct device *dev,
-			struct fsi_priv *fsi,
-			int xck,
-			int ick,
-			int div,
-			int (*set_rate)(struct device *dev,
-					struct fsi_priv *fsi))
-{
-	struct fsi_clk *clock = &fsi->clock;
-	int is_porta = fsi_is_port_a(fsi);
-
-	clock->xck	= NULL;
-	clock->ick	= NULL;
-	clock->div	= NULL;
-	clock->rate	= 0;
-	clock->count	= 0;
-	clock->set_rate	= set_rate;
-
-	clock->own = devm_clk_get(dev, NULL);
-	if (IS_ERR(clock->own))
-		return -EINVAL;
-
-	/* external clock */
-	if (xck) {
-		clock->xck = devm_clk_get(dev, is_porta ? "xcka" : "xckb");
-		if (IS_ERR(clock->xck)) {
-			dev_err(dev, "can't get xck clock\n");
-			return -EINVAL;
-		}
-		if (clock->xck == clock->own) {
-			dev_err(dev, "cpu doesn't support xck clock\n");
-			return -EINVAL;
-		}
-	}
-
-	/* FSIACLK/FSIBCLK */
-	if (ick) {
-		clock->ick = devm_clk_get(dev,  is_porta ? "icka" : "ickb");
-		if (IS_ERR(clock->ick)) {
-			dev_err(dev, "can't get ick clock\n");
-			return -EINVAL;
-		}
-		if (clock->ick == clock->own) {
-			dev_err(dev, "cpu doesn't support ick clock\n");
-			return -EINVAL;
-		}
-	}
-
-	/* FSI-DIV */
-	if (div) {
-		clock->div = devm_clk_get(dev,  is_porta ? "diva" : "divb");
-		if (IS_ERR(clock->div)) {
-			dev_err(dev, "can't get div clock\n");
-			return -EINVAL;
-		}
-		if (clock->div == clock->own) {
-			dev_err(dev, "cpu doesn't support div clock\n");
-			return -EINVAL;
-		}
-	}
-
-	return 0;
-}
-
 #define fsi_clk_invalid(fsi) fsi_clk_valid(fsi, 0)
 static void fsi_clk_valid(struct fsi_priv *fsi, unsigned long rate)
 {
@@ -1034,6 +970,70 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
 	return ret;
 }
 
+static int fsi_clk_init(struct device *dev,
+			struct fsi_priv *fsi,
+			int xck,
+			int ick,
+			int div,
+			int (*set_rate)(struct device *dev,
+					struct fsi_priv *fsi))
+{
+	struct fsi_clk *clock = &fsi->clock;
+	int is_porta = fsi_is_port_a(fsi);
+
+	clock->xck	= NULL;
+	clock->ick	= NULL;
+	clock->div	= NULL;
+	clock->rate	= 0;
+	clock->count	= 0;
+	clock->set_rate	= set_rate;
+
+	clock->own = devm_clk_get(dev, NULL);
+	if (IS_ERR(clock->own))
+		return -EINVAL;
+
+	/* external clock */
+	if (xck) {
+		clock->xck = devm_clk_get(dev, is_porta ? "xcka" : "xckb");
+		if (IS_ERR(clock->xck)) {
+			dev_err(dev, "can't get xck clock\n");
+			return -EINVAL;
+		}
+		if (clock->xck == clock->own) {
+			dev_err(dev, "cpu doesn't support xck clock\n");
+			return -EINVAL;
+		}
+	}
+
+	/* FSIACLK/FSIBCLK */
+	if (ick) {
+		clock->ick = devm_clk_get(dev,  is_porta ? "icka" : "ickb");
+		if (IS_ERR(clock->ick)) {
+			dev_err(dev, "can't get ick clock\n");
+			return -EINVAL;
+		}
+		if (clock->ick == clock->own) {
+			dev_err(dev, "cpu doesn't support ick clock\n");
+			return -EINVAL;
+		}
+	}
+
+	/* FSI-DIV */
+	if (div) {
+		clock->div = devm_clk_get(dev,  is_porta ? "diva" : "divb");
+		if (IS_ERR(clock->div)) {
+			dev_err(dev, "can't get div clock\n");
+			return -EINVAL;
+		}
+		if (clock->div == clock->own) {
+			dev_err(dev, "cpu doesn't support div clock\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static void fsi_pointer_update(struct fsi_stream *io, int size)
 {
 	io->buff_sample_pos += size;
-- 
2.43.0

[PATCH v5 07/11] ASoC: renesas: fsi: Use devm_clk_get_optional() for optional clocks

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

The xck, ick, and div clocks are optional. Switch from devm_clk_get()
to devm_clk_get_optional() to correctly handle cases where these clocks
are missing.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/renesas/fsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 1df7dc014363..e29be2bcf952 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -994,7 +994,7 @@ static int fsi_clk_init(struct device *dev,
 
 	/* external clock */
 	if (xck) {
-		clock->xck = devm_clk_get(dev, is_porta ? "xcka" : "xckb");
+		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
 		if (IS_ERR(clock->xck)) {
 			dev_err(dev, "can't get xck clock\n");
 			return -EINVAL;
@@ -1007,7 +1007,7 @@ static int fsi_clk_init(struct device *dev,
 
 	/* FSIACLK/FSIBCLK */
 	if (ick) {
-		clock->ick = devm_clk_get(dev,  is_porta ? "icka" : "ickb");
+		clock->ick = devm_clk_get_optional(dev,  is_porta ? "icka" : "ickb");
 		if (IS_ERR(clock->ick)) {
 			dev_err(dev, "can't get ick clock\n");
 			return -EINVAL;
@@ -1020,7 +1020,7 @@ static int fsi_clk_init(struct device *dev,
 
 	/* FSI-DIV */
 	if (div) {
-		clock->div = devm_clk_get(dev,  is_porta ? "diva" : "divb");
+		clock->div = devm_clk_get_optional(dev,  is_porta ? "diva" : "divb");
 		if (IS_ERR(clock->div)) {
 			dev_err(dev, "can't get div clock\n");
 			return -EINVAL;
-- 
2.43.0

[PATCH v5 08/11] ASoC: renesas: fsi: refactor clock initialization

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Move fsi_clk_init() from set_fmt() to the probe path.
This ensures that clock resources are acquired only once during device
initialization, instead of being looked up repeatedly whenever set_fmt()
is called.
Together with the previous conversion to devm_clk_get_optional(), the
driver can now probe successfully even when optional clocks are absent.
The set_rate() callbacks continue to validate that all required clocks
are available before applying hardware-specific configuration.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v5:
 - Reorder the patches 
 - update the error handling path to ensure proper cleanup by Sashiko 

Changes in v4:
 - Handle the return value of fsi_clk_init() to properly support deferred 
   probe, as suggested by Mark.

 sound/soc/renesas/fsi.c | 63 +++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index e29be2bcf952..13988c7dc926 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -862,6 +862,11 @@ static int fsi_clk_set_rate_external(struct device *dev,
 	int ackmd, bpfmd;
 	int ret = 0;
 
+	if (!xck || !ick) {
+		dev_err(dev, "xck clock or ick clock is missing\n");
+		return -EINVAL;
+	}
+
 	/* check clock rate */
 	xrate = clk_get_rate(xck);
 	if (xrate % rate) {
@@ -898,6 +903,11 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
 	int ackmd, bpfmd;
 	int ret = -EINVAL;
 
+	if (!ick || !div) {
+		dev_err(dev, "ick clock or div clock is missing\n");
+		return -EINVAL;
+	}
+
 	if (!(12288000 % rate))
 		target = 12288000;
 	if (!(11289600 % rate))
@@ -970,35 +980,35 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
 	return ret;
 }
 
-static int fsi_clk_init(struct device *dev,
-			struct fsi_priv *fsi,
-			int xck,
-			int ick,
-			int div,
-			int (*set_rate)(struct device *dev,
-					struct fsi_priv *fsi))
+static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
 {
 	struct fsi_clk *clock = &fsi->clock;
 	int is_porta = fsi_is_port_a(fsi);
+	int xck, ick, div;
+
+	if (fsi->clk_cpg) {
+		xck = 0; ick = 1; div = 1;
+		clock->set_rate = fsi_clk_set_rate_cpg;
+	} else {
+		xck = 1; ick = 1; div = 0;
+		clock->set_rate = fsi_clk_set_rate_external;
+	}
 
 	clock->xck	= NULL;
 	clock->ick	= NULL;
 	clock->div	= NULL;
 	clock->rate	= 0;
 	clock->count	= 0;
-	clock->set_rate	= set_rate;
 
 	clock->own = devm_clk_get(dev, NULL);
 	if (IS_ERR(clock->own))
-		return -EINVAL;
+		return dev_err_probe(dev, PTR_ERR(clock->own), "Can't get fck clock\n");
 
 	/* external clock */
 	if (xck) {
 		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
-		if (IS_ERR(clock->xck)) {
-			dev_err(dev, "can't get xck clock\n");
-			return -EINVAL;
-		}
+		if (IS_ERR(clock->xck))
+			return dev_err_probe(dev, PTR_ERR(clock->xck), "Can't get xck clock\n");
 		if (clock->xck == clock->own) {
 			dev_err(dev, "cpu doesn't support xck clock\n");
 			return -EINVAL;
@@ -1008,10 +1018,8 @@ static int fsi_clk_init(struct device *dev,
 	/* FSIACLK/FSIBCLK */
 	if (ick) {
 		clock->ick = devm_clk_get_optional(dev,  is_porta ? "icka" : "ickb");
-		if (IS_ERR(clock->ick)) {
-			dev_err(dev, "can't get ick clock\n");
-			return -EINVAL;
-		}
+		if (IS_ERR(clock->ick))
+			return dev_err_probe(dev, PTR_ERR(clock->ick), "Can't get ick clock\n");
 		if (clock->ick == clock->own) {
 			dev_err(dev, "cpu doesn't support ick clock\n");
 			return -EINVAL;
@@ -1021,10 +1029,8 @@ static int fsi_clk_init(struct device *dev,
 	/* FSI-DIV */
 	if (div) {
 		clock->div = devm_clk_get_optional(dev,  is_porta ? "diva" : "divb");
-		if (IS_ERR(clock->div)) {
-			dev_err(dev, "can't get div clock\n");
-			return -EINVAL;
-		}
+		if (IS_ERR(clock->div))
+			return dev_err_probe(dev, PTR_ERR(clock->div), "Can't get div clock\n");
 		if (clock->div == clock->own) {
 			dev_err(dev, "cpu doesn't support div clock\n");
 			return -EINVAL;
@@ -1672,15 +1678,6 @@ static int fsi_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		break;
 	}
 
-	if (fsi_is_clk_master(fsi)) {
-		if (fsi->clk_cpg)
-			fsi_clk_init(dai->dev, fsi, 0, 1, 1,
-				     fsi_clk_set_rate_cpg);
-		else
-			fsi_clk_init(dai->dev, fsi, 1, 1, 0,
-				     fsi_clk_set_rate_external);
-	}
-
 	/* set format */
 	if (fsi_is_spdif(fsi))
 		ret = fsi_set_fmt_spdif(fsi);
@@ -1983,6 +1980,9 @@ static int fsi_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "FSIA stream probe failed\n");
 		return ret;
 	}
+	ret = fsi_clk_init(&pdev->dev, fsi);
+	if (ret)
+		goto exit_fsia;
 
 	/* FSI B setting */
 	fsi		= &master->fsib;
@@ -1996,6 +1996,9 @@ static int fsi_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "FSIB stream probe failed\n");
 		goto exit_fsia;
 	}
+	ret = fsi_clk_init(&pdev->dev, fsi);
+	if (ret)
+		goto exit_fsib;
 
 	pm_runtime_enable(&pdev->dev);
 	dev_set_drvdata(&pdev->dev, master);
-- 
2.43.0

[PATCH v5 09/11] ASoC: renesas: fsi: Add SPU clock support

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

FSI register accesses on the r8a7740 require the SPU bus clock to be
enabled. Add support for acquiring and managing the SPU clock via the
device tree to ensure proper register access.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/renesas/fsi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 13988c7dc926..6c432c7235a4 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -292,6 +292,7 @@ struct fsi_master {
 	void __iomem *base;
 	struct fsi_priv fsia;
 	struct fsi_priv fsib;
+	struct clk *clk_spu;
 	const struct fsi_core *core;
 	spinlock_t lock;
 };
@@ -983,6 +984,7 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
 static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
 {
 	struct fsi_clk *clock = &fsi->clock;
+	struct fsi_master *master = fsi->master;
 	int is_porta = fsi_is_port_a(fsi);
 	int xck, ick, div;
 
@@ -1004,6 +1006,13 @@ static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
 	if (IS_ERR(clock->own))
 		return dev_err_probe(dev, PTR_ERR(clock->own), "Can't get fck clock\n");
 
+	if (!master->clk_spu) {
+		master->clk_spu = devm_clk_get_optional(dev, "spu");
+		if (IS_ERR(master->clk_spu))
+			return dev_err_probe(dev, PTR_ERR(master->clk_spu),
+					"Can't get spu clock\n");
+	}
+
 	/* external clock */
 	if (xck) {
 		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
-- 
2.43.0

[PATCH v5 10/11] ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Add fsi_clk_prepare() and fsi_clk_unprepare() helpers and call them
from fsi_dai_startup() and fsi_dai_shutdown().
This ensures clk_prepare() and clk_unprepare() are executed from
sleepable contexts and keeps clocks prepared only while audio streams
are active.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v5:
 - Drop count & spu_count and rely on the clk core for clock reference
   counting.

Changes in v4:
 - Move clock->count early return check to the beginning of 
   fsi_clk_[un]prepare() to simplify the code.

 sound/soc/renesas/fsi.c | 51 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 6c432c7235a4..80e87b815c1b 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -730,6 +730,54 @@ static int fsi_clk_is_valid(struct fsi_priv *fsi)
 		fsi->clock.rate;
 }
 
+static int fsi_clk_prepare(struct fsi_priv *fsi)
+{
+	struct fsi_clk *clock = &fsi->clock;
+	struct clk *spu = fsi->master->clk_spu;
+	struct clk *xck = clock->xck;
+	struct clk *ick = clock->ick;
+	struct clk *div = clock->div;
+	int ret;
+
+	ret = clk_prepare(spu);
+	if (ret)
+		return ret;
+	ret = clk_prepare(xck);
+	if (ret)
+		goto err_spu;
+	ret = clk_prepare(ick);
+	if (ret)
+		goto err_xck;
+	ret = clk_prepare(div);
+	if (ret)
+		goto err_ick;
+
+	return 0;
+
+err_ick:
+	clk_unprepare(ick);
+err_xck:
+	clk_unprepare(xck);
+err_spu:
+	clk_unprepare(spu);
+
+	return ret;
+}
+
+static void fsi_clk_unprepare(struct fsi_priv *fsi)
+{
+	struct fsi_clk *clock = &fsi->clock;
+	struct clk *spu = fsi->master->clk_spu;
+	struct clk *xck = clock->xck;
+	struct clk *ick = clock->ick;
+	struct clk *div = clock->div;
+
+	clk_unprepare(div);
+	clk_unprepare(ick);
+	clk_unprepare(xck);
+	clk_unprepare(spu);
+}
+
 static int fsi_clk_enable(struct device *dev,
 			  struct fsi_priv *fsi)
 {
@@ -1580,7 +1628,7 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream,
 
 	fsi_clk_invalid(fsi);
 
-	return 0;
+	return fsi_clk_prepare(fsi);
 }
 
 static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
@@ -1588,6 +1636,7 @@ static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
 {
 	struct fsi_priv *fsi = fsi_get_priv(substream);
 
+	fsi_clk_unprepare(fsi);
 	fsi_clk_invalid(fsi);
 }
 
-- 
2.43.0

[PATCH v5 11/11] ASoC: renesas: fsi: Add SPU clock control in hw_startup/shutdown

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Enable and disable the SPU clock in fsi_hw_startup() and
fsi_hw_shutdown() to ensure the clock is active while the
driver accesses hardware registers.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v5:
 - Drop spu_count and rely on the clk core for clock reference
   counting.

 sound/soc/renesas/fsi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 80e87b815c1b..a1c463d73756 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -1560,6 +1560,11 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
 			  struct device *dev)
 {
 	u32 data = 0;
+	int ret;
+	/* enable spu bus bridge clock */
+	ret = clk_enable(fsi->master->clk_spu);
+	if (ret < 0)
+		return ret;
 
 	/* clock setting */
 	if (fsi_is_clk_master(fsi))
@@ -1617,6 +1622,8 @@ static int fsi_hw_shutdown(struct fsi_priv *fsi,
 	/* stop master clock */
 	if (fsi_is_clk_master(fsi))
 		return fsi_clk_disable(dev, fsi);
+	/* stop spu bus bridge clock */
+	clk_disable(fsi->master->clk_spu);
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH v5 01/11] ASoC: dt-bindings: renesas,fsi: add support multiple clocks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The `clocks` property specifies an `items` array with a rigid positional sequence that contradicts the flexible order allowed by `clock-names` and demonstrated in the example.
--

commit e7db6aae2c19d98ecd80fa5d59fbb27124874059
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: dt-bindings: renesas,fsi: add support multiple clocks

The FSI on r8a7740 requires the SPU bus/bridge clock to be enabled before
accessing its registers. This update adds multiple clocks to properly
describe the hardware clock tree.

> diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> index df91991699a72..b966b55ff7728 100644
> --- a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> +++ b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
[ ... ]
> @@ -38,7 +35,32 @@ properties:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: Main FSI module clock
> +      - description: |
> +          SPU bus/bridge clock. On R8A7740, this clock must be enabled to allow
> +          register access as the FSI block is connected behind the SPU bus.
> +      - description: CPG DIV6 functional clocks for FSI port A
> +      - description: CPG DIV6 functional clocks for FSI port B
> +      - description: FSI dividers for port A used for audio clock generation
> +      - description: FSI dividers for port B used for audio clock generation
> +      - description: External clock inputs for FSI port A provided by the board
> +      - description: External clock inputs for FSI port B provided by the board
> +

[Severity: Low]
Does this rigid items array cause a regression with dt-schema validation? 

Defining the items as a list of descriptions enforces strict positional
indexing, meaning the schema expects index 3 to specifically be 'CPG DIV6
functional clocks for FSI port B'. This contradicts the flexible enum defined
below in clock-names.

[ ... ]
> @@ -69,6 +91,31 @@ required:
>  
>  unevaluatedProperties: false
>  
> +allOf:
> +  - $ref: dai-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,fsi2-r8a7740
> +    then:
> +      required:
> +        - clock-names
> +
> +      properties:
> +        clock-names:
> +          minItems: 2
> +          uniqueItems: true
> +          items:
> +            - const: fck
> +            - const: spu
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +

[Severity: Low]
Is this enum-based list in conflict with the strict positional sequence
in the clocks array?

The clock-names property permits these optional clocks to appear
in any arbitrary order, whereas the clocks descriptions require a fixed
sequence.

>  examples:
>    - |
>      #include <dt-bindings/clock/r8a7740-clock.h>
> @@ -77,7 +124,11 @@ examples:
>              compatible = "renesas,fsi2-r8a7740", "renesas,sh_fsi2";
>              reg = <0xfe1f0000 0x400>;
>              interrupts = <GIC_SPI 9 0x4>;
> -            clocks = <&mstp3_clks R8A7740_CLK_FSI>;
> +            clocks = <&mstp3_clks R8A7740_CLK_FSI>, <&spu_clk>,
> +                    <&fsia_clk>, <&fsiack_clk>, <&fsidiva_clk>,
> +                    <&fsib_clk>, <&fsibck_clk>, <&fsidivb_clk>;
> +            clock-names = "fck", "spu", "icka", "xcka", "diva",
> +                         "ickb", "xckb", "divb";

[Severity: Low]
Does this example directly violate the strict positional sequence defined
above in the clocks array? 

The example provides the clocks in an order (fck, spu, icka, xcka, diva...)
that doesn't match the fixed list, which expects ickb in the 4th position.
This leads to semantically incorrect documentation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609013107.5995-1-phucduc.bui@gmail.com?part=1

Re: [PATCH v5 03/11] ASoC: renesas: fsi: Fix trigger stop ordering

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] Clock reference counting logic in `fsi_clk_enable()` permanently blocks subsequent active streams.
- [High] Asynchronous tasks (IRQ and DMA callbacks) dereference `io->substream` after it is freed by `fsi_stream_quit()`, causing a TOCTOU NULL pointer dereference.
- [High] Hardware DMA channels are released before ALSA component unregistration, leading to a Use-After-Free during teardown.
- [High] The ALSA trigger callback calls `clk_set_rate()`, which acquires sleepable mutexes, from an atomic context.
- [High] Missing synchronization with the IRQ handler before disabling the SPU clock in `TRIGGER_STOP`, causing hardware hangs.
- [High] Missing error rollback in `TRIGGER_START` leads to a permanent resource leak and device failure.
--

commit b4a51fcc95452ce19991b681d00a481811b27efc
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Fix trigger stop ordering

Call fsi_stream_stop() before fsi_hw_shutdown(). This matches the existing
order in the suspend path.
This change ensures all register accesses during stream shutdown are fully
completed before disabling the clocks.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 8cbd7acc26f49..800ac40f9680d 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1586,10 +1586,10 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  			ret = fsi_stream_transfer(io);

[Severity: High]
This is a pre-existing issue, but if fsi_stream_transfer() fails during
SNDRV_PCM_TRIGGER_START, the error is returned immediately without rolling
back earlier steps like fsi_hw_startup() and fsi_clk_enable().

Could this leak the clock->count reference and leave hardware clocks
permanently enabled?

[Severity: High]
This isn't a bug introduced by this patch, but does fsi_dai_trigger() run in
atomic context since .nonatomic = 1 is not set on the DAI?

If so, is it safe to call fsi_hw_startup() which calls fsi_clk_enable() and
subsequently clock->set_rate() (which calls clk_set_rate())?

clk_set_rate() acquires a sleepable mutex:
fsi_dai_trigger() -> fsi_hw_startup() -> fsi_clk_enable() -> clk_set_rate()

Could this trigger a scheduling while atomic panic?

[Severity: High]
This is a pre-existing issue, but looking at fsi_clk_enable(), if a second
stream tries to start while one is already active (clock->count > 0), does
the function return -EINVAL instead of 0?

sound/soc/renesas/fsi.c:fsi_clk_enable() {
	int ret = -EINVAL;
	if (0 == clock->count) {
		ret = clock->set_rate(dev, fsi);
		...
		clock->count++;
	}
	return ret;
}

Could this permanently fail full-duplex operations by preventing
simultaneous playback and capture?

Also, could the lack of atomic protection on clock->count lead to races
between independent substream triggers?

>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:
> -		if (!ret)
> -			ret = fsi_hw_shutdown(fsi, dai->dev);
>  		fsi_stream_stop(fsi, io);
>  		fsi_stream_quit(fsi, io);

[Severity: High]
This isn't a bug introduced by this patch, but fsi_stream_quit() sets
io->substream = NULL under master->lock.

Is there a race window with asynchronous tasks like the IRQ handler or DMA
callbacks?

fsi_pio_transfer() checks fsi_stream_is_working() which verifies
io->substream but drops the lock before returning:

sound/soc/renesas/fsi.c:fsi_pio_transfer() {
	if (!fsi_stream_is_working(fsi, io))
		return -EINVAL;
	buf = fsi_pio_get_area(fsi, io);
	...
}

Could fsi_stream_quit() nullify io->substream immediately after the lock is
dropped, causing fsi_pio_get_area() to dereference a NULL pointer when
accessing io->substream->runtime?

> +		if (!ret)
> +			ret = fsi_hw_shutdown(fsi, dai->dev);

[Severity: High]
This is a pre-existing issue, but since fsi_hw_shutdown() disables the SPU
hardware clock, could this cause a hardware hang if the IRQ handler is still
running concurrently on another CPU?

Without synchronize_irq() to wait for completion, couldn't fsi_interrupt()
access hardware registers like CPU_INT_ST or SOFT_RST after the clock is
disabled, resulting in an external abort or bus fault?

>  		break;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does fsi_remove() prematurely
destroy hardware channels before devm tears down the ALSA interface?

sound/soc/renesas/fsi.c:fsi_remove() {
	...
	fsi_stream_remove(&master->fsia);
	fsi_stream_remove(&master->fsib);
	...
}

Since the ALSA component is registered using
devm_snd_soc_register_component(), it is unregistered later during the
devres cleanup phase.

Could userspace trigger operations on the still-visible PCM devices that
call dmaengine_prep_dma_cyclic() with a NULL io->chan, resulting in a
use-after-free during teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609013107.5995-1-phucduc.bui@gmail.com?part=3

Re: [PATCH v5 08/11] ASoC: renesas: fsi: refactor clock initialization

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unbalanced pm_runtime_disable() on the fsi_clk_init() error path before pm_runtime_enable() is called permanently breaks device PM on deferred probe.

Pre-existing issues:
- [High] Concurrent execution of playback and capture triggers causes lockless read-modify-write races on port registers and corrupts clock refcounting.
--

commit 3878a75703ac4c551d532bdff9ddca5b624ed448
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: refactor clock initialization

Move fsi_clk_init() from set_fmt() to the probe path. This ensures that
clock resources are acquired only once during device initialization.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index e29be2bcf952c..13988c7dc926e 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c

[ ... ]

> @@ -1996,6 +1996,9 @@ static int fsi_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "FSIB stream probe failed\n");
>  		goto exit_fsia;
>  	}
> +	ret = fsi_clk_init(&pdev->dev, fsi);
> +	if (ret)
> +		goto exit_fsib;

[Severity: High]
If fsi_clk_init() fails here, we jump to exit_fsib before
pm_runtime_enable() is called.

At the exit_fsib label, pm_runtime_disable() is called unconditionally. Will
this cause an unbalanced pm_runtime_disable() call, incrementing the
device's disable_depth to 2?

If this happens during a probe deferral (e.g. -EPROBE_DEFER because a clock
is not yet available), the subsequent probe attempt's pm_runtime_enable()
will only decrement the depth to 1. Does this leave PM runtime permanently
disabled and break all future hardware initialization?

>  
>  	pm_runtime_enable(&pdev->dev);
>  	dev_set_drvdata(&pdev->dev, master);

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition with
concurrent execution of playback and capture triggers.

ALSA core holds individual substream locks, meaning ALSA trigger callbacks
can be invoked concurrently for playback and capture substreams on the same
DAI.

In fsi_dai_trigger(), both start operations call fsi_hw_startup(), which
performs lockless read-modify-write operations on shared port registers:

fsi_hw_startup()
    fsi_reg_mask_set(fsi, CKG1, (DIMD | DOMD), data);

Could Thread A set ACKMD bits while Thread B concurrently sets DIMD bits on
the same register, causing one of the updates to be lost?

Additionally, fsi_clk_enable() uses a shared clock->count without atomic
operations or locking:

fsi_clk_enable()
    if (0 == clock->count) {
        ret = clock->set_rate(dev, fsi);
        ...
        clock->count++;
    }

Could a concurrent increment of clock->count lead to both streams calling
clk_enable() but clock->count only reaching 1? On shutdown, clk_disable()
would only be called once, potentially leaving unbalanced clock refcounts in
the common clock framework and permanently leaking the clock reference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609013107.5995-1-phucduc.bui@gmail.com?part=8

Re: [PATCH v5 04/11] ASoC: renesas: fsi: Move fsi_stream_is_working()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] `fsi_stream_is_working()` acquires a lock to validate a pointer but returns a boolean and drops the lock before the caller uses the pointer, creating a Time-Of-Check to Time-Of-Use (TOCTOU) race condition that can lead to a NULL pointer dereference.
- [High] Sleepable Common Clock Framework (CCF) APIs are called from within the ALSA PCM atomic trigger path, causing a sleep-in-atomic bug.
--

commit da681410389113931660183bb91da994c3ed66c1
Author: bui duc phuc <phucduc.bui@gmail.com>

    ASoC: renesas: fsi: Move fsi_stream_is_working()

Move fsi_stream_is_working() before fsi_count_fifo_err() to prepare for a
subsequent patch. No functional changes are intended.

>     when handling in-flight IRQ handlers. No functional changwqes intended.

This isn't a bug, but there's a typo (changwqes) in the commit message.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 800ac40f9680d..0bd0e0c8c5a30 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c

[ ... ]

> @@ -442,6 +442,16 @@ static int fsi_sample2frame(struct fsi_priv *fsi, int samples)
>  	return samples / fsi->chan_num;
>  }
>  
> +static int fsi_stream_is_working(struct fsi_priv *fsi,
> +				 struct fsi_stream *io)
> +{
> +	struct fsi_master *master = fsi_get_master(fsi);
> +
> +	guard(spinlock_irqsave)(&master->lock);
> +
> +	return !!(io->substream && io->substream->runtime);
> +}
> +

[Severity: High]
This isn't a bug introduced by this patch, but does returning a boolean here
create a Time-Of-Check to Time-Of-Use (TOCTOU) race condition?

When fsi_pio_transfer() calls this, it uses the boolean result to locklessly
call fsi_pio_get_area():

sound/soc/renesas/fsi.c:fsi_pio_transfer() {
	...
	if (!fsi_stream_is_working(fsi, io))
		return -EINVAL;
	buf = fsi_pio_get_area(fsi, io);
	...
}

fsi_pio_get_area() then directly accesses io->substream->runtime:

sound/soc/renesas/fsi.c:fsi_pio_get_area() {
	...
	struct snd_pcm_runtime *runtime = io->substream->runtime;
	...
}

Concurrently, if ALSA invokes fsi_dai_trigger(SNDRV_PCM_TRIGGER_STOP), it
calls fsi_stream_quit() which acquires master->lock and clears the pointer:

sound/soc/renesas/fsi.c:fsi_stream_quit() {
	...
	guard(spinlock_irqsave)(&master->lock);
	...
	io->substream	= NULL;
	...
}

If fsi_stream_quit() executes after fsi_stream_is_working() drops the lock
but before fsi_pio_get_area() dereferences io->substream->runtime, could
this result in a NULL pointer dereference?

[Severity: High]
This isn't a bug introduced by this patch, but does the ALSA trigger path
call sleepable clock APIs while in atomic context?

When ALSA invokes the snd_pcm_trigger callback fsi_dai_trigger() on the
SNDRV_PCM_TRIGGER_START path, it executes in atomic context (with the
substream spinlock held and local interrupts disabled).

This calls fsi_hw_startup(), which subsequently triggers fsi_clk_enable():

sound/soc/renesas/fsi.c:fsi_dai_trigger() {
	...
	case SNDRV_PCM_TRIGGER_START:
	...
		if (!ret)
			ret = fsi_hw_startup(fsi, io, dai->dev);
	...
}

fsi_hw_startup() then executes:

sound/soc/renesas/fsi.c:fsi_hw_startup() {
	...
	if (fsi_is_clk_master(fsi))
		return fsi_clk_enable(dev, fsi);
	...
}

fsi_clk_enable() calls the driver's dynamic .set_rate callback:

sound/soc/renesas/fsi.c:fsi_clk_enable() {
	...
	ret = clock->set_rate(dev, fsi);
	...
}

For example, fsi_clk_set_rate_cpg() invokes clk_set_rate():

sound/soc/renesas/fsi.c:fsi_clk_set_rate_cpg() {
	...
	ret = clk_set_rate(ick, best_cout);
	...
}

Since clk_set_rate() acquires the prepare_lock mutex, won't this trigger
a sleep-in-atomic bug or deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609013107.5995-1-phucduc.bui@gmail.com?part=4

Re: [PATCH v5 06/11] ASoC: renesas: fsi: Move fsi_clk_init()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A TOCTOU race condition in `fsi_stream_is_working()` allows concurrent stream shutdown, leading to hardware accesses while clocks are gated and NULL pointer dereferences.
--

commit 2a7ff11c2bbdedadae9c569f5a5fb36f784c525a
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Move fsi_clk_init()
    
Move fsi_clk_init() after set_rate() functions to prepare for subsequent
refactoring.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 3e3c6fd7c56b8..1df7dc0143630 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c

[ ... ]

> @@ -1034,6 +970,70 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
>  	return ret;
>  }
>  
> +static int fsi_clk_init(struct device *dev,

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the clock
handling in this file, does fsi_stream_is_working() have a TOCTOU race
condition that allows concurrent stream shutdown?

It appears that fsi_stream_is_working() acquires and immediately releases
master->lock before returning a boolean. Concurrently, an ALSA
SNDRV_PCM_TRIGGER_STOP on another CPU can call fsi_stream_quit(), which
acquires the lock, sets io->substream = NULL, and releases it. This can be
followed by fsi_hw_shutdown() disabling the hardware clocks.

Because the lock is dropped in fsi_stream_is_working(), can an in-flight
fsi_interrupt() on CPU 0 evaluate the check as true, after which CPU 1
processes the STOP trigger, zeroes the substream pointer, and gates the
clocks?

CPU 0 could then proceed to access hardware registers while clocks are
disabled:

sound/soc/renesas/fsi.c:fsi_count_fifo_err() {
	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
	    !fsi_stream_is_working(fsi, &fsi->capture))
		return;

	u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
	...
}

Would this cause a system hang due to hardware access while clocks are
gated?

Additionally, in fsi_pio_transfer(), could this same TOCTOU race allow
execution to reach fsi_pio_get_area(), which unconditionally dereferences
io->substream->runtime?

sound/soc/renesas/fsi.c:fsi_pio_transfer() {
	if (!fsi_stream_is_working(fsi, io))
		return -EINVAL;

	buf = fsi_pio_get_area(fsi, io);
	...
}

sound/soc/renesas/fsi.c:fsi_pio_get_area() {
	struct snd_pcm_runtime *runtime = io->substream->runtime;

	return runtime->dma_area + ...
}

Could this result in a NULL pointer dereference if io->substream was
zeroed? This seems to be an issue that was only partially addressed by the
earlier commit "ASoC: renesas: fsi: Fix register access from in-flight IRQ
after shutdown".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609013107.5995-1-phucduc.bui@gmail.com?part=6

Re: [PATCH v5 09/11] ASoC: renesas: fsi: Add SPU clock support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Redundant device tree clock lookup and devres allocation for missing optional clock.

Pre-existing issues:
- [High] TOCTOU race in interrupt processing leading to NULL pointer dereference and unclocked register accesses.
--

commit 86706e81d382bcf14351e2e3d3945e492662cf7c
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Add SPU clock support

FSI register accesses on the r8a7740 require the SPU bus clock to be
enabled. Add support for acquiring and managing the SPU clock via the
device tree to ensure proper register access.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 13988c7dc926e..6c432c7235a41 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
[ ... ]
> @@ -1004,6 +1006,13 @@ static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
>  	if (IS_ERR(clock->own))
>  		return dev_err_probe(dev, PTR_ERR(clock->own), "Can't get fck clock\n");
>  
> +	if (!master->clk_spu) {
> +		master->clk_spu = devm_clk_get_optional(dev, "spu");
> +		if (IS_ERR(master->clk_spu))
> +			return dev_err_probe(dev, PTR_ERR(master->clk_spu),
> +					"Can't get spu clock\n");
> +	}
> +

[Severity: Low]
Does this code result in redundant device tree lookups and devres allocations
if the optional clock is missing?

Since fsi_clk_init() is called twice (once for FSIA and once for FSIB), if the
optional "spu" clock is not in the device tree, devm_clk_get_optional()
returns NULL.

The second time fsi_clk_init() is called, the (!master->clk_spu) check will
pass again, causing another devm_clk_get_optional() lookup and allocating a
second devres tracking node for a NULL clock pointer.

Could the clock acquisition be moved to a master-level initialization function
instead?

[Severity: High]
This is a pre-existing issue, but does a TOCTOU race condition exist between
interrupt processing and ALSA trigger stops that could lead to a NULL pointer
dereference or external abort?

In the interrupt handler path, functions like fsi_pio_transfer() and
fsi_count_fifo_err() check stream activity by calling fsi_stream_is_working():

sound/soc/renesas/fsi.c:fsi_pio_transfer() {
    ...
	if (!fsi_stream_is_working(fsi, io))
		return -EINVAL;

	buf = fsi_pio_get_area(fsi, io);
    ...
}

However, fsi_stream_is_working() acquires master->lock, checks if
io->substream is valid, and immediately drops the lock before returning.

Concurrently, ALSA core can call fsi_dai_trigger(SNDRV_PCM_TRIGGER_STOP),
which invokes fsi_stream_quit(). This function takes master->lock, sets
io->substream to NULL, drops the lock, and disables the hardware clocks.

If CPU2 stops the stream right after CPU1's fsi_stream_is_working() drops the
lock, CPU1 can then call fsi_pio_get_area() and unconditionally dereference
io->substream->runtime, causing a fatal NULL pointer dereference.

Similarly, in fsi_count_fifo_err(), the same race window exists before
calling fsi_reg_read(fsi, DOFF_ST), which can trigger a synchronous external
abort if the clock has already been disabled by CPU2.

Could we hold the lock across the entire interrupt processing sequence, or
validate io->substream safely under the lock without dropping it prematurely?

>  	/* external clock */
>  	if (xck) {
>  		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609013107.5995-1-phucduc.bui@gmail.com?part=9

Re: [PATCH v5 11/11] ASoC: renesas: fsi: Add SPU clock control in hw_startup/shutdown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The `clk_spu` clock reference is permanently leaked due to an early return in the shutdown path and a missing error cleanup in the startup path.

Pre-existing issues:
- [High] Data race on the shared clock reference counter `clock->count` between concurrent playback and capture triggers.
- [High] Sleeping in atomic context caused by calling `clk_set_rate()` within the ALSA DAI trigger callback.
--

commit 0619c0d2b5fae18d99bc804f41b27cefd1176f67
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Add SPU clock control in hw_startup/shutdown

This patch enables and disables the SPU clock in fsi_hw_startup() and
fsi_hw_shutdown() to ensure the clock is active while the driver accesses
hardware registers.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1560,6 +1560,11 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>  			  struct device *dev)
>  {
>  	u32 data = 0;
> +	int ret;
> +	/* enable spu bus bridge clock */
> +	ret = clk_enable(fsi->master->clk_spu);
> +	if (ret < 0)
> +		return ret;
>  
>  	/* clock setting */

[Severity: High]
This is a pre-existing issue, but calling clk_enable() and interacting with
the clock API from the startup path seems to lead to sleeping in an atomic
context.

The ALSA core invokes the fsi_dai_trigger() callback with the substream
stream_lock spinlock held and hardware interrupts disabled. Inside this
atomic context, the call chain leads to sleepable clock setup functions:

fsi_dai_trigger()
   fsi_hw_startup()
      fsi_clk_enable()
         ret = clock->set_rate(dev, fsi);

For example, fsi_clk_set_rate_cpg() executes clk_set_rate(), which
acquires a mutex. Will taking a mutex while holding a spinlock with
interrupts disabled trigger a sleeping in atomic context warning or a
potential deadlock?

Furthermore, later in fsi_hw_startup(), the function ends with:

    if (fsi_is_clk_master(fsi))
        return fsi_clk_enable(dev, fsi);

If fsi_clk_enable() returns an error, the function exits without disabling
the successfully enabled clk_spu. Does this need an error handling path to
avoid leaking the clock reference?

[ ... ]

> @@ -1617,6 +1622,8 @@ static int fsi_hw_shutdown(struct fsi_priv *fsi,
>  	/* stop master clock */
>  	if (fsi_is_clk_master(fsi))
>  		return fsi_clk_disable(dev, fsi);
> +	/* stop spu bus bridge clock */
> +	clk_disable(fsi->master->clk_spu);
>  
>  	return 0;
>  }

[Severity: High]
If fsi_is_clk_master(fsi) is true, the function returns early and completely
bypasses the newly added clk_disable(fsi->master->clk_spu) call. Will this
cause the SPU clock reference to leak when stopping the audio stream?

[Severity: High]
This is a pre-existing issue, but I noticed a potential data race on the
shared clock reference counter clock->count when starting and stopping
the streams.

Because the ALSA core allows independent and concurrent triggering of
playback and capture substreams on the same DAI, fsi_hw_startup() and
fsi_hw_shutdown() can be executed in parallel by different CPUs.

In fsi_clk_enable(), clock->count is read and incremented locklessly:

    if (0 == clock->count) {
        ret = clock->set_rate(dev, fsi);
        ...
        clock->count++;
    }

Similarly, fsi_clk_disable() decrements it without synchronization:

    if (1 == clock->count--) {
        clk_disable(clock->xck);
        ...
    }

Could this race condition lead to unbalanced clock reference counts and fail
to properly enable or disable the hardware clocks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609013107.5995-1-phucduc.bui@gmail.com?part=11

Re: [PATCH v5 10/11] ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The interrupt handler `fsi_interrupt` reads and writes hardware registers before verifying if any streams are active, risking system hangs.

Pre-existing issues:
- [High] `fsi_clk_enable()` incorrectly returns `-EINVAL` and improperly manages `clock->count` when a second concurrent stream is started, breaking full-duplex operation.
- [High] The driver calls `clk_set_rate` and `clk_set_parent` from the atomic PCM trigger callback, causing a 'scheduling while atomic' bug.
- [High] Opening or closing a stream resets the shared clock rate via `fsi_clk_invalid()`, corrupting the state of an already running concurrent stream and causing permanent clock reference leaks.
- [High] Lockless access to the shared `clock->count` variable leads to race conditions and unbalanced clock reference counts when playback and capture are triggered concurrently.
--

commit f7c3ccc68e9715cc648ab7fe51a1667d7610390b
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

Add fsi_clk_prepare() and fsi_clk_unprepare() helpers and call them
from fsi_dai_startup() and fsi_dai_shutdown(). This ensures clk_prepare()
and clk_unprepare() are executed from sleepable contexts and keeps clocks
prepared only while audio streams are active.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 6c432c7235a41..80e87b815c1b6 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -730,6 +730,54 @@ static int fsi_clk_is_valid(struct fsi_priv *fsi)
>  		fsi->clock.rate;
>  }
>  
> +static int fsi_clk_prepare(struct fsi_priv *fsi)
[ ... ]
> +static void fsi_clk_unprepare(struct fsi_priv *fsi)
[ ... ]
> +}
> +
>  static int fsi_clk_enable(struct device *dev,
>  			  struct fsi_priv *fsi)
>  {

[Severity: High]
This is a pre-existing issue, but does fsi_clk_enable() break when a second
concurrent stream is started?

In fsi_clk_enable():

	int ret = -EINVAL;

	if (!fsi_clk_is_valid(fsi))
		return ret;

	if (0 == clock->count) {
		...
	}

	return ret;

If clock->count > 0, such as when starting a capture stream while playback is
already active, the zero check is skipped entirely. The function then returns
the unassigned ret (-EINVAL), causing the ALSA trigger callback to fail.

[Severity: High]
This isn't a bug introduced by this patch, but does fsi_clk_enable() cause a
"scheduling while atomic" bug?

The ALSA fsi_dai_trigger() callback executes in an atomic context with the PCM
substream spinlock held and interrupts disabled. On a START trigger, it calls
fsi_hw_startup(), which invokes fsi_clk_enable():

	if (0 == clock->count) {
		ret = clock->set_rate(dev, fsi);

The clock->set_rate() functions invoke clk_set_rate() and clk_set_parent(),
which acquire the clk_prepare_lock mutex and can sleep.

Calling them from an atomic context causes a BUG. While this patch moves
clk_prepare() to the sleepable startup callback, rate setting remains in the
atomic trigger path.

[Severity: High]
This is a pre-existing issue, but could lockless access to the shared
clock->count variable lead to race conditions?

The fsi_priv struct and its clock->count member are shared between playback
and capture substreams on the same DAI. ALSA PCM trigger callbacks are
serialized per-substream, not per-DAI, allowing playback and capture triggers
to execute concurrently.

fsi_clk_enable() checks if count is zero and increments it without any
locking. If both streams start concurrently, both CPUs can observe count == 0,
execute the clk_enable logic simultaneously, and increment count to 2, but
clk_enable() was called twice for each clock.

When stopping, fsi_clk_disable() uses `if (1 == clock->count--)`. The first
stop drops count from 2 to 1 and does not call clk_disable(). The second stop
drops count from 1 to 0 and calls clk_disable() once. This leaves the clock
enable counts permanently unbalanced by +1.

> @@ -1580,7 +1628,7 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream,
>  
>  	fsi_clk_invalid(fsi);

[Severity: High]
This is a pre-existing issue, but does calling fsi_clk_invalid() here corrupt
the state of concurrent active streams?

Because fsi_priv is shared between playback and capture, opening a new stream
while another is actively running overwrites the operating stream's clock rate
to 0.

When the active stream is eventually stopped, fsi_hw_shutdown() calls
fsi_clk_disable(). Because fsi_clk_is_valid() relies on the now zeroed
clock->rate, it evaluates to false.

This causes fsi_clk_disable() to return -EINVAL immediately without
decrementing clock->count or disabling the hardware clocks, resulting in a
permanent clock enable leak.

>  
> -	return 0;
> +	return fsi_clk_prepare(fsi);
>  }
>  
>  static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
> @@ -1588,6 +1636,7 @@ static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
>  {
>  	struct fsi_priv *fsi = fsi_get_priv(substream);
>  
> +	fsi_clk_unprepare(fsi);

[Severity: High]
Could unpreparing the clock here risk a system hang in the interrupt handler?

If an in-flight or spurious IRQ fires after streams are shut down, the
fsi_interrupt() handler executes. If the SPU clock is disabled, the handler
immediately accesses hardware registers before verifying if any streams are
active:

    u32 int_st = fsi_irq_get_status(master);

    /* clear irq status */
    fsi_master_mask_set(master, SOFT_RST, IR, 0);
    fsi_master_mask_set(master, SOFT_RST, IR, IR);

Accessing INT_ST and SOFT_RST registers with the SPU clock gated will cause
the system to hang.

>  	fsi_clk_invalid(fsi);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609013107.5995-1-phucduc.bui@gmail.com?part=10

Re: [PATCH v5 05/11] ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The newly added `fsi_stream_is_working()` checks introduce a Time-of-Check to Time-of-Use (TOCTOU) race condition with clock shutdown.

Pre-existing issues:
- [Critical] A pre-existing TOCTOU race condition in PIO transfers causes a NULL pointer dereference because `fsi_stream_is_working()` releases the lock before `io->substream` is accessed.
- [Critical] The DMA transfer handler lacks a stream active check, leading to a fatal NULL pointer dereference if triggered by a shared port interrupt when the stream is stopped.
- [High] The fix fails to add stream activity checks to the PIO data transfer paths called earlier in the same IRQ handler, leaving an active path that still unconditionally triggers system hangs.
--

commit 1b9901efffc6b2d06494ae90af04665e436adbb6
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

This commit attempts to fix system hangs caused by in-flight IRQs accessing
hardware registers after the SPU clock is disabled. It introduces checks
using fsi_stream_is_working() to return early from IRQ handlers if no
streams are active.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -470,6 +470,10 @@ static int fsi_get_current_fifo_samples(struct fsi_priv *fsi,
>  
>  static void fsi_count_fifo_err(struct fsi_priv *fsi)
>  {
> +	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
> +	    !fsi_stream_is_working(fsi, &fsi->capture))
> +		return;
> +

[Severity: High]
Does this introduce a Time-of-Check to Time-of-Use race condition?

fsi_stream_is_working() acquires and immediately releases master->lock.
If snd_pcm_stop executes concurrently on another CPU right after this check
returns true, it will call fsi_stream_quit() and fsi_hw_shutdown(), setting
substream = NULL and disabling the hardware clocks.

Would the interrupted CPU then proceed to access hardware registers
(like fsi_reg_read(fsi, DOFF_ST) below) with the clocks disabled, resulting
in the exact system hang this patch was intended to fix?

[Severity: High]
This is a pre-existing issue, but the patch missed it while attempting
to fix the exact same issue. In fsi_interrupt(), before
fsi_count_fifo_err() is reached, the driver calls fsi_stream_transfer()
which routes to fsi_pio_push() or fsi_pio_pop().

Both functions immediately calculate sample space by calling
fsi_get_current_fifo_samples(), which unconditionally performs bare
register reads before any stream activity check is performed:

fsi_get_current_fifo_samples() {
        ...
        status = is_play ?
                fsi_reg_read(fsi, DOFF_ST) :
                fsi_reg_read(fsi, DIFF_ST);
        ...
}

Will an in-flight IRQ therefore access disabled hardware registers during
the PIO transfer phase, bypassing the intended mitigations?

[Severity: Critical]
This is a pre-existing issue, but does a race condition in PIO transfers
cause a NULL pointer dereference because fsi_stream_is_working() releases
the lock before io->substream is accessed?

fsi_pio_transfer() calls fsi_stream_is_working() to ensure the stream is
active. fsi_stream_is_working() safely verifies io->substream under
master->lock but releases the lock immediately.

fsi_pio_transfer() then proceeds to call fsi_pio_get_area() locklessly,
which unconditionally dereferences io->substream->runtime:

fsi_pio_transfer() {
        if (!fsi_stream_is_working(fsi, io))
                return -EINVAL;

        buf = fsi_pio_get_area(fsi, io);
        ...
}

fsi_pio_get_area() {
        struct snd_pcm_runtime *runtime = io->substream->runtime;
        ...
}

If an ALSA SNDRV_PCM_TRIGGER_STOP command executes fsi_stream_quit()
concurrently on another CPU, setting io->substream = NULL, will this
sequence interleave precisely to cause a NULL pointer dereference?

[Severity: Critical]
This is a pre-existing issue, but does the DMA transfer handler lack a
stream active check, leading to a NULL pointer dereference?

When an FSI port is configured with DMA for playback and PIO for capture,
they share the same hardware interrupt line and fsi_interrupt handler.
If the playback stream is stopped, its hardware FIFO remains empty,
continuously asserting the playback data request bit in the shared
int_st register.

The fsi_interrupt handler reads the raw int_st register without applying
software masks and unconditionally routes to fsi_stream_transfer():

fsi_interrupt() {
        ...
        if (int_st & AB_IO(1, AO_SHIFT))
                fsi_stream_transfer(&master->fsia.playback);
        ...
}

The DMA handler fsi_dma_transfer() is invoked without verifying if the
stream is actively working. It immediately calls fsi_get_dai(io->substream).
Since the playback stream is stopped, io->substream is NULL.

Does this result in an immediate NULL pointer dereference in
asoc_substream_to_rtd()?

>  	u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
>  	u32 istatus = fsi_reg_read(fsi, DIFF_ST);
>  
> @@ -681,6 +685,10 @@ static void fsi_irq_clear_status(struct fsi_priv *fsi)
>  	u32 data = 0;
>  	struct fsi_master *master = fsi_get_master(fsi);
>  
> +	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
> +	    !fsi_stream_is_working(fsi, &fsi->capture))
> +		return;
> +

[Severity: High]
Does this also introduce a Time-of-Check to Time-of-Use race condition?

The stream status is checked and the lock is dropped before clearing the
interrupt status locklessly via fsi_core_mask_set(). Could a concurrent
stream stop and clock shutdown occur between the check and the hardware
access, causing a regression?

>  	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->playback));
>  	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->capture));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609013107.5995-1-phucduc.bui@gmail.com?part=5

Re: [PATCH v5 01/11] ASoC: dt-bindings: renesas,fsi: add support multiple clocks

[Krzysztof Kozlowski]

On Tue, Jun 09, 2026 at 08:30:57AM +0700, phucduc.bui@gmail.com wrote:
> +  clock-names:
> +    minItems: 1
> +    maxItems: 8
> +    items:
> +      enum:
> +        - fck  # Main FSI module clock
> +        - spu  # optional SPU bus/bridge clock
> +        - icka # optional CPG DIV6 functional clocks for FSI port A
> +        - ickb # optional CPG DIV6 functional clocks for FSI port B
> +        - diva # optional FSI dividers for port A used for audio clock generation
> +        - divb # optional FSI dividers for port B used for audio clock generation
> +        - xcka # optional External clock inputs for FSI port A provided by the board
> +        - xckb # optional External clock inputs for FSI port B provided by the board
>  
>    power-domains:
>      maxItems: 1
> @@ -69,6 +91,31 @@ required:
>  
>  unevaluatedProperties: false
>  
> +allOf:
> +  - $ref: dai-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,fsi2-r8a7740
> +    then:
> +      required:
> +        - clock-names
> +
> +      properties:
> +        clock-names:
> +          minItems: 2
> +          uniqueItems: true

You don't need this, it's by default.

> +          items:
> +            - const: fck
> +            - const: spu
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]

Are all optional in the board design? I cannot find answers to that in
commit msg, but it is important - you need to explain WHY you are doing
this and WHY such different way.


Best regards,
Krzysztof

Re: [PATCH v5 00/11] ASoC: renesas: fsi: Fix system hang by adding SPU clock

[Kuninori Morimoto]

Hi

Thank you for the patches

> The FSI on r8a7740 requires the SPU clock to be enabled before accessing
> its internal registers. Without it, register accesses may hang the system
> even when the FSI functional clock is enabled.
> 
> Previously, the SPU clock remained enabled because it was left running by
> the bootloader. After adding the SPU clock to the device tree, it is
> automatically disabled once system initialization completes.
> 
> This series adds the missing clocks and aligns their names with those used
> by the driver.
> 
> Following feedback from Morimoto-san, the driver is also refactored to
> improve stability. Clock initialization is moved from the runtime path to
> the probe function to simplify the flow and avoid redundant setup.
> Additionally, the shutdown sequence is reordered to ensure the stream is
> stopped before the hardware is shut down.
> 
> The driver currently uses clk_enable()/clk_disable() without matching
> clk_prepare()/clk_unprepare() handling. This series adds the missing
> prepare/unprepare operations and moves them into startup/shutdown paths,
> since clk_prepare() may sleep and therefore must not be called from 
> atomic contexts.
> 
> The series also fixes a race where in-flight IRQ handlers may continue
> accessing registers after the SPU clock has been disabled during shutdown.

For all patches (except DT)

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v5 01/11] ASoC: dt-bindings: renesas,fsi: add support multiple clocks

[Bui Duc Phuc]

Hi Krzysztof,

Thank you for your reviews.

> > +      properties:
> > +        clock-names:
> > +          minItems: 2
> > +          uniqueItems: true
>
> You don't need this, it's by default.
>

Could you clarify which part you are referring to?
Are you referring to the "uniqueItems: true" property or another
constraint in this block?


> > +          items:
> > +            - const: fck
> > +            - const: spu
> > +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> > +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> > +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> > +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> > +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> > +            - enum: [icka, ickb, diva, divb, xcka, xckb]
>
> Are all optional in the board design? I cannot find answers to that in
> commit msg, but it is important - you need to explain WHY you are doing
> this and WHY such different way.
>

For r8a7740, "fck" and "spu" are required. The SPU clock must be enabled
to access the FSI registers because the FSI block is located behind the
SPU bus.
The remaining clocks (icka/b, diva/b and xcka/b) are not always required.
Their presence depends on the clock topology used by each FSI port.
In the previous discussion I described the supported clock configurations:
https://lore.kernel.org/all/CAABR9nEhOTz1-0NmCMTbz=-+782Pto0yovSQhBXrXqhLwMg80Q@mail.gmail.com/
The hardware supports several valid configurations, for example:
 - FSIA/FSIB slave: only fck and spu are needed.
 - FSI master using an internal clock: ickx and divx are used.
 - FSI master using an external clock: ickx and xckx are used.

Therefore, while fck and spu are mandatory on r8a7740, the other clocks
depend on the selected master/slave configuration and clock source, so
not all of them are expected to be present in every DT.

Best regards,
Phuc

Re: [PATCH v5 01/11] ASoC: dt-bindings: renesas,fsi: add support multiple clocks

[Krzysztof Kozlowski]

On 09/06/2026 10:11, Bui Duc Phuc wrote:
> Hi Krzysztof,
> 
> Thank you for your reviews.
> 
>>> +      properties:
>>> +        clock-names:
>>> +          minItems: 2
>>> +          uniqueItems: true
>>
>> You don't need this, it's by default.
>>
> 
> Could you clarify which part you are referring to?
> Are you referring to the "uniqueItems: true" property or another
> constraint in this block?

The uniqueItems should not be needed.

> 
> 
>>> +          items:
>>> +            - const: fck
>>> +            - const: spu
>>> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
>>> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
>>> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
>>> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
>>> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
>>> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
>>
>> Are all optional in the board design? I cannot find answers to that in
>> commit msg, but it is important - you need to explain WHY you are doing
>> this and WHY such different way.
>>
> 
> For r8a7740, "fck" and "spu" are required. The SPU clock must be enabled
> to access the FSI registers because the FSI block is located behind the
> SPU bus.
> The remaining clocks (icka/b, diva/b and xcka/b) are not always required.
> Their presence depends on the clock topology used by each FSI port.
> In the previous discussion I described the supported clock configurations:
> https://lore.kernel.org/all/CAABR9nEhOTz1-0NmCMTbz=-+782Pto0yovSQhBXrXqhLwMg80Q@mail.gmail.com/

This commit must stand on its own, thus here you must explain that.

> The hardware supports several valid configurations, for example:
>  - FSIA/FSIB slave: only fck and spu are needed.
>  - FSI master using an internal clock: ickx and divx are used.
>  - FSI master using an external clock: ickx and xckx are used.
> 
> Therefore, while fck and spu are mandatory on r8a7740, the other clocks
> depend on the selected master/slave configuration and clock source, so
> not all of them are expected to be present in every DT.

Explain that in the commit msg and be explicit that all these further
clocks are optional.

Best regards,
Krzysztof

Re: [PATCH v5 01/11] ASoC: dt-bindings: renesas,fsi: add support multiple clocks

[Bui Duc Phuc]

Hi Krzysztof,

Thank you for clarifying.

> >>> +          uniqueItems: true
> >>
> >> You don't need this, it's by default.
> >>
> >
> > Could you clarify which part you are referring to?
> > Are you referring to the "uniqueItems: true" property or another
> > constraint in this block?
>
> The uniqueItems should not be needed.
>

From my testing, if "uniqueItems: true" is removed, the schema still
accepts duplicated clock names, for example:
clock-names = "fck", "spu", "icka", "icka", "xcka", "xckb";
Since duplicate clock names are not valid, I believe
"uniqueItems: true" is still needed here to reject such cases.


> > The hardware supports several valid configurations, for example:
> >  - FSIA/FSIB slave: only fck and spu are needed.
> >  - FSI master using an internal clock: ickx and divx are used.
> >  - FSI master using an external clock: ickx and xckx are used.
> >
> > Therefore, while fck and spu are mandatory on r8a7740, the other clocks
> > depend on the selected master/slave configuration and clock source, so
> > not all of them are expected to be present in every DT.
>
> Explain that in the commit msg and be explicit that all these further
> clocks are optional.
>

Understood. I will update the commit message to clarify that
icka/b, diva/b and xcka/b are optional and depend on the selected
clock topology.

Best regards,
Phuc

Re: [PATCH v5 01/11] ASoC: dt-bindings: renesas,fsi: add support multiple clocks

[Krzysztof Kozlowski]

On 09/06/2026 10:42, Bui Duc Phuc wrote:
> Hi Krzysztof,
> 
> Thank you for clarifying.
> 
>>>>> +          uniqueItems: true
>>>>
>>>> You don't need this, it's by default.
>>>>
>>>
>>> Could you clarify which part you are referring to?
>>> Are you referring to the "uniqueItems: true" property or another
>>> constraint in this block?
>>
>> The uniqueItems should not be needed.
>>
> 
> From my testing, if "uniqueItems: true" is removed, the schema still
> accepts duplicated clock names, for example:
> clock-names = "fck", "spu", "icka", "icka", "xcka", "xckb";
> Since duplicate clock names are not valid, I believe
> "uniqueItems: true" is still needed here to reject such cases.

Odd. clock-names are string-array which has uniqueItems by default.

That's the first need of usage it, so I think you should skip it even if
it does not work correctly - the true fix should be in dtschema.

Best regards,
Krzysztof

Re: [PATCH v5 01/11] ASoC: dt-bindings: renesas,fsi: add support multiple clocks

[Bui Duc Phuc]

Hi Krzysztof,

Thank you for the clarification.

>
> Odd. clock-names are string-array which has uniqueItems by default.
>
> That's the first need of usage it, so I think you should skip it even if
> it does not work correctly - the true fix should be in dtschema.

Understood. I will remove "uniqueItems: true" in the next version.

While testing, I also noticed that the schema accepts cases where the
number of entries in "clock-names" does not match the number of entries
in "clocks". For example:

clocks = <&clk1>, <&clk2>;
clock-names = "fck", "spu", "icka";

This passes dtbs_check on my setup.

Is this also expected to be handled by dtschema, or would an explicit
constraint be needed in the binding?

Best regards,
Phuc

Re: [PATCH v5 00/11] ASoC: renesas: fsi: Fix system hang by adding SPU clock

[Bui Duc Phuc]

Hi Morimoto-san,

> For all patches (except DT)
>
> Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>

Thank you very much for your review and Acked-by.
I will send a v6 shortly to update the commit message for the DT binding
patch based on Krzysztof's feedback. I also have a few small updates for
patches 8 and 11 to address comments from the Sashiko review.
Thank you again for your help and guidance.

Best regards,
Phuc