| Message ID | a62e88a9c29cf7866c251968b5a5b6865aff4a2a.1690217195.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IRQ: address violations of MISRA C:2012 Rules 8.2 and 8.3 | expand |
Hi, On 24/07/2023 18:50, Federico Serafini wrote: > Give a name to unnamed parameters thus addressing violations of > MISRA C:2012 Rule 8.2 ("Function types shall be in prototype form with > named parameters"). > Keep consistency between parameter names and types used in function > declarations and the ones used in the corresponding function > definitions, thus addressing violations of MISRA C:2012 Rule 8.3 > ("All declarations of an object or function shall use the same names > and type qualifiers"). > > No functional changes. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> I think it would be better if this is folded in patch #1 where you modify the prototype. Cheers,
On Mon, 24 Jul 2023, Federico Serafini wrote: > Give a name to unnamed parameters thus addressing violations of > MISRA C:2012 Rule 8.2 ("Function types shall be in prototype form with > named parameters"). > Keep consistency between parameter names and types used in function > declarations and the ones used in the corresponding function > definitions, thus addressing violations of MISRA C:2012 Rule 8.3 > ("All declarations of an object or function shall use the same names > and type qualifiers"). > > No functional changes. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > --- > xen/arch/arm/irq.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 16e56f8945..335e06a2a7 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -58,7 +58,7 @@ hw_irq_controller no_irq_type = { > static irq_desc_t irq_desc[NR_IRQS]; > static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc); > > -irq_desc_t *__irq_to_desc(int irq) > +struct irq_desc *__irq_to_desc(int irq) > { > if ( irq < NR_LOCAL_IRQS ) > return &this_cpu(local_irq_desc)[irq]; > @@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask) > } > > int request_irq(unsigned int irq, unsigned int irqflags, > - void (*handler)(int, void *, struct cpu_user_regs *), > + void (*handler)(int irq, void *dev_id, > + struct cpu_user_regs *regs), We have an inconsistency where the handler functions on x86 typically call it void *data, while on arm they typically use void *dev_id (see xen/arch/x86/irq.c:request_irq and xen/arch/x86/hpet.c:hpet_interrupt_handler). I think we should be consistent. Or, if this is not a MISRA requirement because this is just a function pointer rather than a proper function, then I would leave it alone. > const char *devname, void *dev_id) > { > struct irqaction *action; > @@ -617,7 +618,7 @@ void pirq_guest_unbind(struct domain *d, struct pirq *pirq) > BUG(); > } > > -void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask) > +void pirq_set_affinity(struct domain *d, int irq, const cpumask_t *mask) I think we should leave it as is because there is also the x86 implementation of pirq_set_affinity that uses int pirq as parameter. It is not a good idea to introduce inconsistencies between the x86 and the ARM versions of the same function.
On 24.07.2023 19:50, Federico Serafini wrote: > @@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask) > } > > int request_irq(unsigned int irq, unsigned int irqflags, > - void (*handler)(int, void *, struct cpu_user_regs *), > + void (*handler)(int irq, void *dev_id, > + struct cpu_user_regs *regs), > const char *devname, void *dev_id) > { Before we accept patches, don't we need to first settle on whether to apply the rule(s) also to function type declarations (and not just ordinary prototypes)? Jan
On Tue, 25 Jul 2023, Jan Beulich wrote: > On 24.07.2023 19:50, Federico Serafini wrote: > > @@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask) > > } > > > > int request_irq(unsigned int irq, unsigned int irqflags, > > - void (*handler)(int, void *, struct cpu_user_regs *), > > + void (*handler)(int irq, void *dev_id, > > + struct cpu_user_regs *regs), > > const char *devname, void *dev_id) > > { > > Before we accept patches, don't we need to first settle on whether to > apply the rule(s) also to function type declarations (and not just > ordinary prototypes)? Yes, in retrospect we should have found agreement on this issue this morning but I forgot to bring it up :-( Ooops. (I think the agreement was to change the function type declarations too, that's why docs/misra/rules.rst doesn't have a note about this, but I don't want to make assumptions as I am not certain.)
Hello Stefano, On 25/07/23 00:55, Stefano Stabellini wrote: >> int request_irq(unsigned int irq, unsigned int irqflags, >> - void (*handler)(int, void *, struct cpu_user_regs *), >> + void (*handler)(int irq, void *dev_id, >> + struct cpu_user_regs *regs), > > We have an inconsistency where the handler functions on x86 typically > call it void *data, while on arm they typically use void *dev_id > (see xen/arch/x86/irq.c:request_irq and > xen/arch/x86/hpet.c:hpet_interrupt_handler). I think we should be > consistent. Or, if this is not a MISRA requirement because this is just > a function pointer rather than a proper function, then I would leave it > alone. This is an inconsistency but it is not a violation of the rule 8.3. Regards
Hello Jan, Stefano, On 25/07/23 21:32, Stefano Stabellini wrote: > On Tue, 25 Jul 2023, Jan Beulich wrote: >> On 24.07.2023 19:50, Federico Serafini wrote: >>> @@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask) >>> } >>> >>> int request_irq(unsigned int irq, unsigned int irqflags, >>> - void (*handler)(int, void *, struct cpu_user_regs *), >>> + void (*handler)(int irq, void *dev_id, >>> + struct cpu_user_regs *regs), >>> const char *devname, void *dev_id) >>> { >> >> Before we accept patches, don't we need to first settle on whether to >> apply the rule(s) also to function type declarations (and not just >> ordinary prototypes)? > > Yes, in retrospect we should have found agreement on this issue this > morning but I forgot to bring it up :-( Ooops. > > (I think the agreement was to change the function type declarations too, > that's why docs/misra/rules.rst doesn't have a note about this, but I > don't want to make assumptions as I am not certain.) I have ready a patch for violations of rules 8.2 and 8.3 in xen/include/xen/iommu.h. I am talking about this, in this IRQ thread, because I think the following two options also apply for an eventual v2 patch for the IRQ module, until a decision about rule 8.2 and function pointers is taken: 1) Split patches and submit only the changes *not* involving function pointers. 2) In the meantime that you make a decision, I submit patches thus addressing the existing violations. I personally prefer the second one, but please let me know what you think. Regards
On 27.07.2023 13:02, Federico Serafini wrote: > Hello Jan, Stefano, > > On 25/07/23 21:32, Stefano Stabellini wrote: >> On Tue, 25 Jul 2023, Jan Beulich wrote: >>> On 24.07.2023 19:50, Federico Serafini wrote: >>>> @@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask) >>>> } >>>> >>>> int request_irq(unsigned int irq, unsigned int irqflags, >>>> - void (*handler)(int, void *, struct cpu_user_regs *), >>>> + void (*handler)(int irq, void *dev_id, >>>> + struct cpu_user_regs *regs), >>>> const char *devname, void *dev_id) >>>> { >>> >>> Before we accept patches, don't we need to first settle on whether to >>> apply the rule(s) also to function type declarations (and not just >>> ordinary prototypes)? >> >> Yes, in retrospect we should have found agreement on this issue this >> morning but I forgot to bring it up :-( Ooops. >> >> (I think the agreement was to change the function type declarations too, >> that's why docs/misra/rules.rst doesn't have a note about this, but I >> don't want to make assumptions as I am not certain.) > > I have ready a patch for violations of rules 8.2 and 8.3 in > xen/include/xen/iommu.h. > I am talking about this, in this IRQ thread, because I think the > following two options also apply for an eventual v2 patch for the IRQ > module, until a decision about rule 8.2 and function pointers is taken: > > 1) Split patches and submit only the changes *not* involving function > pointers. > 2) In the meantime that you make a decision, > I submit patches thus addressing the existing violations. > > I personally prefer the second one, but please let me know what you > think. It's not entirely clear to me what 2 means, as I wouldn't expect you intend to deal with "violations" which we may decide aren't any in out world. Jan
On 27/07/23 13:27, Jan Beulich wrote: > On 27.07.2023 13:02, Federico Serafini wrote: >> >> I have ready a patch for violations of rules 8.2 and 8.3 in >> xen/include/xen/iommu.h. >> I am talking about this, in this IRQ thread, because I think the >> following two options also apply for an eventual v2 patch for the IRQ >> module, until a decision about rule 8.2 and function pointers is taken: >> >> 1) Split patches and submit only the changes *not* involving function >> pointers. >> 2) In the meantime that you make a decision, >> I submit patches thus addressing the existing violations. >> >> I personally prefer the second one, but please let me know what you >> think. > > It's not entirely clear to me what 2 means, as I wouldn't expect you > intend to deal with "violations" which we may decide aren't any in > out world. > > Jan In point 2) I would like to act as if the rule 8.2 had been approved without any deviation, I think this will lead to a smaller number of patches and a smaller amount of text attached to each modified function. If you are concerned about inconsistency between the resulting commit messages and your future decision then we can go for option 1).
On Thu, 27 Jul 2023, Federico Serafini wrote: > On 27/07/23 13:27, Jan Beulich wrote: > > On 27.07.2023 13:02, Federico Serafini wrote: > > > > > > I have ready a patch for violations of rules 8.2 and 8.3 in > > > xen/include/xen/iommu.h. > > > I am talking about this, in this IRQ thread, because I think the > > > following two options also apply for an eventual v2 patch for the IRQ > > > module, until a decision about rule 8.2 and function pointers is taken: > > > > > > 1) Split patches and submit only the changes *not* involving function > > > pointers. > > > 2) In the meantime that you make a decision, > > > I submit patches thus addressing the existing violations. > > > > > > I personally prefer the second one, but please let me know what you > > > think. > > > > It's not entirely clear to me what 2 means, as I wouldn't expect you > > intend to deal with "violations" which we may decide aren't any in > > out world. > > > > Jan > > In point 2) I would like to act as if the rule 8.2 had been approved without > any deviation, I think this will lead to a smaller number of patches and a > smaller amount of text attached to each modified function. > If you are concerned about inconsistency between the resulting commit > messages and your future decision then we can go for option 1). Basically I think we need to go with 1), which is also what Jan wrote. Sorry about that, I know it is not great, we should have settled this already.
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 16e56f8945..335e06a2a7 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -58,7 +58,7 @@ hw_irq_controller no_irq_type = { static irq_desc_t irq_desc[NR_IRQS]; static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc); -irq_desc_t *__irq_to_desc(int irq) +struct irq_desc *__irq_to_desc(int irq) { if ( irq < NR_LOCAL_IRQS ) return &this_cpu(local_irq_desc)[irq]; @@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask) } int request_irq(unsigned int irq, unsigned int irqflags, - void (*handler)(int, void *, struct cpu_user_regs *), + void (*handler)(int irq, void *dev_id, + struct cpu_user_regs *regs), const char *devname, void *dev_id) { struct irqaction *action; @@ -617,7 +618,7 @@ void pirq_guest_unbind(struct domain *d, struct pirq *pirq) BUG(); } -void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask) +void pirq_set_affinity(struct domain *d, int irq, const cpumask_t *mask) { BUG(); }
Give a name to unnamed parameters thus addressing violations of MISRA C:2012 Rule 8.2 ("Function types shall be in prototype form with named parameters"). Keep consistency between parameter names and types used in function declarations and the ones used in the corresponding function definitions, thus addressing violations of MISRA C:2012 Rule 8.3 ("All declarations of an object or function shall use the same names and type qualifiers"). No functional changes. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- xen/arch/arm/irq.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)