All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: Ayan Kumar Halder <ayankuma@amd.com>
Cc: stefano.stabellini@xilinx.com, sstabellini@kernel.org,
	julien@xen.org, bertrand.marquis@arm.com, mark.rutland@arm.com,
	daniel.lezcano@linaro.org, tglx@linutronix.de,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq
Date: Thu, 25 May 2023 17:44:31 +0100	[thread overview]
Message-ID: <86mt1scqds.wl-maz@kernel.org> (raw)
In-Reply-To: <0c29756d-76ff-1fd5-09bb-0b13a5fddffd@amd.com>

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

WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org>
To: Ayan Kumar Halder <ayankuma@amd.com>
Cc: stefano.stabellini@xilinx.com, sstabellini@kernel.org,
	julien@xen.org, bertrand.marquis@arm.com, mark.rutland@arm.com,
	daniel.lezcano@linaro.org, tglx@linutronix.de,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] arm: clocksource: Check if timer is enabled for timer irq
Date: Thu, 25 May 2023 17:44:31 +0100	[thread overview]
Message-ID: <86mt1scqds.wl-maz@kernel.org> (raw)
In-Reply-To: <0c29756d-76ff-1fd5-09bb-0b13a5fddffd@amd.com>

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.

  reply	other threads:[~2023-05-25 16:45 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2023-05-25 16:44       ` Marc Zyngier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=86mt1scqds.wl-maz@kernel.org \
    --to=maz@kernel.org \
    --cc=ayankuma@amd.com \
    --cc=bertrand.marquis@arm.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=julien@xen.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=sstabellini@kernel.org \
    --cc=stefano.stabellini@xilinx.com \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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.