[PATCH] aic79xx: fix misuse of static variables

[PATCH] aic79xx: fix misuse of static variables

[Mathias Krause]

The format strings for various printk()s make use of a temporary
variable that is declared 'static'. This is probably not intended,
so fix those.

Found in the PaX patch, written by the PaX Team.

Cc: PaX Team <pageexec@freemail.hu>
Cc: Hannes Reinecke <hare@suse.de>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Signed-off-by: Mathias Krause <minipli@googlemail.com>
---

Remark: Compile tested only! I've no such hardware.

 drivers/scsi/aic7xxx/aic79xx_pci.c |   18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aic79xx_pci.c b/drivers/scsi/aic7xxx/aic79xx_pci.c
index 14b5f8d0e7..cc9bd26f5d 100644
--- a/drivers/scsi/aic7xxx/aic79xx_pci.c
+++ b/drivers/scsi/aic7xxx/aic79xx_pci.c
@@ -827,7 +827,7 @@ ahd_pci_intr(struct ahd_softc *ahd)
 		for (bit = 0; bit < 8; bit++) {
 
 			if ((pci_status[i] & (0x1 << bit)) != 0) {
-				static const char *s;
+				const char *s;
 
 				s = pci_status_strings[bit];
 				if (i == 7/*TARG*/ && bit == 3)
@@ -887,23 +887,15 @@ ahd_pci_split_intr(struct ahd_softc *ahd, u_int intstat)
 
 		for (bit = 0; bit < 8; bit++) {
 
-			if ((split_status[i] & (0x1 << bit)) != 0) {
-				static const char *s;
-
-				s = split_status_strings[bit];
-				printk(s, ahd_name(ahd),
+			if ((split_status[i] & (0x1 << bit)) != 0)
+				printk(split_status_strings[bit], ahd_name(ahd),
 				       split_status_source[i]);
-			}
 
 			if (i > 1)
 				continue;
 
-			if ((sg_split_status[i] & (0x1 << bit)) != 0) {
-				static const char *s;
-
-				s = split_status_strings[bit];
-				printk(s, ahd_name(ahd), "SG");
-			}
+			if ((sg_split_status[i] & (0x1 << bit)) != 0)
+				printk(split_status_strings[bit], ahd_name(ahd), "SG");
 		}
 	}
 	/*
-- 
1.7.10.4

Re: [PATCH] aic79xx: fix misuse of static variables

[Hannes Reinecke]

On 03/30/2014 03:30 PM, Mathias Krause wrote:
> The format strings for various printk()s make use of a temporary
> variable that is declared 'static'. This is probably not intended,
> so fix those.
> 
> Found in the PaX patch, written by the PaX Team.
> 
> Cc: PaX Team <pageexec@freemail.hu>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Signed-off-by: Mathias Krause <minipli@googlemail.com>
> ---
> 
> Remark: Compile tested only! I've no such hardware.
> 
>  drivers/scsi/aic7xxx/aic79xx_pci.c |   18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/scsi/aic7xxx/aic79xx_pci.c b/drivers/scsi/aic7xxx/aic79xx_pci.c
> index 14b5f8d0e7..cc9bd26f5d 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_pci.c
> +++ b/drivers/scsi/aic7xxx/aic79xx_pci.c
> @@ -827,7 +827,7 @@ ahd_pci_intr(struct ahd_softc *ahd)
>  		for (bit = 0; bit < 8; bit++) {
>  
>  			if ((pci_status[i] & (0x1 << bit)) != 0) {
> -				static const char *s;
> +				const char *s;
>  
>  				s = pci_status_strings[bit];
>  				if (i == 7/*TARG*/ && bit == 3)
> @@ -887,23 +887,15 @@ ahd_pci_split_intr(struct ahd_softc *ahd, u_int intstat)
>  
>  		for (bit = 0; bit < 8; bit++) {
>  
> -			if ((split_status[i] & (0x1 << bit)) != 0) {
> -				static const char *s;
> -
> -				s = split_status_strings[bit];
> -				printk(s, ahd_name(ahd),
> +			if ((split_status[i] & (0x1 << bit)) != 0)
> +				printk(split_status_strings[bit], ahd_name(ahd),
>  				       split_status_source[i]);
> -			}
>  
>  			if (i > 1)
>  				continue;
>  
> -			if ((sg_split_status[i] & (0x1 << bit)) != 0) {
> -				static const char *s;
> -
> -				s = split_status_strings[bit];
> -				printk(s, ahd_name(ahd), "SG");
> -			}
> +			if ((sg_split_status[i] & (0x1 << bit)) != 0)
> +				printk(split_status_strings[bit], ahd_name(ahd), "SG");
>  		}
>  	}
>  	/*
> 
Looks good.

Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] aic79xx: fix misuse of static variables

[James Bottomley]

On Sun, 2014-03-30 at 15:30 +0200, Mathias Krause wrote:
> The format strings for various printk()s make use of a temporary
> variable that is declared 'static'. This is probably not intended,
> so fix those.

Actually, it was intended.  It was to work around an assignment to const
gcc bug: some versions of gcc won't allow the first assignment to an
uninitialised const.  I can't remember how long ago this was, but
probably at least 10 years, so it's likely whatever version of gcc that
caused the problem is long gone, but someone should check this
modification works on our earliest supported version.

James

Re: [PATCH] aic79xx: fix misuse of static variables

[Mathias Krause]

On 1 April 2014 16:10, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Sun, 2014-03-30 at 15:30 +0200, Mathias Krause wrote:
>> The format strings for various printk()s make use of a temporary
>> variable that is declared 'static'. This is probably not intended,
>> so fix those.
>
> Actually, it was intended.  It was to work around an assignment to const
> gcc bug: some versions of gcc won't allow the first assignment to an
> uninitialised const.

Could it be you're mixing up 'const char *' with 'char *const'? The
latter would be the wrong type and would generate an error when tying
to be assigned after initialization. But the former is totally fine.
In fact, it is fine with ancient versions of gcc as well. The oldest
one I managed to get running on my system is gcc 2.7.2.3 from 1998. It
compiled this little example just fine:

,--[ test.c ]---
| static const char *strings[] = {
|   "foo", "bar%s", "ba%z"
| };
|
| int main(int argc, char **argv) {
|   const char *s;
|
|   s = strings[argc];
|   printf(s, "got ya!");
|   putchar('\n');
|
|   return 0;
| }
`---

The same is true for gcc 3.2 -- the oldest supported version according
to README. It compiles the above example just fine.

>  I can't remember how long ago this was, but
> probably at least 10 years, so it's likely whatever version of gcc that
> caused the problem is long gone, but someone should check this
> modification works on our earliest supported version.

I haven't tried compiling the kernel with those old compilers -- I've
installed them in a very basic chroot environment only -- but as this
is a very common used pattern, pretty much the rest of the kernel
wouldn't compile either if gcc would require quirks like this. It's
standard C, after all.


Thanks,
Mathias

Re: [PATCH] aic79xx: fix misuse of static variables

[Mathias Krause]

On 30 March 2014 15:30, Mathias Krause <minipli@googlemail.com> wrote:
> The format strings for various printk()s make use of a temporary
> variable that is declared 'static'. This is probably not intended,
> so fix those.
>
> Found in the PaX patch, written by the PaX Team.
>
> Cc: PaX Team <pageexec@freemail.hu>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Signed-off-by: Mathias Krause <minipli@googlemail.com>
> ---
>
> Remark: Compile tested only! I've no such hardware.
>
>  drivers/scsi/aic7xxx/aic79xx_pci.c |   18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/scsi/aic7xxx/aic79xx_pci.c b/drivers/scsi/aic7xxx/aic79xx_pci.c
> index 14b5f8d0e7..cc9bd26f5d 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_pci.c
> +++ b/drivers/scsi/aic7xxx/aic79xx_pci.c
> @@ -827,7 +827,7 @@ ahd_pci_intr(struct ahd_softc *ahd)
>                 for (bit = 0; bit < 8; bit++) {
>
>                         if ((pci_status[i] & (0x1 << bit)) != 0) {
> -                               static const char *s;
> +                               const char *s;
>
>                                 s = pci_status_strings[bit];
>                                 if (i == 7/*TARG*/ && bit == 3)
> @@ -887,23 +887,15 @@ ahd_pci_split_intr(struct ahd_softc *ahd, u_int intstat)
>
>                 for (bit = 0; bit < 8; bit++) {
>
> -                       if ((split_status[i] & (0x1 << bit)) != 0) {
> -                               static const char *s;
> -
> -                               s = split_status_strings[bit];
> -                               printk(s, ahd_name(ahd),
> +                       if ((split_status[i] & (0x1 << bit)) != 0)
> +                               printk(split_status_strings[bit], ahd_name(ahd),
>                                        split_status_source[i]);
> -                       }
>
>                         if (i > 1)
>                                 continue;
>
> -                       if ((sg_split_status[i] & (0x1 << bit)) != 0) {
> -                               static const char *s;
> -
> -                               s = split_status_strings[bit];
> -                               printk(s, ahd_name(ahd), "SG");
> -                       }
> +                       if ((sg_split_status[i] & (0x1 << bit)) != 0)
> +                               printk(split_status_strings[bit], ahd_name(ahd), "SG");
>                 }
>         }
>         /*
> --
> 1.7.10.4
>

Ping? James, Hannes? How to proceed with this patch?

Mathias