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

[PATCH v4 00/10] 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 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.

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 (10):
  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 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                       | 272 +++++++++++++-----
 3 files changed, 260 insertions(+), 85 deletions(-)

-- 
2.43.0

[PATCH v4 01/10] 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

Re: [PATCH v4 01/10] 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] Contradiction between the strict positional order defined in the `clocks` index descriptions and the flexible ordering permitted by `clock-names`.
--

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

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

This commit updates the Renesas FSI binding to support multiple clocks to
properly describe the hardware clock tree. It includes the SPU bus/bridge
clock, CPG DIV6 functional clocks, FSI dividers, and external clock inputs.

> diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> --- 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

[ ... ]

> @@ -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]
Does this flexible enum ordering conflict with the strict positional sequence
defined in the clocks property description above?

The clocks property specifies an items array with 8 distinct descriptions
that enforces a strict positional mapping (e.g., the fourth slot is the
functional clock for port B). However, this clock-names constraint permits
these optional clocks to be specified in an arbitrary order.

If a board omits an optional clock but provides subsequent ones, will
dt-schema incorrectly map the provided clock to the wrong description since
DT arrays do not support skipping elements?

>  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]
Can this example be updated to follow the positional sequence defined in the
clocks property?

The clocks definition reserves the fourth position (index 3) for the CPG DIV6
functional clock for FSI port B ("ickb"), but the example places the external
clock input "xcka" at that position instead.

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

[PATCH v4 02/10] 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 v4 03/10] 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 v4:
 - update commit messages 

 sound/soc/renesas/fsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH v4 03/10] ASoC: renesas: fsi: Fix trigger stop ordering

[sashiko-bot]

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

Pre-existing issues:
- [High] Asynchronous background task (DMA completion or hardirq) races with stream teardown, causing a lockless access and NULL pointer dereference.
- [High] The driver calls clk_set_rate() and clk_set_parent() from the ALSA trigger callback, which runs in atomic context with spinlocks held.
--

commit d41629e308808a974831ffabbba6ec2a77dd96e8
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..94ab2e4908108 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1586,9 +1586,9 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  			ret = fsi_stream_transfer(io);
>  		break;

[Severity: High]
This is a pre-existing issue, but does fsi_dai_trigger() call sleepable
clock functions while holding an atomic spinlock?

The ALSA core invokes the fsi_dai_trigger() callback in atomic context with
the PCM stream spinlock held.

During SNDRV_PCM_TRIGGER_START (handled earlier in this function), it calls
fsi_hw_startup() which enables the clock via fsi_clk_enable(). This
eventually invokes the clock's set_rate function (like fsi_clk_set_rate_cpg()
or fsi_clk_set_rate_external()) and calls clk_set_rate() and/or
clk_set_parent().

Under the Common Clock Framework, clk_set_rate() and clk_set_parent() acquire
the global prepare_lock mutex, which can sleep.

Can this trigger a "scheduling while atomic" bug or deadlock?

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

[Severity: High]
This is also a pre-existing issue, but does this teardown sequence race with
asynchronous DMA completion?

When fsi_stream_stop() invokes dmaengine_terminate_all(), it is an
asynchronous operation and doesn't wait for pending callbacks like
fsi_dma_complete() to finish.

Immediately after this, fsi_stream_quit() executes and sets
io->substream = NULL.

If fsi_dma_complete() runs concurrently on another CPU, it could execute
fsi_pointer_update() which directly dereferences io->substream->runtime
without a NULL check:

fsi_pointer_update() {
    struct snd_pcm_substream *substream = io->substream;
    struct snd_pcm_runtime *runtime = substream->runtime; // NULL dereference
    ...
}

A similar race exists in the PIO path where fsi_pio_transfer() checks
fsi_stream_is_working() under lock, but drops it before accessing
io->substream in fsi_pio_get_area().

Could this lead to a kernel panic?

>  		break;
>  	}

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

[PATCH v4 04/10] 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.

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 94ab2e490810..429c3c9b6ede 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

Re: [PATCH v4 04/10] 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] Time-of-check to time-of-use (TOCTOU) race condition in `fsi_stream_is_working()` leading to a potential NULL pointer dereference or disabled-clock hardware access.
- [High] Asynchronous DMA callbacks (`fsi_dma_complete()`) can execute after hardware clocks are disabled or the stream is stopped, leading to a system hang or NULL pointer dereference.
--

commit 4ad703aa1dde6474675b4563473c933ef82894b5
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().

> This prepares for a subsequent patch that needs to check stream status
> when handling in-flight IRQ handlers. No functional changwqes intended.

[Severity: Low]
This isn't a bug, but there is a typo (changwqes) in the commit message.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 94ab2e4908108..429c3c9b6ede4 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 is a pre-existing flaw that
invalidates logic added later in the series.

Can this Time-of-Check to Time-of-Use (TOCTOU) race condition lead to a
NULL pointer dereference?

fsi_stream_is_working() acquires master->lock, checks if io->substream is
valid, and then releases the lock. In callers like fsi_pio_transfer(), this
boolean result allows the code to proceed and unconditionally dereference
io->substream via fsi_pio_get_area():

    if (!fsi_stream_is_working(fsi, io))
        return -EINVAL;
    buf = fsi_pio_get_area(fsi, io);

If fsi_stream_quit() runs concurrently on another CPU during
SNDRV_PCM_TRIGGER_STOP, it sets io->substream = NULL under the lock. Since
the FSI IRQ handler does not hold any stream locks during this path, could
an in-flight fsi_pio_transfer() pass the working check, get preempted while
another CPU clears the pointer, and then dereference NULL?

Additionally, does this invalidate the fix added later in the series for
register access from in-flight IRQs? Since fsi_stream_quit() is called after
fsi_hw_shutdown() in the trigger sequence, io->substream is still non-NULL
when the clocks are disabled. Will this cause fsi_stream_is_working() to
erroneously return true and allow fatal clock-disabled register reads?


[Severity: High]
This is also a pre-existing issue, but involves the broader shutdown sequence
that this series modifies.

Can asynchronous DMA callbacks execute after hardware clocks are disabled or
the stream is stopped, leading to a system hang or NULL pointer dereference?

During SNDRV_PCM_TRIGGER_STOP, fsi_dma_push_start_stop() uses
dmaengine_terminate_all(), which does not synchronously wait for in-flight
DMA callbacks to finish. Immediately after, the driver calls
fsi_hw_shutdown() (disabling the SPU clock) and fsi_stream_quit() (clearing
io->substream).

If a pending fsi_dma_complete() callback executes after fsi_hw_shutdown(),
it unconditionally calls fsi_pointer_update(), which dereferences
io->substream:

    fsi_pointer_update(io, io->period_samples);
    fsi_count_fifo_err(fsi);

If fsi_stream_quit() has already executed, does this cause a NULL pointer
panic?

If the callback executes between shutdown and quit, it continues to
fsi_count_fifo_err() which reads DOFF_ST and DIFF_ST registers. Because
the SPU clock has already been disabled by fsi_hw_shutdown(), will accessing
these registers cause a synchronous external abort and hang the system?

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

[PATCH v4 05/10] 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 429c3c9b6ede..0f350bddeb1d 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

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

[sashiko-bot]

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

New issues:
- [High] The patch fails to completely fix the hardware hang due to improper ordering with clock shutdown and introduces a Time-Of-Check to Time-Of-Use (TOCTOU) race condition.

Pre-existing issues:
- [High] Potential NULL pointer dereference in `fsi_pointer_update()` if a DMA completion callback runs after stream shutdown.
- [High] The driver releases DMA channels in `remove` before unregistering the ALSA component, creating a Use-After-Free race condition.
--

commit c264d5f1cdc59e54f0e7191a60f9c8c00661657c
Author: bui duc phuc <phucduc.bui@gmail.com>
ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

This commit attempts to prevent system hangs by adding checks in the
in-flight IRQ handlers to skip hardware access if the stream is no
longer working. It relies on fsi_stream_is_working to detect if the
stream is active before reading or writing registers.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 429c3c9b6ede4..0f350bddeb1d5 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;
> +

[Severity: High]
Does this introduce a time-of-check to time-of-use race condition?

Since fsi_stream_is_working acquires and drops the master lock, an
interrupt can check the status and drop the lock. Before the handler
reads the hardware registers below, another thread could call
fsi_hw_shutdown, disabling the clock and causing a hang.

Additionally, when stopping the stream in fsi_dai_trigger, the hardware
shutdown is called before the substream is cleared:

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

Because the clock is disabled before io->substream is cleared, an
in-flight IRQ will still pass the fsi_stream_is_working check
and access the registers. 

The same ordering issue occurs in __fsi_suspend, where io->substream is
never cleared at all.

>  	u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
>  	u32 istatus = fsi_reg_read(fsi, DIFF_ST);

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a potential NULL pointer
dereference in fsi_pointer_update during stream shutdown?

When stopping a DMA stream, fsi_dma_push_start_stop uses
dmaengine_terminate_all, which terminates asynchronously:

sound/soc/renesas/fsi.c:fsi_dma_push_start_stop() {
    ...
    dmaengine_terminate_all(io->chan);
    ...
}

Immediately after, fsi_stream_quit sets io->substream to NULL. If the
DMA callback fsi_dma_complete fires or is still running concurrently
after io->substream is cleared, it will call fsi_pointer_update which
dereferences the pointer without checking:

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

Could this cause a kernel panic if the callback executes after cleanup?


[Severity: High]
This is a pre-existing issue, but does fsi_remove create a use-after-free
race condition by releasing DMA channels before ALSA unregistration?

In fsi_probe, the ALSA component is registered via devres using
devm_snd_soc_register_component, meaning it unregisters automatically
after fsi_remove completes. However, fsi_remove manually destroys
the stream resources:

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

Since the ALSA device is still accessible to userspace while the DMA
channels are being freed, an application could trigger audio playback.
If playback is started concurrently, it calls fsi_dma_transfer which
dereferences the freed channel:

sound/soc/renesas/fsi.c:fsi_dma_transfer() {
    ...
    desc = dmaengine_prep_dma_cyclic(io->chan, ...);
    ...
    dma_async_issue_pending(io->chan);
}

Could this lead to accessing a freed DMA channel pointer?

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

[PATCH v4 06/10] 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 0f350bddeb1d..43e6772c06d5 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

Re: [PATCH v4 06/10] ASoC: renesas: fsi: Move fsi_clk_init()

[sashiko-bot]

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

Pre-existing issues:
- [High] The newly introduced `fsi_stream_is_working()` function creates a Time-of-Check to Time-of-Use (TOCTOU) race condition because it drops the lock before returning.
- [High] The patchset attempts to prevent system hangs from in-flight IRQs when clocks are disabled, but fails because the interrupt handler still unconditionally accesses hardware registers before any stream status checks.
--

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

ASoC: renesas: fsi: Move fsi_clk_init()

This patch moves the fsi_clk_init() function after set_rate() functions to
prepare for subsequent refactoring.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 0f350bddeb1d5..43e6772c06d5d 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c

[ ... ]

While reviewing this patch, I noticed a couple of pre-existing issues in
this file.

sound/soc/renesas/fsi.c:fsi_stream_is_working()
{
	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 this function introduce a
Time-of-Check to Time-of-Use (TOCTOU) race condition?

The fsi_stream_is_working() function checks if io->substream is active
while holding master->lock, but immediately drops the lock using a scoped
guard before returning.

In the interrupt handler, fsi_count_fifo_err() and fsi_irq_clear_status()
call this function and proceed to read hardware registers without holding any
locks if it returns true:

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);
	...
}

If a concurrent SNDRV_PCM_TRIGGER_STOP on another CPU executes
fsi_hw_shutdown() (which disables the SPU clock) right after
fsi_stream_is_working() returns true but before the register accesses,
can this lead to a fatal system hang when the interrupt handler accesses
registers with the disabled clock?

[ ... ]

sound/soc/renesas/fsi.c:fsi_interrupt()
{
	struct fsi_master *master = data;
	u32 int_st = fsi_irq_get_status(master);

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

[Severity: High]
This isn't a bug introduced by this patch, but does the interrupt handler
unconditionally access hardware registers before any stream status checks?

The commit message for a previous patch claims that the IRQ handler will
return immediately if no streams are active. However, fsi_interrupt()
unconditionally calls fsi_irq_get_status() and fsi_master_mask_set() at
the very beginning of the function, before any stream checks are performed.

These functions read and write the INT_ST and SOFT_RST registers, which
sit behind the SPU bus. If an in-flight IRQ executes after the SPU clock has
been disabled, will these initial register accesses immediately hang the
system, completely bypassing the downstream fsi_stream_is_working() checks?

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

[PATCH v4 07/10] 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 resources. Use
devm_clk_get_optional() instead of devm_clk_get() when acquiring these
clocks and switch to dev_err_probe() for error reporting.

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

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 43e6772c06d5..a2d7d17dd2bb 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -990,15 +990,13 @@ static int fsi_clk_init(struct device *dev,
 
 	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(dev, is_porta ? "xcka" : "xckb");
-		if (IS_ERR(clock->xck)) {
-			dev_err(dev, "can't get xck clock\n");
-			return -EINVAL;
-		}
+		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
+		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;
@@ -1007,11 +1005,9 @@ static int fsi_clk_init(struct device *dev,
 
 	/* 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;
-		}
+		clock->ick = devm_clk_get_optional(dev,  is_porta ? "icka" : "ickb");
+		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;
@@ -1020,11 +1016,9 @@ static int fsi_clk_init(struct device *dev,
 
 	/* 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;
-		}
+		clock->div = devm_clk_get_optional(dev,  is_porta ? "diva" : "divb");
+		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;
-- 
2.43.0

[PATCH v4 08/10] ASoC: renesas: fsi: refactor clock initialization

[phucduc.bui]

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

Move fsi_clk_init() from set_fmt() to probe.
This moves clock resource lookup from fsi_dai_set_fmt() to the probe
path. The set_rate() callbacks validate that the required clock
resources are available before they are used for hardware
configuration.

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

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 | 52 +++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index a2d7d17dd2bb..3f303e15e835 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;
 };
@@ -862,6 +863,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 +904,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,28 +981,38 @@ 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;
+	struct fsi_master *master = fsi->master;
 	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 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");
@@ -1666,15 +1687,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);
@@ -1972,6 +1984,9 @@ static int fsi_probe(struct platform_device *pdev)
 	fsi->master	= master;
 	fsi_port_info_init(fsi, &info.port_a);
 	fsi_handler_init(fsi, &info.port_a);
+	ret = fsi_clk_init(&pdev->dev, fsi);
+	if (ret)
+		return ret;
 	ret = fsi_stream_probe(fsi, &pdev->dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "FSIA stream probe failed\n");
@@ -1985,6 +2000,9 @@ static int fsi_probe(struct platform_device *pdev)
 	fsi->master	= master;
 	fsi_port_info_init(fsi, &info.port_b);
 	fsi_handler_init(fsi, &info.port_b);
+	ret = fsi_clk_init(&pdev->dev, fsi);
+	if (ret)
+		return ret;
 	ret = fsi_stream_probe(fsi, &pdev->dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "FSIB stream probe failed\n");
-- 
2.43.0

Re: [PATCH v4 08/10] 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] Missing cleanup for previously allocated FSIA DMA channels if FSIB clock initialization fails in probe.

Pre-existing issues:
- [High] The DAI trigger callback executes sleeping clock operations while holding a PCM spinlock.
--

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

ASoC: renesas: fsi: refactor clock initialization

This commit moves the fsi_clk_init() function call from fsi_dai_set_fmt()
into the fsi_probe() path. It aims to ensure that clock resources are
validated early during the driver probe rather than deferring the lookup
to hardware configuration.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index a2d7d17dd2bb0..3f303e15e8351 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c

[ ... ]

> @@ -898,6 +904,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;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this clock initialization
path ultimately lead to sleeping while atomic?

The ASoC framework invokes fsi_dai_trigger() in an atomic context with the
PCM stream spinlock held and IRQs disabled. For the start command, the call
chain executes:

fsi_dai_trigger()
  fsi_hw_startup()
    fsi_clk_enable()
      fsi_clk_set_rate_cpg()
        clk_set_rate()

clk_set_rate() attempts to acquire the global prepare_lock mutex in the common
clock framework, which will trigger a scheduling-while-atomic panic when
called from the atomic trigger path.

[ ... ]

> @@ -1985,6 +2000,9 @@ static int fsi_probe(struct platform_device *pdev)
>  	fsi->master	= master;
>  	fsi_port_info_init(fsi, &info.port_b);
>  	fsi_handler_init(fsi, &info.port_b);
> +	ret = fsi_clk_init(&pdev->dev, fsi);
> +	if (ret)
> +		return ret;
>  	ret = fsi_stream_probe(fsi, &pdev->dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "FSIB stream probe failed\n");

[Severity: High]
If this fails, does the direct return bypass the cleanup for FSIA?

Earlier in fsi_probe(), FSIA is probed and successfully allocates
non-devres managed DMA channels. If fsi_clk_init() subsequently fails for
FSIB (for example, returning -EPROBE_DEFER for an unavailable clock), the
error path directly returns instead of executing the cleanup routine.

Would this lead to a permanent leak of the DMA channels and other stream
resources allocated for FSIA? Might a goto statement targeting the exit_fsia
label be needed here so that fsi_stream_remove() can safely clean up?

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

[PATCH v4 09/10] 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 v4:
 - Move clock->count early return check to the beginning of 
   fsi_clk_[un]prepare() to simplify the code.

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

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 3f303e15e835..6537321242c0 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -294,6 +294,7 @@ struct fsi_master {
 	struct fsi_priv fsib;
 	struct clk *clk_spu;
 	const struct fsi_core *core;
+	int spu_count;
 	spinlock_t lock;
 };
 
@@ -730,6 +731,78 @@ 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;
+
+	if (clock->count != 0)
+		return 0;
+
+	if (!IS_ERR_OR_NULL(spu) && fsi->master->spu_count == 0) {
+		ret = clk_prepare(spu);
+		if (ret)
+			return ret;
+	}
+
+	if (!IS_ERR_OR_NULL(xck)) {
+		ret = clk_prepare(xck);
+		if (ret)
+			goto err_spu;
+	}
+
+	if (!IS_ERR_OR_NULL(ick)) {
+		ret = clk_prepare(ick);
+		if (ret)
+			goto err_xck;
+	}
+
+	if (!IS_ERR_OR_NULL(div)) {
+		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;
+
+	if (clock->count != 0)
+		return;
+
+	if (!IS_ERR_OR_NULL(div))
+		clk_unprepare(div);
+
+	if (!IS_ERR_OR_NULL(ick))
+		clk_unprepare(ick);
+
+	if (!IS_ERR_OR_NULL(xck))
+		clk_unprepare(xck);
+
+	if (!IS_ERR_OR_NULL(spu) && fsi->master->spu_count == 0)
+		clk_unprepare(spu);
+}
+
 static int fsi_clk_enable(struct device *dev,
 			  struct fsi_priv *fsi)
 {
@@ -1580,7 +1653,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 +1661,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);
 }
 
@@ -1975,6 +2049,7 @@ static int fsi_probe(struct platform_device *pdev)
 
 	/* master setting */
 	master->core		= core;
+	master->spu_count	= 0;
 	spin_lock_init(&master->lock);
 
 	/* FSI A setting */
-- 
2.43.0

Re: [PATCH v4 09/10] ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

[sashiko-bot]

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

New issues:
- [High] fsi_clk_prepare() and fsi_clk_unprepare() incorrectly use clock->count to skip operations, leading to leaked clock prepares and underflows.
- [High] The error rollback path in fsi_clk_prepare() unconditionally unprepares the shared SPU clock, and the conditional prepare relies on spu_count which tracks the incorrect state.

Pre-existing issues:
- [High] fsi_clk_enable() breaks full-duplex operation by returning -EINVAL for the second stream.
--

commit 4aab60e129c589df9d885da5f08b55e2f1002c6c
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.

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

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 3f303e15e8351..6537321242c04 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -730,6 +731,78 @@ static int fsi_clk_is_valid(struct fsi_priv *fsi)
>  
> +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;
> +
> +	if (clock->count != 0)
> +		return 0;

[Severity: High]
Since clock->count tracks the hardware enablement state (modified during ALSA
trigger START/STOP via fsi_clk_enable/disable), could using it here during
the ALSA startup/shutdown (open/close) phase lead to unbalanced clock
prepares?

If Stream A opens, clock->count is 0, and the clocks are prepared.
If Stream A then starts, clock->count becomes 1.

If Stream B opens while Stream A is running, clock->count is 1, so we return
early and skip clk_prepare().

When Stream A stops, clock->count becomes 0.
When Stream A closes, clock->count is 0, so the clocks are unprepared.
When Stream B closes, clock->count is 0, so the clocks are unprepared again,
potentially causing a CCF prepare count underflow.

> +
> +	if (!IS_ERR_OR_NULL(spu) && fsi->master->spu_count == 0) {
> +		ret = clk_prepare(spu);
> +		if (ret)
> +			return ret;
> +	}

[ ... ]

> +err_ick:
> +	clk_unprepare(ick);
> +err_xck:
> +	clk_unprepare(xck);
> +err_spu:
> +	clk_unprepare(spu);
> +
> +	return ret;
> +}

[Severity: High]
If fsi->master->spu_count > 0, we skip preparing the spu clock above.
However, if a subsequent clock (like xck) fails to prepare, we jump to
err_spu and unconditionally call clk_unprepare(spu).

Will this forcefully unprepare the shared spu clock that the first active
stream is currently relying on?

Also, since spu_count tracks START/STOP instead of OPEN/CLOSE, does it
suffer from the same unbalanced prepare/unprepare regression as clock->count?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while looking at clock->count
usage, I noticed fsi_clk_enable() appears to prevent full-duplex operation
by returning -EINVAL for the second stream:

sound/soc/renesas/fsi.c:fsi_clk_enable() {
        int ret = -EINVAL;

        if (!fsi_clk_is_valid(fsi))
                return ret;

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

        return ret;
}

If a second stream attempts to start while the first is active
(clock->count == 1), the function skips the block and returns the
initialized -EINVAL. Does this cause the ALSA trigger to fail and prevent
simultaneous playback and capture?

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

[PATCH v4 10/10] 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>
---
 sound/soc/renesas/fsi.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 6537321242c0..d7cc5a14a099 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -1585,6 +1585,19 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
 			  struct device *dev)
 {
 	u32 data = 0;
+	int ret;
+
+	/* enable spu clock */
+	if (fsi->master->clk_spu) {
+		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
+			if (fsi->master->spu_count == 0) {
+				ret = clk_enable(fsi->master->clk_spu);
+				if (ret < 0)
+					return ret;
+			}
+				fsi->master->spu_count++;
+		}
+	}
 
 	/* clock setting */
 	if (fsi_is_clk_master(fsi))
@@ -1642,6 +1655,12 @@ static int fsi_hw_shutdown(struct fsi_priv *fsi,
 	/* stop master clock */
 	if (fsi_is_clk_master(fsi))
 		return fsi_clk_disable(dev, fsi);
+	if (fsi->master->clk_spu) {
+		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
+			if (--fsi->master->spu_count == 0)
+				clk_disable(fsi->master->clk_spu);
+		}
+	}
 
 	return 0;
 }
-- 
2.43.0

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 724 bytes --]

On Fri, Jun 05, 2026 at 07:19:54PM +0700, phucduc.bui@gmail.com wrote:

> 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.

> +	/* enable spu clock */
> +	if (fsi->master->clk_spu) {
> +		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
> +			if (fsi->master->spu_count == 0) {
> +				ret = clk_enable(fsi->master->clk_spu);
> +				if (ret < 0)
> +					return ret;
> +			}
> +				fsi->master->spu_count++;

The indentation here seems wrong.  We're also using spu_count to
separately guard the clk_prepare() in fsi_clk_prepare() which seems
problematic, I'm having to think too hard about how this might be
robust.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The SPU clock is never disabled, and spu_count is leaked, when the FSI operates as a clock master due to an early return in fsi_hw_shutdown().
- [High] Missing rollback of SPU clock and spu_count on fsi_clk_enable() failure during fsi_hw_startup().
- [High] Overloading spu_count breaks the clock prepare/unprepare lifecycle logic, leading to clk_enable() on an unprepared clock.
- [Medium] Data race on fsi->master->spu_count in fsi_clk_prepare() and fsi_clk_unprepare().
--

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

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

This commit enables and disables the spu clock within fsi_hw_startup() and
fsi_hw_shutdown() to keep the clock active during hardware register accesses.
It tracks the clock state using fsi->master->spu_count.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 6537321242c04..d7cc5a14a0998 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1585,6 +1585,19 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>                            struct device *dev)
>  {
>          u32 data = 0;
> +       int ret;
> +
> +       /* enable spu clock */
> +       if (fsi->master->clk_spu) {
> +               scoped_guard(spinlock_irqsave, &fsi->master->lock) {
> +                       if (fsi->master->spu_count == 0) {
> +                               ret = clk_enable(fsi->master->clk_spu);
> +                               if (ret < 0)
> +                                       return ret;
> +                       }
> +                               fsi->master->spu_count++;

[Severity: High]
Does tracking active stream starts with spu_count interfere with the existing
clock prepare logic?

Before this patch, fsi_clk_prepare() and fsi_clk_unprepare() checked if
spu_count == 0 before calling clk_prepare() and clk_unprepare().

By modifying spu_count during trigger start and stop, a second stream opening
while the first is running might observe spu_count > 0 and skip clk_prepare().

Could this lead to calling clk_enable() on an unprepared clock?

[Severity: Medium]
Since spu_count is now dynamically modified here under the fsi->master->lock,
is it safe that fsi_clk_prepare() and fsi_clk_unprepare() read spu_count
without acquiring this lock?

Could alsa pcm open and close operations running concurrently with trigger
operations result in a data race?

> +               }
> +       }

[ ... ]

>          /* fifo init */
>          fsi_fifo_init(fsi, io, dev);
>  
>          /* start master clock */
>          if (fsi_is_clk_master(fsi))
>                  return fsi_clk_enable(dev, fsi);

[Severity: High]
If fsi_clk_enable() fails here, the function returns the error immediately.

Because the audio core does not execute a stop trigger rollback if the
start trigger fails, fsi_hw_shutdown() will not be called.

Does this regression leave the spu clock erroneously enabled and spu_count
incremented forever?

>  
>          return 0;
>  }
>  
>  static int fsi_hw_shutdown(struct fsi_priv *fsi,
>                             struct device *dev)
>  {
>          /* stop master clock */
>          if (fsi_is_clk_master(fsi))
>                  return fsi_clk_disable(dev, fsi);

[Severity: High]
If the fsi operates as a clock master, this early return bypasses the newly
added spu clock disable logic.

Does this regression permanently leak the spu_count and leave the spu clock
enabled whenever a clock master stream is stopped?

> +       if (fsi->master->clk_spu) {
> +               scoped_guard(spinlock_irqsave, &fsi->master->lock) {
> +                       if (--fsi->master->spu_count == 0)
> +                               clk_disable(fsi->master->clk_spu);
> +               }
> +       }
>  
>          return 0;
>  }

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