Differential D59536

camcontrol: Add an nvfeature subcommand for NVMe Get/Set Features
Needs ReviewPublic
Actions

Authored by ken on Wed, Sep 9, 3:35 PM.

Details

Reviewers

None

Group Reviewers

cam

Summary

Add NVMe Get Features / Set Features support to camcontrol(8):

camcontrol nvfeature ndaX -a <get|set> -f feature [-d data]
    [-s num] [-U]

Currently, only the hostid feature is fully implemented. The
hostid value for set is specified with -d as a UUID or an integer
in decimal, hex, or octal, and -U outputs the hostid as a UUID on
get.

The -s argument exposes the feature select / save fields from the
NVMe specification. If bit 4 of the Optional NVM Command Support
(ONCS) field of the Identify Controller data is set, each feature
has a default, saved and current value, plus a capabilities mask:

For get, -s selects which to display: 0 (current), 1 (default), 2 (saved) or 3 (capabilities).
For set, -s is the save flag; a value of 1 sets the saved value as well as the current value, so the feature persists across an NVMe reset. Setting the save flag only works if the feature's capability mask includes saveable.

sbin/camcontrol/nvfeature.c:

New file, the camcontrol(8) nvfeature subcommand for getting
and setting NVMe features.

sbin/camcontrol/camcontrol.c:

Add the nvfeature subcommand.

sbin/camcontrol/camcontrol.h:

Add the nvmefeature() prototype.

sbin/camcontrol/Makefile:

Add nvfeature.c to the build.

sbin/camcontrol/camcontrol.8:

Document the nvfeature subcommand.

Co-authored-by: Reid Linnemann <reidl@spectralogic.com>
Co-authored-by: Dave Baukus <daveb@spectralogic.com>
Sponsored by: Spectra Logic

Depends on D59535

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 76717
Build 73600: arc lint + arc unit

Event Timeline

ken created this revision.Wed, Sep 9, 3:35 PM

Herald added subscribers: ziaee, imp. · View Herald TranscriptWed, Sep 9, 3:35 PM

ken requested review of this revision.Wed, Sep 9, 3:35 PM

Harbormaster completed remote builds in B76701: Diff 186279.Wed, Sep 9, 3:35 PM

Fix spelling of the changeable capability name

Harbormaster completed remote builds in B76702: Diff 186281.Wed, Sep 9, 3:36 PM

ken edited the summary of this revision. (Show Details)Wed, Sep 9, 3:41 PM

imp added inline comments.Wed, Sep 9, 8:05 PM

sbin/camcontrol/camcontrol.8
421	same comment about nv prefix as before. Why not just feature.
3029	a spot check shows these are all in the tables below. How are they not implemented?
sbin/camcontrol/nvfeature.c
229	This is weird and likely deserves a comment.

Adapt to the wire-byte-order hostid convention from D59531: drop the pre-print byte swapping (which also fixes a bug where the 8-byte hostid was truncated through a uint8_t on big-endian), decode the UUID display with uuid_dec_be() to match the uuid_enc_be() used on set, and use le64enc() to build the 8-byte set payload.

Harbormaster completed remote builds in B76717: Diff 186301.Wed, Sep 9, 8:31 PM

ken mentioned this in D59531: nvme: Add reservation, format and features command building support.Wed, Sep 9, 8:43 PM

linnemannr_gmail.com added a subscriber: linnemannr_gmail.com.Wed, Sep 9, 10:24 PM

linnemannr_gmail.com added inline comments.Wed, Sep 9, 10:44 PM

sbin/camcontrol/camcontrol.8
421	I chose the nv prefix to make it obvious that a subcommand was for nvme devices only. Honestly I think camcontrol is in need of some reorganization at this point to add one or more subcommand layers and split the documentation out to the top layer subcommands. e.g. if we had a 'camcontrol nvme' subcommand, it would be obvious that 'camcontrol nvme feature' is specific to nvme devices.
3029	The constants are defined, yes, but the nvfeature_print_, nvfeature_get_, and nvfeature_set_* transforms between a varargs string and the feature value type aren't implemented for anything but hostid.

linnemannr_gmail.com added inline comments.Wed, Sep 9, 10:50 PM

sbin/camcontrol/nvfeature.c
229	Can you elaborate on what is weird to you here? Are you referring to the entire uuid stringification or the set-error/goto pattern for error handling, or something else?