Re: [PATCH v2 4/6] hw/gpio/aspeed: Fix clear incorrect interrupt status for GPIO index mode

[Andrew Jeffery <andrew@codeconstruct.com.au>]

Hi Jamin,

On Wed, 2024-09-25 at 11:34 +0800, Jamin Lin wrote:
> The interrupt status field is W1C, where a set bit on read indicates an
> interrupt is pending. If the bit extracted from data is set it should
> clear the corresponding bit in group_value. However, if the extracted
> bit is clear then the value of the corresponding bit in group_value
> should be unchanged.
> 
> SHARED_FIELD_EX32() extracts the interrupt status bit from the write
> (data). group_value is set to the set's interrupt status, which means
> that for any pin with an interrupt pending, the corresponding bit is
> set. The deposit32() call updates the bit at pin_idx in the group,
> using the value extracted from the write (data).
> 
> The result is that if multiple interrupt status bits
> were pending and the write was acknowledging specific one bit,
> then the all interrupt status bits will be cleared.
> However, it is index mode and should only clear the corresponding bit.
> 
> For example, say we have an interrupt pending for GPIOA0, where the
> following statements are true:
> 
>    set->int_status == 0b01
>    s->pending == 1
> 
> Before it is acknowledged, an interrupt becomes pending for GPIOA1:
> 
>    set->int_status == 0b11
>    s->pending == 2
> 
> A write is issued to acknowledge the interrupt for GPIOA0. This causes
> the following sequence:
> 
>    reg_value == 0b11
>    pending == 2
>    s->pending = 0

Note that I had a typo in my reply that prompted this patch, this was
meant to be:

    s->pending == 0

>    set->int_status == 0b00
> 
> It should only clear bit 0 in index mode and the correct result
> should be as following.
> 
>    set->int_status == 0b11
>    s->pending == 2
> 
>    pending == 1
>    s->pending = 1

Same typo here now :)

    s->pending == 1

>    set->int_status == 0b10
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>  hw/gpio/aspeed_gpio.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index 6e6ab48b56..58ae63e3c1 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -642,7 +642,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
>      uint32_t pin_idx = reg_idx_number % ASPEED_GPIOS_PER_SET;
>      uint32_t group_idx = pin_idx / GPIOS_PER_GROUP;
>      uint32_t reg_value = 0;
> -    uint32_t cleared;
> +    uint32_t pending = 0;
>  
>      set = &s->sets[set_idx];
>      props = &agc->props[set_idx];
> @@ -705,15 +705,16 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
>          set->int_sens_2 = update_value_control_source(set, set->int_sens_2,
>                                                        reg_value);
>          /* set interrupt status */
> -        reg_value = set->int_status;
> -        reg_value = deposit32(reg_value, pin_idx, 1,
> -                              FIELD_EX32(data, GPIO_INDEX_REG, INT_STATUS));
> -        cleared = ctpop32(reg_value & set->int_status);
> -        if (s->pending && cleared) {
> -            assert(s->pending >= cleared);
> -            s->pending -= cleared;
> +        if (FIELD_EX32(data, GPIO_INDEX_REG, INT_STATUS)) {
> +            pending = extract32(set->int_status, pin_idx, 1);
> +            if (pending) {
> +                if (s->pending) {
> +                    assert(s->pending >= pending);
> +                    s->pending -= pending;
> +                }
> +                set->int_status = deposit32(set->int_status, pin_idx, 1, 0);
> +            }

So I think we can get away without the nested conditionals?

   if (FIELD_EX32(data, GPIO_INDEX_REG, INT_STATUS)) {
       /* pending is either 1 or 0 for a 1-bit field */
       pending = extract32(set->int_status, pin_idx, 1);
       
       /*
        * The assert is effectively a compressed form of
        *
        *     assert((s->pending == 0 && pending == 0) ||
        *            (s->pending >= 1));
        *
        * This seems reasonable.
        *
        * Another way to write it is:
        *
        *     assert(!pending || s->pending));
        */
       assert(s->pending >= pending);
   
       /* No change to s->pending if pending is 0 */
       s->pending -= pending;
   
       /*
        * The write acknowledged the interrupt regardless of whether it
        * was pending or not. The post-condition is that it mustn't be
        * pending. Unconditionally clear the status bit.
        */
       s->int_status = deposit32(set->int_status, pin_idx, 1, 0);
   }
   
Thoughts?

Up to you whether you keep the comments if the idea makes sense and you
choose to adopt it.

Andrew