[PATCH] cxl: Fix ambiguous else warnings

[PATCH] cxl: Fix ambiguous else warnings

[Daniel Axtens]

Every time I build cxl I see the following warnings:

/scratch/dja/linux-capi/drivers/misc/cxl/pci.c: In function ‘sanitise_afu_regs’:
/scratch/dja/linux-capi/drivers/misc/cxl/pci.c:712:6: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
   if (reg & CXL_PSL_DSISR_TRANS)
      ^
/scratch/dja/linux-capi/drivers/misc/cxl/irq.c: In function ‘fail_psl_irq’:
/scratch/dja/linux-capi/drivers/misc/cxl/irq.c:184:5: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
  if (irq_info->dsisr & CXL_PSL_DSISR_TRANS)
     ^

They're annoying. Fix them by inserting the braces.

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 drivers/misc/cxl/irq.c | 5 +++--
 drivers/misc/cxl/pci.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/cxl/irq.c b/drivers/misc/cxl/irq.c
index 121ec48f3ab4..c11e583a15a9 100644
--- a/drivers/misc/cxl/irq.c
+++ b/drivers/misc/cxl/irq.c
@@ -181,10 +181,11 @@ static irqreturn_t cxl_irq(int irq, void *data, struct cxl_irq_info *irq_info)
 
 static irqreturn_t fail_psl_irq(struct cxl_afu *afu, struct cxl_irq_info *irq_info)
 {
-	if (irq_info->dsisr & CXL_PSL_DSISR_TRANS)
+	if (irq_info->dsisr & CXL_PSL_DSISR_TRANS) {
 		cxl_p2n_write(afu, CXL_PSL_TFC_An, CXL_PSL_TFC_An_AE);
-	else
+	} else {
 		cxl_p2n_write(afu, CXL_PSL_TFC_An, CXL_PSL_TFC_An_A);
+	}
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 6d2e538331b6..29c7e4eb309a 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -709,10 +709,11 @@ static int sanitise_afu_regs(struct cxl_afu *afu)
 	reg = cxl_p2n_read(afu, CXL_PSL_DSISR_An);
 	if (reg) {
 		dev_warn(&afu->dev, "AFU had pending DSISR: %#.16llx\n", reg);
-		if (reg & CXL_PSL_DSISR_TRANS)
+		if (reg & CXL_PSL_DSISR_TRANS) {
 			cxl_p2n_write(afu, CXL_PSL_TFC_An, CXL_PSL_TFC_An_AE);
-		else
+		} else {
 			cxl_p2n_write(afu, CXL_PSL_TFC_An, CXL_PSL_TFC_An_A);
+		}
 	}
 	reg = cxl_p1n_read(afu, CXL_PSL_SERR_An);
 	if (reg) {
-- 
2.1.4

Re: [PATCH] cxl: Fix ambiguous else warnings

[Andreas Schwab]

Daniel Axtens <dja@axtens.net> writes:

> Every time I build cxl I see the following warnings:
>
> /scratch/dja/linux-capi/drivers/misc/cxl/pci.c: In function ‘sanitise_afu_regs’:
> /scratch/dja/linux-capi/drivers/misc/cxl/pci.c:712:6: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
>    if (reg & CXL_PSL_DSISR_TRANS)
>       ^
> /scratch/dja/linux-capi/drivers/misc/cxl/irq.c: In function ‘fail_psl_irq’:
> /scratch/dja/linux-capi/drivers/misc/cxl/irq.c:184:5: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
>   if (irq_info->dsisr & CXL_PSL_DSISR_TRANS)
>      ^

Why are they ambigous?  Why doesn't cxl_p2n_write(afu, CXL_PSL_TFC_An,
CXL_PSL_TFC_An_AE) expand to a proper statement?

#define cxl_p2n_write(afu, reg, val) \
	out_be64(_cxl_p2n_addr(afu, reg), val)

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] cxl: Fix ambiguous else warnings

[Thomas Huth]

On 31/07/15 08:35, Daniel Axtens wrote:
> Every time I build cxl I see the following warnings:
> 
> /scratch/dja/linux-capi/drivers/misc/cxl/pci.c: In function ‘sanitise_afu_regs’:
> /scratch/dja/linux-capi/drivers/misc/cxl/pci.c:712:6: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
>    if (reg & CXL_PSL_DSISR_TRANS)
>       ^
> /scratch/dja/linux-capi/drivers/misc/cxl/irq.c: In function ‘fail_psl_irq’:
> /scratch/dja/linux-capi/drivers/misc/cxl/irq.c:184:5: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
>   if (irq_info->dsisr & CXL_PSL_DSISR_TRANS)
>      ^
> 
> They're annoying. Fix them by inserting the braces.
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  drivers/misc/cxl/irq.c | 5 +++--
>  drivers/misc/cxl/pci.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/cxl/irq.c b/drivers/misc/cxl/irq.c
> index 121ec48f3ab4..c11e583a15a9 100644
> --- a/drivers/misc/cxl/irq.c
> +++ b/drivers/misc/cxl/irq.c
> @@ -181,10 +181,11 @@ static irqreturn_t cxl_irq(int irq, void *data, struct cxl_irq_info *irq_info)
>  
>  static irqreturn_t fail_psl_irq(struct cxl_afu *afu, struct cxl_irq_info *irq_info)
>  {
> -	if (irq_info->dsisr & CXL_PSL_DSISR_TRANS)
> +	if (irq_info->dsisr & CXL_PSL_DSISR_TRANS) {
>  		cxl_p2n_write(afu, CXL_PSL_TFC_An, CXL_PSL_TFC_An_AE);
> -	else
> +	} else {
>  		cxl_p2n_write(afu, CXL_PSL_TFC_An, CXL_PSL_TFC_An_A);
> +	}
>  
>  	return IRQ_HANDLED;
>  }

Why does your GCC complain here at all? The code doesn't look ambiguous
to me... Which version of GCC are you using?
I just did a quick test compile here, and these warnings do not occur
for me.

 Thomas

Re: [PATCH] cxl: Fix ambiguous else warnings

[Daniel Axtens]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

Hi Andreas,

On Fri, 2015-07-31 at 11:16 +0200, Andreas Schwab wrote:
> Daniel Axtens <dja@axtens.net> writes:
> 
> > Every time I build cxl I see the following warnings:
> >
> > /scratch/dja/linux-capi/drivers/misc/cxl/pci.c: In function ‘sanitise_afu_regs’:
> > /scratch/dja/linux-capi/drivers/misc/cxl/pci.c:712:6: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
> >    if (reg & CXL_PSL_DSISR_TRANS)
> >       ^
> > /scratch/dja/linux-capi/drivers/misc/cxl/irq.c: In function ‘fail_psl_irq’:
> > /scratch/dja/linux-capi/drivers/misc/cxl/irq.c:184:5: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
> >   if (irq_info->dsisr & CXL_PSL_DSISR_TRANS)
> >      ^
> 
> Why are they ambigous?  Why doesn't cxl_p2n_write(afu, CXL_PSL_TFC_An,
> CXL_PSL_TFC_An_AE) expand to a proper statement?
> 
> #define cxl_p2n_write(afu, reg, val) \
> 	out_be64(_cxl_p2n_addr(afu, reg), val)
> 

I realised that I started seeing this when I was working on my EEH
patches, which change the definition to:

#define cxl_p2n_write(afu, reg, val) \
	if (cxl_adapter_link_ok(afu->adapter)) \
		out_be64(_cxl_p2n_addr(afu, reg), val)


I'll redo the patch to change the write functions to wrap the if
statement so they cease to be ambiguous. I'll also spin a patch to
enable -Werror so I catch this earlier next time.

Thanks for the feedback, it was very helpful in me figuring out the root
cause.

-- 
Regards,
Daniel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]

Re: [PATCH] cxl: Fix ambiguous else warnings

[Michael Ellerman]

On Mon, 2015-08-03 at 10:36 +1000, Daniel Axtens wrote:
> Hi Andreas,
> 
> On Fri, 2015-07-31 at 11:16 +0200, Andreas Schwab wrote:
> > Daniel Axtens <dja@axtens.net> writes:
> > 
> > > Every time I build cxl I see the following warnings:
> > >
> > > /scratch/dja/linux-capi/drivers/misc/cxl/pci.c: In function ‘sanitise_afu_regs’:
> > > /scratch/dja/linux-capi/drivers/misc/cxl/pci.c:712:6: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
> > >    if (reg & CXL_PSL_DSISR_TRANS)
> > >       ^
> > > /scratch/dja/linux-capi/drivers/misc/cxl/irq.c: In function ‘fail_psl_irq’:
> > > /scratch/dja/linux-capi/drivers/misc/cxl/irq.c:184:5: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
> > >   if (irq_info->dsisr & CXL_PSL_DSISR_TRANS)
> > >      ^
> > 
> > Why are they ambigous?  Why doesn't cxl_p2n_write(afu, CXL_PSL_TFC_An,
> > CXL_PSL_TFC_An_AE) expand to a proper statement?
> > 
> > #define cxl_p2n_write(afu, reg, val) \
> > 	out_be64(_cxl_p2n_addr(afu, reg), val)
> > 
> 
> I realised that I started seeing this when I was working on my EEH
> patches, which change the definition to:
> 
> #define cxl_p2n_write(afu, reg, val) \
> 	if (cxl_adapter_link_ok(afu->adapter)) \
> 		out_be64(_cxl_p2n_addr(afu, reg), val)

This should be a static inline, not a #define.

cheers

Re: [PATCH] cxl: Fix ambiguous else warnings

[Daniel Axtens]

[-- Attachment #1: Type: text/plain, Size: 197 bytes --]

> This should be a static inline, not a #define.
> 
I'm changing them to static inlines at the moment. Would you prefer a v3
of the EEH patches, or a new patch on top?

-- 
Regards,
Daniel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]

Re: [PATCH] cxl: Fix ambiguous else warnings

[Michael Ellerman]

On Mon, 2015-08-03 at 10:47 +1000, Daniel Axtens wrote:
> > This should be a static inline, not a #define.
> > 
> I'm changing them to static inlines at the moment. Would you prefer a v3
> of the EEH patches, or a new patch on top?

Without looking closer I think a v3.

ie. before you change the logic of the macro to include an if, it should become
a static inline.

cheers