sense buffer not cleaned when retrying failed command by the eh

DISCUSSION Saeed Bishara 3 participants 4 messages Jan 12, 2004 – Jan 13, 2004 SCSI

Share & tools

Saeed Bishara

author

2004-01-12 11:51 UTC #0 lore raw

Hello,
when the scsi error handler(eh) retries failed command after
resetting the bus and sending test unit ready, it doesn't clean the sense buffer. in my case the test unit ready completed successfully but with check condition (reporting bus reset and parameters changed), after then, the eh retried the failed command, and it was completed successfully without check condition, but since the sense buffer was not cleaned, the eh thinks that the sense data for the retried command was valid!
the failed command I'm talking about it the INQUIRY that sent by
scsi scan, so due the problem described above, the scsi scan decided to but the device offline!
I've fixed the problem in my LLD by cleaning the sense buffer when
there is no sense data to report, but I do think this should be done by the mid layer.

I'm using kernel 2.4.20

Signature · 8 lines

Saeed Bishara
Marvell Semiconductor Israel Ltd
Moshav Manof, D.N. Misgav 20184, ISRAEL
Email    -  [email protected]
Tel      -  + 972 4 9991014
Cell     -  + 972 67 619030
Fax      -  + 972 4 9999574
WWW Page:     http://www.marvell.com

Willem Riede

2004-01-13 00:27 UTC #1 lore raw

Quoted 16 lines · Saeed Bishara

Hello,
when the scsi error handler(eh) retries failed command after
resetting the bus and sending test unit ready, it doesn't clean the sense buffer. in my case the test unit ready completed successfully but with check condition (reporting bus reset and parameters changed), after then, the eh retried the failed command, and it was completed successfully without check condition, but since the sense buffer was not cleaned, the eh thinks that the sense data for the retried command was valid!
the failed command I'm talking about it the INQUIRY that sent by
scsi scan, so due the problem described above, the scsi scan decided to but the device offline!
I've fixed the problem in my LLD by cleaning the sense buffer when
there is no sense data to report, but I do think this should be done by the mid layer.

Quoted 1 line

I'm using kernel 2.4.20

What about in the 2.6 kernel:

scsi/scsi_lib.c renamed from scsi/scsi_lib.c.orig +1 -0

@@ -302,6 +302,7 @@
 void scsi_setup_cmd_retry(struct scsi_cmnd *cmd)
 {
 	memcpy(cmd->cmnd, cmd->data_cmnd, sizeof(cmd->data_cmnd));
+	memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
 	cmd->request_buffer = cmd->buffer;
 	cmd->request_bufflen = cmd->bufflen;
 	cmd->use_sg = cmd->old_use_sg;

and in the 2.4 kernel:

scsi/scsi_lib.c renamed from scsi/scsi_lib.c.orig +1 -0

@@ -223,6 +223,7 @@
 {
 	memcpy((void *) SCpnt->cmnd, (void *) SCpnt->data_cmnd,
 	       sizeof(SCpnt->data_cmnd));
+	memset(SCpnt->sense_buffer, 0, sizeof(SCpnt->sense_buffer));
 	SCpnt->request_buffer = SCpnt->buffer;
 	SCpnt->request_bufflen = SCpnt->bufflen;
 	SCpnt->use_sg = SCpnt->old_use_sg;

Regards, Willem Riede.

Mike Anderson

2004-01-13 07:36 UTC #2 lore raw

Quoted 7 lines · Willem Riede [[email protected]] wrote:

On 2004.01.12 06:50, Saeed Bishara wrote:
What about in the 2.6 kernel:

scsi/scsi_lib.c line 308

Quoted 7 lines

scsi/scsi_lib.c renamed from scsi/scsi_lib.c.orig +1 -0

@@ -302,6 +302,7 @@
 void scsi_setup_cmd_retry(struct scsi_cmnd *cmd)
 {
 	memcpy(cmd->cmnd, cmd->data_cmnd, sizeof(cmd->data_cmnd));
+	memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
 	cmd->request_buffer = cmd->buffer;
 	cmd->request_bufflen = cmd->bufflen;
 	cmd->use_sg = cmd->old_use_sg;

In the 2.6 kernel this is cleared in scsi_retry_command. The other callers of scsi_setup_cmd_retry do not want the sense_buffer cleared as it has been set prior to calling scsi_setup_cmd_retry. To use the change suggested things would need to be reordered a bit.

-andmike
--
Michael Anderson
[email protected]

Willem Riede

2004-01-13 22:56 UTC #3 lore raw

Quoted 4 lines · Mike Anderson

Quoted 3 lines · Willem Riede [[email protected]] wrote:
On 2004.01.12 06:50, Saeed Bishara wrote:
What about in the 2.6 kernel:

Quoted 13 lines

scsi/scsi_lib.c line 308

Quoted 11 lines

scsi/scsi_lib.c renamed from scsi/scsi_lib.c.orig +1 -0

@@ -302,6 +302,7 @@
 void scsi_setup_cmd_retry(struct scsi_cmnd *cmd)
 {
 	memcpy(cmd->cmnd, cmd->data_cmnd, sizeof(cmd->data_cmnd));
+	memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
 	cmd->request_buffer = cmd->buffer;
 	cmd->request_bufflen = cmd->bufflen;
 	cmd->use_sg = cmd->old_use_sg;

In the 2.6 kernel this is cleared in scsi_retry_command.

So it is. But scsi_retry_command is not called by the error handler -- not referenced from scsi_error.c at all. And the memset to zero could trivially be dropped from scsi_retry_command.

Quoted 3 lines

The other
callers of scsi_setup_cmd_retry do not want the sense_buffer cleared as it has been set prior to calling scsi_setup_cmd_retry.

^^^

Set? To what? Is there any case where the sense buffer is supposed to reflect anything else than the result of the retry once it is completed?

If there is, I would not consider that good programming practice... If there isn't, then clearing the sense buffer before the retry is perfectly OK.

Regards, Willem Riede.