Re: [PATCH v1 2/5] clk: cix: Handle Sky1 AUDSS DMA interrupt gates as an irqchip

[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: invalid wait context in sky1_audss_irq_handler() and mask callbacks
- [High] clk: cix: missing irq_dispose_mapping() in sky1_audss_irq_teardown()
- [High] clk: cix: level IRQ storm with handle_untracked_irq() on lazy IRQ disable
- [Low] clk: cix: missing select IRQ_DOMAIN for CLK_SKY1_AUDSS in Kconfig
--

commit d366d15da9922d1961ddd48a45f4b621f1a77320
Author: Jelly Jia <Jelly.Jia@cixtech.com>

clk: cix: Handle Sky1 AUDSS DMA interrupt gates as an irqchip

This commit adds an irqchip and linear IRQ domain to the CIX Sky1 AUDSS clock
driver to manage the DMA-350 channel interrupt gates in the INFO_AP_IRQC
register. It installs a chained IRQ handler on the shared parent GIC line and
registers a devres teardown action to clean up the irqchip.

> diff --git a/drivers/clk/cix/clk-sky1-audss.c b/drivers/clk/cix/clk-sky1-audss.c
> index e096b8121df30..789cbba631d16 100644
> --- a/drivers/clk/cix/clk-sky1-audss.c
> +++ b/drivers/clk/cix/clk-sky1-audss.c

[ ... ]

> @@ -928,6 +939,110 @@ static int sky1_audss_clks_enable(struct sky1_audss_clks_priv *priv)
>  	return err;
>  }
>  
> +static void sky1_audss_irq_mask(struct irq_data *d)
> +{
> +	struct sky1_audss_clks_priv *priv = irq_data_get_irq_chip_data(d);
> +
> +	regmap_update_bits(priv->regmap_cru, INFO_AP_IRQC,
> +			   BIT(d->hwirq), 0);
> +}
> +
> +static void sky1_audss_irq_unmask(struct irq_data *d)
> +{
> +	struct sky1_audss_clks_priv *priv = irq_data_get_irq_chip_data(d);
> +
> +	regmap_update_bits(priv->regmap_cru, INFO_AP_IRQC,
> +			   BIT(d->hwirq), BIT(d->hwirq));
> +}

[Severity: High]
Will calling regmap_update_bits() in sky1_audss_irq_mask() and
sky1_audss_irq_unmask(), as well as regmap_read() in sky1_audss_irq_handler(),
trigger an invalid wait context warning under CONFIG_PROVE_RAW_LOCK_NESTING
and sleep in atomic context on PREEMPT_RT?

In drivers/clk/cix/clk-sky1-audss.c, priv->regmap_cru is initialized via
devm_regmap_init_mmio() using sky1_audss_regmap_config:

    static const struct regmap_config sky1_audss_regmap_config = {
            .reg_bits = 32,
            .val_bits = 32,
            .reg_stride = 4,
    };

Because sky1_audss_regmap_config does not set .use_raw_spinlock = true,
regmap initializes a non-raw spinlock_t for map->spinlock.

The IRQ core invokes .irq_mask and .irq_unmask while holding irq_desc->lock
(a raw_spinlock_t), and sky1_audss_irq_handler() runs in hardirq context.
Acquiring the regmap spinlock_t in those paths violates raw_spinlock_t
nesting rules. Should sky1_audss_regmap_config set .use_raw_spinlock = true?

> +
> +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;
> +}
> +
> +static const struct irq_domain_ops sky1_audss_irq_ops = {
> +	.map	= sky1_audss_irq_map,
> +	.xlate	= irq_domain_xlate_onecell,
> +};
> +
> +static void sky1_audss_irq_handler(struct irq_desc *desc)
> +{
> +	struct sky1_audss_clks_priv *priv = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long enabled;
> +	unsigned int hwirq;
> +	u32 val;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	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);
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}

[Severity: High]
What happens if a child DMA interrupt is disabled via disable_irq(),
disable_irq_nosync(), or suspend_device_irqs() and that channel later
asserts its level-high interrupt?

Because sky1_audss_irq_chip only provides .irq_mask and .irq_unmask (without
.irq_disable or IRQCHIP_MASK_ON_SUSPEND) and sky1_audss_irq_map() does not
set IRQ_DISABLE_UNLAZY, irq_disable() performs a lazy disable: it marks the
interrupt disabled in software without calling sky1_audss_irq_mask(), leaving
BIT(hwirq) set in INFO_AP_IRQC.

Unlike handle_level_irq() and handle_fasteoi_irq(), handle_untracked_irq()
does not call mask_irq() when an interrupt arrives while disabled:

    void handle_untracked_irq(struct irq_desc *desc)
    {
            raw_spin_lock(&desc->lock);

            if (!irq_can_handle(desc))
                    goto out_unlock;
            ...

When the disabled channel asserts its level interrupt, sky1_audss_irq_handler()
sees BIT(hwirq) set in INFO_AP_IRQC and calls generic_handle_domain_irq() ->
handle_untracked_irq(), which returns immediately without masking the gate or
clearing the source. Once chained_irq_exit() unmasks the parent GIC line,
won't the level-high parent interrupt immediately re-fire in an infinite hard
IRQ storm?

> +
> +static void sky1_audss_irq_teardown(void *data)
> +{
> +	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);
> +}

[Severity: High]
Does sky1_audss_irq_teardown() leak mapped irq_desc descriptors and leave
dangling pointers when calling irq_domain_remove(priv->irq_domain) without
first disposing of existing mappings?

When a consumer calls platform_get_irq(), irq_create_fwspec_mapping() maps the
child virq through sky1_audss_irq_map(), setting desc->irq_data.domain to
priv->irq_domain, desc->irq_data.chip_data to priv, and desc->irq_data.chip to
&sky1_audss_irq_chip. Platform consumers do not call irq_dispose_mapping() on
unbind.

Calling irq_domain_remove() directly without iterating over the domain's
hwirqs and calling irq_dispose_mapping(irq_find_mapping(priv->irq_domain,
hwirq)) leaves those virtual IRQ descriptors allocated in the global IRQ tree
with dangling pointers to the freed domain, freed priv, and (if built as a
module and unloaded) unmapped sky1_audss_irq_chip memory.

> +
> +static int sky1_audss_irq_init(struct platform_device *pdev,
> +			       struct sky1_audss_clks_priv *priv)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +	int irq;
> +
> +	if (!of_property_read_bool(dev->of_node, "interrupt-controller"))
> +		return 0;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	priv->irq_domain = irq_domain_add_linear(dev->of_node,
> +						 SKY1_AUDSS_DMAC_IRQS,
> +						 &sky1_audss_irq_ops, priv);

[Severity: Low]
Can this fail to build under CONFIG_COMPILE_TEST=y when CONFIG_IRQ_DOMAIN is
not enabled?

In drivers/clk/cix/Kconfig, CLK_SKY1_AUDSS depends on ARCH_CIX || COMPILE_TEST
and selects AUXILIARY_BUS, REGMAP_MMIO, and RESET_CONTROLLER, but does not
select IRQ_DOMAIN. Because irq_domain_add_linear(), irq_domain_remove(),
irq_domain_xlate_onecell, and generic_handle_domain_irq() are only declared
when CONFIG_IRQ_DOMAIN is enabled, should CLK_SKY1_AUDSS select IRQ_DOMAIN as
well?

[ ... ]

> @@ -1110,6 +1225,13 @@ static int sky1_audss_clk_probe(struct platform_device *pdev)
>  		goto fail_pm;
>  	}
>  
> +	/* 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);
>  
>  	return 0;

[Severity: High]
Can sky1_audss_irq_teardown(), sky1_audss_irq_mask(), sky1_audss_irq_unmask(),
and sky1_audss_irq_handler() access INFO_AP_IRQC while the AUDSS CRU is
runtime-suspended with its NoC reset asserted and parent clocks disabled?

In sky1_audss_clk_runtime_suspend(), suspending the device asserts
priv->rst_noc and disables all four parent clocks:

    reset_control_assert(priv->rst_noc);

    sky1_audss_clks_disable(priv);

There appear to be three paths where INFO_AP_IRQC is accessed while the CRU is
in this unclocked/reset state:

1. At the end of sky1_audss_clk_probe() (and at fail_pm), pm_runtime_put_sync()
suspends the device immediately if no consumer clock is prepared. Likewise,
during driver unbind, __device_release_driver() runs pm_runtime_put_sync()
before devres_release_all(). Because the device is already runtime-suspended
before devres teardown starts, registering sky1_audss_irq_teardown() after
sky1_audss_clk_rpm_cleanup() does not keep the CRU clocked when
sky1_audss_irq_teardown() calls regmap_update_bits() on INFO_AP_IRQC.

2. In sky1_audss_irq_init(), priv->irq_domain is created without calling
irq_domain_set_pm_device(priv->irq_domain, dev). As a result, requesting or
freeing a child DMA IRQ does not runtime-resume dev before
sky1_audss_irq_unmask() or sky1_audss_irq_mask() accesses INFO_AP_IRQC (and
any gate bit written while suspended would be overwritten by sky1_reg_save
on the next runtime resume).

3. In sky1_audss_clk_pm_ops, pm_runtime_force_suspend() and
pm_runtime_force_resume() are wired to normal system sleep callbacks via
SET_SYSTEM_SLEEP_PM_OPS() rather than noirq callbacks, so the CRU is placed
in reset and unclocked during dpm_suspend() while the chained handler
sky1_audss_irq_handler() and child IRQs are still active.

Should the driver set irq_domain_set_pm_device(), ensure runtime PM is active
around the register write in sky1_audss_irq_teardown(), and move the system
sleep callbacks to noirq?

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