qla2xxx: fix RSCN handling on big-endian systems

PATCH In review [email protected] 2 participants 7 messages Feb 19, 2007 – Feb 23, 2007

Subsystems (2)

SCSI QLogic QLA2XXX FC-SCSI driver

lore

Share & tools

lore

[email protected]

author

2007-02-19 18:19 UTC #0 lore raw

PATCH 1/1 qla2xxx: fix RSCN handling on big-endian systems

qla2xxx driver fails to handle RSCN events affecting area or domain due to an endian issue on big endian systems. This fixes the port_id_t structure on big endian systems.

Signed-off-by Malahal Naineni <[email protected]>

drivers/scsi/qla2xxx/qla_def.h +8 -5

diff -r c860739bb0f4 drivers/scsi/qla2xxx/qla_def.h
@@ -1478,14 +1478,17 @@ typedef union {
 	uint32_t b24 : 24;
 
 	struct {
-		uint8_t d_id[3];
-		uint8_t rsvd_1;
-	} r;
-
-	struct {
+#ifdef __BIG_ENDIAN
+		uint8_t domain;
+		uint8_t area;
+		uint8_t al_pa;
+#elif __LITTLE_ENDIAN
 		uint8_t al_pa;
 		uint8_t area;
 		uint8_t domain;
+#else
+#error "__BIG_ENDIAN or __LITTLE_ENDIAN must be defined!"
+#endif
 		uint8_t rsvd_1;
 	} b;
 } port_id_t;

Seokmann Ju

2007-02-21 23:25 UTC #1 lore raw

Quoted 3 lines · Malahal Naineni

qla2xxx driver fails to handle RSCN events affecting area or domain due to an endian issue on big endian systems. This fixes the port_id_t structure on big endian systems.

<NEED MORE INFORMATION>
Can you provide more details on the fails you are getting? In my opinion, those fields in the structure should not get affected by byte ordering.

Thank you,

Seokmann

Quoted 39 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]]
Sent: Monday, February 19, 2007 10:19 AM
To: [email protected]; Linux Driver
Subject: [PATCH] qla2xxx: fix RSCN handling on big-endian systems
qla2xxx driver fails to handle RSCN events affecting area or domain due to an endian issue on big endian systems. This fixes the port_id_t structure on big endian systems.
Signed-off-by Malahal Naineni <[email protected]>
diff -r c860739bb0f4 drivers/scsi/qla2xxx/qla_def.h
drivers/scsi/qla2xxx/qla_def.h +8 -5
@@ -1478,14 +1478,17 @@ typedef union {
 	uint32_t b24 : 24;
 
 	struct {
-		uint8_t d_id[3];
-		uint8_t rsvd_1;
-	} r;
-
-	struct {
+#ifdef __BIG_ENDIAN
+		uint8_t domain;
+		uint8_t area;
+		uint8_t al_pa;
+#elif __LITTLE_ENDIAN
 		uint8_t al_pa;
 		uint8_t area;
 		uint8_t domain;
+#else
+#error "__BIG_ENDIAN or __LITTLE_ENDIAN must be defined!"
+#endif
 		uint8_t rsvd_1;
 	} b;
 } port_id_t;

I have JBOD in FL-port and if I unplug the cable or disable the switch port, the qla2xxx driver doesn't fail the I/O soon. The remote port status is 'online' all the time. The I/O's usually timeout after the usual scsi timeout. Based on the existing structure, al_pa is at the lowest addressed byte, so the b24 field's interpretation as an integer is *incorrect* on big endinan systems. You should be able to print the b24 as an integer and see what you get is incorrect on big endian systems.

Thanks, Malahal.

Quoted 56 lines · Seokmann Ju [[email protected]] wrote:

Quoted 3 lines · Malahal Naineni
qla2xxx driver fails to handle RSCN events affecting area or domain due to an endian issue on big endian systems. This fixes the port_id_t structure on big endian systems.
<NEED MORE INFORMATION>
Can you provide more details on the fails you are getting? In my opinion, those fields in the structure should not get affected by byte ordering.
Thank you,
Seokmann
Quoted 29 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]]
Sent: Monday, February 19, 2007 10:19 AM
To: [email protected]; Linux Driver
Subject: [PATCH] qla2xxx: fix RSCN handling on big-endian systems
qla2xxx driver fails to handle RSCN events affecting area or domain due to an endian issue on big endian systems. This fixes the port_id_t structure on big endian systems.
Signed-off-by Malahal Naineni <[email protected]>
diff -r c860739bb0f4 drivers/scsi/qla2xxx/qla_def.h
drivers/scsi/qla2xxx/qla_def.h +4 -5
@@ -1478,14 +1478,17 @@ typedef union {
 	uint32_t b24 : 24;
 
 	struct {
-		uint8_t d_id[3];
-		uint8_t rsvd_1;
-	} r;
-
-	struct {
+#ifdef __BIG_ENDIAN
+		uint8_t domain;
+		uint8_t area;
+		uint8_t al_pa;
drivers/scsi/qla2xxx/qla_def.h typedef union { · line 1494
Quoted 10 lines
drivers/scsi/qla2xxx/qla_def.h +4 -0
+#elif __LITTLE_ENDIAN
 		uint8_t al_pa;
 		uint8_t area;
 		uint8_t domain;
+#else
+#error "__BIG_ENDIAN or __LITTLE_ENDIAN must be defined!"
+#endif
 		uint8_t rsvd_1;
 	} b;
 } port_id_t;
-
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

Seokmann Ju

2007-02-22 14:04 UTC #3 lore raw

Quoted 4 lines · Malahal Naineni

I have JBOD in FL-port and if I unplug the cable or disable the switch port, the qla2xxx driver doesn't fail the I/O soon. The remote port status is 'online' all the time. The I/O's usually timeout after the usual scsi timeout.

Yes, that is because qla2xxx doesn't want to trigger unnecessary error handling mechanism, which is expensive.
As long as State Change recovered within SCSI timeout period, driver won't let mid-layer know about it.

Quoted 6 lines

Based on
the existing structure, al_pa is at the lowest addressed byte, so the b24 field's interpretation as an integer is *incorrect* on big endinan systems. You should be able to print the b24 as an integer and see what you get is incorrect on big endian systems.

I agree on your assessment. However, the patch won't fix the problem as the problem itself is regardless byte ordering.
A patch which includes your evaluation will be submitted. Thank you for findings.

Seokmann

Quoted 84 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]]
Sent: Wednesday, February 21, 2007 6:54 PM
To: Seokmann Ju
Cc: [email protected]; Linux Driver
Subject: Re: [PATCH] qla2xxx: fix RSCN handling on big-endian systems
I have JBOD in FL-port and if I unplug the cable or disable the switch port, the qla2xxx driver doesn't fail the I/O soon. The remote port status is 'online' all the time. The I/O's usually timeout after the usual scsi timeout. Based on the existing structure, al_pa is at the lowest addressed byte, so the b24 field's interpretation as an integer is *incorrect* on big endinan systems. You should be able to print the b24 as an integer and see what you get is incorrect on big endian systems.
Thanks, Malahal.
Quoted 2 lines · Seokmann Ju [[email protected]] wrote:
Quoted 1 line · Malahal Naineni
qla2xxx driver fails to handle RSCN events affecting area
or domain
Quoted 5 lines
Quoted 2 lines
due to an endian issue on big endian systems. This fixes the port_id_t structure on big endian systems.
<NEED MORE INFORMATION>
Can you provide more details on the fails you are getting? In my opinion, those fields in the structure should not get
affected
Quoted 13 lines
by byte ordering.
Thank you,
Seokmann
Quoted 7 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]]
Sent: Monday, February 19, 2007 10:19 AM
To: [email protected]; Linux Driver
Subject: [PATCH] qla2xxx: fix RSCN handling on big-endian systems
qla2xxx driver fails to handle RSCN events affecting area
or domain
Quoted 7 lines
Quoted 7 lines
due to an endian issue on big endian systems. This fixes the port_id_t structure on big endian systems.
Signed-off-by Malahal Naineni <[email protected]>
diff -r c860739bb0f4 drivers/scsi/qla2xxx/qla_def.h
--- a/drivers/scsi/qla2xxx/qla_def.h Fri Feb 16
14:19:34 2007 -0800
Quoted 1 line
Quoted 1 line
+++ b/drivers/scsi/qla2xxx/qla_def.h Fri Feb 16
14:21:29 2007 -0800
Quoted 18 lines
Quoted 18 lines
drivers/scsi/qla2xxx/qla_def.h +6 -5
@@ -1478,14 +1478,17 @@ typedef union {
 	uint32_t b24 : 24;
 
 	struct {
-		uint8_t d_id[3];
-		uint8_t rsvd_1;
-	} r;
-
-	struct {
+#ifdef __BIG_ENDIAN
+		uint8_t domain;
+		uint8_t area;
+		uint8_t al_pa;
+#elif __LITTLE_ENDIAN
 		uint8_t al_pa;
 		uint8_t area;
 		uint8_t domain;
+#else
Quoted 8 lines
drivers/scsi/qla2xxx/qla_def.h typedef union { · line 1494
Quoted 6 lines
drivers/scsi/qla2xxx/qla_def.h +2 -0
+#error "__BIG_ENDIAN or __LITTLE_ENDIAN must be defined!"
+#endif
 		uint8_t rsvd_1;
 	} b;
 } port_id_t;
-
To unsubscribe from this list: send the line "unsubscribe
linux-scsi"
Quoted 1 line
in the body of a message to [email protected] More
majordomo
Quoted 1 line
info at http://vger.kernel.org/majordomo-info.html

[email protected]

author

2007-02-22 19:09 UTC #4 lore raw

Quoted 9 lines · Seokmann Ju [[email protected]] wrote:

Quoted 4 lines · Malahal Naineni
I have JBOD in FL-port and if I unplug the cable or disable the switch port, the qla2xxx driver doesn't fail the I/O soon. The remote port status is 'online' all the time. The I/O's usually timeout after the usual scsi timeout.
Yes, that is because qla2xxx doesn't want to trigger unnecessary error handling mechanism, which is expensive.
As long as State Change recovered within SCSI timeout period, driver won't let mid-layer know about it.

I removed the cable and didn't put it back at all for more than 1 hour. There is no recovery here. When I disable an F-port, after few seconds the remote port state is set to something other than 'online'. Why not same behaviour for remote ports on FL-port. With my patch, the behaviour is same whether the remote port is attached to an FL-port or an F-port.

Quoted 10 lines

Quoted 6 lines
Based on
the existing structure, al_pa is at the lowest addressed byte, so the b24 field's interpretation as an integer is *incorrect* on big endinan systems. You should be able to print the b24 as an integer and see what you get is incorrect on big endian systems.
I agree on your assessment. However, the patch won't fix the problem as the problem itself is regardless byte ordering.
A patch which includes your evaluation will be submitted. Thank you for findings.

Well, my patch fixed the problem! qla2x00_device_resync() fails to call qla2x00_mark_device_lost() for RSCN's affecting area or domain because the following evaluates to true always due to b24 field being incorrect:

		(fcport->d_id.b24 & mask) != d_id.b24

Thanks, Malahal.

Seokmann Ju

2007-02-22 19:43 UTC #5 lore raw

Quoted 7 lines · Malahal Naineni

I removed the cable and didn't put it back at all for more than 1 hour.
There is no recovery here. When I disable an F-port, after few seconds the remote port state is set to something other than 'online'. Why not same behaviour for remote ports on FL-port. With my patch, the behaviour is same whether the remote port is attached to an FL-port or an F-port.

I'm sorry, I mis-understood your initial comment. The problem is happening only with FL-port (on big-endian platform). Let me verify it with test team in here and will get back to you.

Quoted 7 lines

Well, my patch fixed the problem! qla2x00_device_resync() fails to call
qla2x00_mark_device_lost() for RSCN's affecting area or domain because the following evaluates to true always due to b24 field being incorrect:
		(fcport->d_id.b24 & mask) != d_id.b24

Agree, your patch will fix it. However, my point is that: instead of having #idfef in the header,
1. make access to the structure member to be equal in size.
2. or introduce macro that combins al_pa/area/domain Those are preferred solution, in my opinion.

Again, thank you for your findings.
Based on your findings, let us investigate further and come up with right answer for it.

Thank you,

Seokmann

Seokmann Ju

Acked-by on v1 1/1

2007-02-23 22:51 UTC #6 lore raw

In reply to [email protected]

ACK - Thank you for the findings.

Acked-by Seokmann Ju <[email protected]>

Quoted 39 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]]
Sent: Monday, February 19, 2007 10:19 AM
To: [email protected]; Linux Driver
Subject: [PATCH] qla2xxx: fix RSCN handling on big-endian systems
qla2xxx driver fails to handle RSCN events affecting area or domain due to an endian issue on big endian systems. This fixes the port_id_t structure on big endian systems.
Signed-off-by Malahal Naineni <[email protected]>
diff -r c860739bb0f4 drivers/scsi/qla2xxx/qla_def.h
drivers/scsi/qla2xxx/qla_def.h +8 -5
@@ -1478,14 +1478,17 @@ typedef union {
 	uint32_t b24 : 24;
 
 	struct {
-		uint8_t d_id[3];
-		uint8_t rsvd_1;
-	} r;
-
-	struct {
+#ifdef __BIG_ENDIAN
+		uint8_t domain;
+		uint8_t area;
+		uint8_t al_pa;
+#elif __LITTLE_ENDIAN
 		uint8_t al_pa;
 		uint8_t area;
 		uint8_t domain;
+#else
+#error "__BIG_ENDIAN or __LITTLE_ENDIAN must be defined!"
+#endif
 		uint8_t rsvd_1;
 	} b;
 } port_id_t;