Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nvme: Support show-regs for nvmeof #2521

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

hmi-jeon
Copy link
Contributor

@hmi-jeon hmi-jeon commented Oct 2, 2024

Get properties only at offsets that support nvme fabric registers.
Since crto was not found in the nvmeof spec, it is removed.
Also, NSSR is only optional register. If the device is not supported by the optional register, the error is ignored.

Reviewed-by: Steven Seungcheol [email protected]
Signed-off-by: Minsik Jeon [email protected]

@hmi-jeon hmi-jeon force-pushed the support-nvmeof-show-regs branch 2 times, most recently from 31614f7 to 2496a31 Compare October 2, 2024 08:13
void *bar;
int err;

struct get_reg_config cfg = {
.human_readable = false,
.fabrics = false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't seem necessary. nvme_get_properties operates only on offsets. Anyway, the whole logic is already a bit complex so I'd rather avoid this type of additional info passed into the getters.

Copy link
Contributor Author

@hmi-jeon hmi-jeon Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. But If the device does not support NSSR, it will fail.
So If it is cfg.fabrics, In the nvme_get_single_property check optional register.
Is there a better fix?

Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, we already use it. This is so horrible code. I shouldn't have merged it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I understood it.
As you said, it looks like refactoring is needed.

nvme-print.c Show resolved Hide resolved
@hmi-jeon hmi-jeon force-pushed the support-nvmeof-show-regs branch 2 times, most recently from 9e234cc to 40393af Compare October 2, 2024 09:33
@igaw igaw merged commit b4628c3 into linux-nvme:master Oct 2, 2024
17 checks passed
@igaw
Copy link
Collaborator

igaw commented Oct 2, 2024

Thanks!

We should though rethink this part of the code. Really messy how it currently is.

Get properties only at offsets that support nvme fabric registers.
Since crto was not found in the nvmeof spec, it is removed.
Also, NSSR is only optional register. If the device is not supported by
the optional register, the error is ignored.

Reviewed-by: Steven Seungcheol <[email protected]>
Signed-off-by: Minsik Jeon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants