Re: [PATCH v5 3/4] gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect implementation of IRQ_TYPE_EDGE_BOTH causes double-dispatch on rising edges and missed falling edges.
- [High] Missing bounds check on computed hwirq causes out-of-bounds processing and infinite interrupt storms.
- [High] Missing interrupt enable sanitization and flawed mask/ack logic causes kernel hangs from bootloader-enabled interrupts.
- [Medium] The driver returns -ENOTSUPP for a debounce value of 0, breaking standard API requests to disable debouncing.
--

commit 70be814576adfb9c031580980614a470a5f6afe1
Author: Tzuyi Chang <tychang@realtek.com>
Subject: gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

This commit introduces a new GPIO driver for the Realtek DHC RTD1625 SoC,
which uses a per-pin register architecture. It supports basic GPIO operations
and optional interrupt handling, addressing a hardware quirk where both
assert and de-assert interrupts fire simultaneously on edge toggles.

> diff --git a/drivers/gpio/gpio-rtd1625.c b/drivers/gpio/gpio-rtd1625.c
> new file mode 100644
> index 0000000000000..27af1ac65d281
> --- /dev/null
> +++ b/drivers/gpio/gpio-rtd1625.c
[ ... ]
> +static int rtd1625_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
> +				     unsigned int debounce)
> +{
> +	struct rtd1625_gpio *data = gpiochip_get_data(chip);
> +	u8 deb_val;
> +	u32 val;
> +
> +	switch (debounce) {
> +	case 1:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_1US;
> +		break;
[ ... ]
> +	case 50000:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_50MS;
> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}

[Severity: Medium]
Will this return -ENOTSUPP if callers request a debounce value of 0?

Standard kernel callers, such as gpiolib or the input subsystem, typically
pass a debounce value of 0 to disable GPIO debouncing. By not handling
debounce == 0, does this break standard API requests to disable debouncing?

[ ... ]
> +static void rtd1625_gpio_irq_handle(struct irq_desc *desc)
> +{
[ ... ]
> +	for (i = 0; i < data->info->num_gpios; i += 32) {
> +		reg_offset = get_reg_offset(data, i);
> +		status = readl_relaxed(data->irq_base + reg_offset);
> +
> +		/*
> +		 * Hardware quirk: The controller fires both "assert" and "de-assert"
> +		 * interrupts simultaneously on any edge toggle.
> +		 * We must pre-clear edge interrupts here. If we drop an unwanted
> +		 * de-assert interrupt below, it will never reach the IRQ core
> +		 * (generic_handle_domain_irq), meaning ->irq_ack() won't be called.
> +		 * Failing to clear it here leads to an interrupt storm.
> +		 */
> +		if (irq != data->irqs[RTD1625_IRQ_LEVEL])
> +			writel_relaxed(status, data->irq_base + reg_offset);
> +
> +		for_each_set_bit(j, &status, 32) {
> +			hwirq = i + j;
> +			irq_type = irq_get_trigger_type(irq_find_mapping(domain, hwirq));

[Severity: High]
Is there a missing bounds check on the computed hwirq against num_gpios?

If num_gpios is not a multiple of 32 (like 166 or 4), the final iteration
could include unused or reserved hardware bits. If any of these bits read
as 1, hwirq will exceed num_gpios. Calling generic_handle_domain_irq() with
an unmapped hwirq returns -EINVAL.

For level interrupts where the status is not pre-cleared, since the unmapped
IRQ is rejected, irq_ack() would never be invoked. Could this leave the level
interrupt unacknowledged in hardware and cause an infinite interrupt storm?

> +
> +			/*
> +			 * Filter out the hardware-forced de-assert interrupt unless
> +			 * the user explicitly requested IRQ_TYPE_EDGE_BOTH.
> +			 */
> +			if (irq == data->irqs[RTD1625_IRQ_DEASSERT] &&
> +			    irq_type != IRQ_TYPE_EDGE_BOTH)
> +				continue;
> +
> +			generic_handle_domain_irq(domain, hwirq);
> +		}
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void rtd1625_gpio_ack_irq(struct irq_data *d)
> +{
> +	struct rtd1625_gpio *data = irq_data_get_irq_chip_data(d);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	u32 irq_type = irqd_get_trigger_type(d);
> +	u32 bit_mask = BIT(hwirq % 32);
> +	int reg_offset;
> +
> +	if (irq_type & IRQ_TYPE_LEVEL_MASK) {
> +		reg_offset = rtd1625_gpio_level_offset(data, hwirq);
> +		writel_relaxed(bit_mask, data->irq_base + reg_offset);
> +	}
> +}

[Severity: High]
Does this correctly handle unrequested interrupts, where irq_type defaults
to 0 (IRQ_TYPE_NONE)?

If a bootloader leaves a GPIO interrupt active, it can fire as soon as the OS
registers the IRQ domain. Because the interrupt isn't requested, irq_type is 0.

In rtd1625_gpio_ack_irq(), the code only acks if the type matches
IRQ_TYPE_LEVEL_MASK.

Similarly, in rtd1625_gpio_disable_irq(), it fails to mask the interrupt if
the type is 0:

	if (irq_type & IRQ_TYPE_EDGE_BOTH)
		rtd1625_gpio_disable_edge_irq(data, hwirq);
	else if (irq_type & IRQ_TYPE_LEVEL_MASK)
		rtd1625_gpio_disable_level_irq(data, hwirq);

Could this cause the driver to do nothing to clear or mask the interrupt,
defeating the core IRQ subsystem's spurious interrupt handling
(handle_bad_irq) and causing a hang from an infinite interrupt storm?

[ ... ]
> +static int rtd1625_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		return rtd1625_gpio_irq_set_edge_type(d, 1);
> +
> +	case IRQ_TYPE_EDGE_FALLING:
> +		return rtd1625_gpio_irq_set_edge_type(d, 0);
> +
> +	case IRQ_TYPE_EDGE_BOTH:
> +		return rtd1625_gpio_irq_set_edge_type(d, 1);

[Severity: High]
Does this code configure EDGE_BOTH identically to EDGE_RISING by passing 1 to
rtd1625_gpio_irq_set_edge_type()?

If the hardware only detects the configured edge, will all falling edges be
completely missed? Additionally, in rtd1625_gpio_irq_handle(), the code
explicitly skips filtering the DEASSERT interrupt for EDGE_BOTH requests:

	if (irq == data->irqs[RTD1625_IRQ_DEASSERT] &&
	    irq_type != IRQ_TYPE_EDGE_BOTH)
		continue;

Since a rising edge fires both ASSERT and DEASSERT simultaneously due to the
hardware quirk, will a single rising edge be processed twice, leading to
incorrect input event counting and dropped events?

> +
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		return rtd1625_gpio_irq_set_level_type(d, 0);

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