Opened 10 years ago
Closed 10 years ago
#482 closed patch (fixed)
Support for Prolific PL2773
Reported by: | TomVe | Owned by: | Christian Franke |
---|---|---|---|
Priority: | major | Milestone: | Release 6.4 |
Component: | all | Version: | 6.3 |
Keywords: | Cc: | tommy.vestermark@… |
Description
The ICY BOX IB-120StU3 uses a Prolific PL2773 chip, which is currently not supported by smartmontools. By reverse engineering the Windows tool iSmart.exe i have made the included patch. It seems to work well and supports all of smartctl -a.
Please review and consider including in your next release.
Attachments (6)
Change History (19)
by , 10 years ago
Attachment: | PL2773_ver1.patch added |
---|
comment:1 by , 10 years ago
Thank you for the contribution! May i ask if you tested smartmontools with -x and -t commands to see if basic functionality works fine? Also it would be great to see documentation update in the patch.
follow-up: 3 comment:2 by , 10 years ago
You are welcome :-) And thank you! I am a bit rusty in coding, but found it rather easy to get up and running on your code. Also the code seems pretty well structured, such that adding this driver is (mostly) confined to one file.
I am no SCSI/ATA/SMART wizard, but just tried to replicate the ismart.exe commands by mapping the ATA CDB content by a little googling and trial and error. Although ismart.exe only uses 4 different ATA commands it seems to work pretty well with smartctls larger featureset.
I have attached a file with output from smartctl -x and -t. Please comment whether it looks sane.
I have attached a file with my reverse engineering output. If anything obvious appears please comment.
I would also like some feedback on the following:
- Is the code generally sane?
- I disabled support_data_out, as it will hang the drive. I may be missing a R/W bit in the ATA command or something - any ideas?
- I just tried adding the supports_48bit flag, and it seemed to pass a smartctl -x command with no issues. I do not completely understand how that works as the CDB only contains 3 LBA bytes. Can you see from the code whether it should be safe to enable?
- I am mostly unsure about the Read ATA output registers functionality. I think regbuf[0] may be the status register (See attached file for some examples). Any advice for how to find the 'error' and 'sector_count' register? What is needed for proper function of smartctl/smartd?
- Another ismart.exe ATA command (D0) seems to readout the EEPROM content (up to 256 bytes) with firmware revision etc. Does it provide any value to implement that for smartctl?
- What do you mean by "documentation update"? Should I try to add something to smartctl.8? (I have never made man pages before :-)
If you have any ideas, I can try them out and will rework the patch to your liking.
by , 10 years ago
Attachment: | PL2773 reverse engineering.txt added |
---|
sg_raw commands replicating output from ismart.exe
by , 10 years ago
Attachment: | PL2773 smartctl test.txt added |
---|
Output from running dmesg, smartctl -x and smartctl -t short
comment:3 by , 10 years ago
Milestone: | → Release 6.4 |
---|---|
Owner: | set to |
Status: | new → accepted |
- Is the code generally sane?
Looks very good - thanks.
One suggestion:
+// printf("DEBUG:REG %x %x %x %x %x %x ...
Replace commented out test code by real debug messages. Check scsi_debug, print with pout():
+ if (scsi_debug > 0) + pout("Prolific Registers: %x %x %x %x %x %x ...
- I disabled support_data_out, as it will hang the drive. I may be missing a R/W bit in the ATA command or something - any ideas?
Is there any reverse engineering result for a DATA OUT command?
- I just tried adding the supports_48bit flag, and it seemed to pass a smartctl -x command with no issues. I do not completely understand how that works as the CDB only contains 3 LBA bytes. Can you see from the code whether it should be safe to enable?
For SATA bridges, it is normally safe because the SATA protocol layer (unlike PATA) makes no destinction between 28-bit and 48-bit commands (there is no "28-bit Register FIS"). It works with 3 LBA bytes in CDB if the bridge firmware sets the upper LBA bytes in the SATA Register FIS to 0. Use the "ata_device::supports_48bit_hi_null" flag for "ata_cmd_is_supported()" check. Or implement an extra "-d usbprolific,x" option to enable it, see "class usbjmicron_device" for an example.
- I am mostly unsure about the Read ATA output registers functionality. I think regbuf[0] may be the status register (See attached file for some examples). Any advice for how to find the 'error' and 'sector_count' register? What is needed for proper function of smartctl/smartd?
SMART STATUS requires only LBA MID and HIGH. GET SCT ERC would also require SECTOR COUNT and LBA LOW but this command only works with DATA OUT support. Many interfaces use the traditional pre-IDE register byte order (inherited from some very old WDC controller :-), see "ata_in_regs" and "ata_out_regs".
- Another ismart.exe ATA command (D0) seems to readout the EEPROM content (up to 256 bytes) with firmware revision etc. Does it provide any value to implement that for smartctl?
There is no smartctl option to handle such device specific commands.
- What do you mean by "documentation update"? Should I try to add something to smartctl.8? (I have never made man pages before :-)
Add description for "usbprolific" parameter for "-d" option to smartctl.8.in and smartd.conf.5.in. Not mandatory, we could add this later.
comment:4 by , 10 years ago
Thanks for the feedback!
I have made a version 2 of the patch with the following modifications:
- Removed the debug printf's as I learned about -r scsiioctl,2 :-)
- Added supports_48bit_hi_null, as it works fine with -x
- Added man pages
- Minor clean-up
The patch is tested with two disks attached to the controller, and both drives worked fine. I might experiment with Data Out support at a later time - but it is not used by ismart.exe. It seems to work OK for me at the time being. Please consider merging.
comment:5 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
Patch applied in r4004.
The addition to drivedb.h is omitted for now because the new -d option would break the backward compatibility which is required for /usr/sbin/update-smart-drivedb. I need to create new drivedb.h branches for 6.1 - 6.3 first. You could add the Prolific entry to a local /etc/smart_drivedb.h file.
I also removed the download URL because it apparently requires a login.
follow-ups: 7 8 comment:6 by , 10 years ago
@chrfranke here it is available w/o password and registration: http://prolificusa.com/portfolio/pl-2773-usb3-0-esataii-to-sataii-bridge-controller/
comment:7 by , 10 years ago
comment:8 by , 10 years ago
Replying to samm2:
@chrfranke here it is available w/o password and registration: ...
Thanks. Should we add it to scsiata.cpp or to the Wiki? I am not sure.
by , 10 years ago
Attachment: | PL277X_scsi_pass_thru_cmd_v03_Ollie.doc added |
---|
Documentation from ProlificUSA. Command and Status description.
comment:9 by , 10 years ago
Cc: | added |
---|
I have kindly asked ProlificUSA for information/verification of the pass-through command and status registers. They sent me the attached document. The patch should be OK as is.
I will however try to make a new patch with added support for DATA OUT commands (rebased of r4009). When done, I will attach it to this ticket (to keep it together) and re-open the ticket - unless you want me to make a new ticket instead.
follow-up: 11 comment:10 by , 10 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Made a new patch with support for DATA OUT and OUTPUT REGS and some minor clean up based on the information in the Prolific document. Works fine with '-x' and can now print temperature log and SCT. Test with '-t select' and a span also works now.
It might be possible to add support for true 48 bit LBA. Although not documented, I suspect the PREFIX might be for sending the high order bits (not tested). Furthermore it should be easy add support for readout of the high order bits in the status register. However, I am unsure of the value of doing this, as everything seems to work perfectly now. Furthermore my knowledge of the code base is probably too thin to attempt this...
comment:11 by , 10 years ago
Status: | reopened → accepted |
---|
Replying to TomVe:
Made a new patch with support for DATA OUT and OUTPUT REGS and some minor clean up based on the information in the Prolific document. Works fine with '-x' and can now print temperature log and SCT. Test with '-t select' and a span also works now.
Thanks! I will commit it soon.
It might be possible to add support for true 48 bit LBA. Although not documented, I suspect the PREFIX might be for sending the high order bits (not tested).
I presume the PREFIX is like the "previous registers" (clever:-) hack from the IDE/PATA interface. Nonzero values would be only needed for READ LOG EXT if a log contains more than 255 sectors.
Hmm... it might make sense to always issue the PREFIX command for 48-bit ATA commands to make sure that the upper bytes are properly set. See "usbsunplus_device" for an example.
Furthermore it should be easy add support for readout of the high order bits in the status register.
According to (even recent) ATA standards, the "Status field is one byte". The only use case for the upper bytes of other output registers are failed READ/WRITE EX commands which return the first failed LBA in the output registers. Such commands are not used by smartmontools.
comment:12 by , 10 years ago
If READ LOG EXT with more than 255 sectors is the only use case for true 48 bit, I must confess my motivation for risking adding issues due to misinterpreting PREFIX is low. I wouldn't even know how to test it thoroughly, so I think I will just leave it as is. It currently works pretty well and Prolifics ismart.exe does not issue these commands either.
BTW I asked Prolific for more USB IDs compatible with this interface, but have not yet received an answer. I will open a new ticket, if I can add support for more USB IDs than 0x2773 at a later time.
Patch against SVN rev. 4002