Re: [PATCH v3 04/11] gpio: Add Cortina Systems CS75xx GPIO driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] gpio: hardware direction mutated before acquiring IRQ ownership
- [Medium] gpio: cs75xx_gpio_irq_handler drops latched edges due to early polarity flip
- [Low] kconfig: unmet dependency warning for GPIO_GENERIC without HAS_IOMEM
--

commit 4137a122e8274d6f113967f6692bbb652886d35a
Author: Fil Dunsky <filipp.dunsky@gmail.com>

gpio: Add Cortina Systems CS75xx GPIO driver

Add a driver for the GPIO banks of the Cortina Systems CS75xx SoCs.
The data and direction registers are handled by gpio-mmio; each bank
has its own interrupt with level and edge triggers.

> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
[ ... ]
> @@ -300,6 +300,18 @@ config GPIO_CLPS711X
>  	help
>  	  Say yes here to support GPIO on CLPS711X SoCs.
>  
> +config GPIO_CS75XX
> +	tristate "Cortina Systems CS75xx GPIO support"
> +	depends on ARCH_CORTINA_ACCESS || COMPILE_TEST
> +	select GPIO_GENERIC

[Severity: Low]
Does this Kconfig setup trigger an unmet dependency warning when HAS_IOMEM is
not set? 

By enabling COMPILE_TEST, this configuration allows the driver to be built on
architectures like User-Mode Linux which lack HAS_IOMEM. However, it uses
select GPIO_GENERIC without inheriting its dependency on HAS_IOMEM. This
could lead to unmet direct dependency warnings and potential build failures.
Consider adding depends on HAS_IOMEM here to safely propagate the requirement.

> diff --git a/drivers/gpio/gpio-cs75xx.c b/drivers/gpio/gpio-cs75xx.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpio/gpio-cs75xx.c
[ ... ]
> +static void cs75xx_gpio_irq_ack(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct cs75xx_gpio *g = gpiochip_get_data(gc);
> +
> +	writel(BIT(irqd_to_hwirq(d)), g->base + CS75XX_GPIO_INT);
> +}
[ ... ]
> +static int cs75xx_gpio_irq_reqres(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct cs75xx_gpio *g = gpiochip_get_data(gc);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	int ret;
> +
> +	/* The boot loader may leave the line configured as an output */
> +	ret = gc->direction_input(gc, hwirq);
> +	if (ret)
> +		return ret;
> +
> +	ret = gpiochip_reqres_irq(gc, hwirq);

[Severity: Medium]
Is it safe to mutate the hardware direction before successfully acquiring
the gpiolib IRQ ownership?

If the GPIO line is concurrently requested and actively driven as an output
by another subsystem, changing its hardware direction abruptly could break
the output signal and corrupt the hardware state. While gpiochip_reqres_irq()
checks the in-use state and will ultimately reject the request if it is an
output, the hardware has already been improperly modified here. The core
gpiolib ownership check should likely happen before the hardware state is
changed.

[ ... ]
> +static void cs75xx_gpio_irq_handler(struct irq_desc *desc)
> +{
> +	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> +	struct cs75xx_gpio *g = gpiochip_get_data(gc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long pending;
> +	unsigned int offset;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	pending = readl(g->base + CS75XX_GPIO_INT) &
> +		  readl(g->base + CS75XX_GPIO_IE);
> +
> +	for_each_set_bit(offset, &pending, gc->ngpio) {
> +		scoped_guard(gpio_generic_lock, &g->chip) {
> +			if (g->both_edges & BIT(offset))
> +				cs75xx_gpio_set_polarity(g, BIT(offset));
> +		}
> +		generic_handle_domain_irq(gc->irq.domain, offset);

[Severity: Medium]
Could this drop latched edge interrupts due to an early polarity flip?

In this handler, the edge polarity is toggled to arm the hardware for the
opposite edge, followed by generic_handle_domain_irq(), which uses
handle_edge_irq(). The generic edge handler invokes cs75xx_gpio_irq_ack()
which explicitly clears the interrupt status bit unconditionally.

If the pin state flips again in the small window between
cs75xx_gpio_set_polarity() and cs75xx_gpio_irq_ack(), the hardware latches
the new edge, but the subsequent ack clears it. This could result in edge
interrupts being permanently lost, potentially causing peripheral drivers
waiting for GPIO events to hang.

> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-cs75xx-s2-v3-0-181bb0d23f4c@gmail.com?part=4