sense buffer not cleaned when retrying failed command by the eh

sense buffer not cleaned when retrying failed command by the eh

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

I'm using kernel 2.4.20

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

Re: sense buffer not cleaned when retrying failed command by the eh

[Willem Riede]

On 2004.01.12 06:50, Saeed Bishara wrote:
> 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

What about in the 2.6 kernel:

--- drivers/scsi/scsi_lib.c.orig	2004-01-12 18:40:28.000000000 -0500
+++ drivers/scsi/scsi_lib.c	2004-01-12 18:50:22.000000000 -0500
@@ -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:

--- drivers/scsi/scsi_lib.c.orig	2003-12-01 08:27:56.000000000 -0500
+++ drivers/scsi/scsi_lib.c	2004-01-12 18:49:55.000000000 -0500
@@ -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.

Re: sense buffer not cleaned when retrying failed command by the eh

[Mike Anderson]

Willem Riede [wrlk@riede.org] wrote:
> On 2004.01.12 06:50, Saeed Bishara wrote:
> What about in the 2.6 kernel:
> 
> --- drivers/scsi/scsi_lib.c.orig	2004-01-12 18:40:28.000000000 -0500
> +++ drivers/scsi/scsi_lib.c	2004-01-12 18:50:22.000000000 -0500
> @@ -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
andmike@us.ibm.com

Re: sense buffer not cleaned when retrying failed command by the eh

[Willem Riede]

On 2004.01.13 02:40, Mike Anderson wrote:
> Willem Riede [wrlk@riede.org] wrote:
> > On 2004.01.12 06:50, Saeed Bishara wrote:
> > What about in the 2.6 kernel:
> > 
> > --- drivers/scsi/scsi_lib.c.orig	2004-01-12 18:40:28.000000000 -0500
> > +++ drivers/scsi/scsi_lib.c	2004-01-12 18:50:22.000000000 -0500
> > @@ -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.

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