JMicron USB bridges may return empty sense data on error

[Eaton Zveare]

I noticed that when an incorrect pass-through command (or command data) is sent over a JMicron USB, usbjmicron_device::ata_pass_through returns true, indicating success.

If I use a different USB, the call is routed to sat_device::ata_pass_through instead. That method correctly returns false with the same incorrect command/data because you parse the sense data extensively and check for various errors.

When debugging and breaking at this spot, I noticed there is indeed an indication the command failed (io_hdr.scsi_status == 2 (CHECK CONDITION)), but you aren't checking it. There's barely any error checking at all compared to sat_device::ata_pass_through.

Is this an oversight with the JMicron support? I noticed with Cypress support, you do a basic check here. This doesn't appear to be the case with Prolific support though, where you also don't check the sense.

Let me know what you think. Maybe consider copying all the error checking code over from sat_device::ata_pass_through.

[Christian Franke]

Please provide the output of smartctl -r ioctl,2 ... for a command where CHECK_CONDITION is returned and the error is apparently ignored.

CHECK_CONDITION only indicates presence of valid sense data. The sense data does not necessarily indicate an error (e.g. with smartctl -d sat -H, ATA output registers are returned in sense data not indicating an error).

​usbjmicron_device::ata_pass_through calls scsi_device::scsi_pass_through_and_check.
After the actual scsi_pass_through, this function calls scsi_do_sense_disect (which sets key/asc/ascq if (SCSI_STATUS_CHECK_CONDITION == io_buf->scsi_status)) and then scsiSimpleSenseFilter which returns nonzero if and only if the sense key indicates an error.

Please debug into scsi_device::scsi_pass_through_and_check and examine sinfo.sense_key and err:

  // Check sense
  scsi_sense_disect sinfo;
  scsi_do_sense_disect(iop, &sinfo);
  int err = scsiSimpleSenseFilter(&sinfo);
  if (err) { // <== here

[Eaton Zveare]

Hi Christian, I'm actually working in the source code and not with a specific command.
My custom code is basically this inside of atacmds.cpp:

ata_cmd_in in;
in.in_regs.command = command;
in.in_regs.sector_count = 1;
in.set_data_out(out_data_buf, 1);
return device->ata_pass_through(in);

The last line is what is erroneously returning true.

I have debugged what you asked and both values are 0 when io_hdr.scsi_status == SCSI_STATUS_CHECK_CONDITION.

[Christian Franke]

This suggests that the USB bridge firmware returns empty sense data. It should not do this if an error occurred, so this is possibly a firmware bug. Check the raw sense data block, see scsi_do_sense_disect.

[Eaton Zveare]

io_buf->sensep is all zeros. So even though there is a CHECK CONDITION, scsi_do_sense_disect doesn't do anything since resp_code is 0.

[Christian Franke]

Even with resp_code != 0, the remaining zeros would mean ​SENSE KEY = 0 (NO_SENSE), ​ASC/ASCQ = 0/0 (NO ADDITIONAL SENSE INFORMATION).

Conclusion: If the JMicron USB bridge returns this on error (i.e. unsupported SCSI command), this is a firmware bug.

If this is common behavior of various JMicron bridges, we could possibly add something like:

scsiata.cpp

@@ -1011 +1011 @@
          "usbjmicron_device::ata_pass_through: "))
     return set_err(scsidev->get_err());
 
+  if (io_hdr.scsi_status == SCSI_STATUS_CHECK_CONDITION)
+    // Some JMicron USB bridges return zero filled sense data on error
+    return set_err(EIO, "Unknown error, invalid sense data [JMicron]");
+
   if (in.out_needed.is_set()) {
     if (is_smart_status) {
       if (io_hdr.resid == 1)

But this would require that CHECK_CONDITION is never set on success.

Change summary accordingly and leave ticket open as undecided for now.

[Eaton Zveare]

Yes, it appears that is the only check that can be done in this case.

Do you have any JMicron adapters to check? It would be interesting to know if affects other bridge models.