Skip to content

Conversation

@Gopinath511
Copy link

…and api

Enabled Get Feature command (FID=C3h) api with sel, namespace-id, no-uuid command line arguments. namespace-id added to the test the command with active, inactive and invalid nsid values.

Reviewed-by: Karthik Balan [email protected]
Reviewed-by: Arunpandian J [email protected]

@Gopinath511
Copy link
Author

Gopinath511 commented May 2, 2025

Hi @ikegami-t , Please review the PR and let me know any modification required.
Thanks!

@ikegami-t
Copy link
Contributor

ikegami-t commented May 2, 2025

Let me confirm again as confirmed for the PR: #2768 before.
As same with the OCP feature C5h the command feature described as below but still is this command really required to add?

A Get Features command for Feature Identifier C3h should not be issued because no useful information is
returned.

I just thought only for the testing purpose we can use the existing get-feature command. How do you think about this?

(Added)
My concern is the command to add seems not for generic purpose but only for your testing so still I can not understand the reason to add.

(Added)
The PR changes themselves seem basically okay but still some minor comments. But before the comments let me confirm above questions to make sure.

@igaw
Copy link
Collaborator

igaw commented May 7, 2025

Asides @ikegami-t question, this needs to be rebased.

@Gopinath511 Gopinath511 force-pushed the plugin/ocp_get_feature_fid_c3h branch from 0e3de75 to 4b01426 Compare May 8, 2025 13:05
@Gopinath511
Copy link
Author

Asides @ikegami-t question, this needs to be rebased.

@igaw - I have resolved the conflict issue.

@Gopinath511
Copy link
Author

Let me confirm again as confirmed for the PR: #2768 before. As same with the OCP feature C5h the command feature described as below but still is this command really required to add?

A Get Features command for Feature Identifier C3h should not be issued because no useful information is
returned.

I just thought only for the testing purpose we can use the existing get-feature command. How do you think about this?

(Added) My concern is the command to add seems not for generic purpose but only for your testing so still I can not understand the reason to add.

(Added) The PR changes themselves seem basically okay but still some minor comments. But before the comments let me confirm above questions to make sure.

@ikegami-t - We have added the get feature FID=C3h command api to test the command with various NSID value and also ocp-nvme.c library should have set feature and get feature command api for all the FIDs

@ikegami-t
Copy link
Contributor

ikegami-t commented May 8, 2025

Okay (but I do not think the command added by this PR not used actually (basically)).
By the as mentioned by the comment: #2788 (review) the commit message should be fixed for this PR.

  1. For the commit message seems needed to be followed the following guide line.
    https://docs.kernel.org/process/submitting-patches.html#subject-line

Enabled Get Feature command (FID=C3h) API with sel, namespace-id,
no-uuid command line arguments. namespace-id added to the test the
command with active, inactive and invalid nsid values.

Reviewed-by: Karthik Balan <[email protected]>
Reviewed-by: Arunpandian J <[email protected]>
Signed-off-by: Gopinath P <[email protected]>
[wagi: fixed build issues, reformated commit message]
Signed-off-by: Daniel Wagner <[email protected]>
@igaw igaw force-pushed the plugin/ocp_get_feature_fid_c3h branch from 4b01426 to 55bce50 Compare May 9, 2025 07:40
@igaw
Copy link
Collaborator

igaw commented May 9, 2025

Didn't even build, fixed the obvious problem and also updated the commit message to follow the basic contributions rules. @Gopinath511 please take a look and try to follow them for the next time. Thanks!

@igaw igaw merged commit f70579e into linux-nvme:master May 9, 2025
17 checks passed
@Gopinath511
Copy link
Author

Didn't even build, fixed the obvious problem and also updated the commit message to follow the basic contributions rules. @Gopinath511 please take a look and try to follow them for the next time. Thanks!

Sure. Thank you

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