[PATCH] qla2xxx: fix RSCN handling on big-endian systems

[PATCH] qla2xxx: fix RSCN handling on big-endian systems

[malahal]

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 <malahal@us.ibm.com>

diff -r c860739bb0f4 drivers/scsi/qla2xxx/qla_def.h
--- a/drivers/scsi/qla2xxx/qla_def.h	Fri Feb 16 14:19:34 2007 -0800
+++ b/drivers/scsi/qla2xxx/qla_def.h	Fri Feb 16 14:21:29 2007 -0800
@@ -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;

RE: [PATCH] qla2xxx: fix RSCN handling on big-endian systems

[Seokmann Ju]

On Monday, February 19, 2007 10:19 AM, Malahal Naineni wrote:
> 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

> -----Original Message-----
> From: malahal@us.ibm.com [mailto:malahal@us.ibm.com] 
> Sent: Monday, February 19, 2007 10:19 AM
> To: linux-scsi@vger.kernel.org; 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 <malahal@us.ibm.com>
> 
> diff -r c860739bb0f4 drivers/scsi/qla2xxx/qla_def.h
> --- a/drivers/scsi/qla2xxx/qla_def.h	Fri Feb 16 14:19:34 2007 -0800
> +++ b/drivers/scsi/qla2xxx/qla_def.h	Fri Feb 16 14:21:29 2007 -0800
> @@ -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;
> 

Re: [PATCH] qla2xxx: fix RSCN handling on big-endian systems

[malahal]

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.

Seokmann Ju [seokmann.ju@qlogic.com] wrote:
> On Monday, February 19, 2007 10:19 AM, Malahal Naineni wrote:
> > 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
> 
> > -----Original Message-----
> > From: malahal@us.ibm.com [mailto:malahal@us.ibm.com] 
> > Sent: Monday, February 19, 2007 10:19 AM
> > To: linux-scsi@vger.kernel.org; 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 <malahal@us.ibm.com>
> > 
> > diff -r c860739bb0f4 drivers/scsi/qla2xxx/qla_def.h
> > --- a/drivers/scsi/qla2xxx/qla_def.h	Fri Feb 16 14:19:34 2007 -0800
> > +++ b/drivers/scsi/qla2xxx/qla_def.h	Fri Feb 16 14:21:29 2007 -0800
> > @@ -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;
> > 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] qla2xxx: fix RSCN handling on big-endian systems

[Seokmann Ju]

On Wednesday, February 21, 2007 6:54 PM, Malahal Naineni wrote:
> 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.
> 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

> -----Original Message-----
> From: malahal@us.ibm.com [mailto:malahal@us.ibm.com] 
> Sent: Wednesday, February 21, 2007 6:54 PM
> To: Seokmann Ju
> Cc: linux-scsi@vger.kernel.org; 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.
> 
> Seokmann Ju [seokmann.ju@qlogic.com] wrote:
> > On Monday, February 19, 2007 10:19 AM, Malahal Naineni wrote:
> > > 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
> > 
> > > -----Original Message-----
> > > From: malahal@us.ibm.com [mailto:malahal@us.ibm.com]
> > > Sent: Monday, February 19, 2007 10:19 AM
> > > To: linux-scsi@vger.kernel.org; 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 <malahal@us.ibm.com>
> > > 
> > > diff -r c860739bb0f4 drivers/scsi/qla2xxx/qla_def.h
> > > --- a/drivers/scsi/qla2xxx/qla_def.h	Fri Feb 16 
> 14:19:34 2007 -0800
> > > +++ b/drivers/scsi/qla2xxx/qla_def.h	Fri Feb 16 
> 14:21:29 2007 -0800
> > > @@ -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;
> > > 
> > -
> > To unsubscribe from this list: send the line "unsubscribe 
> linux-scsi" 
> > in the body of a message to majordomo@vger.kernel.org More 
> majordomo 
> > info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] qla2xxx: fix RSCN handling on big-endian systems

[malahal]

Seokmann Ju [seokmann.ju@qlogic.com] wrote:
> On Wednesday, February 21, 2007 6:54 PM, Malahal Naineni wrote:
> > 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.

> > 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.

RE: [PATCH] qla2xxx: fix RSCN handling on big-endian systems

[Seokmann Ju]

On Thursday, February 22, 2007 11:09 AM, Malahal Naineni wrote:
> 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.

> 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

RE: [PATCH] qla2xxx: fix RSCN handling on big-endian systems

[Seokmann Ju]

ACK - Thank you for the findings.

Acked-by: Seokmann Ju <seokmann.ju@qlogic.com>

> -----Original Message-----
> From: malahal@us.ibm.com [mailto:malahal@us.ibm.com] 
> Sent: Monday, February 19, 2007 10:19 AM
> To: linux-scsi@vger.kernel.org; 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 <malahal@us.ibm.com>
> 
> diff -r c860739bb0f4 drivers/scsi/qla2xxx/qla_def.h
> --- a/drivers/scsi/qla2xxx/qla_def.h	Fri Feb 16 14:19:34 2007 -0800
> +++ b/drivers/scsi/qla2xxx/qla_def.h	Fri Feb 16 14:21:29 2007 -0800
> @@ -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;
>