Details

Reviewers

marcel
bapt

Group Reviewers

manpages

Commits

rS287988: Improve and expand sesutil(8)

Summary

Return an error if no matching device is found when the locate command is run

Enhance the locate command to be able to address drive bays with no drive, or where the SES controller has not made the mapping to the device name

Added the fault command, similar to locate, but a different SES property. On some of my controllers locate blinks the activity light, others the fault light. The fault command keeps the fault light on constant.

Improve the usage() output and use it everywhere

Added the map command, displays all elements connected to each (or the specified) ses(4) controller

Added the status command, returns the overall status of the ses(4) controller

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

allanjude updated this revision to Diff 8535.Sep 7 2015, 12:49 AM

allanjude retitled this revision from to Improve and expand sesutil(8).

allanjude updated this object.

allanjude edited the test plan for this revision. (Show Details)

allanjude added reviewers: bapt, marcel.

allanjude added a subscriber: araujo.

Herald added a reviewer: manpages. · View Herald TranscriptSep 7 2015, 12:49 AM

Herald added a subscriber: imp. · View Herald Transcript

do_locate and do_fault are really similar they should be merged into one function
why limiting -u to a given sesid? there is no reason why I could not to locate -u /dev/ses0 all on or locate -u /dev/ses0 daX on

In general I'm wondering if it won't be a good idea to be turn the command into:
sesutil [-u /dev/sesX] [cmd] [cmdarguments]

it seems allo arguments are taking -u anyway so doing it once for all would probably make sense.

usr.sbin/sesutil/sesutil.c
174 ↗	(On Diff #8535)	The main function should in that case not increment by 2 but only by one to avoid doing that
189 ↗	(On Diff #8535)	I do not like calling usage here which will show you all possible commands, while the user here is explicitly calling a given command and would expect only the syntax of the given command to be printed
193 ↗	(On Diff #8535)	a simple strtol would also do the trick here (sorry I really don't like sscanf un general)

In D3580#73943, @bapt wrote:

do_locate and do_fault are really similar they should be merged into one function

Yeah, should move the 'fault = true/false' flag to do_locate.

why limiting -u to a given sesid? there is no reason why I could not to locate -u /dev/ses0 all on or locate -u /dev/ses0 daX on

SES includes things that are not disks, like temperature sensors, fans, etc. I wasn't sure we'd want to set those flags on non-disk items. After reading the SES spec, it seems those flags are always locate/fault, so maybe it does make sense.

Element 0x19: Temperature Sensors, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'Temperature Sensors'
Element 0x1a: Temperature Sensors, status: OK (0x01 0x00 0x37 0x00), descriptor: 'Temperature'
Element 0x1b: Cooling, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'Fans'
Element 0x1c: Cooling, status: OK (0x01 0x01 0xfe 0x21), descriptor: 'Fan1'
Element 0x1d: Cooling, status: OK (0x01 0x02 0x04 0x21), descriptor: 'Fan2'
Element 0x1e: Cooling, status: OK (0x01 0x02 0x1c 0x21), descriptor: 'Fan3'
Element 0x1f: Cooling, status: Not Available (0x07 0x00 0x00 0x10), descriptor: 'JBOD_Fan1'
Element 0x20: Cooling, status: Not Available (0x07 0x00 0x00 0x10), descriptor: 'JBOD_Fan2'
Element 0x21: Voltage Sensor, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'Voltage Sensors'
Element 0x22: Voltage Sensor, status: OK (0x01 0x00 0x01 0xf9), descriptor: '5V'
Element 0x23: Voltage Sensor, status: OK (0x01 0x00 0x04 0x9d), descriptor: '12V'
Element 0x24: Power Supply, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'Power Supplies'
Element 0x25: Power Supply, status: Not Available, Prd.Fail (0x47 0x80 0x00 0x20), descriptor: 'Power Supply 1'
Element 0x26: Power Supply, status: Not Available, Prd.Fail (0x47 0x80 0x00 0x20), descriptor: 'Power Supply 2'
Element 0x27: Enclosure, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'Enclosure'
Element 0x28: Enclosure, status: OK (0x01 0x00 0x00 0x00), descriptor: 'Enclosure'
Element 0x29: SAS Expander, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'SAS Expanders'
Element 0x2a: SAS Expander, status: OK (0x01 0x00 0x00 0x00), descriptor: 'Primary Expander'

In general I'm wondering if it won't be a good idea to be turn the command into:
sesutil [-u /dev/sesX] [cmd] [cmdarguments]

it seems allo arguments are taking -u anyway so doing it once for all would probably make sense.

yes, this probably makes sense, and not providing -u = all. all in the case of a specific SES id# is ambiguous though, and likely needs to be an error.

allanjude added inline comments.Sep 7 2015, 6:47 AM

usr.sbin/sesutil/sesutil.c
174 ↗	(On Diff #8535)	this was because not every function took the argument, but if we move -u to be in main, we avoid this issue all together
189 ↗	(On Diff #8535)	Yeah, maybe make usage take a 2nd argument, the subcommand? I just got annoyed updating the usage likes when they appeared three+ times inside the locate function.
193 ↗	(On Diff #8535)	ok

In D3580#73947, @allanjude wrote:

In D3580#73943, @bapt wrote:

do_locate and do_fault are really similar they should be merged into one function

Yeah, should move the 'fault = true/false' flag to do_locate.

And probably rename it: do_led or something like that

why limiting -u to a given sesid? there is no reason why I could not to locate -u /dev/ses0 all on or locate -u /dev/ses0 daX on

SES includes things that are not disks, like temperature sensors, fans, etc. I wasn't sure we'd want to set those flags on non-disk items. After reading the SES spec, it seems those flags are always locate/fault, so maybe it does make sense.

Element 0x19: Temperature Sensors, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'Temperature Sensors'
Element 0x1a: Temperature Sensors, status: OK (0x01 0x00 0x37 0x00), descriptor: 'Temperature'
Element 0x1b: Cooling, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'Fans'
Element 0x1c: Cooling, status: OK (0x01 0x01 0xfe 0x21), descriptor: 'Fan1'
Element 0x1d: Cooling, status: OK (0x01 0x02 0x04 0x21), descriptor: 'Fan2'
Element 0x1e: Cooling, status: OK (0x01 0x02 0x1c 0x21), descriptor: 'Fan3'
Element 0x1f: Cooling, status: Not Available (0x07 0x00 0x00 0x10), descriptor: 'JBOD_Fan1'
Element 0x20: Cooling, status: Not Available (0x07 0x00 0x00 0x10), descriptor: 'JBOD_Fan2'
Element 0x21: Voltage Sensor, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'Voltage Sensors'
Element 0x22: Voltage Sensor, status: OK (0x01 0x00 0x01 0xf9), descriptor: '5V'
Element 0x23: Voltage Sensor, status: OK (0x01 0x00 0x04 0x9d), descriptor: '12V'
Element 0x24: Power Supply, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'Power Supplies'
Element 0x25: Power Supply, status: Not Available, Prd.Fail (0x47 0x80 0x00 0x20), descriptor: 'Power Supply 1'
Element 0x26: Power Supply, status: Not Available, Prd.Fail (0x47 0x80 0x00 0x20), descriptor: 'Power Supply 2'
Element 0x27: Enclosure, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'Enclosure'
Element 0x28: Enclosure, status: OK (0x01 0x00 0x00 0x00), descriptor: 'Enclosure'
Element 0x29: SAS Expander, status: Unsupported (0x00 0x00 0x00 0x00), descriptor: 'SAS Expanders'
Element 0x2a: SAS Expander, status: OK (0x01 0x00 0x00 0x00), descriptor: 'Primary Expander'

@mav do you have an opinion?

In general I'm wondering if it won't be a good idea to be turn the command into:
sesutil [-u /dev/sesX] [cmd] [cmdarguments]

it seems allo arguments are taking -u anyway so doing it once for all would probably make sense.

yes, this probably makes sense, and not providing -u = all. all in the case of a specific SES id# is ambiguous though, and likely needs to be an error.

yup

bapt added inline comments.Sep 7 2015, 8:41 AM

usr.sbin/sesutil/sesutil.c
189 ↗	(On Diff #8535)	I do like the idea of a second argument

Revised with feedback from bapt@

Herald edited edge metadata. · View Herald TranscriptSep 7 2015, 6:48 PM

Cleanup man page and usage

Herald edited edge metadata. · View Herald TranscriptSep 7 2015, 7:11 PM

Fix check requiring -u if 'disk' is numeric (for locate and fault)

Herald edited edge metadata. · View Herald TranscriptSep 7 2015, 7:24 PM

wblock added a subscriber: wblock.Sep 8 2015, 5:05 PM

wblock added inline comments.

usr.sbin/sesutil/sesutil.8
38 ↗	(On Diff #8561)	Are these flags really all optional? It implies that I can use `sesutil fault off` or `sesutils fault all`, but is ambiguous about what those might assume for the unspecified options.
65 ↗	(On Diff #8561)	`mdoc warning: Using a macro as first argument cancels effect of .Cm (#65)`
82 ↗	(On Diff #8561)	`mdoc warning: Using a macro as first argument cancels effect of .Cm (#82)`

allanjude added inline comments.Sep 8 2015, 6:28 PM

usr.sbin/sesutil/sesutil.8
38 ↗	(On Diff #8563)	That is the tricky bit. -u /dev/sesN is optional, except in the case where you specify a sesid. that is why it is optional for: seslocate fault da12 on but not: seslocate fault -u /dev/ses0 7 on Is there a better macro to use for 'pick 1 of [ disk \| all ]' ?
65 ↗	(On Diff #8563)	this is a mistake, will fix

Fix mandoc lint errors pointed out by wblock

Herald edited edge metadata. · View Herald TranscriptSep 10 2015, 12:30 AM

Testing the code the map is showing weird things:

Element 79, Type: SAS Connector
        Status: OK, LED=Locate (0x01 0x20 0xff 0x00)
        Description: SAS Connector 21
Element 80, Type: SAS Connector
        Status: OK, LED=Locate (0x01 0x20 0xff 0x00)
        Description: SAS Connector 22

Element 55, Type: Voltage Sensor
        Status: OK, LED=Fault (0x01 0x00 0x04 0xa9)
        Description: Voltage 12.00V

for reason I do not understand the fault command does not work on
SAS2308 PCI-Express Fusion-MPT SAS-2

I got it working with a previous code, I'm investigating the issue

sesutil map -u /dev/ses0

Usage: sesutil map [-u /dev/ses<N>]

Print a map of the devices managed by the enclosure

usage and man page are not up to date

In D3580#74733, @bapt wrote:

Testing the code the map is showing weird things:

Element 79, Type: SAS Connector
        Status: OK, LED=Locate (0x01 0x20 0xff 0x00)
        Description: SAS Connector 21
Element 80, Type: SAS Connector
        Status: OK, LED=Locate (0x01 0x20 0xff 0x00)
        Description: SAS Connector 22

Element 55, Type: Voltage Sensor
        Status: OK, LED=Fault (0x01 0x00 0x04 0xa9)
        Description: Voltage 12.00V

Fixed, now only check the LEDs if 'element type' is a drive slot

In D3580#74738, @bapt wrote:
sesutil map -u /dev/ses0

Usage: sesutil map [-u /dev/ses<N>]
Print a map of the devices managed by the enclosure
usage and man page are not up to date

had to switch to getopt_long. getopt() stops parsing as soon as it encounters something that is not an option, instead of 'doing the right thing'

In D3580#74737, @bapt wrote:

for reason I do not understand the fault command does not work on
SAS2308 PCI-Express Fusion-MPT SAS-2

I got it working with a previous code, I'm investigating the issue

On some of my machines, the only difference is:
locate: red light blinks
fault: red light always on

Whereas on another, locate blinks the blue activity led, and fault lights up the red LED

Updated with feedback from bapt

Herald edited edge metadata. · View Herald TranscriptSep 10 2015, 5:46 PM

Restore a blank line I accidently removed

Herald edited edge metadata. · View Herald TranscriptSep 10 2015, 5:47 PM

In D3580#74781, @allanjude wrote:

In D3580#74737, @bapt wrote:

for reason I do not understand the fault command does not work on
SAS2308 PCI-Express Fusion-MPT SAS-2

I got it working with a previous code, I'm investigating the issue

On some of my machines, the only difference is:
locate: red light blinks
fault: red light always on

Whereas on another, locate blinks the blue activity led, and fault lights up the red LED

On my SAS locate is a green LED and faut is a red light, I manage to get the fault working with slightly the same code that is why I do not understand why yours is not workin :-

bapt accepted this revision.Sep 11 2015, 8:44 AM

bapt edited edge metadata.

This revision is now accepted and ready to land.Sep 11 2015, 8:44 AM

In D3580#74932, @bapt wrote:

In D3580#74781, @allanjude wrote:

In D3580#74737, @bapt wrote:

for reason I do not understand the fault command does not work on
SAS2308 PCI-Express Fusion-MPT SAS-2

I got it working with a previous code, I'm investigating the issue

On some of my machines, the only difference is:
locate: red light blinks
fault: red light always on

Whereas on another, locate blinks the blue activity led, and fault lights up the red LED

On my SAS locate is a green LED and faut is a red light, I manage to get the fault working with slightly the same code that is why I do not understand why yours is not workin :-

What did you have to do different to get the fault light on yours?

Note: I saw some strange behaviour where sometimes it seemed like the 'get' IOCTL was cached or something. I would have to do the operation twice to make it apply. Try the 'not working' code by running it twice, and see what happens. Or just run 'map' after setting the fault light on/off, and see if the status has changed or not. if not, set it again, then see

Fix manpage markup to address feedback from wblock

This revision now requires review to proceed.Sep 16 2015, 3:25 AM

Herald edited edge metadata. · View Herald TranscriptSep 16 2015, 3:25 AM

Closed by commit rS287988: Improve and expand sesutil(8) (authored by allanjude). · Explain WhySep 19 2015, 4:36 PM

This revision was automatically updated to reflect the committed changes.

Improve and expand sesutil(8)
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

sesutil map -u /dev/ses0

sesutil map -u /dev/ses0

Revision Contents
Changeset List

Diff 8845

head/usr.sbin/sesutil/Makefile

head/usr.sbin/sesutil/eltsub.h

head/usr.sbin/sesutil/eltsub.c

head/usr.sbin/sesutil/sesutil.8

head/usr.sbin/sesutil/sesutil.c

Improve and expand sesutil(8)ClosedPublicActions

Details

Diff Detail

Event Timeline

sesutil map -u /dev/ses0

sesutil map -u /dev/ses0

Revision ContentsChangeset List

Diff 8845

head/usr.sbin/sesutil/Makefile

head/usr.sbin/sesutil/eltsub.h

head/usr.sbin/sesutil/eltsub.c

head/usr.sbin/sesutil/sesutil.8

head/usr.sbin/sesutil/sesutil.c

Improve and expand sesutil(8)
ClosedPublic
Actions

Revision Contents
Changeset List