Page MenuHomeFreeBSD

nvmecontrol: Implement Get/Set Feature for nvmecontrol
Needs ReviewPublic

Authored by wanpengqian_gmail.com on Oct 28 2021, 1:34 AM.

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 way as log pages.
Currently D33860 Intel vendor specify feature 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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 43874
Build 40762: arc lint + arc unit

Event Timeline

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

Print detail error message when failure.

rename function name to match other files's code style.

Generally, I like this. I only saw a few coding issues. I did not double check constants or pedantic compliance with the standard.

One other question: Are there vendor specific features that might benefit from the extensible module loading we do for commands and mode pages?

Finally, I see you're updating bhyve to support more things. Is there any sort of nvme test suite we can use to validate our nvme driver and associated tools like nvmecontrol and camcontrol? It's well beyond the scope for this commit, but is an interesting longer term project.

sbin/nvmecontrol/feature.c
63–87

I think all of these would be better in sys/dev/nvme/nvme.h. Can you move them there?

174

Are there #defines that can be used instead of numbers here?

sbin/nvmecontrol/nvmecontrol.c
185 ↗(On Diff #97992)

Would these be better as an array? Or as an array of value string pairs? I forget of these are a 'dense' set of numbers or 'sparse'. A simple array would be better for the former, and a list for the latter.

291 ↗(On Diff #97992)

Same comment as above.

In D32700#741111, @imp wrote:

One other question: Are there vendor specific features that might benefit from the extensible module loading we do for commands and mode pages?

Right, I think it is useful to support extensible module loading. modules can be installed from ports, and it can be maintained separately.
as more and more vendors use vendor specific commands/options.

Finally, I see you're updating bhyve to support more things. Is there any sort of nvme test suite we can use to validate our nvme driver and associated tools like nvmecontrol and camcontrol? It's well beyond the scope for this commit, but is an interesting longer term project.

Yes, I am planning to make bhyve's NVMe controller more REAL. Currently, I am not aware of any test suite, maybe Chuck knows. I am happy that tools are available for us that will make development more efficient.

sbin/nvmecontrol/feature.c
63–87

I will create a new Differential for these.

In D32700#741111, @imp wrote:

One other question: Are there vendor specific features that might benefit from the extensible module loading we do for commands and mode pages?

Right, I think it is useful to support extensible module loading. modules can be installed from ports, and it can be maintained separately.
as more and more vendors use vendor specific commands/options.

Finally, I see you're updating bhyve to support more things. Is there any sort of nvme test suite we can use to validate our nvme driver and associated tools like nvmecontrol and camcontrol? It's well beyond the scope for this commit, but is an interesting longer term project.

Yes, I am planning to make bhyve's NVMe controller more REAL. Currently, I am not aware of any test suite, maybe Chuck knows. I am happy that tools are available for us that will make development more efficient.

sbin/nvmecontrol/feature.c
589

Ideally, you'd handle this the same way that we handle the different log pages. There's a structure there that looks up the page type and then uses a routine that matches that type. There's also an optional vendor field for that.

If you were to have something like that, rather than this huge switch table, you could more easily extend it with loadable modules. That's where I was going with my other comments.

I'm happy to provide additional details if you think that's helpful...

define more define and other updates.

sbin/nvmecontrol/nvmecontrol.c
185 ↗(On Diff #97992)

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
268

"includes"

271

"summary"

272

"sub-commands"

318

"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
318

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

sbin/nvmecontrol/feature.c
443

"milliseconds"

Use logpage like procedure to process feature.

Fix varible naming issue for passing the build.

sbin/nvmecontrol/feature.c
589

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

sbin/nvmecontrol/feature.c
443

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

pauamma_gundo.com added inline comments.
sbin/nvmecontrol/nvmecontrol.8
196

Same typo as elsewhere.

268

Not clear to me whether it should be "... command includes" or "... commands include", but yes, number agreement.

273

"a feature" or "features", I think.

303

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.