Re: [PATCH V8 1/3] irq: Add flags to request_percpu_irq function
On Thu, Mar 23, 2017 at 06:42:01PM +0100, Daniel Lezcano wrote: > diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c > index da1f798..dbdb622 100644 > --- a/drivers/clocksource/timer-nps.c > +++ b/drivers/clocksource/timer-nps.c > @@ -256,7 +256,7 @@ static int __init nps_setup_clockevent(struct device_node > *node) > return ret; > > /* Needs apriori irq_set_percpu_devid() done in intc map function */ > - ret = request_percpu_irq(nps_timer0_irq, timer_irq_handler, > + ret = request_percpu_irq(nps_timer0_irq, timer_irq_handler, IRQF_TIMER, >"Timer0 (per-cpu-tick)", >&nps_clockevent_device); Wrong parameter order here. drew
Re: [PATCH V8 1/3] irq: Add flags to request_percpu_irq function
Hi Mark,
On Thu, Mar 23, 2017 at 06:54:52PM +, Mark Rutland wrote:
> Hi Daniel,
>
> On Thu, Mar 23, 2017 at 06:42:01PM +0100, Daniel Lezcano wrote:
> > In the next changes, we track the interrupts but we discard the timers as
> > that does not make sense. The next interrupt on a timer is predictable.
>
> Sorry, but I could not parse this.
I meant we are measuring when are happening the interrupts by getting the local
clock in the interrupt handler. But if the interrupts are coming from a timer,
it
is not necessary to do that because we already know when they will occur.
So, in order to sort out which interrupt we measure, we use the IRQF_TIMER flag.
Unfortunately, this flag is missing when we do a request_percpu_irq. The
purpose of this patch is to fix that.
> > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> > index 9612b84..0f5ab4a 100644
> > --- a/drivers/perf/arm_pmu.c
> > +++ b/drivers/perf/arm_pmu.c
> > @@ -661,7 +661,7 @@ static int cpu_pmu_request_irq(struct arm_pmu *cpu_pmu,
> > irq_handler_t handler)
> >
> > irq = platform_get_irq(pmu_device, 0);
> > if (irq > 0 && irq_is_percpu(irq)) {
> > - err = request_percpu_irq(irq, handler, "arm-pmu",
> > + err = request_percpu_irq(irq, 0, handler, "arm-pmu",
> > &hw_events->percpu_pmu);
> > if (err) {
> > pr_err("unable to request IRQ%d for ARM PMU counters\n",
>
> Please Cc myself and Will Deacon when modifying the arm_pmu driver, as
> per MAINTAINERS. I only spotted this patch by chance.
Ah, ok, sorry for that. Thanks for spotting this, you should have been Cc'ed by
my cccmd script. I will check that.
> This conflicts with arm_pmu changes I have queued for v4.12 [1].
>
> So, can we leave the prototype of request_percpu_irq() as-is?
>
> Why not add a new request_percpu_irq_flags() function, and leave
> request_percpu_irq() as a wrapper for that? e.g.
[ ... ]
> static inline int
> request_percpu_irq(unsigned int irq, irq_handler_t handler,
> const char *devname, void __percpu *percpu_dev_id)
> {
> return request_percpu_irq_flags(irq, handler, devname,
> percpu_dev_id, 0);
> }
>
> ... that would avoid having to touch any non-timer driver for now.
Mmh, yes. That's a good suggestion.
> [...]
>
> > -request_percpu_irq(unsigned int irq, irq_handler_t handler,
> > - const char *devname, void __percpu *percpu_dev_id);
> > +request_percpu_irq(unsigned int irq, unsigned long flags,
> > + irq_handler_t handler, const char *devname,
> > + void __percpu *percpu_dev_id);
> >
>
> Looking at request_irq, the prototype is:
>
> int __must_check
> request_irq(unsigned int irq, irq_handler_t handler,
> unsigned long flags, const char *name,
> void *dev);
>
> ... surely it would be better to share the same argument order? i.e.
>
> int __must_check
> request_percpu_irq(unsigned int irq, irq_handler_t handler,
> unsigned long flags, const char *devname,
> void __percpu *percpu_dev_id);
>
Agree.
Thanks for the review.
-- Daniel
--
Facebook |
Twitter |
Re: [PATCH V8 1/3] irq: Add flags to request_percpu_irq function
Hi Daniel,
On Thu, Mar 23, 2017 at 06:42:01PM +0100, Daniel Lezcano wrote:
> In the next changes, we track the interrupts but we discard the timers as
> that does not make sense. The next interrupt on a timer is predictable.
Sorry, but I could not parse this.
[...]
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 9612b84..0f5ab4a 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -661,7 +661,7 @@ static int cpu_pmu_request_irq(struct arm_pmu *cpu_pmu,
> irq_handler_t handler)
>
> irq = platform_get_irq(pmu_device, 0);
> if (irq > 0 && irq_is_percpu(irq)) {
> - err = request_percpu_irq(irq, handler, "arm-pmu",
> + err = request_percpu_irq(irq, 0, handler, "arm-pmu",
>&hw_events->percpu_pmu);
> if (err) {
> pr_err("unable to request IRQ%d for ARM PMU counters\n",
Please Cc myself and Will Deacon when modifying the arm_pmu driver, as
per MAINTAINERS. I only spotted this patch by chance.
This conflicts with arm_pmu changes I have queued for v4.12 [1].
So, can we leave the prototype of request_percpu_irq() as-is?
Why not add a new request_percpu_irq_flags() function, and leave
request_percpu_irq() as a wrapper for that? e.g.
static inline int
request_percpu_irq(unsigned int irq, irq_handler_t handler,
const char *devname, void __percpu *percpu_dev_id)
{
return request_percpu_irq_flags(irq, handler, devname,
percpu_dev_id, 0);
}
... that would avoid having to touch any non-timer driver for now.
[...]
> -request_percpu_irq(unsigned int irq, irq_handler_t handler,
> -const char *devname, void __percpu *percpu_dev_id);
> +request_percpu_irq(unsigned int irq, unsigned long flags,
> +irq_handler_t handler, const char *devname,
> +void __percpu *percpu_dev_id);
>
Looking at request_irq, the prototype is:
int __must_check
request_irq(unsigned int irq, irq_handler_t handler,
unsigned long flags, const char *name,
void *dev);
... surely it would be better to share the same argument order? i.e.
int __must_check
request_percpu_irq(unsigned int irq, irq_handler_t handler,
unsigned long flags, const char *devname,
void __percpu *percpu_dev_id);
Thanks,
Mark.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm/perf/refactoring
Re: [PATCH V8 1/3] irq: Add flags to request_percpu_irq function
On 03/23/2017 10:42 AM, Daniel Lezcano wrote: > In the next changes, we track the interrupts but we discard the timers as > that does not make sense. The next interrupt on a timer is predictable. > > But, the API request_percpu_irq does not allow to pass a flag, hence > specifying > if the interrupt type is a timer. > > Solve this by passing a 'flags' parameter to the function and change all the > callers to pass IRQF_TIMER when the interrupt is a timer interrupt, zero > otherwise. > > For now, in order to prevent a misusage of this parameter, only the IRQF_TIMER > flag is a valid parameter to be passed to the request_percpu_irq function. > > Signed-off-by: Daniel Lezcano Acked-by: Vineet Gupta# for arch/arc, arc_timer bits
[PATCH V8 1/3] irq: Add flags to request_percpu_irq function
In the next changes, we track the interrupts but we discard the timers as
that does not make sense. The next interrupt on a timer is predictable.
But, the API request_percpu_irq does not allow to pass a flag, hence specifying
if the interrupt type is a timer.
Solve this by passing a 'flags' parameter to the function and change all the
callers to pass IRQF_TIMER when the interrupt is a timer interrupt, zero
otherwise.
For now, in order to prevent a misusage of this parameter, only the IRQF_TIMER
flag is a valid parameter to be passed to the request_percpu_irq function.
Signed-off-by: Daniel Lezcano
---
arch/arc/kernel/perf_event.c | 2 +-
arch/arc/kernel/smp.c| 2 +-
arch/arm/kernel/smp_twd.c| 3 ++-
arch/arm/xen/enlighten.c | 2 +-
drivers/clocksource/arc_timer.c | 2 +-
drivers/clocksource/arm_arch_timer.c | 15 +--
drivers/clocksource/arm_global_timer.c | 2 +-
drivers/clocksource/exynos_mct.c | 2 +-
drivers/clocksource/qcom-timer.c | 2 +-
drivers/clocksource/time-armada-370-xp.c | 2 +-
drivers/clocksource/timer-nps.c | 2 +-
drivers/net/ethernet/marvell/mvneta.c| 2 +-
drivers/perf/arm_pmu.c | 2 +-
include/linux/interrupt.h| 5 +++--
kernel/irq/manage.c | 11 ---
virt/kvm/arm/arch_timer.c| 5 +++--
virt/kvm/arm/vgic/vgic-init.c| 2 +-
17 files changed, 37 insertions(+), 26 deletions(-)
diff --git a/arch/arc/kernel/perf_event.c b/arch/arc/kernel/perf_event.c
index 2ce24e7..2a90c7a 100644
--- a/arch/arc/kernel/perf_event.c
+++ b/arch/arc/kernel/perf_event.c
@@ -525,7 +525,7 @@ static int arc_pmu_device_probe(struct platform_device
*pdev)
arc_pmu->irq = irq;
/* intc map function ensures irq_set_percpu_devid() called */
- request_percpu_irq(irq, arc_pmu_intr, "ARC perf counters",
+ request_percpu_irq(irq, 0, arc_pmu_intr, "ARC perf counters",
this_cpu_ptr(&arc_pmu_cpu));
on_each_cpu(arc_cpu_pmu_irq_init, &irq, 1);
diff --git a/arch/arc/kernel/smp.c b/arch/arc/kernel/smp.c
index f462671..5cdd3c9 100644
--- a/arch/arc/kernel/smp.c
+++ b/arch/arc/kernel/smp.c
@@ -381,7 +381,7 @@ int smp_ipi_irq_setup(int cpu, irq_hw_number_t hwirq)
if (!cpu) {
int rc;
- rc = request_percpu_irq(virq, do_IPI, "IPI Interrupt", dev);
+ rc = request_percpu_irq(virq, 0, do_IPI, "IPI Interrupt", dev);
if (rc)
panic("Percpu IRQ request failed for %u\n", virq);
}
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 895ae51..988f9b9 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -332,7 +332,8 @@ static int __init twd_local_timer_common_register(struct
device_node *np)
goto out_free;
}
- err = request_percpu_irq(twd_ppi, twd_handler, "twd", twd_evt);
+ err = request_percpu_irq(twd_ppi, IRQF_TIMER, twd_handler, "twd",
+twd_evt);
if (err) {
pr_err("twd: can't register interrupt %d (%d)\n", twd_ppi, err);
goto out_free;
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 81e3217..2897f94 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -400,7 +400,7 @@ static int __init xen_guest_init(void)
xen_init_IRQ();
- if (request_percpu_irq(xen_events_irq, xen_arm_callback,
+ if (request_percpu_irq(xen_events_irq, 0, xen_arm_callback,
"events", &xen_vcpu)) {
pr_err("Error request IRQ %d\n", xen_events_irq);
return -EINVAL;
diff --git a/drivers/clocksource/arc_timer.c b/drivers/clocksource/arc_timer.c
index 7517f95..e78e306 100644
--- a/drivers/clocksource/arc_timer.c
+++ b/drivers/clocksource/arc_timer.c
@@ -301,7 +301,7 @@ static int __init arc_clockevent_setup(struct device_node
*node)
}
/* Needs apriori irq_set_percpu_devid() done in intc map function */
- ret = request_percpu_irq(arc_timer_irq, timer_irq_handler,
+ ret = request_percpu_irq(arc_timer_irq, IRQF_TIMER, timer_irq_handler,
"Timer0 (per-cpu-tick)", evt);
if (ret) {
pr_err("clockevent: unable to request irq\n");
diff --git a/drivers/clocksource/arm_arch_timer.c
b/drivers/clocksource/arm_arch_timer.c
index 7a8a411..11398ff 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -768,16 +768,19 @@ static int __init arch_timer_register(void)
ppi = arch_timer_ppi[arch_timer_uses_ppi];
switch (arch_timer_uses_ppi) {
case VIRT_PPI:
- err = request_percpu_irq(ppi, arch_timer_handler_virt,
-
