Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1323872 > unrolled thread

[PATCH] EDAC, mpc85xx: fix build warning

Started bySudip Mukherjee <sudipm.mukherjee@gmail.com>
First post2016-02-02 09:10 +0100
Last post2016-02-02 15:50 +0100
Articles 10 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] EDAC, mpc85xx: fix build warning Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-02-02 09:10 +0100
    Re: [PATCH] EDAC, mpc85xx: fix build warning Borislav Petkov <bp@alien8.de> - 2016-02-02 12:20 +0100
      Re: [PATCH] EDAC, mpc85xx: fix build warning Guenter Roeck <linux@roeck-us.net> - 2016-02-02 15:40 +0100
        Re: [PATCH] EDAC, mpc85xx: fix build warning Johannes Thumshirn <jthumshirn@suse.de> - 2016-02-02 15:50 +0100
          Re: [PATCH] EDAC, mpc85xx: fix build warning Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-02-02 16:00 +0100
            Re: [PATCH] EDAC, mpc85xx: fix build warning Borislav Petkov <bp@alien8.de> - 2016-02-02 16:10 +0100
              Re: [PATCH] EDAC, mpc85xx: fix build warning Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-02-02 16:20 +0100
                Re: [PATCH] EDAC, mpc85xx: fix build warning Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-02-02 16:20 +0100
                Re: [PATCH] EDAC, mpc85xx: fix build warning Borislav Petkov <bp@alien8.de> - 2016-02-02 16:20 +0100
      Re: [PATCH] EDAC, mpc85xx: fix build warning Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-02-02 15:50 +0100

#1323872 — [PATCH] EDAC, mpc85xx: fix build warning

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2016-02-02 09:10 +0100
Subject[PATCH] EDAC, mpc85xx: fix build warning
Message-ID<qXEca-80M-21@gated-at.bofh.it>
We were getting build warning about:
drivers/edac/mpc85xx_edac.c:1247:6: warning: unused variable 'pvr'

pvr is only used if CONFIG_FSL_SOC_BOOKE was defined. Declare the
variable as a local variable inside the #ifdef block.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/edac/mpc85xx_edac.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
index b7139c1..951be71 100644
--- a/drivers/edac/mpc85xx_edac.c
+++ b/drivers/edac/mpc85xx_edac.c
@@ -1244,7 +1244,6 @@ static struct platform_driver * const drivers[] = {
 static int __init mpc85xx_mc_init(void)
 {
 	int res = 0;
-	u32 pvr = 0;
 
 	printk(KERN_INFO "Freescale(R) MPC85xx EDAC driver, "
 	       "(C) 2006 Montavista Software\n");
@@ -1264,16 +1263,18 @@ static int __init mpc85xx_mc_init(void)
 		printk(KERN_WARNING EDAC_MOD_STR "drivers fail to register\n");
 
 #ifdef CONFIG_FSL_SOC_BOOKE
-	pvr = mfspr(SPRN_PVR);
-
-	if ((PVR_VER(pvr) == PVR_VER_E500V1) ||
-	    (PVR_VER(pvr) == PVR_VER_E500V2)) {
-		/*
-		 * need to clear HID1[RFXE] to disable machine check int
-		 * so we can catch it
-		 */
-		if (edac_op_state == EDAC_OPSTATE_INT)
-			on_each_cpu(mpc85xx_mc_clear_rfxe, NULL, 0);
+	{
+		u32 pvr = mfspr(SPRN_PVR);
+
+		if ((PVR_VER(pvr) == PVR_VER_E500V1) ||
+		    (PVR_VER(pvr) == PVR_VER_E500V2)) {
+			/*
+			 * need to clear HID1[RFXE] to disable machine check int
+			 * so we can catch it
+			 */
+			if (edac_op_state == EDAC_OPSTATE_INT)
+				on_each_cpu(mpc85xx_mc_clear_rfxe, NULL, 0);
+		}
 	}
 #endif
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1323984

FromBorislav Petkov <bp@alien8.de>
Date2016-02-02 12:20 +0100
Message-ID<qXHa1-1VJ-1@gated-at.bofh.it>
In reply to#1323872
On Tue, Feb 02, 2016 at 01:30:21PM +0530, Sudip Mukherjee wrote:
> We were getting build warning about:
> drivers/edac/mpc85xx_edac.c:1247:6: warning: unused variable 'pvr'
> 
> pvr is only used if CONFIG_FSL_SOC_BOOKE was defined. Declare the
> variable as a local variable inside the #ifdef block.

What's wrong with doing the simpler thing:

---
diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
index b7139c160baf..f756b6215228 100644
--- a/drivers/edac/mpc85xx_edac.c
+++ b/drivers/edac/mpc85xx_edac.c
@@ -1244,7 +1244,6 @@ static struct platform_driver * const drivers[] = {
 static int __init mpc85xx_mc_init(void)
 {
 	int res = 0;
-	u32 pvr = 0;
 
 	printk(KERN_INFO "Freescale(R) MPC85xx EDAC driver, "
 	       "(C) 2006 Montavista Software\n");
@@ -1264,10 +1263,8 @@ static int __init mpc85xx_mc_init(void)
 		printk(KERN_WARNING EDAC_MOD_STR "drivers fail to register\n");
 
 #ifdef CONFIG_FSL_SOC_BOOKE
-	pvr = mfspr(SPRN_PVR);
-
-	if ((PVR_VER(pvr) == PVR_VER_E500V1) ||
-	    (PVR_VER(pvr) == PVR_VER_E500V2)) {
+	if ((PVR_VER(mfspr(SPRN_PVR)) == PVR_VER_E500V1) ||
+	    (PVR_VER(mfspr(SPRN_PVR)) == PVR_VER_E500V2)) {
 		/*
 		 * need to clear HID1[RFXE] to disable machine check int
 		 * so we can catch it
---

Granted, we get MFSPR issued twice by the compiler but that's the init
path.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1324138

FromGuenter Roeck <linux@roeck-us.net>
Date2016-02-02 15:40 +0100
Message-ID<qXKhB-4fv-39@gated-at.bofh.it>
In reply to#1323984
On Tue, Feb 02, 2016 at 12:15:45PM +0100, Borislav Petkov wrote:
> On Tue, Feb 02, 2016 at 01:30:21PM +0530, Sudip Mukherjee wrote:
> > We were getting build warning about:
> > drivers/edac/mpc85xx_edac.c:1247:6: warning: unused variable 'pvr'
> > 
> > pvr is only used if CONFIG_FSL_SOC_BOOKE was defined. Declare the
> > variable as a local variable inside the #ifdef block.
> 
> What's wrong with doing the simpler thing:
> 
> ---
> diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
> index b7139c160baf..f756b6215228 100644
> --- a/drivers/edac/mpc85xx_edac.c
> +++ b/drivers/edac/mpc85xx_edac.c
> @@ -1244,7 +1244,6 @@ static struct platform_driver * const drivers[] = {
>  static int __init mpc85xx_mc_init(void)
>  {
>  	int res = 0;
> -	u32 pvr = 0;
>  
Or with just adding __maybe_unused here.

Guenter

>  	printk(KERN_INFO "Freescale(R) MPC85xx EDAC driver, "
>  	       "(C) 2006 Montavista Software\n");
> @@ -1264,10 +1263,8 @@ static int __init mpc85xx_mc_init(void)
>  		printk(KERN_WARNING EDAC_MOD_STR "drivers fail to register\n");
>  
>  #ifdef CONFIG_FSL_SOC_BOOKE
> -	pvr = mfspr(SPRN_PVR);
> -
> -	if ((PVR_VER(pvr) == PVR_VER_E500V1) ||
> -	    (PVR_VER(pvr) == PVR_VER_E500V2)) {
> +	if ((PVR_VER(mfspr(SPRN_PVR)) == PVR_VER_E500V1) ||
> +	    (PVR_VER(mfspr(SPRN_PVR)) == PVR_VER_E500V2)) {
>  		/*
>  		 * need to clear HID1[RFXE] to disable machine check int
>  		 * so we can catch it
> ---
> 
> Granted, we get MFSPR issued twice by the compiler but that's the init
> path.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> ECO tip #101: Trim your mails when you reply.
> 
>

[toc] | [prev] | [next] | [standalone]

#1324144

FromJohannes Thumshirn <jthumshirn@suse.de>
Date2016-02-02 15:50 +0100
Message-ID<qXKrg-4kd-17@gated-at.bofh.it>
In reply to#1324138
On Tue, Feb 02, 2016 at 06:38:01AM -0800, Guenter Roeck wrote:
> On Tue, Feb 02, 2016 at 12:15:45PM +0100, Borislav Petkov wrote:
> > On Tue, Feb 02, 2016 at 01:30:21PM +0530, Sudip Mukherjee wrote:
> > > We were getting build warning about:
> > > drivers/edac/mpc85xx_edac.c:1247:6: warning: unused variable 'pvr'
> > > 
> > > pvr is only used if CONFIG_FSL_SOC_BOOKE was defined. Declare the
> > > variable as a local variable inside the #ifdef block.
> > 
> > What's wrong with doing the simpler thing:
> > 
> > ---
> > diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
> > index b7139c160baf..f756b6215228 100644
> > --- a/drivers/edac/mpc85xx_edac.c
> > +++ b/drivers/edac/mpc85xx_edac.c
> > @@ -1244,7 +1244,6 @@ static struct platform_driver * const drivers[] = {
> >  static int __init mpc85xx_mc_init(void)
> >  {
> >  	int res = 0;
> > -	u32 pvr = 0;
> >  
> Or with just adding __maybe_unused here.

I'd favour this one as well.

Just my 2c.

byte,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [next] | [standalone]

#1324175

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2016-02-02 16:00 +0100
Message-ID<qXKAW-4oM-17@gated-at.bofh.it>
In reply to#1324144
On Tue, Feb 02, 2016 at 03:48:03PM +0100, Johannes Thumshirn wrote:
> On Tue, Feb 02, 2016 at 06:38:01AM -0800, Guenter Roeck wrote:
> > On Tue, Feb 02, 2016 at 12:15:45PM +0100, Borislav Petkov wrote:
> > > On Tue, Feb 02, 2016 at 01:30:21PM +0530, Sudip Mukherjee wrote:
> > > > We were getting build warning about:
> > > > drivers/edac/mpc85xx_edac.c:1247:6: warning: unused variable 'pvr'
> > > > 
> > > > pvr is only used if CONFIG_FSL_SOC_BOOKE was defined. Declare the
> > > > variable as a local variable inside the #ifdef block.
> > > 
> > > What's wrong with doing the simpler thing:
> > > 
> > > ---
> > > diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
> > > index b7139c160baf..f756b6215228 100644
> > > --- a/drivers/edac/mpc85xx_edac.c
> > > +++ b/drivers/edac/mpc85xx_edac.c
> > > @@ -1244,7 +1244,6 @@ static struct platform_driver * const drivers[] = {
> > >  static int __init mpc85xx_mc_init(void)
> > >  {
> > >  	int res = 0;
> > > -	u32 pvr = 0;
> > >  
> > Or with just adding __maybe_unused here.
> 
> I'd favour this one as well.

oops... i just sent the v2 while removing pvr. Which is the better way
then?
Personally, I donot like __maybe_unused, it is telling gcc that this
variable is unused so donot give any warning for it. But in reality it
is being used in some cases.

regards
sudip

[toc] | [prev] | [next] | [standalone]

#1324181

FromBorislav Petkov <bp@alien8.de>
Date2016-02-02 16:10 +0100
Message-ID<qXKKB-4KV-11@gated-at.bofh.it>
In reply to#1324175
On Tue, Feb 02, 2016 at 08:25:54PM +0530, Sudip Mukherjee wrote:
> oops... i just sent the v2 while removing pvr. Which is the better way
> then?
> Personally, I donot like __maybe_unused, it is telling gcc that this
> variable is unused so donot give any warning for it. But in reality it
> is being used in some cases.

And? You want to shut up the warning, right?

Adding __maybe_unused is the simplest variant without disadvantages. Or
are there any?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1324187

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2016-02-02 16:20 +0100
Message-ID<qXKUh-4Qa-1@gated-at.bofh.it>
In reply to#1324181
On Tue, Feb 02, 2016 at 04:09:50PM +0100, Borislav Petkov wrote:
> On Tue, Feb 02, 2016 at 08:25:54PM +0530, Sudip Mukherjee wrote:
> > oops... i just sent the v2 while removing pvr. Which is the better way
> > then?
> > Personally, I donot like __maybe_unused, it is telling gcc that this
> > variable is unused so donot give any warning for it. But in reality it
> > is being used in some cases.
> 
> And? You want to shut up the warning, right?
> 
> Adding __maybe_unused is the simplest variant without disadvantages. Or
> are there any?

another way might be:

diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
index b7139c1..968c0c0 100644
--- a/drivers/edac/mpc85xx_edac.c
+++ b/drivers/edac/mpc85xx_edac.c
@@ -1244,7 +1244,9 @@ static struct platform_driver * const drivers[] = {
 static int __init mpc85xx_mc_init(void)
 {
 	int res = 0;
+#ifdef CONFIG_FSL_SOC_BOOKE
 	u32 pvr = 0;
+#endif
 
 	printk(KERN_INFO "Freescale(R) MPC85xx EDAC driver, "
 	       "(C) 2006 Montavista Software\n");

regards
sudip

[toc] | [prev] | [next] | [standalone]

#1324192

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2016-02-02 16:20 +0100
Message-ID<qXKUi-4Qa-23@gated-at.bofh.it>
In reply to#1324187
On Tue, Feb 02, 2016 at 04:15:43PM +0100, Borislav Petkov wrote:
> On Tue, Feb 02, 2016 at 08:43:57PM +0530, Sudip Mukherjee wrote:
> > another way might be:
> > 
> > diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
> > index b7139c1..968c0c0 100644
> > --- a/drivers/edac/mpc85xx_edac.c
> > +++ b/drivers/edac/mpc85xx_edac.c
> > @@ -1244,7 +1244,9 @@ static struct platform_driver * const drivers[] = {
> >  static int __init mpc85xx_mc_init(void)
> >  {
> >  	int res = 0;
> > +#ifdef CONFIG_FSL_SOC_BOOKE
> >  	u32 pvr = 0;
> > +#endif
> 
> Nah, this is the ugliest of them all. We want less ifdeffery, not more.

ok, i am resending with  __maybe_unused

regards
sudip

[toc] | [prev] | [next] | [standalone]

#1324193

FromBorislav Petkov <bp@alien8.de>
Date2016-02-02 16:20 +0100
Message-ID<qXKUi-4Qa-21@gated-at.bofh.it>
In reply to#1324187
On Tue, Feb 02, 2016 at 08:43:57PM +0530, Sudip Mukherjee wrote:
> another way might be:
> 
> diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
> index b7139c1..968c0c0 100644
> --- a/drivers/edac/mpc85xx_edac.c
> +++ b/drivers/edac/mpc85xx_edac.c
> @@ -1244,7 +1244,9 @@ static struct platform_driver * const drivers[] = {
>  static int __init mpc85xx_mc_init(void)
>  {
>  	int res = 0;
> +#ifdef CONFIG_FSL_SOC_BOOKE
>  	u32 pvr = 0;
> +#endif

Nah, this is the ugliest of them all. We want less ifdeffery, not more.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1324154

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2016-02-02 15:50 +0100
Message-ID<qXKrh-4kd-41@gated-at.bofh.it>
In reply to#1323984
On Tue, Feb 02, 2016 at 12:15:45PM +0100, Borislav Petkov wrote:
> On Tue, Feb 02, 2016 at 01:30:21PM +0530, Sudip Mukherjee wrote:
> > We were getting build warning about:
> > drivers/edac/mpc85xx_edac.c:1247:6: warning: unused variable 'pvr'
> > 
> > pvr is only used if CONFIG_FSL_SOC_BOOKE was defined. Declare the
> > variable as a local variable inside the #ifdef block.
> 
> What's wrong with doing the simpler thing:

I hesitated because of issuing MFSPR twice. But anyway, I have sent the
v2 for your review.

regards
sudip

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web