Many clockevent drivers are using a switch block for handling modes in their ->set_mode() callback. Some of these do not have a 'default' case and adding a new mode in the 'enum clock_event_mode', starts giving following warnings for these platforms about unhandled modes (e.g. XXX).
warning: enumeration value ‘XXX’ not handled in switch [-Wswitch]
This patch adds default cases for them.
In order to keep things simple, add following to the switch blocks:
default: break;
This can lead to different behavior for individual cases.
1) Some of the drivers don't have any special stuff in their ->set_mode() callback before or after the switch blocks. And so this default case would simply return for them without any updates to the clockevent device.
2) But in some cases, the clockevent device is disabled/stopped as soon as we enter the ->set_mode() callback and is enabled within the switch block or after it. And the clockevent device *may* stay disabled for default case.
The rationale behind only adding a 'break' was that the default case *will never* be hit during execution of code. All new modes (beyond RESUME) are handled with mode specific ->set_mode_*() callbacks and ->set_mode() is never called for them. And all modes before and including RESUME are already explicitly handled by the clockevent drivers.
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Rebased over: tip/master, due to recently added rockchip driver. 8e3210338295 Merge branch 'x86/urgent'
arch/arm/mach-at91/at91rm9200_time.c | 2 ++ arch/arm/mach-davinci/time.c | 2 ++ arch/arm/mach-footbridge/dc21285-timer.c | 2 ++ arch/arm/mach-imx/epit.c | 2 ++ arch/arm/mach-imx/time.c | 2 ++ arch/arm/mach-lpc32xx/timer.c | 2 ++ arch/arm/mach-mmp/time.c | 2 ++ arch/arm/mach-omap1/time.c | 2 ++ arch/arm/mach-omap1/timer32k.c | 2 ++ arch/arm/mach-omap2/timer.c | 2 ++ arch/arm/mach-sa1100/time.c | 2 ++ arch/arm/mach-w90x900/time.c | 2 ++ arch/blackfin/kernel/time-ts.c | 4 ++++ arch/c6x/platforms/timer64.c | 2 ++ arch/m68k/coldfire/pit.c | 2 ++ arch/microblaze/kernel/timer.c | 2 ++ arch/mips/kernel/cevt-bcm1480.c | 2 ++ arch/mips/kernel/cevt-sb1250.c | 2 ++ arch/mips/kernel/cevt-txx9.c | 2 ++ arch/mips/loongson/common/cs5536/cs5536_mfgpt.c | 2 ++ arch/mips/loongson/loongson-3/hpet.c | 2 ++ arch/mips/sni/time.c | 2 ++ arch/nios2/kernel/time.c | 2 ++ arch/openrisc/kernel/time.c | 2 ++ arch/sparc/kernel/time_64.c | 2 ++ arch/um/kernel/time.c | 2 ++ arch/unicore32/kernel/time.c | 2 ++ arch/x86/kernel/apic/apic.c | 2 ++ arch/x86/kernel/hpet.c | 2 ++ arch/x86/lguest/boot.c | 2 ++ arch/x86/platform/uv/uv_time.c | 2 ++ arch/x86/xen/time.c | 4 ++++ drivers/clocksource/cadence_ttc_timer.c | 2 ++ drivers/clocksource/dw_apb_timer.c | 2 ++ drivers/clocksource/exynos_mct.c | 4 ++++ drivers/clocksource/i8253.c | 2 ++ drivers/clocksource/metag_generic.c | 2 ++ drivers/clocksource/mxs_timer.c | 2 ++ drivers/clocksource/nomadik-mtu.c | 2 ++ drivers/clocksource/pxa_timer.c | 2 ++ drivers/clocksource/qcom-timer.c | 2 ++ drivers/clocksource/rockchip_timer.c | 2 ++ drivers/clocksource/samsung_pwm_timer.c | 2 ++ drivers/clocksource/tegra20_timer.c | 2 ++ drivers/clocksource/time-efm32.c | 2 ++ drivers/clocksource/timer-atmel-pit.c | 2 ++ drivers/clocksource/timer-prima2.c | 2 ++ drivers/clocksource/timer-u300.c | 2 ++ drivers/clocksource/vt8500_timer.c | 2 ++ 49 files changed, 104 insertions(+)
diff --git a/arch/arm/mach-at91/at91rm9200_time.c b/arch/arm/mach-at91/at91rm9200_time.c index 51761f8927b7..053ae0e3c3cb 100644 --- a/arch/arm/mach-at91/at91rm9200_time.c +++ b/arch/arm/mach-at91/at91rm9200_time.c @@ -138,6 +138,8 @@ clkevt32k_mode(enum clock_event_mode mode, struct clock_event_device *dev) case CLOCK_EVT_MODE_RESUME: irqmask = 0; break; + default: + break; } at91_st_write(AT91_ST_IER, irqmask); } diff --git a/arch/arm/mach-davinci/time.c b/arch/arm/mach-davinci/time.c index 160c9602f490..59347b8e8228 100644 --- a/arch/arm/mach-davinci/time.c +++ b/arch/arm/mach-davinci/time.c @@ -326,6 +326,8 @@ static void davinci_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/arch/arm/mach-footbridge/dc21285-timer.c b/arch/arm/mach-footbridge/dc21285-timer.c index bf7aa7d298e7..445965c216cd 100644 --- a/arch/arm/mach-footbridge/dc21285-timer.c +++ b/arch/arm/mach-footbridge/dc21285-timer.c @@ -74,6 +74,8 @@ static void ckevt_dc21285_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: *CSR_TIMER1_CNTL = 0; break; + default: + break; } }
diff --git a/arch/arm/mach-imx/epit.c b/arch/arm/mach-imx/epit.c index 074b1a81ba76..76b78f0ea739 100644 --- a/arch/arm/mach-imx/epit.c +++ b/arch/arm/mach-imx/epit.c @@ -152,6 +152,8 @@ static void epit_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Left event sources disabled, no more interrupts appear */ break; + default: + break; } }
diff --git a/arch/arm/mach-imx/time.c b/arch/arm/mach-imx/time.c index 15d18e198303..58c0423ef5f7 100644 --- a/arch/arm/mach-imx/time.c +++ b/arch/arm/mach-imx/time.c @@ -245,6 +245,8 @@ static void mxc_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Left event sources disabled, no more interrupts appear */ break; + default: + break; } }
diff --git a/arch/arm/mach-lpc32xx/timer.c b/arch/arm/mach-lpc32xx/timer.c index 4e5837299c04..027a6c5c77b6 100644 --- a/arch/arm/mach-lpc32xx/timer.c +++ b/arch/arm/mach-lpc32xx/timer.c @@ -64,6 +64,8 @@ static void lpc32xx_clkevt_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c index 2756351dbb35..f38519135e00 100644 --- a/arch/arm/mach-mmp/time.c +++ b/arch/arm/mach-mmp/time.c @@ -140,6 +140,8 @@ static void timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: case CLOCK_EVT_MODE_PERIODIC: break; + default: + break; } local_irq_restore(flags); } diff --git a/arch/arm/mach-omap1/time.c b/arch/arm/mach-omap1/time.c index a7588cfd0286..53242cc6c3fd 100644 --- a/arch/arm/mach-omap1/time.c +++ b/arch/arm/mach-omap1/time.c @@ -139,6 +139,8 @@ static void omap_mpu_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/arch/arm/mach-omap1/timer32k.c b/arch/arm/mach-omap1/timer32k.c index 107e7ab3edba..c21188c5b502 100644 --- a/arch/arm/mach-omap1/timer32k.c +++ b/arch/arm/mach-omap1/timer32k.c @@ -134,6 +134,8 @@ static void omap_32k_timer_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index 7d45c84c69ba..15ea6af11933 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -126,6 +126,8 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c index 1dea6cfafb31..e44c5b77169c 100644 --- a/arch/arm/mach-sa1100/time.c +++ b/arch/arm/mach-sa1100/time.c @@ -70,6 +70,8 @@ sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) case CLOCK_EVT_MODE_RESUME: case CLOCK_EVT_MODE_PERIODIC: break; + default: + break; } }
diff --git a/arch/arm/mach-w90x900/time.c b/arch/arm/mach-w90x900/time.c index 9230d3725599..5b9d9171bb95 100644 --- a/arch/arm/mach-w90x900/time.c +++ b/arch/arm/mach-w90x900/time.c @@ -70,6 +70,8 @@ static void nuc900_clockevent_setmode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + break; }
__raw_writel(val, REG_TCSR0); diff --git a/arch/blackfin/kernel/time-ts.c b/arch/blackfin/kernel/time-ts.c index cb0a4845339e..fcb082c8993d 100644 --- a/arch/blackfin/kernel/time-ts.c +++ b/arch/blackfin/kernel/time-ts.c @@ -173,6 +173,8 @@ static void bfin_gptmr0_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
@@ -279,6 +281,8 @@ static void bfin_coretmr_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/arch/c6x/platforms/timer64.c b/arch/c6x/platforms/timer64.c index 3c73d74a4674..027c45417106 100644 --- a/arch/c6x/platforms/timer64.c +++ b/arch/c6x/platforms/timer64.c @@ -146,6 +146,8 @@ static void set_clock_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/arch/m68k/coldfire/pit.c b/arch/m68k/coldfire/pit.c index 493b3111d4c1..373dc3c62b35 100644 --- a/arch/m68k/coldfire/pit.c +++ b/arch/m68k/coldfire/pit.c @@ -72,6 +72,8 @@ static void init_cf_pit_timer(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Nothing to do here */ break; + default: + break; } }
diff --git a/arch/microblaze/kernel/timer.c b/arch/microblaze/kernel/timer.c index dd96f0e4bfa2..7eb95b73c7bf 100644 --- a/arch/microblaze/kernel/timer.c +++ b/arch/microblaze/kernel/timer.c @@ -142,6 +142,8 @@ static void xilinx_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: pr_info("%s: resume\n", __func__); break; + default: + break; } }
diff --git a/arch/mips/kernel/cevt-bcm1480.c b/arch/mips/kernel/cevt-bcm1480.c index 7976457184b1..a08cd30c1d76 100644 --- a/arch/mips/kernel/cevt-bcm1480.c +++ b/arch/mips/kernel/cevt-bcm1480.c @@ -66,6 +66,8 @@ static void sibyte_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: /* shuddup gcc */ case CLOCK_EVT_MODE_RESUME: ; + default: + break; } }
diff --git a/arch/mips/kernel/cevt-sb1250.c b/arch/mips/kernel/cevt-sb1250.c index 5ea6d6b1de15..fdb7eed6dfd3 100644 --- a/arch/mips/kernel/cevt-sb1250.c +++ b/arch/mips/kernel/cevt-sb1250.c @@ -64,6 +64,8 @@ static void sibyte_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: /* shuddup gcc */ case CLOCK_EVT_MODE_RESUME: ; + default: + break; } }
diff --git a/arch/mips/kernel/cevt-txx9.c b/arch/mips/kernel/cevt-txx9.c index 2ae08462e46e..7f468a3c9a93 100644 --- a/arch/mips/kernel/cevt-txx9.c +++ b/arch/mips/kernel/cevt-txx9.c @@ -105,6 +105,8 @@ static void txx9tmr_set_mode(enum clock_event_mode mode, __raw_writel(TIMER_CCD, &tmrptr->ccdr); __raw_writel(0, &tmrptr->itmr); break; + default: + break; } }
diff --git a/arch/mips/loongson/common/cs5536/cs5536_mfgpt.c b/arch/mips/loongson/common/cs5536/cs5536_mfgpt.c index 12c75db23420..ec4a800b36dd 100644 --- a/arch/mips/loongson/common/cs5536/cs5536_mfgpt.c +++ b/arch/mips/loongson/common/cs5536/cs5536_mfgpt.c @@ -77,6 +77,8 @@ static void init_mfgpt_timer(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Nothing to do here */ break; + default: + break; } raw_spin_unlock(&mfgpt_lock); } diff --git a/arch/mips/loongson/loongson-3/hpet.c b/arch/mips/loongson/loongson-3/hpet.c index e898d68668a9..7833f5d5757f 100644 --- a/arch/mips/loongson/loongson-3/hpet.c +++ b/arch/mips/loongson/loongson-3/hpet.c @@ -125,6 +125,8 @@ static void hpet_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: hpet_enable_legacy_int(); break; + default: + break; } spin_unlock(&hpet_lock); } diff --git a/arch/mips/sni/time.c b/arch/mips/sni/time.c index cf8ec568b9df..97e1678c4956 100644 --- a/arch/mips/sni/time.c +++ b/arch/mips/sni/time.c @@ -40,6 +40,8 @@ static void a20r_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/arch/nios2/kernel/time.c b/arch/nios2/kernel/time.c index 7f4547418ee1..1c5103cf1729 100644 --- a/arch/nios2/kernel/time.c +++ b/arch/nios2/kernel/time.c @@ -181,6 +181,8 @@ static void nios2_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: nios2_timer_start(timer); break; + default: + break; } }
diff --git a/arch/openrisc/kernel/time.c b/arch/openrisc/kernel/time.c index 7c52e9494a8d..ca82d2daf16e 100644 --- a/arch/openrisc/kernel/time.c +++ b/arch/openrisc/kernel/time.c @@ -68,6 +68,8 @@ static void openrisc_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: pr_debug(KERN_INFO "%s: resume\n", __func__); break; + default: + break; } }
diff --git a/arch/sparc/kernel/time_64.c b/arch/sparc/kernel/time_64.c index edbbeb157d46..850ea2c476d7 100644 --- a/arch/sparc/kernel/time_64.c +++ b/arch/sparc/kernel/time_64.c @@ -704,6 +704,8 @@ static void sparc64_timer_setup(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: WARN_ON(1); break; + default: + break; } }
diff --git a/arch/um/kernel/time.c b/arch/um/kernel/time.c index 117568d4f64a..8e0a5b4cce4f 100644 --- a/arch/um/kernel/time.c +++ b/arch/um/kernel/time.c @@ -38,6 +38,8 @@ static void itimer_set_mode(enum clock_event_mode mode,
case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/arch/unicore32/kernel/time.c b/arch/unicore32/kernel/time.c index d3824b2ff644..179d7547edc5 100644 --- a/arch/unicore32/kernel/time.c +++ b/arch/unicore32/kernel/time.c @@ -60,6 +60,8 @@ puv3_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) case CLOCK_EVT_MODE_RESUME: case CLOCK_EVT_MODE_PERIODIC: break; + default: + break; } }
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c index b665d241efad..e7141d8f108b 100644 --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -493,6 +493,8 @@ static void lapic_timer_setup(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Nothing to do here */ break; + default: + break; }
local_irq_restore(flags); diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c index 319bcb9372fe..8c10da669d63 100644 --- a/arch/x86/kernel/hpet.c +++ b/arch/x86/kernel/hpet.c @@ -364,6 +364,8 @@ static void hpet_set_mode(enum clock_event_mode mode, } hpet_print_config(); break; + default: + break; } }
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c index c1c1544b8485..95b9705240ab 100644 --- a/arch/x86/lguest/boot.c +++ b/arch/x86/lguest/boot.c @@ -978,6 +978,8 @@ static void lguest_clockevent_set_mode(enum clock_event_mode mode, BUG(); case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/arch/x86/platform/uv/uv_time.c b/arch/x86/platform/uv/uv_time.c index a244237f3cfa..6612046adfe2 100644 --- a/arch/x86/platform/uv/uv_time.c +++ b/arch/x86/platform/uv/uv_time.c @@ -338,6 +338,8 @@ static void uv_rtc_timer_setup(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: uv_rtc_unset_timer(ced_cpu, 1); break; + default: + break; } }
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c index 69087341d9ae..02e6d6f688a0 100644 --- a/arch/x86/xen/time.c +++ b/arch/x86/xen/time.c @@ -291,6 +291,8 @@ static void xen_timerop_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: HYPERVISOR_set_timer_op(0); /* cancel timeout */ break; + default: + break; } }
@@ -349,6 +351,8 @@ static void xen_vcpuop_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c index 510c8a1d37b3..f93d1d8b8a9a 100644 --- a/drivers/clocksource/cadence_ttc_timer.c +++ b/drivers/clocksource/cadence_ttc_timer.c @@ -224,6 +224,8 @@ static void ttc_set_mode(enum clock_event_mode mode, writel_relaxed(ctrl_reg, timer->base_addr + TTC_CNT_CNTRL_OFFSET); break; + default: + break; } }
diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c index f3656a6b0382..ddfd00325e43 100644 --- a/drivers/clocksource/dw_apb_timer.c +++ b/drivers/clocksource/dw_apb_timer.c @@ -173,6 +173,8 @@ static void apbt_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: apbt_enable_int(&dw_ced->timer); break; + default: + break; } }
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c index 83564c9cfdbe..e604e1c5eb12 100644 --- a/drivers/clocksource/exynos_mct.c +++ b/drivers/clocksource/exynos_mct.c @@ -306,6 +306,8 @@ static void exynos4_comp_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
@@ -410,6 +412,8 @@ static inline void exynos4_tick_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/drivers/clocksource/i8253.c b/drivers/clocksource/i8253.c index 14ee3efcc404..d06c1df00e1e 100644 --- a/drivers/clocksource/i8253.c +++ b/drivers/clocksource/i8253.c @@ -136,6 +136,8 @@ static void init_pit_timer(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Nothing to do here */ break; + default: + break; } raw_spin_unlock(&i8253_lock); } diff --git a/drivers/clocksource/metag_generic.c b/drivers/clocksource/metag_generic.c index b7384b853e5a..834dc25676b8 100644 --- a/drivers/clocksource/metag_generic.c +++ b/drivers/clocksource/metag_generic.c @@ -72,6 +72,8 @@ static void metag_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: WARN_ON(1); break; + default: + break; }; }
diff --git a/drivers/clocksource/mxs_timer.c b/drivers/clocksource/mxs_timer.c index 445b68a01dc5..afaf4e1f170b 100644 --- a/drivers/clocksource/mxs_timer.c +++ b/drivers/clocksource/mxs_timer.c @@ -190,6 +190,8 @@ static void mxs_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Left event sources disabled, no more interrupts appear */ break; + default: + break; } }
diff --git a/drivers/clocksource/nomadik-mtu.c b/drivers/clocksource/nomadik-mtu.c index a709cfa49d85..0f75ac8cdddd 100644 --- a/drivers/clocksource/nomadik-mtu.c +++ b/drivers/clocksource/nomadik-mtu.c @@ -140,6 +140,8 @@ static void nmdk_clkevt_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/drivers/clocksource/pxa_timer.c b/drivers/clocksource/pxa_timer.c index 941f3f344e08..cb6b0bb74825 100644 --- a/drivers/clocksource/pxa_timer.c +++ b/drivers/clocksource/pxa_timer.c @@ -107,6 +107,8 @@ pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) case CLOCK_EVT_MODE_RESUME: case CLOCK_EVT_MODE_PERIODIC: break; + default: + break; } }
diff --git a/drivers/clocksource/qcom-timer.c b/drivers/clocksource/qcom-timer.c index 098c542e5c53..9fadcde8eed7 100644 --- a/drivers/clocksource/qcom-timer.c +++ b/drivers/clocksource/qcom-timer.c @@ -95,6 +95,8 @@ static void msm_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: break; + default: + break; } writel_relaxed(ctrl, event_base + TIMER_ENABLE); } diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c index a35993bafb20..cf572f161d5c 100644 --- a/drivers/clocksource/rockchip_timer.c +++ b/drivers/clocksource/rockchip_timer.c @@ -98,6 +98,8 @@ static inline void rk_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: rk_timer_disable(ce); break; + default: + break; } }
diff --git a/drivers/clocksource/samsung_pwm_timer.c b/drivers/clocksource/samsung_pwm_timer.c index 5645cfc90c41..c4a34818011b 100644 --- a/drivers/clocksource/samsung_pwm_timer.c +++ b/drivers/clocksource/samsung_pwm_timer.c @@ -225,6 +225,8 @@ static void samsung_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c index d2616ef16770..859ebe8728ce 100644 --- a/drivers/clocksource/tegra20_timer.c +++ b/drivers/clocksource/tegra20_timer.c @@ -90,6 +90,8 @@ static void tegra_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/drivers/clocksource/time-efm32.c b/drivers/clocksource/time-efm32.c index bba62f9deefb..67c5f5e43352 100644 --- a/drivers/clocksource/time-efm32.c +++ b/drivers/clocksource/time-efm32.c @@ -81,6 +81,8 @@ static void efm32_clock_event_set_mode(enum clock_event_mode mode,
case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/drivers/clocksource/timer-atmel-pit.c b/drivers/clocksource/timer-atmel-pit.c index b5b4d4585c9a..2c5e026b9fc3 100644 --- a/drivers/clocksource/timer-atmel-pit.c +++ b/drivers/clocksource/timer-atmel-pit.c @@ -116,6 +116,8 @@ pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) break; case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/drivers/clocksource/timer-prima2.c b/drivers/clocksource/timer-prima2.c index ce18d570e1cd..405400018e85 100644 --- a/drivers/clocksource/timer-prima2.c +++ b/drivers/clocksource/timer-prima2.c @@ -123,6 +123,8 @@ static void sirfsoc_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME: break; + default: + break; } }
diff --git a/drivers/clocksource/timer-u300.c b/drivers/clocksource/timer-u300.c index 5dcf756970e7..746f65ed13b0 100644 --- a/drivers/clocksource/timer-u300.c +++ b/drivers/clocksource/timer-u300.c @@ -265,6 +265,8 @@ static void u300_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Ignore this call */ break; + default: + break; } }
diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c index 1098ed3b9b89..ca982352a637 100644 --- a/drivers/clocksource/vt8500_timer.c +++ b/drivers/clocksource/vt8500_timer.c @@ -102,6 +102,8 @@ static void vt8500_timer_set_mode(enum clock_event_mode mode, regbase + TIMER_CTRL_VAL); writel(0, regbase + TIMER_IER_VAL); break; + default: + break; } }
* Viresh Kumar viresh.kumar@linaro.org wrote:
Many clockevent drivers are using a switch block for handling modes in their ->set_mode() callback. Some of these do not have a 'default' case and adding a new mode in the 'enum clock_event_mode', starts giving following warnings for these platforms about unhandled modes (e.g. XXX).
warning: enumeration value ‘XXX’ not handled in switch [-Wswitch]
This patch adds default cases for them.
In order to keep things simple, add following to the switch blocks:
default: break;
This can lead to different behavior for individual cases.
Some of the drivers don't have any special stuff in their ->set_mode() callback before or after the switch blocks. And so this default case would simply return for them without any updates to the clockevent device.
But in some cases, the clockevent device is disabled/stopped as soon as we enter the ->set_mode() callback and is enabled within the switch block or after it. And the clockevent device *may* stay disabled for default case.
So this whole approach looks fragile for several reasons:
- 'mode setting' callbacks are just bad by design because they mix several functions into the same entry point, complicating the handler functions unnecessarily. We should reduce complexity, not expand on it.
- now by adding 'default' you hide from drivers the ability to easily discover whether it has been updated to some new core clockevents mode setting feature or not.
So NAK: the real solution would be to eliminate 'mode setting' altogether: treat it as a legacy, introduce properly separated callbacks and function pointer callback driver templates, and let drivers fill in (or not) individual entries. Clockevents core can mandate certain entries to be filled in, or allow NULL with some default behavior.
The ->set_mode() approach allows none of this. It is ioctl()-alike interface design, and that is a singularly bad design for the aforementioned reasons.
So to give a quick example what I have in mind, I looked up a random clockevents driver callback from your patch (arch/arm/mach-mmp/time.c):
static void timer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) { unsigned long flags;
local_irq_save(flags); switch (mode) { case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: /* disable the matching interrupt */ __raw_writel(0x00, mmp_timer_base + TMR_IER(0)); break; case CLOCK_EVT_MODE_RESUME: case CLOCK_EVT_MODE_PERIODIC: break; default: break; } local_irq_restore(flags); }
Instead of that I'd let the driver define just a single function:
static void mmp_timer_disable(struct clock_event_device *dev __unused) { unsigned long flags;
local_irq_save(flags); __raw_writel(0x00, mmp_timer_base + TMR_IER(0)); local_irq_restore(flags); }
static struct clockevents_driver mmp_clockevents { .clock_event__oneshot = mmp_timer_disable, .clock_event__shutdown = mmp_timer_disable,
/* clock_event__resume and __periodic are NULL */ };
See how much cleaner and easier to read it all is?
Note how the NULL entries express a 'don't do anything' default in a natural fashion. It's also future proof: if a new callback is added to 'struct clockevents_driver' then it's filled in with NULL and the clockevents core may define a default behavior, without having to touch the driver.
It's also _faster_: no function call needed and there's a pointless local_irq_save()/restore() call optimized out as well.
So let's clean up the clockevents mode setting design, okay?
Thanks,
Ingo
On Fri, Feb 20, 2015 at 09:38:42AM +0100, Ingo Molnar wrote:
- Viresh Kumar viresh.kumar@linaro.org wrote:
Many clockevent drivers are using a switch block for handling modes in their ->set_mode() callback. Some of these do not have a 'default' case and adding a new mode in the 'enum clock_event_mode', starts giving following warnings for these platforms about unhandled modes (e.g. XXX).
warning: enumeration value ‘XXX’ not handled in switch [-Wswitch]
This patch adds default cases for them.
In order to keep things simple, add following to the switch blocks:
default: break;
This can lead to different behavior for individual cases.
Some of the drivers don't have any special stuff in their ->set_mode() callback before or after the switch blocks. And so this default case would simply return for them without any updates to the clockevent device.
But in some cases, the clockevent device is disabled/stopped as soon as we enter the ->set_mode() callback and is enabled within the switch block or after it. And the clockevent device *may* stay disabled for default case.
So this whole approach looks fragile for several reasons:
'mode setting' callbacks are just bad by design because they mix several functions into the same entry point, complicating the handler functions unnecessarily. We should reduce complexity, not expand on it.
now by adding 'default' you hide from drivers the ability to easily discover whether it has been updated to some new core clockevents mode setting feature or not.
So this patch was a follow on from bd624d75db21 ("clockevents: Introduce mode specific callbacks").
That patch changes the set_mode() interface; and provides per mode functions.
New (and updated) drivers should not use ->set_mode() anymore, but it was felt that we do not want to go do flag day changes.
And it allows for adding optional modes; not every driver needs to go implement all mode functions if there is a sane default action.
But it does mean we need to be able to add values to the enum.
* Peter Zijlstra peterz@infradead.org wrote:
So this whole approach looks fragile for several reasons:
'mode setting' callbacks are just bad by design because they mix several functions into the same entry point, complicating the handler functions unnecessarily. We should reduce complexity, not expand on it.
now by adding 'default' you hide from drivers the ability to easily discover whether it has been updated to some new core clockevents mode setting feature or not.
So this patch was a follow on from bd624d75db21 ("clockevents: Introduce mode specific callbacks").
That patch changes the set_mode() interface; and provides per mode functions.
So why is a 'default' mode needed then? It makes the addition of new modes to the legacy handler easier, which looks backwards.
New (and updated) drivers should not use ->set_mode() anymore, but it was felt that we do not want to go do flag day changes.
I fully agree that we don't want flag day changes, but make it really apparent that it's an obsolete interface:
- rename it to set_mode_obsolete()
- try to convert as many of the easy cases as possible - the overwhelming majority of mode setting functions look reasonably simple.
- get rid of the mode enum in the core, and rename the mode bits to CLOCK_EVT_MODE_OBSOLETE_XXX.
etc.
And it allows for adding optional modes; not every driver needs to go implement all mode functions if there is a sane default action.
But it does mean we need to be able to add values to the enum.
So I'm confused: if we are using proper callbacks (like my example outlined) , why is a 'mode enum' needed at all?
Thanks,
Ingo
On 20 February 2015 at 15:06, Ingo Molnar mingo@kernel.org wrote:
- Peter Zijlstra peterz@infradead.org wrote:
So this whole approach looks fragile for several reasons:
'mode setting' callbacks are just bad by design because they mix several functions into the same entry point, complicating the handler functions unnecessarily. We should reduce complexity, not expand on it.
now by adding 'default' you hide from drivers the ability to easily discover whether it has been updated to some new core clockevents mode setting feature or not.
So this patch was a follow on from bd624d75db21 ("clockevents: Introduce mode specific callbacks").
That patch changes the set_mode() interface; and provides per mode functions.
Thanks Peter ..
So why is a 'default' mode needed then? It makes the addition of new modes to the legacy handler easier, which looks backwards.
The requirement was to add another mode ONESHOT_STOPPED [1], to be supported only by the new per-mode callbacks..
We have got a clear check in core with the patch Peter mentioned above, which doesn't let us call legacy ->set_mode() for the newer modes.
if (dev->set_mode) { /* Legacy callback doesn't support new modes */ if (mode > CLOCK_EVT_MODE_RESUME) return -ENOSYS; dev->set_mode(mode, dev); return 0; }
The intention of adding a 'default' case (which is already present in most of clockevents drivers) here was to make compiler happy and that's why the commit logs mentioned this:
" The rationale behind only adding a 'break' was that the default case *will never* be hit during execution of code. "
New (and updated) drivers should not use ->set_mode() anymore, but it was felt that we do not want to go do flag day changes.
I fully agree that we don't want flag day changes, but make it really apparent that it's an obsolete interface:
rename it to set_mode_obsolete()
try to convert as many of the easy cases as possible - the overwhelming majority of mode setting functions look reasonably simple.
Yes, we can do that..
- get rid of the mode enum in the core, and rename the mode bits to CLOCK_EVT_MODE_OBSOLETE_XXX.
So I'm confused: if we are using proper callbacks (like my example outlined) , why is a 'mode enum' needed at all?
The enum has two uses today:
- pass mode to the legacy ->set_mode() callback, which isn't required for the new callbacks.
- flag for clockevent core's internal state machine, which it would still require. For example, it checks new-mode != old-mode before changing the mode..
I believe the enum is still required for the state machine, even with new per-mode callbacks.
-- viresh
* Viresh Kumar viresh.kumar@linaro.org wrote:
So why is a 'default' mode needed then? It makes the addition of new modes to the legacy handler easier, which looks backwards.
The requirement was to add another mode ONESHOT_STOPPED [1], to be supported only by the new per-mode callbacks..
Why would a callback need any flag, and why would a flag be visible to old legacy callbacks?
We have got a clear check in core with the patch Peter mentioned above, which doesn't let us call legacy ->set_mode() for the newer modes.
if (dev->set_mode) { /* Legacy callback doesn't support new modes */ if (mode > CLOCK_EVT_MODE_RESUME) return -ENOSYS; dev->set_mode(mode, dev); return 0; }
So here is where one of your problems comes from: why did you add CLOCK_EVT_MODE_RESUME to the interface? Phase it out, it's a legacy interface - new callbacks shouldn't need any mode flags to begin with.
So I'm confused: if we are using proper callbacks (like my example outlined) , why is a 'mode enum' needed at all?
The enum has two uses today:
- pass mode to the legacy ->set_mode() callback, which
isn't required for the new callbacks.
But this is misguided, as per above.
- flag for clockevent core's internal state machine, which it would still require. For example, it checks new-mode != old-mode before changing the mode..
Internal state machine state should be decoupled from any interface flags - especially when the interface is legacy.
I believe the enum is still required for the state machine, even with new per-mode callbacks.
That needs to be fixed first then, before introducing new API variants.
Thanks,
Ingo
On Fri, Feb 20, 2015 at 10:36:59AM +0100, Ingo Molnar wrote:
But it does mean we need to be able to add values to the enum.
So I'm confused: if we are using proper callbacks (like my example outlined) , why is a 'mode enum' needed at all?
Ah, its because the enum is shared between two different use-cases. The one is the clockevent driver for the clock_event_device::set_mode() call, and one is the clockevent core call: clockevent_set_mode().
The previous patch changed the driver interface, but retained the sharing of the enum across both interfaces.
Maybe we should break that enum into two; one for devices and one for the core interface and avoid the problem that way.
* Peter Zijlstra peterz@infradead.org wrote:
On Fri, Feb 20, 2015 at 10:36:59AM +0100, Ingo Molnar wrote:
But it does mean we need to be able to add values to the enum.
So I'm confused: if we are using proper callbacks (like my example outlined) , why is a 'mode enum' needed at all?
Ah, its because the enum is shared between two different use-cases. The one is the clockevent driver for the clock_event_device::set_mode() call, and one is the clockevent core call: clockevent_set_mode().
The previous patch changed the driver interface, but retained the sharing of the enum across both interfaces.
Maybe we should break that enum into two; one for devices and one for the core interface and avoid the problem that way.
Yeah, that would do the trick.
Thanks,
Ingo
On 20 February 2015 at 17:11, Ingo Molnar mingo@kernel.org wrote:
- Peter Zijlstra peterz@infradead.org wrote:
Maybe we should break that enum into two; one for devices and one for the core interface and avoid the problem that way.
Yeah, that would do the trick.
Thanks for your suggestions. Just to confirm (before I spam lists with patches), is this somewhat similar to what you are looking for ?
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 59af26b54d15..80b669cb836d 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -32,17 +32,24 @@ enum clock_event_nofitiers { struct clock_event_device; struct module;
-/* Clock event mode commands */ +/* Clock event mode commands for legacy ->set_mode(): OBSOLETE */ enum clock_event_mode { CLOCK_EVT_MODE_UNUSED = 0, CLOCK_EVT_MODE_SHUTDOWN, CLOCK_EVT_MODE_PERIODIC, CLOCK_EVT_MODE_ONESHOT, CLOCK_EVT_MODE_RESUME, - - /* Legacy ->set_mode() callback doesn't support below modes */ };
+/* Clock event modes, only for core's internal use */ +enum clock_event_dev_mode { + CLOCK_EVT_DEV_MODE_UNUSED = 0, + CLOCK_EVT_DEV_MODE_SHUTDOWN, + CLOCK_EVT_DEV_MODE_PERIODIC, + CLOCK_EVT_DEV_MODE_ONESHOT, + CLOCK_EVT_DEV_MODE_RESUME, + CLOCK_EVT_DEV_MODE_ONESHOT_STOPPED, /* This would be the new mode which I will add later */ +}; + /* * Clock event features */ diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index 489642b08d64..16555d3db94d 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -95,14 +95,18 @@ u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt) EXPORT_SYMBOL_GPL(clockevent_delta2ns);
static int __clockevents_set_mode(struct clock_event_device *dev, - enum clock_event_mode mode) + enum clock_event_dev_mode mode) { /* Transition with legacy set_mode() callback */ if (dev->set_mode) { /* Legacy callback doesn't support new modes */ - if (mode > CLOCK_EVT_MODE_RESUME) + if (mode > CLOCK_EVT_DEV_MODE_RESUME) return -ENOSYS; - dev->set_mode(mode, dev); + /* + * 'clock_event_dev_mode' and 'clock_event_mode' have 1-to-1 + * mapping until *_RESUME, and so a simple cast will work. + */ + dev->set_mode((enum clock_event_mode)mode, dev); return 0; }
@@ -111,29 +115,29 @@ static int __clockevents_set_mode(struct clock_event_device *dev,
/* Transition with new mode-specific callbacks */ switch (mode) { - case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_DEV_MODE_UNUSED: /* * This is an internal state, which is guaranteed to go from * SHUTDOWN to UNUSED. No driver interaction required. */ return 0;
- case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_DEV_MODE_SHUTDOWN: return dev->set_mode_shutdown(dev);
- case CLOCK_EVT_MODE_PERIODIC: + case CLOCK_EVT_DEV_MODE_PERIODIC: /* Core internal bug */ if (!(dev->features & CLOCK_EVT_FEAT_PERIODIC)) return -ENOSYS; return dev->set_mode_periodic(dev);
- case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_DEV_MODE_ONESHOT: /* Core internal bug */ if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT)) return -ENOSYS; return dev->set_mode_oneshot(dev);
- case CLOCK_EVT_MODE_RESUME: + case CLOCK_EVT_DEV_MODE_RESUME: /* Optional callback */ if (dev->set_mode_resume) return dev->set_mode_resume(dev);
Ofcourse, we also need to replace 'clock_event_mode' with 'clock_event_dev_mode' and 'CLOCK_EVT_MODE_*' with 'CLOCK_EVT_DEV_MODE_*' in all core code..
-- viresh
* Viresh Kumar viresh.kumar@linaro.org wrote:
On 20 February 2015 at 17:11, Ingo Molnar mingo@kernel.org wrote:
- Peter Zijlstra peterz@infradead.org wrote:
Maybe we should break that enum into two; one for devices and one for the core interface and avoid the problem that way.
Yeah, that would do the trick.
Thanks for your suggestions. Just to confirm (before I spam lists with patches), is this somewhat similar to what you are looking for ?
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 59af26b54d15..80b669cb836d 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -32,17 +32,24 @@ enum clock_event_nofitiers { struct clock_event_device; struct module;
-/* Clock event mode commands */ +/* Clock event mode commands for legacy ->set_mode(): OBSOLETE */ enum clock_event_mode { CLOCK_EVT_MODE_UNUSED = 0, CLOCK_EVT_MODE_SHUTDOWN, CLOCK_EVT_MODE_PERIODIC, CLOCK_EVT_MODE_ONESHOT, CLOCK_EVT_MODE_RESUME,
/* Legacy ->set_mode() callback doesn't support below modes */};
+/* Clock event modes, only for core's internal use */ +enum clock_event_dev_mode {
CLOCK_EVT_DEV_MODE_UNUSED = 0,
What is 'unused' - not initialized yet?
CLOCK_EVT_DEV_MODE_SHUTDOWN,CLOCK_EVT_DEV_MODE_PERIODIC,CLOCK_EVT_DEV_MODE_ONESHOT,CLOCK_EVT_DEV_MODE_RESUME,
What is 'resume' mode?
CLOCK_EVT_DEV_MODE_ONESHOT_STOPPED, /* This would be the newmode which I will add later */
What does this mode express?
So for state machines it's important to document the states and the transitions between them very clearly - please start with that.
+};
Also, this should not be in a generic header, it should be somewhere internal in kernel/time/.
Ofcourse, we also need to replace 'clock_event_mode' with 'clock_event_dev_mode' and 'CLOCK_EVT_MODE_*' with 'CLOCK_EVT_DEV_MODE_*' in all core code..
Yes.
Thanks,
Ingo
On 20 February 2015 at 18:52, Ingo Molnar mingo@kernel.org wrote:
- Viresh Kumar viresh.kumar@linaro.org wrote:
CLOCK_EVT_DEV_MODE_UNUSED = 0,What is 'unused' - not initialized yet?
Unused. Initially all clockevent devices are supposed to be in this mode but later if another device replaces an existing one, the existing one is put into this mode.
CLOCK_EVT_DEV_MODE_SHUTDOWN,CLOCK_EVT_DEV_MODE_PERIODIC,CLOCK_EVT_DEV_MODE_ONESHOT,CLOCK_EVT_DEV_MODE_RESUME,What is 'resume' mode?
Introduced with: 18de5bc4c1f1 ("clockevents: fix resume logic") and is only called during system resume to resume the clockevent devices before resuming the tick. Only few implementations do meaningful stuff here.
CLOCK_EVT_DEV_MODE_ONESHOT_STOPPED, /* This would be the newmode which I will add later */
What does this mode express?
I have added it here to show how things would look like eventually, but it wouldn't be present in the patch which splits the enum into two parts..
Its only important for NOHZ_FULL (IDLE ? Maybe). When we decide that the tick (LOWRES) or hrtimer interrupt (HIGHRES) isn't required for indefinite period of time (i.e. no timers/hrtimers are present to serve), we skip reprogramming the clockevent device. But its already reprogrammed from the tick-handler and so will fire atleast once again.
The case is worst for implementations where the underlying hardware doesn't have support for ONESHOT mode. And so they emulate ONESHOT over PERIODIC. And in those cases, these spurious interrupts come at a rate last programmed for the clockevent device. And that is mostly tick-rate..
So for state machines it's important to document the states and the transitions between them very clearly - please start with that.
Sure.
Also, this should not be in a generic header, it should be somewhere internal in kernel/time/.
Right. But there are some excellent drivers which are comparing things against dev->mode (i.e. enum clock_event_dev_mode now..). I need to fix them as well first to push this into some internal header.
Ofcourse, we also need to replace 'clock_event_mode' with 'clock_event_dev_mode' and 'CLOCK_EVT_MODE_*' with 'CLOCK_EVT_DEV_MODE_*' in all core code..
Yes.
Thanks for your suggestions.
* Viresh Kumar viresh.kumar@linaro.org wrote:
On 20 February 2015 at 18:52, Ingo Molnar mingo@kernel.org wrote:
- Viresh Kumar viresh.kumar@linaro.org wrote:
CLOCK_EVT_DEV_MODE_UNUSED = 0,What is 'unused' - not initialized yet?
Unused. Initially all clockevent devices are supposed to be in this mode but later if another device replaces an existing one, the existing one is put into this mode.
I'd suggest to rename it to MODE_INIT - at first glance it gave me the impression that it's some sort of API placeholder - i.e. an unused flag or so.
Also, I'd suggest to rename all 'modes' to true state machine naming: STATE_INITIALIZED, STATE_SHUT_DOWN, STATE_PERIODIC, STATE_RESUMED, etc.: if these are enums for states and not state transition names, see my later questions:
CLOCK_EVT_DEV_MODE_SHUTDOWN,CLOCK_EVT_DEV_MODE_PERIODIC,CLOCK_EVT_DEV_MODE_ONESHOT,CLOCK_EVT_DEV_MODE_RESUME,What is 'resume' mode?
Introduced with: 18de5bc4c1f1 ("clockevents: fix resume logic") and is only called during system resume to resume the clockevent devices before resuming the tick. Only few implementations do meaningful stuff here.
So is it a state that a clockevents device reaches, or a state transition? The two purposes seem to be mixed up in the nomenclature.
CLOCK_EVT_DEV_MODE_ONESHOT_STOPPED, /* This would be the newmode which I will add later */
What does this mode express?
I have added it here to show how things would look like eventually, but it wouldn't be present in the patch which splits the enum into two parts..
Yeah.
Its only important for NOHZ_FULL (IDLE ? Maybe). When we decide that the tick (LOWRES) or hrtimer interrupt (HIGHRES) isn't required for indefinite period of time (i.e. no timers/hrtimers are present to serve), we skip reprogramming the clockevent device. But its already reprogrammed from the tick-handler and so will fire atleast once again.
So this new 'mode' appears to be a true state of the device?
Thanks,
Ingo
On 20 February 2015 at 19:34, Ingo Molnar mingo@kernel.org wrote:
- Viresh Kumar viresh.kumar@linaro.org wrote:
Unused. Initially all clockevent devices are supposed to be in this mode but later if another device replaces an existing one, the existing one is put into this mode.
I'd suggest to rename it to MODE_INIT - at first glance it gave me the impression that it's some sort of API placeholder - i.e. an unused flag or so.
Sorry, if I wasn't able to clarify this earlier. The impression you got at first glance is correct. And it should be UNUSED only instead of INIT. Its not about if the the device is initialized or not, but if it is used or not. And that's why there is no callback for this in the new per-mode callbacks.
Also, I'd suggest to rename all 'modes' to true state machine naming: STATE_INITIALIZED, STATE_SHUT_DOWN, STATE_PERIODIC, STATE_RESUMED, etc.: if these are enums for
I thought so initially and it looked like the diff will be huge as all the variables for the enum, i.e. 'mode', need to be renamed to 'state'..
But, if you are okay with it then I would be happy to do that..
states and not state transition names, see my later questions:
CLOCK_EVT_DEV_MODE_SHUTDOWN,CLOCK_EVT_DEV_MODE_PERIODIC,CLOCK_EVT_DEV_MODE_ONESHOT,CLOCK_EVT_DEV_MODE_RESUME,What is 'resume' mode?
Introduced with: 18de5bc4c1f1 ("clockevents: fix resume logic") and is only called during system resume to resume the clockevent devices before resuming the tick. Only few implementations do meaningful stuff here.
So is it a state that a clockevents device reaches, or a state transition? The two purposes seem to be mixed up in the nomenclature.
Where all other modes are states, 'resume' probably represents transition. It is immediately followed by a transition to ONESHOT or PERIODIC mode and so it is just a preliminary step before moving to final states during resume. We *may* not need to keep this in the states enum..
Its only important for NOHZ_FULL (IDLE ? Maybe). When we decide that the tick (LOWRES) or hrtimer interrupt (HIGHRES) isn't required for indefinite period of time (i.e. no timers/hrtimers are present to serve), we skip reprogramming the clockevent device. But its already reprogrammed from the tick-handler and so will fire atleast once again.
So this new 'mode' appears to be a true state of the device?
Yes.
* Viresh Kumar viresh.kumar@linaro.org wrote:
On 20 February 2015 at 19:34, Ingo Molnar mingo@kernel.org wrote:
- Viresh Kumar viresh.kumar@linaro.org wrote:
Unused. Initially all clockevent devices are supposed to be in this mode but later if another device replaces an existing one, the existing one is put into this mode.
I'd suggest to rename it to MODE_INIT - at first glance it gave me the impression that it's some sort of API placeholder - i.e. an unused flag or so.
Sorry, if I wasn't able to clarify this earlier. The impression you got at first glance is correct. And it should be UNUSED only instead of INIT. Its not about if the the device is initialized or not, but if it is used or not. And that's why there is no callback for this in the new per-mode callbacks.
Ok, could we rename it to something like DETACHED?
'UNUSED' really gives me the wrong impression - it's what we do for unused fields, unused ABI enumertion constants, etc.
Also, I'd suggest to rename all 'modes' to true state machine naming: STATE_INITIALIZED, STATE_SHUT_DOWN, STATE_PERIODIC, STATE_RESUMED, etc.: if these are enums for
I thought so initially and it looked like the diff will be huge as all the variables for the enum, i.e. 'mode', need to be renamed to 'state'..
But, if you are okay with it then I would be happy to do that..
Well, how does the diffstat look like?
But if the conversion was scripted (i.e. is relatively secure from typos) then that's still fine IMO. Peter?
Thanks,
Ingo
On Monday 23 February 2015 10:07 PM, Ingo Molnar wrote:>
- Viresh Kumar viresh.kumar@linaro.org wrote:
Ok, could we rename it to something like DETACHED?
'UNUSED' really gives me the wrong impression - it's what we do for unused fields, unused ABI enumertion constants, etc.
Sure.
Also, I'd suggest to rename all 'modes' to true state machine naming: STATE_INITIALIZED, STATE_SHUT_DOWN, STATE_PERIODIC, STATE_RESUMED, etc.: if these are enums for
I thought so initially and it looked like the diff will be huge as all the variables for the enum, i.e. 'mode', need to be renamed to 'state'..
But, if you are okay with it then I would be happy to do that..
Well, how does the diffstat look like?
But if the conversion was scripted (i.e. is relatively secure from typos) then that's still fine IMO. Peter?
Here is the diff:
NOTE: I have still kept the new enum in include/linux/clockchips.h as bL_switcher needs to use them.
diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c index 6eaddc47c43d..d4f970a4d255 100644 --- a/arch/arm/common/bL_switcher.c +++ b/arch/arm/common/bL_switcher.c @@ -152,7 +152,7 @@ static int bL_switch_to(unsigned int new_cluster_id) unsigned int ob_mpidr, ob_cpu, ob_cluster, ib_mpidr, ib_cpu, ib_cluster; struct completion inbound_alive; struct tick_device *tdev; - enum clock_event_mode tdev_mode; + enum clock_event_state tdev_state; long volatile *handshake_ptr; int ipi_nr, ret;
@@ -223,8 +223,8 @@ static int bL_switch_to(unsigned int new_cluster_id) if (tdev && !cpumask_equal(tdev->evtdev->cpumask, cpumask_of(this_cpu))) tdev = NULL; if (tdev) { - tdev_mode = tdev->evtdev->mode; - clockevents_set_mode(tdev->evtdev, CLOCK_EVT_MODE_SHUTDOWN); + tdev_state = tdev->evtdev->state; + clockevents_set_state(tdev->evtdev, CLOCK_EVT_STATE_SHUTDOWN); }
ret = cpu_pm_enter(); @@ -252,7 +252,7 @@ static int bL_switch_to(unsigned int new_cluster_id) ret = cpu_pm_exit();
if (tdev) { - clockevents_set_mode(tdev->evtdev, tdev_mode); + clockevents_set_state(tdev->evtdev, tdev_state); clockevents_program_event(tdev->evtdev, tdev->evtdev->next_event, 1); } diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 59af26b54d15..58d1c3922e4d 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -32,15 +32,31 @@ enum clock_event_nofitiers { struct clock_event_device; struct module;
-/* Clock event mode commands */ +/* Clock event mode commands for legacy ->set_mode(): OBSOLETE */ enum clock_event_mode { CLOCK_EVT_MODE_UNUSED = 0, CLOCK_EVT_MODE_SHUTDOWN, CLOCK_EVT_MODE_PERIODIC, CLOCK_EVT_MODE_ONESHOT, CLOCK_EVT_MODE_RESUME, +};
- /* Legacy ->set_mode() callback doesn't support below modes */ +/* + * Possible states of a clock event device. + * + * DETACHED: Device is not used by clockevents core. Initial state or can be + * reached from SHUTDOWN. + * SHUTDOWN: Device is powered-off. Can be reached from PERIODIC or ONESHOT. + * PERIODIC: Device is programmed to generate events periodically. Can be + * reached from DETACHED or SHUTDOWN. + * ONESHOT: Device is programmed to generate event only once. Can be reached + * from DETACHED or SHUTDOWN. + */ +enum clock_event_state { + CLOCK_EVT_STATE_DETACHED = 0, + CLOCK_EVT_STATE_SHUTDOWN, + CLOCK_EVT_STATE_PERIODIC, + CLOCK_EVT_STATE_ONESHOT, };
/* @@ -80,14 +96,15 @@ enum clock_event_mode { * @min_delta_ns: minimum delta value in ns * @mult: nanosecond to cycles multiplier * @shift: nanoseconds to cycles divisor (power of two) - * @mode: operating mode assigned by the management code + * @mode: operating mode, relevant only to ->set_mode(), OBSOLETE + * @state: current state of the device, assigned by the core code * @features: features * @retries: number of forced programming retries * @set_mode: legacy set mode function, only for modes <= CLOCK_EVT_MODE_RESUME. - * @set_mode_periodic: switch mode to periodic, if !set_mode - * @set_mode_oneshot: switch mode to oneshot, if !set_mode - * @set_mode_shutdown: switch mode to shutdown, if !set_mode - * @set_mode_resume: resume clkevt device, if !set_mode + * @set_state_periodic: switch state to periodic, if !set_mode + * @set_state_oneshot: switch state to oneshot, if !set_mode + * @set_state_shutdown: switch state to shutdown, if !set_mode + * @set_state_resume: resume clkevt device, if !set_mode * @broadcast: function to broadcast events * @min_delta_ticks: minimum delta value in ticks stored for reconfiguration * @max_delta_ticks: maximum delta value in ticks stored for reconfiguration @@ -111,21 +128,22 @@ struct clock_event_device { u32 mult; u32 shift; enum clock_event_mode mode; + enum clock_event_state state; unsigned int features; unsigned long retries;
/* - * Mode transition callback(s): Only one of the two groups should be + * State transition callback(s): Only one of the two groups should be * defined: * - set_mode(), only for modes <= CLOCK_EVT_MODE_RESUME. - * - set_mode_{shutdown|periodic|oneshot|resume}(). + * - set_state_{shutdown|periodic|oneshot|resume}(). */ void (*set_mode)(enum clock_event_mode mode, struct clock_event_device *); - int (*set_mode_periodic)(struct clock_event_device *); - int (*set_mode_oneshot)(struct clock_event_device *); - int (*set_mode_shutdown)(struct clock_event_device *); - int (*set_mode_resume)(struct clock_event_device *); + int (*set_state_periodic)(struct clock_event_device *); + int (*set_state_oneshot)(struct clock_event_device *); + int (*set_state_shutdown)(struct clock_event_device *); + int (*set_state_resume)(struct clock_event_device *);
void (*broadcast)(const struct cpumask *mask); void (*suspend)(struct clock_event_device *); @@ -177,8 +195,8 @@ extern int clockevents_update_freq(struct clock_event_device *ce, u32 freq);
extern void clockevents_exchange_device(struct clock_event_device *old, struct clock_event_device *new); -extern void clockevents_set_mode(struct clock_event_device *dev, - enum clock_event_mode mode); +extern void clockevents_set_state(struct clock_event_device *dev, + enum clock_event_state state); extern int clockevents_program_event(struct clock_event_device *dev, ktime_t expires, bool force);
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index 489642b08d64..5aacfabd7df7 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -94,51 +94,49 @@ u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt) } EXPORT_SYMBOL_GPL(clockevent_delta2ns);
-static int __clockevents_set_mode(struct clock_event_device *dev, - enum clock_event_mode mode) +static int __clockevents_set_state(struct clock_event_device *dev, + enum clock_event_state state) { /* Transition with legacy set_mode() callback */ if (dev->set_mode) { /* Legacy callback doesn't support new modes */ - if (mode > CLOCK_EVT_MODE_RESUME) + if (state > CLOCK_EVT_STATE_ONESHOT) return -ENOSYS; - dev->set_mode(mode, dev); + /* + * 'clock_event_state' and 'clock_event_mode' have 1-to-1 + * mapping until *_ONESHOT, and so a simple cast will work. + */ + dev->set_mode((enum clock_event_mode)state, dev); + dev->mode = (enum clock_event_mode)state; return 0; }
if (dev->features & CLOCK_EVT_FEAT_DUMMY) return 0;
- /* Transition with new mode-specific callbacks */ - switch (mode) { - case CLOCK_EVT_MODE_UNUSED: + /* Transition with new state-specific callbacks */ + switch (state) { + case CLOCK_EVT_STATE_DETACHED: /* * This is an internal state, which is guaranteed to go from * SHUTDOWN to UNUSED. No driver interaction required. */ return 0;
- case CLOCK_EVT_MODE_SHUTDOWN: - return dev->set_mode_shutdown(dev); + case CLOCK_EVT_STATE_SHUTDOWN: + return dev->set_state_shutdown(dev);
- case CLOCK_EVT_MODE_PERIODIC: + case CLOCK_EVT_STATE_PERIODIC: /* Core internal bug */ if (!(dev->features & CLOCK_EVT_FEAT_PERIODIC)) return -ENOSYS; - return dev->set_mode_periodic(dev); + return dev->set_state_periodic(dev);
- case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_STATE_ONESHOT: /* Core internal bug */ if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT)) return -ENOSYS; - return dev->set_mode_oneshot(dev); - - case CLOCK_EVT_MODE_RESUME: - /* Optional callback */ - if (dev->set_mode_resume) - return dev->set_mode_resume(dev); - else - return 0; + return dev->set_state_oneshot(dev);
default: return -ENOSYS; @@ -146,26 +144,26 @@ static int __clockevents_set_mode(struct clock_event_device *dev, }
/** - * clockevents_set_mode - set the operating mode of a clock event device + * clockevents_set_state - set the operating state of a clock event device * @dev: device to modify - * @mode: new mode + * @state: new state * * Must be called with interrupts disabled ! */ -void clockevents_set_mode(struct clock_event_device *dev, - enum clock_event_mode mode) +void clockevents_set_state(struct clock_event_device *dev, + enum clock_event_state state) { - if (dev->mode != mode) { - if (__clockevents_set_mode(dev, mode)) + if (dev->state != state) { + if (__clockevents_set_state(dev, state)) return;
- dev->mode = mode; + dev->state = state;
/* * A nsec2cyc multiplicator of 0 is invalid and we'd crash * on it, so fix it up and emit a warning: */ - if (mode == CLOCK_EVT_MODE_ONESHOT) { + if (state == CLOCK_EVT_STATE_ONESHOT) { if (unlikely(!dev->mult)) { dev->mult = 1; WARN_ON(1); @@ -180,10 +178,28 @@ void clockevents_set_mode(struct clock_event_device *dev, */ void clockevents_shutdown(struct clock_event_device *dev) { - clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN); + clockevents_set_state(dev, CLOCK_EVT_STATE_SHUTDOWN); dev->next_event.tv64 = KTIME_MAX; }
+/** + * clockevent_device_resume - Resume the device before using it again + * @dev: device to resume + */ +int clockevent_device_resume(struct clock_event_device *dev) +{ + int ret = 0; + + if (dev->set_mode) { + dev->set_mode(CLOCK_EVT_MODE_RESUME, dev); + dev->mode = CLOCK_EVT_MODE_RESUME; + } else if (dev->set_state_resume) { + ret = dev->set_state_resume(dev); + } + + return ret; +} + #ifdef CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST
/* Limit min_delta to a jiffie */ @@ -236,7 +252,7 @@ static int clockevents_program_min_delta(struct clock_event_device *dev) delta = dev->min_delta_ns; dev->next_event = ktime_add_ns(ktime_get(), delta);
- if (dev->mode == CLOCK_EVT_MODE_SHUTDOWN) + if (dev->state == CLOCK_EVT_STATE_SHUTDOWN) return 0;
dev->retries++; @@ -273,7 +289,7 @@ static int clockevents_program_min_delta(struct clock_event_device *dev) delta = dev->min_delta_ns; dev->next_event = ktime_add_ns(ktime_get(), delta);
- if (dev->mode == CLOCK_EVT_MODE_SHUTDOWN) + if (dev->state == CLOCK_EVT_STATE_SHUTDOWN) return 0;
dev->retries++; @@ -305,7 +321,7 @@ int clockevents_program_event(struct clock_event_device *dev, ktime_t expires,
dev->next_event = expires;
- if (dev->mode == CLOCK_EVT_MODE_SHUTDOWN) + if (dev->state == CLOCK_EVT_STATE_SHUTDOWN) return 0;
/* Shortcut for clockevent devices that can deal with ktime. */ @@ -350,7 +366,7 @@ static int clockevents_replace(struct clock_event_device *ced) struct clock_event_device *dev, *newdev = NULL;
list_for_each_entry(dev, &clockevent_devices, list) { - if (dev == ced || dev->mode != CLOCK_EVT_MODE_UNUSED) + if (dev == ced || dev->state != CLOCK_EVT_STATE_DETACHED) continue;
if (!tick_check_replacement(newdev, dev)) @@ -376,7 +392,7 @@ static int clockevents_replace(struct clock_event_device *ced) static int __clockevents_try_unbind(struct clock_event_device *ced, int cpu) { /* Fast track. Device is unused */ - if (ced->mode == CLOCK_EVT_MODE_UNUSED) { + if (ced->state == CLOCK_EVT_STATE_DETACHED) { list_del_init(&ced->list); return 0; } @@ -432,8 +448,8 @@ static int clockevents_sanity_check(struct clock_event_device *dev) /* Legacy set_mode() callback */ if (dev->set_mode) { /* We shouldn't be supporting new modes now */ - WARN_ON(dev->set_mode_periodic || dev->set_mode_oneshot || - dev->set_mode_shutdown || dev->set_mode_resume); + WARN_ON(dev->set_state_periodic || dev->set_state_oneshot || + dev->set_state_shutdown || dev->set_state_resume); return 0; }
@@ -441,15 +457,15 @@ static int clockevents_sanity_check(struct clock_event_device *dev) return 0;
/* New mode-specific callbacks */ - if (!dev->set_mode_shutdown) + if (!dev->set_state_shutdown) return -EINVAL;
if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) && - !dev->set_mode_periodic) + !dev->set_state_periodic) return -EINVAL;
if ((dev->features & CLOCK_EVT_FEAT_ONESHOT) && - !dev->set_mode_oneshot) + !dev->set_state_oneshot) return -EINVAL;
return 0; @@ -466,6 +482,9 @@ void clockevents_register_device(struct clock_event_device *dev) BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED); BUG_ON(clockevents_sanity_check(dev));
+ /* Initialize state to DETACHED */ + dev->state = CLOCK_EVT_STATE_DETACHED; + if (!dev->cpumask) { WARN_ON(num_possible_cpus() > 1); dev->cpumask = cpumask_of(smp_processor_id()); @@ -529,11 +548,11 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 freq) { clockevents_config(dev, freq);
- if (dev->mode == CLOCK_EVT_MODE_ONESHOT) + if (dev->state == CLOCK_EVT_STATE_ONESHOT) return clockevents_program_event(dev, dev->next_event, false);
- if (dev->mode == CLOCK_EVT_MODE_PERIODIC) - return __clockevents_set_mode(dev, CLOCK_EVT_MODE_PERIODIC); + if (dev->state == CLOCK_EVT_STATE_PERIODIC) + return __clockevents_set_state(dev, CLOCK_EVT_STATE_PERIODIC);
return 0; } @@ -589,13 +608,13 @@ void clockevents_exchange_device(struct clock_event_device *old, */ if (old) { module_put(old->owner); - clockevents_set_mode(old, CLOCK_EVT_MODE_UNUSED); + clockevents_set_state(old, CLOCK_EVT_STATE_DETACHED); list_del(&old->list); list_add(&old->list, &clockevents_released); }
if (new) { - BUG_ON(new->mode != CLOCK_EVT_MODE_UNUSED); + BUG_ON(new->state != CLOCK_EVT_STATE_DETACHED); clockevents_shutdown(new); } local_irq_restore(flags); @@ -681,7 +700,7 @@ int clockevents_notify(unsigned long reason, void *arg) if (cpumask_test_cpu(cpu, dev->cpumask) && cpumask_weight(dev->cpumask) == 1 && !tick_is_broadcast_device(dev)) { - BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED); + BUG_ON(dev->state != CLOCK_EVT_STATE_DETACHED); list_del(&dev->list); } } diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index 066f0ec05e48..e926618507e9 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -303,7 +303,7 @@ static void tick_handle_periodic_broadcast(struct clock_event_device *dev) /* * The device is in periodic mode. No reprogramming necessary: */ - if (dev->mode == CLOCK_EVT_MODE_PERIODIC) + if (dev->state == CLOCK_EVT_STATE_PERIODIC) goto unlock;
/* @@ -464,7 +464,7 @@ int tick_resume_broadcast(void) bc = tick_broadcast_device.evtdev;
if (bc) { - clockevents_set_mode(bc, CLOCK_EVT_MODE_RESUME); + clockevent_device_resume(bc);
switch (tick_broadcast_device.mode) { case TICKDEV_MODE_PERIODIC: @@ -532,8 +532,8 @@ static int tick_broadcast_set_event(struct clock_event_device *bc, int cpu, { int ret;
- if (bc->mode != CLOCK_EVT_MODE_ONESHOT) - clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); + if (bc->state != CLOCK_EVT_STATE_ONESHOT) + clockevents_set_state(bc, CLOCK_EVT_STATE_ONESHOT);
ret = clockevents_program_event(bc, expires, force); if (!ret) @@ -543,7 +543,7 @@ static int tick_broadcast_set_event(struct clock_event_device *bc, int cpu,
int tick_resume_broadcast_oneshot(struct clock_event_device *bc) { - clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); + clockevents_set_state(bc, CLOCK_EVT_STATE_ONESHOT); return 0; }
@@ -562,8 +562,8 @@ void tick_check_oneshot_broadcast_this_cpu(void) * switched over, leave the device alone. */ if (td->mode == TICKDEV_MODE_ONESHOT) { - clockevents_set_mode(td->evtdev, - CLOCK_EVT_MODE_ONESHOT); + clockevents_set_state(td->evtdev, + CLOCK_EVT_STATE_ONESHOT); } } } @@ -666,7 +666,7 @@ static void broadcast_shutdown_local(struct clock_event_device *bc, if (dev->next_event.tv64 < bc->next_event.tv64) return; } - clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN); + clockevents_set_state(dev, CLOCK_EVT_STATE_SHUTDOWN); }
static void broadcast_move_bc(int deadcpu) @@ -741,7 +741,7 @@ int tick_broadcast_oneshot_control(unsigned long reason) cpumask_clear_cpu(cpu, tick_broadcast_oneshot_mask); } else { if (cpumask_test_and_clear_cpu(cpu, tick_broadcast_oneshot_mask)) { - clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); + clockevents_set_state(dev, CLOCK_EVT_STATE_ONESHOT); /* * The cpu which was handling the broadcast * timer marked this cpu in the broadcast @@ -842,7 +842,7 @@ void tick_broadcast_setup_oneshot(struct clock_event_device *bc)
/* Set it up only once ! */ if (bc->event_handler != tick_handle_oneshot_broadcast) { - int was_periodic = bc->mode == CLOCK_EVT_MODE_PERIODIC; + int was_periodic = bc->state == CLOCK_EVT_STATE_PERIODIC;
bc->event_handler = tick_handle_oneshot_broadcast;
@@ -858,7 +858,7 @@ void tick_broadcast_setup_oneshot(struct clock_event_device *bc) tick_broadcast_oneshot_mask, tmpmask);
if (was_periodic && !cpumask_empty(tmpmask)) { - clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); + clockevents_set_state(bc, CLOCK_EVT_STATE_ONESHOT); tick_broadcast_init_next_event(tmpmask, tick_next_period); tick_broadcast_set_event(bc, cpu, tick_next_period, 1); diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index f7c515595b42..cedc8a07ef85 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -102,7 +102,7 @@ void tick_handle_periodic(struct clock_event_device *dev)
tick_periodic(cpu);
- if (dev->mode != CLOCK_EVT_MODE_ONESHOT) + if (dev->state != CLOCK_EVT_STATE_ONESHOT) return; for (;;) { /* @@ -140,7 +140,7 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) && !tick_broadcast_oneshot_active()) { - clockevents_set_mode(dev, CLOCK_EVT_MODE_PERIODIC); + clockevents_set_state(dev, CLOCK_EVT_STATE_PERIODIC); } else { unsigned long seq; ktime_t next; @@ -150,7 +150,7 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast) next = tick_next_period; } while (read_seqretry(&jiffies_lock, seq));
- clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); + clockevents_set_state(dev, CLOCK_EVT_STATE_ONESHOT);
for (;;) { if (!clockevents_program_event(dev, next, false)) @@ -365,6 +365,7 @@ void tick_shutdown(unsigned int *cpup) * Prevent that the clock events layer tries to call * the set mode function! */ + dev->state = CLOCK_EVT_STATE_DETACHED; dev->mode = CLOCK_EVT_MODE_UNUSED; clockevents_exchange_device(dev, NULL); dev->event_handler = clockevents_handle_noop; @@ -384,7 +385,7 @@ void tick_resume(void) struct tick_device *td = this_cpu_ptr(&tick_cpu_device); int broadcast = tick_resume_broadcast();
- clockevents_set_mode(td->evtdev, CLOCK_EVT_MODE_RESUME); + clockevent_device_resume(td->evtdev);
if (!broadcast) { if (td->mode == TICKDEV_MODE_PERIODIC) diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h index 366aeb4f2c66..71744543a676 100644 --- a/kernel/time/tick-internal.h +++ b/kernel/time/tick-internal.h @@ -32,6 +32,7 @@ extern bool tick_check_replacement(struct clock_event_device *curdev, extern void tick_install_replacement(struct clock_event_device *dev);
extern void clockevents_shutdown(struct clock_event_device *dev); +extern int clockevent_device_resume(struct clock_event_device *dev);
extern ssize_t sysfs_get_uname(const char *buf, char *dst, size_t cnt);
diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c index 7ce740e78e1b..67a64b1670bf 100644 --- a/kernel/time/tick-oneshot.c +++ b/kernel/time/tick-oneshot.c @@ -38,7 +38,7 @@ void tick_resume_oneshot(void) { struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
- clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); + clockevents_set_state(dev, CLOCK_EVT_STATE_ONESHOT); clockevents_program_event(dev, ktime_get(), true); }
@@ -50,7 +50,7 @@ void tick_setup_oneshot(struct clock_event_device *newdev, ktime_t next_event) { newdev->event_handler = handler; - clockevents_set_mode(newdev, CLOCK_EVT_MODE_ONESHOT); + clockevents_set_state(newdev, CLOCK_EVT_STATE_ONESHOT); clockevents_program_event(newdev, next_event, true); }
@@ -81,7 +81,7 @@ int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *))
td->mode = TICKDEV_MODE_ONESHOT; dev->event_handler = handler; - clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); + clockevents_set_state(dev, CLOCK_EVT_STATE_ONESHOT); tick_broadcast_switch_to_oneshot(); return 0; } diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c index 2cfd19485824..1d8c69f8e5be 100644 --- a/kernel/time/timer_list.c +++ b/kernel/time/timer_list.c @@ -233,27 +233,27 @@ print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu) print_name_offset(m, dev->set_mode); SEQ_printf(m, "\n"); } else { - if (dev->set_mode_shutdown) { + if (dev->set_state_shutdown) { SEQ_printf(m, " shutdown: "); - print_name_offset(m, dev->set_mode_shutdown); + print_name_offset(m, dev->set_state_shutdown); SEQ_printf(m, "\n"); }
- if (dev->set_mode_periodic) { + if (dev->set_state_periodic) { SEQ_printf(m, " periodic: "); - print_name_offset(m, dev->set_mode_periodic); + print_name_offset(m, dev->set_state_periodic); SEQ_printf(m, "\n"); }
- if (dev->set_mode_oneshot) { + if (dev->set_state_oneshot) { SEQ_printf(m, " oneshot: "); - print_name_offset(m, dev->set_mode_oneshot); + print_name_offset(m, dev->set_state_oneshot); SEQ_printf(m, "\n"); }
- if (dev->set_mode_resume) { + if (dev->set_state_resume) { SEQ_printf(m, " resume: "); - print_name_offset(m, dev->set_mode_resume); + print_name_offset(m, dev->set_state_resume); SEQ_printf(m, "\n"); } }
* viresh kumar viresh.kumar@linaro.org wrote:
On Monday 23 February 2015 10:07 PM, Ingo Molnar wrote:>
- Viresh Kumar viresh.kumar@linaro.org wrote:
Ok, could we rename it to something like DETACHED?
'UNUSED' really gives me the wrong impression - it's what we do for unused fields, unused ABI enumertion constants, etc.
Sure.
Also, I'd suggest to rename all 'modes' to true state machine naming: STATE_INITIALIZED, STATE_SHUT_DOWN, STATE_PERIODIC, STATE_RESUMED, etc.: if these are enums for
I thought so initially and it looked like the diff will be huge as all the variables for the enum, i.e. 'mode', need to be renamed to 'state'..
But, if you are okay with it then I would be happy to do that..
Well, how does the diffstat look like?
But if the conversion was scripted (i.e. is relatively secure from typos) then that's still fine IMO. Peter?
Here is the diff:
And here's the diffstat:
arch/arm/common/bL_switcher.c | 8 +-- include/linux/clockchips.h | 48 ++++++++++++------ kernel/time/clockevents.c | 109 ++++++++++++++++++++++++------------------ kernel/time/tick-broadcast.c | 22 ++++---- kernel/time/tick-common.c | 9 +-- kernel/time/tick-internal.h | 1 kernel/time/tick-oneshot.c | 6 +- kernel/time/timer_list.c | 16 +++--- 8 files changed, 129 insertions(+), 90 deletions(-)
Not too bad I think.
Thanks,
Ingo
On 24 February 2015 at 20:24, Ingo Molnar mingo@kernel.org wrote:
And here's the diffstat:
Sorry for not providing that :(
arch/arm/common/bL_switcher.c | 8 +-- include/linux/clockchips.h | 48 ++++++++++++------ kernel/time/clockevents.c | 109 ++++++++++++++++++++++++------------------ kernel/time/tick-broadcast.c | 22 ++++---- kernel/time/tick-common.c | 9 +-- kernel/time/tick-internal.h | 1 kernel/time/tick-oneshot.c | 6 +- kernel/time/timer_list.c | 16 +++--- 8 files changed, 129 insertions(+), 90 deletions(-)
Not too bad I think.
Thanks, I will send the patches now after some rework.
linaro-kernel@lists.linaro.org
-
Ingo Molnar -
Peter Zijlstra -
Viresh Kumar -
viresh kumar