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:[EMAIL PROTECTED] 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 [EMAIL PROTECTED]
Index: linux-2.6/drivers/scsi/aacraid/aachba.c
===
--- linux-2.6.orig/drivers/scsi/aacraid/aachba.c2007-11-08
17:09:50.0 +0100
+++ linux-2.6/drivers/scsi/aacraid/aachba.c 2007-11-08 17:14:43.0
+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)(lba0x));
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)(lba0x));
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.0 +0100
+++ linux-2.6/drivers/scsi/aacraid/commsup.c2007-11-08 17:14:43.0
+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:
-
RE: [PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8
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:[EMAIL PROTECTED] 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:[EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8
Christoph Hellwig [mailto:[EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] aacraid: don't assign cpu_to_le32(int) to u8
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 [EMAIL PROTECTED] 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.c2007-11-07 10:35:16.603727464 -0500 +++ b/drivers/scsi/aacraid/commsup.c2007-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:[EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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(0x); - 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. aacraid_BlinkLED.patch Description: aacraid_BlinkLED.patch
