Message ID | 20240402154339.2448435-1-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | x86: Address MISRA Rule 13.6 | expand |
On 02.04.2024 17:43, Andrew Cooper wrote: > MISRA Rule 13.6 doesn't like having an expression in a sizeof() which > potentially has side effects. > > Address several violations by pulling the expression out into a local > variable. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with one caveat: > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) > { > struct domain *d = action->guest[i]; > unsigned int pirq = domain_irq_to_pirq(d, irq); > + struct pirq *pirq_info = pirq_info(d, pirq); Misra won't like the var's name matching the macro's. Can we go with just "info"? Jan
On 02/04/2024 4:46 pm, Jan Beulich wrote: > On 02.04.2024 17:43, Andrew Cooper wrote: >> MISRA Rule 13.6 doesn't like having an expression in a sizeof() which >> potentially has side effects. >> >> Address several violations by pulling the expression out into a local >> variable. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with one caveat: > >> --- a/xen/arch/x86/irq.c >> +++ b/xen/arch/x86/irq.c >> @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) >> { >> struct domain *d = action->guest[i]; >> unsigned int pirq = domain_irq_to_pirq(d, irq); >> + struct pirq *pirq_info = pirq_info(d, pirq); > Misra won't like the var's name matching the macro's. Can we go with just > "info"? Ah - missed that. I can name it to just info, but I considered "struct pirq *info" to be a little odd. ~Andrew
On 02.04.2024 17:54, Andrew Cooper wrote: > On 02/04/2024 4:46 pm, Jan Beulich wrote: >> On 02.04.2024 17:43, Andrew Cooper wrote: >>> MISRA Rule 13.6 doesn't like having an expression in a sizeof() which >>> potentially has side effects. >>> >>> Address several violations by pulling the expression out into a local >>> variable. >>> >>> No functional change. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> with one caveat: >> >>> --- a/xen/arch/x86/irq.c >>> +++ b/xen/arch/x86/irq.c >>> @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) >>> { >>> struct domain *d = action->guest[i]; >>> unsigned int pirq = domain_irq_to_pirq(d, irq); >>> + struct pirq *pirq_info = pirq_info(d, pirq); >> Misra won't like the var's name matching the macro's. Can we go with just >> "info"? > > Ah - missed that. > > I can name it to just info, but I considered "struct pirq *info" to be a > little odd. I agree, but what do you do with another "pirq" already there. Or wait, what about struct pirq *pirq = pirq_info(d, domain_irq_to_pirq(d, irq)); ? Jan
On 02/04/2024 5:06 pm, Jan Beulich wrote: > On 02.04.2024 17:54, Andrew Cooper wrote: >> On 02/04/2024 4:46 pm, Jan Beulich wrote: >>> On 02.04.2024 17:43, Andrew Cooper wrote: >>>> MISRA Rule 13.6 doesn't like having an expression in a sizeof() which >>>> potentially has side effects. >>>> >>>> Address several violations by pulling the expression out into a local >>>> variable. >>>> >>>> No functional change. >>>> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> with one caveat: >>> >>>> --- a/xen/arch/x86/irq.c >>>> +++ b/xen/arch/x86/irq.c >>>> @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) >>>> { >>>> struct domain *d = action->guest[i]; >>>> unsigned int pirq = domain_irq_to_pirq(d, irq); >>>> + struct pirq *pirq_info = pirq_info(d, pirq); >>> Misra won't like the var's name matching the macro's. Can we go with just >>> "info"? >> Ah - missed that. >> >> I can name it to just info, but I considered "struct pirq *info" to be a >> little odd. > I agree, but what do you do with another "pirq" already there. > > Or wait, what about > > struct pirq *pirq = pirq_info(d, domain_irq_to_pirq(d, irq)); > > ? That should work. I'll switch to this locally, and wait for the feedback on whether the patch works for 13.6. ~Andrew
On 02/04/24 17:54, Andrew Cooper wrote: > On 02/04/2024 4:46 pm, Jan Beulich wrote: >> On 02.04.2024 17:43, Andrew Cooper wrote: >>> MISRA Rule 13.6 doesn't like having an expression in a sizeof() which >>> potentially has side effects. >>> >>> Address several violations by pulling the expression out into a local >>> variable. >>> >>> No functional change. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> with one caveat: >> >>> --- a/xen/arch/x86/irq.c >>> +++ b/xen/arch/x86/irq.c >>> @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) >>> { >>> struct domain *d = action->guest[i]; >>> unsigned int pirq = domain_irq_to_pirq(d, irq); >>> + struct pirq *pirq_info = pirq_info(d, pirq); >> Misra won't like the var's name matching the macro's. Can we go with just >> "info"? > > Ah - missed that. > > I can name it to just info, but I considered "struct pirq *info" to be a > little odd. The local variable is a non-callable entity; "clashes" between function-like macros and non-callable entities will be deviated (as agreed during MISRA meetings).
On 02/04/24 18:09, Andrew Cooper wrote: > On 02/04/2024 5:06 pm, Jan Beulich wrote: >> On 02.04.2024 17:54, Andrew Cooper wrote: >>> On 02/04/2024 4:46 pm, Jan Beulich wrote: >>>> On 02.04.2024 17:43, Andrew Cooper wrote: >>>>> MISRA Rule 13.6 doesn't like having an expression in a sizeof() which >>>>> potentially has side effects. >>>>> >>>>> Address several violations by pulling the expression out into a local >>>>> variable. >>>>> >>>>> No functional change. >>>>> >>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>> with one caveat: >>>> >>>>> --- a/xen/arch/x86/irq.c >>>>> +++ b/xen/arch/x86/irq.c >>>>> @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) >>>>> { >>>>> struct domain *d = action->guest[i]; >>>>> unsigned int pirq = domain_irq_to_pirq(d, irq); >>>>> + struct pirq *pirq_info = pirq_info(d, pirq); >>>> Misra won't like the var's name matching the macro's. Can we go with just >>>> "info"? >>> Ah - missed that. >>> >>> I can name it to just info, but I considered "struct pirq *info" to be a >>> little odd. >> I agree, but what do you do with another "pirq" already there. >> >> Or wait, what about >> >> struct pirq *pirq = pirq_info(d, domain_irq_to_pirq(d, irq)); >> >> ? > > That should work. I'll switch to this locally, and wait for the > feedback on whether the patch works for 13.6. I confirm that both versions of the patch address some violations of 13.6.
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index 0487f734a5d2..d73f687f7617 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data) { struct domain *d = action->guest[i]; unsigned int pirq = domain_irq_to_pirq(d, irq); + struct pirq *pirq_info = pirq_info(d, pirq); - if ( test_and_clear_bool(pirq_info(d, pirq)->masked) ) + if ( test_and_clear_bool(pirq_info->masked) ) action->in_flight--; } diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index 60870047894b..6f136f4b14bf 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -2503,7 +2503,9 @@ static long cmos_utc_offset; /* in seconds */ int time_suspend(void) { - if ( smp_processor_id() == 0 ) + unsigned int cpu = smp_processor_id(); + + if ( cpu == 0 ) { cmos_utc_offset = -get_wallclock_time(); cmos_utc_offset += get_sec(); @@ -2514,7 +2516,7 @@ int time_suspend(void) } /* Better to cancel calibration timer for accuracy. */ - clear_bit(TIME_CALIBRATE_SOFTIRQ, &softirq_pending(smp_processor_id())); + clear_bit(TIME_CALIBRATE_SOFTIRQ, &softirq_pending(cpu)); return 0; } diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c index d9eefcd8e411..7fc796dec25b 100644 --- a/xen/drivers/passthrough/amd/iommu_intr.c +++ b/xen/drivers/passthrough/amd/iommu_intr.c @@ -183,6 +183,7 @@ static void free_intremap_entry(const struct amd_iommu *iommu, unsigned int bdf, unsigned int index) { union irte_ptr entry = get_intremap_entry(iommu, bdf, index); + struct ivrs_mappings *ivrs = get_ivrs_mappings(iommu->seg); if ( iommu->ctrl.ga_en ) { @@ -201,7 +202,7 @@ static void free_intremap_entry(const struct amd_iommu *iommu, else ACCESS_ONCE(entry.ptr32->raw) = 0; - __clear_bit(index, get_ivrs_mappings(iommu->seg)[bdf].intremap_inuse); + __clear_bit(index, ivrs[bdf].intremap_inuse); } static void update_intremap_entry(const struct amd_iommu *iommu,
MISRA Rule 13.6 doesn't like having an expression in a sizeof() which potentially has side effects. Address several violations by pulling the expression out into a local variable. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> CC: consulting@bugseng.com <consulting@bugseng.com> Slightly RFC. --- xen/arch/x86/irq.c | 3 ++- xen/arch/x86/time.c | 6 ++++-- xen/drivers/passthrough/amd/iommu_intr.c | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-) base-commit: 7a09966e7b2823b70f6d56d0cf66c11124f4a3c1