Opened 2 years ago

Last modified 2 years ago

#1609 new enhancement

JMicron USB bridges may return empty sense data on error

Reported by: Eaton Zveare Owned by:
Priority: minor Milestone: undecided
Component: all Version: 7.3
Keywords: jmicron Cc:

Description

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.

Change History (6)

comment:1 by Christian Franke, 2 years ago

Component: smartctlall
Keywords: jmicron added
Milestone: undecided

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

comment:2 by Eaton Zveare, 2 years ago

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.

comment:3 by Christian Franke, 2 years ago

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.

comment:4 by Eaton Zveare, 2 years ago

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.

comment:5 by Christian Franke, 2 years ago

Summary: JMicron pass-through does not check returned sense dataJMicron USB bridges may return empty sense data on error
Type: defectenhancement

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

     
    10111011         "usbjmicron_device::ata_pass_through: "))
    10121012    return set_err(scsidev->get_err());
    10131013
     1014  if (io_hdr.scsi_status == SCSI_STATUS_CHECK_CONDITION)
     1015    // Some JMicron USB bridges return zero filled sense data on error
     1016    return set_err(EIO, "Unknown error, invalid sense data [JMicron]");
     1017
    10141018  if (in.out_needed.is_set()) {
    10151019    if (is_smart_status) {
    10161020      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.

comment:6 by Eaton Zveare, 2 years ago

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.

Note: See TracTickets for help on using tickets.