Support for 3snic raid cards on Linux

[steven.song]

Hello,

I work at 3SNIC and want to support our RAID cards descripted as below link in smartmontools, what else needs to provide besides patch files and related log files?

​https://www.3snic.com/en/raid/raid_3S580

[Christian Franke]

Thanks for offering patches. Please provide patch files in unified diff format which include the changes to os_linux.cpp​ and/or os_win32.cpp​, the ChangeLog​ and optionally AUTHORS​.

Alternatively you could provide the patch as a PR in our read only GH mirror at
​https://github.com/smartmontools/smartmontools/

Please note that we allow (and recommend) to use C++11, but not yet C++14.

[steven.song]

The attached source files are modified based on v7.3, please see the log files for --scan and --all parameters.

[Christian Franke]

Thanks. Unfortunately you didn't follow the hints from the above comment.

• Please provide a patch (e.g. output of svn diff) file. Do not provide full versions of changed and new files.
• Merge your changes of os_linux.cpp to current SVN head. Could easily be done after a svn checkout: run svn update -r 5338, add your version of os_linux.cpp, then svn update to let SVN do the merge. Resolve conflicts if any.
• open(), close() and usage of m_fd look strange and are possibly not needed at all. The sscanf(..., &m_hba) could be done in the ctor because the device name does not change later.
• Prepend all private member variables *id with m_.
• Don't use printf(). It is not compatible with smartctl --json and the syslog output of smartd. Use pout() instead.
• Remove the linux_sssraid_device::autodetect_open() function. Use get_sat_device("sat,auto", new linux_sssraid_device(...)) instead. See linux_cciss_device for an example.
• Add author and licensing header to sssraid.h. For licensing info, use SPDX-License-Identifier: GPL-2.0-or-later or a more relaxed but compatible one, see ​https://spdx.org/licenses/.
• Add this new file to Makefile.am.
• Add an entry to the ChangeLog file.
• Briefly describe new -d parameter in smartctl.8.in and smartd.conf.5.in.
• Use smartctl -x instead of smartctl -a for testing.
• Also provide smartctl -x output for a SATA device behind this controller. This also tests whether SCSI sense data is properly returned. This is mandatory when ATA command SMART RETURN STATUS is issued through SAT ATA PASS-THROUGH commands. If this does not work, remove the "sat,auto" detection.
• Also test non-DATA commands like smartctl -t short for SAS and SATA.
• Also test with smartd to make sure that repeated open()/close() calls work.
• Note that comprehensive testing is *very* important. Smartmontools is maintained by few volunteers in unpaid spare time. No developer has access to a test machine with such a controller.

[steven.song]

Thanks for your advice, will update soon after code modification and testing.

Replying to Christian Franke:

Thanks. Unfortunately you didn't follow the hints from the above comment.

• Please provide a patch (e.g. output of svn diff) file. Do not provide full versions of changed and new files.
• Merge your changes of os_linux.cpp to current SVN head. Could easily be done after a svn checkout: run svn update -r 5338, add your version of os_linux.cpp, then svn update to let SVN do the merge. Resolve conflicts if any.
• open(), close() and usage of m_fd look strange and are possibly not needed at all. The sscanf(..., &m_hba) could be done in the ctor because the device name does not change later.
• Prepend all private member variables *id with m_.
• Don't use printf(). It is not compatible with smartctl --json and the syslog output of smartd. Use pout() instead.
• Remove the linux_sssraid_device::autodetect_open() function. Use get_sat_device("sat,auto", new linux_sssraid_device(...)) instead. See linux_cciss_device for an example.
• Add author and licensing header to sssraid.h. For licensing info, use SPDX-License-Identifier: GPL-2.0-or-later or a more relaxed but compatible one, see ​https://spdx.org/licenses/.
• Add this new file to Makefile.am.
• Add an entry to the ChangeLog file.
• Briefly describe new -d parameter in smartctl.8.in and smartd.conf.5.in.
• Use smartctl -x instead of smartctl -a for testing.
• Also provide smartctl -x output for a SATA device behind this controller. This also tests whether SCSI sense data is properly returned. This is mandatory when ATA command SMART RETURN STATUS is issued through SAT ATA PASS-THROUGH commands. If this does not work, remove the "sat,auto" detection.
• Also test non-DATA commands like smartctl -t short for SAS and SATA.
• Also test with smartd to make sure that repeated open()/close() calls work.
• Note that comprehensive testing is *very* important. Smartmontools is maintained by few volunteers in unpaid spare time. No developer has access to a test machine with such a controller.

[steven.song]

Uploaded the source patch and test log, please help review, thanks.

Replying to Christian Franke:

Thanks. Unfortunately you didn't follow the hints from the above comment.

• Please provide a patch (e.g. output of svn diff) file. Do not provide full versions of changed and new files.
• Merge your changes of os_linux.cpp to current SVN head. Could easily be done after a svn checkout: run svn update -r 5338, add your version of os_linux.cpp, then svn update to let SVN do the merge. Resolve conflicts if any.
• open(), close() and usage of m_fd look strange and are possibly not needed at all. The sscanf(..., &m_hba) could be done in the ctor because the device name does not change later.
• Prepend all private member variables *id with m_.
• Don't use printf(). It is not compatible with smartctl --json and the syslog output of smartd. Use pout() instead.
• Remove the linux_sssraid_device::autodetect_open() function. Use get_sat_device("sat,auto", new linux_sssraid_device(...)) instead. See linux_cciss_device for an example.
• Add author and licensing header to sssraid.h. For licensing info, use SPDX-License-Identifier: GPL-2.0-or-later or a more relaxed but compatible one, see ​https://spdx.org/licenses/.
• Add this new file to Makefile.am.
• Add an entry to the ChangeLog file.
• Briefly describe new -d parameter in smartctl.8.in and smartd.conf.5.in.
• Use smartctl -x instead of smartctl -a for testing.
• Also provide smartctl -x output for a SATA device behind this controller. This also tests whether SCSI sense data is properly returned. This is mandatory when ATA command SMART RETURN STATUS is issued through SAT ATA PASS-THROUGH commands. If this does not work, remove the "sat,auto" detection.
• Also test non-DATA commands like smartctl -t short for SAS and SATA.
• Also test with smartd to make sure that repeated open()/close() calls work.
• Note that comprehensive testing is *very* important. Smartmontools is maintained by few volunteers in unpaid spare time. No developer has access to a test machine with such a controller.

[Christian Franke]

Please provide ONE patch file which contains the changes for all files. This is long standing common practice in F(L)OSS community.

[steven.song]

Please see the attached smartctl.diff for the all changes.
Replying to Christian Franke:

Please provide ONE patch file which contains the changes for all files. This is long standing common practice in F(L)OSS community.

[Christian Franke]

Patch applied with some modifications in r5420.

The following issues were fixed before the commit:

• The new file sssraid.h was not included in the patch. You need to use svn add ... before svn diff to get new files included.
• The conflicts in ChangeLog were not resolved. The conflicting range was marked with <<<<<<< .mine ... ======= ... >>>>>>> .r5419.

Remaining issues fixed afterwards:

• The code did not compile, see ​CircleCI build "smartmontools 691". Fixed in r5421.
• The documentation syntax in smartctl.8.in was incorrect and smartd.conf.5.in documentation was missing. Fixed in r5422.

Please test a r5422 or later linux binary from ​https://builds.smartmontools.org/.

[Christian Franke]

r5420, r5421, r5422.

[Christian Franke]

Open issue as seen in smartctl-x.log above:

SMART Status not supported: Incomplete response, ATA output registers missing

This means that the ATA PASS-THROUGH(16) SCSI command (cdb[0]=0x85) does not honor the CK_COND bit (cdb[2]|=(1<<5)). If set, ATA output registers must be returned in SCSI sense data. This is not optional and should be fixed.

Smartmontools supports either ATA Status Return sense data descriptor or Fixed format sense data fields for ATA PASS-THROUGH commands.

See SCSI/ATA Translation standards at ​https://www.t10.org/drafts.htm#SAT for details.

Please report this issue to the authors of controller firmware and Linux driver.

[steven.song]

New patch

[Christian Franke]

This "New patch" is now completely useless as all changes are already committed and fixed in r5420, r5421, r5422. See comment 8 for details.

Run svn revert -R . and then svn up to update your working copy of the code. Compile and test this code. Alternatively test r5422 or any later linux binary from ​​https://builds.smartmontools.org/.

Test both smartctl and smartd with SAS and SATA devices. Report the test result here. Remember that no smartmontools developer could do these tests for you due to lack of access to such a controller.

Smartmontools runs as root and serious bugs may damage stored data or hardware.

---

Did you report the CK_COND issue described in comment 10 to the authors of controller firmware and Linux driver?

[steven.song]

Thanks for your comments, this issue was reported to our FW team yesterday, will update test log after fixed.