Re: [PATCH v1 5/5] arm64: dts: cix: Add the Sky1 AUDSS DMA-350 integration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] clk: cix: unclocked MMIO access in sky1_audss_irq_teardown() and IRQ callbacks
- [High] clk: cix: IRQ storm in sky1_audss_irq_handler() due to handle_untracked_irq()
- [High] clk: cix: invalid wait context in sky1_audss_irq_handler() via non-raw regmap
- [High] clk: cix: missing virq disposal before irq_domain_remove() in teardown
- [High] arm64: dts: cix: audss_dmac reset AUDSS_SW_RST lost across AUDSS CRU suspend
--

commit 5fc04062b961f79314208fe23d9fe7413f932103
Author: Jelly Jia <Jelly.Jia@cixtech.com>

arm64: dts: cix: Add the Sky1 AUDSS DMA-350 integration

This commit adds the audss-dma-bus and audss_dmac nodes for the Sky1 AUDSS
DMA-350 controller and wires its clocks, resets, and channel interrupts
through the audss_cru controller.

> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> index cd498ee7e9a76..6ca94f606463c 100644
> --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> @@ -588,17 +588,41 @@ mbox_pm2ap: mailbox@65a0080 {
>  			cix,mbox-dir = "rx";
>  		};
>  
> +		audss-dma-bus {
> +			compatible = "simple-bus";
> +			ranges;
> +			#address-cells = <2>;
> +			dma-ranges = <0x0 0x20000000 0x0 0x07010000 0x0 0x00100000>,
> +				     <0x0 0x30000000 0x0 0xc0000000 0x0 0x20000000>;
> +			#size-cells = <2>;
> +
> +			audss_dmac: dma-controller@7010000 {
> +				compatible = "arm,dma-350";
> +				reg = <0x0 0x07010000 0x0 0x10000>;
> +				clocks = <&audss_cru CLK_DMAC_AXI>;
> +				#dma-cells = <1>;
> +				dma-channel-mask = <0xff>;
> +				interrupt-parent = <&audss_cru>;
> +				interrupts = <0>, <1>, <2>, <3>,
> +					     <4>, <5>, <6>, <7>;
> +				resets = <&audss_cru AUDSS_DMAC_SW_RST>;

[Severity: High]
Will the AUDSS_DMAC_SW_RST reset state for audss_dmac survive a system
suspend and resume cycle of audss_cru?

In drivers/dma/arm-dma350.c, d350_probe() deasserts the reset once at probe
time and the driver defines no power management callbacks:

drivers/dma/arm-dma350.c:d350_probe() {
	reset = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
	if (IS_ERR(reset))
		return dev_err_probe(dev, PTR_ERR(reset),
				     "failed to deassert reset\n");
}

That call goes through sky1_audss_reset_deassert() -> sky1_audss_reset_set()
in drivers/reset/reset-sky1-audss.c to set BIT(15) in AUDSS_SW_RST (0x78).

However, when audss_cru suspends via sky1_audss_clk_runtime_suspend() in
drivers/clk/cix/clk-sky1-audss.c, it saves only the registers listed in
sky1_reg_save[] before asserting priv->rst_noc:

drivers/clk/cix/clk-sky1-audss.c:
static u32 sky1_reg_save[][2] = {
	{ INFO_HIFI0,  0 },
	{ INFO_CLK_GATE,  0 },
	{ INFO_CLK_DIV, 0 },
	{ INFO_CLK_MUX, 0 },
	{ INFO_AP_IRQC, 0 },
	{ INFO_MCLK, 0 },
};

Because AUDSS_SW_RST (0x78) is not included in sky1_reg_save[], asserting
priv->rst_noc and powering down SKY1_PD_AUDIO resets AUDSS_SW_RST, leaving
audss_dmac held in hardware reset after sky1_audss_clk_runtime_resume()
runs.

> +			};
> +		};
> +
>  		audss_cru: clock-controller@7110000 {
>  			compatible = "cix,sky1-audss-cru";
>  			reg = <0x0 0x07110000 0x0 0x10000>;
>  			#clock-cells = <1>;
> -			#reset-cells = <1>;
>  			clocks = <&scmi_clk CLK_TREE_AUDIO_CLK0>,
>  				 <&scmi_clk CLK_TREE_AUDIO_CLK2>,
>  				 <&scmi_clk CLK_TREE_AUDIO_CLK4>,
>  				 <&scmi_clk CLK_TREE_AUDIO_CLK5>;
>  			clock-names = "x8k", "x11k", "sys", "48m";
> +			#interrupt-cells = <1>;
> +			interrupt-controller;
> +			interrupts = <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>;

[Severity: High]
Can wiring audss_cru as an interrupt-controller trigger unclocked register
accesses to INFO_AP_IRQC while audss_cru is runtime-suspended and held in
reset?

In drivers/clk/cix/clk-sky1-audss.c, sky1_audss_clk_probe() initializes the
irqchip and then drops the runtime PM reference:

drivers/clk/cix/clk-sky1-audss.c:sky1_audss_clk_probe() {
	/* Tear the irqchip down before the runtime-PM cleanup disables clocks. */
	ret = sky1_audss_irq_init(pdev, priv);
	if (ret) {
		dev_err_probe(dev, ret, "failed to set up AUDSS DMA IRQs\n");
		goto fail_pm;
	}

	pm_runtime_put_sync(dev);
}

Whenever no consumer clock is prepared, sky1_audss_clk_runtime_suspend()
asserts priv->rst_noc and disables the parent clocks via
sky1_audss_clks_disable(priv):

drivers/clk/cix/clk-sky1-audss.c:sky1_audss_clk_runtime_suspend() {
	...
	reset_control_assert(priv->rst_noc);

	sky1_audss_clks_disable(priv);
}

During driver unbind in __device_release_driver(),
device_links_unbind_consumers() unbinds audss_dmac (unpreparing
CLK_DMAC_AXI), and pm_runtime_put_sync() suspends audss_cru before
device_unbind_cleanup() -> devres_release_all() runs
sky1_audss_irq_teardown():

drivers/clk/cix/clk-sky1-audss.c:sky1_audss_irq_teardown() {
	struct sky1_audss_clks_priv *priv = data;

	regmap_update_bits(priv->regmap_cru, INFO_AP_IRQC,
			   INFO_AP_IRQC_DMAC_MASK, 0);
	irq_set_chained_handler_and_data(priv->parent_irq, NULL, NULL);
	irq_domain_remove(priv->irq_domain);
}

In addition, sky1_audss_irq_init() does not call irq_domain_set_pm_device()
and leaves GIC_SPI 230 active while suspended, so sky1_audss_irq_teardown(),
sky1_audss_irq_mask(), sky1_audss_irq_unmask(), or sky1_audss_irq_handler()
can access INFO_AP_IRQC while priv->rst_noc is asserted and the parent
clocks are disabled.

[Severity: High]
Could routing the level-high GIC_SPI 230 interrupt through
sky1_audss_irq_handler() and handle_untracked_irq() cause an interrupt storm
or spurious interrupt replay?

In drivers/clk/cix/clk-sky1-audss.c, sky1_audss_irq_chip provides only
.irq_mask and .irq_unmask, and sky1_audss_irq_map() sets
handle_untracked_irq without IRQ_DISABLE_UNLAZY or IRQ_NOTHREAD:

drivers/clk/cix/clk-sky1-audss.c:
static struct irq_chip sky1_audss_irq_chip = {
	.name		= "sky1-audss-dmac",
	.irq_mask	= sky1_audss_irq_mask,
	.irq_unmask	= sky1_audss_irq_unmask,
	.flags		= IRQCHIP_SKIP_SET_WAKE,
};

static int sky1_audss_irq_map(struct irq_domain *d, unsigned int irq,
			      irq_hw_number_t hwirq)
{
	if (hwirq >= SKY1_AUDSS_DMAC_IRQS)
		return -EINVAL;

	irq_set_chip_data(irq, d->host_data);
	irq_set_chip_and_handler(irq, &sky1_audss_irq_chip,
				 handle_untracked_irq);
	irq_set_noprobe(irq);

	return 0;
}

When sky1_audss_irq_handler() runs, it reads the gate mask in INFO_AP_IRQC
and dispatches every unmasked channel bit:

drivers/clk/cix/clk-sky1-audss.c:sky1_audss_irq_handler() {
	if (!regmap_read(priv->regmap_cru, INFO_AP_IRQC, &val)) {
		enabled = val & INFO_AP_IRQC_DMAC_MASK;
		for_each_set_bit(hwirq, &enabled, SKY1_AUDSS_DMAC_IRQS)
			generic_handle_domain_irq(priv->irq_domain, hwirq);
	}
}

Because handle_untracked_irq() in kernel/irq/chip.c returns early when
!irq_can_handle(desc) without calling mask_irq(desc), and does not mask
IRQS_ONESHOT interrupts:

1. If disable_irq() or suspend_device_irqs() lazily disables a channel, its
   bit in INFO_AP_IRQC stays set. If that channel asserts its level-high
   interrupt, handle_untracked_irq() neither masks INFO_AP_IRQC nor runs
   d350_irq() to clear CH_STATUS, leaving GIC_SPI 230 asserted in an
   infinite hard-IRQ loop.
2. Under CONFIG_PREEMPT_RT or threadirqs, request_irq() in
   d350_alloc_chan_resources() is force-threaded with IRQF_ONESHOT, and
   handle_untracked_irq() leaves the interrupt unmasked while waking the
   IRQ thread, re-triggering GIC_SPI 230 continuously.
3. When any enabled channel fires, sky1_audss_irq_handler() calls
   generic_handle_domain_irq() for every lazily-disabled sibling channel
   still enabled in INFO_AP_IRQC, which sets IRQS_PENDING on idle channels
   and causes a spurious replay on enable_irq() or resume_device_irqs().

[Severity: High]
Does accessing priv->regmap_cru from sky1_audss_irq_mask(),
sky1_audss_irq_unmask(), and sky1_audss_irq_handler() trigger an invalid
wait context warning?

In drivers/clk/cix/clk-sky1-audss.c, sky1_audss_regmap_config does not set
.use_raw_spinlock = true:

drivers/clk/cix/clk-sky1-audss.c:
static const struct regmap_config sky1_audss_regmap_config = {
	.reg_bits = 32,
	.val_bits = 32,
	.reg_stride = 4,
};

As a result, __regmap_init() configures priv->regmap_cru to use a non-raw
spinlock_t (which becomes a sleeping lock on CONFIG_PREEMPT_RT). When
audss_dmac requests or handles its channel interrupts:

- sky1_audss_irq_mask() and sky1_audss_irq_unmask() call
  regmap_update_bits(priv->regmap_cru, INFO_AP_IRQC, ...) while the IRQ core
  holds desc->lock (a raw_spinlock_t).
- sky1_audss_irq_handler() runs as a chained handler in hard-IRQ context and
  calls regmap_read(priv->regmap_cru, INFO_AP_IRQC, &val).

Should sky1_audss_regmap_config set .use_raw_spinlock = true so that
priv->regmap_cru can be safely accessed from raw_spinlock_t and hard-IRQ
contexts?

[Severity: High]
Does sky1_audss_irq_teardown() leak the mapped virq descriptors and leave
dangling pointers when audss_cru is unbound?

When d350_probe() calls platform_get_irq() for the 8 audss_dmac channels,
of_irq_get() -> irq_create_of_mapping() populates priv->irq_domain's
revmap_tree and invokes sky1_audss_irq_map(), storing pointers to priv,
priv->irq_domain, and sky1_audss_irq_chip in each irq_desc.

However, sky1_audss_irq_teardown() calls irq_domain_remove() without first
disposing the mappings via irq_dispose_mapping():

drivers/clk/cix/clk-sky1-audss.c:sky1_audss_irq_teardown() {
	struct sky1_audss_clks_priv *priv = data;

	regmap_update_bits(priv->regmap_cru, INFO_AP_IRQC,
			   INFO_AP_IRQC_DMAC_MASK, 0);
	irq_set_chained_handler_and_data(priv->parent_irq, NULL, NULL);
	irq_domain_remove(priv->irq_domain);
}

In kernel/irq/irqdomain.c, irq_domain_remove() triggers
WARN_ON(!radix_tree_empty(&domain->revmap_tree)) and frees priv->irq_domain,
leaving the 8 allocated irq_desc descriptors in sparse_irqs with dangling
references to the freed domain, devm-freed priv data, and module rodata if
/proc/interrupts or /sys/kernel/irq/<virq>/chip_name is subsequently read.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023093045.3248630-1-Jelly.Jia@cixtech.com?part=5