| Message ID | 512FC581-4097-4433-9C3D-CBCB7CD61954@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clocksource: riscv: Patch riscv_clock_next_event() jump before first use | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be for-next |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 13 and now 13 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/alphanumeric_selects | success | Out of order selects before the patch: 59 and now 59 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 2 this patch: 2 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 22 lines checked |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
Hey Matt, On Wed, Feb 01, 2023 at 06:41:52PM +0000, Matt Evans wrote: > A static key is used to select between SBI and Sstc timer usage in > riscv_clock_next_event(), but currently the direction is resolved > after cpuhp_setup_state() is called (which sets the next event). The > first event will therefore fall through the sbi_set_timer() path; this > breaks Sstc-only systems. So, apply the jump patching before first > use. > > Signed-off-by: Matt Evans <mev@rivosinc.com> Your logic in this patch looks grand, just got two process bits for ya. Firstly, you're missing a fixes tag, which I would imagine is: Fixes: 9f7a8ff6391f ("RISC-V: Prefer sstc extension if available") Also, of late, the clocksource maintainers, who you have not CCed, have been the ones that pick up patches that only touch this driver. Might be worth a resubmission CCing them... Cheers, Conor. > --- > drivers/clocksource/timer-riscv.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c > index 969a552da8d2..a36d173fd6cd 100644 > --- a/drivers/clocksource/timer-riscv.c > +++ b/drivers/clocksource/timer-riscv.c > @@ -177,6 +177,11 @@ static int __init riscv_timer_init_dt(struct device_node *n) > return error; > } > > + if (riscv_isa_extension_available(NULL, SSTC)) { > + pr_info("Timer interrupt in S-mode is available via sstc extension\n"); > + static_branch_enable(&riscv_sstc_available); > + } > + > error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING, > "clockevents/riscv/timer:starting", > riscv_timer_starting_cpu, riscv_timer_dying_cpu); > @@ -184,11 +189,6 @@ static int __init riscv_timer_init_dt(struct device_node *n) > pr_err("cpu hp setup state failed for RISCV timer [%d]\n", > error); > > - if (riscv_isa_extension_available(NULL, SSTC)) { > - pr_info("Timer interrupt in S-mode is available via sstc extension\n"); > - static_branch_enable(&riscv_sstc_available); > - } > - > return error; > } > > -- > 2.30.2 > > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Conor, > On 1 Feb 2023, at 19:21, Conor Dooley <conor@kernel.org> wrote: > > Hey Matt, > > On Wed, Feb 01, 2023 at 06:41:52PM +0000, Matt Evans wrote: >> A static key is used to select between SBI and Sstc timer usage in >> riscv_clock_next_event(), but currently the direction is resolved >> after cpuhp_setup_state() is called (which sets the next event). The >> first event will therefore fall through the sbi_set_timer() path; this >> breaks Sstc-only systems. So, apply the jump patching before first >> use. >> >> Signed-off-by: Matt Evans <mev@rivosinc.com> > > Your logic in this patch looks grand, just got two process bits for ya. > Firstly, you're missing a fixes tag, which I would imagine is: > Fixes: 9f7a8ff6391f ("RISC-V: Prefer sstc extension if available") > > Also, of late, the clocksource maintainers, who you have not CCed, > have been the ones that pick up patches that only touch this driver. > Might be worth a resubmission CCing them... Both good points, many thanks. True, it fixes(:) 9f7a8ff6391f, and TIL on the shepherd chain. I’ll re-send. Cheers, Matt > > Cheers, > Conor. > >> --- >> drivers/clocksource/timer-riscv.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c >> index 969a552da8d2..a36d173fd6cd 100644 >> --- a/drivers/clocksource/timer-riscv.c >> +++ b/drivers/clocksource/timer-riscv.c >> @@ -177,6 +177,11 @@ static int __init riscv_timer_init_dt(struct device_node *n) >> return error; >> } >> >> + if (riscv_isa_extension_available(NULL, SSTC)) { >> + pr_info("Timer interrupt in S-mode is available via sstc extension\n"); >> + static_branch_enable(&riscv_sstc_available); >> + } >> + >> error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING, >> "clockevents/riscv/timer:starting", >> riscv_timer_starting_cpu, riscv_timer_dying_cpu); >> @@ -184,11 +189,6 @@ static int __init riscv_timer_init_dt(struct device_node *n) >> pr_err("cpu hp setup state failed for RISCV timer [%d]\n", >> error); >> >> - if (riscv_isa_extension_available(NULL, SSTC)) { >> - pr_info("Timer interrupt in S-mode is available via sstc extension\n"); >> - static_branch_enable(&riscv_sstc_available); >> - } >> - >> return error; >> } >> >> -- >> 2.30.2 >> >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c index 969a552da8d2..a36d173fd6cd 100644 --- a/drivers/clocksource/timer-riscv.c +++ b/drivers/clocksource/timer-riscv.c @@ -177,6 +177,11 @@ static int __init riscv_timer_init_dt(struct device_node *n) return error; } + if (riscv_isa_extension_available(NULL, SSTC)) { + pr_info("Timer interrupt in S-mode is available via sstc extension\n"); + static_branch_enable(&riscv_sstc_available); + } + error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING, "clockevents/riscv/timer:starting", riscv_timer_starting_cpu, riscv_timer_dying_cpu); @@ -184,11 +189,6 @@ static int __init riscv_timer_init_dt(struct device_node *n) pr_err("cpu hp setup state failed for RISCV timer [%d]\n", error); - if (riscv_isa_extension_available(NULL, SSTC)) { - pr_info("Timer interrupt in S-mode is available via sstc extension\n"); - static_branch_enable(&riscv_sstc_available); - } - return error; }
A static key is used to select between SBI and Sstc timer usage in riscv_clock_next_event(), but currently the direction is resolved after cpuhp_setup_state() is called (which sets the next event). The first event will therefore fall through the sbi_set_timer() path; this breaks Sstc-only systems. So, apply the jump patching before first use. Signed-off-by: Matt Evans <mev@rivosinc.com> --- drivers/clocksource/timer-riscv.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)