Details

Reviewers

imp
chuck

Group Reviewers

manpages

Summary

sbin/nvmecontrol didn't support Get/Set Feature yet,

this patch will add following sub commands

feature get
feature set
feature summary

Use the same implementation as logpages, support vendor modules.
Intel vendor specific features were also added.

Test Plan

execute each commands and compare the result with Linux's nvme command.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 42648
Build 39536: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

use #define instead of numbers.

Harbormaster completed remote builds in B42647: Diff 98174.Nov 8 2021, 5:22 AM

copy code from sys/dev/nvme/nvme_qpair.c,
move _SHIFT _MASK difinition to sys/dev/nvme/nvme.h, seperate ticket: D32885
remove these difinitions when D32885 is accepted.

Harbormaster completed remote builds in B42648: Diff 98175.Nov 8 2021, 6:22 AM

wanpengqian_gmail.com edited the summary of this revision. (Show Details)Nov 8 2021, 6:26 AM

update manpage

Herald added a reviewer: manpages. · View Herald TranscriptNov 8 2021, 7:33 AM

Harbormaster completed remote builds in B42650: Diff 98177.Nov 8 2021, 7:33 AM

fix typo.

Harbormaster completed remote builds in B42651: Diff 98178.Nov 8 2021, 7:37 AM

Add command examples.

Harbormaster completed remote builds in B42652: Diff 98179.Nov 8 2021, 7:54 AM

update manpage.

Harbormaster completed remote builds in B42656: Diff 98191.Nov 8 2021, 1:06 PM

rpokala added a subscriber: rpokala.Nov 8 2021, 7:11 PM

rpokala added inline comments.

sbin/nvmecontrol/nvmecontrol.8
267 ↗	(On Diff #98191)	"includes"
270 ↗	(On Diff #98191)	"summary"
271 ↗	(On Diff #98191)	"sub-commands"
317 ↗	(On Diff #98191)	"summary" I suggest running the manpage through `igor`; it will identify common misspellings, and also find incorrect tags.

update manpage as comments suggested.
also add print Timestamp for details.

Harbormaster completed remote builds in B42693: Diff 98285.Nov 10 2021, 3:17 AM

wanpengqian_gmail.com added inline comments.Nov 10 2021, 3:18 AM

sbin/nvmecontrol/nvmecontrol.8
317 ↗	(On Diff #98191)	Thank you very much. I am happy with this tool.

rpokala added inline comments.Nov 10 2021, 6:25 AM

sbin/nvmecontrol/feature.c
444	"milliseconds"

correct the timestamp output.

Harbormaster completed remote builds in B42715: Diff 98341.Nov 11 2021, 1:56 AM

Use logpage like procedure to process feature.

Harbormaster completed remote builds in B43519: Diff 100387.Dec 21 2021, 8:17 AM

wanpengqian_gmail.com added a parent revision: D32885: nvme: Add some definitions for Features.Dec 21 2021, 8:18 AM

add missing feature.c

Harbormaster completed remote builds in B43520: Diff 100388.Dec 21 2021, 8:22 AM

Fix varible naming issue for passing the build.

Harbormaster completed remote builds in B43521: Diff 100389.Dec 21 2021, 8:28 AM

wanpengqian_gmail.com added inline comments.Dec 21 2021, 8:32 AM

sbin/nvmecontrol/feature.c
590	Handle features just like logpage. and implement intel p3700's vendor features. It seems to be working.

Fix typo and code style.

Harbormaster completed remote builds in B43522: Diff 100390.Dec 21 2021, 8:36 AM

wanpengqian_gmail.com added inline comments.Dec 21 2021, 8:38 AM

sbin/nvmecontrol/feature.c
444	Thanks. I always spell millisecond to milisecond. It's my fault.

pauamma_chezdork.com added a subscriber: pauamma_chezdork.com.Dec 26 2021, 12:54 AM

pauamma_chezdork.com added inline comments.

sbin/nvmecontrol/nvmecontrol.8
174 ↗	(On Diff #98191)	Same typo as elsewhere.
267 ↗	(On Diff #98191)	Not clear to me whether it should be "... command includes" or "... commands include", but yes, number agreement.
272 ↗	(On Diff #98191)	"a feature" or "features", I think.
302 ↗	(On Diff #98191)	I don't think "shall" as a future imperative is common anymore, and it will likely confuse EFL users.

update as comments suggested.

Harbormaster completed remote builds in B43663: Diff 100802.Jan 1 2022, 3:23 PM

Fix Possible Spelling Mistake warning.
Update manpage

Harbormaster completed remote builds in B43664: Diff 100803.Jan 1 2022, 3:31 PM

wanpengqian_gmail.com marked 12 inline comments as done.Jan 1 2022, 3:32 PM

wanpengqian_gmail.com retitled this revision from nvme: [WIP] implement Get/Set Feature for nvmecontrol to nvme: Implement Get/Set Feature for nvmecontrol.Jan 1 2022, 3:36 PM

wanpengqian_gmail.com edited the summary of this revision. (Show Details)

Manual page LGTM now. Can't speak about the rest.

seperate intel's feature patch to another review.

Harbormaster completed remote builds in B43874: Diff 101343.Jan 12 2022, 4:15 AM

wanpengqian_gmail.com added a child revision: D33860: nvmecontrol: Implement feature command for intel.Jan 12 2022, 4:22 AM

wanpengqian_gmail.com retitled this revision from nvme: Implement Get/Set Feature for nvmecontrol to nvmecontrl: Implement Get/Set Feature for nvmecontrol.Jan 12 2022, 6:14 AM

wanpengqian_gmail.com edited the summary of this revision. (Show Details)

wanpengqian_gmail.com retitled this revision from nvmecontrl: Implement Get/Set Feature for nvmecontrol to nvmecontrol: Implement Get/Set Feature for nvmecontrol.

%anual page change still LGTM.

wanpengqian_gmail.com mentioned this in D33860: nvmecontrol: Implement feature command for intel.Jan 12 2022, 11:06 PM

wanpengqian_gmail.com planned changes to this revision.Aug 26 2022, 12:08 AM

With these edits, the manual page LGTM at this point; I'll rereview it when it's no longer a WIP.++