nvme: fw-download offset only describes FW offset, not file offset

[jk-ozlabs]

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

[jk-ozlabs]

@tkenbo, can you test this against your use-case?

[tkenbo]

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.

INFO: autodetecting backend as ninja
INFO: calculating backend command to run: /usr/local/bin/ninja -C /root/Downloads/nvme-cli-pr-fw-offset/.build
ninja: Entering directory `/root/Downloads/nvme-cli-pr-fw-offset/.build'
[3/160] Compiling C object subprojects/libnvme/src/libnvme.so.1.5.0.p/nvme_json.c.o
FAILED: subprojects/libnvme/src/libnvme.so.1.5.0.p/nvme_json.c.o
cc -Isubprojects/libnvme/src/libnvme.so.1.5.0.p -Isubprojects/libnvme/src -I../subprojects/libnvme/src -Isubprojects/libnvme -I../subprojects/libnvme -Isubprojects/libnvme/ccan -I../subprojects/libnvme/ccan -Isubprojects/libnvme/internal -I../subprojects/libnvme/internal -I/usr/include/json-c -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -std=gnu99 -O0 -g -fomit-frame-pointer -D_GNU_SOURCE -include subprojects/libnvme/internal/config.h -DCCAN_LIST_DEBUG=1 -DCCAN_STR_DEBUG=1 -fPIC -MD -MQ subprojects/libnvme/src/libnvme.so.1.5.0.p/nvme_json.c.o -MF subprojects/libnvme/src/libnvme.so.1.5.0.p/nvme_json.c.o.d -o subprojects/libnvme/src/libnvme.so.1.5.0.p/nvme_json.c.o -c ../subprojects/libnvme/src/nvme/json.c
In file included from /usr/include/json-c/json.h:27,
                 from ../subprojects/libnvme/src/nvme/json.c:15:
/usr/include/json-c/json_object.h:27:10: fatal error: json_types.h: No such file or directory
 #include "json_types.h"
          ^~~~~~~~~~~~~~
compilation terminated.
[56/160] Compiling C object subprojects/libnvme/src/libnvme.so.1.5.0.p/nvme_tree.c.o
ninja: build stopped: subcommand failed.

[jk-ozlabs]

I haven't used Rocky here, but looking at the json-c-devel RPM:

$ rpm2cpio json-c-devel-0.13.1-3.el8.x86_64.rpm | cpio -t  | grep include
221 blocks
./usr/include/json-c
./usr/include/json-c/arraylist.h
./usr/include/json-c/bits.h
./usr/include/json-c/debug.h
./usr/include/json-c/json.h
./usr/include/json-c/json_c_version.h
./usr/include/json-c/json_config.h
./usr/include/json-c/json_inttypes.h
./usr/include/json-c/json_object.h
./usr/include/json-c/json_object_iterator.h
./usr/include/json-c/json_pointer.h
./usr/include/json-c/json_tokener.h
./usr/include/json-c/json_util.h
./usr/include/json-c/json_visit.h
./usr/include/json-c/linkhash.h
./usr/include/json-c/printbuf.h

so there is indeed no json_types.h file there. However:

$ rpm2cpio json-c-devel-0.13.1-3.el8.x86_64.rpm | cpio -i --to-stdout ./usr/include/json-c/json_object.h | grep include
#include <stddef.h>
#include "json_inttypes.h"
#include "printbuf.h"

it doesn't look like the packaged json_object.h references a json_types.h file anyway? Are you using the vanilla packages there? or is there a chance your json_object.h has been overwritten?

[igaw]

Looks good. Should I wait with the merge until we have a positive test result?

[tkenbo]

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.

[igaw]

I think you don't need libjson-c for this tests, so you could just disable the build for json-c.

meson setup -C .build -Djson-c=disabled -Dlibnvme:json-c=disabled

[tkenbo]

cool. Thanks @igaw!

The error I was getting was random order of segment download.
I downloaded segment 2 -> 5 -> 1 -> 4 -> 0 -> 6 -> 3, then commit returned the error.

nvme fw-download /dev/nvme3 -f file_2.bin -x 262144 -o 146432
nvme fw-download /dev/nvme3 -f file_5.bin -x 262144 -o 479232
nvme fw-download /dev/nvme3 -f file_1.bin -x 262144 -o 41984
nvme fw-download /dev/nvme3 -f file_4.bin -x 262144 -o 357376
nvme fw-download /dev/nvme3 -f file_0.bin -x 262144 -o 0
nvme fw-download /dev/nvme3 -f file_6.bin -x 262144 -o 605184
nvme fw-download /dev/nvme3 -f file_3.bin -x 262144 -o 260096
nvme fw-commit /dev/nvme3 -s 1 -a 1
CRITICAL NvmeCli Error : NVMe status: Invalid Firmware Image: The firmware image specified for activation is invalid and not loaded by the controller(0x2107)

I looked at the change and noticed one issue at line 5072.
cfg.xfer = min(cfg.xfer, fw_size);

in here I believe fw_size should be the actual remaining file size.
For example,
fw_size = 3500
cfg.xfer =1000
then, cfg.xfer for the last loop should be 500 instead of 1000.

[jk-ozlabs]

Ah! yes, this should clamp to the min of the remaining size. New patch pushed.

[jk-ozlabs]

@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)

[tkenbo]

@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.
By the way, I tested the new change and it passed the test! Thanks again!

[jk-ozlabs]

which made little harder to test along the spec definition using nvmecli

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)

By the way, I tested the new change and it passed the test! Thanks again!

Excellent, thanks for that!

in which case: @igaw , I'm ok to merge if you are.