| Message ID | 1560143274-10547-1-git-send-email-chenbaodong@mxnavi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: vtimer: fix return value to void for virt_timer_[save|restore] | expand |
Hi, NIT: I would use "change" instead of "fix". I feel "fix" is more when there are an actual bug. On 6/10/19 6:07 AM, Baodong Chen wrote: > The original type is int and not used at all so fix to void. The commit message is a bit unclear, you mention the type whereas the key point is none of the callers are using the return value. So how about: "virt_timer_{save, return} always return 0 and none of the caller actually check it. So change the return type to void." If you are happy with it, I can make the modifications them on commit. Cheers, > > Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com> > --- > xen/arch/arm/vtimer.c | 6 ++---- > xen/include/asm-arm/vtimer.h | 4 ++-- > 2 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c > index c99dd23..e6aebda 100644 > --- a/xen/arch/arm/vtimer.c > +++ b/xen/arch/arm/vtimer.c > @@ -136,7 +136,7 @@ void vcpu_timer_destroy(struct vcpu *v) > kill_timer(&v->arch.phys_timer.timer); > } > > -int virt_timer_save(struct vcpu *v) > +void virt_timer_save(struct vcpu *v) > { > ASSERT(!is_idle_vcpu(v)); > > @@ -149,10 +149,9 @@ int virt_timer_save(struct vcpu *v) > set_timer(&v->arch.virt_timer.timer, ticks_to_ns(v->arch.virt_timer.cval + > v->domain->arch.virt_timer_base.offset - boot_count)); > } > - return 0; > } > > -int virt_timer_restore(struct vcpu *v) > +void virt_timer_restore(struct vcpu *v) > { > ASSERT(!is_idle_vcpu(v)); > > @@ -163,7 +162,6 @@ int virt_timer_restore(struct vcpu *v) > WRITE_SYSREG64(v->domain->arch.virt_timer_base.offset, CNTVOFF_EL2); > WRITE_SYSREG64(v->arch.virt_timer.cval, CNTV_CVAL_EL0); > WRITE_SYSREG32(v->arch.virt_timer.ctl, CNTV_CTL_EL0); > - return 0; > } > > static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read) > diff --git a/xen/include/asm-arm/vtimer.h b/xen/include/asm-arm/vtimer.h > index 91d88b3..9d4fb4c 100644 > --- a/xen/include/asm-arm/vtimer.h > +++ b/xen/include/asm-arm/vtimer.h > @@ -24,8 +24,8 @@ extern int domain_vtimer_init(struct domain *d, > struct xen_arch_domainconfig *config); > extern int vcpu_vtimer_init(struct vcpu *v); > extern bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr); > -extern int virt_timer_save(struct vcpu *v); > -extern int virt_timer_restore(struct vcpu *v); > +extern void virt_timer_save(struct vcpu *v); > +extern void virt_timer_restore(struct vcpu *v); > extern void vcpu_timer_destroy(struct vcpu *v); > void vtimer_update_irqs(struct vcpu *v); > >
On 6/11/19 04:16, Julien Grall wrote: > Hi, > > NIT: I would use "change" instead of "fix". I feel "fix" is more when > there are an actual bug. Sound good to me. > > On 6/10/19 6:07 AM, Baodong Chen wrote: >> The original type is int and not used at all so fix to void. > > The commit message is a bit unclear, you mention the type whereas the > key point is none of the callers are using the return value. So how > about: > > "virt_timer_{save, return} always return 0 and none of the caller > actually check it. So change the return type to void." > > If you are happy with it, I can make the modifications them on commit. happy with it, please. > > Cheers, > >> >> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com> >> --- >> xen/arch/arm/vtimer.c | 6 ++---- >> xen/include/asm-arm/vtimer.h | 4 ++-- >> 2 files changed, 4 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c >> index c99dd23..e6aebda 100644 >> --- a/xen/arch/arm/vtimer.c >> +++ b/xen/arch/arm/vtimer.c >> @@ -136,7 +136,7 @@ void vcpu_timer_destroy(struct vcpu *v) >> kill_timer(&v->arch.phys_timer.timer); >> } >> -int virt_timer_save(struct vcpu *v) >> +void virt_timer_save(struct vcpu *v) >> { >> ASSERT(!is_idle_vcpu(v)); >> @@ -149,10 +149,9 @@ int virt_timer_save(struct vcpu *v) >> set_timer(&v->arch.virt_timer.timer, >> ticks_to_ns(v->arch.virt_timer.cval + >> v->domain->arch.virt_timer_base.offset - >> boot_count)); >> } >> - return 0; >> } >> -int virt_timer_restore(struct vcpu *v) >> +void virt_timer_restore(struct vcpu *v) >> { >> ASSERT(!is_idle_vcpu(v)); >> @@ -163,7 +162,6 @@ int virt_timer_restore(struct vcpu *v) >> WRITE_SYSREG64(v->domain->arch.virt_timer_base.offset, CNTVOFF_EL2); >> WRITE_SYSREG64(v->arch.virt_timer.cval, CNTV_CVAL_EL0); >> WRITE_SYSREG32(v->arch.virt_timer.ctl, CNTV_CTL_EL0); >> - return 0; >> } >> static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t >> *r, bool read) >> diff --git a/xen/include/asm-arm/vtimer.h b/xen/include/asm-arm/vtimer.h >> index 91d88b3..9d4fb4c 100644 >> --- a/xen/include/asm-arm/vtimer.h >> +++ b/xen/include/asm-arm/vtimer.h >> @@ -24,8 +24,8 @@ extern int domain_vtimer_init(struct domain *d, >> struct xen_arch_domainconfig *config); >> extern int vcpu_vtimer_init(struct vcpu *v); >> extern bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr); >> -extern int virt_timer_save(struct vcpu *v); >> -extern int virt_timer_restore(struct vcpu *v); >> +extern void virt_timer_save(struct vcpu *v); >> +extern void virt_timer_restore(struct vcpu *v); >> extern void vcpu_timer_destroy(struct vcpu *v); >> void vtimer_update_irqs(struct vcpu *v); >> >
On 11/06/2019 01:06, chenbaodong wrote: > > On 6/11/19 04:16, Julien Grall wrote: >> Hi, >> >> NIT: I would use "change" instead of "fix". I feel "fix" is more when there >> are an actual bug. > Sound good to me. >> >> On 6/10/19 6:07 AM, Baodong Chen wrote: >>> The original type is int and not used at all so fix to void. >> >> The commit message is a bit unclear, you mention the type whereas the key >> point is none of the callers are using the return value. So how about: >> >> "virt_timer_{save, return} always return 0 and none of the caller actually >> check it. So change the return type to void." >> >> If you are happy with it, I can make the modifications them on commit. > happy with it, please. Committed, thank you! Cheers,
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c index c99dd23..e6aebda 100644 --- a/xen/arch/arm/vtimer.c +++ b/xen/arch/arm/vtimer.c @@ -136,7 +136,7 @@ void vcpu_timer_destroy(struct vcpu *v) kill_timer(&v->arch.phys_timer.timer); } -int virt_timer_save(struct vcpu *v) +void virt_timer_save(struct vcpu *v) { ASSERT(!is_idle_vcpu(v)); @@ -149,10 +149,9 @@ int virt_timer_save(struct vcpu *v) set_timer(&v->arch.virt_timer.timer, ticks_to_ns(v->arch.virt_timer.cval + v->domain->arch.virt_timer_base.offset - boot_count)); } - return 0; } -int virt_timer_restore(struct vcpu *v) +void virt_timer_restore(struct vcpu *v) { ASSERT(!is_idle_vcpu(v)); @@ -163,7 +162,6 @@ int virt_timer_restore(struct vcpu *v) WRITE_SYSREG64(v->domain->arch.virt_timer_base.offset, CNTVOFF_EL2); WRITE_SYSREG64(v->arch.virt_timer.cval, CNTV_CVAL_EL0); WRITE_SYSREG32(v->arch.virt_timer.ctl, CNTV_CTL_EL0); - return 0; } static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read) diff --git a/xen/include/asm-arm/vtimer.h b/xen/include/asm-arm/vtimer.h index 91d88b3..9d4fb4c 100644 --- a/xen/include/asm-arm/vtimer.h +++ b/xen/include/asm-arm/vtimer.h @@ -24,8 +24,8 @@ extern int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config); extern int vcpu_vtimer_init(struct vcpu *v); extern bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr); -extern int virt_timer_save(struct vcpu *v); -extern int virt_timer_restore(struct vcpu *v); +extern void virt_timer_save(struct vcpu *v); +extern void virt_timer_restore(struct vcpu *v); extern void vcpu_timer_destroy(struct vcpu *v); void vtimer_update_irqs(struct vcpu *v);
The original type is int and not used at all so fix to void. Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com> --- xen/arch/arm/vtimer.c | 6 ++---- xen/include/asm-arm/vtimer.h | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-)