| Message ID | 20201231150936.4013764-1-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Remove spurious semicolon in reg_to_encoding() | expand |
Hi Marc, On 12/31/20 3:09 PM, Marc Zyngier wrote: > Although not a problem right now, it flared up while working > on some other aspects of the code-base. Remove the useless > semicolon. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/sys_regs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index d46e7f706cb0..42ccc27fb684 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -923,7 +923,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > > #define reg_to_encoding(x) \ > sys_reg((u32)(x)->Op0, (u32)(x)->Op1, \ > - (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2); > + (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2) Macro's shouldn't really end with a semicolon, otherwise code might break unexpectedly: Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> I skimmed over the code and the macro is used like a function. Do you think it's worth turning it into a function to gain type checking for the argument (which is expected to be sys_reg_desc) and for the return value? Thanks, Alex > > /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */ > #define DBG_BCR_BVR_WCR_WVR_EL1(n) \
On 2021-01-05 13:54, Alexandru Elisei wrote: > Hi Marc, > > On 12/31/20 3:09 PM, Marc Zyngier wrote: >> Although not a problem right now, it flared up while working >> on some other aspects of the code-base. Remove the useless >> semicolon. >> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> arch/arm64/kvm/sys_regs.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index d46e7f706cb0..42ccc27fb684 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -923,7 +923,7 @@ static bool access_pmuserenr(struct kvm_vcpu >> *vcpu, struct sys_reg_params *p, >> >> #define reg_to_encoding(x) \ >> sys_reg((u32)(x)->Op0, (u32)(x)->Op1, \ >> - (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2); >> + (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2) > > Macro's shouldn't really end with a semicolon, otherwise code might > break > unexpectedly: > > Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> > > I skimmed over the code and the macro is used like a function. Do you > think it's > worth turning it into a function to gain type checking for the > argument (which is > expected to be sys_reg_desc) and for the return value? Not quite. This macro takes indifferently a sys_reg_desc or a sys_reg_params, making it impossible to turn into a function. Is it disgusting and makes you want to pull your eyes out? Yes. The whole params/desc duality dates back to the early 32bit code, and I blame Rusty Russell for it. Just saying... ;-) That's one of things I always wanted to refactor... Thanks, M.
Hi Marc, On 1/5/21 2:04 PM, Marc Zyngier wrote: > On 2021-01-05 13:54, Alexandru Elisei wrote: >> Hi Marc, >> >> On 12/31/20 3:09 PM, Marc Zyngier wrote: >>> Although not a problem right now, it flared up while working >>> on some other aspects of the code-base. Remove the useless >>> semicolon. >>> >>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>> --- >>> arch/arm64/kvm/sys_regs.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>> index d46e7f706cb0..42ccc27fb684 100644 >>> --- a/arch/arm64/kvm/sys_regs.c >>> +++ b/arch/arm64/kvm/sys_regs.c >>> @@ -923,7 +923,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct >>> sys_reg_params *p, >>> >>> #define reg_to_encoding(x) \ >>> sys_reg((u32)(x)->Op0, (u32)(x)->Op1, \ >>> - (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2); >>> + (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2) >> >> Macro's shouldn't really end with a semicolon, otherwise code might break >> unexpectedly: >> >> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> >> >> I skimmed over the code and the macro is used like a function. Do you think it's >> worth turning it into a function to gain type checking for the >> argument (which is >> expected to be sys_reg_desc) and for the return value? > > Not quite. > > This macro takes indifferently a sys_reg_desc or a sys_reg_params, Right, find_reg() calls reg_to_encoding() with a struct sys_reg_params argument, I missed that. Found a few instances where sys_reg() calls can be replaced with reg_to_encoding(), will send a trivial patch for that. Thanks, Alex > making it impossible to turn into a function. Is it disgusting and > makes you want to pull your eyes out? Yes. > > The whole params/desc duality dates back to the early 32bit code, > and I blame Rusty Russell for it. Just saying... ;-) > > That's one of things I always wanted to refactor... > > Thanks, > > M.
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index d46e7f706cb0..42ccc27fb684 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -923,7 +923,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, #define reg_to_encoding(x) \ sys_reg((u32)(x)->Op0, (u32)(x)->Op1, \ - (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2); + (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2) /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */ #define DBG_BCR_BVR_WCR_WVR_EL1(n) \
Although not a problem right now, it flared up while working on some other aspects of the code-base. Remove the useless semicolon. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/sys_regs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)