The macro parameter 'off' is used as an expression and it is good to be
enclosed in parentheses to prevent against unintended expansion.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/drivers/char/imx-lpuart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/imx-lpuart.c b/xen/drivers/char/imx-lpuart.c
index 2709136081..9c1f3b71a3 100644
--- a/xen/drivers/char/imx-lpuart.c
+++ b/xen/drivers/char/imx-lpuart.c
@@ -26,8 +26,8 @@
 #include <asm/imx-lpuart.h>
 #include <asm/io.h>
 
-#define imx_lpuart_read(uart, off)       readl((uart)->regs + off)
-#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + off)
+#define imx_lpuart_read(uart, off)       readl((uart)->regs + (off))
+#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + (off))
 
 static struct imx_lpuart {
     uint32_t baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
-- 
2.34.1

On 02.08.2022 09:54, Xenia Ragiadakou wrote:
> --- a/xen/drivers/char/imx-lpuart.c
> +++ b/xen/drivers/char/imx-lpuart.c
> @@ -26,8 +26,8 @@
>  #include <asm/imx-lpuart.h>
>  #include <asm/io.h>
>  
> -#define imx_lpuart_read(uart, off)       readl((uart)->regs + off)
> -#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + off)
> +#define imx_lpuart_read(uart, off)       readl((uart)->regs + (off))
> +#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + (off))

As elsewhere before I think at the same time you want to drop the
parentheses from the single use of "val".

Jan

Hi Jan,

On 8/2/22 14:58, Jan Beulich wrote:
> On 02.08.2022 09:54, Xenia Ragiadakou wrote:
>> --- a/xen/drivers/char/imx-lpuart.c
>> +++ b/xen/drivers/char/imx-lpuart.c
>> @@ -26,8 +26,8 @@
>>   #include <asm/imx-lpuart.h>
>>   #include <asm/io.h>
>>   
>> -#define imx_lpuart_read(uart, off)       readl((uart)->regs + off)
>> -#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + off)
>> +#define imx_lpuart_read(uart, off)       readl((uart)->regs + (off))
>> +#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + (off))
> 
> As elsewhere before I think at the same time you want to drop the
> parentheses from the single use of "val".
> 

In general I do not want to include irrelevant changes in my patches.
Also, here, I do not want to drop the parentheses from val because the 
removal of the parentheses
- consists a violation of the rule 20.7
- would allow the following to compile
#define VAL x, y, z);(
imx_lpuart_write(uart, off, VAL)
- is not justifiable (i.e does not fix a bug, does not result in more 
readable code etc)

I understand that the rest of the community does not agree with adding 
parentheses around macro parameters used as function arguments and as 
the right-side operand of the assignment operator, but I consider them 
useful and I do not want to remove them myself. Maybe somebody else from 
the community could do that.

Also, these exceptions would be good to be mentioned in the rules.rst. 
So, that other contributors do not try to fix relevant warnings.

-- 
Xenia

On 02.08.2022 14:40, Xenia Ragiadakou wrote:
> On 8/2/22 14:58, Jan Beulich wrote:
>> On 02.08.2022 09:54, Xenia Ragiadakou wrote:
>>> --- a/xen/drivers/char/imx-lpuart.c
>>> +++ b/xen/drivers/char/imx-lpuart.c
>>> @@ -26,8 +26,8 @@
>>>   #include <asm/imx-lpuart.h>
>>>   #include <asm/io.h>
>>>   
>>> -#define imx_lpuart_read(uart, off)       readl((uart)->regs + off)
>>> -#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + off)
>>> +#define imx_lpuart_read(uart, off)       readl((uart)->regs + (off))
>>> +#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + (off))
>>
>> As elsewhere before I think at the same time you want to drop the
>> parentheses from the single use of "val".
>>
> 
> In general I do not want to include irrelevant changes in my patches.
> Also, here, I do not want to drop the parentheses from val because the 
> removal of the parentheses
> - consists a violation of the rule 20.7
> - would allow the following to compile
> #define VAL x, y, z);(
> imx_lpuart_write(uart, off, VAL)

Parenthesization won't help against all forms of odd use of parentheses
in macro expansions anyway. Maybe MISRA should (or even does) have a
rule disallowing unbalanced parentheses (an square brackets) in macro
expansions ...

> - is not justifiable (i.e does not fix a bug, does not result in more 
> readable code etc)

As said before, I very much view too many parentheses as affecting
readability.

> I understand that the rest of the community does not agree with adding 
> parentheses around macro parameters used as function arguments

This may indeed be the case, while ...

> and as 
> the right-side operand of the assignment operator,

... iirc for this one it was only Julien so far who disliked the
addition of parentheses.

> but I consider them 
> useful and I do not want to remove them myself. Maybe somebody else from 
> the community could do that.

Fair enough; my remark was indeed _only_ a remark - the maintainers
will have to judge in the end.

Jan

On Tue, 2 Aug 2022, Jan Beulich wrote:
> On 02.08.2022 14:40, Xenia Ragiadakou wrote:
> > On 8/2/22 14:58, Jan Beulich wrote:
> >> On 02.08.2022 09:54, Xenia Ragiadakou wrote:
> >>> --- a/xen/drivers/char/imx-lpuart.c
> >>> +++ b/xen/drivers/char/imx-lpuart.c
> >>> @@ -26,8 +26,8 @@
> >>>   #include <asm/imx-lpuart.h>
> >>>   #include <asm/io.h>
> >>>   
> >>> -#define imx_lpuart_read(uart, off)       readl((uart)->regs + off)
> >>> -#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + off)
> >>> +#define imx_lpuart_read(uart, off)       readl((uart)->regs + (off))
> >>> +#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + (off))
> >>
> >> As elsewhere before I think at the same time you want to drop the
> >> parentheses from the single use of "val".
> >>
> > 
> > In general I do not want to include irrelevant changes in my patches.
> > Also, here, I do not want to drop the parentheses from val because the 
> > removal of the parentheses
> > - consists a violation of the rule 20.7
> > - would allow the following to compile
> > #define VAL x, y, z);(
> > imx_lpuart_write(uart, off, VAL)
> 
> Parenthesization won't help against all forms of odd use of parentheses
> in macro expansions anyway. Maybe MISRA should (or even does) have a
> rule disallowing unbalanced parentheses (an square brackets) in macro
> expansions ...
> 
> > - is not justifiable (i.e does not fix a bug, does not result in more 
> > readable code etc)
> 
> As said before, I very much view too many parentheses as affecting
> readability.

This patch is correct and it fixes the issue which is meant to fix.

Dropping the parentheses from the single use of "val" is not something
currently covered by our coding style document or by MISRA, so it is
normal that we are going to get a variety of opinions and preferences on
it.

I think it is better to avoid asking for changes not currently in
CODING_STYLE and docs/misra. It is less work for both reviewers and
contributors to add the rule to the coding style first, then ask for
changes.

So my preference is to keep this patch as is (regardless of what we are
going to do with "val"):

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


But I am happy to add the "avoid too many parenthesis" point to the list
of coding style things to discuss. FYI I paused the MISRA C discussion
meetings so that we can make some progress on fixing violations, but I
plan to resume those meetings in a month or two.

On 03.08.2022 00:58, Stefano Stabellini wrote:
> I think it is better to avoid asking for changes not currently in
> CODING_STYLE and docs/misra. It is less work for both reviewers and
> contributors to add the rule to the coding style first, then ask for
> changes.

I very specifically disagree with this statement: Attempts to add
text there have been ignored altogether, i.e. have not even been
seen worth a comment against the clarification and/or addition. I
have therefore given up to propose changes to this document, and
I'm also not going to suggest to anyone to make any attempt up and
until I see movement on the adjustment proposals already pending
(the two of them that I can easily locate for now over 2 years,
and iirc there were more which predate our switching of email
systems and which hence I wouldn't have in my outbox anymore).

Jan