linux-nvme / nvme-cli

NVMe management command line interface.
https://nvmexpress.org
GNU General Public License v2.0
1.41k stars 637 forks source link

nvme-rpmb: send RPMB_REQ_READ_RESULT for authentication key programming #2383

Closed ikegami-t closed 6 days ago

ikegami-t commented 2 weeks ago

This follows the NVMe revision 2.0a authentication key data flow.

ikegami-t commented 2 weeks ago

@hanumanthuh Could you please revier the patch for the issue #2280? Note:

  1. I am not sure if still needed to read the result back for the authentication key programming request as same rpmb_read_request() used before.
  2. I will do refactor the nvme-rpmb.c functions since not followed the kernel coding style.

@yang8621 Could you please test the changes since I do not have a drive support supported the RPMB feature . ?

yang8621 commented 2 weeks ago

Hi @ikegami-t My RPMB was programmed. I'm not sure a virgin one can be found, but I will try to seek one. Before that, it would be great if the review could be done because key programming is a one-time operation (it can be tested only once).

igaw commented 2 weeks ago

Okay, I'll see if I can wrap my head around the change for the review

igaw commented 2 weeks ago

The first patch looks good.

The second one is not really necessary to fix the problem and it doesn't really make sense just to update one function and leave the rest in the 'old' style. The third one is big and thus it easy to miss something which shouldn't be changed. A quick binary compare between old and new version (third patch) shows that the resulting binary was not identically. So I am not really sure if we should touch this code without being able to test it. In short, it's quite a bit of a code churn without any real benefit. I would suggest we only do the minimal change as long we can't make sure the the resulting binary is identically and we know nothing has been broken by the change.

ikegami-t commented 2 weeks ago

Thanks for your reviewing. Understood so deleted the second and third patches and rebased.

igaw commented 6 days ago

@yang8621 did you find a test drive? I think it's fairly safe to merge it. If it really breaks something, we can fix it up when we get a bug report.

yang8621 commented 6 days ago

No. But I think it can be merged. Thanks

igaw commented 6 days ago

Thanks!