[PATCH v3 0/2] Resolve issues related to macros in device_tree.h

[PATCH v3 0/2] Resolve issues related to macros in device_tree.h

[Xenia Ragiadakou]

Xenia Ragiadakou (2):
  xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7
  xen/device_tree: remove incorrect and unused dt_irq() and
    dt_irq_flags() macros

 xen/include/xen/device_tree.h | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

-- 
2.37.2

[PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Xenia Ragiadakou]

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---

Changes in v3:
  - the fixes are based solely to Eclair findings (the tool has been
    adjusted to report only those violations that can result to a bug)
  - reflect this in the commit title

 xen/include/xen/device_tree.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index a28937d12ae8..7839a199e311 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -37,11 +37,11 @@ struct dt_device_match {
     const void *data;
 };
 
-#define __DT_MATCH_PATH(p)              .path = p
-#define __DT_MATCH_TYPE(typ)            .type = typ
-#define __DT_MATCH_COMPATIBLE(compat)   .compatible = compat
+#define __DT_MATCH_PATH(p)              .path = (p)
+#define __DT_MATCH_TYPE(typ)            .type = (typ)
+#define __DT_MATCH_COMPATIBLE(compat)   .compatible = (compat)
 #define __DT_MATCH_NOT_AVAILABLE()      .not_available = 1
-#define __DT_MATCH_PROP(p)              .prop = p
+#define __DT_MATCH_PROP(p)              .prop = (p)
 
 #define DT_MATCH_PATH(p)                { __DT_MATCH_PATH(p) }
 #define DT_MATCH_TYPE(typ)              { __DT_MATCH_TYPE(typ) }
@@ -226,13 +226,13 @@ dt_find_interrupt_controller(const struct dt_device_match *matches);
 #define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
 
 #define dt_for_each_property_node(dn, pp)                   \
-    for ( pp = dn->properties; pp != NULL; pp = pp->next )
+    for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )
 
 #define dt_for_each_device_node(dt, dn)                     \
-    for ( dn = dt; dn != NULL; dn = dn->allnext )
+    for ( dn = dt; (dn) != NULL; dn = (dn)->allnext )
 
 #define dt_for_each_child_node(dt, dn)                      \
-    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
+    for ( dn = (dt)->child; (dn) != NULL; dn = (dn)->sibling )
 
 /* Helper to read a big number; size is in cells (not bytes) */
 static inline u64 dt_read_number(const __be32 *cell, int size)
-- 
2.37.2

[PATCH v3 2/2] xen/device_tree: remove incorrect and unused dt_irq() and dt_irq_flags() macros

[Xenia Ragiadakou]

Macro dt_irq() is broken because the macro parameter has the same name with
the struct dt_irq member "irq".
Macro dt_irq_flags() is broken as well because struct dt_irq has no member
named "flags".

Since no one seems to have ever tried to use them and eventually stumble upon
the issues above, remove them instead of fixing them.

Fixes: 886f34045bf0 ("xen/arm: Add helpers to retrieve an interrupt description from the device tree")
Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---

Changes in v3:
  - new patch

 xen/include/xen/device_tree.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 7839a199e311..19a74909cece 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -159,9 +159,6 @@ struct dt_raw_irq {
     u32 specifier[DT_MAX_IRQ_SPEC];
 };
 
-#define dt_irq(irq) ((irq)->irq)
-#define dt_irq_flags(irq) ((irq)->flags)
-
 typedef int (*device_tree_node_func)(const void *fdt,
                                      int node, const char *name, int depth,
                                      u32 address_cells, u32 size_cells,
-- 
2.37.2

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Luca Fancellu]

Hi Xenia,

> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> ---
> 
> Changes in v3:
>  - the fixes are based solely to Eclair findings (the tool has been
>    adjusted to report only those violations that can result to a bug)
>  - reflect this in the commit title
> 
> xen/include/xen/device_tree.h | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index a28937d12ae8..7839a199e311 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -37,11 +37,11 @@ struct dt_device_match {
>     const void *data;
> };
> 
> -#define __DT_MATCH_PATH(p)              .path = p
> -#define __DT_MATCH_TYPE(typ)            .type = typ
> -#define __DT_MATCH_COMPATIBLE(compat)   .compatible = compat
> +#define __DT_MATCH_PATH(p)              .path = (p)
> +#define __DT_MATCH_TYPE(typ)            .type = (typ)
> +#define __DT_MATCH_COMPATIBLE(compat)   .compatible = (compat)
> #define __DT_MATCH_NOT_AVAILABLE()      .not_available = 1
> -#define __DT_MATCH_PROP(p)              .prop = p
> +#define __DT_MATCH_PROP(p)              .prop = (p)
> 
> #define DT_MATCH_PATH(p)                { __DT_MATCH_PATH(p) }
> #define DT_MATCH_TYPE(typ)              { __DT_MATCH_TYPE(typ) }
> @@ -226,13 +226,13 @@ dt_find_interrupt_controller(const struct dt_device_match *matches);
> #define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
> 
> #define dt_for_each_property_node(dn, pp)                   \
> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
> +    for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )
> 
> #define dt_for_each_device_node(dt, dn)                     \
> -    for ( dn = dt; dn != NULL; dn = dn->allnext )
> +    for ( dn = dt; (dn) != NULL; dn = (dn)->allnext )
> 
> #define dt_for_each_child_node(dt, dn)                      \
> -    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
> +    for ( dn = (dt)->child; (dn) != NULL; dn = (dn)->sibling )
> 
> /* Helper to read a big number; size is in cells (not bytes) */
> static inline u64 dt_read_number(const __be32 *cell, int size)
> -- 
> 2.37.2
> 
> 

While the changes looks sensible to me, I’ve had a look in eclair but I was unable to find the findings,
here what findings I have in the latest report: 
https://eclairit.com:8443/job/XEN/Target=ARM64,agent=docker1/lastBuild/eclair/packageName.832204620/fileName.1811675806/

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Michal Orzel]

Hi Luca,

On 06/02/2023 15:31, Luca Fancellu wrote:
> 
> 
> Hi Xenia,
> 
>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>>
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>> ---
>>
>> Changes in v3:
>>  - the fixes are based solely to Eclair findings (the tool has been
>>    adjusted to report only those violations that can result to a bug)
>>  - reflect this in the commit title
>>
>> xen/include/xen/device_tree.h | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index a28937d12ae8..7839a199e311 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -37,11 +37,11 @@ struct dt_device_match {
>>     const void *data;
>> };
>>
>> -#define __DT_MATCH_PATH(p)              .path = p
>> -#define __DT_MATCH_TYPE(typ)            .type = typ
>> -#define __DT_MATCH_COMPATIBLE(compat)   .compatible = compat
>> +#define __DT_MATCH_PATH(p)              .path = (p)
>> +#define __DT_MATCH_TYPE(typ)            .type = (typ)
>> +#define __DT_MATCH_COMPATIBLE(compat)   .compatible = (compat)
>> #define __DT_MATCH_NOT_AVAILABLE()      .not_available = 1
>> -#define __DT_MATCH_PROP(p)              .prop = p
>> +#define __DT_MATCH_PROP(p)              .prop = (p)
>>
>> #define DT_MATCH_PATH(p)                { __DT_MATCH_PATH(p) }
>> #define DT_MATCH_TYPE(typ)              { __DT_MATCH_TYPE(typ) }
>> @@ -226,13 +226,13 @@ dt_find_interrupt_controller(const struct dt_device_match *matches);
>> #define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
>>
>> #define dt_for_each_property_node(dn, pp)                   \
>> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
>> +    for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )
>>
>> #define dt_for_each_device_node(dt, dn)                     \
>> -    for ( dn = dt; dn != NULL; dn = dn->allnext )
>> +    for ( dn = dt; (dn) != NULL; dn = (dn)->allnext )
>>
>> #define dt_for_each_child_node(dt, dn)                      \
>> -    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
>> +    for ( dn = (dt)->child; (dn) != NULL; dn = (dn)->sibling )
>>
>> /* Helper to read a big number; size is in cells (not bytes) */
>> static inline u64 dt_read_number(const __be32 *cell, int size)
>> --
>> 2.37.2
>>
>>
> 
> While the changes looks sensible to me, I’ve had a look in eclair but I was unable to find the findings,
> here what findings I have in the latest report:
> https://eclairit.com:8443/job/XEN/Target=ARM64,agent=docker1/lastBuild/eclair/packageName.832204620/fileName.1811675806/

Eclair does not report violations at the definition but rather at the use.
Check domain_build.c for example to see the reports for 20.7 related to these macros.

~Michal

Re: [PATCH v3 2/2] xen/device_tree: remove incorrect and unused dt_irq() and dt_irq_flags() macros

[Luca Fancellu]

> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
> 
> Macro dt_irq() is broken because the macro parameter has the same name with
> the struct dt_irq member "irq".

I’m not sure about the wording “broken”, it should work anyway or am I wrong?

> Macro dt_irq_flags() is broken as well because struct dt_irq has no member
> named "flags".

Yes this depends if the macro was meant to access the structure dt_irq, I’ve had a look
on the commit introducing it but I could not figure out the usage.

> 
> Since no one seems to have ever tried to use them and eventually stumble upon
> the issues above, remove them instead of fixing them.
> 
> Fixes: 886f34045bf0 ("xen/arm: Add helpers to retrieve an interrupt description from the device tree")
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> ---
> 
> Changes in v3:
>  - new patch
> 
> xen/include/xen/device_tree.h | 3 ---
> 1 file changed, 3 deletions(-)
> 
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 7839a199e311..19a74909cece 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -159,9 +159,6 @@ struct dt_raw_irq {
>     u32 specifier[DT_MAX_IRQ_SPEC];
> };
> 
> -#define dt_irq(irq) ((irq)->irq)
> -#define dt_irq_flags(irq) ((irq)->flags)
> -
> typedef int (*device_tree_node_func)(const void *fdt,
>                                      int node, const char *name, int depth,
>                                      u32 address_cells, u32 size_cells,
> -- 
> 2.37.2
> 
> 

They seems indeed unused, so for me the code looks good:

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Re: [PATCH v3 2/2] xen/device_tree: remove incorrect and unused dt_irq() and dt_irq_flags() macros

[Xenia Ragiadakou]

Hi Luca

On 2/6/23 16:42, Luca Fancellu wrote:
> 
> 
>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>>
>> Macro dt_irq() is broken because the macro parameter has the same name with
>> the struct dt_irq member "irq".
> 
> I’m not sure about the wording “broken”, it should work anyway or am I wrong?

No, it won't work because the structure member will be substituted as 
well by the macro argument (for instance dt_irq(blah) will be replaced 
by (blah)->blah).

> 
>> Macro dt_irq_flags() is broken as well because struct dt_irq has no member
>> named "flags".
> 
> Yes this depends if the macro was meant to access the structure dt_irq, I’ve had a look
> on the commit introducing it but I could not figure out the usage.

Given the macro name, I assumed that it was meant to be used for 
accessing a dt_irq field.

> 
>>
>> Since no one seems to have ever tried to use them and eventually stumble upon
>> the issues above, remove them instead of fixing them.
>>
>> Fixes: 886f34045bf0 ("xen/arm: Add helpers to retrieve an interrupt description from the device tree")
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>> ---
>>
>> Changes in v3:
>>   - new patch
>>
>> xen/include/xen/device_tree.h | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index 7839a199e311..19a74909cece 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -159,9 +159,6 @@ struct dt_raw_irq {
>>      u32 specifier[DT_MAX_IRQ_SPEC];
>> };
>>
>> -#define dt_irq(irq) ((irq)->irq)
>> -#define dt_irq_flags(irq) ((irq)->flags)
>> -
>> typedef int (*device_tree_node_func)(const void *fdt,
>>                                       int node, const char *name, int depth,
>>                                       u32 address_cells, u32 size_cells,
>> -- 
>> 2.37.2
>>
>>
> 
> They seems indeed unused, so for me the code looks good:
> 
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> 

-- 
Xenia

Re: [PATCH v3 2/2] xen/device_tree: remove incorrect and unused dt_irq() and dt_irq_flags() macros

[Luca Fancellu]

> On 6 Feb 2023, at 14:51, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
> 
> Hi Luca
> 
> On 2/6/23 16:42, Luca Fancellu wrote:
>>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>>> 
>>> Macro dt_irq() is broken because the macro parameter has the same name with
>>> the struct dt_irq member "irq".
>> I’m not sure about the wording “broken”, it should work anyway or am I wrong?
> 
> No, it won't work because the structure member will be substituted as well by the macro argument (for instance dt_irq(blah) will be replaced by (blah)->blah).

Yes you are right, I was reading it in the wrong way! 

> 
>>> Macro dt_irq_flags() is broken as well because struct dt_irq has no member
>>> named "flags".
>> Yes this depends if the macro was meant to access the structure dt_irq, I’ve had a look
>> on the commit introducing it but I could not figure out the usage.
> 
> Given the macro name, I assumed that it was meant to be used for accessing a dt_irq field.

Yes I would come to the same conclusion

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Luca Fancellu]

> On 6 Feb 2023, at 14:37, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> Hi Luca,
> 
> On 06/02/2023 15:31, Luca Fancellu wrote:
>> 
>> 
>> Hi Xenia,
>> 
>>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>>> 
>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>>> ---
>>> 
>>> Changes in v3:
>>> - the fixes are based solely to Eclair findings (the tool has been
>>>   adjusted to report only those violations that can result to a bug)
>>> - reflect this in the commit title
>>> 
>>> xen/include/xen/device_tree.h | 14 +++++++-------
>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>>> index a28937d12ae8..7839a199e311 100644
>>> --- a/xen/include/xen/device_tree.h
>>> +++ b/xen/include/xen/device_tree.h
>>> @@ -37,11 +37,11 @@ struct dt_device_match {
>>>    const void *data;
>>> };
>>> 
>>> -#define __DT_MATCH_PATH(p)              .path = p
>>> -#define __DT_MATCH_TYPE(typ)            .type = typ
>>> -#define __DT_MATCH_COMPATIBLE(compat)   .compatible = compat
>>> +#define __DT_MATCH_PATH(p)              .path = (p)
>>> +#define __DT_MATCH_TYPE(typ)            .type = (typ)
>>> +#define __DT_MATCH_COMPATIBLE(compat)   .compatible = (compat)
>>> #define __DT_MATCH_NOT_AVAILABLE()      .not_available = 1
>>> -#define __DT_MATCH_PROP(p)              .prop = p
>>> +#define __DT_MATCH_PROP(p)              .prop = (p)
>>> 
>>> #define DT_MATCH_PATH(p)                { __DT_MATCH_PATH(p) }
>>> #define DT_MATCH_TYPE(typ)              { __DT_MATCH_TYPE(typ) }
>>> @@ -226,13 +226,13 @@ dt_find_interrupt_controller(const struct dt_device_match *matches);
>>> #define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
>>> 
>>> #define dt_for_each_property_node(dn, pp)                   \
>>> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
>>> +    for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )
>>> 
>>> #define dt_for_each_device_node(dt, dn)                     \
>>> -    for ( dn = dt; dn != NULL; dn = dn->allnext )
>>> +    for ( dn = dt; (dn) != NULL; dn = (dn)->allnext )
>>> 
>>> #define dt_for_each_child_node(dt, dn)                      \
>>> -    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
>>> +    for ( dn = (dt)->child; (dn) != NULL; dn = (dn)->sibling )
>>> 
>>> /* Helper to read a big number; size is in cells (not bytes) */
>>> static inline u64 dt_read_number(const __be32 *cell, int size)
>>> --
>>> 2.37.2
>>> 
>>> 
>> 
>> While the changes looks sensible to me, I’ve had a look in eclair but I was unable to find the findings,
>> here what findings I have in the latest report:
>> https://eclairit.com:8443/job/XEN/Target=ARM64,agent=docker1/lastBuild/eclair/packageName.832204620/fileName.1811675806/
> 
> Eclair does not report violations at the definition but rather at the use.
> Check domain_build.c for example to see the reports for 20.7 related to these macros.

Wow yes that’s right, a bit annoying to link the issues! Who knows if there is a mode to make it point out the violation at the definition
like cppcheck.

I’ll review this patch later this week if I manage to find the time

> 
> ~Michal

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Luca Fancellu]

> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
> 

I’m not really a supporter of empty commit message, but it’s up to the maintainer :)

For me the changes looks good

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>


> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> ---
> 
> Changes in v3:
>  - the fixes are based solely to Eclair findings (the tool has been
>    adjusted to report only those violations that can result to a bug)
>  - reflect this in the commit title
> 
> xen/include/xen/device_tree.h | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index a28937d12ae8..7839a199e311 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -37,11 +37,11 @@ struct dt_device_match {
>     const void *data;
> };
> 
> -#define __DT_MATCH_PATH(p)              .path = p
> -#define __DT_MATCH_TYPE(typ)            .type = typ
> -#define __DT_MATCH_COMPATIBLE(compat)   .compatible = compat
> +#define __DT_MATCH_PATH(p)              .path = (p)
> +#define __DT_MATCH_TYPE(typ)            .type = (typ)
> +#define __DT_MATCH_COMPATIBLE(compat)   .compatible = (compat)
> #define __DT_MATCH_NOT_AVAILABLE()      .not_available = 1
> -#define __DT_MATCH_PROP(p)              .prop = p
> +#define __DT_MATCH_PROP(p)              .prop = (p)
> 
> #define DT_MATCH_PATH(p)                { __DT_MATCH_PATH(p) }
> #define DT_MATCH_TYPE(typ)              { __DT_MATCH_TYPE(typ) }
> @@ -226,13 +226,13 @@ dt_find_interrupt_controller(const struct dt_device_match *matches);
> #define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
> 
> #define dt_for_each_property_node(dn, pp)                   \
> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
> +    for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )
> 
> #define dt_for_each_device_node(dt, dn)                     \
> -    for ( dn = dt; dn != NULL; dn = dn->allnext )
> +    for ( dn = dt; (dn) != NULL; dn = (dn)->allnext )
> 
> #define dt_for_each_child_node(dt, dn)                      \
> -    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
> +    for ( dn = (dt)->child; (dn) != NULL; dn = (dn)->sibling )
> 
> /* Helper to read a big number; size is in cells (not bytes) */
> static inline u64 dt_read_number(const __be32 *cell, int size)
> -- 
> 2.37.2
> 
> 

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Julien Grall]

Hi,

On 07/02/2023 10:23, Luca Fancellu wrote:
> 
> 
>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>>
> 
> I’m not really a supporter of empty commit message, but it’s up to the maintainer :)

+1. In this case a brief summary of the rule would be handy for those 
that are not well-versed with MISRA.

This can be dealt on commit if you propose a new commit message.

> 
> For me the changes looks good
> 
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Xenia Ragiadakou]

On 2/7/23 12:39, Julien Grall wrote:
> Hi,
> 
> On 07/02/2023 10:23, Luca Fancellu wrote:
>>
>>
>>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>>>
>>
>> I’m not really a supporter of empty commit message, but it’s up to the 
>> maintainer :)
> 
> +1. In this case a brief summary of the rule would be handy for those 
> that are not well-versed with MISRA.
> 
> This can be dealt on commit if you propose a new commit message.

I 'm refrained from stating the rule as is because it is strict and it 
is not applied as is.

"Add parentheses around macro parameters when the precedence and 
associativity of the performed operators can lead to unintended order of 
evaluation."

Is this ok?

> 
>>
>> For me the changes looks good
>>
>> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> Cheers,
> 

-- 
Xenia

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Julien Grall]

On 07/02/2023 10:46, Xenia Ragiadakou wrote:
> 
> On 2/7/23 12:39, Julien Grall wrote:
>> Hi,
>>
>> On 07/02/2023 10:23, Luca Fancellu wrote:
>>>
>>>
>>>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> 
>>>> wrote:
>>>>
>>>
>>> I’m not really a supporter of empty commit message, but it’s up to 
>>> the maintainer :)
>>
>> +1. In this case a brief summary of the rule would be handy for those 
>> that are not well-versed with MISRA.
>>
>> This can be dealt on commit if you propose a new commit message.
> 
> I 'm refrained from stating the rule as is because it is strict and it 
> is not applied as is.

I am a bit confused with this statement. From misra/..., we are 
supporting rule 20.7. So why aren't applying it strictly?

And if it is not applied as-is, shouldn't we document the violation (if 
any)?

> 
> "Add parentheses around macro parameters when the precedence and 
> associativity of the performed operators can lead to unintended order of 
> evaluation."
> 
> Is this ok?

I am OK with this. Is there any ID from Eclair that could be used to 
track each error (and so we can confirm they have disappeared)?

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Xenia Ragiadakou]

On 2/7/23 14:25, Julien Grall wrote:
> 
> 
> On 07/02/2023 10:46, Xenia Ragiadakou wrote:
>>
>> On 2/7/23 12:39, Julien Grall wrote:
>>> Hi,
>>>
>>> On 07/02/2023 10:23, Luca Fancellu wrote:
>>>>
>>>>
>>>>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> 
>>>>> wrote:
>>>>>
>>>>
>>>> I’m not really a supporter of empty commit message, but it’s up to 
>>>> the maintainer :)
>>>
>>> +1. In this case a brief summary of the rule would be handy for those 
>>> that are not well-versed with MISRA.
>>>
>>> This can be dealt on commit if you propose a new commit message.
>>
>> I 'm refrained from stating the rule as is because it is strict and it 
>> is not applied as is.
> 
> I am a bit confused with this statement. From misra/..., we are 
> supporting rule 20.7. So why aren't applying it strictly?
> 
> And if it is not applied as-is, shouldn't we document the violation (if 
> any)?

I applied it strictly on v2, but there was no review.
Then Eclair was adjusted to have a less strict approach. Still there is 
a finding asking to add parentheses around dt in 
dt_for_each_device_node(dt, dn), i.e dn = (dt);, to which AFAIK you object.

> 
>>
>> "Add parentheses around macro parameters when the precedence and 
>> associativity of the performed operators can lead to unintended order 
>> of evaluation."
>>
>> Is this ok?
> 
> I am OK with this. Is there any ID from Eclair that could be used to 
> track each error (and so we can confirm they have disappeared)?

I am not aware of any.

The patch can be decoupled from misra and Eclair (I mean have a generic 
commit title) and just mention in the commit message that it fixes some 
Eclair findings for MISRA C rule 20.7.

> 
> Cheers,
> 

-- 
Xenia

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Julien Grall]

Hi,

On 07/02/2023 12:46, Xenia Ragiadakou wrote:
> On 2/7/23 14:25, Julien Grall wrote:
>>
>>
>> On 07/02/2023 10:46, Xenia Ragiadakou wrote:
>>>
>>> On 2/7/23 12:39, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 07/02/2023 10:23, Luca Fancellu wrote:
>>>>>
>>>>>
>>>>>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> 
>>>>>> wrote:
>>>>>>
>>>>>
>>>>> I’m not really a supporter of empty commit message, but it’s up to 
>>>>> the maintainer :)
>>>>
>>>> +1. In this case a brief summary of the rule would be handy for 
>>>> those that are not well-versed with MISRA.
>>>>
>>>> This can be dealt on commit if you propose a new commit message.
>>>
>>> I 'm refrained from stating the rule as is because it is strict and 
>>> it is not applied as is.
>>
>> I am a bit confused with this statement. From misra/..., we are 
>> supporting rule 20.7. So why aren't applying it strictly?
>>
>> And if it is not applied as-is, shouldn't we document the violation 
>> (if any)?
> 
> I applied it strictly on v2, but there was no review.

Ah! In general it is best to ping if there are no answers.

> Then Eclair was adjusted to have a less strict approach. Still there is 
> a finding asking to add parentheses around dt in 
> dt_for_each_device_node(dt, dn), i.e dn = (dt);, to which AFAIK you object.

Are you referring to the discussion in [1]? If so, I wasn't totally 
opposed to the suggestion so long we are consistent.

> 
>>
>>>
>>> "Add parentheses around macro parameters when the precedence and 
>>> associativity of the performed operators can lead to unintended order 
>>> of evaluation."
>>>
>>> Is this ok?
>>
>> I am OK with this. Is there any ID from Eclair that could be used to 
>> track each error (and so we can confirm they have disappeared)?
> 
> I am not aware of any.
Hmmm ok. It might be a nice feature to suggest in Eclair because anyone 
can check whether an issue was resolved.

Here, I don't exactly know what to check in Eclair. So I have to rely on 
you. Anyway, nothing that can be fixed for this commit.

> 
> The patch can be decoupled from misra and Eclair (I mean have a generic 
> commit title) and just mention in the commit message that it fixes some 
> Eclair findings for MISRA C rule 20.7.

I have a slight preference for a more generic title. But the current one 
also work for me.

I will commit later on.

Cheers,

[1] b2f2d1e7-0c18-206f-5e9d-d0115e398840@xen.org
> 
>>
>> Cheers,
>>
> 

-- 
Julien Grall

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Xenia Ragiadakou]

Hi Julien,

On 2/7/23 15:01, Julien Grall wrote:
> Hi,
> 
> On 07/02/2023 12:46, Xenia Ragiadakou wrote:
>> On 2/7/23 14:25, Julien Grall wrote:
>>>
>>>
>>> On 07/02/2023 10:46, Xenia Ragiadakou wrote:
>>>>
>>>> On 2/7/23 12:39, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 07/02/2023 10:23, Luca Fancellu wrote:
>>>>>>
>>>>>>
>>>>>>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou <burzalodowa@gmail.com> 
>>>>>>> wrote:
>>>>>>>
>>>>>>
>>>>>> I’m not really a supporter of empty commit message, but it’s up to 
>>>>>> the maintainer :)
>>>>>
>>>>> +1. In this case a brief summary of the rule would be handy for 
>>>>> those that are not well-versed with MISRA.
>>>>>
>>>>> This can be dealt on commit if you propose a new commit message.
>>>>
>>>> I 'm refrained from stating the rule as is because it is strict and 
>>>> it is not applied as is.
>>>
>>> I am a bit confused with this statement. From misra/..., we are 
>>> supporting rule 20.7. So why aren't applying it strictly?
>>>
>>> And if it is not applied as-is, shouldn't we document the violation 
>>> (if any)?
>>
>> I applied it strictly on v2, but there was no review.
> 
> Ah! In general it is best to ping if there are no answers.

Me too I ve forgotten about it. A ticket in gitlab reminded me that it 
was pending.

> 
>> Then Eclair was adjusted to have a less strict approach. Still there 
>> is a finding asking to add parentheses around dt in 
>> dt_for_each_device_node(dt, dn), i.e dn = (dt);, to which AFAIK you 
>> object.
> 
> Are you referring to the discussion in [1]? If so, I wasn't totally 
> opposed to the suggestion so long we are consistent.

I am not able to find [1]. I 'm referring to the discussion in 
20221220085100.22848-6-luca.fancellu@arm.com and 
20220728134943.1185621-1-burzalodowa@gmail.com

> 
>>
>>>
>>>>
>>>> "Add parentheses around macro parameters when the precedence and 
>>>> associativity of the performed operators can lead to unintended 
>>>> order of evaluation."
>>>>
>>>> Is this ok?
>>>
>>> I am OK with this. Is there any ID from Eclair that could be used to 
>>> track each error (and so we can confirm they have disappeared)?
>>
>> I am not aware of any.
> Hmmm ok. It might be a nice feature to suggest in Eclair because anyone 
> can check whether an issue was resolved.

Currently, the violations in Eclair are reported per macro call (I guess 
because it is acceptable to have parentheses around the macro args) so 
it is difficult to track all of them.

> 
> Here, I don't exactly know what to check in Eclair. So I have to rely on 
> you. Anyway, nothing that can be fixed for this commit.
> 
>>
>> The patch can be decoupled from misra and Eclair (I mean have a 
>> generic commit title) and just mention in the commit message that it 
>> fixes some Eclair findings for MISRA C rule 20.7.
> 
> I have a slight preference for a more generic title. But the current one 
> also work for me.

It can be changed into "xen/device_tree: add parentheses around macro 
parameters"

> 
> I will commit later on.

Do you want me to send a v4?

> 
> Cheers,
> 
> [1] b2f2d1e7-0c18-206f-5e9d-d0115e398840@xen.org
>>
>>>
>>> Cheers,
>>>
>>
> 

-- 
Xenia

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Julien Grall]

On 07/02/2023 13:59, Xenia Ragiadakou wrote:
> Hi Julien,

Hi Xenia,

> 
> On 2/7/23 15:01, Julien Grall wrote:
>> Hi,
>>
>> On 07/02/2023 12:46, Xenia Ragiadakou wrote:
>>> On 2/7/23 14:25, Julien Grall wrote:
>>>>
>>>>
>>>> On 07/02/2023 10:46, Xenia Ragiadakou wrote:
>>>>>
>>>>> On 2/7/23 12:39, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 07/02/2023 10:23, Luca Fancellu wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou 
>>>>>>>> <burzalodowa@gmail.com> wrote:
>>>>>>>>
>>>>>>>
>>>>>>> I’m not really a supporter of empty commit message, but it’s up 
>>>>>>> to the maintainer :)
>>>>>>
>>>>>> +1. In this case a brief summary of the rule would be handy for 
>>>>>> those that are not well-versed with MISRA.
>>>>>>
>>>>>> This can be dealt on commit if you propose a new commit message.
>>>>>
>>>>> I 'm refrained from stating the rule as is because it is strict and 
>>>>> it is not applied as is.
>>>>
>>>> I am a bit confused with this statement. From misra/..., we are 
>>>> supporting rule 20.7. So why aren't applying it strictly?
>>>>
>>>> And if it is not applied as-is, shouldn't we document the violation 
>>>> (if any)?
>>>
>>> I applied it strictly on v2, but there was no review.
>>
>> Ah! In general it is best to ping if there are no answers.
> 
> Me too I ve forgotten about it. A ticket in gitlab reminded me that it 
> was pending.
> 
>>
>>> Then Eclair was adjusted to have a less strict approach. Still there 
>>> is a finding asking to add parentheses around dt in 
>>> dt_for_each_device_node(dt, dn), i.e dn = (dt);, to which AFAIK you 
>>> object.
>>
>> Are you referring to the discussion in [1]? If so, I wasn't totally 
>> opposed to the suggestion so long we are consistent.
> 
> I am not able to find [1].

https://lore.kernel.org/xen-devel/b2f2d1e7-0c18-206f-5e9d-d0115e398840@xen.org/

which is...


> I 'm referring to the discussion in 
> 20221220085100.22848-6-luca.fancellu@arm.com and 
> 20220728134943.1185621-1-burzalodowa@gmail.com

... a reply to this message. At the end of that reply, I said that I 
wasn't totally against adding the parentheses but we should be 
consistent in how we do it.

> 
>>
>>>
>>>>
>>>>>
>>>>> "Add parentheses around macro parameters when the precedence and 
>>>>> associativity of the performed operators can lead to unintended 
>>>>> order of evaluation."
>>>>>
>>>>> Is this ok?
>>>>
>>>> I am OK with this. Is there any ID from Eclair that could be used to 
>>>> track each error (and so we can confirm they have disappeared)?
>>>
>>> I am not aware of any.
>> Hmmm ok. It might be a nice feature to suggest in Eclair because 
>> anyone can check whether an issue was resolved.
> 
> Currently, the violations in Eclair are reported per macro call (I guess 
> because it is acceptable to have parentheses around the macro args) so 
> it is difficult to track all of them.
> 
>>
>> Here, I don't exactly know what to check in Eclair. So I have to rely 
>> on you. Anyway, nothing that can be fixed for this commit.
>>
>>>
>>> The patch can be decoupled from misra and Eclair (I mean have a 
>>> generic commit title) and just mention in the commit message that it 
>>> fixes some Eclair findings for MISRA C rule 20.7.
>>
>> I have a slight preference for a more generic title. But the current 
>> one also work for me.
> 
> It can be changed into "xen/device_tree: add parentheses around macro 
> parameters"
> 
>>
>> I will commit later on.
> 
> Do you want me to send a v4?

No need. It is now merged with the following commit message:

     xen/device_tree: add parentheses around macro parameters

     Add parentheses around macro parameters when the precedence and
     associativity of the performed operators can lead to unintended 
order of evaluation.

     This is fixing some ECLAIR finding for Misra Rule 20.7.

     Link: 
https://lore.kernel.org/xen-devel/20230203190908.211541-2-burzalodowa@gmail.com/
     Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
     Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
     [jgrall: Reworded the commit message]
     Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Xenia Ragiadakou]

On 2/8/23 10:38, Julien Grall wrote:
> 
> 
> On 07/02/2023 13:59, Xenia Ragiadakou wrote:
>> Hi Julien,
> 
> Hi Xenia,
> 
>>
>> On 2/7/23 15:01, Julien Grall wrote:
>>> Hi,
>>>
>>> On 07/02/2023 12:46, Xenia Ragiadakou wrote:
>>>> On 2/7/23 14:25, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 07/02/2023 10:46, Xenia Ragiadakou wrote:
>>>>>>
>>>>>> On 2/7/23 12:39, Julien Grall wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 07/02/2023 10:23, Luca Fancellu wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> On 3 Feb 2023, at 19:09, Xenia Ragiadakou 
>>>>>>>>> <burzalodowa@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>
>>>>>>>> I’m not really a supporter of empty commit message, but it’s up 
>>>>>>>> to the maintainer :)
>>>>>>>
>>>>>>> +1. In this case a brief summary of the rule would be handy for 
>>>>>>> those that are not well-versed with MISRA.
>>>>>>>
>>>>>>> This can be dealt on commit if you propose a new commit message.
>>>>>>
>>>>>> I 'm refrained from stating the rule as is because it is strict 
>>>>>> and it is not applied as is.
>>>>>
>>>>> I am a bit confused with this statement. From misra/..., we are 
>>>>> supporting rule 20.7. So why aren't applying it strictly?
>>>>>
>>>>> And if it is not applied as-is, shouldn't we document the violation 
>>>>> (if any)?
>>>>
>>>> I applied it strictly on v2, but there was no review.
>>>
>>> Ah! In general it is best to ping if there are no answers.
>>
>> Me too I ve forgotten about it. A ticket in gitlab reminded me that it 
>> was pending.
>>
>>>
>>>> Then Eclair was adjusted to have a less strict approach. Still there 
>>>> is a finding asking to add parentheses around dt in 
>>>> dt_for_each_device_node(dt, dn), i.e dn = (dt);, to which AFAIK you 
>>>> object.
>>>
>>> Are you referring to the discussion in [1]? If so, I wasn't totally 
>>> opposed to the suggestion so long we are consistent.
>>
>> I am not able to find [1].
> 
> https://lore.kernel.org/xen-devel/b2f2d1e7-0c18-206f-5e9d-d0115e398840@xen.org/
> 
> which is...
> 
> 
>> I 'm referring to the discussion in 
>> 20221220085100.22848-6-luca.fancellu@arm.com and 
>> 20220728134943.1185621-1-burzalodowa@gmail.com
> 
> ... a reply to this message. At the end of that reply, I said that I 
> wasn't totally against adding the parentheses but we should be 
> consistent in how we do it.

Ok, I must have got it wrong.

> 
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> "Add parentheses around macro parameters when the precedence and 
>>>>>> associativity of the performed operators can lead to unintended 
>>>>>> order of evaluation."
>>>>>>
>>>>>> Is this ok?
>>>>>
>>>>> I am OK with this. Is there any ID from Eclair that could be used 
>>>>> to track each error (and so we can confirm they have disappeared)?
>>>>
>>>> I am not aware of any.
>>> Hmmm ok. It might be a nice feature to suggest in Eclair because 
>>> anyone can check whether an issue was resolved.
>>
>> Currently, the violations in Eclair are reported per macro call (I 
>> guess because it is acceptable to have parentheses around the macro 
>> args) so it is difficult to track all of them.
>>
>>>
>>> Here, I don't exactly know what to check in Eclair. So I have to rely 
>>> on you. Anyway, nothing that can be fixed for this commit.
>>>
>>>>
>>>> The patch can be decoupled from misra and Eclair (I mean have a 
>>>> generic commit title) and just mention in the commit message that it 
>>>> fixes some Eclair findings for MISRA C rule 20.7.
>>>
>>> I have a slight preference for a more generic title. But the current 
>>> one also work for me.
>>
>> It can be changed into "xen/device_tree: add parentheses around macro 
>> parameters"
>>
>>>
>>> I will commit later on.
>>
>> Do you want me to send a v4?
> 
> No need. It is now merged with the following commit message:
> 
>      xen/device_tree: add parentheses around macro parameters
> 
>      Add parentheses around macro parameters when the precedence and
>      associativity of the performed operators can lead to unintended 
> order of evaluation.
> 
>      This is fixing some ECLAIR finding for Misra Rule 20.7.
> 
>      Link: 
> https://lore.kernel.org/xen-devel/20230203190908.211541-2-burzalodowa@gmail.com/
>      Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>      Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
>      [jgrall: Reworded the commit message]
>      Acked-by: Julien Grall <jgrall@amazon.com>

Thanks a lot.

> 
> Cheers,
> 

-- 
Xenia