| Message ID | d1bdd54b6751a047626b0271fff882484f98ea1a.1712305581.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C Rule 16.2 | expand |
On 05.04.2024 11:14, Nicola Vetrini wrote: > Remove unneded blank lines between switch clauses. NAK for this part again. > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -2882,7 +2882,7 @@ int allocate_and_map_gsi_pirq(struct domain *d, int index, int *pirq_p) > int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p, > int type, struct msi_info *msi) > { > - int irq, pirq, ret; > + int irq = -1, pirq, ret; > > ASSERT_PDEV_LIST_IS_READ_LOCKED(d); > > @@ -2892,12 +2892,10 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p, > if ( !msi->table_base ) > msi->entry_nr = 1; > irq = index; > - if ( irq == -1 ) > - { > + fallthrough; > case MAP_PIRQ_TYPE_MULTI_MSI: > + if( type == MAP_PIRQ_TYPE_MULTI_MSI || irq == -1 ) > irq = create_irq(NUMA_NO_NODE, true); It may seem small, but this extra comparison already is duplication I'd rather see avoided. At the very least though you want to clarify in the description whether the compiler manages to eliminate it again. > - } > - > if ( irq < nr_irqs_gsi || irq >= nr_irqs ) Why would the blank line above here need removing? Jan
On Mon, 8 Apr 2024, Jan Beulich wrote: > On 05.04.2024 11:14, Nicola Vetrini wrote: > > Remove unneded blank lines between switch clauses. > > NAK for this part again. > > > --- a/xen/arch/x86/irq.c > > +++ b/xen/arch/x86/irq.c > > @@ -2882,7 +2882,7 @@ int allocate_and_map_gsi_pirq(struct domain *d, int index, int *pirq_p) > > int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p, > > int type, struct msi_info *msi) > > { > > - int irq, pirq, ret; > > + int irq = -1, pirq, ret; > > > > ASSERT_PDEV_LIST_IS_READ_LOCKED(d); > > > > @@ -2892,12 +2892,10 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p, > > if ( !msi->table_base ) > > msi->entry_nr = 1; > > irq = index; > > - if ( irq == -1 ) > > - { > > + fallthrough; > > case MAP_PIRQ_TYPE_MULTI_MSI: > > + if( type == MAP_PIRQ_TYPE_MULTI_MSI || irq == -1 ) > > irq = create_irq(NUMA_NO_NODE, true); > > It may seem small, but this extra comparison already is duplication I'd rather > see avoided. At the very least though you want to clarify in the description > whether the compiler manages to eliminate it again. It could just be: if ( irq == -1 ) because in the MAP_PIRQ_TYPE_MULTI_MSI case we know the irq will be -1. No duplication needed.
On 2024-04-09 02:14, Stefano Stabellini wrote: > On Mon, 8 Apr 2024, Jan Beulich wrote: >> On 05.04.2024 11:14, Nicola Vetrini wrote: >> > Remove unneded blank lines between switch clauses. >> >> NAK for this part again. >> >> > --- a/xen/arch/x86/irq.c >> > +++ b/xen/arch/x86/irq.c >> > @@ -2882,7 +2882,7 @@ int allocate_and_map_gsi_pirq(struct domain *d, int index, int *pirq_p) >> > int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p, >> > int type, struct msi_info *msi) >> > { >> > - int irq, pirq, ret; >> > + int irq = -1, pirq, ret; >> > >> > ASSERT_PDEV_LIST_IS_READ_LOCKED(d); >> > >> > @@ -2892,12 +2892,10 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p, >> > if ( !msi->table_base ) >> > msi->entry_nr = 1; >> > irq = index; >> > - if ( irq == -1 ) >> > - { >> > + fallthrough; >> > case MAP_PIRQ_TYPE_MULTI_MSI: >> > + if( type == MAP_PIRQ_TYPE_MULTI_MSI || irq == -1 ) >> > irq = create_irq(NUMA_NO_NODE, true); >> >> It may seem small, but this extra comparison already is duplication >> I'd rather >> see avoided. At the very least though you want to clarify in the >> description >> whether the compiler manages to eliminate it again. > > It could just be: > > if ( irq == -1 ) > > because in the MAP_PIRQ_TYPE_MULTI_MSI case we know the irq will be -1. > No duplication needed. Ok then. Didn't know whether I could avoid the check to get this structuring.
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index 0487f734a5d2..16c023f077da 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -2882,7 +2882,7 @@ int allocate_and_map_gsi_pirq(struct domain *d, int index, int *pirq_p) int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p, int type, struct msi_info *msi) { - int irq, pirq, ret; + int irq = -1, pirq, ret; ASSERT_PDEV_LIST_IS_READ_LOCKED(d); @@ -2892,12 +2892,10 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p, if ( !msi->table_base ) msi->entry_nr = 1; irq = index; - if ( irq == -1 ) - { + fallthrough; case MAP_PIRQ_TYPE_MULTI_MSI: + if( type == MAP_PIRQ_TYPE_MULTI_MSI || irq == -1 ) irq = create_irq(NUMA_NO_NODE, true); - } - if ( irq < nr_irqs_gsi || irq >= nr_irqs ) { dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n", @@ -2905,7 +2903,6 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p, return -EINVAL; } break; - default: dprintk(XENLOG_G_ERR, "dom%d: wrong pirq type %x\n", d->domain_id, type);
Remove unneded blank lines between switch clauses. Refactor the clauses so that a MISRA C Rule 16.2 violation is resolved (A switch label shall only be used when the most closely-enclosing compound statement is the body of a switch statement). Note that the switch clause ending with the pseudo keyword "fallthrough" is an allowed exception to Rule 16.3. No functional change. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- The initialization of irq is due to gcc thinking that irq may be used uninitizalied in the test after MAP_PIRQ_TYPE_MULTI_MSI --- xen/arch/x86/irq.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)