Re: [PATCH v12 5/5] mfd: Add support for MediaTek SPMI PMICs and MT6363/73

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Double free of the IRQ domain on probe error handling.
- [High] Completely broken IRQ unmasking logic causes out-of-bounds shifts and targets incorrect registers.
- [High] Out-of-bounds array read in chained IRQ handler due to double indexing.
- [High] Uninitialized variable dictates control flow on regmap read failure.
- [High] IRQ allocation loop overwrites the same descriptor and leaves subsequent ones uninitialized.
- [High] Race condition causing lost interrupts due to delayed W1C register clearing.
- [Medium] Failure to properly check for a mapping error from `of_irq_get()`.
--

commit 4bb145c21ce1c6663167ab05057b958ba97cdc22
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

mfd: Add support for MediaTek SPMI PMICs and MT6363/73

This driver adds support for MediaTek SPMI PMICs and their interrupt
controller. It sets up a regmap from the SPMI bus and initializes an
irqchip to handle chained interrupts for sub-devices.

> diff --git a/drivers/mfd/mtk-spmi-pmic.c b/drivers/mfd/mtk-spmi-pmic.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/mfd/mtk-spmi-pmic.c

[ ... ]

> +static void mtk_spmi_pmic_irq_set_unmasking(struct irq_data *d, bool unmask)
> +{
> +	struct mtk_spmi_pmic *pmic = irq_data_get_irq_chip_data(d);
> +	const struct mtk_spmi_pmic_variant *variant = pmic->variant;
> +	struct regmap *regmap = pmic->regmap;
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	unsigned short i;
> +
> +	for (i = 0; i < variant->num_groups; i++) {
> +		const struct mtk_spmi_pmic_irq_grp *irq_grp = &variant->pmic_irq[i];
> +		u32 con_reg;
> +		u8 irq_en_bit;
> +
> +		if (hwirq < irq_grp->hwirq_base)
> +			continue;
> +
> +		con_reg = irq_grp->con_reg + (variant->con_reg_len * i);
> +		irq_en_bit = hwirq - irq_grp->hwirq_base;
> +		regmap_assign_bits(regmap, con_reg, BIT(irq_en_bit), unmask);
> +
> +		break;
> +	}
> +}

Does this loop evaluate correctly for hwirq? Since the first group has a
hwirq_base of 0, the check (hwirq < irq_grp->hwirq_base) will always be
false for any unsigned hwirq. This means the loop will process every
interrupt as if it belongs to group 0, and then immediately break.

Additionally, could this result in an out-of-bounds shift? If hwirq is 40,
irq_en_bit evaluates to 40 - 0 = 40. Using BIT(40) can result in undefined
behavior for 32-bit values or a shift out of bounds for the register.

Finally, is the register offset calculated correctly here? The code uses the
group index i to calculate the register offset rather than the chunk offset
((hwirq - base) / 8), which seems to target incorrect registers.

[ ... ]

> +static int mtk_spmi_pmic_irq_alloc(struct irq_domain *d, unsigned int virq,
> +				   unsigned int nr_irqs, void *data)
> +{
> +	struct mtk_spmi_pmic *pmic = d->host_data;
> +	struct irq_fwspec *fwspec = data;
> +	irq_hw_number_t hwirq;
> +	unsigned int irqtype;
> +	int i, ret;
> +
> +	ret = mtk_spmi_pmic_irq_translate(d, fwspec, &hwirq, &irqtype);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		irq_set_lockdep_class(virq, &mtk_spmi_pmic_irq_lock_class,
> +				      &mtk_spmi_pmic_irq_request_class);
> +		irq_domain_set_info(d, virq, hwirq, &mtk_spmi_pmic_irq_chip,
> +				    pmic, handle_level_irq, NULL, NULL);
> +	}
> +
> +	return 0;
> +}

When nr_irqs is greater than 1, does this loop repeatedly initialize the
same descriptor? Because virq and hwirq are passed directly without adding
the loop index i, the first IRQ is overwritten multiple times while the
remaining descriptors are left uninitialized.

[ ... ]

> +static int mtk_spmi_pmic_handle_group_irq(struct mtk_spmi_pmic *pmic, int group)
> +{
> +	const struct mtk_spmi_pmic_irq_grp *irq_grp = &pmic->variant->pmic_irq[group];
> +	struct regmap *regmap = pmic->regmap;
> +	struct device *dev = pmic->dev;
> +	int i, ret;
> +
> +	for (i = 0; i < irq_grp->num_int_regs; i++) {
> +		u32 status, saved_status;
> +
> +		ret = regmap_read(regmap, irq_grp->sta_reg + i, &status);
> +		if (ret) {
> +			dev_err(dev, "Could not read IRQ status register: %d", ret);
> +			return ret;
> +		}
> +
> +		if (status == 0)
> +			continue;
> +
> +		saved_status = status;
> +		do {
> +			irq_hw_number_t hwirq;
> +			u8 bit = __ffs(status);
> +
> +			/* Each register has 8 bits: this is the first IRQ of this group */
> +			hwirq = MTK_SPMI_PMIC_VAL_BITS * i;
> +
> +			/* Offset by this group's start interrupt */
> +			hwirq += irq_grp->hwirq_base;
> +
> +			/* Finally, offset by the fired IRQ's bit number */
> +			hwirq += bit;
> +
> +			status &= ~BIT(bit);
> +
> +			generic_handle_domain_irq_safe(pmic->dom, hwirq);
> +		} while (status);
> +
> +		/* Clear the interrupts by writing the previous status */
> +		regmap_write(regmap, irq_grp->sta_reg + i, saved_status);
> +	}
> +
> +	return 0;
> +}

Could this write-one-to-clear implementation lose interrupts? If a child
device triggers a new interrupt after being serviced but before the
regmap_write() call, writing back saved_status (which has the serviced bits
set) will clear the newly arrived interrupt without it ever being handled.

[ ... ]

> +static void mtk_spmi_pmic_handle_chained_irq(struct irq_desc *desc)
> +{
> +	struct mtk_spmi_pmic *pmic = irq_desc_get_handler_data(desc);
> +	const struct mtk_spmi_pmic_variant *variant = pmic->variant;
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct regmap *regmap = pmic->regmap;
> +	bool irq_handled = false;
> +	int i, ret;
> +	u32 val;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	/* If irq_grp_reg is present there are multiple IRQ groups */
> +	if (variant->irq_grp_reg > 0) {
> +		ret = regmap_read(regmap, variant->irq_grp_reg, &val);
> +		if (ret)
> +			handle_bad_irq(desc);
> +
> +		/* This is very unlikely to happen */
> +		if (val == 0) {
> +			chained_irq_exit(chip, desc);
> +			return;
> +		}
> +	} else {

If regmap_read() fails above, handle_bad_irq() is called, but it returns
void and does not abort the handler. Does this mean the code will proceed
to evaluate val == 0 and val & group_bit using val as an uninitialized
stack variable?

> +		val = BIT(0);
> +	}
> +
> +	for (i = 0; i < variant->num_groups; i++) {
> +		const struct mtk_spmi_pmic_irq_grp *irq_grp = &variant->pmic_irq[i];
> +		u8 group_bit = BIT(irq_grp[i].group_num);

Are we double-indexing the irq_grp array here? Since irq_grp is already
defined as &variant->pmic_irq[i], accessing irq_grp[i] effectively evaluates
to variant->pmic_irq[i + i]. For MT6363 which has 6 groups, this will read
out of bounds when i >= 3.

[ ... ]

> +static int mtk_spmi_pmic_irq_init(struct device *dev, struct regmap *regmap,
> +				  const struct mtk_spmi_pmic_variant *variant)
> +{
> +	struct fwnode_handle *fwnode = of_fwnode_handle(dev->of_node);
> +	struct mtk_spmi_pmic *pmic;
> +	int ret;
> +
> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> +	if (!pmic)
> +		return -ENOMEM;
> +
> +	pmic->irq = of_irq_get(dev->of_node, 0);
> +	if (pmic->irq < 0)
> +		return dev_err_probe(dev, pmic->irq, "Cannot get IRQ\n");

Does this properly handle the NO_IRQ case? of_irq_get() returns 0 on mapping
failures. If it returns 0, the check passes and the driver may attempt to
configure hardware IRQ 0, which is typically reserved.

> +
> +	pmic->dev = dev;
> +	pmic->regmap = regmap;
> +	pmic->variant = variant;
> +
> +	pmic->dom = irq_domain_create_tree(fwnode, &mtk_spmi_pmic_irq_domain_ops, pmic);
> +	if (!pmic->dom)
> +		return dev_err_probe(dev, -ENOMEM, "Cannot create IRQ domain\n");
> +
> +	ret = devm_add_action_or_reset(dev, mtk_spmi_pmic_irq_remove, pmic);
> +	if (ret) {
> +		irq_domain_remove(pmic->dom);
> +		return ret;
> +	}

Could this lead to a double-free of the IRQ domain?
devm_add_action_or_reset() automatically executes the cleanup action (which
calls irq_domain_remove) if it fails. Manually calling
irq_domain_remove(pmic->dom) again in the error path will free the domain a
second time.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511101355.122478-1-angelogioacchino.delregno@collabora.com?part=5