[PATCH] aacraid: don't assign cpu_to_le32(constant) to u8

[PATCH] aacraid: don't assign cpu_to_le32(constant) to u8

[Stephen Rothwell]

Noticed on PowerPC allmod config build:

drivers/scsi/aacraid/commsup.c:1342: warning: large integer implicitly truncated to unsigned type
drivers/scsi/aacraid/commsup.c:1343: warning: large integer implicitly truncated to unsigned type
drivers/scsi/aacraid/commsup.c:1344: warning: large integer implicitly truncated to unsigned type

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 drivers/scsi/aacraid/commsup.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 240a0bb..b9682a8 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1339,9 +1339,9 @@ int aac_check_health(struct aac_dev * aac)
 			aif = (struct aac_aifcmd *)hw_fib->data;
 			aif->command = cpu_to_le32(AifCmdEventNotify);
 		 	aif->seqnum = cpu_to_le32(0xFFFFFFFF);
-		 	aif->data[0] = cpu_to_le32(AifEnExpEvent);
-			aif->data[1] = cpu_to_le32(AifExeFirmwarePanic);
-		 	aif->data[2] = cpu_to_le32(AifHighPriority);
+		 	aif->data[0] = AifEnExpEvent;
+			aif->data[1] = AifExeFirmwarePanic;
+		 	aif->data[2] = AifHighPriority;
 			aif->data[3] = cpu_to_le32(BlinkLED);
 
 			/*
-- 
1.5.3.4

RE: [PATCH] aacraid: don't assign cpu_to_le32(constant) to u8

[Salyzyn, Mark]

ACK

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Stephen Rothwell [mailto:sfr@canb.auug.org.au] 
> Sent: Wednesday, October 31, 2007 12:02 AM
> To: AACRAID
> Cc: linux-scsi@vger.kernel.org; LKML
> Subject: [PATCH] aacraid: don't assign cpu_to_le32(constant) to u8
> 
> Noticed on PowerPC allmod config build:
> 
> drivers/scsi/aacraid/commsup.c:1342: warning: large integer 
> implicitly truncated to unsigned type
> drivers/scsi/aacraid/commsup.c:1343: warning: large integer 
> implicitly truncated to unsigned type
> drivers/scsi/aacraid/commsup.c:1344: warning: large integer 
> implicitly truncated to unsigned type
> 
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
>  drivers/scsi/aacraid/commsup.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> -- 
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au
> 
> diff --git a/drivers/scsi/aacraid/commsup.c 
> b/drivers/scsi/aacraid/commsup.c
> index 240a0bb..b9682a8 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -1339,9 +1339,9 @@ int aac_check_health(struct aac_dev * aac)
>  			aif = (struct aac_aifcmd *)hw_fib->data;
>  			aif->command = cpu_to_le32(AifCmdEventNotify);
>  		 	aif->seqnum = cpu_to_le32(0xFFFFFFFF);
> -		 	aif->data[0] = cpu_to_le32(AifEnExpEvent);
> -			aif->data[1] = cpu_to_le32(AifExeFirmwarePanic);
> -		 	aif->data[2] = cpu_to_le32(AifHighPriority);
> +		 	aif->data[0] = AifEnExpEvent;
> +			aif->data[1] = AifExeFirmwarePanic;
> +		 	aif->data[2] = AifHighPriority;
>  			aif->data[3] = cpu_to_le32(BlinkLED);
>  
>  			/*
> -- 
> 1.5.3.4
> 

[PATCHv2] aacraid: don't assign cpu_to_le32(constant) to u8

[Stephen Rothwell]

Noticed on PowerPC allmod config build:

drivers/scsi/aacraid/commsup.c:1342: warning: large integer implicitly truncated to unsigned type
drivers/scsi/aacraid/commsup.c:1343: warning: large integer implicitly truncated to unsigned type
drivers/scsi/aacraid/commsup.c:1344: warning: large integer implicitly truncated to unsigned type

Also fix some whitespace on the changed lines.

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 drivers/scsi/aacraid/commsup.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

This version just fixes a couple of whitespace anomolies on the lines I
changed.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 240a0bb..3c2dbc0 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1339,9 +1339,9 @@ int aac_check_health(struct aac_dev * aac)
 			aif = (struct aac_aifcmd *)hw_fib->data;
 			aif->command = cpu_to_le32(AifCmdEventNotify);
 		 	aif->seqnum = cpu_to_le32(0xFFFFFFFF);
-		 	aif->data[0] = cpu_to_le32(AifEnExpEvent);
-			aif->data[1] = cpu_to_le32(AifExeFirmwarePanic);
-		 	aif->data[2] = cpu_to_le32(AifHighPriority);
+			aif->data[0] = AifEnExpEvent;
+			aif->data[1] = AifExeFirmwarePanic;
+			aif->data[2] = AifHighPriority;
 			aif->data[3] = cpu_to_le32(BlinkLED);
 
 			/*
-- 
1.5.3.4

RE: [PATCHv2] aacraid: don't assign cpu_to_le32(constant) to u8

[Salyzyn, Mark]

ACK v2

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Stephen Rothwell [mailto:sfr@canb.auug.org.au] 
> Sent: Thursday, November 01, 2007 2:32 AM
> To: AACRAID
> Cc: linux-scsi@vger.kernel.org; LKML
> Subject: [PATCHv2] aacraid: don't assign cpu_to_le32(constant) to u8
> 
> Noticed on PowerPC allmod config build:
> 
> drivers/scsi/aacraid/commsup.c:1342: warning: large integer 
> implicitly truncated to unsigned type
> drivers/scsi/aacraid/commsup.c:1343: warning: large integer 
> implicitly truncated to unsigned type
> drivers/scsi/aacraid/commsup.c:1344: warning: large integer 
> implicitly truncated to unsigned type
> 
> Also fix some whitespace on the changed lines.
> 
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
>  drivers/scsi/aacraid/commsup.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> This version just fixes a couple of whitespace anomolies on 
> the lines I
> changed.
> 
> -- 
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au
> 
> diff --git a/drivers/scsi/aacraid/commsup.c 
> b/drivers/scsi/aacraid/commsup.c
> index 240a0bb..3c2dbc0 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -1339,9 +1339,9 @@ int aac_check_health(struct aac_dev * aac)
>  			aif = (struct aac_aifcmd *)hw_fib->data;
>  			aif->command = cpu_to_le32(AifCmdEventNotify);
>  		 	aif->seqnum = cpu_to_le32(0xFFFFFFFF);
> -		 	aif->data[0] = cpu_to_le32(AifEnExpEvent);
> -			aif->data[1] = cpu_to_le32(AifExeFirmwarePanic);
> -		 	aif->data[2] = cpu_to_le32(AifHighPriority);
> +			aif->data[0] = AifEnExpEvent;
> +			aif->data[1] = AifExeFirmwarePanic;
> +			aif->data[2] = AifHighPriority;
>  			aif->data[3] = cpu_to_le32(BlinkLED);
>  
>  			/*
> -- 
> 1.5.3.4
> 

Re: [PATCHv2] aacraid: don't assign cpu_to_le32(constant) to u8

[Andreas Schwab]

Stephen Rothwell <sfr@canb.auug.org.au> writes:

> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
> index 240a0bb..3c2dbc0 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -1339,9 +1339,9 @@ int aac_check_health(struct aac_dev * aac)
>  			aif = (struct aac_aifcmd *)hw_fib->data;
>  			aif->command = cpu_to_le32(AifCmdEventNotify);
>  		 	aif->seqnum = cpu_to_le32(0xFFFFFFFF);
> -		 	aif->data[0] = cpu_to_le32(AifEnExpEvent);
> -			aif->data[1] = cpu_to_le32(AifExeFirmwarePanic);
> -		 	aif->data[2] = cpu_to_le32(AifHighPriority);
> +			aif->data[0] = AifEnExpEvent;
> +			aif->data[1] = AifExeFirmwarePanic;
> +			aif->data[2] = AifHighPriority;
>  			aif->data[3] = cpu_to_le32(BlinkLED);

What about the last line?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
-
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

[PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8

[Salyzyn, Mark]

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

Good point, thanks. The intent of the management applications
utilization of this AIF report is to observe the LSB of the value of
integer value in BlinkLED. The actions of the cpu_to_le32 actually
breaks this and reports the wrong content in swapped architectures.

This attached follow-up patch is against current scsi-misc-2.6 *after*
the application of the 'don't assign cpu_to_le32(constant) to u8' patch
submitted by Stephen Rothwell which has already been taken by the -mm
tree. Inspection of other areas of the aacraid driver came up blank for
similar style bugs.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
handling of patch attachments (inline gets damaged, use attachment).

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/commsup.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -ru a/drivers/scsi/aacraid/commsup.c
b/drivers/scsi/aacraid/commsup.c
--- a/drivers/scsi/aacraid/commsup.c    2007-11-07 10:35:16.603727464
-0500
+++ b/drivers/scsi/aacraid/commsup.c    2007-11-07 10:37:50.540311107
-0500
@@ -1342,7 +1342,7 @@
                        aif->data[0] = AifEnExpEvent;
                        aif->data[1] = AifExeFirmwarePanic;
                        aif->data[2] = AifHighPriority;
-                       aif->data[3] = cpu_to_le32(BlinkLED);
+                       aif->data[3] = BlinkLED;

                        /*
                         * Put the FIB onto the

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Andreas Schwab [mailto:schwab@suse.de] 
> Sent: Thursday, November 01, 2007 9:31 AM
> To: Stephen Rothwell
> Cc: AACRAID; linux-scsi@vger.kernel.org; LKML
> Subject: Re: [PATCHv2] aacraid: don't assign 
> cpu_to_le32(constant) to u8
> 
> Stephen Rothwell <sfr@canb.auug.org.au> writes:
> 
> > diff --git a/drivers/scsi/aacraid/commsup.c 
> b/drivers/scsi/aacraid/commsup.c
> > index 240a0bb..3c2dbc0 100644
> > --- a/drivers/scsi/aacraid/commsup.c
> > +++ b/drivers/scsi/aacraid/commsup.c
> > @@ -1339,9 +1339,9 @@ int aac_check_health(struct aac_dev * aac)
> >  			aif = (struct aac_aifcmd *)hw_fib->data;
> >  			aif->command = cpu_to_le32(AifCmdEventNotify);
> >  		 	aif->seqnum = cpu_to_le32(0xFFFFFFFF);
> > -		 	aif->data[0] = cpu_to_le32(AifEnExpEvent);
> > -			aif->data[1] = cpu_to_le32(AifExeFirmwarePanic);
> > -		 	aif->data[2] = cpu_to_le32(AifHighPriority);
> > +			aif->data[0] = AifEnExpEvent;
> > +			aif->data[1] = AifExeFirmwarePanic;
> > +			aif->data[2] = AifHighPriority;
> >  			aif->data[3] = cpu_to_le32(BlinkLED);
> 
> What about the last line?
> 
> Andreas.

[-- Attachment #2: aacraid_BlinkLED.patch --]
[-- Type: application/octet-stream, Size: 461 bytes --]

diff -ru a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
--- a/drivers/scsi/aacraid/commsup.c	2007-11-07 10:35:16.603727464 -0500
+++ b/drivers/scsi/aacraid/commsup.c	2007-11-07 10:37:50.540311107 -0500
@@ -1342,7 +1342,7 @@
 			aif->data[0] = AifEnExpEvent;
 			aif->data[1] = AifExeFirmwarePanic;
 			aif->data[2] = AifHighPriority;
-			aif->data[3] = cpu_to_le32(BlinkLED);
+			aif->data[3] = BlinkLED;
 
 			/*
 			 * Put the FIB onto the

Re: [PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8

[Christoph Hellwig]

On Wed, Nov 07, 2007 at 10:58:12AM -0500, Salyzyn, Mark wrote:
> Good point, thanks. The intent of the management applications
> utilization of this AIF report is to observe the LSB of the value of
> integer value in BlinkLED. The actions of the cpu_to_le32 actually
> breaks this and reports the wrong content in swapped architectures.
> 
> This attached follow-up patch is against current scsi-misc-2.6 *after*
> the application of the 'don't assign cpu_to_le32(constant) to u8' patch
> submitted by Stephen Rothwell which has already been taken by the -mm
> tree. Inspection of other areas of the aacraid driver came up blank for
> similar style bugs.

Did anyone run the driver through sparse to see if we have more issues like
this?

RE: [PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8

[Salyzyn, Mark]

Christoph Hellwig [mailto:hch@infradead.org] sez:
> Did anyone run the driver through sparse to see if we have 
> more issues like this?

There are some warnings from sparse, none like this one. I will deal
with the warnings ...

Sincerely -- Mark Salyzyn

Re: [PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8

[Christoph Hellwig]

On Wed, Nov 07, 2007 at 01:51:44PM -0500, Salyzyn, Mark wrote:
> Christoph Hellwig [mailto:hch@infradead.org] sez:
> > Did anyone run the driver through sparse to see if we have 
> > more issues like this?
> 
> There are some warnings from sparse, none like this one. I will deal
> with the warnings ...

Actually there are a lot of endianess warnings, fortunately most of them
harmless.  The patch below fixes all of them up (including the ones in
the patch I replied to), except for aac_init_adapter which is really odd
and I don't know what to do.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/scsi/aacraid/aachba.c
===================================================================
--- linux-2.6.orig/drivers/scsi/aacraid/aachba.c	2007-11-08 17:09:50.000000000 +0100
+++ linux-2.6/drivers/scsi/aacraid/aachba.c	2007-11-08 17:14:43.000000000 +0100
@@ -981,7 +981,7 @@
 	aac_fib_init(fib);
 	readcmd = (struct aac_read *) fib_data(fib);
 	readcmd->command = cpu_to_le32(VM_CtBlockRead);
-	readcmd->cid = cpu_to_le16(scmd_id(cmd));
+	readcmd->cid = cpu_to_le32(scmd_id(cmd));
 	readcmd->block = cpu_to_le32((u32)(lba&0xffffffff));
 	readcmd->count = cpu_to_le32(count * 512);
 
@@ -1072,7 +1072,7 @@
 	aac_fib_init(fib);
 	writecmd = (struct aac_write *) fib_data(fib);
 	writecmd->command = cpu_to_le32(VM_CtBlockWrite);
-	writecmd->cid = cpu_to_le16(scmd_id(cmd));
+	writecmd->cid = cpu_to_le32(scmd_id(cmd));
 	writecmd->block = cpu_to_le32((u32)(lba&0xffffffff));
 	writecmd->count = cpu_to_le32(count * 512);
 	writecmd->sg.count = cpu_to_le32(1);
@@ -1306,8 +1306,8 @@
 			  dev->supplement_adapter_info.VpdInfo.Tsid);
 		}
 		if (!aac_check_reset ||
-		  (dev->supplement_adapter_info.SupportedOptions2 &
-		  le32_to_cpu(AAC_OPTION_IGNORE_RESET))) {
+		    (dev->supplement_adapter_info.SupportedOptions2 &
+		     cpu_to_le32(AAC_OPTION_IGNORE_RESET))) {
 			printk(KERN_INFO "%s%d: Reset Adapter Ignored\n",
 			  dev->name, dev->id);
 		}
Index: linux-2.6/drivers/scsi/aacraid/commsup.c
===================================================================
--- linux-2.6.orig/drivers/scsi/aacraid/commsup.c	2007-11-08 17:09:50.000000000 +0100
+++ linux-2.6/drivers/scsi/aacraid/commsup.c	2007-11-08 17:14:43.000000000 +0100
@@ -796,13 +796,13 @@
 	 */
 	switch (le32_to_cpu(aifcmd->command)) {
 	case AifCmdDriverNotify:
-		switch (le32_to_cpu(((u32 *)aifcmd->data)[0])) {
+		switch (le32_to_cpu(((__le32 *)aifcmd->data)[0])) {
 		/*
 		 *	Morph or Expand complete
 		 */
 		case AifDenMorphComplete:
 		case AifDenVolumeExtendComplete:
-			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			container = le32_to_cpu(((__le32 *)aifcmd->data)[1]);
 			if (container >= dev->maximum_num_containers)
 				break;
 
@@ -835,25 +835,25 @@
 			if (container >= dev->maximum_num_containers)
 				break;
 			if ((dev->fsa_dev[container].config_waiting_on ==
-			    le32_to_cpu(*(u32 *)aifcmd->data)) &&
+			    le32_to_cpu(*(__le32 *)aifcmd->data)) &&
 			 time_before(jiffies, dev->fsa_dev[container].config_waiting_stamp + AIF_SNIFF_TIMEOUT))
 				dev->fsa_dev[container].config_waiting_on = 0;
 		} else for (container = 0;
 		    container < dev->maximum_num_containers; ++container) {
 			if ((dev->fsa_dev[container].config_waiting_on ==
-			    le32_to_cpu(*(u32 *)aifcmd->data)) &&
+			    le32_to_cpu(*(__le32 *)aifcmd->data)) &&
 			 time_before(jiffies, dev->fsa_dev[container].config_waiting_stamp + AIF_SNIFF_TIMEOUT))
 				dev->fsa_dev[container].config_waiting_on = 0;
 		}
 		break;
 
 	case AifCmdEventNotify:
-		switch (le32_to_cpu(((u32 *)aifcmd->data)[0])) {
+		switch (le32_to_cpu(((__le32 *)aifcmd->data)[0])) {
 		/*
 		 *	Add an Array.
 		 */
 		case AifEnAddContainer:
-			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			container = le32_to_cpu(((__le32 *)aifcmd->data)[1]);
 			if (container >= dev->maximum_num_containers)
 				break;
 			dev->fsa_dev[container].config_needed = ADD;
@@ -866,7 +866,7 @@
 		 *	Delete an Array.
 		 */
 		case AifEnDeleteContainer:
-			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			container = le32_to_cpu(((__le32 *)aifcmd->data)[1]);
 			if (container >= dev->maximum_num_containers)
 				break;
 			dev->fsa_dev[container].config_needed = DELETE;
@@ -880,7 +880,7 @@
 		 * waiting on something else, setup to wait on a Config Change.
 		 */
 		case AifEnContainerChange:
-			container = le32_to_cpu(((u32 *)aifcmd->data)[1]);
+			container = le32_to_cpu(((__le32 *)aifcmd->data)[1]);
 			if (container >= dev->maximum_num_containers)
 				break;
 			if (dev->fsa_dev[container].config_waiting_on &&
@@ -905,13 +905,13 @@
 			if (container >= dev->maximum_num_containers)
 				break;
 			if ((dev->fsa_dev[container].config_waiting_on ==
-			    le32_to_cpu(*(u32 *)aifcmd->data)) &&
+			    le32_to_cpu(*(__le32 *)aifcmd->data)) &&
 			 time_before(jiffies, dev->fsa_dev[container].config_waiting_stamp + AIF_SNIFF_TIMEOUT))
 				dev->fsa_dev[container].config_waiting_on = 0;
 		} else for (container = 0;
 		    container < dev->maximum_num_containers; ++container) {
 			if ((dev->fsa_dev[container].config_waiting_on ==
-			    le32_to_cpu(*(u32 *)aifcmd->data)) &&
+			    le32_to_cpu(*(__le32 *)aifcmd->data)) &&
 			 time_before(jiffies, dev->fsa_dev[container].config_waiting_stamp + AIF_SNIFF_TIMEOUT))
 				dev->fsa_dev[container].config_waiting_on = 0;
 		}
@@ -926,9 +926,9 @@
 		 * wait for a container change.
 		 */
 
-		if ((((u32 *)aifcmd->data)[1] == cpu_to_le32(AifJobCtrZero))
-		 && ((((u32 *)aifcmd->data)[6] == ((u32 *)aifcmd->data)[5])
-		  || (((u32 *)aifcmd->data)[4] == cpu_to_le32(AifJobStsSuccess)))) {
+		if (((__le32 *)aifcmd->data)[1] == cpu_to_le32(AifJobCtrZero) &&
+		    (((__le32 *)aifcmd->data)[6] == ((__le32 *)aifcmd->data)[5] ||
+		     ((__le32 *)aifcmd->data)[4] == cpu_to_le32(AifJobStsSuccess))) {
 			for (container = 0;
 			    container < dev->maximum_num_containers;
 			    ++container) {
@@ -943,9 +943,9 @@
 					jiffies;
 			}
 		}
-		if ((((u32 *)aifcmd->data)[1] == cpu_to_le32(AifJobCtrZero))
-		 && (((u32 *)aifcmd->data)[6] == 0)
-		 && (((u32 *)aifcmd->data)[4] == cpu_to_le32(AifJobStsRunning))) {
+		if (((__le32 *)aifcmd->data)[1] == cpu_to_le32(AifJobCtrZero) &&
+		    ((__le32 *)aifcmd->data)[6] == 0 &&
+		    ((__le32 *)aifcmd->data)[4] == cpu_to_le32(AifJobStsRunning)) {
 			for (container = 0;
 			    container < dev->maximum_num_containers;
 			    ++container) {
@@ -1339,10 +1339,10 @@
 			aif = (struct aac_aifcmd *)hw_fib->data;
 			aif->command = cpu_to_le32(AifCmdEventNotify);
 		 	aif->seqnum = cpu_to_le32(0xFFFFFFFF);
-		 	aif->data[0] = cpu_to_le32(AifEnExpEvent);
-			aif->data[1] = cpu_to_le32(AifExeFirmwarePanic);
-		 	aif->data[2] = cpu_to_le32(AifHighPriority);
-			aif->data[3] = cpu_to_le32(BlinkLED);
+		 	aif->data[0] = AifEnExpEvent;
+			aif->data[1] = AifExeFirmwarePanic;
+		 	aif->data[2] = AifHighPriority;
+			aif->data[3] = BlinkLED;
 
 			/*
 			 * Put the FIB onto the
@@ -1373,8 +1373,8 @@
 	printk(KERN_ERR "%s: Host adapter BLINK LED 0x%x\n", aac->name, BlinkLED);
 
 	if (!aac_check_reset ||
-		(aac->supplement_adapter_info.SupportedOptions2 &
-			le32_to_cpu(AAC_OPTION_IGNORE_RESET)))
+	    (aac->supplement_adapter_info.SupportedOptions2 &
+	     cpu_to_le32(AAC_OPTION_IGNORE_RESET)))
 		goto out;
 	host = aac->scsi_host_ptr;
 	if (aac->thread->pid != current->pid)
@@ -1655,11 +1655,11 @@
 				struct fib *fibptr;
 
 				if ((fibptr = aac_fib_alloc(dev))) {
-					u32 * info;
+					__le32 *info;
 
 					aac_fib_init(fibptr);
 
-					info = (u32 *) fib_data(fibptr);
+					info = (__le32 *) fib_data(fibptr);
 					if (now.tv_usec > 500000)
 						++now.tv_sec;
 
Index: linux-2.6/drivers/scsi/aacraid/dpcsup.c
===================================================================
--- linux-2.6.orig/drivers/scsi/aacraid/dpcsup.c	2007-11-08 17:09:50.000000000 +0100
+++ linux-2.6/drivers/scsi/aacraid/dpcsup.c	2007-11-08 17:14:43.000000000 +0100
@@ -229,11 +229,9 @@
  *	all QE there are and wake up all the waiters before exiting.
  */
 
-unsigned int aac_intr_normal(struct aac_dev * dev, u32 Index)
+unsigned int aac_intr_normal(struct aac_dev * dev, u32 index)
 {
-	u32 index = le32_to_cpu(Index);
-
-	dprintk((KERN_INFO "aac_intr_normal(%p,%x)\n", dev, Index));
+	dprintk((KERN_INFO "aac_intr_normal(%p,%x)\n", dev, index));
 	if ((index & 0x00000002L)) {
 		struct hw_fib * hw_fib;
 		struct fib * fib;
@@ -301,7 +299,7 @@
 
 		if (hwfib->header.Command == cpu_to_le16(NuFileSystem))
 		{
-			u32 *pstatus = (u32 *)hwfib->data;
+			__le32 *pstatus = (__le32 *)hwfib->data;
 			if (*pstatus & cpu_to_le32(0xffff0000))
 				*pstatus = cpu_to_le32(ST_OK);
 		}
Index: linux-2.6/drivers/scsi/aacraid/linit.c
===================================================================
--- linux-2.6.orig/drivers/scsi/aacraid/linit.c	2007-11-08 17:09:50.000000000 +0100
+++ linux-2.6/drivers/scsi/aacraid/linit.c	2007-11-08 17:14:43.000000000 +0100
@@ -584,8 +584,8 @@
 	 * support a register, instead of a commanded, reset.
 	 */
 	if ((aac->supplement_adapter_info.SupportedOptions2 &
-	  le32_to_cpu(AAC_OPTION_MU_RESET|AAC_OPTION_IGNORE_RESET)) ==
-	  le32_to_cpu(AAC_OPTION_MU_RESET))
+	     cpu_to_le32(AAC_OPTION_MU_RESET|AAC_OPTION_IGNORE_RESET)) ==
+	     cpu_to_le32(AAC_OPTION_MU_RESET))
 		aac_reset_adapter(aac, 2); /* Bypass wait for command quiesce */
 	return SUCCESS; /* Cause an immediate retry of the command with a ten second delay after successful tur */
 }
Index: linux-2.6/drivers/scsi/aacraid/rx.c
===================================================================
--- linux-2.6.orig/drivers/scsi/aacraid/rx.c	2007-11-08 17:09:50.000000000 +0100
+++ linux-2.6/drivers/scsi/aacraid/rx.c	2007-11-08 17:14:43.000000000 +0100
@@ -465,7 +465,7 @@
 	u32 var;
 
 	if (!(dev->supplement_adapter_info.SupportedOptions2 &
-	  le32_to_cpu(AAC_OPTION_MU_RESET)) || (bled >= 0) || (bled == -2)) {
+	    cpu_to_le32(AAC_OPTION_MU_RESET)) || (bled >= 0) || (bled == -2)) {
 		if (bled)
 			printk(KERN_ERR "%s%d: adapter kernel panic'd %x.\n",
 				dev->name, dev->id, bled);

RE: [PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8

[Salyzyn, Mark]

Resounding ACK.

I just finished *exactly* the same set of changes, composed the patch
and was about to hit send when this one came over the wire from you!
There was absolutely no differences between our patches (save for the
fact I did not place the AIF ones in as they are already in the queue,
one is already on -mm).

I am going to return to this at some future date and figure out the
problems surrounding the context imbalances that are present, making
code that determines which context it is called from (sysfs, error
recovery or from the background thread) and plays with the various locks
confuses sparse. Rewriting so that the contexts are less programmatic is
in order...

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org] 
> Sent: Thursday, November 08, 2007 12:28 PM
> To: Salyzyn, Mark
> Cc: Christoph Hellwig; Andreas Schwab; Stephen Rothwell; 
> linux-scsi@vger.kernel.org; LKML
> Subject: Re: [PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8
> 
> On Wed, Nov 07, 2007 at 01:51:44PM -0500, Salyzyn, Mark wrote:
> > Christoph Hellwig [mailto:hch@infradead.org] sez:
> > > Did anyone run the driver through sparse to see if we have 
> > > more issues like this?
> > 
> > There are some warnings from sparse, none like this one. I will deal
> > with the warnings ...
> 
> Actually there are a lot of endianess warnings, fortunately 
> most of them
> harmless.  The patch below fixes all of them up (including the ones in
> the patch I replied to), except for aac_init_adapter which is 
> really odd
> and I don't know what to do.
> 

[PATCH 1/1] aacraid: add aacraid.cache parameter to driver to control FUA and SYNCHRONIZE_CACHE policy

[Salyzyn, Mark]

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

aacraid.cache parameter, Disable Queue Flush commands:
        bit 0 - Disable FUA in WRITE SCSI commands
        bit 1 - Disable SYNCHRONIZE_CACHE SCSI command
        bit 2 - Disable only if Battery not protecting adapter supplied Cache

e.g.: aacraid.cache=7 will disable the FUA and SYNCHRONIZE_CACHE commands if the adapter has reported that it's cache is battery backed up.

This parameter permits experimentation with tradeoffs between performance and caching policy.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments.

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/aachba.c  |   36 ++++++++++++++++++++++++------------
 drivers/scsi/aacraid/aacraid.h |    2 ++
 drivers/scsi/aacraid/commsup.c |    4 ++++
 3 files changed, 30 insertions(+), 12 deletions(-)

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_cache_parm.patch --]
[-- Type: application/octet-stream, Size: 5251 bytes --]

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c	2008-01-08 11:56:51.485033090 -0500
+++ b/drivers/scsi/aacraid/aachba.c	2008-01-08 12:17:08.293855342 -0500
@@ -144,6 +144,7 @@
  */	
  
 static int nondasd = -1;
+static int aac_cache = 0;
 static int dacmode = -1;
 
 int aac_commit = -1;
@@ -152,6 +153,8 @@
 
 module_param(nondasd, int, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(nondasd, "Control scanning of hba for nondasd devices. 0=off, 1=on");
+module_param_named(cache, aac_cache, int, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(cache, "Disable Queue Flush commands:\n\tbit 0 - Disable FUA in WRITE SCSI commands\n\tbit 1 - Disable SYNCHRONIZE_CACHE SCSI command\n\tbit 2 - Disable only if Battery not protecting Cache");
 module_param(dacmode, int, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(dacmode, "Control whether dma addressing is using 64 bit DAC. 0=off, 1=on");
 module_param_named(commit, aac_commit, int, S_IRUGO|S_IWUSR);
@@ -1013,7 +1016,8 @@
 	writecmd->block[1] = cpu_to_le32((u32)((lba&0xffffffff00000000LL)>>32));
 	writecmd->count = cpu_to_le32(count<<9);
 	writecmd->cid = cpu_to_le16(scmd_id(cmd));
-	writecmd->flags = fua ?
+	writecmd->flags = (fua && ((aac_cache & 5) != 1) &&
+	  (((aac_cache & 5) != 5) || !fib->dev->cache_protected)) ?
 		cpu_to_le16(IO_TYPE_WRITE|IO_SUREWRITE) :
 		cpu_to_le16(IO_TYPE_WRITE);
 	writecmd->bpTotal = 0;
@@ -1325,11 +1329,11 @@
 		}
 	}
 
+	dev->cache_protected = 0;
 	dev->nondasd_support = 0;
 	dev->raid_scsi_mode = 0;
-	if(dev->adapter_info.options & AAC_OPT_NONDASD){
+	if(dev->adapter_info.options & AAC_OPT_NONDASD)
 		dev->nondasd_support = 1;
-	}
 
 	/*
 	 * If the firmware supports ROMB RAID/SCSI mode and we are currently
@@ -1351,10 +1355,9 @@
 		printk(KERN_INFO "%s%d: ROMB RAID/SCSI mode enabled\n",
 				dev->name, dev->id);
 		
-	if(nondasd != -1) {  
+	if (nondasd != -1)
 		dev->nondasd_support = (nondasd!=0);
-	}
-	if(dev->nondasd_support != 0){
+	if(dev->nondasd_support != 0) {
 		printk(KERN_INFO "%s%d: Non-DASD support enabled.\n",dev->name, dev->id);
 	}
 
@@ -2106,7 +2109,7 @@
 		mode_buf[2] = 0;	/* Device-specific param,
 					   bit 8: 0/1 = write enabled/protected
 					   bit 4: 0/1 = FUA enabled */
-		if (dev->raw_io_interface)
+		if (dev->raw_io_interface && ((aac_cache & 5) != 1))
 			mode_buf[2] = 0x10;
 		mode_buf[3] = 0;	/* Block descriptor length */
 		if (((scsicmd->cmnd[2] & 0x3f) == 8) ||
@@ -2114,7 +2117,8 @@
 			mode_buf[0] = 6;
 			mode_buf[4] = 8;
 			mode_buf[5] = 1;
-			mode_buf[6] = 0x04; /* WCE */
+			mode_buf[6] = ((aac_cache & 6) == 2)
+				? 0 : 0x04; /* WCE */
 			mode_buf_length = 7;
 			if (mode_buf_length > scsicmd->cmnd[4])
 				mode_buf_length = scsicmd->cmnd[4];
@@ -2137,7 +2141,7 @@
 		mode_buf[3] = 0;	/* Device-specific param,
 					   bit 8: 0/1 = write enabled/protected
 					   bit 4: 0/1 = FUA enabled */
-		if (dev->raw_io_interface)
+		if (dev->raw_io_interface && ((aac_cache & 5) != 1))
 			mode_buf[3] = 0x10;
 		mode_buf[4] = 0;	/* reserved */
 		mode_buf[5] = 0;	/* reserved */
@@ -2148,7 +2152,8 @@
 			mode_buf[1] = 9;
 			mode_buf[8] = 8;
 			mode_buf[9] = 1;
-			mode_buf[10] = 0x04; /* WCE */
+			mode_buf[10] = ((aac_cache & 6) == 2)
+				? 0 : 0x04; /* WCE */
 			mode_buf_length = 11;
 			if (mode_buf_length > scsicmd->cmnd[8])
 				mode_buf_length = scsicmd->cmnd[8];
@@ -2224,9 +2229,16 @@
 			return aac_write(scsicmd);
 
 		case SYNCHRONIZE_CACHE:
+			if (((aac_cache & 6) == 6) && dev->cache_protected) {
+				scsicmd->result = DID_OK << 16 |
+					COMMAND_COMPLETE << 8 | SAM_STAT_GOOD;
+				scsicmd->scsi_done(scsicmd);
+				return 0;
+			}
 			/* Issue FIB to tell Firmware to flush it's cache */
-			return aac_synchronize(scsicmd);
-			
+			if ((aac_cache & 6) != 2)
+				return aac_synchronize(scsicmd);
+			/* FALLTHRU */
 		default:
 			/*
 			 *	Unhandled commands
diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h	2008-01-08 11:56:51.485033090 -0500
+++ b/drivers/scsi/aacraid/aacraid.h	2008-01-08 12:11:16.876515258 -0500
@@ -1016,6 +1016,7 @@
 	 * lets break them out so we don't have to do an AND to check them
 	 */
 	u8			nondasd_support; 
+	u8			cache_protected;
 	u8			dac_support;
 	u8			raid_scsi_mode;
 	u8			comm_interface;
@@ -1770,6 +1771,7 @@
 #define			AifEnConfigChange	3	/* Adapter configuration change */
 #define			AifEnContainerChange	4	/* Container configuration change */
 #define			AifEnDeviceFailure	5	/* SCSI device failed */
+#define			AifEnBatteryEvent	14	/* Change in Battery State */
 #define			AifEnAddContainer	15	/* A new array was created */
 #define			AifEnDeleteContainer	16	/* A container was deleted */
 #define			AifEnExpEvent		23	/* Firmware Event Log */
diff -ru a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
--- a/drivers/scsi/aacraid/commsup.c	2008-01-08 11:56:51.487032838 -0500
+++ b/drivers/scsi/aacraid/commsup.c	2008-01-08 12:12:14.947135086 -0500
@@ -849,6 +849,10 @@
 
 	case AifCmdEventNotify:
 		switch (le32_to_cpu(((__le32 *)aifcmd->data)[0])) {
+		case AifEnBatteryEvent:
+			dev->cache_protected =
+				(((__le32 *)aifcmd->data)[1] == cpu_to_le32(3));
+			break;
 		/*
 		 *	Add an Array.
 		 */

[PATCH 1/1] aacraid: add call to flush_kernel_dcache_page for architectures that require it.

[Salyzyn, Mark]

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

Some architectures require a call to flush_kernel_dcache_page for processor spoofed DMA operations.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments (inline gets damaged, use attachment).

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 aachba.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c     2008-01-08 15:01:21.503932722 -0500
+++ b/drivers/scsi/aacraid/aachba.c     2008-01-08 15:02:35.849634368 -0500
@@ -31,9 +31,9 @@
 #include <linux/slab.h>
 #include <linux/completion.h>
 #include <linux/blkdev.h>
-#include <linux/dma-mapping.h>
 #include <asm/semaphore.h>
 #include <asm/uaccess.h>
+#include <linux/highmem.h> /* For flush_kernel_dcache_page */

 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -366,6 +366,9 @@
        if (buf && transfer_len > 0)
                memcpy(buf + offset, data, transfer_len);

+#ifdef ARCH_HAS_FLUSH_ANON_PAGE
+       flush_kernel_dcache_page(kmap_atomic_to_page(buf - sg->offset));
+#endif
        kunmap_atomic(buf - sg->offset, KM_IRQ0);

 }

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_flush_dcache3.patch --]
[-- Type: application/octet-stream, Size: 777 bytes --]

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c	2008-01-08 15:01:21.503932722 -0500
+++ b/drivers/scsi/aacraid/aachba.c	2008-01-08 15:02:35.849634368 -0500
@@ -31,9 +31,9 @@
 #include <linux/slab.h>
 #include <linux/completion.h>
 #include <linux/blkdev.h>
-#include <linux/dma-mapping.h>
 #include <asm/semaphore.h>
 #include <asm/uaccess.h>
+#include <linux/highmem.h> /* For flush_kernel_dcache_page */
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -366,6 +366,9 @@
 	if (buf && transfer_len > 0)
 		memcpy(buf + offset, data, transfer_len);
 
+#ifdef ARCH_HAS_FLUSH_ANON_PAGE
+	flush_kernel_dcache_page(kmap_atomic_to_page(buf - sg->offset));
+#endif
 	kunmap_atomic(buf - sg->offset, KM_IRQ0);
 
 }

[PATCH 1/1] aacraid: add sysfs report of RAID level

[Salyzyn, Mark]

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

Report the RAID level string for the SCSI device representing the array. Report is in /sys/class/scsi_device/#:#:#:#/device/level.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments.

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/aachba.c  |    7 ++++++-
 drivers/scsi/aacraid/aacraid.h |    1 +
 drivers/scsi/aacraid/linit.c   |   25 +++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_sysfs_raid_level.patch --]
[-- Type: application/octet-stream, Size: 2353 bytes --]

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c	2008-01-08 15:10:02.894673368 -0500
+++ b/drivers/scsi/aacraid/aachba.c	2008-01-08 15:16:23.513965396 -0500
@@ -693,7 +693,12 @@
         "Unknown"
 };
 
-
+char * get_container_type(unsigned tindex)
+{
+	if (tindex >= ARRAY_SIZE(container_types))
+		tindex = ARRAY_SIZE(container_types) - 1;
+	return container_types[tindex];
+}
 
 /* Function: setinqstr
  *
diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h	2008-01-08 15:10:02.894673368 -0500
+++ b/drivers/scsi/aacraid/aacraid.h	2008-01-08 15:16:23.514965271 -0500
@@ -1869,6 +1869,7 @@
 int _aac_rx_init(struct aac_dev *dev);
 int aac_rx_select_comm(struct aac_dev *dev, int comm);
 int aac_rx_deliver_producer(struct fib * fib);
+char * get_container_type(unsigned type);
 extern int numacb;
 extern int acbsize;
 extern char aac_driver_version[];
diff -ru a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
--- a/drivers/scsi/aacraid/linit.c	2008-01-08 15:10:02.896673118 -0500
+++ b/drivers/scsi/aacraid/linit.c	2008-01-08 15:16:23.515965145 -0500
@@ -487,6 +487,30 @@
 	return sdev->queue_depth;
 }
 
+static ssize_t aac_show_raid_level(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct scsi_device * sdev = to_scsi_device(dev);
+	if (sdev_channel(sdev) != CONTAINER_CHANNEL)
+		return snprintf(buf, PAGE_SIZE, sdev->no_uld_attach
+		  ? "Hidden\n" : "JBOD");
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+	  get_container_type(((struct aac_dev *)(sdev->host->hostdata))
+	    ->fsa_dev[sdev_id(sdev)].type));
+}
+
+static struct device_attribute aac_raid_level_attr = {
+	.attr = {
+		.name = "level",
+		.mode = S_IRUGO,
+	},
+	.show = aac_show_raid_level
+};
+
+static struct device_attribute *aac_dev_attrs[] = {
+	&aac_raid_level_attr,
+	NULL,
+};
+
 static int aac_ioctl(struct scsi_device *sdev, int cmd, void __user * arg)
 {
 	struct aac_dev *dev = (struct aac_dev *)sdev->host->hostdata;
@@ -941,6 +965,7 @@
 	.shost_attrs			= aac_attrs,
 	.slave_configure		= aac_slave_configure,
 	.change_queue_depth		= aac_change_queue_depth,
+	.sdev_attrs			= aac_dev_attrs,
 	.eh_abort_handler		= aac_eh_abort,
 	.eh_host_reset_handler		= aac_eh_reset,
 	.can_queue      		= AAC_NUM_IO_FIB,	

[PATCH 1/1] aacraid: variable redefinition hides earlier warning

[Salyzyn, Mark]

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

The parameter 'info' is reused, renamed the second to sinfo to represent supplemental adapter info, to suppress compile warning message.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments (inline gets damaged, use attachment).

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/aachba.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

--- a/drivers/scsi/aacraid/aachba.c     2008-01-08 15:24:06.451870018 -0500
+++ b/drivers/scsi/aacraid/aachba.c     2008-01-08 15:25:08.971018896 -0500
@@ -1243,24 +1243,24 @@
        memcpy(&dev->adapter_info, info, sizeof(*info));

        if (dev->adapter_info.options & AAC_OPT_SUPPLEMENT_ADAPTER_INFO) {
-               struct aac_supplement_adapter_info * info;
+               struct aac_supplement_adapter_info * sinfo;

                aac_fib_init(fibptr);

-               info = (struct aac_supplement_adapter_info *) fib_data(fibptr);
+               sinfo = (struct aac_supplement_adapter_info *) fib_data(fibptr);


-               memset(info,0,sizeof(*info));
+               memset(sinfo,0,sizeof(*sinfo));

                rcode = aac_fib_send(RequestSupplementAdapterInfo,
                                 fibptr,
-                                sizeof(*info),
+                                sizeof(*sinfo),
                                 FsaNormal,
                                 1, 1,
                                 NULL,
                                 NULL);

                if (rcode >= 0)
-                       memcpy(&dev->supplement_adapter_info, info, sizeof(*info));
+                       memcpy(&dev->supplement_adapter_info, sinfo, sizeof(*sinfo));
        }

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_info_var_hides_warning.patch --]
[-- Type: application/octet-stream, Size: 1016 bytes --]

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c	2008-01-08 15:24:06.451870018 -0500
+++ b/drivers/scsi/aacraid/aachba.c	2008-01-08 15:25:08.971018896 -0500
@@ -1243,24 +1243,24 @@
 	memcpy(&dev->adapter_info, info, sizeof(*info));
 
 	if (dev->adapter_info.options & AAC_OPT_SUPPLEMENT_ADAPTER_INFO) {
-		struct aac_supplement_adapter_info * info;
+		struct aac_supplement_adapter_info * sinfo;
 
 		aac_fib_init(fibptr);
 
-		info = (struct aac_supplement_adapter_info *) fib_data(fibptr);
+		sinfo = (struct aac_supplement_adapter_info *) fib_data(fibptr);
 
-		memset(info,0,sizeof(*info));
+		memset(sinfo,0,sizeof(*sinfo));
 
 		rcode = aac_fib_send(RequestSupplementAdapterInfo,
 				 fibptr,
-				 sizeof(*info),
+				 sizeof(*sinfo),
 				 FsaNormal,
 				 1, 1,
 				 NULL,
 				 NULL);
 
 		if (rcode >= 0)
-			memcpy(&dev->supplement_adapter_info, info, sizeof(*info));
+			memcpy(&dev->supplement_adapter_info, sinfo, sizeof(*sinfo));
 	}
 
 

[PATCH 1/1] aacraid: big endian issues

[Salyzyn, Mark]

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

Big endian systems issues discovered in the aacraid driver. Somewhat reverses a patch from November 7th of last year that removed swap operations because they formerly were being assigned to an u8 array when they should have been assigned to an le32 array.

This patch is largely inert for any little endian processor architecture. It resolves a bug in delivering the BlinkLED AIF event to registered applications when the adapter or associated hardware was reset due to ill health. A rare corner case occurrence, also largely unnoticed by any as it was a new (untested!) feature.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments.

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/aachba.c   |    7 +++----
 drivers/scsi/aacraid/aacraid.h  |    6 +++---
 drivers/scsi/aacraid/comminit.c |    4 ++--
 drivers/scsi/aacraid/commsup.c  |   17 ++++++++---------
 drivers/scsi/aacraid/linit.c    |   10 +++++-----
 drivers/scsi/aacraid/rx.c       |    2 +-
 6 files changed, 22 insertions(+), 24 deletions(-)

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_big_endian.patch --]
[-- Type: application/octet-stream, Size: 4990 bytes --]

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c	2008-01-08 15:32:28.328810979 -0500
+++ b/drivers/scsi/aacraid/aachba.c	2008-01-08 15:37:35.631163858 -0500
@@ -1328,10 +1328,9 @@
 			  (int)sizeof(dev->supplement_adapter_info.VpdInfo.Tsid),
 			  dev->supplement_adapter_info.VpdInfo.Tsid);
 		}
-		if (!aac_check_reset ||
-		    ((aac_check_reset != 1) &&
-		     (dev->supplement_adapter_info.SupportedOptions2 &
-		      cpu_to_le32(AAC_OPTION_IGNORE_RESET)))) {
+		if (!aac_check_reset || ((aac_check_reset != 1) &&
+		  (dev->supplement_adapter_info.SupportedOptions2 &
+		  AAC_OPTION_IGNORE_RESET))) {
 			printk(KERN_INFO "%s%d: Reset Adapter Ignored\n",
 			  dev->name, dev->id);
 		}
diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h	2008-01-08 15:32:28.328810979 -0500
+++ b/drivers/scsi/aacraid/aacraid.h	2008-01-08 15:37:35.632163732 -0500
@@ -869,9 +869,9 @@
 	__le32	SupportedOptions2;
 	__le32	ReservedGrowth[1];
 };
-#define AAC_FEATURE_FALCON	0x00000010
-#define AAC_OPTION_MU_RESET	0x00000001
-#define AAC_OPTION_IGNORE_RESET	0x00000002
+#define AAC_FEATURE_FALCON	cpu_to_le32(0x00000010)
+#define AAC_OPTION_MU_RESET	cpu_to_le32(0x00000001)
+#define AAC_OPTION_IGNORE_RESET	cpu_to_le32(0x00000002)
 #define AAC_SIS_VERSION_V3	3
 #define AAC_SIS_SLOT_UNKNOWN	0xFF
 
diff -ru a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
--- a/drivers/scsi/aacraid/comminit.c	2008-01-08 15:32:28.329810853 -0500
+++ b/drivers/scsi/aacraid/comminit.c	2008-01-08 15:37:35.633163607 -0500
@@ -301,10 +301,10 @@
 	if ((!aac_adapter_sync_cmd(dev, GET_ADAPTER_PROPERTIES,
 		0, 0, 0, 0, 0, 0, status+0, status+1, status+2, NULL, NULL)) &&
 	 		(status[0] == 0x00000001)) {
-		if (status[1] & AAC_OPT_NEW_COMM_64)
+		if (status[1] & le32_to_cpu(AAC_OPT_NEW_COMM_64))
 			dev->raw_io_64 = 1;
 		if (dev->a_ops.adapter_comm &&
-		    (status[1] & AAC_OPT_NEW_COMM))
+		    (status[1] & le32_to_cpu(AAC_OPT_NEW_COMM)))
 			dev->comm_interface = AAC_COMM_MESSAGE;
 		if ((dev->comm_interface == AAC_COMM_MESSAGE) &&
 		    (status[2] > dev->base_size)) {
diff -ru a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
--- a/drivers/scsi/aacraid/commsup.c	2008-01-08 15:32:28.329810853 -0500
+++ b/drivers/scsi/aacraid/commsup.c	2008-01-08 15:37:35.634163481 -0500
@@ -1343,11 +1343,11 @@
 			fib->data = hw_fib->data;
 			aif = (struct aac_aifcmd *)hw_fib->data;
 			aif->command = cpu_to_le32(AifCmdEventNotify);
-		 	aif->seqnum = cpu_to_le32(0xFFFFFFFF);
-			aif->data[0] = AifEnExpEvent;
-			aif->data[1] = AifExeFirmwarePanic;
-			aif->data[2] = AifHighPriority;
-			aif->data[3] = BlinkLED;
+			aif->seqnum = cpu_to_le32(0xFFFFFFFF);
+			((__le32 *)aif->data)[0] = cpu_to_le32(AifEnExpEvent);
+			((__le32 *)aif->data)[1] = cpu_to_le32(AifExeFirmwarePanic);
+			((__le32 *)aif->data)[2] = cpu_to_le32(AifHighPriority);
+			((__le32 *)aif->data)[3] = cpu_to_le32(BlinkLED);
 
 			/*
 			 * Put the FIB onto the
@@ -1377,10 +1377,9 @@
 
 	printk(KERN_ERR "%s: Host adapter BLINK LED 0x%x\n", aac->name, BlinkLED);
 
-	if (!aac_check_reset ||
-	    ((aac_check_reset != 1) &&
-	     (aac->supplement_adapter_info.SupportedOptions2 &
-	      cpu_to_le32(AAC_OPTION_IGNORE_RESET))))
+	if (!aac_check_reset || ((aac_check_reset != 1) &&
+		(aac->supplement_adapter_info.SupportedOptions2 &
+			AAC_OPTION_IGNORE_RESET)))
 		goto out;
 	host = aac->scsi_host_ptr;
 	if (aac->thread->pid != current->pid)
diff -ru a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
--- a/drivers/scsi/aacraid/linit.c	2008-01-08 15:32:28.329810853 -0500
+++ b/drivers/scsi/aacraid/linit.c	2008-01-08 15:37:35.634163481 -0500
@@ -614,11 +614,11 @@
 	 * support a register, instead of a commanded, reset.
 	 */
 	if ((aac->supplement_adapter_info.SupportedOptions2 &
-	     cpu_to_le32(AAC_OPTION_MU_RESET)) &&
-	    aac_check_reset &&
-	    ((aac_check_reset != 1) ||
-	     (aac->supplement_adapter_info.SupportedOptions2 &
-	      cpu_to_le32(AAC_OPTION_IGNORE_RESET))))
+	   AAC_OPTION_MU_RESET) &&
+	  aac_check_reset &&
+	  ((aac_check_reset != 1) ||
+	   (aac->supplement_adapter_info.SupportedOptions2 &
+	    AAC_OPTION_IGNORE_RESET)))
 		aac_reset_adapter(aac, 2); /* Bypass wait for command quiesce */
 	return SUCCESS; /* Cause an immediate retry of the command with a ten second delay after successful tur */
 }
diff -ru a/drivers/scsi/aacraid/rx.c b/drivers/scsi/aacraid/rx.c
--- a/drivers/scsi/aacraid/rx.c	2008-01-08 15:32:28.330810727 -0500
+++ b/drivers/scsi/aacraid/rx.c	2008-01-08 15:37:35.635163355 -0500
@@ -465,7 +465,7 @@
 	u32 var;
 
 	if (!(dev->supplement_adapter_info.SupportedOptions2 &
-	    cpu_to_le32(AAC_OPTION_MU_RESET)) || (bled >= 0) || (bled == -2)) {
+	  AAC_OPTION_MU_RESET) || (bled >= 0) || (bled == -2)) {
 		if (bled)
 			printk(KERN_ERR "%s%d: adapter kernel panic'd %x.\n",
 				dev->name, dev->id, bled);

[PATCH 1/1] aacraid: multiple definition of automatic variable warning.

[Salyzyn, Mark]

The 'entry' automatic variable was defined at the top and within a block that uses it, removed the definition from the block that uses it. Some cosmetic changes were made while in the same file. This patch should be inert.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments.

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

This attached patch is against current scsi-misc-2.6.

 commctrl.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Sincerely -- Mark Salyzyn

RE: [PATCH 1/1] aacraid: multiple definition of automatic variable warning.

[Salyzyn, Mark]

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

Missed the patch attachment 8-}

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Salyzyn, Mark
> Sent: Tuesday, January 08, 2008 3:58 PM
> To: 'linux-scsi@vger.kernel.org'
> Subject: [PATCH 1/1] aacraid: multiple definition of
> automatic variable warning.
>
> The 'entry' automatic variable was defined at the top and
> within a block that uses it, removed the definition from the
> block that uses it. Some cosmetic changes were made while in
> the same file. This patch should be inert.
>
> This attached patch is against current scsi-misc-2.6.
>
> ObligatoryDisclaimer: Please accept my condolences regarding
> Outlook's handling of patch attachments.
>
> Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>
>
> This attached patch is against current scsi-misc-2.6.
>
>  commctrl.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_multiple_definition.patch --]
[-- Type: application/octet-stream, Size: 1706 bytes --]

diff -ru a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
--- a/drivers/scsi/aacraid/commctrl.c	2008-01-08 15:48:29.962793278 -0500
+++ b/drivers/scsi/aacraid/commctrl.c	2008-01-08 15:52:57.047142975 -0500
@@ -275,7 +275,6 @@
 	 */
 return_fib:
 	if (!list_empty(&fibctx->fib_list)) {
-		struct list_head * entry;
 		/*
 		 *	Pull the next fib from the fibs
 		 */
@@ -582,7 +581,7 @@
 				void* p;
 				/* Does this really need to be GFP_DMA? */
 				p = kmalloc(upsg->sg[i].count,GFP_KERNEL|__GFP_DMA);
-				if(p == 0) {
+				if(!p) {
 					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
 					  upsg->sg[i].count,i,upsg->count));
 					rcode = -ENOMEM;
@@ -626,7 +625,7 @@
 				void* p;
 				/* Does this really need to be GFP_DMA? */
 				p = kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
-				if(p == 0) {
+				if(!p) {
 					kfree (usg);
 					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
 					  usg->sg[i].count,i,usg->count));
@@ -668,7 +667,7 @@
 				void* p;
 				/* Does this really need to be GFP_DMA? */
 				p = kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
-				if(p == 0) {
+				if(!p) {
 					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
 					  usg->sg[i].count,i,usg->count));
 					rcode = -ENOMEM;
@@ -698,7 +697,7 @@
 				dma_addr_t addr;
 				void* p;
 				p = kmalloc(upsg->sg[i].count, GFP_KERNEL);
-				if(p == 0) {
+				if (!p) {
 					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
 					  upsg->sg[i].count, i, upsg->count));
 					rcode = -ENOMEM;

Re: [PATCH 1/1] aacraid: big endian issues

[Grant Grundler]

On Jan 8, 2008 12:48 PM, Salyzyn, Mark <Mark_Salyzyn@adaptec.com> wrote:
> Big endian systems issues discovered in the aacraid driver.

...
--- a/drivers/scsi/aacraid/comminit.c	2008-01-08 15:32:28.329810853 -0500
+++ b/drivers/scsi/aacraid/comminit.c	2008-01-08 15:37:35.633163607 -0500
@@ -301,10 +301,10 @@
 	if ((!aac_adapter_sync_cmd(dev, GET_ADAPTER_PROPERTIES,
 		0, 0, 0, 0, 0, 0, status+0, status+1, status+2, NULL, NULL)) &&
 	 		(status[0] == 0x00000001)) {
-		if (status[1] & AAC_OPT_NEW_COMM_64)
+		if (status[1] & le32_to_cpu(AAC_OPT_NEW_COMM_64))
...

Why apply le32_to_cpu() to the constant instead of the variable?
On systems were le32_to_cpu() is doing something, can gcc or
preprocessor optimize the constant?
I've always assumed it could not but that might be wrong.

thanks,
grant

[PATCH 1/1] aacraid: OS panic after Adapter panic (hardening).

[Salyzyn, Mark]

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

In experiments in the lab we managed to trigger an Adapter firmware panic (BlinkLED) coincidentally while several pass-through ioctl command from the management software were outstanding on a bug only present on a class of RAID Adapters that require a hardware reset rather than a commanded reset. The net result was an attempt to time out the management software command as if it came from the SCSI layer resulting in an OS panic.

Adapters that use commanded reset, management commands are returned failed by the Adapter correctly. The adapter firmware panic that resulted in this condition was also resolved, and there were no adapters in the field with this specific firmware bug so we do not expect any field reports. This is a rare or unlikely corner condition, and no reports have ever been forwarded from the field.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments.

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/aacraid.h |    1 +
 drivers/scsi/aacraid/commsup.c |    4 +++-
 drivers/scsi/aacraid/dpcsup.c  |    4 +++-
 drivers/scsi/aacraid/linit.c   |   19 ++++++++++++++++++-
 4 files changed, 25 insertions(+), 3 deletions(-)

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_fib_context.patch --]
[-- Type: application/octet-stream, Size: 4004 bytes --]

diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h	2008-01-08 15:58:09.201795366 -0500
+++ b/drivers/scsi/aacraid/aacraid.h	2008-01-08 16:05:00.175954480 -0500
@@ -1073,6 +1073,7 @@
 	(dev)->a_ops.adapter_comm(dev, comm)
 
 #define FIB_CONTEXT_FLAG_TIMED_OUT		(0x00000001)
+#define FIB_CONTEXT_FLAG			(0x00000002)
 
 /*
  *	Define the command values
diff -ru a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
--- a/drivers/scsi/aacraid/commsup.c	2008-01-08 15:58:09.202795240 -0500
+++ b/drivers/scsi/aacraid/commsup.c	2008-01-08 16:05:00.176954351 -0500
@@ -171,6 +171,7 @@
 	 *	each I/O
 	 */
 	fibptr->hw_fib_va->header.XferState = 0;
+	fibptr->flags = 0;
 	fibptr->callback = NULL;
 	fibptr->callback_data = NULL;
 
@@ -402,6 +403,7 @@
 	 *	will have a debug mode where the adapter can notify the host
 	 *	it had a problem and the host can log that fact.
 	 */
+	fibptr->flags = 0;
 	if (wait && !reply) {
 		return -EINVAL;
 	} else if (!wait && reply) {
@@ -450,10 +452,10 @@
 	if (!wait) {
 		fibptr->callback = callback;
 		fibptr->callback_data = callback_data;
+		fibptr->flags = FIB_CONTEXT_FLAG;
 	}
 
 	fibptr->done = 0;
-	fibptr->flags = 0;
 
 	FIB_COUNTER_INCREMENT(aac_config.FibsSent);
 
diff -ru a/drivers/scsi/aacraid/dpcsup.c b/drivers/scsi/aacraid/dpcsup.c
--- a/drivers/scsi/aacraid/dpcsup.c	2008-01-08 15:58:09.202795240 -0500
+++ b/drivers/scsi/aacraid/dpcsup.c	2008-01-08 16:05:00.176954351 -0500
@@ -120,6 +120,7 @@
 			 *	NOTE:  we cannot touch the fib after this
 			 *	    call, because it may have been deallocated.
 			 */
+			fib->flags = 0;
 			fib->callback(fib->callback_data, fib);
 		} else {
 			unsigned long flagv;
@@ -229,7 +230,7 @@
  *	all QE there are and wake up all the waiters before exiting.
  */
 
-unsigned int aac_intr_normal(struct aac_dev *dev, u32 index)
+unsigned int aac_intr_normal(struct aac_dev * dev, u32 index)
 {
 	dprintk((KERN_INFO "aac_intr_normal(%p,%x)\n", dev, index));
 	if ((index & 0x00000002L)) {
@@ -313,6 +314,7 @@
 			 *	NOTE:  we cannot touch the fib after this
 			 *	    call, because it may have been deallocated.
 			 */
+			fib->flags = 0;
 			fib->callback(fib->callback_data, fib);
 		} else {
 			unsigned long flagv;
diff -ru a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
--- a/drivers/scsi/aacraid/linit.c	2008-01-08 15:58:09.203795114 -0500
+++ b/drivers/scsi/aacraid/linit.c	2008-01-08 16:05:00.177954222 -0500
@@ -536,17 +536,33 @@
 			break;
 	case INQUIRY:
 	case READ_CAPACITY:
-	case TEST_UNIT_READY:
 		/* Mark associated FIB to not complete, eh handler does this */
 		for (count = 0; count < (host->can_queue + AAC_NUM_MGT_FIB); ++count) {
 			struct fib * fib = &aac->fibs[count];
 			if (fib->hw_fib_va->header.XferState &&
+			  (fib->flags & FIB_CONTEXT_FLAG) &&
 			  (fib->callback_data == cmd)) {
 				fib->flags |= FIB_CONTEXT_FLAG_TIMED_OUT;
 				cmd->SCp.phase = AAC_OWNER_ERROR_HANDLER;
 				ret = SUCCESS;
 			}
 		}
+		break;
+	case TEST_UNIT_READY:
+		/* Mark associated FIB to not complete, eh handler does this */
+		for (count = 0; count < (host->can_queue + AAC_NUM_MGT_FIB); ++count) {
+			struct scsi_cmnd * command;
+			struct fib * fib = &aac->fibs[count];
+			if ((fib->hw_fib_va->header.XferState & cpu_to_le32(Async | NoResponseExpected)) &&
+			  (fib->flags & FIB_CONTEXT_FLAG) &&
+			  ((command = fib->callback_data)) &&
+			  (command->device == cmd->device)) {
+				fib->flags |= FIB_CONTEXT_FLAG_TIMED_OUT;
+				command->SCp.phase = AAC_OWNER_ERROR_HANDLER;
+				if (command == cmd)
+					ret = SUCCESS;
+			}
+		}
 	}
 	return ret;
 }
@@ -569,6 +585,7 @@
 	for (count = 0; count < (host->can_queue + AAC_NUM_MGT_FIB); ++count) {
 		struct fib * fib = &aac->fibs[count];
 		if (fib->hw_fib_va->header.XferState &&
+		  (fib->flags & FIB_CONTEXT_FLAG) &&
 		  (fib->callback_data == cmd)) {
 			fib->flags |= FIB_CONTEXT_FLAG_TIMED_OUT;
 			cmd->SCp.phase = AAC_OWNER_ERROR_HANDLER;

RE: [PATCH 1/1] aacraid: big endian issues

[Salyzyn, Mark]

I've always assumed that byte swapping of constants would be optimized where a variable would not :-)

I have confirmed in assembler output of the compiler that constant merely become byte reversed constants optimized or no in at least one architectural case. I have *not* confirmed that a variable byte reversal requires processing overhead, as one could possibly expect the compiler to instead optimize by byte reversing the constant when comparing. However, I will guarantee you that if optimization is turned off in the compiler that such an optimization will not take place...

I do not think this turns into a readability issue in either case and view this as a simple cosmetic coding precaution much like likely()/unlikely() offers hints to the compiler on code intent...

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Grant Grundler [mailto:grundler@google.com]
> Sent: Tuesday, January 08, 2008 4:17 PM
> To: Salyzyn, Mark
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/1] aacraid: big endian issues
>
> On Jan 8, 2008 12:48 PM, Salyzyn, Mark
> <Mark_Salyzyn@adaptec.com> wrote:
> > Big endian systems issues discovered in the aacraid driver.
>
> ...
> --- a/drivers/scsi/aacraid/comminit.c   2008-01-08
> 15:32:28.329810853 -0500
> +++ b/drivers/scsi/aacraid/comminit.c   2008-01-08
> 15:37:35.633163607 -0500
> @@ -301,10 +301,10 @@
>         if ((!aac_adapter_sync_cmd(dev, GET_ADAPTER_PROPERTIES,
>                 0, 0, 0, 0, 0, 0, status+0, status+1,
> status+2, NULL, NULL)) &&
>                         (status[0] == 0x00000001)) {
> -               if (status[1] & AAC_OPT_NEW_COMM_64)
> +               if (status[1] & le32_to_cpu(AAC_OPT_NEW_COMM_64))
> ...
>
> Why apply le32_to_cpu() to the constant instead of the variable?
> On systems were le32_to_cpu() is doing something, can gcc or
> preprocessor optimize the constant?
> I've always assumed it could not but that might be wrong.
>
> thanks,
> grant

Re: [PATCH 1/1] aacraid: big endian issues

[Matthew Wilcox]

On Tue, Jan 08, 2008 at 01:17:15PM -0800, Grant Grundler wrote:
> Why apply le32_to_cpu() to the constant instead of the variable?
> On systems were le32_to_cpu() is doing something, can gcc or
> preprocessor optimize the constant?
> I've always assumed it could not but that might be wrong.

$ grep constant_p include/linux/byteorder/*
include/linux/byteorder/swabb.h:(__builtin_constant_p((__u32)(x)) ? \
include/linux/byteorder/swabb.h:(__builtin_constant_p((__u32)(x)) ? \
include/linux/byteorder/swab.h:(__builtin_constant_p((__u16)(x)) ? \
include/linux/byteorder/swab.h:(__builtin_constant_p((__u32)(x)) ? \
include/linux/byteorder/swab.h:(__builtin_constant_p((__u64)(x)) ? \

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

[PATCH 1/1] aacraid: improve queue balancing

[Salyzyn, Mark]

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

The adapter queue is divided up equally to all the arrays to prevent command starvation to any individual array. On the other hand, physical targets are only granted a queue depth of one each. The code prior to this patch used to deal with the incremental discovery of targets, but the driver knows how many arrays are present prior to the scan so this knowledge is used to generate a better estimate for the queue depth.

Remove the capability of 'physical=0' from preventing access to the class of adapters that have the RAID/SCSI mode of operation since none of the physicals on the SCSI channel are candidates ever for an array.

As always, the user can override this default queue depth policy by making the appropriate adjustments utilizing sysfs.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments (inline gets damaged, use attachment).

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/linit.c |   23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

--- a/drivers/scsi/aacraid/linit.c      2008-01-08 16:38:32.783320829 -0500
+++ b/drivers/scsi/aacraid/linit.c      2008-01-08 16:55:07.805050564 -0500
@@ -401,16 +401,14 @@

 static int aac_slave_configure(struct scsi_device *sdev)
 {
+       struct aac_dev *aac = (struct aac_dev *)sdev->host->hostdata;
        if ((sdev->type == TYPE_DISK) &&
-                       (sdev_channel(sdev) != CONTAINER_CHANNEL)) {
+                       (sdev_channel(sdev) != CONTAINER_CHANNEL) &&
+                       (!aac->raid_scsi_mode || (sdev_channel(sdev) != 2))) {
                if (expose_physicals == 0)
                        return -ENXIO;
-               if (expose_physicals < 0) {
-                       struct aac_dev *aac =
-                               (struct aac_dev *)sdev->host->hostdata;
-                       if (!aac->raid_scsi_mode || (sdev_channel(sdev) != 2))
-                               sdev->no_uld_attach = 1;
-               }
+               if (expose_physicals < 0)
+                       sdev->no_uld_attach = 1;
        }
        if (sdev->tagged_supported && (sdev->type == TYPE_DISK) &&
                        (sdev_channel(sdev) == CONTAINER_CHANNEL)) {
@@ -419,6 +417,7 @@
                unsigned num_lsu = 0;
                unsigned num_one = 0;
                unsigned depth;
+               unsigned cid;

                /*
                 * Firmware has an individual device recovery time typically
@@ -426,11 +425,15 @@
                 */
                if (sdev->timeout < (45 * HZ))
                        sdev->timeout = 45 * HZ;
+               for (cid = 0; cid < aac->maximum_num_containers; ++cid)
+                       if (aac->fsa_dev[cid].valid)
+                               ++num_lsu;
                __shost_for_each_device(dev, host) {
                        if (dev->tagged_supported && (dev->type == TYPE_DISK) &&

-                               (sdev_channel(dev) == CONTAINER_CHANNEL))
-                               ++num_lsu;
-                       else
+                               (sdev_channel(dev) == CONTAINER_CHANNEL)) {
+                               if (!aac->fsa_dev[sdev_id(dev)].valid)
+                                       ++num_lsu;
+                       } else
                                ++num_one;
                }
                if (num_lsu == 0)

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_queue_depth.patch --]
[-- Type: application/octet-stream, Size: 1685 bytes --]

diff -ru a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
--- a/drivers/scsi/aacraid/linit.c	2008-01-08 16:38:32.783320829 -0500
+++ b/drivers/scsi/aacraid/linit.c	2008-01-08 16:55:07.805050564 -0500
@@ -401,16 +401,14 @@
 
 static int aac_slave_configure(struct scsi_device *sdev)
 {
+	struct aac_dev *aac = (struct aac_dev *)sdev->host->hostdata;
 	if ((sdev->type == TYPE_DISK) &&
-			(sdev_channel(sdev) != CONTAINER_CHANNEL)) {
+			(sdev_channel(sdev) != CONTAINER_CHANNEL) &&
+			(!aac->raid_scsi_mode || (sdev_channel(sdev) != 2))) {
 		if (expose_physicals == 0)
 			return -ENXIO;
-		if (expose_physicals < 0) {
-			struct aac_dev *aac =
-				(struct aac_dev *)sdev->host->hostdata;
-			if (!aac->raid_scsi_mode || (sdev_channel(sdev) != 2))
-				sdev->no_uld_attach = 1;
-		}
+		if (expose_physicals < 0)
+			sdev->no_uld_attach = 1;
 	}
 	if (sdev->tagged_supported && (sdev->type == TYPE_DISK) &&
 			(sdev_channel(sdev) == CONTAINER_CHANNEL)) {
@@ -419,6 +417,7 @@
 		unsigned num_lsu = 0;
 		unsigned num_one = 0;
 		unsigned depth;
+		unsigned cid;
 
 		/*
 		 * Firmware has an individual device recovery time typically
@@ -426,11 +425,15 @@
 		 */
 		if (sdev->timeout < (45 * HZ))
 			sdev->timeout = 45 * HZ;
+		for (cid = 0; cid < aac->maximum_num_containers; ++cid)
+			if (aac->fsa_dev[cid].valid)
+				++num_lsu;
 		__shost_for_each_device(dev, host) {
 			if (dev->tagged_supported && (dev->type == TYPE_DISK) &&
-				(sdev_channel(dev) == CONTAINER_CHANNEL))
-				++num_lsu;
-			else
+				(sdev_channel(dev) == CONTAINER_CHANNEL)) {
+				if (!aac->fsa_dev[sdev_id(dev)].valid)
+					++num_lsu;
+			} else
 				++num_one;
 		}
 		if (num_lsu == 0)

Re: [PATCH 1/1] aacraid: big endian issues

[Grant Grundler]

On Jan 8, 2008 1:37 PM, Salyzyn, Mark <Mark_Salyzyn@adaptec.com> wrote:
> I've always assumed that byte swapping of constants would be optimized where a variable would not :-)

definitely...I understood that. It just seemed odd usage.

>
> I have confirmed in assembler output of the compiler that constant
> merely become byte reversed constants optimized or no in at least
> one architectural case.

Thanks!
And apologies, I didn't mean to cause you additional work since I
was just curious.

> I have *not* confirmed that a variable byte reversal requires processing overhead, as one could possibly expect the compiler to instead optimize by byte reversing the constant when comparing. However, I will guarantee you that if optimization is turned off in the compiler that such an optimization will not take place...

:)

>
> I do not think this turns into a readability issue in either case
> and view this as a simple cosmetic coding precaution much
> like likely()/unlikely() offers hints to the compiler on code intent...

Agreed - it's not a readability issue.

thanks,
grant

>
> Sincerely -- Mark Salyzyn
>
>
> > -----Original Message-----
> > From: Grant Grundler [mailto:grundler@google.com]
> > Sent: Tuesday, January 08, 2008 4:17 PM
> > To: Salyzyn, Mark
> > Cc: linux-scsi@vger.kernel.org
> > Subject: Re: [PATCH 1/1] aacraid: big endian issues
> >
> > On Jan 8, 2008 12:48 PM, Salyzyn, Mark
> > <Mark_Salyzyn@adaptec.com> wrote:
> > > Big endian systems issues discovered in the aacraid driver.
> >
> > ...
> > --- a/drivers/scsi/aacraid/comminit.c   2008-01-08
> > 15:32:28.329810853 -0500
> > +++ b/drivers/scsi/aacraid/comminit.c   2008-01-08
> > 15:37:35.633163607 -0500
> > @@ -301,10 +301,10 @@
> >         if ((!aac_adapter_sync_cmd(dev, GET_ADAPTER_PROPERTIES,
> >                 0, 0, 0, 0, 0, 0, status+0, status+1,
> > status+2, NULL, NULL)) &&
> >                         (status[0] == 0x00000001)) {
> > -               if (status[1] & AAC_OPT_NEW_COMM_64)
> > +               if (status[1] & le32_to_cpu(AAC_OPT_NEW_COMM_64))
> > ...
> >
> > Why apply le32_to_cpu() to the constant instead of the variable?
> > On systems were le32_to_cpu() is doing something, can gcc or
> > preprocessor optimize the constant?
> > I've always assumed it could not but that might be wrong.
> >
> > thanks,
> > grant
>

[PATCH 1/1] aacraid: respond to enclosure service events

[Salyzyn, Mark]

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

Feature enhancement.

Added support to respond to enclosure service events (controller AIFs) to add, online or offline physical targets reported to sg. Also added online and offlining of arrays. Removed an automatic variable definition in a sub block that hid an earlier definition, determined to be inert as the sub-block use did not interfere. Bumped the driver versioning to stamp the addition of this feature.

This attached patch is against current scsi-misc-2.6 *after* series of patches from past week are applied, as there is some overlap on the aacraid.h file.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments.

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/aacraid.h |    5 +-
 drivers/scsi/aacraid/commsup.c |   99 +++++++++++++++++++++++++++++++----------
 2 files changed, 79 insertions(+), 25 deletions(-)

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_enclosure.patch --]
[-- Type: application/octet-stream, Size: 5730 bytes --]

diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h	2008-01-11 10:48:23.944702435 -0500
+++ b/drivers/scsi/aacraid/aacraid.h	2008-01-11 10:58:54.547475323 -0500
@@ -12,7 +12,7 @@
  *----------------------------------------------------------------------------*/
 
 #ifndef AAC_DRIVER_BUILD
-# define AAC_DRIVER_BUILD 2449
+# define AAC_DRIVER_BUILD 2454
 # define AAC_DRIVER_BRANCH "-ms"
 #endif
 #define MAXIMUM_NUM_CONTAINERS	32
@@ -1772,6 +1772,9 @@
 #define			AifEnConfigChange	3	/* Adapter configuration change */
 #define			AifEnContainerChange	4	/* Container configuration change */
 #define			AifEnDeviceFailure	5	/* SCSI device failed */
+#define			AifEnEnclosureManagement 13	/* EM_DRIVE_* */
+#define				EM_DRIVE_INSERTION	31
+#define				EM_DRIVE_REMOVAL	32
 #define			AifEnBatteryEvent	14	/* Change in Battery State */
 #define			AifEnAddContainer	15	/* A new array was created */
 #define			AifEnDeleteContainer	16	/* A container was deleted */
diff -ru a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
--- a/drivers/scsi/aacraid/commsup.c	2008-01-11 10:48:23.919705616 -0500
+++ b/drivers/scsi/aacraid/commsup.c	2008-01-11 10:58:54.548475196 -0500
@@ -775,20 +775,20 @@
 {
 	struct hw_fib * hw_fib = fibptr->hw_fib_va;
 	struct aac_aifcmd * aifcmd = (struct aac_aifcmd *)hw_fib->data;
-	u32 container;
+	u32 channel, id, lun, container;
 	struct scsi_device *device;
 	enum {
 		NOTHING,
 		DELETE,
 		ADD,
 		CHANGE
-	} device_config_needed;
+	} device_config_needed = NOTHING;
 
 	/* Sniff for container changes */
 
 	if (!dev || !dev->fsa_dev)
 		return;
-	container = (u32)-1;
+	container = channel = id = lun = (u32)-1;
 
 	/*
 	 *	We have set this up to try and minimize the number of
@@ -901,6 +901,36 @@
 		case AifEnConfigChange:
 			break;
 
+		case AifEnEnclosureManagement:
+			switch (le32_to_cpu(((__le32 *)aifcmd->data)[3])) {
+			case EM_DRIVE_INSERTION:
+			case EM_DRIVE_REMOVAL:
+				container = le32_to_cpu(
+					((__le32 *)aifcmd->data)[2]);
+				if ((container >> 28))
+					break;
+				channel = (container >> 24) & 0xF;
+				if (channel >= dev->maximum_num_channels)
+					break;
+				id = container & 0xFFFF;
+				lun = (container >> 16) & 0xFF;
+				if (id >= dev->maximum_num_physicals) {
+					/* legacy dev_t ? */
+					if ((0x2000 <= id) || lun || channel ||
+					  ((channel = (id >> 7) & 0x3F) >=
+					  dev->maximum_num_channels))
+						break;
+					lun = (id >> 4) & 7;
+					id &= 0xF;
+				}
+				channel = aac_phys_to_logical(channel);
+				device_config_needed =
+				  (((__le32 *)aifcmd->data)[3]
+				    == cpu_to_le32(EM_DRIVE_INSERTION)) ?
+				  ADD : DELETE;
+				break;
+			}
+			break;
 		}
 
 		/*
@@ -969,7 +999,7 @@
 		break;
 	}
 
-	device_config_needed = NOTHING;
+	if (device_config_needed == NOTHING)
 	for (container = 0; container < dev->maximum_num_containers;
 	    ++container) {
 		if ((dev->fsa_dev[container].config_waiting_on == 0) &&
@@ -978,6 +1008,9 @@
 			device_config_needed =
 				dev->fsa_dev[container].config_needed;
 			dev->fsa_dev[container].config_needed = NOTHING;
+			channel = CONTAINER_TO_CHANNEL(container);
+			id = CONTAINER_TO_ID(container);
+			lun = CONTAINER_TO_LUN(container);
 			break;
 		}
 	}
@@ -1001,34 +1034,53 @@
 	/*
 	 * force reload of disk info via aac_probe_container
 	 */
-	if ((device_config_needed == CHANGE)
-	 && (dev->fsa_dev[container].valid == 1))
-		dev->fsa_dev[container].valid = 2;
-	if ((device_config_needed == CHANGE) ||
-			(device_config_needed == ADD))
+	if ((channel == CONTAINER_CHANNEL) &&
+	  (device_config_needed != NOTHING)) {
+		if (dev->fsa_dev[container].valid == 1)
+			dev->fsa_dev[container].valid = 2;
 		aac_probe_container(dev, container);
-	device = scsi_device_lookup(dev->scsi_host_ptr, 
-		CONTAINER_TO_CHANNEL(container), 
-		CONTAINER_TO_ID(container), 
-		CONTAINER_TO_LUN(container));
+	}
+	device = scsi_device_lookup(dev->scsi_host_ptr, channel, id, lun);
 	if (device) {
 		switch (device_config_needed) {
 		case DELETE:
+			if (scsi_device_online(device)) {
+				scsi_device_set_state(device, SDEV_OFFLINE);
+				sdev_printk(KERN_INFO, device,
+					"Device offlined - %s\n",
+					(channel == CONTAINER_CHANNEL) ?
+						"array deleted" :
+						"enclosure services event");
+			}
+			break;
+		case ADD:
+			if (!scsi_device_online(device)) {
+				sdev_printk(KERN_INFO, device,
+					"Device online - %s\n",
+					(channel == CONTAINER_CHANNEL) ?
+						"array created" :
+						"enclosure services event");
+				scsi_device_set_state(device, SDEV_RUNNING);
+			}
+			/* FALLTHRU */
 		case CHANGE:
+			if ((channel == CONTAINER_CHANNEL)
+			 && (!dev->fsa_dev[container].valid)) {
+				if (!scsi_device_online(device))
+					break;
+				scsi_device_set_state(device, SDEV_OFFLINE);
+				sdev_printk(KERN_INFO, device,
+					"Device offlined - %s\n",
+					"array failed");
+				break;
+			}
 			scsi_rescan_device(&device->sdev_gendev);
-
-		default:
-			break;
 		}
 		scsi_device_put(device);
+		device_config_needed = NOTHING;
 	}
-	if (device_config_needed == ADD) {
-		scsi_add_device(dev->scsi_host_ptr,
-		  CONTAINER_TO_CHANNEL(container),
-		  CONTAINER_TO_ID(container),
-		  CONTAINER_TO_LUN(container));
-	}
-
+	if (device_config_needed == ADD)
+		scsi_add_device(dev->scsi_host_ptr, channel, id, lun);
 }
 
 static int _aac_reset_adapter(struct aac_dev *aac, int forced)
@@ -1469,7 +1521,6 @@
 				*(__le32 *)hw_fib->data = cpu_to_le32(ST_OK);
 				aac_fib_adapter_complete(fib, (u16)sizeof(u32));
 			} else {
-				struct list_head *entry;
 				/* The u32 here is important and intended. We are using
 				   32bit wrapping time to fit the adapter field */
 				   

Re: [PATCH 1/1] aacraid: add call to flush_kernel_dcache_page for architectures that require it.

[James Bottomley]

On Tue, 2008-01-08 at 12:09 -0800, Salyzyn, Mark wrote:
> Some architectures require a call to flush_kernel_dcache_page for processor spoofed DMA operations.
> 
> This attached patch is against current scsi-misc-2.6.
> 
> ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments (inline gets damaged, use attachment).
> 
> Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>
> 
>  aachba.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> --- a/drivers/scsi/aacraid/aachba.c     2008-01-08 15:01:21.503932722 -0500
> +++ b/drivers/scsi/aacraid/aachba.c     2008-01-08 15:02:35.849634368 -0500
> @@ -31,9 +31,9 @@
>  #include <linux/slab.h>
>  #include <linux/completion.h>
>  #include <linux/blkdev.h>
> -#include <linux/dma-mapping.h>
>  #include <asm/semaphore.h>
>  #include <asm/uaccess.h>
> +#include <linux/highmem.h> /* For flush_kernel_dcache_page */
> 
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_cmnd.h>
> @@ -366,6 +366,9 @@
>         if (buf && transfer_len > 0)
>                 memcpy(buf + offset, data, transfer_len);
> 
> +#ifdef ARCH_HAS_FLUSH_ANON_PAGE
> +       flush_kernel_dcache_page(kmap_atomic_to_page(buf - sg->offset));
> +#endif

This #ifdef/#endif guard is unnecessary ... flush_kernel_dcache_page()
is available on all architectures (it's a nop on most).

However, ARCH_HAS_FLUSH_ANON_PAGE is the wrong guard anyway ... that's
for the flush_anon_page() function ...

James

[RFC] aacraid: driver feature flags?

[Salyzyn, Mark]

Feature enhancement.

Do we have a developing standard on how a driver may report to user tools via sysfs what features are supported or not by the driver? In the following I am proposing a 'flags' entry that will reside in the host controllers tree, with a newline separated list of arbitrary ascii named features that indicate whether the combination of driver and controller has support for said feature. Breaking from the one-line output typical of sysfs entries, newline was added to tailor for grep, or simple gets line by line string match within an application. I added one for a compiler time check for existence of debug print output, one for an optional manifest defined enhanced status reporting in the logs, and one for runtime reporting whether the controller and driver supports arrays larger than 2TB to my
  example below.

Suggestions?

This following patch is against current scsi-misc-2.6

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments, so one can NOT use the following to patch, it is only present to demonstrate the idea.

 drivers/scsi/aacraid/linit.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff -ru a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
--- a/drivers/scsi/aacraid/linit.c      2008-01-11 11:19:11.378159940 -0500
+++ b/drivers/scsi/aacraid/linit.c      2008-01-11 11:23:19.040145945 -0500
@@ -788,6 +788,23 @@
        return len;
 }

+static ssize_t aac_show_flags(struct class_device *class_dev, char *buf)
+{
+       int len = 0;
+       struct aac_dev *dev = (struct aac_dev*)class_to_shost(class_dev)->hostdata;
+
+       if (nblank(dprintk(x)))
+               len = snprintf(buf, PAGE_SIZE, "dprintk\n");
+#      ifdef AAC_DETAILED_STATUS_INFO
+               len += snprintf(buf + len, PAGE_SIZE - len,
+                 "AAC_DETAILED_STATUS_INFO\n");
+#      endif
+       if (dev->raw_io_interface && dev->raw_io_64)
+               len += snprintf(buf + len, PAGE_SIZE - len,
+                 "SAI_READ_CAPACITY_16\n");
+       return len;
+}
+
 static ssize_t aac_show_kernel_version(struct class_device *class_dev,
                char *buf)
 {
@@ -897,6 +914,13 @@
        },
        .show = aac_show_vendor,
 };
+static struct class_device_attribute aac_flags = {
+       .attr = {
+               .name = "flags",
+               .mode = S_IRUGO,
+       },
+       .show = aac_show_flags,
+};
 static struct class_device_attribute aac_kernel_version = {
        .attr = {
                .name = "hba_kernel_version",
@@ -951,6 +975,7 @@
 static struct class_device_attribute *aac_attrs[] = {
        &aac_model,
        &aac_vendor,
+       &aac_flags,
        &aac_kernel_version,
        &aac_monitor_version,
        &aac_bios_version,

[PATCH 1/1] aacraid: add call to flush_kernel_dcache_page for architectures that require it (take 2)

[Salyzyn, Mark]

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

The reason for the guard was for Linux Distributions that had limited their list of exported APIs on architectures that did *not* require the flush_kernel_dcache_page ... Sorry for not filtering out Distribution kernel issues into the list! :-)

Dropped guard in the enclosed patch.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments (inline gets damaged, use attachment).

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 aachba.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c     2008-01-11 14:38:53.342378372 -0500
+++ b/drivers/scsi/aacraid/aachba.c     2008-01-11 14:39:39.364534313 -0500
@@ -31,9 +31,9 @@
 #include <linux/slab.h>
 #include <linux/completion.h>
 #include <linux/blkdev.h>
-#include <linux/dma-mapping.h>
 #include <asm/semaphore.h>
 #include <asm/uaccess.h>
+#include <linux/highmem.h> /* For flush_kernel_dcache_page */

 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -366,6 +366,7 @@
        if (buf && transfer_len > 0)
                memcpy(buf + offset, data, transfer_len);

+       flush_kernel_dcache_page(kmap_atomic_to_page(buf - sg->offset));
        kunmap_atomic(buf - sg->offset, KM_IRQ0);

 }

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Friday, January 11, 2008 11:41 AM
> To: Salyzyn, Mark
> Cc: 'linux-scsi@vger.kernel.org'
> Subject: Re: [PATCH 1/1] aacraid: add call to
> flush_kernel_dcache_page for architectures that require it.
>
> On Tue, 2008-01-08 at 12:09 -0800, Salyzyn, Mark wrote:
> > Some architectures require a call to
> flush_kernel_dcache_page for processor spoofed DMA operations.
> >
> > This attached patch is against current scsi-misc-2.6.
> >
> > ObligatoryDisclaimer: Please accept my condolences
> regarding Outlook's handling of patch attachments (inline
> gets damaged, use attachment).
> >
> > Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>
> >
> >  aachba.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff -ru a/drivers/scsi/aacraid/aachba.c
> b/drivers/scsi/aacraid/aachba.c
> > --- a/drivers/scsi/aacraid/aachba.c     2008-01-08
> 15:01:21.503932722 -0500
> > +++ b/drivers/scsi/aacraid/aachba.c     2008-01-08
> 15:02:35.849634368 -0500
> > @@ -31,9 +31,9 @@
> >  #include <linux/slab.h>
> >  #include <linux/completion.h>
> >  #include <linux/blkdev.h>
> > -#include <linux/dma-mapping.h>
> >  #include <asm/semaphore.h>
> >  #include <asm/uaccess.h>
> > +#include <linux/highmem.h> /* For flush_kernel_dcache_page */
> >
> >  #include <scsi/scsi.h>
> >  #include <scsi/scsi_cmnd.h>
> > @@ -366,6 +366,9 @@
> >         if (buf && transfer_len > 0)
> >                 memcpy(buf + offset, data, transfer_len);
> >
> > +#ifdef ARCH_HAS_FLUSH_ANON_PAGE
> > +       flush_kernel_dcache_page(kmap_atomic_to_page(buf -
> sg->offset));
> > +#endif
>
> This #ifdef/#endif guard is unnecessary ... flush_kernel_dcache_page()
> is available on all architectures (it's a nop on most).
>
> However, ARCH_HAS_FLUSH_ANON_PAGE is the wrong guard anyway ... that's
> for the flush_anon_page() function ...

[-- Attachment #2: aacraid_flush_dcache4.patch --]
[-- Type: application/octet-stream, Size: 736 bytes --]

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c	2008-01-11 14:38:53.342378372 -0500
+++ b/drivers/scsi/aacraid/aachba.c	2008-01-11 14:39:39.364534313 -0500
@@ -31,9 +31,9 @@
 #include <linux/slab.h>
 #include <linux/completion.h>
 #include <linux/blkdev.h>
-#include <linux/dma-mapping.h>
 #include <asm/semaphore.h>
 #include <asm/uaccess.h>
+#include <linux/highmem.h> /* For flush_kernel_dcache_page */
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -366,6 +366,7 @@
 	if (buf && transfer_len > 0)
 		memcpy(buf + offset, data, transfer_len);
 
+	flush_kernel_dcache_page(kmap_atomic_to_page(buf - sg->offset));
 	kunmap_atomic(buf - sg->offset, KM_IRQ0);
 
 }

[PATCH 1/1] aacraid: respond to enclosure service events (take 2)

[Salyzyn, Mark]

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

The original patch removed a default: break from the switch, which ignited a warning message regarding an unhandled enum value. This enclosed patch leaves the default case in to suppress the warning.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments.

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/aacraid.h |    5 +-
 drivers/scsi/aacraid/commsup.c |   96 ++++++++++++++++++++++++++++++++---------
 2 files changed, 79 insertions(+), 22 deletions(-)

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Salyzyn, Mark
> Sent: Friday, January 11, 2008 11:15 AM
> To: 'linux-scsi@vger.kernel.org'
> Subject: [PATCH 1/1] aacraid: respond to enclosure service events
>
> Feature enhancement.
>
> Added support to respond to enclosure service events
> (controller AIFs) to add, online or offline physical targets
> reported to sg. Also added online and offlining of arrays.
> Removed an automatic variable definition in a sub block that
> hid an earlier definition, determined to be inert as the
> sub-block use did not interfere. Bumped the driver versioning
> to stamp the addition of this feature.
>
> This attached patch is against current scsi-misc-2.6 *after*
> series of patches from past week are applied, as there is
> some overlap on the aacraid.h file.
>
> ObligatoryDisclaimer: Please accept my condolences regarding
> Outlook's handling of patch attachments.
>
> Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>
>
>  drivers/scsi/aacraid/aacraid.h |    5 +-
>  drivers/scsi/aacraid/commsup.c |   99
> +++++++++++++++++++++++++++++++----------
>  2 files changed, 79 insertions(+), 25 deletions(-)
>
> Sincerely -- Mark Salyzyn
>

[-- Attachment #2: aacraid_enclosure2.patch --]
[-- Type: application/octet-stream, Size: 5730 bytes --]

diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h	2008-01-11 14:47:25.785233866 -0500
+++ b/drivers/scsi/aacraid/aacraid.h	2008-01-11 14:49:31.924183151 -0500
@@ -12,7 +12,7 @@
  *----------------------------------------------------------------------------*/
 
 #ifndef AAC_DRIVER_BUILD
-# define AAC_DRIVER_BUILD 2449
+# define AAC_DRIVER_BUILD 2454
 # define AAC_DRIVER_BRANCH "-ms"
 #endif
 #define MAXIMUM_NUM_CONTAINERS	32
@@ -1772,6 +1772,9 @@
 #define			AifEnConfigChange	3	/* Adapter configuration change */
 #define			AifEnContainerChange	4	/* Container configuration change */
 #define			AifEnDeviceFailure	5	/* SCSI device failed */
+#define			AifEnEnclosureManagement 13	/* EM_DRIVE_* */
+#define				EM_DRIVE_INSERTION	31
+#define				EM_DRIVE_REMOVAL	32
 #define			AifEnBatteryEvent	14	/* Change in Battery State */
 #define			AifEnAddContainer	15	/* A new array was created */
 #define			AifEnDeleteContainer	16	/* A container was deleted */
diff -ru a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
--- a/drivers/scsi/aacraid/commsup.c	2008-01-11 14:47:25.786233739 -0500
+++ b/drivers/scsi/aacraid/commsup.c	2008-01-11 14:49:31.926182897 -0500
@@ -775,20 +775,20 @@
 {
 	struct hw_fib * hw_fib = fibptr->hw_fib_va;
 	struct aac_aifcmd * aifcmd = (struct aac_aifcmd *)hw_fib->data;
-	u32 container;
+	u32 channel, id, lun, container;
 	struct scsi_device *device;
 	enum {
 		NOTHING,
 		DELETE,
 		ADD,
 		CHANGE
-	} device_config_needed;
+	} device_config_needed = NOTHING;
 
 	/* Sniff for container changes */
 
 	if (!dev || !dev->fsa_dev)
 		return;
-	container = (u32)-1;
+	container = channel = id = lun = (u32)-1;
 
 	/*
 	 *	We have set this up to try and minimize the number of
@@ -901,6 +901,36 @@
 		case AifEnConfigChange:
 			break;
 
+		case AifEnEnclosureManagement:
+			switch (le32_to_cpu(((__le32 *)aifcmd->data)[3])) {
+			case EM_DRIVE_INSERTION:
+			case EM_DRIVE_REMOVAL:
+				container = le32_to_cpu(
+					((__le32 *)aifcmd->data)[2]);
+				if ((container >> 28))
+					break;
+				channel = (container >> 24) & 0xF;
+				if (channel >= dev->maximum_num_channels)
+					break;
+				id = container & 0xFFFF;
+				lun = (container >> 16) & 0xFF;
+				if (id >= dev->maximum_num_physicals) {
+					/* legacy dev_t ? */
+					if ((0x2000 <= id) || lun || channel ||
+					  ((channel = (id >> 7) & 0x3F) >=
+					  dev->maximum_num_channels))
+						break;
+					lun = (id >> 4) & 7;
+					id &= 0xF;
+				}
+				channel = aac_phys_to_logical(channel);
+				device_config_needed =
+				  (((__le32 *)aifcmd->data)[3]
+				    == cpu_to_le32(EM_DRIVE_INSERTION)) ?
+				  ADD : DELETE;
+				break;
+			}
+			break;
 		}
 
 		/*
@@ -969,7 +999,7 @@
 		break;
 	}
 
-	device_config_needed = NOTHING;
+	if (device_config_needed == NOTHING)
 	for (container = 0; container < dev->maximum_num_containers;
 	    ++container) {
 		if ((dev->fsa_dev[container].config_waiting_on == 0) &&
@@ -978,6 +1008,9 @@
 			device_config_needed =
 				dev->fsa_dev[container].config_needed;
 			dev->fsa_dev[container].config_needed = NOTHING;
+			channel = CONTAINER_TO_CHANNEL(container);
+			id = CONTAINER_TO_ID(container);
+			lun = CONTAINER_TO_LUN(container);
 			break;
 		}
 	}
@@ -1001,34 +1034,56 @@
 	/*
 	 * force reload of disk info via aac_probe_container
 	 */
-	if ((device_config_needed == CHANGE)
-	 && (dev->fsa_dev[container].valid == 1))
-		dev->fsa_dev[container].valid = 2;
-	if ((device_config_needed == CHANGE) ||
-			(device_config_needed == ADD))
+	if ((channel == CONTAINER_CHANNEL) &&
+	  (device_config_needed != NOTHING)) {
+		if (dev->fsa_dev[container].valid == 1)
+			dev->fsa_dev[container].valid = 2;
 		aac_probe_container(dev, container);
-	device = scsi_device_lookup(dev->scsi_host_ptr, 
-		CONTAINER_TO_CHANNEL(container), 
-		CONTAINER_TO_ID(container), 
-		CONTAINER_TO_LUN(container));
+	}
+	device = scsi_device_lookup(dev->scsi_host_ptr, channel, id, lun);
 	if (device) {
 		switch (device_config_needed) {
 		case DELETE:
+			if (scsi_device_online(device)) {
+				scsi_device_set_state(device, SDEV_OFFLINE);
+				sdev_printk(KERN_INFO, device,
+					"Device offlined - %s\n",
+					(channel == CONTAINER_CHANNEL) ?
+						"array deleted" :
+						"enclosure services event");
+			}
+			break;
+		case ADD:
+			if (!scsi_device_online(device)) {
+				sdev_printk(KERN_INFO, device,
+					"Device online - %s\n",
+					(channel == CONTAINER_CHANNEL) ?
+						"array created" :
+						"enclosure services event");
+				scsi_device_set_state(device, SDEV_RUNNING);
+			}
+			/* FALLTHRU */
 		case CHANGE:
+			if ((channel == CONTAINER_CHANNEL)
+			 && (!dev->fsa_dev[container].valid)) {
+				if (!scsi_device_online(device))
+					break;
+				scsi_device_set_state(device, SDEV_OFFLINE);
+				sdev_printk(KERN_INFO, device,
+					"Device offlined - %s\n",
+					"array failed");
+				break;
+			}
 			scsi_rescan_device(&device->sdev_gendev);
 
 		default:
 			break;
 		}
 		scsi_device_put(device);
+		device_config_needed = NOTHING;
 	}
-	if (device_config_needed == ADD) {
-		scsi_add_device(dev->scsi_host_ptr,
-		  CONTAINER_TO_CHANNEL(container),
-		  CONTAINER_TO_ID(container),
-		  CONTAINER_TO_LUN(container));
-	}
-
+	if (device_config_needed == ADD)
+		scsi_add_device(dev->scsi_host_ptr, channel, id, lun);
 }
 
 static int _aac_reset_adapter(struct aac_dev *aac, int forced)
@@ -1469,7 +1524,6 @@
 				*(__le32 *)hw_fib->data = cpu_to_le32(ST_OK);
 				aac_fib_adapter_complete(fib, (u16)sizeof(u32));
 			} else {
-				struct list_head *entry;
 				/* The u32 here is important and intended. We are using
 				   32bit wrapping time to fit the adapter field */
 				   

Re: [PATCH 1/1] aacraid: add call to flush_kernel_dcache_page for architectures that require it (take 2)

[James Bottomley]

On Fri, 2008-01-11 at 11:46 -0800, Salyzyn, Mark wrote:
> The reason for the guard was for Linux Distributions that had limited
> their list of exported APIs on architectures that did *not* require
> the flush_kernel_dcache_page ... Sorry for not filtering out
> Distribution kernel issues into the list! :-)

That's OK, but you might want to update your distro patch.  The correct
guard check is ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE for them ...

> Dropped guard in the enclosed patch.


Thanks!

James

[PATCH 1/1] aacraid: SMC vendor identification

[Salyzyn, Mark]

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

Due to an internal limit associated with the AdapterTypeText field, SMC required a product ID that overloaded the combined vendor and product ID. A decision was made to ship the SMC products without a vendor string dropping the defacto space that used to delineate vendor and product to boot. To correct this, we needed to adjust the code in the driver to parse out the vendor and product strings for the adapter. We match of 'AOC' in the AdapterTypeText, if so we set the vendor to SMC and place the entire AdapterTypeText into the product field.

This only affects the cosmetic presentation of the Adapter vendor and product in the logs and in sysfs.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments, use the attachment, not the inline patch.

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/aachba.c |   25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c     2008-01-11 16:28:15.129189613 -0500
+++ b/drivers/scsi/aacraid/aachba.c     2008-01-11 16:31:22.032344801 -0500
@@ -716,16 +716,21 @@

        if (dev->supplement_adapter_info.AdapterTypeText[0]) {
                char * cp = dev->supplement_adapter_info.AdapterTypeText;
-               int c = sizeof(str->vid);
-               while (*cp && *cp != ' ' && --c)
-                       ++cp;
-               c = *cp;
-               *cp = '\0';
-               inqstrcpy (dev->supplement_adapter_info.AdapterTypeText,
-                 str->vid);
-               *cp = c;
-               while (*cp && *cp != ' ')
-                       ++cp;
+               int c;
+               if ((cp[0] == 'A') && (cp[1] == 'O') && (cp[2] == 'C'))
+                       inqstrcpy("SMC", str->vid);
+               else {
+                       c = sizeof(str->vid);
+                       while (*cp && *cp != ' ' && --c)
+                               ++cp;
+                       c = *cp;
+                       *cp = '\0';
+                       inqstrcpy (dev->supplement_adapter_info.AdapterTypeText,
+                         str->vid);
+                       *cp = c;
+                       while (*cp && *cp != ' ')
+                               ++cp;
+               }
                while (*cp == ' ')
                        ++cp;
                /* last six chars reserved for vol type */

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_SMC_vid.patch --]
[-- Type: application/octet-stream, Size: 1019 bytes --]

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c	2008-01-11 16:28:15.129189613 -0500
+++ b/drivers/scsi/aacraid/aachba.c	2008-01-11 16:31:22.032344801 -0500
@@ -716,16 +716,21 @@
 
 	if (dev->supplement_adapter_info.AdapterTypeText[0]) {
 		char * cp = dev->supplement_adapter_info.AdapterTypeText;
-		int c = sizeof(str->vid);
-		while (*cp && *cp != ' ' && --c)
-			++cp;
-		c = *cp;
-		*cp = '\0';
-		inqstrcpy (dev->supplement_adapter_info.AdapterTypeText,
-		  str->vid); 
-		*cp = c;
-		while (*cp && *cp != ' ')
-			++cp;
+		int c;
+		if ((cp[0] == 'A') && (cp[1] == 'O') && (cp[2] == 'C'))
+			inqstrcpy("SMC", str->vid);
+		else {
+			c = sizeof(str->vid);
+			while (*cp && *cp != ' ' && --c)
+				++cp;
+			c = *cp;
+			*cp = '\0';
+			inqstrcpy (dev->supplement_adapter_info.AdapterTypeText,
+			  str->vid); 
+			*cp = c;
+			while (*cp && *cp != ' ')
+				++cp;
+		}
 		while (*cp == ' ')
 			++cp;
 		/* last six chars reserved for vol type */

[PATCH 1/1] aacraid: fib context lock for management ioctls

[Salyzyn, Mark]

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

Alan noticed the lack of locking surrounding the driver's dealings with the fib context managed by the trio of ioctls that are used by the RAID management applications to retrieve Adapter Initiated FIBs. I merely expanded the fib lock to include the fib context. There have been no field reports of any issues generally because the applications are relatively static and do not come and go often enough to stress this area. I bloated this patch a little with some space junk.

This attached patch is against current scsi-misc-2.6.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments. The following inline patch is 'diff -rub' to pull out the space junk to enable convenient inspection, please use the attached file to patch.

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/commctrl.c |   29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff -rub a/commctrl.c b/commctrl.c
--- a/drivers/scsi/aacraid/commctrl.c        2008-01-24 09:44:33.080806785 -0500
+++ b/drivers/scsi/aacraid/commctrl.c        2008-01-24 09:50:41.071552674 -0500
@@ -243,6 +243,7 @@
         *      Search the list of AdapterFibContext addresses on the adapter
         *      to be sure this is a valid address
         */
+       spin_lock_irqsave(&dev->fib_lock, flags);
        entry = dev->fib_list.next;
        fibctx = NULL;

@@ -258,17 +259,18 @@
                fibctx = NULL;
        }
        if (!fibctx) {
+               spin_unlock_irqrestore(&dev->fib_lock, flags);
                dprintk ((KERN_INFO "Fib Context not found\n"));
                return -EINVAL;
        }

        if((fibctx->type != FSAFS_NTC_GET_ADAPTER_FIB_CONTEXT) ||
                 (fibctx->size != sizeof(struct aac_fib_context))) {
+               spin_unlock_irqrestore(&dev->fib_lock, flags);
                dprintk ((KERN_INFO "Fib Context corrupt?\n"));
                return -EINVAL;
        }
        status = 0;
-       spin_lock_irqsave(&dev->fib_lock, flags);
        /*
         *      If there are no fibs to send back, then either wait or return
         *      -EAGAIN
@@ -326,7 +328,9 @@
 int aac_close_fib_context(struct aac_dev * dev, struct aac_fib_context * fibctx)
 {
        struct fib *fib;
+       unsigned long flags;

+       spin_lock_irqsave(&dev->fib_lock, flags);
        /*
         *      First free any FIBs that have not been consumed.
         */
@@ -349,6 +353,7 @@
         *      Remove the Context from the AdapterFibContext List
         */
        list_del(&fibctx->next);
+       spin_unlock_irqrestore(&dev->fib_lock, flags);
        /*
         *      Invalidate context
         */

Sincerely - Mark Salyzyn

[-- Attachment #2: aacraid_fibctx_lock.patch --]
[-- Type: application/octet-stream, Size: 3464 bytes --]

diff -ru a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
--- a/drivers/scsi/aacraid/commctrl.c	2008-01-24 09:44:33.080806785 -0500
+++ b/drivers/scsi/aacraid/commctrl.c	2008-01-24 09:50:41.071552674 -0500
@@ -243,6 +243,7 @@
 	 *	Search the list of AdapterFibContext addresses on the adapter
 	 *	to be sure this is a valid address
 	 */
+	spin_lock_irqsave(&dev->fib_lock, flags);
 	entry = dev->fib_list.next;
 	fibctx = NULL;
 
@@ -251,24 +252,25 @@
 		/*
 		 *	Extract the AdapterFibContext from the Input parameters.
 		 */
-		if (fibctx->unique == f.fibctx) {   /* We found a winner */
+		if (fibctx->unique == f.fibctx) { /* We found a winner */
 			break;
 		}
 		entry = entry->next;
 		fibctx = NULL;
 	}
 	if (!fibctx) {
+		spin_unlock_irqrestore(&dev->fib_lock, flags);
 		dprintk ((KERN_INFO "Fib Context not found\n"));
 		return -EINVAL;
 	}
 
 	if((fibctx->type != FSAFS_NTC_GET_ADAPTER_FIB_CONTEXT) ||
 		 (fibctx->size != sizeof(struct aac_fib_context))) {
+		spin_unlock_irqrestore(&dev->fib_lock, flags);
 		dprintk ((KERN_INFO "Fib Context corrupt?\n"));
 		return -EINVAL;
 	}
 	status = 0;
-	spin_lock_irqsave(&dev->fib_lock, flags);
 	/*
 	 *	If there are no fibs to send back, then either wait or return
 	 *	-EAGAIN
@@ -326,7 +328,9 @@
 int aac_close_fib_context(struct aac_dev * dev, struct aac_fib_context * fibctx)
 {
 	struct fib *fib;
+	unsigned long flags;
 
+	spin_lock_irqsave(&dev->fib_lock, flags);
 	/*
 	 *	First free any FIBs that have not been consumed.
 	 */
@@ -349,6 +353,7 @@
 	 *	Remove the Context from the AdapterFibContext List
 	 */
 	list_del(&fibctx->next);
+	spin_unlock_irqrestore(&dev->fib_lock, flags);
 	/*
 	 *	Invalidate context
 	 */
@@ -414,8 +419,8 @@
  *	@arg: ioctl arguments
  *
  *	This routine returns the driver version.
- *      Under Linux, there have been no version incompatibilities, so this is
- *      simple!
+ *	Under Linux, there have been no version incompatibilities, so this is
+ *	simple!
  */
 
 static int check_revision(struct aac_dev *dev, void __user *arg)
@@ -463,7 +468,7 @@
 	u32 data_dir;
 	void __user *sg_user[32];
 	void *sg_list[32];
-	u32   sg_indx = 0;
+	u32 sg_indx = 0;
 	u32 byte_count = 0;
 	u32 actual_fibsize64, actual_fibsize = 0;
 	int i;
@@ -517,11 +522,11 @@
 	// Fix up srb for endian and force some values
 
 	srbcmd->function = cpu_to_le32(SRBF_ExecuteScsi);	// Force this
-	srbcmd->channel  = cpu_to_le32(user_srbcmd->channel);
+	srbcmd->channel	 = cpu_to_le32(user_srbcmd->channel);
 	srbcmd->id	 = cpu_to_le32(user_srbcmd->id);
-	srbcmd->lun      = cpu_to_le32(user_srbcmd->lun);
-	srbcmd->timeout  = cpu_to_le32(user_srbcmd->timeout);
-	srbcmd->flags    = cpu_to_le32(flags);
+	srbcmd->lun	 = cpu_to_le32(user_srbcmd->lun);
+	srbcmd->timeout	 = cpu_to_le32(user_srbcmd->timeout);
+	srbcmd->flags	 = cpu_to_le32(flags);
 	srbcmd->retry_limit = 0; // Obsolete parameter
 	srbcmd->cdb_size = cpu_to_le32(user_srbcmd->cdb_size);
 	memcpy(srbcmd->cdb, user_srbcmd->cdb, sizeof(srbcmd->cdb));
@@ -786,9 +791,9 @@
 	pci_info.bus = dev->pdev->bus->number;
 	pci_info.slot = PCI_SLOT(dev->pdev->devfn);
 
-       if (copy_to_user(arg, &pci_info, sizeof(struct aac_pci_info))) {
-	       dprintk((KERN_DEBUG "aacraid: Could not copy pci info\n"));
-	       return -EFAULT;
+	if (copy_to_user(arg, &pci_info, sizeof(struct aac_pci_info))) {
+		dprintk((KERN_DEBUG "aacraid: Could not copy pci info\n"));
+		return -EFAULT;
 	}
 	return 0;
 }

[PATCH 1/1] aacraid: fib context lock for management ioctls (take 2)

[Salyzyn, Mark]

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

The first patch was a bit too aggressive and nested the locks (!) unit testing was in error.

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

 drivers/scsi/aacraid/commctrl.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Salyzyn, Mark
> Sent: Thursday, January 24, 2008 10:40 AM
> To: 'linux-scsi@vger.kernel.org'
> Cc: 'Alan Cox'; 'Alan Cox'
> Subject: [PATCH 1/1] aacraid: fib context lock for management ioctls
>
> Alan noticed the lack of locking surrounding the driver's
> dealings with the fib context managed by the trio of ioctls
> that are used by the RAID management applications to retrieve
> Adapter Initiated FIBs. I merely expanded the fib lock to
> include the fib context. There have been no field reports of
> any issues generally because the applications are relatively
> static and do not come and go often enough to stress this
> area. I bloated this patch a little with some space junk.
>
> This attached patch is against current scsi-misc-2.6.
>
> ObligatoryDisclaimer: Please accept my condolences regarding
> Outlook's handling of patch attachments. The following inline
> patch is 'diff -rub' to pull out the space junk to enable
> convenient inspection, please use the attached file to patch.
>
> Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>
>
>  drivers/scsi/aacraid/commctrl.c |   29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff -rub a/commctrl.c b/commctrl.c
> --- a/drivers/scsi/aacraid/commctrl.c        2008-01-24
> 09:44:33.080806785 -0500
> +++ b/drivers/scsi/aacraid/commctrl.c        2008-01-24
> 09:50:41.071552674 -0500
> @@ -243,6 +243,7 @@
>          *      Search the list of AdapterFibContext
> addresses on the adapter
>          *      to be sure this is a valid address
>          */
> +       spin_lock_irqsave(&dev->fib_lock, flags);
>         entry = dev->fib_list.next;
>         fibctx = NULL;
>
> @@ -258,17 +259,18 @@
>                 fibctx = NULL;
>         }
>         if (!fibctx) {
> +               spin_unlock_irqrestore(&dev->fib_lock, flags);
>                 dprintk ((KERN_INFO "Fib Context not found\n"));
>                 return -EINVAL;
>         }
>
>         if((fibctx->type != FSAFS_NTC_GET_ADAPTER_FIB_CONTEXT) ||
>                  (fibctx->size != sizeof(struct aac_fib_context))) {
> +               spin_unlock_irqrestore(&dev->fib_lock, flags);
>                 dprintk ((KERN_INFO "Fib Context corrupt?\n"));
>                 return -EINVAL;
>         }
>         status = 0;
> -       spin_lock_irqsave(&dev->fib_lock, flags);
>         /*
>          *      If there are no fibs to send back, then
> either wait or return
>          *      -EAGAIN

Here is the offending portion of the previous patch (two chunks):

> @@ -326,7 +328,9 @@
>  int aac_close_fib_context(struct aac_dev * dev, struct
> aac_fib_context * fibctx)
>  {
>         struct fib *fib;
> +       unsigned long flags;
>
> +       spin_lock_irqsave(&dev->fib_lock, flags);
>         /*
>          *      First free any FIBs that have not been consumed.
>          */
> @@ -349,6 +353,7 @@
>          *      Remove the Context from the AdapterFibContext List
>          */
>         list_del(&fibctx->next);
> +       spin_unlock_irqrestore(&dev->fib_lock, flags);
>         /*
>          *      Invalidate context
>          */
>
> Sincerely - Mark Salyzyn

[-- Attachment #2: aacraid_fibctx_lock2.patch --]
[-- Type: application/octet-stream, Size: 3013 bytes --]

diff -ru a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
--- a/drivers/scsi/aacraid/commctrl.c	2008-01-28 15:09:42.556855325 -0500
+++ b/drivers/scsi/aacraid/commctrl.c	2008-01-28 15:10:59.115001866 -0500
@@ -243,6 +243,7 @@
 	 *	Search the list of AdapterFibContext addresses on the adapter
 	 *	to be sure this is a valid address
 	 */
+	spin_lock_irqsave(&dev->fib_lock, flags);
 	entry = dev->fib_list.next;
 	fibctx = NULL;
 
@@ -251,24 +252,25 @@
 		/*
 		 *	Extract the AdapterFibContext from the Input parameters.
 		 */
-		if (fibctx->unique == f.fibctx) {   /* We found a winner */
+		if (fibctx->unique == f.fibctx) { /* We found a winner */
 			break;
 		}
 		entry = entry->next;
 		fibctx = NULL;
 	}
 	if (!fibctx) {
+		spin_unlock_irqrestore(&dev->fib_lock, flags);
 		dprintk ((KERN_INFO "Fib Context not found\n"));
 		return -EINVAL;
 	}
 
 	if((fibctx->type != FSAFS_NTC_GET_ADAPTER_FIB_CONTEXT) ||
 		 (fibctx->size != sizeof(struct aac_fib_context))) {
+		spin_unlock_irqrestore(&dev->fib_lock, flags);
 		dprintk ((KERN_INFO "Fib Context corrupt?\n"));
 		return -EINVAL;
 	}
 	status = 0;
-	spin_lock_irqsave(&dev->fib_lock, flags);
 	/*
 	 *	If there are no fibs to send back, then either wait or return
 	 *	-EAGAIN
@@ -414,8 +416,8 @@
  *	@arg: ioctl arguments
  *
  *	This routine returns the driver version.
- *      Under Linux, there have been no version incompatibilities, so this is
- *      simple!
+ *	Under Linux, there have been no version incompatibilities, so this is
+ *	simple!
  */
 
 static int check_revision(struct aac_dev *dev, void __user *arg)
@@ -463,7 +465,7 @@
 	u32 data_dir;
 	void __user *sg_user[32];
 	void *sg_list[32];
-	u32   sg_indx = 0;
+	u32 sg_indx = 0;
 	u32 byte_count = 0;
 	u32 actual_fibsize64, actual_fibsize = 0;
 	int i;
@@ -517,11 +519,11 @@
 	// Fix up srb for endian and force some values
 
 	srbcmd->function = cpu_to_le32(SRBF_ExecuteScsi);	// Force this
-	srbcmd->channel  = cpu_to_le32(user_srbcmd->channel);
+	srbcmd->channel	 = cpu_to_le32(user_srbcmd->channel);
 	srbcmd->id	 = cpu_to_le32(user_srbcmd->id);
-	srbcmd->lun      = cpu_to_le32(user_srbcmd->lun);
-	srbcmd->timeout  = cpu_to_le32(user_srbcmd->timeout);
-	srbcmd->flags    = cpu_to_le32(flags);
+	srbcmd->lun	 = cpu_to_le32(user_srbcmd->lun);
+	srbcmd->timeout	 = cpu_to_le32(user_srbcmd->timeout);
+	srbcmd->flags	 = cpu_to_le32(flags);
 	srbcmd->retry_limit = 0; // Obsolete parameter
 	srbcmd->cdb_size = cpu_to_le32(user_srbcmd->cdb_size);
 	memcpy(srbcmd->cdb, user_srbcmd->cdb, sizeof(srbcmd->cdb));
@@ -786,9 +788,9 @@
 	pci_info.bus = dev->pdev->bus->number;
 	pci_info.slot = PCI_SLOT(dev->pdev->devfn);
 
-       if (copy_to_user(arg, &pci_info, sizeof(struct aac_pci_info))) {
-	       dprintk((KERN_DEBUG "aacraid: Could not copy pci info\n"));
-	       return -EFAULT;
+	if (copy_to_user(arg, &pci_info, sizeof(struct aac_pci_info))) {
+		dprintk((KERN_DEBUG "aacraid: Could not copy pci info\n"));
+		return -EFAULT;
 	}
 	return 0;
 }