usb-storage: fix bogus hardware error messages for ATA pass-thru devices

Ever since commit a621bac3044e ("scsi_lib: correctly retry failed zero
length REQ_TYPE_FS commands"), people have been getting bogus error
messages for USB disk drives using ATA pass-thru.  For example:

[ 1344.880193] sd 6:0:0:0: [sdb] Attached SCSI disk
[ 1345.069152] sd 6:0:0:0: [sdb] tag#0 FAILED Result: hostbyte=DID_ERROR driverbyte=DRIVER_SENSE
[ 1345.069159] sd 6:0:0:0: [sdb] tag#0 Sense Key : Hardware Error [current] [descriptor]
[ 1345.069162] sd 6:0:0:0: [sdb] tag#0 Add. Sense: No additional sense information
[ 1345.069168] sd 6:0:0:0: [sdb] tag#0 CDB: ATA command pass through(16) 85 06 20 00 00 00 00 00 00 00 00 00 00 00 e5 00
[ 1345.172252] sd 6:0:0:0: [sdb] tag#0 FAILED Result: hostbyte=DID_ERROR driverbyte=DRIVER_SENSE
[ 1345.172258] sd 6:0:0:0: [sdb] tag#0 Sense Key : Hardware Error [current] [descriptor]
[ 1345.172261] sd 6:0:0:0: [sdb] tag#0 Add. Sense: No additional sense information
[ 1345.172266] sd 6:0:0:0: [sdb] tag#0 CDB: ATA command pass through(12)/Blank a1 06 20 da 00 00 4f c2 00 b0 00 00

These messages can be quite annoying, because programs like udisks2
provoke them every 10 minutes or so.  Other programs can also have
this effect, such as those in smartmontools.

I don't fully understand how that commit induced the SCSI core to log
these error messages, but the underlying cause for them is code added
to usb-storage by commit f1a0743bc0e7 ("USB: storage: When a device
returns no sense data, call it a Hardware Error").  At the time it was
necessary to do this, in order to prevent an infinite retry loop with
some not-so-great mass storage devices.

However, the ATA pass-thru protocol uses SCSI sense data to return
command status values, and some devices always report Check Condition
status for ATA pass-thru commands to ensure that the host retrieves
the sense data, even if the command succeeded.  This violates the USB
mass-storage protocol (Check Condition status is supposed to mean the
command failed), but we can't help that.

This patch attempts to mitigate the problem of these bogus error
reports by changing usb-storage.  The HARDWARE ERROR sense key will be
inserted only for commands that aren't ATA pass-thru.

Thanks to Ewan Milne for pointing out that this mechanism was present
in usb-storage.  8 years after writing it, I had completely forgotten
its existence.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Tested-by: Kris Lindgren <kris.lindgren@gmail.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1351305
CC: Ewan D. Milne <emilne@redhat.com>
CC: <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index 1a59f335b063e7e79f8ece2173f2f03574ce3a5b..a3ccb899df60c39dd537ae5d3664d937715d0338 100644 (file)
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -834,13 +834,25 @@ Retry_Sense:
                        if (result == USB_STOR_TRANSPORT_GOOD) {
                                srb->result = SAM_STAT_GOOD;
                                srb->sense_buffer[0] = 0x0;
+                       }
+
+                       /*
+                        * ATA-passthru commands use sense data to report
+                        * the command completion status, and often devices
+                        * return Check Condition status when nothing is
+                        * wrong.
+                        */
+                       else if (srb->cmnd[0] == ATA_16 ||
+                                       srb->cmnd[0] == ATA_12) {
+                               /* leave the data alone */
+                       }
 
                        /*
                         * If there was a problem, report an unspecified
                         * hardware error to prevent the higher layers from
                         * entering an infinite retry loop.
                         */
-                       } else {
+                       else {
                                srb->result = DID_ERROR << 16;
                                if ((sshdr.response_code & 0x72) == 0x72)
                                        srb->sense_buffer[1] = HARDWARE_ERROR;