Skip to content

Conversation

@MaisenbacherD
Copy link

@MaisenbacherD MaisenbacherD commented Sep 11, 2025

This adopts the libnvme API changes until and including nvme-experiments/libnvme@b6c6bd5

I also cross-checked the specs one more time for the libnvme API changes that were picked up with these commits.

@MaisenbacherD
Copy link
Author

@igaw I did not touch spec related variable names for vendor plugins (nvme: nvme_set_features use nvme_passthru_cmd directly). What do think of this decision?

@igaw
Copy link

igaw commented Sep 12, 2025

@igaw I did not touch spec related variable names for vendor plugins (nvme: nvme_set_features use nvme_passthru_cmd directly). What do think of this decision?

I would not touch the vendor plugins either. Only the own plugins lm, feature, ocp IIRC

@igaw
Copy link

igaw commented Sep 12, 2025

Looks good to me. Let me pull them.

@igaw
Copy link

igaw commented Sep 12, 2025

There are 54 nvme_set_features* in the code base, not all of them need an update.

git diff HEAD~ | grep nvme_set_features | grep -v nvme_set_features_args | wc -l
76

but at least this one looks fishy

../plugins/micron/micron-nvme.c: In function ‘micron_clr_fw_activation_history’:
../plugins/micron/micron-nvme.c:3285:50: warning: unsigned conversion from ‘int’ to ‘__u8’ {aka ‘unsigned char’} changes value from ‘-2147483648’ to ‘0’ [-Woverflow]
 3285 |         err = nvme_set_features_simple(l, fid, 1 << 31, 0, 0, &result);
      |        

I suppose I should do a review of the patches in sequence. At least the last one needs some eagle eyes :)

@igaw
Copy link

igaw commented Sep 12, 2025

nvme-experiments/libnvme#28

@igaw
Copy link

igaw commented Sep 12, 2025

nvme-experiments/libnvme#29

@igaw
Copy link

igaw commented Sep 12, 2025

Sorry, while reviewing I found a few things we should fix also on the libnvme API. Let me do that first.

@MaisenbacherD
Copy link
Author

There are 54 nvme_set_features* in the code base, not all of them need an update.

git diff HEAD~ | grep nvme_set_features | grep -v nvme_set_features_args | wc -l
76

but at least this one looks fishy

../plugins/micron/micron-nvme.c: In function ‘micron_clr_fw_activation_history’:
../plugins/micron/micron-nvme.c:3285:50: warning: unsigned conversion from ‘int’ to ‘__u8’ {aka ‘unsigned char’} changes value from ‘-2147483648’ to ‘0’ [-Woverflow]
 3285 |         err = nvme_set_features_simple(l, fid, 1 << 31, 0, 0, &result);
      |        

I suppose I should do a review of the patches in sequence. At least the last one needs some eagle eyes :)

I agree this looks fishy. Nice that you found this one! I will fix this.

@MaisenbacherD
Copy link
Author

There are 54 nvme_set_features* in the code base, not all of them need an update.

git diff HEAD~ | grep nvme_set_features | grep -v nvme_set_features_args | wc -l
76

but at least this one looks fishy

../plugins/micron/micron-nvme.c: In function ‘micron_clr_fw_activation_history’:
../plugins/micron/micron-nvme.c:3285:50: warning: unsigned conversion from ‘int’ to ‘__u8’ {aka ‘unsigned char’} changes value from ‘-2147483648’ to ‘0’ [-Woverflow]
 3285 |         err = nvme_set_features_simple(l, fid, 1 << 31, 0, 0, &result);
      |        

I suppose I should do a review of the patches in sequence. At least the last one needs some eagle eyes :)

I agree this looks fishy. Nice that you found this one! I will fix this.

This one should be resolved now. I double-checked the other commits in this PR for other functions that I missed for adopting the new API.

I will continue with addressing the two issues you created out of this thread :)

Drop nvme_format_nvm_args entirely.

Signed-off-by: Dennis Maisenbacher <[email protected]>
Drop nvme_ns_mgmt_args entirely.

Signed-off-by: Dennis Maisenbacher <[email protected]>
Drop nvme_fw_download_args entirely.

Signed-off-by: Dennis Maisenbacher <[email protected]>
Drop nvme_fw_commit_args entirely.

Signed-off-by: Dennis Maisenbacher <[email protected]>
Drop nvme_security_send_args entirely.

Signed-off-by: Dennis Maisenbacher <[email protected]>
Drop nvme_set_features_args entirely.

Signed-off-by: Dennis Maisenbacher <[email protected]>
@MaisenbacherD
Copy link
Author

I also addressed issues nvme-experiments/libnvme#28 and nvme-experiments/libnvme#29 in this PR. This PR has a dependency on nvme-experiments/libnvme#31 now.

@igaw
Copy link

igaw commented Sep 19, 2025

Can you please rebase it to the current nvme-cli3 branch? Unfortunately, the branch is in a bit sad state at the moment. I'll have to reorder the patches to match the order of the libnvme changes first though.

@igaw
Copy link

igaw commented Sep 22, 2025

I've merged these patches now. Thanks!

@igaw igaw closed this Sep 22, 2025
@MaisenbacherD MaisenbacherD deleted the nvme-cli3-format-nvm branch September 23, 2025 12:27
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