Re: [PATCH v5 4/8] irqchip/realtek-intc-common: Add basic support for Realtek DHC SoCs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] irqchip/realtek-intc-common: Non-raw spinlock regmap used in hardirq context
- [High] irqchip/realtek-intc-common: Broken level IRQ ack and unmaskable lines
- [High] irqchip/realtek-intc-common: Missing irq_dispose_mapping() on teardown
- [High] irqchip/realtek-intc-common: Hardware interrupts not masked in probe or remove
- [Medium] irqchip/realtek-intc-common: Unconditional ISR clear in suspend and resume
- [Low] irqchip/realtek-intc-common: Missing struct forward declarations in header
--

commit 6cf821f34bf6b6377c2ded61f4b04bd1ec0308c3
Author: James Tai <james.tai@realtek.com>

irqchip/realtek-intc-common: Add basic support for Realtek DHC SoCs

This patch adds a common interrupt controller driver for Realtek DHC SoCs,
supporting single- and multi-bank configurations, shared status registers for
direct-to-GIC interrupts, and wire-OR shared parent IRQ lines.

> diff --git a/drivers/irqchip/irq-realtek-intc-common.c b/drivers/irqchip/irq-realtek-intc-common.c
> new file mode 100644
> index 0000000000000..cc08535217bd1
> --- /dev/null
> +++ b/drivers/irqchip/irq-realtek-intc-common.c
> @@ -0,0 +1,353 @@

[ ... ]

> +static bool realtek_intc_dispatch(struct realtek_intc_subset_data *subset_data)
> +{
> +	const struct realtek_intc_info *bank_info = subset_data->bank_info;
> +	struct realtek_intc_data *data = subset_data->common;
> +	u32 ints, inte, mask, val;
> +	irq_hw_number_t hwirq;
> +	bool handled = false;
> +	int irq;
> +
> +	if (regmap_read(data->base, bank_info->isr_offset, &val))
> +		return false;
> +
> +	ints = val & subset_data->subset_mask->ints_mask;
> +
> +	if (!ints)
> +		return false;
> +
> +	if (regmap_read(data->base, bank_info->scpu_int_en_offset, &inte))
> +		return false;
> +
> +	while (ints) {
> +		irq = __ffs(ints);
> +		ints &= ~BIT(irq);
> +
> +		mask = bank_info->isr_to_scpu_int_en_mask[irq];
> +		if (mask != IRQ_ALWAYS_ENABLED && !(inte & mask))
> +			continue;
> +
> +		hwirq = (subset_data->bank_idx * 32) + irq;
> +
> +		if (!generic_handle_domain_irq(data->domain, hwirq))
> +			handled = true;
> +
> +		regmap_write(data->base, bank_info->isr_offset, BIT(irq) & CLEAN_INTC_STATUS);

[Severity: High]
Does clearing bank_info->isr_offset after generic_handle_domain_irq() cause
lost or duplicate interrupts with handle_level_irq()?

realtek_intc_domain_map() configures all domain interrupts with
handle_level_irq and realtek_intc_chip, which only provides .irq_mask and
.irq_unmask (no .irq_ack or .irq_mask_ack).

For non-threaded handlers, handle_level_irq() masks scpu_int_en_offset, runs
the peripheral handler (clearing the device interrupt), and immediately
unmasks scpu_int_en_offset via cond_unmask_irq() before
generic_handle_domain_irq() returns. Clearing BIT(irq) in isr_offset
afterwards can wipe out a new interrupt latched right after the peripheral
handler ran.

For threaded interrupts (IRQF_ONESHOT or forced irqthreads),
handle_level_irq() masks scpu_int_en_offset, wakes the IRQ thread, and
returns to realtek_intc_dispatch() while the peripheral's level output is
still asserted. Clearing BIT(irq) in isr_offset here happens before the IRQ
thread services the device, so BIT(irq) immediately re-latches in
isr_offset. When the IRQ thread finishes and unmask_threaded_irq() calls
realtek_intc_unmask_irq(), isr_offset is not cleared, triggering a spurious
second interrupt.

Should status clearing be handled via irq_chip callbacks (and when unmasking)
rather than after generic_handle_domain_irq() returns?

> +	}
> +
> +	return handled;
> +}

[ ... ]

> +static void realtek_intc_mask_irq(struct irq_data *data)
> +{
> +	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
> +	const struct realtek_intc_desc *desc = intc_data->desc;
> +	const struct realtek_intc_info *bank_info;
> +	int bank_idx, bit_idx;
> +	u32 mask;
> +
> +	bank_idx = data->hwirq / 32;
> +	bit_idx  = data->hwirq % 32;
> +
> +	bank_info = &desc->banks[bank_idx];
> +
> +	mask = bank_info->isr_to_scpu_int_en_mask[bit_idx];
> +	if (!mask || mask == IRQ_ALWAYS_ENABLED)
> +		return;

[Severity: High]
What happens if an interrupt with mask == IRQ_ALWAYS_ENABLED uses a threaded
handler (IRQF_ONESHOT) or is disabled with disable_irq()?

realtek_intc_domain_map() assigns handle_level_irq to all interrupts in the
domain:

    irq_set_chip_and_handler(irq, &realtek_intc_chip, handle_level_irq);

handle_level_irq() relies on .irq_mask to silence the interrupt line while a
threaded handler is running or when the IRQ is disabled. Because
realtek_intc_mask_irq() returns early without masking hardware when mask is
IRQ_ALWAYS_ENABLED, can an active interrupt on that line immediately re-fire
and cause an endless hardirq storm?

> +
> +	regmap_assign_bits(intc_data->base, bank_info->scpu_int_en_offset, mask, false);
> +}
> +
> +static void realtek_intc_unmask_irq(struct irq_data *data)
> +{
> +	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
> +	const struct realtek_intc_desc *desc = intc_data->desc;
> +	const struct realtek_intc_info *bank_info;
> +	int bank_idx, bit_idx;
> +	u32 mask;
> +
> +	bank_idx = data->hwirq / 32;
> +	bit_idx  = data->hwirq % 32;
> +
> +	bank_info = &desc->banks[bank_idx];
> +	mask = bank_info->isr_to_scpu_int_en_mask[bit_idx];
> +	if (!mask || mask == IRQ_ALWAYS_ENABLED)
> +		return;
> +
> +	regmap_set_bits(intc_data->base, bank_info->scpu_int_en_offset, mask);
> +}

[ ... ]

> +int realtek_intc_suspend(struct device *dev)
> +{
> +	struct realtek_intc_data *data = dev_get_drvdata(dev);
> +	const struct realtek_intc_desc *desc = data->desc;
> +	unsigned int i = 0;
> +	int ret;
> +
> +	for (; i < desc->bank_num; i++) {
> +		const struct realtek_intc_info *info = &desc->banks[i];
> +
> +		ret = regmap_read(data->base, info->scpu_int_en_offset, &data->saved_en[i]);
> +		if (ret)
> +			goto err;
> +
> +		ret = regmap_write(data->base, info->scpu_int_en_offset, DISABLE_INTC);
> +		if (ret)
> +			goto err;
> +
> +		ret = regmap_write(data->base, info->isr_offset, CLEAN_INTC_STATUS);
> +		if (ret)
> +			goto err;
> +	}

[ ... ]

> +int realtek_intc_resume(struct device *dev)
> +{
> +	struct realtek_intc_data *data = dev_get_drvdata(dev);
> +	const struct realtek_intc_desc *desc = data->desc;
> +	unsigned int i = 0;
> +	int ret;
> +
> +	for (; i < desc->bank_num; i++) {
> +		const struct realtek_intc_info *info = &desc->banks[i];
> +
> +		ret = regmap_write(data->base, info->isr_offset, CLEAN_INTC_STATUS);
> +		if (ret)
> +			goto err;
> +
> +		ret = regmap_write(data->base, info->scpu_int_en_offset, data->saved_en[i]);
> +		if (ret)
> +			goto err;
> +	}

[Severity: Medium]
Does writing CLEAN_INTC_STATUS (GENMASK(31, 1)) to info->isr_offset in
realtek_intc_suspend() and realtek_intc_resume() clobber shared status bits
and wakeup interrupts?

As noted in the commit message, some peripherals have their interrupt lines
routed directly to the GIC while sharing the same status register
(isr_offset) within this mux. While realtek_intc_dispatch() filters by
subset_mask and isr_to_scpu_int_en_mask to avoid clearing status bits
belonging to direct-to-GIC peripherals, realtek_intc_suspend() and
realtek_intc_resume() unconditionally clear all 31 bits in info->isr_offset
and disable all bits in info->scpu_int_en_offset.

In addition, clearing info->isr_offset in realtek_intc_resume() right before
restoring info->scpu_int_en_offset will discard any wakeup interrupts that
latched during suspend.

[ ... ]

> +int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_desc *desc)
> +{
> +	struct realtek_intc_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	unsigned int total_subset_num = 0;
> +	int ret, parent_irq = 0;
> +
> +	for (unsigned int i = 0; i < desc->bank_num; i++)
> +		total_subset_num += desc->banks[i].subset_num;
> +
> +	data = devm_kzalloc(dev, struct_size(data, subset_data, total_subset_num),
> +			    GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->saved_en = devm_kcalloc(dev, desc->bank_num, sizeof(*data->saved_en), GFP_KERNEL);
> +	if (!data->saved_en)
> +		return -ENOMEM;
> +
> +	data->base = syscon_node_to_regmap(node->parent);

[Severity: High]
Does using syscon_node_to_regmap() here cause an invalid wait context in
hardirq and irq_chip callbacks?

In drivers/mfd/syscon.c, of_syscon_register() initializes the regmap using
syscon_regmap_config, which leaves .use_raw_spinlock as false (and may also
attach a clock whose clk_enable()/clk_disable() path takes a non-raw
spinlock_t). __regmap_init() therefore configures data->base with
regmap_lock_spinlock(), which acquires a sleeping spinlock_t
(LD_WAIT_CONFIG) rather than a raw_spinlock_t.

data->base is then accessed from:
- realtek_intc_mask_irq() -> regmap_assign_bits() and
  realtek_intc_unmask_irq() -> regmap_set_bits() while the genirq core holds
  desc->lock (a raw_spinlock_t, LD_WAIT_SPIN)
- realtek_intc_handler() and realtek_intc_shared_handler() (registered with
  IRQF_SHARED | IRQF_NO_THREAD) -> realtek_intc_dispatch() -> regmap_read() /
  regmap_write() in primary hardirq context

Won't this trigger lockdep "[ BUG: Invalid wait context ]" warnings with
CONFIG_PROVE_RAW_LOCK_NESTING=y and sleep in atomic context on PREEMPT_RT
kernels?

> +	if (IS_ERR(data->base))
> +		return PTR_ERR(data->base);
> +
> +	data->desc = desc;
> +
> +	data->domain = irq_domain_create_linear(of_fwnode_handle(node),
> +						32 * desc->bank_num,
> +						&realtek_intc_domain_ops, data);
> +	if (!data->domain)
> +		return -ENOMEM;
> +
> +	for (unsigned int i = 0; i < desc->bank_num; i++) {
> +		const struct realtek_intc_info *info = &desc->banks[i];
> +
> +		for (unsigned int j = 0; j < info->subset_num; j++) {
> +			ret = realtek_intc_subset(pdev, data, i, j, parent_irq);

[Severity: High]
Should realtek_intc_probe() and realtek_intc_remove() mask controlled
interrupts in hardware before registering or after tearing down the parent
IRQ handlers?

In the genirq core (desc_set_defaults()), newly allocated irq_desc instances
start with IRQD_IRQ_DISABLED and IRQD_IRQ_MASKED set in software, assuming
all hardware interrupts start masked at the controller. Unlike
realtek_intc_suspend(), neither realtek_intc_probe() nor
realtek_intc_remove() disables interrupts in info->scpu_int_en_offset.

If the bootloader, kexec, or a previous driver bind leaves any interrupt bit
enabled in scpu_int_en_offset and it asserts once realtek_intc_subset()
registers the parent handler:
- If the hwirq is not yet mapped in data->domain,
  generic_handle_domain_irq() returns -EINVAL without masking
  scpu_int_en_offset, causing an unmasked level interrupt to storm (and
  returning IRQ_NONE on shared parent IRQs until the GIC disables the parent
  line).
- If the hwirq is mapped but not yet requested, handle_level_irq() calls
  mask_ack_irq() -> mask_irq(), which sees IRQD_IRQ_MASKED already set in
  software and skips calling realtek_intc_mask_irq(), leaving the hardware
  interrupt enabled in scpu_int_en_offset.

> +			if (ret < 0) {
> +				dev_err(dev, "failed to init bank %u subset %u: %d\n",
> +					i, j, ret);
> +				goto irq_domain_cleanup;
> +			}
> +
> +			parent_irq++;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +
> +irq_domain_cleanup:
> +	realtek_intc_teardown_irqs(data, dev, parent_irq);
> +	irq_domain_remove(data->domain);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(realtek_intc_probe);
> +
> +void realtek_intc_remove(struct platform_device *pdev)
> +{
> +	struct realtek_intc_data *data = platform_get_drvdata(pdev);
> +	const struct realtek_intc_desc *desc = data->desc;
> +	struct device *dev = &pdev->dev;
> +	unsigned int count = 0;
> +
> +	for (unsigned int i = 0; i < desc->bank_num; i++)
> +		count += desc->banks[i].subset_num;
> +
> +	realtek_intc_teardown_irqs(data, dev, count);
> +	irq_domain_remove(data->domain);
> +}

[Severity: High]
Does calling irq_domain_remove(data->domain) in realtek_intc_remove() (and
the irq_domain_cleanup path of realtek_intc_probe()) without first disposing
of mapped virqs leak irq_desc mappings and leave dangling pointers?

When interrupts are mapped in data->domain, realtek_intc_domain_map() sets
desc->irq_data.chip to &realtek_intc_chip and desc->irq_data.chip_data to
data, while desc->irq_data.domain points to data->domain.

Removing data->domain without iterating over the domain hwirqs and calling
irq_dispose_mapping(irq_find_mapping(data->domain, hwirq)) triggers:

    WARN_ON(!radix_tree_empty(&domain->revmap_tree));

in irq_domain_remove(), and leaves any mapped virq descriptors active in
sparse_irqs with dangling pointers to the freed data->domain, devm-freed
struct realtek_intc_data, and realtek_intc_chip.

> diff --git a/drivers/irqchip/irq-realtek-intc-common.h b/drivers/irqchip/irq-realtek-intc-common.h
> new file mode 100644
> index 0000000000000..8ea34416d0365
> --- /dev/null
> +++ b/drivers/irqchip/irq-realtek-intc-common.h
> @@ -0,0 +1,93 @@

[ ... ]

> +#define IRQ_ALWAYS_ENABLED	U32_MAX
> +#define DISABLE_INTC		0
> +#define CLEAN_INTC_STATUS	GENMASK(31, 1)
> +
> +int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_desc *desc);
> +void realtek_intc_remove(struct platform_device *pdev);
> +int realtek_intc_suspend(struct device *dev);
> +int realtek_intc_resume(struct device *dev);
> +
> +#endif /* _IRQ_REALTEK_INTC_COMMON_H */

[Severity: Low]
drivers/irqchip/irq-realtek-intc-common.h only includes <linux/bits.h>,
<linux/limits.h>, and <linux/types.h>, without forward declarations or
headers for struct platform_device, struct device, struct regmap, or struct
irq_domain.

If a translation unit includes this header without first including
<linux/platform_device.h> or <linux/device.h>, will struct platform_device
and struct device be scoped only to these function prototypes and trigger
-Wvisibility warnings?

Adding forward declarations (or the corresponding headers) would make the
header self-contained.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018160653.3940371-1-eleanor.lin@realtek.com?part=4