Re: [PATCH v2 2/2] gpio: axiado: add SGPIO controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] ax3000_sgpio_get() returns a stale cached value for input GPIOs, which breaks polling loops and risks livelocks.
- [High] Edge interrupts are permanently lost if they occur while the child interrupt is masked.
- [High] Missing required `irq_ack` callback for `handle_edge_irq` causes a NULL pointer dereference.
- [High] Hardware configuration for Clock (Slice A0) and Output Enable (Slice E4) is hardcoded to 32 pins, truncating SGPIO operation.
- [High] Function pointer type mismatch for `sgpio->chip.set` causes Control-Flow Integrity (KCFI) panics.
- [High] Devres registration order causes a Use-After-Free of the IRQ domain when the device is unbound.
- [Medium] The threaded IRQ handler `sgpio_irq_handler` unconditionally returns `IRQ_HANDLED`, preventing the detection of spurious interrupts.
--

--- Patch [2]: [PATCH v2 2/2] gpio: axiado: add SGPIO controller support ---
commit f070fa50e06960bd582fe83c1386133514675e7d
Author: Petar Stepanovic <pstepanovic@axiado.com>

gpio: axiado: add SGPIO controller support
    
Add support for the Axiado SGPIO controller.

Each SGPIO position provides one input GPIO and one output GPIO with
fixed directions. The driver registers the controller as a gpio_chip,
supports interrupts on input GPIOs, and uses regmap for register access.

> diff --git a/drivers/gpio/gpio-axiado-sgpio.c b/drivers/gpio/gpio-axiado-sgpio.c
> new file mode 100644
> index 0000000000000..91e63239c99d4
> --- /dev/null
> +++ b/drivers/gpio/gpio-axiado-sgpio.c

[ ... ]

> +static const struct irq_chip axiado_sgpio_irqchip = {
> +	.name = "axiado-sgpio",
> +	.irq_mask = sgpio_mask_irq,
> +	.irq_unmask = sgpio_unmask_irq,
> +	.irq_set_type = sgpio_set_irq_type,
> +	.irq_shutdown = sgpio_irq_shutdown,
> +	.flags = IRQCHIP_IMMUTABLE | IRQCHIP_MASK_ON_SUSPEND,
> +	GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};

[Severity: High]
Is a .irq_ack callback required here?

The flow handler for edge interrupts is set to handle_edge_irq() below in
sgpio_set_irq_type(). The kernel's handle_edge_irq() unconditionally calls
the .irq_ack callback on the irq_chip.

Will omitting .irq_ack from this structure cause a NULL pointer dereference
when the handler fires?

[ ... ]

> +static int ax3000_sgpio_set(struct gpio_chip *chip, unsigned int offset,
> +			     int value)

[Severity: High]
Is it safe to define this function as returning an int?

The set callback in struct gpio_chip is expected to return void.

Could assigning an int-returning function to the chip.set pointer trigger a
Control-Flow Integrity (KCFI) panic when the callback is indirectly invoked?

[ ... ]

> +static int ax3000_sgpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct ax3000_sgpio *sgpio = gpiochip_get_data(chip);
> +	u32 position = (offset / 2) % 32;
> +	u32 bank = (offset / 2) / 32;
> +	u32 val;
> +
> +	if (!(offset % 2)) {
> +		guard(spinlock_irqsave)(&sgpio->lock);
> +
> +		val = sgpio->slices[3].reg_ss[bank];

[Severity: High]
Does this return a stale cached value for input GPIOs?

The slices[3].reg_ss cache is only updated during the threaded IRQ handler
sgpio_irq_handler().

If a consumer driver loops polling this GPIO state while preventing the
IRQ thread from running, could this cause a permanent livelock since the
cache will never be refreshed?

[ ... ]

> +static irqreturn_t sgpio_irq_handler(int irq, void *arg)
> +{
> +	struct ax3000_sgpio *sgpio = (struct ax3000_sgpio *)arg;
> +	u32 status, new_value;
> +	u32 changed_value;
> +	int bit, reg_ptr;
> +	int ret, i;
> +
> +	/* Read-on-clear (ACK) parent cause */
> +	ret = regmap_read(sgpio->regmap,
> +			  sgpio->regmap_base_offset + sgpio->regs->status,
> +			  &status);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	status >>= 16;
> +
> +	bool has_shifted_layout = (sgpio->max_offset_regs == MAX_OFFSET_REG);
> +
> +	reg_ptr = has_shifted_layout ? 16 - DIV_ROUND_UP(sgpio->ngpios, 32) : 0;
> +
> +	for (i = 0; i < DIV_ROUND_UP(sgpio->ngpios, 32); i++, reg_ptr++) {
> +		if (status & BIT(reg_ptr)) {
> +			ret = regmap_read(sgpio->regmap,
> +					  sgpio->regmap_base_offset +
> +					  sgpio->regs->din_ss +
> +					  reg_ptr * sizeof(u32),
> +					  &new_value);
> +			if (ret)
> +				continue;
> +
> +			{
> +				guard(spinlock_irqsave)(&sgpio->lock);
> +				changed_value = sgpio->slices[3].reg_ss[i] ^ new_value;
> +				sgpio->slices[3].reg_ss[i] = new_value;
> +			}

[Severity: High]
Can this permanently drop edge interrupts if they occur while the child
interrupt is masked?

When an edge event triggers the parent handler, the software cache is updated
here regardless of the child IRQ's mask state.

If the child IRQ is currently masked below, the event isn't pended or
otherwise saved. Once the IRQ is unmasked, wouldn't the edge be completely
lost since the cache has already absorbed the state change?

> +
> +			while (changed_value) {
> +				irq_hw_number_t hwirq;
> +				unsigned int position;
> +				unsigned int type;
> +				bool rising;
> +
> +				bit = __ffs(changed_value);
> +				changed_value &= ~BIT(bit);
> +
> +				position = i * 32 + bit;
> +				hwirq = position * 2;
> +
> +				rising = !!(new_value & BIT(bit));
> +				type = READ_ONCE(sgpio->irq_type[position]);
> +
> +				if (rising &&
> +				    !(type & IRQ_TYPE_EDGE_RISING))
> +					continue;
> +
> +				if (!rising &&
> +				    !(type & IRQ_TYPE_EDGE_FALLING))
> +					continue;
> +
> +				if (READ_ONCE(sgpio->irq_unmasked[position])) {
> +					unsigned int child_irq;
> +
> +					child_irq = irq_find_mapping(sgpio->chip.irq.domain,
> +								     hwirq);
> +					if (child_irq)
> +						handle_nested_irq(child_irq);
> +				}
> +			}
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
Should this return IRQ_HANDLED unconditionally?

If no bits are set in the status register that match the loop conditions,
no events are processed but the handler still claims the interrupt.

Could this prevent the kernel's spurious interrupt detector from noticing
an interrupt storm and disabling the misbehaving IRQ line?

[ ... ]

> +static void sgpio_hw_init(struct ax3000_sgpio *sgpio)
> +{
> +	bool has_shifted_layout = (sgpio->max_offset_regs == MAX_OFFSET_REG);
> +	u32 position;
> +	u32 bank;
> +	int i;
> +
> +	regmap_write(sgpio->regmap,
> +		     sgpio->regmap_base_offset + sgpio->regs->mask, 0);
> +
> +	/* slice A0, Clock Pin - 0 */
> +	regmap_write(sgpio->regmap,
> +		     sgpio->regmap_base_offset + sgpio->regs->mux_0, 0x306);
> +	regmap_write(sgpio->regmap,
> +		     sgpio->regmap_base_offset + sgpio->regs->preset_0,
> +		     sgpio->preset_value);
> +	regmap_write(sgpio->regmap,
> +		     sgpio->regmap_base_offset + sgpio->regs->count_0,
> +		     sgpio->count_value);
> +	regmap_write(sgpio->regmap,
> +		     sgpio->regmap_base_offset + sgpio->regs->pos_0, 0x1f001f);

[Severity: High]
Are the Clock (Slice A0) and Output Enable (Slice E4) hardware configurations
hardcoded to only support 32 pins?

Writing 0x1f001f configures the hardware position for 32 bits. Later on,
only a single 32-bit register is initialized for the Output Enable (regs->oe).

If this driver is bound to a 128-pin or 512-pin controller, won't this
leave all pins beyond the first 32 completely disabled and unclocked?

[ ... ]

> +	/* Slice E4, Output Enable for respective pins */
> +	regmap_write(sgpio->regmap,
> +		     sgpio->regmap_base_offset + sgpio->regs->mux_4,
> +		     has_shifted_layout ? 0x10c : 0x104);
> +	regmap_write(sgpio->regmap, sgpio->regmap_base_offset + sgpio->regs->oe,
> +		     0xffffffff);
> +	regmap_write(sgpio->regmap,
> +		     sgpio->regmap_base_offset + sgpio->regs->oe_ss,
> +		     0xffffffff);
> +	regmap_write(sgpio->regmap,
> +		     sgpio->regmap_base_offset + sgpio->regs->preset_4,
> +		     sgpio->preset_value);
> +	regmap_write(sgpio->regmap,
> +		     sgpio->regmap_base_offset + sgpio->regs->count_4,
> +		     sgpio->count_value);
> +	regmap_write(sgpio->regmap,
> +		     sgpio->regmap_base_offset + sgpio->regs->pos_4, 0x1f001f);

[ ... ]

> +static int sgpio_probe(struct platform_device *pdev)
> +{
...
> +	rc = devm_request_threaded_irq(&pdev->dev, irq, NULL, sgpio_irq_handler,
> +				       IRQF_ONESHOT, dev_name(&pdev->dev), sgpio);
> +
> +	if (rc)
> +		return dev_err_probe(&pdev->dev, rc, "Failed to request IRQ\n");

[Severity: High]
Does registering the IRQ before adding the gpiochip cause a use-after-free
on device unbind?

Devres manages resource cleanup in reverse order (LIFO).

If the IRQ is requested here, and then the gpiochip is added below:

> +
> +	sgpio->chip.parent = &pdev->dev;
> +	sgpio->chip.ngpio = sgpio->ngpios * 2;
> +	sgpio->chip.get_direction = ax3000_sgpio_get_direction;
> +	sgpio->chip.direction_input = ax3000_sgpio_dir_in;
> +	sgpio->chip.direction_output = ax3000_sgpio_dir_out;
> +	sgpio->chip.get = ax3000_sgpio_get;
> +	sgpio->chip.set = ax3000_sgpio_set;
> +	sgpio->chip.can_sleep = true;
> +	sgpio->chip.label = dev_name(&pdev->dev);
> +	sgpio->chip.base = -1;
> +
> +	girq = &sgpio->chip.irq;
> +
> +	gpio_irq_chip_set_chip(girq, &axiado_sgpio_irqchip);
> +	girq->handler = handle_bad_irq;
> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->init_valid_mask = axiado_sgpio_irq_init_valid_mask;
> +	girq->threaded = true;
> +	girq->parent_handler = NULL;
> +	girq->num_parents = 0;
> +	girq->parents = NULL;
> +
> +	rc = devm_gpiochip_add_data(&pdev->dev, &sgpio->chip, sgpio);

won't the gpiochip and its IRQ domain be torn down during removal while the
interrupt handler is still registered?

If the interrupt fires in this window, the handler calls irq_find_mapping()
on a freed IRQ domain.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-axiado-ax3000-sgpio-controller-v2-0-976f4ed11438@axiado.com?part=2