Refer ARM DDI 0487G.b, CNTP_CTL_EL0,
ISTATUS, bit [2] - When the value of the ENABLE bit is 1, ISTATUS
indicates whether the timer condition is met.
Thus, one need to check ENABLE bit along with ISTATUS, to confirm
whether the timer condition is met. Further as the doc says,
"When the value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN."
Signed-off-by: Ayan Kumar Halder <[email protected]>
---
Please refer to https://lore.kernel.org/all/[email protected]/T/
for the previous discussion on this issue on xen-devel mailing list.
drivers/clocksource/arm_arch_timer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 9ab8221ee3c6..96921772814c 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -647,7 +647,7 @@ static __always_inline irqreturn_t timer_handler(const int access,
unsigned long ctrl;
ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, evt);
- if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
+ if ((ctrl & ARCH_TIMER_CTRL_IT_STAT) && (ctrl & ARCH_TIMER_CTRL_ENABLE)) {
ctrl |= ARCH_TIMER_CTRL_IT_MASK;
arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, evt);
evt->event_handler(evt);
--
2.17.1
On Thu, 11 Aug 2022 10:36:20 +0100,
Ayan Kumar Halder <[email protected]> wrote:
>
> Refer ARM DDI 0487G.b, CNTP_CTL_EL0,
> ISTATUS, bit [2] - When the value of the ENABLE bit is 1, ISTATUS
> indicates whether the timer condition is met.
>
> Thus, one need to check ENABLE bit along with ISTATUS, to confirm
> whether the timer condition is met. Further as the doc says,
> "When the value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN."
>
> Signed-off-by: Ayan Kumar Halder <[email protected]>
> ---
>
> Please refer to https://lore.kernel.org/all/[email protected]/T/
> for the previous discussion on this issue on xen-devel mailing list.
>
> drivers/clocksource/arm_arch_timer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 9ab8221ee3c6..96921772814c 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -647,7 +647,7 @@ static __always_inline irqreturn_t timer_handler(const int access,
> unsigned long ctrl;
>
> ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, evt);
> - if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
> + if ((ctrl & ARCH_TIMER_CTRL_IT_STAT) && (ctrl & ARCH_TIMER_CTRL_ENABLE)) {
> ctrl |= ARCH_TIMER_CTRL_IT_MASK;
> arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, evt);
> evt->event_handler(evt);
And how can the timer be disabled if we're in the interrupt handler?
M.
--
Without deviation from the norm, progress is not possible.
Hi Marc,
Apologies, this got lost in my mailbox.
On 11/08/2022 10:49, Marc Zyngier wrote:
> On Thu, 11 Aug 2022 10:36:20 +0100,
> Ayan Kumar Halder <[email protected]> wrote:
>> Refer ARM DDI 0487G.b, CNTP_CTL_EL0,
>> ISTATUS, bit [2] - When the value of the ENABLE bit is 1, ISTATUS
>> indicates whether the timer condition is met.
>>
>> Thus, one need to check ENABLE bit along with ISTATUS, to confirm
>> whether the timer condition is met. Further as the doc says,
>> "When the value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN."
>>
>> Signed-off-by: Ayan Kumar Halder <[email protected]>
>> ---
>>
>> Please refer to https://lore.kernel.org/all/[email protected]/T/
>> for the previous discussion on this issue on xen-devel mailing list.
>>
>> drivers/clocksource/arm_arch_timer.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index 9ab8221ee3c6..96921772814c 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -647,7 +647,7 @@ static __always_inline irqreturn_t timer_handler(const int access,
>> unsigned long ctrl;
>>
>> ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, evt);
>> - if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
>> + if ((ctrl & ARCH_TIMER_CTRL_IT_STAT) && (ctrl & ARCH_TIMER_CTRL_ENABLE)) {
>> ctrl |= ARCH_TIMER_CTRL_IT_MASK;
>> arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, evt);
>> evt->event_handler(evt);
> And how can the timer be disabled if we're in the interrupt handler?
I am not very sure about this.
Is it possible for a pending interrupt to arrive just after the timer
has been disabled ?
- Ayan
>
> M.
>
On Thu, 25 May 2023 17:03:11 +0100,
Ayan Kumar Halder <[email protected]> wrote:
>
> Hi Marc,
>
> Apologies, this got lost in my mailbox.
>
> On 11/08/2022 10:49, Marc Zyngier wrote:
> > On Thu, 11 Aug 2022 10:36:20 +0100,
> > Ayan Kumar Halder <[email protected]> wrote:
> >> Refer ARM DDI 0487G.b, CNTP_CTL_EL0,
> >> ISTATUS, bit [2] - When the value of the ENABLE bit is 1, ISTATUS
> >> indicates whether the timer condition is met.
> >>
> >> Thus, one need to check ENABLE bit along with ISTATUS, to confirm
> >> whether the timer condition is met. Further as the doc says,
> >> "When the value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN."
> >>
> >> Signed-off-by: Ayan Kumar Halder <[email protected]>
> >> ---
> >>
> >> Please refer to https://lore.kernel.org/all/[email protected]/T/
> >> for the previous discussion on this issue on xen-devel mailing list.
> >>
> >> drivers/clocksource/arm_arch_timer.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> >> index 9ab8221ee3c6..96921772814c 100644
> >> --- a/drivers/clocksource/arm_arch_timer.c
> >> +++ b/drivers/clocksource/arm_arch_timer.c
> >> @@ -647,7 +647,7 @@ static __always_inline irqreturn_t timer_handler(const int access,
> >> unsigned long ctrl;
> >> ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, evt);
> >> - if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
> >> + if ((ctrl & ARCH_TIMER_CTRL_IT_STAT) && (ctrl & ARCH_TIMER_CTRL_ENABLE)) {
> >> ctrl |= ARCH_TIMER_CTRL_IT_MASK;
> >> arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, evt);
> >> evt->event_handler(evt);
> > And how can the timer be disabled if we're in the interrupt handler?
>
> I am not very sure about this.
>
> Is it possible for a pending interrupt to arrive just after the timer
> has been disabled ?
I can't see how. This is a level interrupt, so disabling it at the
source also retires the pending bit from the GIC. And if what you
describe actually happens, this is only a spurious interrupt and most
probably a slightly broken interrupt controller implementation.
Overall, this change doesn't make much sense, unless you can show me a
HW implementation that is broken enough that this leads to actual
problems.
M.
--
Without deviation from the norm, progress is not possible.