Opened 2 months ago

Closed 43 hours ago

Last modified 11 hours ago

#1653 closed enhancement (fixed)

Support for 3snic raid cards on Linux

Reported by: steven.song Owned by:
Priority: minor Milestone: Release 7.4
Component: all Version:
Keywords: 3snic linux Cc:

Description

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

Attachments (14)

os_linux.cpp (120.7 KB) - added by steven.song 7 weeks ago.
sssraid.h (3.0 KB) - added by steven.song 7 weeks ago.
smartctl-scan.txt (651 bytes) - added by steven.song 7 weeks ago.
smartctl-a.txt (2.3 KB) - added by steven.song 7 weeks ago.
os_linux.cpp.diff (11.7 KB) - added by steven.song 5 days ago.
sssraid.h.diff (3.5 KB) - added by steven.song 5 days ago.
smartctl.8.in.diff (927 bytes) - added by steven.song 5 days ago.
Makefile.am.diff (531 bytes) - added by steven.song 5 days ago.
ChangeLog.diff (351 bytes) - added by steven.song 5 days ago.
smartctl-scan.log (688 bytes) - added by steven.song 5 days ago.
smartctl-x.log (15.1 KB) - added by steven.song 5 days ago.
smartctl.diff (16.9 KB) - added by steven.song 35 hours ago.
New patch
smartctl-x-sas.log (4.6 KB) - added by steven.song 11 hours ago.
smartctl-x-sata.log (14.1 KB) - added by steven.song 11 hours ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 2 months ago by Christian Franke

Keywords: 3snic linux windows added
Milestone: undecided

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.

Changed 7 weeks ago by steven.song

Attachment: os_linux.cpp added

Changed 7 weeks ago by steven.song

Attachment: sssraid.h added

Changed 7 weeks ago by steven.song

Attachment: smartctl-scan.txt added

Changed 7 weeks ago by steven.song

Attachment: smartctl-a.txt added

comment:2 Changed 7 weeks ago by steven.song

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

comment:3 Changed 5 weeks ago by 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.

comment:4 in reply to:  3 Changed 4 weeks ago by 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.

Changed 5 days ago by steven.song

Attachment: os_linux.cpp.diff added

Changed 5 days ago by steven.song

Attachment: sssraid.h.diff added

Changed 5 days ago by steven.song

Attachment: smartctl.8.in.diff added

Changed 5 days ago by steven.song

Attachment: Makefile.am.diff added

Changed 5 days ago by steven.song

Attachment: ChangeLog.diff added

Changed 5 days ago by steven.song

Attachment: smartctl-scan.log added

Changed 5 days ago by steven.song

Attachment: smartctl-x.log added

comment:5 in reply to:  3 Changed 5 days ago by 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.

comment:6 Changed 5 days ago by 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.

comment:7 in reply to:  6 Changed 2 days ago by 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.

comment:8 Changed 44 hours ago by Christian Franke

Keywords: windows removed
Milestone: undecidedRelease 7.4
Summary: Support for 3snic raid cardsSupport for 3snic raid cards on Linux
Type: patchenhancement

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:

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

comment:9 Changed 43 hours ago by Christian Franke

Resolution: fixed
Status: newclosed

comment:10 Changed 43 hours ago by 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.

Changed 35 hours ago by steven.song

Attachment: smartctl.diff added

New patch

comment:11 Changed 24 hours ago by 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?

comment:12 Changed 11 hours ago by steven.song

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

Changed 11 hours ago by steven.song

Attachment: smartctl-x-sas.log added

Changed 11 hours ago by steven.song

Attachment: smartctl-x-sata.log added
Note: See TracTickets for help on using tickets.