Re: [PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing

[Cédric Le Goater]

On 9/25/21 16:28, Peter Delevoryas wrote:
> > On Sep 25, 2021, at 4:03 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > Hi Peter,
> >
> > > On 9/24/21 08:19, pdel@fb.com wrote:
> > > From: Peter Delevoryas <pdel@fb.com>
> > > The gpio array is declared as a dense array:
> > > ...
> > > qemu_irq gpios[ASPEED_GPIO_NR_PINS];
> > > (AST2500 has 228, AST2400 has 216, AST2600 has 208)
> > > However, this array is used like a matrix of GPIO sets
> > > (e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])
> > > size_t offset = set * GPIOS_PER_SET + gpio;
> > > qemu_set_irq(s->gpios[offset], !!(new & mask));
> > > This can result in an out-of-bounds access to "s->gpios" because the
> > > gpio sets do _not_ have the same length. Some of the groups (e.g.
> > > GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.
> > > To fix this, I converted the gpio array from dense to sparse, to that
> > > match both the hardware layout and this existing indexing code.
> >
> > This is one logical change: 1 patch
> >
> > > Also, I noticed that some of the property specifications looked wrong:
> > > the lower 8 bits in the input and output u32's correspond to the first
> > > group label, and the upper 8 bits correspond to the last group label.
> >
> > Another logical change: another patch :)
> >
> > So please split this patch in 2. Maybe easier to fix GPIOSetProperties
> > first, then convert from dense to sparse array.
>
> Good point, I’ll split it up then!

Yes. We can surely merge the GPIOSetProperties patch quickly.

I hope Rashmica has some time to check the second part.
 
Thanks,

C.


> > Regards,
> >
> > Phil.
> >
> > > I looked at the datasheet and several of these declarations seemed
> > > incorrect to me (I was basing it off of the I/O column). If somebody
> > > can double-check this, I'd really appreciate it!
> > > Some were definitely wrong though, like "Y" and "Z" in the 2600.
> > > @@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
> > >       [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
> > >       [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
> > >       [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> > > -    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
> > > +    [6] = {0x0fffffff,  0x0fffffff,  {"Y", "Z", "AA", "AB"} },
> > >       [7] = {0x000000ff,  0x000000ff,  {"AC"} },
> > >   };
> > > @@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
> > >       [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
> > >       [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
> > >       [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
> > > -    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
> > > -    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> > > -    [6] = {0xffff0000,  0x0fff0000,  {"Y", "Z", "", ""} },
> > > +    [4] = {0xffffffff,  0x00ffffff,  {"Q", "R", "S", "T"} },
> > > +    [5] = {0xffffffff,  0xffffff00,  {"U", "V", "W", "X"} },
> > > +    [6] = {0x0000ffff,  0x0000ffff,  {"Y", "Z", "", ""} },
> > >   };
> > > Signed-off-by: Peter Delevoryas <pdel@fb.com>
> > > ---
> > >   hw/gpio/aspeed_gpio.c         | 80 +++++++++++++++--------------------
> > >   include/hw/gpio/aspeed_gpio.h |  5 +--
> > >   2 files changed, 35 insertions(+), 50 deletions(-)