[PATCH] SCSI: detect missing data for INQUIRY

[PATCH] SCSI: detect missing data for INQUIRY

[Alan Stern]

This patch (as1154) fixes a problem in scsi_probe_lun(): The function
doesn't check whether the device has actually sent back any INQUIRY
data!  The patch adds a test to see if the result buffer is still
empty after the command has been executed.

This enables the Thecus N2050 storage device to work.  The firmware on
that device starts up strangely; it sends no data in response to the
initial INQUIRY, and it sends the INQUIRY information in response to
the followup REQUEST SENSE!  But after that it works better, so
retrying the INQUIRY is enough to get it going.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

This should get into 2.6.28.


Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -602,6 +602,13 @@ static int scsi_probe_lun(struct scsi_de
 				    (sshdr.ascq == 0))
 					continue;
 			}
+		} else {
+			/*
+			 * Additional Length and Vendor fields missing
+			 * probably means nothing was transferred.  Try again.
+			 */
+			if (inq_result[4] == 0 && inq_result[8] == 0)
+				continue;
 		}
 		break;
 	}

Re: [PATCH] SCSI: detect missing data for INQUIRY

[James Bottomley]

On Mon, 2008-10-27 at 12:18 -0400, Alan Stern wrote:
> This patch (as1154) fixes a problem in scsi_probe_lun(): The function
> doesn't check whether the device has actually sent back any INQUIRY
> data!  The patch adds a test to see if the result buffer is still
> empty after the command has been executed.
> 
> This enables the Thecus N2050 storage device to work.  The firmware on
> that device starts up strangely; it sends no data in response to the
> initial INQUIRY, and it sends the INQUIRY information in response to
> the followup REQUEST SENSE!  But after that it works better, so
> retrying the INQUIRY is enough to get it going.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> This should get into 2.6.28.
> 
> 
> Index: usb-2.6/drivers/scsi/scsi_scan.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/scsi_scan.c
> +++ usb-2.6/drivers/scsi/scsi_scan.c
> @@ -602,6 +602,13 @@ static int scsi_probe_lun(struct scsi_de
>  				    (sshdr.ascq == 0))
>  					continue;
>  			}
> +		} else {
> +			/*
> +			 * Additional Length and Vendor fields missing
> +			 * probably means nothing was transferred.  Try again.
> +			 */
> +			if (inq_result[4] == 0 && inq_result[8] == 0)
> +				continue;

Really, no.

A legitimate minimal response from a device is zero in the additional
length field.  If it does that, then the vendor field is bound to be
zero as well.

About the best we can do is check the first four fields.  For them to be
all zero, it would have to be a minimal response SCSI-1 device (RDF of
zero) ... hopefully they're all dead by now.

James

Re: [PATCH] SCSI: detect missing data for INQUIRY

[Alan Stern]

On Mon, 27 Oct 2008, James Bottomley wrote:

> > --- usb-2.6.orig/drivers/scsi/scsi_scan.c
> > +++ usb-2.6/drivers/scsi/scsi_scan.c
> > @@ -602,6 +602,13 @@ static int scsi_probe_lun(struct scsi_de
> >  				    (sshdr.ascq == 0))
> >  					continue;
> >  			}
> > +		} else {
> > +			/*
> > +			 * Additional Length and Vendor fields missing
> > +			 * probably means nothing was transferred.  Try again.
> > +			 */
> > +			if (inq_result[4] == 0 && inq_result[8] == 0)
> > +				continue;
> 
> Really, no.
> 
> A legitimate minimal response from a device is zero in the additional
> length field.  If it does that, then the vendor field is bound to be
> zero as well.
> 
> About the best we can do is check the first four fields.  For them to be
> all zero, it would have to be a minimal response SCSI-1 device (RDF of
> zero) ... hopefully they're all dead by now.

Then you're saying that the patch should everything up to
inq_result[8]?  Or maybe even beyond?  I can do that.  Would that be 
acceptable?

Alan Stern

Re: [PATCH] SCSI: detect missing data for INQUIRY

[James Bottomley]

On Mon, 2008-10-27 at 13:00 -0400, Alan Stern wrote:
> On Mon, 27 Oct 2008, James Bottomley wrote:
> 
> > > --- usb-2.6.orig/drivers/scsi/scsi_scan.c
> > > +++ usb-2.6/drivers/scsi/scsi_scan.c
> > > @@ -602,6 +602,13 @@ static int scsi_probe_lun(struct scsi_de
> > >  				    (sshdr.ascq == 0))
> > >  					continue;
> > >  			}
> > > +		} else {
> > > +			/*
> > > +			 * Additional Length and Vendor fields missing
> > > +			 * probably means nothing was transferred.  Try again.
> > > +			 */
> > > +			if (inq_result[4] == 0 && inq_result[8] == 0)
> > > +				continue;
> > 
> > Really, no.
> > 
> > A legitimate minimal response from a device is zero in the additional
> > length field.  If it does that, then the vendor field is bound to be
> > zero as well.
> > 
> > About the best we can do is check the first four fields.  For them to be
> > all zero, it would have to be a minimal response SCSI-1 device (RDF of
> > zero) ... hopefully they're all dead by now.
> 
> Then you're saying that the patch should everything up to
> inq_result[8]?  Or maybe even beyond?  I can do that.  Would that be 
> acceptable?

No ... "first four fields" means everything up to inq_result[3].
Anything beyond that would be legitimately zero if they were.

James

Re: [PATCH] SCSI: detect missing data for INQUIRY

[Alan Stern]

On Mon, 27 Oct 2008, James Bottomley wrote:

> > > About the best we can do is check the first four fields.  For them to be
> > > all zero, it would have to be a minimal response SCSI-1 device (RDF of
> > > zero) ... hopefully they're all dead by now.
> > 
> > Then you're saying that the patch should everything up to
> > inq_result[8]?  Or maybe even beyond?  I can do that.  Would that be 
> > acceptable?
> 
> No ... "first four fields" means everything up to inq_result[3].
> Anything beyond that would be legitimately zero if they were.

But what about the case where the first four bytes are zero and some of 
the fields beyond them are nonzero.  Isn't that possible?

Alan Stern

Re: [PATCH] SCSI: detect missing data for INQUIRY

[James Bottomley]

On Mon, 2008-10-27 at 13:27 -0400, Alan Stern wrote:
> On Mon, 27 Oct 2008, James Bottomley wrote:
> 
> > > > About the best we can do is check the first four fields.  For them to be
> > > > all zero, it would have to be a minimal response SCSI-1 device (RDF of
> > > > zero) ... hopefully they're all dead by now.
> > > 
> > > Then you're saying that the patch should everything up to
> > > inq_result[8]?  Or maybe even beyond?  I can do that.  Would that be 
> > > acceptable?
> > 
> > No ... "first four fields" means everything up to inq_result[3].
> > Anything beyond that would be legitimately zero if they were.
> 
> But what about the case where the first four bytes are zero and some of 
> the fields beyond them are nonzero.  Isn't that possible?

Not if the device conforms to the standard.

James

[PATCH v2] SCSI: detect missing data for INQUIRY

[Alan Stern]

This patch (as1154b) fixes a problem in scsi_probe_lun(): The function
doesn't check whether the device has actually sent back any INQUIRY
data!  The patch adds a test to see if the result buffer is still
empty after the command has been executed.

This enables the Thecus N2050 storage device to work.  The firmware on
that device starts up strangely; it sends no data in response to the
initial INQUIRY, and it sends the INQUIRY information in response to
the followup REQUEST SENSE!  But after that it works better, so
retrying the INQUIRY is enough to get it going.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -602,6 +602,14 @@ static int scsi_probe_lun(struct scsi_de
 				    (sshdr.ascq == 0))
 					continue;
 			}
+		} else {
+			/*
+			 * If the first four bytes are all 0 then probably
+			 * nothing was transferred.  Try again.
+			 */
+			if ((inq_result[0] | inq_result[1] |
+					inq_result[2] | inq_result[3]) == 0)
+				continue;
 		}
 		break;
 	}

Re: [PATCH v2] SCSI: detect missing data for INQUIRY

[James Smart]

Checking the payload like this is really hokey - but probably works.
Have we lost all notion from the LLDD on what the actual payload length 
was (calculated by expected transfer length and resid) ?  Seems like a 
much better idea than just checking contents.

-- james s

Alan Stern wrote:
> This patch (as1154b) fixes a problem in scsi_probe_lun(): The function
> doesn't check whether the device has actually sent back any INQUIRY
> data!  The patch adds a test to see if the result buffer is still
> empty after the command has been executed.
> 
> This enables the Thecus N2050 storage device to work.  The firmware on
> that device starts up strangely; it sends no data in response to the
> initial INQUIRY, and it sends the INQUIRY information in response to
> the followup REQUEST SENSE!  But after that it works better, so
> retrying the INQUIRY is enough to get it going.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> Index: usb-2.6/drivers/scsi/scsi_scan.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/scsi_scan.c
> +++ usb-2.6/drivers/scsi/scsi_scan.c
> @@ -602,6 +602,14 @@ static int scsi_probe_lun(struct scsi_de
>                                     (sshdr.ascq == 0))
>                                         continue;
>                         }
> +               } else {
> +                       /*
> +                        * If the first four bytes are all 0 then probably
> +                        * nothing was transferred.  Try again.
> +                        */
> +                       if ((inq_result[0] | inq_result[1] |
> +                                       inq_result[2] | inq_result[3]) == 0)
> +                               continue;
>                 }
>                 break;
>         }
> 
> --
> 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 v2] SCSI: detect missing data for INQUIRY

[Alan Stern]

On Tue, 28 Oct 2008, James Smart wrote:

> Checking the payload like this is really hokey - but probably works.

I agree completely; it _is_ hokey.  There is precedent, however -- 
scsi_mode_sense() behaves similarly.

> Have we lost all notion from the LLDD on what the actual payload length 
> was (calculated by expected transfer length and resid) ?

In short: Yes, we have.  That information is discarded by
scsi_execute().

>  Seems like a 
> much better idea than just checking contents.

Again I agree.  The SCSI midlayer simply is not designed around the
idea of LLD's returning an actual transfer length.  I have no idea why
not -- I wasn't there when the SCSI core was initially designed.  :-)

Alan Stern

Re: [PATCH v2] SCSI: detect missing data for INQUIRY

[Alan Stern]

James:

There hasn't been any feedback on this patch:

	http://marc.info/?l=linux-scsi&m=122513368301598&w=2

It was modified in the way you requested.  Does it need to be revised 
any more?

Alan Stern

Re: [PATCH v2] SCSI: detect missing data for INQUIRY

[James Bottomley]

On Wed, 2008-12-03 at 12:44 -0500, Alan Stern wrote:
> James:
> 
> There hasn't been any feedback on this patch:
> 
> 	http://marc.info/?l=linux-scsi&m=122513368301598&w=2
> 
> It was modified in the way you requested.  Does it need to be revised 
> any more?

Probably not.  I really dislike a check and retry on something which is
theoretically a legal return, but I suppose I can put it in and see if
anything breaks.

James

Re: [PATCH v2] SCSI: detect missing data for INQUIRY

[Alan Stern]

On Wed, 3 Dec 2008, James Bottomley wrote:

> On Wed, 2008-12-03 at 12:44 -0500, Alan Stern wrote:
> > James:
> > 
> > There hasn't been any feedback on this patch:
> > 
> > 	http://marc.info/?l=linux-scsi&m=122513368301598&w=2
> > 
> > It was modified in the way you requested.  Does it need to be revised 
> > any more?
> 
> Probably not.  I really dislike a check and retry on something which is
> theoretically a legal return, but I suppose I can put it in and see if
> anything breaks.

It's too bad that the scsi_execute_req() interface loses all 
information about how much data the device actually sent.  If that 
information were preserved then it wouldn't be necessary to rely on 
this not-quite-satisfactory sort of test.

Alan Stern

Re: [PATCH v2] SCSI: detect missing data for INQUIRY

[Boaz Harrosh]

Alan Stern wrote:
> On Wed, 3 Dec 2008, James Bottomley wrote:
> 
>> On Wed, 2008-12-03 at 12:44 -0500, Alan Stern wrote:
>>> James:
>>>
>>> There hasn't been any feedback on this patch:
>>>
>>> 	http://marc.info/?l=linux-scsi&m=122513368301598&w=2
>>>
>>> It was modified in the way you requested.  Does it need to be revised 
>>> any more?
>> Probably not.  I really dislike a check and retry on something which is
>> theoretically a legal return, but I suppose I can put it in and see if
>> anything breaks.
> 
> It's too bad that the scsi_execute_req() interface loses all 
> information about how much data the device actually sent.  If that 
> information were preserved then it wouldn't be necessary to rely on 
> this not-quite-satisfactory sort of test.
> 
> Alan Stern
> 

Just a comment:
You are the third person this week that wanted residual return from
scsi_execute/scsi_execute_req. Perhaps it's time as come ;)

(This is a TODO for me)

Boaz