* [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq
@ 2022-08-11 9:36 ` Ayan Kumar Halder
0 siblings, 0 replies; 8+ messages in thread
From: Ayan Kumar Halder @ 2022-08-11 9:36 UTC (permalink / raw)
To: stefano.stabellini, sstabellini, julien, bertrand.marquis,
mark.rutland, maz, daniel.lezcano, tglx
Cc: linux-arm-kernel, linux-kernel, Ayan Kumar Halder
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq
@ 2022-08-11 9:36 ` Ayan Kumar Halder
0 siblings, 0 replies; 8+ messages in thread
From: Ayan Kumar Halder @ 2022-08-11 9:36 UTC (permalink / raw)
To: stefano.stabellini, sstabellini, julien, bertrand.marquis,
mark.rutland, maz, daniel.lezcano, tglx
Cc: linux-arm-kernel, linux-kernel, Ayan Kumar Halder
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq
2022-08-11 9:36 ` Ayan Kumar Halder
@ 2022-08-11 9:49 ` Marc Zyngier
-1 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2022-08-11 9:49 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: stefano.stabellini, sstabellini, julien, bertrand.marquis,
mark.rutland, daniel.lezcano, tglx, linux-arm-kernel,
linux-kernel
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq
@ 2022-08-11 9:49 ` Marc Zyngier
0 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2022-08-11 9:49 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: stefano.stabellini, sstabellini, julien, bertrand.marquis,
mark.rutland, daniel.lezcano, tglx, linux-arm-kernel,
linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq
2022-08-11 9:49 ` Marc Zyngier
@ 2023-05-25 16:03 ` Ayan Kumar Halder
-1 siblings, 0 replies; 8+ messages in thread
From: Ayan Kumar Halder @ 2023-05-25 16:03 UTC (permalink / raw)
To: Marc Zyngier
Cc: stefano.stabellini, sstabellini, julien, bertrand.marquis,
mark.rutland, daniel.lezcano, tglx, linux-arm-kernel,
linux-kernel
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.
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq
@ 2023-05-25 16:03 ` Ayan Kumar Halder
0 siblings, 0 replies; 8+ messages in thread
From: Ayan Kumar Halder @ 2023-05-25 16:03 UTC (permalink / raw)
To: Marc Zyngier
Cc: stefano.stabellini, sstabellini, julien, bertrand.marquis,
mark.rutland, daniel.lezcano, tglx, linux-arm-kernel,
linux-kernel
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.
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq
2023-05-25 16:03 ` Ayan Kumar Halder
@ 2023-05-25 16:44 ` Marc Zyngier
-1 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2023-05-25 16:44 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: stefano.stabellini, sstabellini, julien, bertrand.marquis,
mark.rutland, daniel.lezcano, tglx, linux-arm-kernel,
linux-kernel
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq
@ 2023-05-25 16:44 ` Marc Zyngier
0 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2023-05-25 16:44 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: stefano.stabellini, sstabellini, julien, bertrand.marquis,
mark.rutland, daniel.lezcano, tglx, linux-arm-kernel,
linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-05-25 16:45 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-11 9:36 [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq Ayan Kumar Halder
2022-08-11 9:36 ` Ayan Kumar Halder
2022-08-11 9:49 ` Marc Zyngier
2022-08-11 9:49 ` Marc Zyngier
2023-05-25 16:03 ` Ayan Kumar Halder
2023-05-25 16:03 ` Ayan Kumar Halder
2023-05-25 16:44 ` Marc Zyngier
2023-05-25 16:44 ` Marc Zyngier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.