Use Request or Response Length in DLEN and DOFF for MI

[chorkin]

Added support for variable length DLEN/DOFF for requests since it was only being handled for responses.

[igaw]

Could you update the commit message format, following the existing style?

prefix: subject title

explain why this change

Signed-off-by: Joe Hacker <[email protected]>

Thanks!

[jk-ozlabs]

Thanks for the changes!

Just for record-keeping, the change here does align with the spec requirement for the DLEN field:

For commands that transmit data from the Management Controller to the Management Endpoint (i.e., the Data Transfer subfield in the opcode field of the NVMe Admin Command as defined by the NVM Express Base Specification is 01b), this field specifies the length, in bytes, of the data contained in the Request Data field in the Request Message.

There is no change in behaviour to the DOFF field, mentioned below.

A comment on the implementation inline though.

[jk-ozlabs]

You're missing the endian conversion in cases where req_data_size is set (ie, the case you care about). Given the logic here is getting more complex, I would suggesting separating this out a little - calculate the values first, then do the endian conversion when setting up the admin_req struct.

Something like:

__u32 dlen, doff;

[...]

/* dlen and doff have different interpretations depending on the data direction */
if (req_data_size) {
    dlen = req_data_size & 0xffffffff;
    doff = 0;
} else {
    dlen = *resp_data_size & 0xffffffff;
    doff = resp_data_offset & 0xffffffff;
}
admin_req->dlen = cpu_to_le32(dlen);
admin_req->doff = cpu_to_le32(doff);

We do already know that resp_data_offset will always 0 for the former case (due to the checks above), but no harm in being explicit about this.

[jk-ozlabs]

Also, can you add a testcase here? Let me know if you need a hand writing one.

[chorkin]

Also, can you add a testcase here? Let me know if you need a hand writing one.

@jk-ozlabs , I could definitely at least use some pointers/suggestions here. The test code in mi.c appears a bit dense. I'm guessing we want to test something where we send and admin command and verify the DLEN and DOFF values in the message that gets sent? What's the best way to approach this given the infrastructure that already exists?

[chorkin]

Could you update the commit message format, following the existing style?

prefix: subject title

explain why this change

Signed-off-by: Joe Hacker <[email protected]>

Thanks!

Let me know if I did that right.

[igaw]

If you could hit enter after roughly 72 chars it would be perfect :)

[igaw]

And also just squash the commits into one (and git rid of the merge commit).

[chorkin]

What's the best way to do that? Do I need to delete something on the origin or is there a way to do it through git hub? Or a full new pr and branch? I normally use ADO. Also what's the best way to run the checkpatch locally? Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Daniel Wagner ***@***.***> Sent: Tuesday, October 29, 2024 11:56:10 AM To: linux-nvme/libnvme ***@***.***> Cc: Chuck Horkin ***@***.***>; Author ***@***.***> Subject: Re: [linux-nvme/libnvme] Use Request or Response Length in DLEN and DOFF for MI (PR #897) And also just squash the commits into one (and git rid of the merge commit). — Reply to this email directly, view it on GitHub<#897 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AWCQL34JSATERA3RXOY27ITZ57K4VAVCNFSM6AAAAABQGOI4BSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBVGA4TANRWGA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[chorkin]

What's the best way to do that? Do I need to delete something on the origin or is there a way to do it through git hub? Or a full new pr and branch? I normally use ADO. Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Androidhttps://aka.ms/AAb9ysg
…
________________________________ From: Daniel Wagner @.> Sent: Tuesday, October 29, 2024 11:56:10 AM To: linux-nvme/libnvme @.> Cc: Chuck Horkin @.>; Author @.> Subject: Re: [linux-nvme/libnvme] Use Request or Response Length in DLEN and DOFF for MI (PR #897) And also just squash the commits into one (and git rid of the merge commit). — Reply to this email directly, view it on GitHub<#897 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AWCQL34JSATERA3RXOY27ITZ57K4VAVCNFSM6AAAAABQGOI4BSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBVGA4TANRWGA. You are receiving this because you authored the thread.Message ID: @.***>

[jk-ozlabs]

The test code in mi.c appears a bit dense.

Yes, we're calling the libnvme API then manually inspecting the resulting MCTP transactions, so it's a bit layer-violation-ey.

I'm guessing we want to test something where we send and admin command and verify the DLEN and DOFF values in the message that gets sent?

Yes, exactly.

@jk-ozlabs , I could definitely at least use some pointers/suggestions here.

If you like, I can put together a test case commit which you can steal for this PR, would that work for you?

[chorkin]

Sounds great. Thanks so much. Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Jeremy Kerr ***@***.***> Sent: Tuesday, October 29, 2024 5:05:15 PM To: linux-nvme/libnvme ***@***.***> Cc: Chuck Horkin ***@***.***>; Author ***@***.***> Subject: Re: [linux-nvme/libnvme] Use Request or Response Length in DLEN and DOFF for MI (PR #897) The test code in mi.c appears a bit dense. Yes, we're calling the libnvme API then manually inspecting the resulting MCTP transactions, so it's a bit layer-violation-ey. I'm guessing we want to test something where we send and admin command and verify the DLEN and DOFF values in the message that gets sent? Yes, exactly. @jk-ozlabs<https://github.com/jk-ozlabs> , I could definitely at least use some pointers/suggestions here. If you like, I can put together a test case commit which you can steal for this PR, would that work for you? — Reply to this email directly, view it on GitHub<#897 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AWCQL32B4KDQEIAPCIN7XATZ6APDXAVCNFSM6AAAAABQGOI4BSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBVGU2DAOJSG4>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[igaw]

The simplest way merge/squash existing commits, is to use git rebase. Though be careful if you don't have experience with it. It's a very sharp tool. Though git also has git reflog which gives you access to all commits. That means if your
git rebase got terrible wrong, you can lookup with git reflog the last good commit and restore it with git reset --hard ref

So in this particular case I would first update the master branch:

$ git switch master
$ git pull
$ git switch req_resp_data_admin_adjustments
$ git rebase -i master      # this will remove the merge commit
[mark the second patch to squash into the first one]

There are other possible ways to achieve it. This is just I do it... Maybe someone has a better workflow.

[chorkin]

y merge/squash existing commits, is to use git rebase. Though be careful if you don't have experience with it. It's a very sharp tool. Though git also has git reflog which gives you access to all commits. That means if your
git rebase got terrible wrong, you can lookup with git reflog the last good

The piece I was missing was this: git push origin user/chorkin/req_resp_data_admin_adjustments --force

[jk-ozlabs]

Sounds great. Thanks so much.

ok, no problem!

I have pushed a dev/dlen branch: https://github.com/jk-ozlabs/libnvme/tree/dev/dlen. This contains a one commit that adds couple of testcases for the dlen behaviour in nvme_mi_admin_xfer. The req test currently fails for me (due to the bug that you're addressing), and should pass once your change is applied (let me know if not!).

If you fetch from my repo, then:

git cherry-pick 32274119c4d078ab7e7d8a160177755b5bd3600a

you'll get the test case in your branch.

[igaw]

I've merged it now. No need to make it more complex as need. @jk-ozlabs just send your test case directly :)

Thanks!

[jk-ozlabs]

Ack, will turn that branch into a PR!

[jk-ozlabs]

done: #903