Re: [PATCH 7/7] pinctrl: sunxi: add support for the Allwinner A733

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Incorrect mapping of logical to physical IRQ banks for NCAT3 layout breaks all A733 GPIO interrupts.
- [Medium] Commit message contradicts code regarding pin counts per port.

Pre-existing issues:
- [High] Off-by-one logic in `irq_bank` assignment causes out-of-bounds array writes and reads when a SoC's first bank has no pins.
- [High] Global static struct mutation across probe deferrals causes NULL pointer dereference and kernel panic.
--

commit 8c3148d94675fca6724f59ce7243638c15e54f3b
Author: Andre Przywara <andre.przywara@arm.com>

pinctrl: sunxi: add support for the Allwinner A733

The Allwinner A733 changes the layout of the pinctrl MMIO register frame, but
stays otherwise rather close to the previous IP. The main PIO looks similar to
the one in the A523, with ports B-K being implemented, each also with the same
number of pins per port as on the A523.

> The Allwinner A733 changes the layout of the pinctrl MMIO register
> frame, but stays otherwise rather close to the previous IP.
> The main PIO looks similar to the one in the A523, with ports B-K being
> implemented, each also with the same number of pins per port as on the
> A523.

[Severity: Medium]
Are these pin counts actually identical? The commit message explicitly states
ports B-K are implemented with the same number of pins per port as the A523,
but in the patch PB has 11 pins and PK has 26 pins. In contrast, 
a523_nr_bank_pins defines PB with 15 pins and PK with 24 pins.

> diff --git a/drivers/pinctrl/sunxi/pinctrl-sun60i-a733.c b/drivers/pinctrl/sunxi/pinctrl-sun60i-a733.c
> new file mode 100644
> index 0000000000000..ca82f08a3a65b
> --- /dev/null
> +++ b/drivers/pinctrl/sunxi/pinctrl-sun60i-a733.c
[ ... ]
> +static const u8 a733_nr_bank_pins[SUNXI_PINCTRL_MAX_BANKS] =
> +/*	  PA  PB  PC  PD  PE  PF  PG  PH  PI  PJ  PK */
> +	{  0, 11, 17, 24, 16,  7, 15, 20, 17, 28, 26 };

[Severity: High]
This is a pre-existing issue, but will setting PA to 0 pins cause an
out-of-bounds array write later during initialization?

When a SoC's first bank has 0 pins, prepare_function_table() processes PB
(bank = 1) first. The initialization logic for irq_bank does this:

drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c:prepare_function_table() {
    ...
    if (bank > last_bank)
        irq_bank++;
    func[lastfunc].muxval = irq_mux;
    func[lastfunc].irqbank = irq_bank;
    ...
}

Because bank (1) > last_bank (0), irq_bank increments from 0 to 1 before its
first use. This shifts all subsequent banks by one, giving PK an irqbank of 10.

Later in sunxi_pinctrl_build_state():

drivers/pinctrl/sunxi/pinctrl-sunxi.c:sunxi_pinctrl_build_state() {
    ...
    int irqnum = func->irqnum + func->irqbank * IRQ_PER_BANK;
    pctl->irq_array[irqnum] = pin->pin.number;
    ...
}

For PK, this calculates an irqnum up to 351. Since pctl->irq_array is
allocated with exactly 320 elements (10 * 32), does this write past the end
of the array? This can also cause an out-of-bounds read on desc->irq_bank_map
when handling PK interrupts.

> +
> +static const u8 a733_irq_bank_muxes[SUNXI_PINCTRL_MAX_BANKS] =
> +/*	  PA  PB  PC  PD  PE  PF  PG  PH  PI  PJ  PK */
> +	{  0, 14, 14, 14, 14, 14, 14, 14, 14, 14, 14};
> +
> +static struct sunxi_pinctrl_desc a733_pinctrl_data = {
> +	.irq_banks = 10,
> +	.irq_read_needs_mux = true,
> +	.io_bias_cfg_variant = BIAS_VOLTAGE_PIO_POW_MODE_SEL,
> +};
> +
> +static int a733_pinctrl_probe(struct platform_device *pdev)
> +{
> +	return sunxi_pinctrl_dt_table_init(pdev, a733_nr_bank_pins,
> +					   a733_irq_bank_muxes,
> +					   &a733_pinctrl_data,

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to pass a statically
allocated structure to sunxi_pinctrl_dt_table_init() where its npins state is
mutated?

In init_pins_table(), the pin counts are accumulated indefinitely:

drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c:init_pins_table() {
    ...
    for (i = 0; i < SUNXI_PINCTRL_MAX_BANKS; i++) {
        desc->npins += pins_per_bank[i];
    ...
}

If the probe function defers, npins retains the accumulated value and doubles
on the next probe attempt. While devm_kcalloc allocates an array for the
doubled size, only the first elements are initialized.

When fill_pin_function() iterates up to the doubled npins, will it call
strcmp(pins[i].pin.name, name) on zero-initialized elements, triggering a
NULL pointer dereference and kernel panic?

> +					   SUNXI_PINCTRL_NCAT3_REG_LAYOUT);
> +}

[Severity: High]
Does the SUNXI_PINCTRL_NCAT3_REG_LAYOUT incorrectly map logical to physical IRQ
banks, breaking all A733 GPIO interrupts?

When an interrupt fires on A733, sunxi_pinctrl_irq_handler() loops over the
logical IRQ bank indices (0 to 9) and calls sunxi_irq_status_reg_from_bank(pctl, bank).

However, the layout logic directly multiplies by the logical bank index:

drivers/pinctrl/sunxi/pinctrl-sunxi.h:sunxi_irq_base_reg() {
    ...
    if (pctl->flags & SUNXI_PINCTRL_NCAT3_REG_LAYOUT)
        return pctl->bank_offset + bank * pctl->bank_mem_size +
               A733_IRQ_REGS_OFFSET;
    ...
}

For the first IRQ bank (PB, logical index 0), this calculates the offset for
physical bank 0 (PA). PA has no pins and no interrupts. The physical registers
for PB are at physical bank index 1. Because the mapping is bypassed, doesn't
this cause the handler to read from the non-existent PA IRQ registers instead
of PB?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910133519.459011-1-andre.przywara@arm.com?part=7