Bug 200619 - camcontrol trashes the drive modepage if it's changed
Summary: camcontrol trashes the drive modepage if it's changed
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Enji Cooper
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-03 19:54 UTC by Enji Cooper
Modified: 2015-11-08 00:52 UTC (History)
3 users (show)

See Also:
ngie: mfc-stable10+
ngie: mfc-stable9+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Enji Cooper freebsd_committer 2015-06-03 19:54:09 UTC
Description
===========

Something is broken in camcontrol in bsd10. 
It trashes a drive modepage if it changed. 

Here is some sample output. 

ls-10-min252-1# isi_radish -e
camcontrol: value 0 is out of range for entry Report Count; clipping to -1

This will happen for other values if you attempt to change them as well. 

ls-10-min252-1# echo -e "Interval Timer: 1800" | camcontrol modepage da4 -m 0x1c -P 3 -e
camcontrol: value 1800 is out of range for entry Interval Timer; clipping to -1

ls-10-min252-1# camcontrol modepage da4 -m 0x1c -P 3
Interval Timer:  -1258291201

The line of code that fixes this is changing....
#define RESOLUTION_MAX(size) ((resolution * (size) == 32)?              \
        (int)0xffffffff: (1 << (resolution * (size))) - 1)

to 

#define RESOLUTION_MAX(size) ((resolution * (size) == 32)?              \
        INT_MAX: (1 << (resolution * (size))) - 1)

Even with this change in place the error goes away, but the value reported by cam control after "changing" the modepage field does not change.
Essentially changing mode page fields is broken.

Proposed Fix
============

diff --git a/sbin/camcontrol/modeedit.c b/sbin/camcontrol/modeedit.c
index 00ab974..8262c3c 100644
--- a/sbin/camcontrol/modeedit.c
+++ b/sbin/camcontrol/modeedit.c
@@ -246,7 +246,7 @@ editentry_set(char *name, char *newvalue, int editonly)
  *     currently workaround it (even for int64's), so we have to kludge it.
  */
 #define        RESOLUTION_MAX(size) ((resolution * (size) == 32)?              \
-       (int)0xffffffff: (1 << (resolution * (size))) - 1)
+       INT_MAX: (1 << (resolution * (size))) - 1)
 
        assert(newvalue != NULL);
        if (*newvalue == '\0')
diff --git a/share/misc/scsi_modes b/share/misc/scsi_modes
index 781b8f1..80752e7 100644
--- a/share/misc/scsi_modes
+++ b/share/misc/scsi_modes
@@ -106,7 +106,7 @@
        {EBACKERR} t1
        {LogErr} t1
        {Reserved} *t4
-       {MRIE} b4
+       {MRIE} t4
        {Interval Timer} i4
        {Report Count} i4
 }

Reported by: Michael Baptist
Submitted by: Lars Skodje
Sponsored by: EMC / Isilon Storage Division
Comment 1 Enji Cooper freebsd_committer 2015-06-03 19:54:44 UTC
Scott/Warner: does the attached patch look ok for commit?
Comment 2 commit-hook freebsd_committer 2015-10-25 04:05:00 UTC
A commit references this bug:

Author: ngie
Date: Sun Oct 25 04:04:25 UTC 2015
New revision: 289915
URL: https://svnweb.freebsd.org/changeset/base/289915

Log:
  Use 't' (bit-field) not 'b' (bit-sized integral type) for describing MRIE (aka
  "Method of Reporting Informational Exceptions") in the SCSI mode database.
  T10/04-371 revision 2 (revision 4; page 2, table 1) describes it as a
  bit-field of 4 bits wide.

  1. http://www.t10.org/ftp/t10/document.04/04-371r2.pdf

  This a recommit of head@r289913 to fix the original commit message, in
  particular:
  - I incorrectly claimed that unit change was 'i' -> 't'.
  - The spec I reference in this commit is 2 decades newer than the one noted in
    r289913. The fields in the SCSI mode database are more complete in the newer
    spec, so it'll be easier for someone to decipher this commit if need be
    later.
  - I screwed up the bug entry in the previous commit message

  Pointyhat to: ngie (for botching up r289913)
  PR: 200619
  Reported by: Michael Baptist
  Submitted by: Lars Skodje
  Sponsored by: EMC / Isilon Storage Divisionf

Changes:
  head/share/misc/scsi_modes
Comment 3 commit-hook freebsd_committer 2015-10-25 04:37:04 UTC
A commit references this bug:

Author: ngie
Date: Sun Oct 25 04:37:01 UTC 2015
New revision: 289916
URL: https://svnweb.freebsd.org/changeset/base/289916

Log:
  Limit RESOLUTION_MAX to INT_MAX, not UINT_MAX (all spelled out) so the
  mode value isn't always clipped to -1 when (resolution * size) == 32, which
  would have been the case with values => {4i,32b,32t}.

  This seems to have been broken in r64382.

  MFC after: 1 week
  X-MFC with: r289915
  PR: 200619
  Reported by: Michael Baptist
  Submitted by: Lars Skodje
  Sponsored by: EMC / Isilon Storage Division

Changes:
  head/sbin/camcontrol/modeedit.c
Comment 4 commit-hook freebsd_committer 2015-11-05 07:49:17 UTC
A commit references this bug:

Author: ngie
Date: Thu Nov  5 07:48:48 UTC 2015
New revision: 290385
URL: https://svnweb.freebsd.org/changeset/base/290385

Log:
  MFC r289913,r289916:

  r289913:

  Use 't' (bits) not 'i' (bytes) for describing MRIE (aka
  "Method of Reporting Informational Exceptions") in the SCSI mode database as
  the field described in X3T10/94-190 (revision 4; page 2, table 1) [1.] is
  4 bits wide, not 4 bytes wide

  1. http://ftp.t10.org/ftp/t10/document.94/94-190r4.pdf

  Bug 200619
  Reported by: Michael Baptist <mbaptist@isilon.com>
  Submitted by: Lars Skodje <lskodje@isilon.com>
  Sponsored by: EMC / Isilon Storage Division

  r289916:

  Limit RESOLUTION_MAX to INT_MAX, not UINT_MAX (all spelled out) so the
  mode value isn't always clipped to -1 when (resolution * size) == 32, which
  would have been the case with values => {4i,32b,32t}.

  This seems to have been broken in r64382.

  PR: 200619
  Reported by: Michael Baptist
  Submitted by: Lars Skodje
  Sponsored by: EMC / Isilon Storage Division

Changes:
_U  stable/10/
  stable/10/sbin/camcontrol/modeedit.c
  stable/10/share/misc/scsi_modes
Comment 5 commit-hook freebsd_committer 2015-11-08 00:51:48 UTC
A commit references this bug:

Author: ngie
Date: Sun Nov  8 00:50:46 UTC 2015
New revision: 290527
URL: https://svnweb.freebsd.org/changeset/base/290527

Log:
  MFstable/10 r290385:

  MFC r289913,r289916:

  r289913:

  Use 't' (bits) not 'i' (bytes) for describing MRIE (aka
  "Method of Reporting Informational Exceptions") in the SCSI mode database as
  the field described in X3T10/94-190 (revision 4; page 2, table 1) [1.] is
  4 bits wide, not 4 bytes wide

  1. http://ftp.t10.org/ftp/t10/document.94/94-190r4.pdf

  Bug 200619
  Reported by: Michael Baptist <mbaptist@isilon.com>
  Submitted by: Lars Skodje <lskodje@isilon.com>
  Sponsored by: EMC / Isilon Storage Division

  r289916:

  Limit RESOLUTION_MAX to INT_MAX, not UINT_MAX (all spelled out) so the
  mode value isn't always clipped to -1 when (resolution * size) == 32, which
  would have been the case with values => {4i,32b,32t}.

  This seems to have been broken in r64382.

  PR: 200619
  Reported by: Michael Baptist
  Submitted by: Lars Skodje
  Sponsored by: EMC / Isilon Storage Division

Changes:
_U  stable/9/
_U  stable/9/sbin/
_U  stable/9/sbin/camcontrol/
  stable/9/sbin/camcontrol/modeedit.c
_U  stable/9/share/
_U  stable/9/share/misc/
  stable/9/share/misc/scsi_modes