Page MenuHomeFreeBSD

nvmecontrol: Implement Get/Set Feature for nvmecontrol
Needs ReviewPublic

Authored by wanpengqian_gmail.com on Oct 28 2021, 1:34 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 19, 6:20 AM
Unknown Object (File)
Sat, Mar 16, 10:32 AM
Unknown Object (File)
Feb 26 2024, 11:38 AM
Unknown Object (File)
Feb 8 2024, 8:39 PM
Unknown Object (File)
Feb 8 2024, 8:39 PM
Unknown Object (File)
Feb 8 2024, 8:39 PM
Unknown Object (File)
Feb 8 2024, 8:35 PM
Unknown Object (File)
Feb 8 2024, 8:35 PM
Tokens
"Like" token, awarded by wanpengqian_gmail.com.

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 42647
Build 39535: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sbin/nvmecontrol/nvmecontrol.c
185

Right, you are correct.
Actually, I found the sys/dev/nvme/nvme_qpair.c has already have these defines, and the function a not accessable outside.

static const char *
get_status_string(uint16_t sct, uint16_t sc)
{
        struct nvme_status_string *entry;

        switch (sct) {
        case NVME_SCT_GENERIC:
                entry = generic_status;
                break;
        case NVME_SCT_COMMAND_SPECIFIC:
                entry = command_specific_status;
                break;
        case NVME_SCT_MEDIA_ERROR:
                entry = media_error_status;
                break;
        case NVME_SCT_PATH_RELATED:
                entry = path_related_status;
                break;
        case NVME_SCT_VENDOR_SPECIFIC:
                return ("VENDOR SPECIFIC");
        default:
                return ("RESERVED");
        }

        while (entry->sc != 0xFFFF) {
                if (entry->sc == sc)
                        return (entry->str);
                entry++;
        }
        return (entry->str);
}

Can I make it non-static?

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.

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.

sbin/nvmecontrol/nvmecontrol.8
317 ↗(On Diff #98191)

Thank you very much. I am happy with this tool.

sbin/nvmecontrol/feature.c
444

"milliseconds"

Use logpage like procedure to process feature.

Fix varible naming issue for passing the build.

sbin/nvmecontrol/feature.c
590

Handle features just like logpage. and implement intel p3700's vendor features. It seems to be working.

sbin/nvmecontrol/feature.c
444

Thanks. I always spell millisecond to milisecond. It's my fault.

pauamma_gundo.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.

Fix Possible Spelling Mistake warning.
Update manpage

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.

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.

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

sbin/nvmecontrol/nvmecontrol.8
274 ↗(On Diff #101343)
275 ↗(On Diff #101343)
276 ↗(On Diff #101343)
279 ↗(On Diff #101343)
303 ↗(On Diff #101343)
  • combine all changes to one commit.
  • build check with CURRENT source.
  • correct manpages
  • remove a unnecessary include
  • fix typo
  • fix code style of switch

Found a line break after a sentence stop. You can run textproc/igor over your man page to find these.
Also, "mandoc -T lint" may also be helpful.

sbin/nvmecontrol/nvmecontrol.8
387 ↗(On Diff #112317)

You need a line break after the sentence stop here.