arm: clocksource: Check if timer is enabled for timer irq

[PATCH v1] arm: clocksource: Check if timer is enabled for timer irq

Posted by Ayan Kumar Halder 3 years, 8 months ago

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 <ayankuma@amd.com>
---

Please refer to https://lore.kernel.org/all/6cfcd4fa-3afd-1c70-6a70-9df557ee1811@xen.org/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

Re: [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq

Posted by Marc Zyngier 3 years, 8 months ago

On Thu, 11 Aug 2022 10:36:20 +0100,
Ayan Kumar Halder <ayankuma@amd.com> 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 <ayankuma@amd.com>
> ---
> 
> Please refer to https://lore.kernel.org/all/6cfcd4fa-3afd-1c70-6a70-9df557ee1811@xen.org/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.

Re: [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq

Posted by Ayan Kumar Halder 2 years, 10 months ago

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 <ayankuma@amd.com> 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 <ayankuma@amd.com>
>> ---
>>
>> Please refer to https://lore.kernel.org/all/6cfcd4fa-3afd-1c70-6a70-9df557ee1811@xen.org/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.
>

Re: [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq

Posted by Marc Zyngier 2 years, 10 months ago

On Thu, 25 May 2023 17:03:11 +0100,
Ayan Kumar Halder <ayankuma@amd.com> 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 <ayankuma@amd.com> 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 <ayankuma@amd.com>
> >> ---
> >> 
> >> Please refer to https://lore.kernel.org/all/6cfcd4fa-3afd-1c70-6a70-9df557ee1811@xen.org/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.