Re: [PATCH 01/10] pinctrl: sunxi: Fix out-of-bounds irq_bank index when first bank has no IRQs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pinctrl: sunxi: off-by-one IRQ bank mapping in prepare_function_table()
--

commit c76ce1661381ff598e234c433e51d5307fae79a4
Author: Lee Jones <lee@kernel.org>

pinctrl: sunxi: Fix out-of-bounds irq_bank index when first bank has no IRQs

This commit initializes irq_bank and last_bank to -1 in
prepare_function_table() and only updates last_bank on IRQ-capable banks so
that irq_bank starts at 0 even when bank 0 (PA) has no pins or interrupts.

> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c b/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
> index 8bcd566593ae8..79ca1a7c48374 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
> @@ -208,8 +208,8 @@ static int prepare_function_table(struct device *dev, struct device_node *pnode,
>  	 * Assign the function's memory and fill in GPIOs, IRQ and a sentinel.
>  	 * The extra functions will be filled in later.
>  	 */
> -	irq_bank = 0;
> -	last_bank = 0;
> +	irq_bank = -1;
> +	last_bank = -1;
>  	for (i = 0; i < npins; i++) {
>  		struct sunxi_desc_pin *pin = &pins[i];
>  		int bank = (pin->pin.number - pin_base) / PINS_PER_BANK;
> @@ -222,17 +222,16 @@ static int prepare_function_table(struct device *dev, struct device_node *pnode,
>  		func[1].muxval = 1;
>  
>  		if (irq_mux) {
> -			if (bank > last_bank)
> +			if (bank > last_bank) {
>  				irq_bank++;
> +				last_bank = bank;
> +			}
>  			func[lastfunc].muxval = irq_mux;
>  			func[lastfunc].irqbank = irq_bank;
>  			func[lastfunc].irqnum = pin->pin.number % PINS_PER_BANK;
>  			func[lastfunc].name = "irq";
>  		}
>  
> -		if (bank > last_bank)
> -			last_bank = bank;

[Severity: High]
Won't assigning irqbank starting at 0 for Port B shift the hardware IRQ
register offsets and GIC interrupt mappings off by one bank on Allwinner A523
and A733?

On Allwinner A523 and A733, Port A (bank 0) has 0 pins, while Ports B through
K (banks 1 through 10) implement pins and interrupts. With this change,
sunxi_pinctrl_dt_table_init() -> prepare_function_table() assigns
func[lastfunc].irqbank = 0..9 to Ports B..K instead of 1..10.

However, as described in commits 11f25ac56372 ("dt-bindings: pinctrl:
sun55i-a523: increase IRQ banks number") and 6b81aa0c8a4f ("arm64: dts:
allwinner: a523: Add missing GPIO interrupt"), the hardware IRQ register
layout and Device Tree interrupt numbering on A523 and A733 start at index 0
for the unimplemented Port A (RAZ/WI), with Ports B..K occupying hardware and
DT indices 1..10.

On Allwinner A523, drivers/pinctrl/sunxi/pinctrl-sun55i-a523.c defines an
identity bank map starting at 0:

    static const unsigned int a523_irq_bank_map[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };

When Ports B..K are given irqbank 0..9 without updating a523_pinctrl_data,
sunxi_irq_base_reg() maps Port B (irqbank 0) via a523_irq_bank_map[0] = 0 to
Port A's unimplemented IRQ registers at offset 0x200 instead of Port B's at
0x220, Port C (irqbank 1) to Port B's registers at 0x220, and so on through
Port K (irqbank 9) accessing Port J's registers at 0x320, leaving Port K's
registers at 0x340 unused.

Furthermore, arch/arm64/boot/dts/allwinner/sun55i-a523.dtsi and
Documentation/devicetree/bindings/pinctrl/allwinner,sun55i-a523-pinctrl.yaml
specify 11 GIC interrupts starting with Port A (GIC_SPI 67) at index 0, Port B
(GIC_SPI 69) at index 1, through Port K (GIC_SPI 140) at index 10:

    interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
                 <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>,
                 ...
                 <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>,
                 <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;

Because sunxi_pinctrl_init_with_flags() populates
pctl->irq[i] = platform_get_irq(pdev, i) for i = 0 .. desc->irq_banks - 1,
assigning irqbank 0..9 to Ports B..K pairs Port B with Port A's GIC_SPI 67
(pctl->irq[0]), Port C with Port B's GIC_SPI 69 (pctl->irq[1]), through Port K
with Port J's GIC_SPI 85 (pctl->irq[9]), while index 10 (GIC_SPI 140 for
Port K) is never requested.

Similarly, on Allwinner A733, drivers/pinctrl/sunxi/pinctrl-sun60i-a733.c
uses SUNXI_PINCTRL_NCAT3_REG_LAYOUT with no irq_bank_map:

    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,
    };

In drivers/pinctrl/sunxi/pinctrl-sunxi.h, sunxi_irq_base_reg() computes the
register offset directly from bank without calling sunxi_irq_hw_bank_num():

    static inline u32 sunxi_irq_base_reg(const struct sunxi_pinctrl *pctl, u16 bank)
    {
            if (pctl->flags & SUNXI_PINCTRL_NCAT3_REG_LAYOUT)
                    return pctl->bank_offset + bank * pctl->bank_mem_size +
                           A733_IRQ_REGS_OFFSET;

            return IRQ_REGS_OFFSET +
                   sunxi_irq_hw_bank_num(pctl->desc, bank) * IRQ_MEM_SIZE;
    }

Because pctl->bank_offset (0x80) is Port A's register base (IRQ-A at 0x0c0 for
bank 0) and Port B's IRQ registers are at bank 1 (0x140), assigning irqbank
0..9 to Ports B..K shifts all A733 IRQ register accesses down by one bank to
0x0c0..0x540 (Ports A..J) instead of 0x140..0x5c0 (Ports B..K).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103213002.1673560-1-lee@kernel.org?part=1