[PATCH] qla2xxx: Fix endianness of task management response code

[PATCH] qla2xxx: Fix endianness of task management response code

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

The qla2xxx firmware actually expects the task management response
code in a CTIO IOCB with SCSI status mode 1 to be in little-endian
byte order, ie the response code should be the first byte in the
sense_data[] array.  The old code erroneously byte-swapped the
response code, which puts it in the wrong place on the wire and leads
to initiators thinking every task management request succeeds (since
they see 0 in the byte where they look for the response code).

Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Arun Easi <arun.easi@qlogic.com>
Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/qla2xxx/qla_target.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 5b30132..41b74ba 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1403,7 +1403,7 @@ static void qlt_24xx_send_task_mgmt_ctio(struct scsi_qla_host *ha,
 	ctio->u.status1.scsi_status =
 	    __constant_cpu_to_le16(SS_RESPONSE_INFO_LEN_VALID);
 	ctio->u.status1.response_len = __constant_cpu_to_le16(8);
-	((uint32_t *)ctio->u.status1.sense_data)[0] = cpu_to_be32(resp_code);
+	ctio->u.status1.sense_data[0] = resp_code;
 
 	qla2x00_start_iocbs(ha, ha->req);
 }
-- 
1.7.10.4

Re: [PATCH] qla2xxx: Fix endianness of task management response code

[Nicholas A. Bellinger]

On Tue, 2012-09-18 at 15:10 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> The qla2xxx firmware actually expects the task management response
> code in a CTIO IOCB with SCSI status mode 1 to be in little-endian
> byte order, ie the response code should be the first byte in the
> sense_data[] array.  The old code erroneously byte-swapped the
> response code, which puts it in the wrong place on the wire and leads
> to initiators thinking every task management request succeeds (since
> they see 0 in the byte where they look for the response code).
> 
> Cc: Chad Dupuis <chad.dupuis@qlogic.com>
> Cc: Arun Easi <arun.easi@qlogic.com>
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---

Very nice catch here Roland!

>  drivers/scsi/qla2xxx/qla_target.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 5b30132..41b74ba 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -1403,7 +1403,7 @@ static void qlt_24xx_send_task_mgmt_ctio(struct scsi_qla_host *ha,
>  	ctio->u.status1.scsi_status =
>  	    __constant_cpu_to_le16(SS_RESPONSE_INFO_LEN_VALID);
>  	ctio->u.status1.response_len = __constant_cpu_to_le16(8);
> -	((uint32_t *)ctio->u.status1.sense_data)[0] = cpu_to_be32(resp_code);
> +	ctio->u.status1.sense_data[0] = resp_code;
>  
>  	qla2x00_start_iocbs(ha, ha->req);
>  }

Dropping this into queue for the moment, and will CC' to stable -> move
into for-next once Chad & Co. have a chance to give their ACK.

Thank you!

Re: [PATCH] qla2xxx: Fix endianness of task management response code

[Saurav Kashyap]

>From: Roland Dreier <roland@purestorage.com>
>
>The qla2xxx firmware actually expects the task management response
>code in a CTIO IOCB with SCSI status mode 1 to be in little-endian
>byte order, ie the response code should be the first byte in the
>sense_data[] array.  The old code erroneously byte-swapped the
>response code, which puts it in the wrong place on the wire and leads
>to initiators thinking every task management request succeeds (since
>they see 0 in the byte where they look for the response code).
>
>Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>Cc: Arun Easi <arun.easi@qlogic.com>
>Signed-off-by: Roland Dreier <roland@purestorage.com>
>---
> drivers/scsi/qla2xxx/qla_target.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_target.c
>b/drivers/scsi/qla2xxx/qla_target.c
>index 5b30132..41b74ba 100644
>--- a/drivers/scsi/qla2xxx/qla_target.c
>+++ b/drivers/scsi/qla2xxx/qla_target.c
>@@ -1403,7 +1403,7 @@ static void qlt_24xx_send_task_mgmt_ctio(struct
>scsi_qla_host *ha,
>       ctio->u.status1.scsi_status =
>           __constant_cpu_to_le16(SS_RESPONSE_INFO_LEN_VALID);
>       ctio->u.status1.response_len = __constant_cpu_to_le16(8);
>-      ((uint32_t *)ctio->u.status1.sense_data)[0] = cpu_to_be32(resp_code);
>+      ctio->u.status1.sense_data[0] = resp_code;
>
>       qla2x00_start_iocbs(ha, ha->req);
> }
>--
>1.7.10.4

Acked-by: Saurav Kashyap <saurav.kashyap@qlogic.com>

Thanks,
~Saurav


This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

Re: [PATCH] qla2xxx: Fix endianness of task management response code

[Saurav Kashyap]

Acked-by: Saurav Kashyap <saurav.kashyap@qlogic.com>

Thanks,
~Saurav

>On Tue, 2012-09-18 at 15:10 -0700, Roland Dreier wrote:
>> From: Roland Dreier <roland@purestorage.com>
>>
>> The qla2xxx firmware actually expects the task management response
>> code in a CTIO IOCB with SCSI status mode 1 to be in little-endian
>> byte order, ie the response code should be the first byte in the
>> sense_data[] array.  The old code erroneously byte-swapped the
>> response code, which puts it in the wrong place on the wire and leads
>> to initiators thinking every task management request succeeds (since
>> they see 0 in the byte where they look for the response code).
>>
>> Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>> Cc: Arun Easi <arun.easi@qlogic.com>
>> Signed-off-by: Roland Dreier <roland@purestorage.com>
>> ---
>
>Very nice catch here Roland!
>
>>  drivers/scsi/qla2xxx/qla_target.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/qla2xxx/qla_target.c
>>b/drivers/scsi/qla2xxx/qla_target.c
>> index 5b30132..41b74ba 100644
>> --- a/drivers/scsi/qla2xxx/qla_target.c
>> +++ b/drivers/scsi/qla2xxx/qla_target.c
>> @@ -1403,7 +1403,7 @@ static void qlt_24xx_send_task_mgmt_ctio(struct
>>scsi_qla_host *ha,
>>      ctio->u.status1.scsi_status =
>>          __constant_cpu_to_le16(SS_RESPONSE_INFO_LEN_VALID);
>>      ctio->u.status1.response_len = __constant_cpu_to_le16(8);
>> -    ((uint32_t *)ctio->u.status1.sense_data)[0] = cpu_to_be32(resp_code);
>> +    ctio->u.status1.sense_data[0] = resp_code;
>>
>>      qla2x00_start_iocbs(ha, ha->req);
>>  }
>
>Dropping this into queue for the moment, and will CC' to stable -> move
>into for-next once Chad & Co. have a chance to give their ACK.
>
>Thank you!
>
>--
>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
>


This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

Re: [PATCH] qla2xxx: Fix endianness of task management response code

[James Bottomley]

On Tue, 2012-09-18 at 15:10 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> The qla2xxx firmware actually expects the task management response
> code in a CTIO IOCB with SCSI status mode 1 to be in little-endian
> byte order, ie the response code should be the first byte in the
> sense_data[] array.

Is this also true on Big Endian Hardware?  Because the fix you have
assumes that the TIO IOCB with SCSI status mode 1 should be CPU
endian ... that doesn't look right since this is passed directly over
the PCI bus (and the PCI bus is little endian), so shouldn't the correct
fix be to replace cpu_to_be32 with cpu_to_le32?

James

>   The old code erroneously byte-swapped the
> response code, which puts it in the wrong place on the wire and leads
> to initiators thinking every task management request succeeds (since
> they see 0 in the byte where they look for the response code).
> 
> Cc: Chad Dupuis <chad.dupuis@qlogic.com>
> Cc: Arun Easi <arun.easi@qlogic.com>
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/scsi/qla2xxx/qla_target.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 5b30132..41b74ba 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -1403,7 +1403,7 @@ static void qlt_24xx_send_task_mgmt_ctio(struct scsi_qla_host *ha,
>  	ctio->u.status1.scsi_status =
>  	    __constant_cpu_to_le16(SS_RESPONSE_INFO_LEN_VALID);
>  	ctio->u.status1.response_len = __constant_cpu_to_le16(8);
> -	((uint32_t *)ctio->u.status1.sense_data)[0] = cpu_to_be32(resp_code);
> +	ctio->u.status1.sense_data[0] = resp_code;
>  
>  	qla2x00_start_iocbs(ha, ha->req);
>  }

Re: [PATCH] qla2xxx: Fix endianness of task management response code

[Roland Dreier]

On Wed, Sep 19, 2012 at 12:59 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> Is this also true on Big Endian Hardware?  Because the fix you have
> assumes that the TIO IOCB with SCSI status mode 1 should be CPU
> endian ... that doesn't look right since this is passed directly over
> the PCI bus (and the PCI bus is little endian), so shouldn't the correct
> fix be to replace cpu_to_be32 with cpu_to_le32?

After my patch the assignment is to a u8, and that byte is in the right
place in memory.  So I don't think there's any endianness bug.

 - R.

Re: [PATCH] qla2xxx: Fix endianness of task management response code

[James Bottomley]

On Wed, 2012-09-19 at 07:09 -0700, Roland Dreier wrote:
> On Wed, Sep 19, 2012 at 12:59 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > Is this also true on Big Endian Hardware?  Because the fix you have
> > assumes that the TIO IOCB with SCSI status mode 1 should be CPU
> > endian ... that doesn't look right since this is passed directly over
> > the PCI bus (and the PCI bus is little endian), so shouldn't the correct
> > fix be to replace cpu_to_be32 with cpu_to_le32?
> 
> After my patch the assignment is to a u8, and that byte is in the right
> place in memory.  So I don't think there's any endianness bug.

The data in status1 appears to get used a word at a time ... what about
the other three bytes you don't set; are they guaranteed to be zero? (in
which case this works, it just looks wrong from the way the thing is
used in the rest of the code).

James

Re: [PATCH] qla2xxx: Fix endianness of task management response code

[Roland Dreier]

On Fri, Sep 21, 2012 at 1:02 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> The data in status1 appears to get used a word at a time ... what about
> the other three bytes you don't set; are they guaranteed to be zero? (in
> which case this works, it just looks wrong from the way the thing is
> used in the rest of the code).

Yes, the structure comes from

        ctio = (struct ctio7_to_24xx *)qla2x00_alloc_iocbs(ha, NULL);

a few lines earlier, and that does:

	/* Prep packet */
	memset(pkt, 0, REQUEST_ENTRY_SIZE);

before

	return pkt;

Re: [PATCH] qla2xxx: Fix endianness of task management response code

[Nicholas A. Bellinger]

On Wed, 2012-09-19 at 00:43 -0500, Saurav Kashyap wrote:
> 
> >From: Roland Dreier <roland@purestorage.com>
> >
> >The qla2xxx firmware actually expects the task management response
> >code in a CTIO IOCB with SCSI status mode 1 to be in little-endian
> >byte order, ie the response code should be the first byte in the
> >sense_data[] array.  The old code erroneously byte-swapped the
> >response code, which puts it in the wrong place on the wire and leads
> >to initiators thinking every task management request succeeds (since
> >they see 0 in the byte where they look for the response code).
> >
> >Cc: Chad Dupuis <chad.dupuis@qlogic.com>
> >Cc: Arun Easi <arun.easi@qlogic.com>
> >Signed-off-by: Roland Dreier <roland@purestorage.com>
> >---
> > drivers/scsi/qla2xxx/qla_target.c |    2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/scsi/qla2xxx/qla_target.c
> >b/drivers/scsi/qla2xxx/qla_target.c
> >index 5b30132..41b74ba 100644
> >--- a/drivers/scsi/qla2xxx/qla_target.c
> >+++ b/drivers/scsi/qla2xxx/qla_target.c
> >@@ -1403,7 +1403,7 @@ static void qlt_24xx_send_task_mgmt_ctio(struct
> >scsi_qla_host *ha,
> >       ctio->u.status1.scsi_status =
> >           __constant_cpu_to_le16(SS_RESPONSE_INFO_LEN_VALID);
> >       ctio->u.status1.response_len = __constant_cpu_to_le16(8);
> >-      ((uint32_t *)ctio->u.status1.sense_data)[0] = cpu_to_be32(resp_code);
> >+      ctio->u.status1.sense_data[0] = resp_code;
> >
> >       qla2x00_start_iocbs(ha, ha->req);
> > }
> >--
> >1.7.10.4
> 
> Acked-by: Saurav Kashyap <saurav.kashyap@qlogic.com>
> 

Applied to target-pending/for-next with CC' to stable

Thanks Roland & Co!