Skip to content

Conversation

@ikegami-t
Copy link
Contributor

The set feature is prohibited for the managemnet endpoint support.

@igaw
Copy link
Collaborator

igaw commented Jun 2, 2025

ping @jk-ozlabs

@igaw igaw requested a review from jk-ozlabs July 15, 2025 15:19
Copy link
Collaborator

@jk-ozlabs jk-ozlabs left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just a couple of doc tweaks that I would suggest.

Also, you have 'management' misspelt in both commit logs, if you end up respinning.

src/nvme/mi.h Outdated
* nvme_mi_admin_get_features_arbitration() - Get arbitration feature
* @ctrl: Controller to send command to
* @sel: Select which type of attribute to return, see &enum nvme_get_features_sel
* @result: The command completion result from CQE dword0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest rewording this to indicate that the feature data is returned in this argument; the CQE dword0 reference is a bit opaque.

src/nvme/mi.h Outdated
* nvme_mi_admin_get_features_power_mgmt() - Get power management feature
* @ctrl: Controller to send command to
* @sel: Select which type of attribute to return, see &enum nvme_get_features_sel
* @result: The command completion result from CQE dword0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same with this doc argument

src/nvme/mi.h Outdated
* @ps: Power State
* @wh: Workload Hint
* @save: Save value across power states
* @result: The command completion result from CQE dword0
Copy link
Collaborator

Choose a reason for hiding this comment

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

... and this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Actually only zero result value set for the set feature command.

The set feature is prohibited for the management endpoint support.

Signed-off-by: Tokunori Ikegami <[email protected]>
The feature is optional for the management endpoint support.

Signed-off-by: Tokunori Ikegami <[email protected]>
@ikegami-t
Copy link
Contributor Author

Fixed for the review comments. Thank you.

Copy link
Collaborator

@jk-ozlabs jk-ozlabs left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@igaw igaw merged commit bfd251b into linux-nvme:master Jul 22, 2025
12 checks passed
@igaw
Copy link
Collaborator

igaw commented Jul 22, 2025

Thanks!

@ikegami-t
Copy link
Contributor Author

Thanks for your reviewing and merging. Seems we need to wait the next libnvme version release to use the functions added by the PR. Is this understanding correct?

@igaw
Copy link
Collaborator

igaw commented Jul 23, 2025

I should work to them in these function in a freshly compiled libnvme with a freshly compiled nvme-cli together. When libnvme is released the symbol name will get a new version. This will just break the unreleased binaries. So it's fine to start using these newly added function in nvme-cli.

@ikegami-t
Copy link
Contributor Author

Thank you for your comment. Just realized the PR changes map file is not for mi then the build link errors below caused on my local envrionment so just fixed the errors by the PR #1039 . Sorry for that.

tokunori@tokunori-desktop:~/nvme-cli$ make -j32
meson compile -C .build
INFO: autodetecting backend as ninja
INFO: calculating backend command to run: /home/tokunori/.local/bin/ninja -C /home/tokunori/nvme-cli/.build
ninja: Entering directory `/home/tokunori/nvme-cli/.build'
[72/72] Linking target nvme
FAILED: nvme 
cc  -o nvme nvme.p/nbft.c.o nvme.p/fabrics.c.o nvme.p/nvme.c.o nvme.p/nvme-models.c.o nvme.p/nvme-print.c.o nvme.p/nvme-print-stdout.c.o nvme.p/nvme-print-binary.c.o nvme.p/nvme-rpmb.c.o nvme.p/nvme-wrap.c.o nvme.p/plugin.c.o nvme.p/libnvme-wrap.c.o nvme.p/logging.c.o nvme.p/nvme-print-json.c.o nvme.p/ccan_ccan_hash_hash.c.o nvme.p/ccan_ccan_htable_htable.c.o nvme.p/ccan_ccan_ilog_ilog.c.o nvme.p/ccan_ccan_likely_likely.c.o nvme.p/ccan_ccan_list_list.c.o nvme.p/ccan_ccan_str_debug.c.o nvme.p/ccan_ccan_str_str.c.o nvme.p/ccan_ccan_strset_strset.c.o nvme.p/plugins_amzn_amzn-nvme.c.o nvme.p/plugins_dapustor_dapustor-nvme.c.o nvme.p/plugins_dell_dell-nvme.c.o nvme.p/plugins_dera_dera-nvme.c.o nvme.p/plugins_fdp_fdp.c.o nvme.p/plugins_huawei_huawei-nvme.c.o nvme.p/plugins_innogrit_innogrit-nvme.c.o nvme.p/plugins_inspur_inspur-nvme.c.o nvme.p/plugins_intel_intel-nvme.c.o nvme.p/plugins_mangoboost_mangoboost-nvme.c.o nvme.p/plugins_memblaze_memblaze-nvme.c.o nvme.p/plugins_micron_micron-nvme.c.o nvme.p/plugins_nbft_nbft-plugin.c.o nvme.p/plugins_netapp_netapp-nvme.c.o nvme.p/plugins_nvidia_nvidia-nvme.c.o nvme.p/plugins_sandisk_sandisk-nvme.c.o nvme.p/plugins_sandisk_sandisk-utils.c.o nvme.p/plugins_scaleflux_sfx-nvme.c.o nvme.p/plugins_seagate_seagate-nvme.c.o nvme.p/plugins_shannon_shannon-nvme.c.o nvme.p/plugins_ssstc_ssstc-nvme.c.o nvme.p/plugins_toshiba_toshiba-nvme.c.o nvme.p/plugins_transcend_transcend-nvme.c.o nvme.p/plugins_virtium_virtium-nvme.c.o nvme.p/plugins_wdc_wdc-nvme.c.o nvme.p/plugins_wdc_wdc-utils.c.o nvme.p/plugins_ymtc_ymtc-nvme.c.o nvme.p/plugins_zns_zns.c.o nvme.p/plugins_feat_feat-nvme.c.o nvme.p/plugins_lm_lm-nvme.c.o nvme.p/plugins_lm_lm-print.c.o nvme.p/plugins_lm_lm-print-stdout.c.o nvme.p/plugins_lm_lm-print-binary.c.o nvme.p/plugins_lm_lm-print-json.c.o nvme.p/plugins_ocp_ocp-utils.c.o nvme.p/plugins_ocp_ocp-nvme.c.o nvme.p/plugins_ocp_ocp-clear-features.c.o nvme.p/plugins_ocp_ocp-smart-extended-log.c.o nvme.p/plugins_ocp_ocp-fw-activation-history.c.o nvme.p/plugins_ocp_ocp-telemetry-decode.c.o nvme.p/plugins_ocp_ocp-hardware-component-log.c.o nvme.p/plugins_ocp_ocp-print.c.o nvme.p/plugins_ocp_ocp-print-stdout.c.o nvme.p/plugins_ocp_ocp-print-binary.c.o nvme.p/plugins_ocp_ocp-print-json.c.o nvme.p/plugins_sed_sed.c.o nvme.p/plugins_sed_sedopal_cmd.c.o nvme.p/plugins_solidigm_solidigm-nvme.c.o nvme.p/plugins_solidigm_solidigm-id-ctrl.c.o nvme.p/plugins_solidigm_solidigm-util.c.o nvme.p/plugins_solidigm_solidigm-smart.c.o nvme.p/plugins_solidigm_solidigm-garbage-collection.c.o nvme.p/plugins_solidigm_solidigm-latency-tracking.c.o nvme.p/plugins_solidigm_solidigm-log-page-dir.c.o nvme.p/plugins_solidigm_solidigm-telemetry.c.o nvme.p/plugins_solidigm_solidigm-internal-logs.c.o nvme.p/plugins_solidigm_solidigm-market-log.c.o nvme.p/plugins_solidigm_solidigm-temp-stats.c.o nvme.p/plugins_solidigm_solidigm-get-drive-info.c.o nvme.p/plugins_solidigm_solidigm-ocp-version.c.o nvme.p/plugins_solidigm_solidigm-workload-tracker.c.o nvme.p/plugins_solidigm_solidigm-telemetry_cod.c.o nvme.p/plugins_solidigm_solidigm-telemetry_header.c.o nvme.p/plugins_solidigm_solidigm-telemetry_config.c.o nvme.p/plugins_solidigm_solidigm-telemetry_data-area.c.o nvme.p/plugins_solidigm_solidigm-telemetry_nlog.c.o nvme.p/util_argconfig.c.o nvme.p/util_base64.c.o nvme.p/util_crc32.c.o nvme.p/util_mem.c.o nvme.p/util_sighdl.c.o nvme.p/util_suffix.c.o nvme.p/util_types.c.o nvme.p/util_utils.c.o nvme.p/util_json.c.o -Wl,--as-needed -Wl,--no-undefined -Wl,-rpath,/usr/local/lib/x86_64-linux-gnu:/usr/local/lib -Wl,-rpath-link,/usr/local/lib/x86_64-linux-gnu -Wl,-rpath-link,/usr/local/lib -Wl,--start-group -ldl /usr/local/lib/x86_64-linux-gnu/libnvme.so /usr/local/lib/x86_64-linux-gnu/libnvme-mi.so /usr/local/lib/libjson-c.so -Wl,--end-group
/usr/bin/ld: nvme.p/nvme-wrap.c.o: in function `nvme_cli_get_features_arbitration':
/home/tokunori/nvme-cli/.build/../nvme-wrap.c:125:(.text+0x934): undefined reference to `nvme_mi_admin_get_features_arbitration'
/usr/bin/ld: nvme.p/nvme-wrap.c.o: in function `nvme_cli_get_features_power_mgmt':
/home/tokunori/nvme-cli/.build/../nvme-wrap.c:131:(.text+0x974): undefined reference to `nvme_mi_admin_get_features_power_mgmt'
/usr/bin/ld: nvme.p/nvme-wrap.c.o: in function `nvme_cli_features_power_mgmt':
/home/tokunori/nvme-cli/.build/../nvme-wrap.c:150:(.text+0xa41): undefined reference to `nvme_mi_admin_set_features_power_mgmt'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
make: *** [Makefile:23: nvme] エラー 1

@igaw
Copy link
Collaborator

igaw commented Jul 23, 2025

The wrap file in nvme-cli needs to be updated, so it gets the libnvme changes.

Nevermind, I didn't not get your point. But after seeing your PR to fixup the map files it makes sense. Thanks!

@ikegami-t
Copy link
Contributor Author

Thank you so much. Just updated the nvme-cli wrap file changes (linux-nvme/nvme-cli#2873).

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.

3 participants