Re: [PATCH v3] pinctrl: mediatek: Add MT8901/MT8971 ACPI driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [High] pinctrl: mediatek: skip-list EINTs masked on PM/IRQ and clobber dom_en
- [High] pinctrl: mediatek: missing bias_set/get_combo callbacks in mt8971_data
- [High] pinctrl: mediatek: missing virtual GPIO descriptors for MT8971 EINTs 187+
- [High] pinctrl: mediatek: OOB pull_type read in mtk_pinctrl_program_bias_by_gpio()
- [High] pinctrl: mediatek: OOB MMIO for virtual GPIOs in mtk_hw_pin_field_lookup()
- [High] pinctrl: mediatek: uninitialized hw->eint left on mtk_build_eint() error
- [High] pinctrl: mediatek: broken GPIO pin range registration via pinctrl_add_gpio_range()
- [High] pinctrl: mediatek: MT8971 debounce pin flags cause -EOPNOTSUPP on ACPI GpioInt
- [Medium] pinctrl: mediatek: wrong nfuncs=8 in mt8971_data for mux functions 8..10
- [Medium] pinctrl: mediatek: unset has_event_mask in mt8971_eint_hw for eint_event
- [Medium] pinctrl: mediatek: mtk_pinctrl_program_bias_by_gpio() fails on R1R0 pins
- [Medium] pinctrl: mediatek: leak of EINT MMIO mappings in mtk_build_eint()
- [Medium] pinctrl: mediatek: mtk_pinctrl_program_bias_by_gpio() fails on R1R0 pins
- [Low] pinctrl: mediatek: leftover debug dev_info() logging in shared probe paths
--

commit ab3a30ea19f9a05365ff5bd3b4f69185efb16390
Author: Shubham Paralikar <shubham.paralikar@mediatek.com>

pinctrl: mediatek: Add MT8901/MT8971 ACPI driver

This commit adds an ACPI pinctrl and GPIO driver for the MediaTek MT8901 and
MT8971 SoCs, selecting the SoC tables at probe time via a vendor _DSD ChipId
property. It also updates the shared Paris, common-v2, and EINT drivers to
support ACPI resources, fixed-stride register tables, bus-hold bias, EINT
wake-event masks, and firmware-owned EINT skip lists.

> diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c
> index 827231dc0d246..7b3198396b6ec 100644
> --- a/drivers/pinctrl/mediatek/mtk-eint.c
> +++ b/drivers/pinctrl/mediatek/mtk-eint.c
[ ... ]
> @@ -327,12 +508,38 @@ static unsigned int mtk_eint_hw_init(struct mtk_eint *eint)
>  		mask_reg = eint->base[i] + eint->regs->mask_set;
>  		for (j = 0; j < eint->base_pin_num[i]; j += 32) {
>  			writel(0xffffffff, dom_reg);
> -			writel(0xffffffff, mask_reg);
> +			writel(mtk_eint_mask_word_for_skip(eint, i, j), mask_reg);
>  			dom_reg += 4;
>  			mask_reg += 4;
>  		}
>  	}
> +	return 0;
> +}
> +
> +/* Make sure skip-listed EINTs are unmasked and domain enabled. */
> +static int mtk_eint_restore_skip_list(struct mtk_eint *eint)
> +{
> +	unsigned int k, eint_num, inst, idx;
> +	void __iomem *reg;
>  
> +	if (!eint->skip_eint_list || !eint->num_skip_eint)
> +		return 0;
> +
> +	for (k = 0; k < eint->num_skip_eint; k++) {\n> +		eint_num = eint->skip_eint_list[k];
> +		if (eint_num >= eint->hw->ap_num)
> +			continue;
> +		inst = eint->pins[eint_num].instance;
> +		if (inst >= eint->nbase)
> +			continue;
> +		idx = eint->pins[eint_num].index;
> +
> +		reg = mtk_eint_get_offset(eint, eint_num, eint->regs->mask_clr);
> +		writel(BIT(idx & 0x1f), reg);
> +
> +		reg = mtk_eint_get_offset(eint, eint_num, eint->regs->dom_en);
> +		writel(BIT(idx & 0x1f), reg);
> +	}
>  	return 0;
>  }

[Severity: High]
Does mtk_eint_restore_skip_list() clobber dom_en for sibling EINTs, and do
suspend/resume and runtime IRQ handling end up masking skip-listed EINTs?

First, in mtk_eint_hw_init(), eint->regs->dom_en (offset 0x400, a plain 32-bit
register rather than write-1-to-set) is written with 0xffffffff for each
32-pin word. Immediately afterward in mtk_eint_do_init(),
mtk_eint_restore_skip_list() writes BIT(idx & 0x1f) to dom_en, which
overwrites the entire 32-bit register and clears domain enable for the other
31 EINTs in that word.

Second, mtk_eint_do_init() zero-allocates eint->wake_mask and eint->cur_mask
(where 0 means masked and 1 means unmasked when mtk_eint_chip_write_mask()
writes ~buf[inst][port] to mask_set). Neither mtk_eint_do_init() nor
mtk_eint_restore_skip_list() sets the skip_eint_list bits in wake_mask or
cur_mask, and mtk_eint_do_resume() does not call mtk_eint_restore_skip_list().
When the system suspends and resumes:

mtk_eint_do_suspend()
  mtk_eint_chip_write_mask(eint, eint->wake_mask)
    writel_relaxed(~buf[inst][port], reg + eint->regs->mask_set)

mtk_eint_do_resume()
  mtk_eint_chip_write_mask(eint, eint->cur_mask)
    writel_relaxed(~buf[inst][port], reg + eint->regs->mask_set)

Will this mask firmware-owned EINTs such as MT8971 EINT215 (PROCHOT) during
suspend and leave them permanently masked after resume?

Third, mtk_eint_do_init() maps a Linux virq with handle_level_irq for every
pin with inst < eint->nbase, including skip-listed pins. Since dom_en is set
and the pin is unmasked, if a skip-listed EINT asserts at runtime,
mtk_eint_irq_handler() will call generic_handle_domain_irq(), causing
handle_level_irq() to see no registered Linux action and call mask_ack_irq()
to mask and acknowledge the firmware-owned interrupt. Should skip-listed EINTs
also be excluded from dom_en or skipped in mtk_eint_irq_handler(), and have
their bits preserved across suspend and resume?

[ ... ]
> @@ -536,6 +747,9 @@ int mtk_eint_do_init(struct mtk_eint *eint, struct mtk_eint_pin *eint_pin)
>  {
>  	unsigned int size, i, port, virq, inst = 0;
>  
> +	dev_info(eint->dev, "EINT init: ap_num=%u nbase=%u db_cnt=%u\n",
> +		 eint->hw->ap_num, eint->nbase, eint->hw->db_cnt);
> +
>  	/* If clients don't assign a specific regs, let's use generic one */
>  	if (!eint->regs)
>  		eint->regs = &mtk_generic_eint_regs;
> @@ -553,6 +767,9 @@ int mtk_eint_do_init(struct mtk_eint *eint, struct mtk_eint_pin *eint_pin)
>  				continue;
>  			eint->base_pin_num[inst]++;
>  		}
> +		for (i = 0; i < eint->nbase; i++)
> +			dev_info(eint->dev, "EINT init: base[%u] has %u pins\n",
> +				 i, eint->base_pin_num[i]);
>  	} else {
[ ... ]
> @@ -602,14 +819,23 @@ int mtk_eint_do_init(struct mtk_eint *eint, struct mtk_eint_pin *eint_pin)
>  	if (!eint->domain)
>  		goto err_eint;
>  
> +	dev_info(eint->dev, "EINT init: irq domain created (ap_num=%u)\n",
> +		 eint->hw->ap_num);
> +
>  	if (eint->hw->db_time) {
>  		for (i = 0; i < MTK_EINT_DBNC_MAX; i++)
>  			if (eint->hw->db_time[i] == 0)
>  				break;
>  		eint->num_db_time = i;
> +		dev_info(eint->dev, "EINT init: debounce table has %u entries\n",
> +			 eint->num_db_time);
> +	} else {
> +		dev_info(eint->dev, "EINT init: no debounce table (db_time=NULL)\n");
> +	}
>  
>  	mtk_eint_hw_init(eint);
> +	dev_info(eint->dev, "EINT init: hw_init done, creating IRQ mappings\n");
> +	mtk_eint_restore_skip_list(eint);

[Severity: Low]
This isn't a bug, but are these step-by-step dev_info() trace messages in
mtk_eint_do_init(), mtk_build_eint(), and mtk_paris_pinctrl_probe_soc()
(including printing hw->eint with %p in mtk_paris_pinctrl_probe_soc())
leftover bringup debugging that should be removed or converted to dev_dbg() so
they do not spam the boot log on every MediaTek Paris SoC?

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8971.c b/drivers/pinctrl/mediatek/pinctrl-mt8971.c
> new file mode 100644
> index 0000000000000..951d3feeafa3f
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt8971.c
> @@ -0,0 +1,58 @@
[ ... ]
> +static const struct mtk_eint_hw mt8971_eint_hw = {
> +	.port_mask = 0xf,
> +	.ports     = 4,
> +	.ap_num    = 216,
> +	.db_cnt    = 32,
> +	/*
> +	 * The debounce registers are not confirmed for MT8971 yet, so leave
> +	 * db_time unset and report debounce as unsupported.
> +	 */
> +	.db_time   = NULL,
> +};
> +
> +static const unsigned int eint_event_mt8971[] = {20};

[Severity: Medium]
Should mt8971_eint_hw set .has_event_mask = true?

mt8971_data wires up eint_event_mt8971 to .eint_event and .total_wake_eints,
but mtk_eint_apply_event_unmask() and mtk_eint_write_event_mask() return early
when !eint->hw->has_event_mask, leaving eint_event_mt8971 unused.

> +/* EINT215 is used by firmware for PROCHOT and must stay unmasked. */
> +static const unsigned int mt8971_skip_eint_list[] = {215};
> +
> +const struct mtk_pin_soc mt8971_data = {
> +	.reg_in_order = true,
> +	.reg_order = mt8971_reg_order,
> +	.reg_order_stride = 0x4000,
> +	.pins = mtk_pins_mt8971,
> +	.npins = ARRAY_SIZE(mtk_pins_mt8971),
> +	.ngrps = ARRAY_SIZE(mtk_pins_mt8971),
> +	.eint_hw = &mt8971_eint_hw,
> +	.eint_pin = eint_pins_mt8971,
> +	.nfuncs = 8,

[Severity: Medium]
Should .nfuncs in mt8971_data be 16 instead of 8?

In pinctrl-mtk-mt8971.h, mt8971_reg_order[PINCTRL_PIN_REG_MODE] has a 4-bit
mask (0xf) and mtk_pins_mt8971[] defines functions with muxval 8, 9, and 10.
In mtk_pctrl_show_one_pin():

	pinmux = mtk_pctrl_get_pinmux(hw, gpio);
	if (pinmux >= hw->soc->nfuncs)
		pinmux -= hw->soc->nfuncs;

With .nfuncs = 8, any MT8971 pin muxed to function 8, 9, or 10 will have 8
subtracted from its reported pinmux value in debugfs.

> +	.gpio_m = 0,
> +	.base_names = mt8971_pinctrl_register_base_name,
> +	.nbase_names = ARRAY_SIZE(mt8971_pinctrl_register_base_name),
> +	.pull_type = NULL,
> +	.drive_set = mtk_pinconf_drive_set_rev1,
> +	.drive_get = mtk_pinconf_drive_get_rev1,
> +	.adv_drive_set = mtk_pinconf_adv_drive_set_raw,
> +	.adv_drive_get = mtk_pinconf_adv_drive_get_raw,
> +	.eint_event = eint_event_mt8971,
> +	.total_wake_eints = ARRAY_SIZE(eint_event_mt8971),
> +	.skip_eint_list = mt8971_skip_eint_list,
> +	.n_skip_eint = ARRAY_SIZE(mt8971_skip_eint_list),
> +};

[Severity: High]
Are .bias_set_combo and .bias_get_combo (as well as .pull_type) missing from
mt8971_data?

mt8971_reg_order defines PINCTRL_PIN_REG_PU, PINCTRL_PIN_REG_PD,
PINCTRL_PIN_REG_PUPD, PINCTRL_PIN_REG_R0, PINCTRL_PIN_REG_R1, and
PINCTRL_PIN_REG_RSEL, but mt8971_data leaves .bias_set_combo and
.bias_get_combo NULL. As a result, mtk_pinconf_get() and mtk_pinconf_set() in
pinctrl-paris.c return -ENOTSUPP for all bias parameters, and
mtk_pinctrl_program_bias_by_gpio() returns -EOPNOTSUPP on MT8971.

Also, if .bias_set_combo and .bias_get_combo are set to
mtk_pinconf_bias_set_combo and mtk_pinconf_bias_get_combo while .pull_type is
NULL, try_all_type defaults to MTK_PULL_TYPE_MASK. Because mt8971_reg_order
provides non-zero masks for all bias register fields on every pin, won't
mtk_pinconf_bias_set_combo() and mtk_pinconf_bias_get_combo() always match
MTK_PULL_RSEL_TYPE first for every pin unless a per-pin pull_type table is
provided?

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> index faa6dbf0f6356..c8d88c9d3f05a 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> @@ -81,7 +82,50 @@ static int mtk_hw_pin_field_lookup(struct mtk_pinctrl *hw,
>  	bool found = false;
>  	u32 bits;
>  
> -	if (hw->soc->reg_cal && hw->soc->reg_cal[field].range) {
> +	/*
> +	 * With reg_in_order, each pin has a fixed-size register block:
> +	 * offset = reg_order[field].offset + pin * reg_order_stride.
> +	 * Fields not present on this SoC have a zero mask and are handled by
> +	 * the "not supported" path below.
> +	 */
> +	if (hw->soc->reg_in_order && hw->soc->reg_order &&
> +	    hw->soc->reg_order[field].mask) {
> +		const struct mtk_pin_field_order *o = &hw->soc->reg_order[field];
> +
> +		if (desc->number > U32_MAX / hw->soc->reg_order_stride) {
> +			dev_err(hw->dev,
> +				"pin %d * reg_order_stride overflow\n",
> +				desc->number);
> +			return -EINVAL;
> +		}
> +
> +		if (o->offset > U32_MAX - desc->number * hw->soc->reg_order_stride) {
> +			dev_err(hw->dev,
> +				"offset + pin %d * reg_order_stride overflow\n",
> +				desc->number);
> +			return -EINVAL;
> +		}
> +
> +		if (o->index >= hw->nbase) {
> +			dev_err(hw->dev,
> +				"Invalid base for field %d for pin = %d (%s)\n",
> +				field, desc->number, desc->name);
> +			return -EINVAL;
> +		}
> +
> +		pfd->index  = o->index;
> +		pfd->offset = o->offset +
> +			      desc->number * hw->soc->reg_order_stride;
> +		pfd->bitpos = o->bitpos;
> +		pfd->mask   = o->mask;
> +		/* Fields never cross a register boundary in this layout. */
> +		pfd->next   = 0;
> +
> +		return 0;
> +	}

[Severity: High]
Can the reg_in_order path in mtk_hw_pin_field_lookup() perform out-of-bounds
MMIO accesses when called on virtual GPIOs?

On MT8901, the reg_cal range tables stop at physical pin 181, so
mtk_hw_pin_field_lookup() returns -ENOTSUPP for virtual GPIOs 182..208. On
MT8971, reg_in_order is true and pins 182..186 in mtk_pins_mt8971[] are
virtual GPIOs with no backing pad hardware, but this branch calculates
pfd->offset = o->offset + desc->number * hw->soc->reg_order_stride for any
desc->number without checking mtk_is_virt_gpio(hw, desc->number).

When mtk_gpio_get(), mtk_gpio_set(), or mtk_xt_get_gpio_state() (e.g. via
debugfs /sys/kernel/debug/gpio or dual-edge EINT handling) is called on a
virtual GPIO on MT8971, won't mtk_hw_get_value() / mtk_hw_set_value() read or
write unbacked MMIO offsets past the physical pin banks?

> @@ -369,26 +413,49 @@ static const struct mtk_eint_xt mtk_eint_xt = {
>  int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> -	int ret, i, j, count_reg_names;
> +	int ret, i, count_reg_names;
> +	unsigned int j, nbase;
> +	struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
> +	struct resource *res;
> +
> +	dev_info(&pdev->dev, "EINT build: starting (CONFIG_EINT_MTK=%d)\n",
> +		 IS_ENABLED(CONFIG_EINT_MTK));
>  
>  	if (!IS_ENABLED(CONFIG_EINT_MTK))
>  		return 0;
>  
> -	if (!of_property_read_bool(np, "interrupt-controller"))
> +	if (is_of_node(fwnode) && !of_property_read_bool(np, "interrupt-controller"))
>  		return -ENODEV;
>  
>  	hw->eint = devm_kzalloc(hw->dev, sizeof(*hw->eint), GFP_KERNEL);
>  	if (!hw->eint)
>  		return -ENOMEM;
>  
> -	count_reg_names = of_property_count_strings(np, "reg-names");
> +	if (is_of_node(fwnode)) {
> +		count_reg_names = of_property_count_strings(np, "reg-names");
> +	} else {
> +		count_reg_names = 0;
> +		for (i = 0; i < pdev->num_resources; i++) {
> +			struct resource *r = &pdev->resource[i];
> +
> +			if (resource_type(r) == IORESOURCE_MEM)
> +				count_reg_names++;
> +		}
> +	}
>  	if (count_reg_names < 0)
>  		return -EINVAL;
>  
> -	hw->eint->nbase = count_reg_names - (int)hw->soc->nbase_names;
> -	if (hw->eint->nbase <= 0)
> +	dev_info(&pdev->dev,
> +		 "EINT build: count_reg_names=%d nbase_names=%u\n",
> +		 count_reg_names, hw->soc->nbase_names);
> +
> +	/* need at least one EINT bank after the pin register bases */
> +	if ((unsigned int)count_reg_names <= hw->soc->nbase_names)
>  		return -EINVAL;

[Severity: High]
If count_reg_names < 0 or (unsigned int)count_reg_names <=
hw->soc->nbase_names, mtk_build_eint() returns -EINVAL directly after
allocating hw->eint without jumping to err_free_base to free hw->eint and
reset hw->eint = NULL.

Because mtk_paris_pinctrl_probe_soc() treats mtk_build_eint() failure as
non-fatal and continues probing, leaving hw->eint non-NULL with uninitialized
fields (hw->eint->pins == NULL, hw->eint->hw == NULL, hw->eint->domain ==
NULL) will cause subsequent checks gated on if (hw->eint) in
mtk_paris_pinctrl_probe_soc(), mtk_gpio_set_config() (dereferencing
hw->eint->pins[desc->eint.eint_n].debounce), mtk_gpio_to_irq(), and
mtk_paris_pinctrl_suspend() / mtk_paris_pinctrl_resume() to dereference NULL
pointers. Should these two error paths goto err_free_base (or allocate
hw->eint after validating count_reg_names)?

> +	nbase = (unsigned int)count_reg_names - hw->soc->nbase_names;
> +	hw->eint->nbase = nbase;
>  
>  	hw->eint->base = devm_kmalloc_array(&pdev->dev, hw->eint->nbase,
>  					    sizeof(*hw->eint->base), GFP_KERNEL | __GFP_ZERO);
>  	if (!hw->eint->base) {
> @@ -397,20 +464,30 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev)
>  	}
>  
>  	for (i = hw->soc->nbase_names, j = 0; i < count_reg_names; i++, j++) {
> -		hw->eint->base[j] = of_iomap(np, i);
> -		if (IS_ERR(hw->eint->base[j])) {
> -			ret = PTR_ERR(hw->eint->base[j]);
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		hw->eint->base[j] = is_of_node(fwnode) ? of_iomap(np, i) :
> +				ioremap(res->start, resource_size(res));
> +		if (!hw->eint->base[j]) {
> +			ret = -ENOMEM;
>  			goto err_free_eint;
>  		}
> +		dev_info(&pdev->dev, "EINT build: base[%u] mapped @ %pK (res %d)\n",
> +			 j, hw->eint->base[j], i);
>  	}

[Severity: Medium]
Does mtk_build_eint() leak the hw->eint->base[j] MMIO mappings when the device
is unbound or when a later step in mtk_paris_pinctrl_probe_soc() fails?

Each hw->eint->base[j] is mapped with unmanaged of_iomap() or ioremap(). Once
mtk_eint_do_init() succeeds, the devm teardown callback mtk_eint_teardown()
removes the IRQ domain and mappings, but nothing calls iounmap() on
eint->base[j] during devres release. Could this use devm_ioremap() /
devm_of_iomap(), or unmap eint->base[j] in mtk_eint_teardown()?

[ ... ]
> @@ -659,6 +761,12 @@ static int mtk_pinconf_bias_set_pupd_r1_r0(struct mtk_pinctrl *hw,
>  {
>  	int err, r0, r1;
>  
> +	/* Bus-hold is not supported for R1R0 path; reject explicitly */
> +	if (pullup == MTK_BUS_HOLD) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
>  	if ((arg == MTK_DISABLE) || (arg == MTK_PUPD_SET_R1R0_00)) {
>  		pullup = 0;
>  		r0 = 0;
[ ... ]
> @@ -814,6 +923,96 @@ int mtk_pinconf_bias_set_combo(struct mtk_pinctrl *hw,
[ ... ]
> +static int mtk_pinctrl_public_arg_to_internal(u32 pub, u32 *out)
> +{
> +	switch (pub) {
> +	case MTK_PIN_ENABLE:\n> +		*out = MTK_ENABLE;
> +		return 0;
> +	case MTK_PIN_DISABLE:
> +		*out = MTK_DISABLE;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}

[Severity: Medium]
When mtk_pinctrl_program_bias_by_gpio() is called with MTK_PIN_ENABLE on a pin
of type MTK_PULL_PUPD_R1R0_TYPE, mtk_pinctrl_public_arg_to_internal() maps
MTK_PIN_ENABLE to MTK_ENABLE (1), which is then passed to
mtk_pinconf_bias_set_combo() -> mtk_pinconf_bias_set_pupd_r1_r0().

However, mtk_pinconf_bias_set_pupd_r1_r0() only accepts MTK_DISABLE (0) or
MTK_PUPD_SET_R1R0_00..11 (100..103) and rejects arg == MTK_ENABLE (1) with
-EINVAL. Won't mtk_pinctrl_program_bias_by_gpio() fail with -EINVAL whenever
enabling pull-up or pull-down on any R1R0 pin (such as 138 of the 182 physical
pins on MT8901)?

> +int mtk_pinctrl_program_bias_by_gpio(unsigned int gpio, u32 pullup, u32 arg)
> +{
> +	struct mtk_pinctrl *hw;
> +	const struct mtk_pin_desc *desc;
> +	u32 int_pullup, int_arg;
> +	int err;
> +
> +	err = mtk_pinctrl_public_pullup_to_internal(pullup, &int_pullup);
> +	if (err)
> +		return err;
> +	err = mtk_pinctrl_public_arg_to_internal(arg, &int_arg);
> +	if (err)
> +		return err;
> +
> +	err = -ENODEV;
> +	mutex_lock(&mtk_pinctrl_instances_lock);
> +	list_for_each_entry(hw, &mtk_pinctrl_instances, instance_node) {
> +		if (gpio >= hw->soc->npins)
> +			continue;
> +
> +		desc = &hw->soc->pins[gpio];
> +		if (desc->number != gpio)
> +			continue;
> +
> +		if (!hw->soc->bias_set_combo) {
> +			err = -EOPNOTSUPP;
> +			break;
> +		}
> +
> +		err = hw->soc->bias_set_combo(hw, desc, int_pullup, int_arg);
> +		break;
> +	}
> +	mutex_unlock(&mtk_pinctrl_instances_lock);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(mtk_pinctrl_program_bias_by_gpio);

[Severity: High]
Does mtk_pinctrl_program_bias_by_gpio() read out-of-bounds from
mt8901_pull_type[] when called with a virtual GPIO number?

On MT8901, hw->soc->npins is 209 (pins 0..181 are physical pads and pins
182..208 are virtual GPIOs), whereas mt8901_pull_type[] in pinctrl-mt8901.c
only has 182 entries (indices 0..181). Unlike mtk_pinconf_set() in
pinctrl-paris.c, which checks mtk_is_virt_gpio(hw, pin) before calling
bias_set_combo, mtk_pinctrl_program_bias_by_gpio() only checks gpio >=
hw->soc->npins. If called with gpio in 182..208, it calls
mtk_pinconf_bias_set_combo():

	if (hw->soc->pull_type)
		try_all_type = hw->soc->pull_type[desc->number];

which reads past the end of mt8901_pull_type[]. Should
mtk_pinctrl_program_bias_by_gpio() reject virtual GPIOs with
mtk_is_virt_gpio(hw, gpio)?

[Severity: Medium]
In addition, since mtk_pinctrl_public_arg_to_internal() only accepts
MTK_PIN_ENABLE and maps it to int_arg = MTK_ENABLE (1), callers of
mtk_pinctrl_program_bias_by_gpio() cannot pass R1R0 resistance selectors and
any call with MTK_PIN_ENABLE on an MTK_PULL_PUPD_R1R0_TYPE pin will pass
int_arg = 1 to hw->soc->bias_set_combo(), causing
mtk_pinconf_bias_set_pupd_r1_r0() to fail with -EINVAL (and failing with
-EOPNOTSUPP on MT8971 where bias_set_combo is NULL). Should
mtk_pinconf_bias_set_pupd_r1_r0() accept MTK_ENABLE with a default R1R0
setting, or should mtk_pinctrl_public_arg_to_internal() support R1R0
arguments?

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-mt8971.h b/drivers/pinctrl/mediatek/pinctrl-mtk-mt8971.h
> new file mode 100644
> index 0000000000000..7e59bc9507a18
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-mt8971.h
> @@ -0,0 +1,1646 @@
[ ... ]
> +static struct mtk_eint_pin eint_pins_mt8971[] = {
[ ... ]
> +	MTK_EINT_PIN(10, 4, 28, 0),
> +	MTK_EINT_PIN(11, 4, 34, 0),
> +	MTK_EINT_PIN(12, 5, 0, 0),
> +	MTK_EINT_PIN(13, 5, 0, 0),
[ ... ]
> +	MTK_EINT_PIN(185, 0, 54, 0),
> +	MTK_EINT_PIN(186, 0, 55, 0),
> +	MTK_EINT_PIN(187, 1, 0, 1),
> +	MTK_EINT_PIN(188, 1, 1, 1),
[ ... ]
> +	MTK_EINT_PIN(214, 1, 27, 0),
> +	MTK_EINT_PIN(215, 2, 0, 1),
> +};
[ ... ]
> +	/*
> +	 * Virtual GPIO: no ball, but backed by EINT hardware. funcs[0] has
> +	 * no name so that mtk_is_virt_gpio() treats it as virtual.
> +	 */
> +	MTK_PIN(182, "GPIO182", MTK_EINT_FUNCTION(0, 182), DRV_GRP4,
> +		MTK_FUNCTION(0, NULL)),
> +	MTK_PIN(183, "GPIO183", MTK_EINT_FUNCTION(0, 183), DRV_GRP4,
> +		MTK_FUNCTION(0, NULL)),
> +	MTK_PIN(184, "GPIO184", MTK_EINT_FUNCTION(0, 184), DRV_GRP4,
> +		MTK_FUNCTION(0, NULL)),
> +	MTK_PIN(185, "GPIO185", MTK_EINT_FUNCTION(0, 185), DRV_GRP4,
> +		MTK_FUNCTION(0, NULL)),
> +	MTK_PIN(186, "GPIO186", MTK_EINT_FUNCTION(0, 186), DRV_GRP4,
> +		MTK_FUNCTION(0, NULL)),
> +};

[Severity: High]
Are virtual GPIO descriptors for EINTs 187..214 missing from
mtk_pins_mt8971[], and should non-EINT pins in eint_pins_mt8971[] use
INVALID_BASE (0xff) instead of instance 5?

In eint_pins_mt8971[], EINTs 187..214 are populated on EINT bank 1 (indices
0..27), and mt8971_eint_hw.ap_num is 216. However, mtk_pins_mt8971[] stops at
pin 186 (npins = 187). When any interrupt on EINT bank 1 (hwirq 187..214) is
requested, mtk_eint_irq_request_resources() calls mtk_xt_get_gpio_n(), which
searches mtk_pins_mt8971[0..186], fails to find the EINT, and fails the IRQ
request with -EINVAL (and gpiolib rejects any ACPI GpioInt resource >= 187
because chip->ngpio is 187).

Also, the 12 pins without EINT support in mtk_pins_mt8971[] (pins 12, 13, 17,
18, 22, 56..59, 148, 149, 181) are given instance 5 in eint_pins_mt8971[]
rather than INVALID_BASE (0xff). If firmware provides more than 5 EINT memory
resources, inst < eint->nbase will treat all 12 non-EINT pins as valid EINTs
aliased to bank 5 index 0.

> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index 09098b68f7257..a827ae618eff3 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
[ ... ]
> @@ -928,9 +956,15 @@ static int mtk_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
>  
>  	desc = (const struct mtk_pin_desc *)&hw->soc->pins[offset];
>  
> +	/*
> +	 * Not every EINT has debounce hardware, the virtual GPIOs among them.
> +	 * Report that as unsupported so gpiolib treats the request as optional
> +	 * rather than warning about an -EINVAL from the EINT driver.
> +	 */
>  	if (!hw->eint ||
>  	    pinconf_to_config_param(config) != PIN_CONFIG_INPUT_DEBOUNCE ||
> -	    desc->eint.eint_n == EINT_NA)
> +	    desc->eint.eint_n == EINT_NA ||
> +	    !hw->eint->pins[desc->eint.eint_n].debounce)
>  		return -ENOTSUPP;
>  
>  	debounce = pinconf_to_config_argument(config);
>  
>  	return mtk_eint_set_debounce(hw->eint, desc->eint.eint_n, debounce);
>  }

[Severity: High]
Should mtk_gpio_set_config() and mtk_eint_irq_handler() also check
hw->eint->hw->db_time (or should eint_pins_mt8971[] clear .debounce)?

In pinctrl-mt8971.c, mt8971_eint_hw sets .db_time = NULL with a comment noting
that debounce registers are not confirmed on MT8971 and debounce should be
reported as unsupported. However, 32 pins in eint_pins_mt8971[] still have
.debounce = 1.

When acpi_dev_gpio_irq_wake_get_by() requests an ACPI GpioInt on any of those
32 pins on MT8971, it calls gpio_set_debounce_timeout() ->
mtk_gpio_set_config(). Because .debounce is 1 and mtk_gpio_set_config() does
not check !hw->eint->hw->db_time, it calls mtk_eint_set_debounce(), which
returns -EOPNOTSUPP. Since gpio_set_config_with_argument_optional() only
ignores -ENOTSUPP and treats -EOPNOTSUPP as a fatal error, won't ACPI GpioInt
setup fail on all 32 debounce-marked pins on MT8971? Additionally, in
mtk_eint_irq_handler(), if (eint->pins[eint_num].debounce) will still call
mtk_eint_debounce_process() and touch the unconfirmed dbnc_ctrl and dbnc_set
registers on MT8971.

[ ... ]
> @@ -1069,26 +1139,79 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev)
[ ... ]
>  	/* Build gpiochip should be after pinctrl_enable is done */
>  	err = mtk_build_gpiochip(hw);
>  	if (err)
>  		return dev_err_probe(dev, err, "Failed to add gpio_chip\n");
>  
> +	dev_info(dev, "PROBE: gpiochip registered, adding GPIO range\n");
> +
> +	pinctrl_add_gpio_range(hw->pctrl, &hw->range);
> +#ifdef CONFIG_GPIO_ACPI
> +	if (hw->eint) {
> +		dev_info(dev, "PROBE: requesting ACPI GPIO interrupts\n");
> +		acpi_gpiochip_request_interrupts(&hw->chip);
> +		dev_info(dev, "PROBE: ACPI GPIO interrupts requested\n");
> +
> +		err = devm_add_action_or_reset(dev,
> +					       mtk_pinctrl_acpi_free_interrupts,
> +					       &hw->chip);
> +		if (err) {
> +			pinctrl_remove_gpio_range(hw->pctrl, &hw->range);
> +			return dev_err_probe(dev, err,
> +					     "Failed to add ACPI GPIO cleanup\n");
> +		}
> +	} else {
> +		dev_info(dev, "PROBE: no EINT, skipping ACPI GPIO interrupts\n");
> +	}
> +#endif
> +
>  	platform_set_drvdata(pdev, hw);
>  
> +	/*
> +	 * Register so mtk_pinctrl_program_bias_by_gpio() can find this
> +	 * controller. The devm action unregisters it before hw is freed.
> +	 */
> +	mtk_pinctrl_register_instance(hw);
> +
> +	err = devm_add_action_or_reset(&pdev->dev,
> +				       mtk_paris_unregister_instance, hw);
> +	if (err)
> +		return err;

[Severity: High]
Does registering hw->range with pinctrl_add_gpio_range() here leave
gdev->pin_ranges empty on ACPI, duplicate ranges on Device Tree platforms, and
leave a dangling range on error or teardown?

First, mtk_build_gpiochip() sets chip->request = gpiochip_generic_request and
chip->free = gpiochip_generic_free. In gpiolib, gpiochip_generic_request()
checks:

	if (list_empty(&gc->gpiodev->pin_ranges))
		return 0;

Calling pinctrl_add_gpio_range() only adds hw->range to hw->pctrl->gpio_ranges
and does not add a pin range to gc->gpiodev->pin_ranges (unlike
gpiochip_add_pin_range()). On ACPI platforms, gc->gpiodev->pin_ranges remains
empty, so gpiochip_generic_request() returns early without calling
pinctrl_gpio_request() or mtk_pinmux_gpio_request_enable() to mux the pin to
GPIO mode.

Second, on Device Tree platforms, devm_gpiochip_add_data() in
mtk_build_gpiochip() already parses the DT gpio-ranges property via
of_gpiochip_add_pin_range(), so unconditionally adding hw->range registers a
duplicate range.

Third, if devm_add_action_or_reset(&pdev->dev, mtk_paris_unregister_instance,
hw) fails (or when the driver is unbound), pinctrl_remove_gpio_range() is
never called for hw->range. Should this use gpiochip_add_pin_range() (for
example via chip->add_pin_ranges when gpio-ranges is not present in DT) so
that gpiolib populates gdev->pin_ranges and automatically removes the range
when the gpio_chip is torn down?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105050535.2551413-1-shubham.paralikar@mediatek.com?part=1