| Message ID | e139df541183df7c92b3bd73841cf1fb2851e054.1686575613.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN] xen: fixed violations of MISRA C:2012 Rule 3.1 | expand |
On 12.06.2023 18:10, nicola.vetrini@bugseng.com wrote: > From: Nicola Vetrini <nicola.vetrini@bugseng.com> > > The xen sources contain several violations of Rule 3.1 from MISRA C:2012, > whose headline states: > "The character sequences '/*' and '//' shall not be used within a comment". > > Most of the violations are due to the presence of links to webpages within > C-style comment blocks, such as: > > xen/arch/arm/include/asm/smccc.h:37.1-41.3 > /* > * This file provides common defines for ARM SMC Calling Convention as > * specified in > * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html > */ > > In this case, we propose to deviate all of these occurrences with a > project deviation to be captured by a tool configuration. Here "propose" is appropriate in the description, as this is something the patch does not do. Further down, however, you mean to describe what the patch does, not what an eventual patch might do. Somewhat similarly you don't want to use past tense in the title, as that wants to describe what is being done, rather than describing a patch that has already landed. > There are, however, a few other violations that do not fall under this > category, namely: > > 1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to > avoid the usage of a nested comment; > 2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the > commented-out if statement with a "#if 0 .. #endif; > 3. in file "xen/include/xen/atomic.h" and > "xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style > comment containing the nested comment into two doxygen comments, clearly > identifying the second as a code sample. This can then be captured with a > project deviation by a tool configuration. Finally I find the use of "we" somewhat suspicious. Who besides you is it that you're talking about? Also please don't forget to Cc relevant maintainers. > --- a/xen/drivers/passthrough/arm/smmu-v3.c > +++ b/xen/drivers/passthrough/arm/smmu-v3.c > @@ -1045,15 +1045,18 @@ static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, > /* > * Ensure that we've completed prior invalidation of the main TLBs > * before we read 'nr_ats_masters' in case of a concurrent call to > - * arm_smmu_enable_ats(): > + * arm_smmu_enable_ats(). > + */ > + /** > + * Code sample: Ensures that we always see the incremented > + * 'nr_ats_masters' count if ATS was enabled at the PCI device before > + * completion of the TLBI. > * > * // unmap() // arm_smmu_enable_ats() > * TLBI+SYNC atomic_inc(&nr_ats_masters); > * smp_mb(); [...] > * atomic_read(&nr_ats_masters); pci_enable_ats() // writel() > * > - * Ensures that we always see the incremented 'nr_ats_masters' count if > - * ATS was enabled at the PCI device before completion of the TLBI. > */ > smp_mb(); > if (!atomic_read(&smmu_domain->nr_ats_masters)) I don't really know this code, but the use of inner comments looks unnecessary to me here. The same could e.g. be achieved by * unmap(): arm_smmu_enable_ats(): * TLBI+SYNC atomic_inc(&nr_ats_masters); * smp_mb(); [...] * atomic_read(&nr_ats_masters); pci_enable_ats(); (i.e. writel()) and it would avoid the somewhat odd splitting of adjacent comments (which then is at risk of someone later coming forward with a patch re-combining them). > --- a/xen/include/xen/atomic.h > +++ b/xen/include/xen/atomic.h > @@ -71,7 +71,10 @@ static inline void _atomic_set(atomic_t *v, int i); > * Returns the initial value in @v, hence succeeds when the return value > * matches that of @old. > * > - * Sample (tries atomic increment of v until the operation succeeds): > + */ > +/** > + * > + * Code sample: Tries atomic increment of v until the operation succeeds. > * > * while(1) > * { Somewhat similarly here: I don't think the inner comment provides much value, and could hence be dropped instead of splitting the comment. Jan
On 13.06.2023 09:42, Nicola Vetrini wrote: > The xen sources contain several violations of Rule 3.1 from MISRA C:2012, > whose headline states: > "The character sequences '/*' and '//' shall not be used within a comment". > > Most of the violations are due to the presence of links to webpages within > C-style comment blocks, such as: > > xen/arch/arm/include/asm/smccc.h:37.1-41.3 > /* > * This file provides common defines for ARM SMC Calling Convention as > * specified in > * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html > */ > > In this case, we propose to deviate all of these occurrences with a > project deviation to be captured by a tool configuration. > > There are, however, a few other violations that do not fall under this > category, namely: > > 1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to > avoid the usage of a nested comment; > 2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the > commented-out if statement with a "#if 0 .. #endif; > 3. in file "xen/include/xen/atomic.h" and > "xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style > comment containing the nested comment into two doxygen comments, clearly > identifying the second as a code sample. This can then be captured with a > project deviation by a tool configuration. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > Changes: > - Resending the patch with the right maintainers in CC. But without otherwise addressing comments already given, afaics. One more remark: > --- a/xen/common/xmalloc_tlsf.c > +++ b/xen/common/xmalloc_tlsf.c > @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) > *fl = flsl(*r) - 1; > *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; > *fl -= FLI_OFFSET; > - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! > - *fl = *sl = 0; > - */ > +#if 0 > + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! > + fl = *sl = 0; You want to get indentation right here, and you don't want to lose the indirection on fl. > +#endif > *r &= ~t; > } > } If you split this to 4 patches, leaving the URL proposal in just the cover letter, then I think this one change (with the adjustments) could go in right away. Similarly I expect the arm64/flushtlb.h change could be ack-ed right away by an Arm maintainer. Jan
Hi, On 13/06/2023 09:27, Jan Beulich wrote: > On 13.06.2023 09:42, Nicola Vetrini wrote: >> The xen sources contain several violations of Rule 3.1 from MISRA C:2012, >> whose headline states: >> "The character sequences '/*' and '//' shall not be used within a comment". >> >> Most of the violations are due to the presence of links to webpages within >> C-style comment blocks, such as: >> >> xen/arch/arm/include/asm/smccc.h:37.1-41.3 >> /* >> * This file provides common defines for ARM SMC Calling Convention as >> * specified in >> * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html >> */ >> >> In this case, we propose to deviate all of these occurrences with a >> project deviation to be captured by a tool configuration. >> >> There are, however, a few other violations that do not fall under this >> category, namely: >> >> 1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to >> avoid the usage of a nested comment; >> 2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the >> commented-out if statement with a "#if 0 .. #endif; >> 3. in file "xen/include/xen/atomic.h" and >> "xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style >> comment containing the nested comment into two doxygen comments, clearly >> identifying the second as a code sample. This can then be captured with a >> project deviation by a tool configuration. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> Changes: >> - Resending the patch with the right maintainers in CC. > > But without otherwise addressing comments already given, afaics. One more > remark: > >> --- a/xen/common/xmalloc_tlsf.c >> +++ b/xen/common/xmalloc_tlsf.c >> @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) >> *fl = flsl(*r) - 1; >> *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; >> *fl -= FLI_OFFSET; >> - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >> - *fl = *sl = 0; >> - */ >> +#if 0 >> + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >> + fl = *sl = 0; > > You want to get indentation right here, and you don't want to lose > the indirection on fl. > >> +#endif >> *r &= ~t; >> } >> } > > If you split this to 4 patches, leaving the URL proposal in just > the cover letter, then I think this one change (with the adjustments) > could go in right away. Similarly I expect the arm64/flushtlb.h > change could be ack-ed right away by an Arm maintainer. I actually dislike the proposal. In this case, the code is meant to look like assembly code. I would replace the // with ;. Also, I would like to keep the comment style in sync in arm32/flushtlb.h. So can this be updated as well? Cheers,
On 13/06/23 10:27, Jan Beulich wrote: > On 13.06.2023 09:42, Nicola Vetrini wrote: >> --- a/xen/common/xmalloc_tlsf.c >> +++ b/xen/common/xmalloc_tlsf.c >> @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) >> *fl = flsl(*r) - 1; >> *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; >> *fl -= FLI_OFFSET; >> - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >> - *fl = *sl = 0; >> - */ >> +#if 0 >> + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >> + fl = *sl = 0; > You want to get indentation right here, and you don't want to lose > the indirection on fl. Understood. >> +#endif >> *r &= ~t; >> } >> } > If you split this to 4 patches, leaving the URL proposal in just > the cover letter, then I think this one change (with the adjustments) > could go in right away. Similarly I expect the arm64/flushtlb.h > change could be ack-ed right away by an Arm maintainer. Ok. I do not understand what you mean by "leaving the URL proposal in just the cover letter". Which URL? I'm sorry, I didn't notice your previous reply. I'm going to address your observations now: > Here "propose" is appropriate in the description, as this is something > the patch does not do. Further down, however, you mean to describe > what the patch does, not what an eventual patch might do. > To my knowledge, there is not a standard format to define a project deviation for a certain MISRA rule in Xen right now (this can also be discussed in a separate thread). To clarify, I meant to describe why I wasn't addressing these violations in the patch (they are the vast majority, but they do not have any implication w.r.t. functional safety and can therefore be safely deviated with an appropriate written justification). > Somewhat similarly you don't want to use past tense in the title, as > that wants to describe what is being done, rather than describing a > patch that has already landed. Understood. > and it would avoid the somewhat odd splitting of adjacent comments > (which then is at risk of someone later coming forward with a patch > re-combining them). > > >/--- a/xen/include/xen/atomic.h/ > >/+++ b/xen/include/xen/atomic.h/ > >/@@ -71,7 +71,10 @@ static inline void _atomic_set(atomic_t *v, int i);/ > >/* Returns the initial value in @v, hence succeeds when the return value/ > >/* matches that of @old./ > >/*/ > >/- * Sample (tries atomic increment of v until the operation succeeds):/ > >/+ *// > >/+/**/ > >/+ */ > >/+ * Code sample: Tries atomic increment of v until the operation > succeeds./ > >/*/ > >/* while(1)/ > >/* {/ > > Somewhat similarly here: I don't think the inner comment provides > much value, and could hence be dropped instead of splitting the comment. The rationale behind these modifications was to separate clearly comments and code samples, so that the latter can be easily deviated by tool configurations. I'm ok with the suggestion of removing the nested comments, and otherwise leave the code as is, but i'll probably do this by splitting the patch as you suggested above. Regards, Nicola
First of all - please don't drop Cc-s when replying, unless you have a specific reason to. On 13.06.2023 11:56, nicola wrote: > On 13/06/23 10:27, Jan Beulich wrote: >> If you split this to 4 patches, leaving the URL proposal in just >> the cover letter, then I think this one change (with the adjustments) >> could go in right away. Similarly I expect the arm64/flushtlb.h >> change could be ack-ed right away by an Arm maintainer. > Ok. I do not understand what you mean by "leaving the URL proposal in > just the cover letter". Which URL? In your description you had a proposal to deviate the // occurring in URLs. The latest when splitting the patch, this doesn't belong into any of the patches anymore, but just in the cover letter. >> Here "propose" is appropriate in the description, as this is something >> the patch does not do. Further down, however, you mean to describe >> what the patch does, not what an eventual patch might do. >> > To my knowledge, there is not a standard format to define a project > deviation for a certain MISRA rule in Xen right now (this can also be > discussed in a separate thread). To clarify, I meant to describe why I > wasn't addressing these violations in the patch (they are the vast > majority, but they do not have any implication w.r.t. functional safety > and can therefore be safely deviated with an appropriate written > justification). And as said, for what you're not doing in the patch, using "propose" is quite fine (as per above, whether that actually belongs in the description is another question). I view the word as inapplicable though when you describe what you're actually doing in a patch. But I'm not a native speaker, so I may be wrong here. Jan
On 13/06/23 11:44, Julien Grall wrote: > Hi, > > On 13/06/2023 09:27, Jan Beulich wrote: >> On 13.06.2023 09:42, Nicola Vetrini wrote: >>> The xen sources contain several violations of Rule 3.1 from MISRA >>> C:2012, >>> whose headline states: >>> "The character sequences '/*' and '//' shall not be used within a >>> comment". >>> >>> Most of the violations are due to the presence of links to webpages >>> within >>> C-style comment blocks, such as: >>> >>> xen/arch/arm/include/asm/smccc.h:37.1-41.3 >>> /* >>> * This file provides common defines for ARM SMC Calling Convention as >>> * specified in >>> * >>> http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html >>> */ >>> >>> In this case, we propose to deviate all of these occurrences with a >>> project deviation to be captured by a tool configuration. >>> >>> There are, however, a few other violations that do not fall under this >>> category, namely: >>> >>> 1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to >>> avoid the usage of a nested comment; >>> 2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the >>> commented-out if statement with a "#if 0 .. #endif; >>> 3. in file "xen/include/xen/atomic.h" and >>> "xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style >>> comment containing the nested comment into two doxygen comments, >>> clearly >>> identifying the second as a code sample. This can then be captured >>> with a >>> project deviation by a tool configuration. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> --- >>> Changes: >>> - Resending the patch with the right maintainers in CC. >> >> But without otherwise addressing comments already given, afaics. One >> more >> remark: >> >>> --- a/xen/common/xmalloc_tlsf.c >>> +++ b/xen/common/xmalloc_tlsf.c >>> @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long >>> *r, int *fl, int *sl) >>> *fl = flsl(*r) - 1; >>> *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; >>> *fl -= FLI_OFFSET; >>> - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >>> - *fl = *sl = 0; >>> - */ >>> +#if 0 >>> + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >>> + fl = *sl = 0; >> >> You want to get indentation right here, and you don't want to lose >> the indirection on fl. >> >>> +#endif >>> *r &= ~t; >>> } >>> } >> >> If you split this to 4 patches, leaving the URL proposal in just >> the cover letter, then I think this one change (with the adjustments) >> could go in right away. Similarly I expect the arm64/flushtlb.h >> change could be ack-ed right away by an Arm maintainer. > > I actually dislike the proposal. In this case, the code is meant to > look like assembly code. I would replace the // with ;. Also, I would > like to keep the comment style in sync in arm32/flushtlb.h. So can > this be updated as well? > > Cheers, > Hi, Julien. I'm not authorized to send patches about files in the arm32 tree, but surely the patch can be easily replicated in any place where it makes sense for consistency. Regards, Nicola
On 13/06/2023 8:42 am, Nicola Vetrini wrote: > diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c > index 75bdf18c4e..ea6ec47a59 100644 > --- a/xen/common/xmalloc_tlsf.c > +++ b/xen/common/xmalloc_tlsf.c > @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) > *fl = flsl(*r) - 1; > *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; > *fl -= FLI_OFFSET; > - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! > - *fl = *sl = 0; > - */ > +#if 0 > + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! > + fl = *sl = 0; > +#endif > *r &= ~t; > } > } This logic has been commented out right from it's introduction in c/s 9736b76d829b2d in 2008, and never touched since. I think it can safely be deleted, and not placed inside an #if 0. ~Andrew
On 14.06.2023 16:28, Andrew Cooper wrote: > On 13/06/2023 8:42 am, Nicola Vetrini wrote: >> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c >> index 75bdf18c4e..ea6ec47a59 100644 >> --- a/xen/common/xmalloc_tlsf.c >> +++ b/xen/common/xmalloc_tlsf.c >> @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) >> *fl = flsl(*r) - 1; >> *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; >> *fl -= FLI_OFFSET; >> - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >> - *fl = *sl = 0; >> - */ >> +#if 0 >> + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >> + fl = *sl = 0; >> +#endif >> *r &= ~t; >> } >> } > > This logic has been commented out right from it's introduction in c/s > 9736b76d829b2d in 2008, and never touched since. > > I think it can safely be deleted, and not placed inside an #if 0. I have to admit that I wouldn't be happy with deleting without any replacement. Instead of the commented out code, how about instead having ASSERT(*fl >= 0)? (What isn't clear to me is whether the commented out code is actually meant to replace the earlier line, rather than (optionally) be there in addition - at least it very much looks like so. With such an uncertainty I'd be further inclined to not remove what's there.) Jan
diff --git a/xen/arch/arm/include/asm/arm64/flushtlb.h b/xen/arch/arm/include/asm/arm64/flushtlb.h index 3a9092b814..90ac3f9809 100644 --- a/xen/arch/arm/include/asm/arm64/flushtlb.h +++ b/xen/arch/arm/include/asm/arm64/flushtlb.h @@ -4,10 +4,10 @@ /* * Every invalidation operation use the following patterns: * - * DSB ISHST // Ensure prior page-tables updates have completed - * TLBI... // Invalidate the TLB - * DSB ISH // Ensure the TLB invalidation has completed - * ISB // See explanation below + * DSB ISHST Ensure prior page-tables updates have completed + * TLBI... Invalidate the TLB + * DSB ISH Ensure the TLB invalidation has completed + * ISB See explanation below * * ARM64_WORKAROUND_REPEAT_TLBI: * Modification of the translation table for a virtual address might lead to diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index 75bdf18c4e..ea6ec47a59 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) *fl = flsl(*r) - 1; *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; *fl -= FLI_OFFSET; - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! - *fl = *sl = 0; - */ +#if 0 + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! + fl = *sl = 0; +#endif *r &= ~t; } } diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c index 720aa69ff2..b1c536e7d9 100644 --- a/xen/drivers/passthrough/arm/smmu-v3.c +++ b/xen/drivers/passthrough/arm/smmu-v3.c @@ -1045,15 +1045,18 @@ static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, /* * Ensure that we've completed prior invalidation of the main TLBs * before we read 'nr_ats_masters' in case of a concurrent call to - * arm_smmu_enable_ats(): + * arm_smmu_enable_ats(). + */ + /** + * Code sample: Ensures that we always see the incremented + * 'nr_ats_masters' count if ATS was enabled at the PCI device before + * completion of the TLBI. * * // unmap() // arm_smmu_enable_ats() * TLBI+SYNC atomic_inc(&nr_ats_masters); * smp_mb(); [...] * atomic_read(&nr_ats_masters); pci_enable_ats() // writel() * - * Ensures that we always see the incremented 'nr_ats_masters' count if - * ATS was enabled at the PCI device before completion of the TLBI. */ smp_mb(); if (!atomic_read(&smmu_domain->nr_ats_masters)) diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h index 529213ebbb..829646dda0 100644 --- a/xen/include/xen/atomic.h +++ b/xen/include/xen/atomic.h @@ -71,7 +71,10 @@ static inline void _atomic_set(atomic_t *v, int i); * Returns the initial value in @v, hence succeeds when the return value * matches that of @old. * - * Sample (tries atomic increment of v until the operation succeeds): + */ +/** + * + * Code sample: Tries atomic increment of v until the operation succeeds. * * while(1) * {