All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule violations
@ 2022-07-27 15:32 Xenia Ragiadakou
  2022-07-27 15:32 ` [PATCH 1/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 20.7 violation Xenia Ragiadakou
  2022-07-27 15:32 ` [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation Xenia Ragiadakou
  0 siblings, 2 replies; 13+ messages in thread
From: Xenia Ragiadakou @ 2022-07-27 15:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk

This patch series fixes violations of rules 20.7 and 2.5 found in
xen/arch/arm/include/asm/atomic.h.
Rule 2.5 (a project should not contain unused macro declarations) is advisory.

Xenia Ragiadakou (2):
  xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 20.7 violation
  xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation

 xen/arch/arm/include/asm/atomic.h | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 20.7 violation
  2022-07-27 15:32 [PATCH 0/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule violations Xenia Ragiadakou
@ 2022-07-27 15:32 ` Xenia Ragiadakou
  2022-07-27 15:36   ` Jan Beulich
  2022-07-27 15:32 ` [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation Xenia Ragiadakou
  1 sibling, 1 reply; 13+ messages in thread
From: Xenia Ragiadakou @ 2022-07-27 15:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk

The macro parameter 'p' is used as an expression and needs to be enclosed in
parentheses.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/arm/include/asm/atomic.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h
index ac2798d095..f5ef744b4b 100644
--- a/xen/arch/arm/include/asm/atomic.h
+++ b/xen/arch/arm/include/asm/atomic.h
@@ -123,15 +123,15 @@ static always_inline void write_atomic_size(volatile void *p,
 }
 
 #define read_atomic(p) ({                                               \
-    union { typeof(*p) val; char c[0]; } x_;                            \
-    read_atomic_size(p, x_.c, sizeof(*p));                              \
+    union { typeof(*(p)) val; char c[0]; } x_;                          \
+    read_atomic_size((p), x_.c, sizeof(*(p)));                          \
     x_.val;                                                             \
 })
 
 #define write_atomic(p, x)                                              \
     do {                                                                \
-        typeof(*p) x_ = (x);                                            \
-        write_atomic_size(p, &x_, sizeof(*p));                          \
+        typeof(*(p)) x_ = (x);                                          \
+        write_atomic_size((p), &x_, sizeof(*(p)));                      \
     } while ( false )
 
 #define add_sized(p, x) ({                                              \
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
  2022-07-27 15:32 [PATCH 0/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule violations Xenia Ragiadakou
  2022-07-27 15:32 ` [PATCH 1/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 20.7 violation Xenia Ragiadakou
@ 2022-07-27 15:32 ` Xenia Ragiadakou
  2022-07-27 15:46   ` Julien Grall
  1 sibling, 1 reply; 13+ messages in thread
From: Xenia Ragiadakou @ 2022-07-27 15:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk

Remove unused macro atomic_xchg().

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/arm/include/asm/atomic.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h
index f5ef744b4b..a2dc125291 100644
--- a/xen/arch/arm/include/asm/atomic.h
+++ b/xen/arch/arm/include/asm/atomic.h
@@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
     return __atomic_add_unless(v, a, u);
 }
 
-#define atomic_xchg(v, new) (xchg(&((v)->counter), new))
-
 #endif /* __ARCH_ARM_ATOMIC__ */
 /*
  * Local variables:
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 20.7 violation
  2022-07-27 15:32 ` [PATCH 1/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 20.7 violation Xenia Ragiadakou
@ 2022-07-27 15:36   ` Jan Beulich
  2022-07-27 16:18     ` Xenia Ragiadakou
  0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2022-07-27 15:36 UTC (permalink / raw)
  To: Xenia Ragiadakou
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, xen-devel

On 27.07.2022 17:32, Xenia Ragiadakou wrote:
> The macro parameter 'p' is used as an expression and needs to be enclosed in
> parentheses.

Yes, but ...

> --- a/xen/arch/arm/include/asm/atomic.h
> +++ b/xen/arch/arm/include/asm/atomic.h
> @@ -123,15 +123,15 @@ static always_inline void write_atomic_size(volatile void *p,
>  }
>  
>  #define read_atomic(p) ({                                               \
> -    union { typeof(*p) val; char c[0]; } x_;                            \
> -    read_atomic_size(p, x_.c, sizeof(*p));                              \
> +    union { typeof(*(p)) val; char c[0]; } x_;                          \
> +    read_atomic_size((p), x_.c, sizeof(*(p)));                          \

... not in the first argument's case - that's not an expression.
Too few parentheses are a risk, but too many are as well, as they
negatively affect readability.

Jan


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
  2022-07-27 15:32 ` [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation Xenia Ragiadakou
@ 2022-07-27 15:46   ` Julien Grall
  2022-07-27 16:23     ` Xenia Ragiadakou
  2022-07-28  7:57     ` Bertrand Marquis
  0 siblings, 2 replies; 13+ messages in thread
From: Julien Grall @ 2022-07-27 15:46 UTC (permalink / raw)
  To: Xenia Ragiadakou, xen-devel
  Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk

Hi Xenia,

On 27/07/2022 16:32, Xenia Ragiadakou wrote:
> Remove unused macro atomic_xchg().
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> ---
>   xen/arch/arm/include/asm/atomic.h | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h
> index f5ef744b4b..a2dc125291 100644
> --- a/xen/arch/arm/include/asm/atomic.h
> +++ b/xen/arch/arm/include/asm/atomic.h
> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
>       return __atomic_add_unless(v, a, u);
>   }
>   
> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new))
> -

While I agree this is unused today, the wrapper is quite trivial and 
part of the generic API (x86 also provides one). So I am not in favor of 
removing it just to please MISRA.

That said, if Bertrand and Stefano agrees with removing it then you 
should also remove the x86 version to avoid inconsistency.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 20.7 violation
  2022-07-27 15:36   ` Jan Beulich
@ 2022-07-27 16:18     ` Xenia Ragiadakou
  0 siblings, 0 replies; 13+ messages in thread
From: Xenia Ragiadakou @ 2022-07-27 16:18 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, xen-devel

Hi Jan,

On 7/27/22 18:36, Jan Beulich wrote:
> On 27.07.2022 17:32, Xenia Ragiadakou wrote:
>> The macro parameter 'p' is used as an expression and needs to be enclosed in
>> parentheses.
> 
> Yes, but ...
> 
>> --- a/xen/arch/arm/include/asm/atomic.h
>> +++ b/xen/arch/arm/include/asm/atomic.h
>> @@ -123,15 +123,15 @@ static always_inline void write_atomic_size(volatile void *p,
>>   }
>>   
>>   #define read_atomic(p) ({                                               \
>> -    union { typeof(*p) val; char c[0]; } x_;                            \
>> -    read_atomic_size(p, x_.c, sizeof(*p));                              \
>> +    union { typeof(*(p)) val; char c[0]; } x_;                          \
>> +    read_atomic_size((p), x_.c, sizeof(*(p)));                          \
> 
> ... not in the first argument's case - that's not an expression.
> Too few parentheses are a risk, but too many are as well, as they
> negatively affect readability.
> 

Yes you are right. Here
write_atomic_size((p), &x_, sizeof(*(p)));
as well.

I will fix and resend.

-- 
Xenia


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
  2022-07-27 15:46   ` Julien Grall
@ 2022-07-27 16:23     ` Xenia Ragiadakou
  2022-07-28  7:57     ` Bertrand Marquis
  1 sibling, 0 replies; 13+ messages in thread
From: Xenia Ragiadakou @ 2022-07-27 16:23 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk

Hi Julien,

On 7/27/22 18:46, Julien Grall wrote:
> Hi Xenia,
> 
> On 27/07/2022 16:32, Xenia Ragiadakou wrote:
>> Remove unused macro atomic_xchg().
>>
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>> ---
>>   xen/arch/arm/include/asm/atomic.h | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/atomic.h 
>> b/xen/arch/arm/include/asm/atomic.h
>> index f5ef744b4b..a2dc125291 100644
>> --- a/xen/arch/arm/include/asm/atomic.h
>> +++ b/xen/arch/arm/include/asm/atomic.h
>> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, 
>> int a, int u)
>>       return __atomic_add_unless(v, a, u);
>>   }
>> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>> -
> 
> While I agree this is unused today, the wrapper is quite trivial and 
> part of the generic API (x86 also provides one). So I am not in favor of 
> removing it just to please MISRA.

That's fine, the rule 2.5 is advisory. I sent the patch because I 
noticed that the macro was unused, just in case ...

> 
> That said, if Bertrand and Stefano agrees with removing it then you 
> should also remove the x86 version to avoid inconsistency.
> 
> Cheers,
> 

-- 
Xenia


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
  2022-07-27 15:46   ` Julien Grall
  2022-07-27 16:23     ` Xenia Ragiadakou
@ 2022-07-28  7:57     ` Bertrand Marquis
  2022-07-28  9:35       ` Julien Grall
  1 sibling, 1 reply; 13+ messages in thread
From: Bertrand Marquis @ 2022-07-28  7:57 UTC (permalink / raw)
  To: Julien Grall
  Cc: Xenia Ragiadakou, xen-devel, Stefano Stabellini, Volodymyr Babchuk

Hi Julien,

> On 27 Jul 2022, at 16:46, Julien Grall <julien@xen.org> wrote:
> 
> Hi Xenia,
> 
> On 27/07/2022 16:32, Xenia Ragiadakou wrote:
>> Remove unused macro atomic_xchg().
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>> ---
>> xen/arch/arm/include/asm/atomic.h | 2 --
>> 1 file changed, 2 deletions(-)
>> diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h
>> index f5ef744b4b..a2dc125291 100644
>> --- a/xen/arch/arm/include/asm/atomic.h
>> +++ b/xen/arch/arm/include/asm/atomic.h
>> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
>> return __atomic_add_unless(v, a, u);
>> }
>> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>> -
> 
> While I agree this is unused today, the wrapper is quite trivial and part of the generic API (x86 also provides one). So I am not in favor of removing it just to please MISRA.
> 
> That said, if Bertrand and Stefano agrees with removing it then you should also remove the x86 version to avoid inconsistency.

I think we can keep this and maybe add a comment on top to document a known violation:
/* TODO: MISRA_VIOLATION 2.5 */

The FUSA SIG is still working on defining how to document those in the code.

I think I suggested one way to do this at some point but the discussion never finished.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
  2022-07-28  7:57     ` Bertrand Marquis
@ 2022-07-28  9:35       ` Julien Grall
  2022-07-28  9:45         ` Bertrand Marquis
  0 siblings, 1 reply; 13+ messages in thread
From: Julien Grall @ 2022-07-28  9:35 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Xenia Ragiadakou, xen-devel, Stefano Stabellini, Volodymyr Babchuk



On 28/07/2022 08:57, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

> 
>> On 27 Jul 2022, at 16:46, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Xenia,
>>
>> On 27/07/2022 16:32, Xenia Ragiadakou wrote:
>>> Remove unused macro atomic_xchg().
>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>>> ---
>>> xen/arch/arm/include/asm/atomic.h | 2 --
>>> 1 file changed, 2 deletions(-)
>>> diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h
>>> index f5ef744b4b..a2dc125291 100644
>>> --- a/xen/arch/arm/include/asm/atomic.h
>>> +++ b/xen/arch/arm/include/asm/atomic.h
>>> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
>>> return __atomic_add_unless(v, a, u);
>>> }
>>> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>>> -
>>
>> While I agree this is unused today, the wrapper is quite trivial and part of the generic API (x86 also provides one). So I am not in favor of removing it just to please MISRA.
>>
>> That said, if Bertrand and Stefano agrees with removing it then you should also remove the x86 version to avoid inconsistency.
> 
> I think we can keep this and maybe add a comment on top to document a known violation:
> /* TODO: MISRA_VIOLATION 2.5 */

While I am fine with this goal of the comment (i.e. indicating where Xen 
is not MISRA compliant), I think this is one place where I would rather 
not want one because it can get stale if someones decide to use the 
function.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
  2022-07-28  9:35       ` Julien Grall
@ 2022-07-28  9:45         ` Bertrand Marquis
  2022-07-28 10:21           ` Julien Grall
  0 siblings, 1 reply; 13+ messages in thread
From: Bertrand Marquis @ 2022-07-28  9:45 UTC (permalink / raw)
  To: Julien Grall
  Cc: Xenia Ragiadakou, xen-devel, Stefano Stabellini, Volodymyr Babchuk

Hi Julien,

> On 28 Jul 2022, at 10:35, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 28/07/2022 08:57, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 27 Jul 2022, at 16:46, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi Xenia,
>>> 
>>> On 27/07/2022 16:32, Xenia Ragiadakou wrote:
>>>> Remove unused macro atomic_xchg().
>>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>>>> ---
>>>> xen/arch/arm/include/asm/atomic.h | 2 --
>>>> 1 file changed, 2 deletions(-)
>>>> diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h
>>>> index f5ef744b4b..a2dc125291 100644
>>>> --- a/xen/arch/arm/include/asm/atomic.h
>>>> +++ b/xen/arch/arm/include/asm/atomic.h
>>>> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
>>>> return __atomic_add_unless(v, a, u);
>>>> }
>>>> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>>>> -
>>> 
>>> While I agree this is unused today, the wrapper is quite trivial and part of the generic API (x86 also provides one). So I am not in favor of removing it just to please MISRA.
>>> 
>>> That said, if Bertrand and Stefano agrees with removing it then you should also remove the x86 version to avoid inconsistency.
>> I think we can keep this and maybe add a comment on top to document a known violation:
>> /* TODO: MISRA_VIOLATION 2.5 */
> 
> While I am fine with this goal of the comment (i.e. indicating where Xen is not MISRA compliant), I think this is one place where I would rather not want one because it can get stale if someones decide to use the function.

I think the one doing that will have to update the comment otherwise we will never manage to have an analysis without findings.
Having those kind of comments in the code for violation also means that they must be updated if the violation is solved.

Maybe we will need a run ignoring those to identify possible violations which are not violations anymore but this might be hard to do.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
  2022-07-28  9:45         ` Bertrand Marquis
@ 2022-07-28 10:21           ` Julien Grall
  2022-07-28 10:26             ` Bertrand Marquis
  0 siblings, 1 reply; 13+ messages in thread
From: Julien Grall @ 2022-07-28 10:21 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Xenia Ragiadakou, xen-devel, Stefano Stabellini, Volodymyr Babchuk



On 28/07/2022 10:45, Bertrand Marquis wrote:
>> On 28 Jul 2022, at 10:35, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 28/07/2022 08:57, Bertrand Marquis wrote:
>>> Hi Julien,
>>
>> Hi Bertrand,
>>
>>>> On 27 Jul 2022, at 16:46, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi Xenia,
>>>>
>>>> On 27/07/2022 16:32, Xenia Ragiadakou wrote:
>>>>> Remove unused macro atomic_xchg().
>>>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>>>>> ---
>>>>> xen/arch/arm/include/asm/atomic.h | 2 --
>>>>> 1 file changed, 2 deletions(-)
>>>>> diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h
>>>>> index f5ef744b4b..a2dc125291 100644
>>>>> --- a/xen/arch/arm/include/asm/atomic.h
>>>>> +++ b/xen/arch/arm/include/asm/atomic.h
>>>>> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
>>>>> return __atomic_add_unless(v, a, u);
>>>>> }
>>>>> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>>>>> -
>>>>
>>>> While I agree this is unused today, the wrapper is quite trivial and part of the generic API (x86 also provides one). So I am not in favor of removing it just to please MISRA.
>>>>
>>>> That said, if Bertrand and Stefano agrees with removing it then you should also remove the x86 version to avoid inconsistency.
>>> I think we can keep this and maybe add a comment on top to document a known violation:
>>> /* TODO: MISRA_VIOLATION 2.5 */
>>
>> While I am fine with this goal of the comment (i.e. indicating where Xen is not MISRA compliant), I think this is one place where I would rather not want one because it can get stale if someones decide to use the function.
> 
> I think the one doing that will have to update the comment otherwise we will never manage to have an analysis without findings.

I was under the impression that Xen will never officially follow some of 
the MISRA rules. So I would expect the tools to be able to detect such 
cases so we don't have to add a comment for every deviation on something 
we will never support.

> Having those kind of comments in the code for violation also means that they must be updated if the violation is solved.

Right, but for thing like unused function, this is quite easy to miss by 
both the developer and reviewers. So we are going to end up to comments 
for nothing.

> 
> Maybe we will need a run ignoring those to identify possible violations which are not violations anymore but this might be hard to do.

TBH, I think it would be best if we can teach the tools to ignore 
certain rules.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
  2022-07-28 10:21           ` Julien Grall
@ 2022-07-28 10:26             ` Bertrand Marquis
  2022-07-28 23:01               ` Stefano Stabellini
  0 siblings, 1 reply; 13+ messages in thread
From: Bertrand Marquis @ 2022-07-28 10:26 UTC (permalink / raw)
  To: Julien Grall
  Cc: Xenia Ragiadakou, xen-devel, Stefano Stabellini, Volodymyr Babchuk

Hi Julien,

> On 28 Jul 2022, at 11:21, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 28/07/2022 10:45, Bertrand Marquis wrote:
>>> On 28 Jul 2022, at 10:35, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 28/07/2022 08:57, Bertrand Marquis wrote:
>>>> Hi Julien,
>>> 
>>> Hi Bertrand,
>>> 
>>>>> On 27 Jul 2022, at 16:46, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> Hi Xenia,
>>>>> 
>>>>> On 27/07/2022 16:32, Xenia Ragiadakou wrote:
>>>>>> Remove unused macro atomic_xchg().
>>>>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>>>>>> ---
>>>>>> xen/arch/arm/include/asm/atomic.h | 2 --
>>>>>> 1 file changed, 2 deletions(-)
>>>>>> diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h
>>>>>> index f5ef744b4b..a2dc125291 100644
>>>>>> --- a/xen/arch/arm/include/asm/atomic.h
>>>>>> +++ b/xen/arch/arm/include/asm/atomic.h
>>>>>> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
>>>>>> return __atomic_add_unless(v, a, u);
>>>>>> }
>>>>>> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>>>>>> -
>>>>> 
>>>>> While I agree this is unused today, the wrapper is quite trivial and part of the generic API (x86 also provides one). So I am not in favor of removing it just to please MISRA.
>>>>> 
>>>>> That said, if Bertrand and Stefano agrees with removing it then you should also remove the x86 version to avoid inconsistency.
>>>> I think we can keep this and maybe add a comment on top to document a known violation:
>>>> /* TODO: MISRA_VIOLATION 2.5 */
>>> 
>>> While I am fine with this goal of the comment (i.e. indicating where Xen is not MISRA compliant), I think this is one place where I would rather not want one because it can get stale if someones decide to use the function.
>> I think the one doing that will have to update the comment otherwise we will never manage to have an analysis without findings.
> 
> I was under the impression that Xen will never officially follow some of the MISRA rules. So I would expect the tools to be able to detect such cases so we don't have to add a comment for every deviation on something we will never support.
> 
>> Having those kind of comments in the code for violation also means that they must be updated if the violation is solved.
> 
> Right, but for thing like unused function, this is quite easy to miss by both the developer and reviewers. So we are going to end up to comments for nothing.
> 
>> Maybe we will need a run ignoring those to identify possible violations which are not violations anymore but this might be hard to do.
> 
> TBH, I think it would be best if we can teach the tools to ignore certain rules.

Definitely it is possible to instruct the tool to ignore this you are right and for 2.5 we should (for some reason I was under the impression that we said we would follow 2.5 but accept deviations).

@Xenia: please ignore and do not add a comment for this.

I think we will need to distinguish 2 kind of not following:
- not following at all (disable in the tools)
- accepting some deviations (documented in the code)

As much as we can, I think we should target the second unless we have a lot of violations.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation
  2022-07-28 10:26             ` Bertrand Marquis
@ 2022-07-28 23:01               ` Stefano Stabellini
  0 siblings, 0 replies; 13+ messages in thread
From: Stefano Stabellini @ 2022-07-28 23:01 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Julien Grall, Xenia Ragiadakou, xen-devel, Stefano Stabellini,
	Volodymyr Babchuk

On Thu, 28 Jul 2022, Bertrand Marquis wrote:
> > On 28 Jul 2022, at 11:21, Julien Grall <julien@xen.org> wrote:
> > On 28/07/2022 10:45, Bertrand Marquis wrote:
> >>> On 28 Jul 2022, at 10:35, Julien Grall <julien@xen.org> wrote:
> >>> On 28/07/2022 08:57, Bertrand Marquis wrote:
> >>>> Hi Julien,
> >>> 
> >>> Hi Bertrand,
> >>> 
> >>>>> On 27 Jul 2022, at 16:46, Julien Grall <julien@xen.org> wrote:
> >>>>> 
> >>>>> Hi Xenia,
> >>>>> 
> >>>>> On 27/07/2022 16:32, Xenia Ragiadakou wrote:
> >>>>>> Remove unused macro atomic_xchg().
> >>>>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> >>>>>> ---
> >>>>>> xen/arch/arm/include/asm/atomic.h | 2 --
> >>>>>> 1 file changed, 2 deletions(-)
> >>>>>> diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h
> >>>>>> index f5ef744b4b..a2dc125291 100644
> >>>>>> --- a/xen/arch/arm/include/asm/atomic.h
> >>>>>> +++ b/xen/arch/arm/include/asm/atomic.h
> >>>>>> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
> >>>>>> return __atomic_add_unless(v, a, u);
> >>>>>> }
> >>>>>> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new))
> >>>>>> -
> >>>>> 
> >>>>> While I agree this is unused today, the wrapper is quite trivial and part of the generic API (x86 also provides one). So I am not in favor of removing it just to please MISRA.
> >>>>> 
> >>>>> That said, if Bertrand and Stefano agrees with removing it then you should also remove the x86 version to avoid inconsistency.
> >>>> I think we can keep this and maybe add a comment on top to document a known violation:
> >>>> /* TODO: MISRA_VIOLATION 2.5 */
> >>> 
> >>> While I am fine with this goal of the comment (i.e. indicating where Xen is not MISRA compliant), I think this is one place where I would rather not want one because it can get stale if someones decide to use the function.
> >> I think the one doing that will have to update the comment otherwise we will never manage to have an analysis without findings.
> > 
> > I was under the impression that Xen will never officially follow some of the MISRA rules. So I would expect the tools to be able to detect such cases so we don't have to add a comment for every deviation on something we will never support.
> > 
> >> Having those kind of comments in the code for violation also means that they must be updated if the violation is solved.
> > 
> > Right, but for thing like unused function, this is quite easy to miss by both the developer and reviewers. So we are going to end up to comments for nothing.
> > 
> >> Maybe we will need a run ignoring those to identify possible violations which are not violations anymore but this might be hard to do.
> > 
> > TBH, I think it would be best if we can teach the tools to ignore certain rules.
> 
> Definitely it is possible to instruct the tool to ignore this you are right and for 2.5 we should (for some reason I was under the impression that we said we would follow 2.5 but accept deviations).

Absolutely possible, basically we (the community) are the ones providing
the list of rules to the MISRA C checkers.


> @Xenia: please ignore and do not add a comment for this.
> 
> I think we will need to distinguish 2 kind of not following:
> - not following at all (disable in the tools)
> - accepting some deviations (documented in the code)

Yes, exactly right.


> As much as we can, I think we should target the second unless we have a lot of violations.

+1


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-07-28 23:01 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-27 15:32 [PATCH 0/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule violations Xenia Ragiadakou
2022-07-27 15:32 ` [PATCH 1/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 20.7 violation Xenia Ragiadakou
2022-07-27 15:36   ` Jan Beulich
2022-07-27 16:18     ` Xenia Ragiadakou
2022-07-27 15:32 ` [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation Xenia Ragiadakou
2022-07-27 15:46   ` Julien Grall
2022-07-27 16:23     ` Xenia Ragiadakou
2022-07-28  7:57     ` Bertrand Marquis
2022-07-28  9:35       ` Julien Grall
2022-07-28  9:45         ` Bertrand Marquis
2022-07-28 10:21           ` Julien Grall
2022-07-28 10:26             ` Bertrand Marquis
2022-07-28 23:01               ` Stefano Stabellini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.