Re: [PATCH 1/1] hw/intc/aspeed: Fix IRQ handler mask check

["Cédric Le Goater" <clg@kaod.org>]

Hello Steven,

On 3/21/25 03:54, Steven Lee wrote:
> Hi Cédric,
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@kaod.org>
>> Sent: Thursday, March 20, 2025 11:29 PM
>> To: Steven Lee <steven_lee@aspeedtech.com>; Peter Maydell
>> <peter.maydell@linaro.org>; Troy Lee <leetroy@gmail.com>; Jamin Lin
>> <jamin_lin@aspeedtech.com>; Andrew Jeffery
>> <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>; open
>> list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here
>> <qemu-devel@nongnu.org>
>> Cc: Troy Lee <troy_lee@aspeedtech.com>; longzl2@lenovo.com; Yunlin Tang
>> <yunlin.tang@aspeedtech.com>
>> Subject: Re: [PATCH 1/1] hw/intc/aspeed: Fix IRQ handler mask check
>>
>> Hello Steven,
>>
>> On 3/20/25 10:25, Steven Lee wrote:
>>> Updated the IRQ handler mask check to AND with select variable.
>>> This ensures that the interrupt service routine is correctly triggered
>>> for the interrupts within the same irq group.
>>>
>>> For example, both `eth0` and the debug UART are handled in `GICINT132`.
>>> Without this fix, the debug console may hang if the `eth0` ISR is not
>>> handled.
>>>
>>> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
>>> Change-Id: Ic3609eb72218dfd68be6057d78b8953b18828709
>>> ---
>>
>> I think the issue was introduced by the initial commit :
>>
>>     d831c5fd8682 ("aspeed/intc: Add AST2700 support")
>>
>> Is that correct ?
>>
> 
> Yes, and the implementation is based on commit 38ba38d (hw/intc/aspeed: Add Support for AST2700 INTCIO Controller).
>
> Additionally, I sent a patch series for AST27x0 multi-SoC support on March 13th. However, I forgot to append the v2 tag to the series. 

That's OK.

We are in -rc phase of the QEMU 10.0 cycle, so I am looking at fixes
first.

> Should I resend it with the correct version tag?

No need. This is enough:

Fixes: d831c5fd8682 ("aspeed/intc: Add AST2700 support")



Thanks,

C.




> Patchwork link:
> https://patchwork.kernel.org/project/qemu-devel/list/?series=943379
> 
> Thanks,
> Steven
> 
>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>>
>> Thanks,
>>
>> C.
>>
>>
>>>    hw/intc/aspeed_intc.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index
>>> 3fd417084f..f17bf43925 100644
>>> --- a/hw/intc/aspeed_intc.c
>>> +++ b/hw/intc/aspeed_intc.c
>>> @@ -111,7 +111,7 @@ static void
>> aspeed_intc_set_irq_handler(AspeedINTCState *s,
>>>        outpin_idx = intc_irq->outpin_idx;
>>>        inpin_idx = intc_irq->inpin_idx;
>>>
>>> -    if (s->mask[inpin_idx] || s->regs[status_reg]) {
>>> +    if ((s->mask[inpin_idx] & select) || (s->regs[status_reg] &
>>> + select)) {
>>>            /*
>>>             * a. mask is not 0 means in ISR mode
>>>             * sources interrupt routine are executing.
>