-
Notifications
You must be signed in to change notification settings - Fork 655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nvme: fw-download offset only describes FW offset, not file offset #2060
Conversation
@tkenbo, can you test this against your use-case? |
Hi @jk-ozlabs Thanks for the update. I tried to install it but having json-types.h missing error on compile. I have Rocky 8.7 and json-c is v1.3 installed. will try figuring out.
|
I haven't used Rocky here, but looking at the
so there is indeed no
it doesn't look like the packaged |
Looks good. Should I wait with the merge until we have a positive test result? |
After struggling to build due to json-c and libnvme location, finally I was able to get it working and tested. However, the current build has another issue affecting our test and couldn't run. So I ported this change to 2.4 and tested again. Now it passed the original failure section but failed in another section for the same error message and I'm looking at the error. the test passes with 2.2.1, so there could be some more things in the newer build. Will keep posted. |
I think you don't need libjson-c for this tests, so you could just disable the build for json-c.
|
cool. Thanks @igaw! The error I was getting was random order of segment download.
I looked at the change and noticed one issue at line 5072. in here I believe fw_size should be the actual remaining file size. |
In e7ca3bc, we (unintentionally) changed the semantics of the fw-download's --offset argument to also apply an offset to the source file. According to linux-nvme#2013, the old behaviour is useful when the source firmware image was provided in separate chunks. This change restored to the old hehaviour, where --offset only applies to the destination firmware buffer on the device. This means that the start of the source file will end up at the destination offset. Fixes: linux-nvme#2013 Signed-off-by: Jeremy Kerr <[email protected]>
4ffaf8f
to
da91a5a
Compare
Ah! yes, this should clamp to the min of the remaining size. New patch pushed. |
@tkenbo purely out of curiosity: why are you transferring this as separate files / chunks? (I'm glad we're covering this use-case, just want to understand more about your requirements here) |
@jk-ozlabs we are testing NVMe devices for the spec compliance in various system configurations in the market with various OS platforms as a part of our system integration testing. We test the device for a unique customer environment and also in the off-the-shelf environment. I believe the original change you made was to improve a user experience, but we ran into this behavior change, which made little harder to test along the spec definition using nvmecli. so I though I would ask. Hope this answers. |
ah, that makes sense - this restored behaviour allows you to transfer the offsets out-of-order. (We can do the same thing with an offset and fixed length argument, which means you don't need to split the file up into chunks - you would just use the same file for all transfers. However, I'm fine with the behaviour restored by this change if you prefer)
Excellent, thanks for that! in which case: @igaw , I'm ok to merge if you are. |
In e7ca3bc, we (unintentionally) changed the semantics of the fw-download's --offset argument to also apply an offset to the source file.
According to #2013, the old behaviour is useful when the source firmware image was provided in separate chunks.
This change restored to the old hehaviour, where --offset only applies to the destination firmware buffer on the device. This means that the start of the source file will end up at the destination offset.
Fixes: #2013