nvme-cli 1.5 in Bionic does not support Micron NVME drives

[David A. Desrosiers]

[Impact]

This was discovered at a customer site and affects all of their Bionic installs that have Micron NVMe drives.

The version of nvme-cli present in Ubuntu 18.04 Bionic Beaver (1.5-1) does not include support to manage updating the firmware on Micron NVMe drives. The missing support also means that the customer cannot format their block size to 4k, as needed by Ceph.

Version 1.6-1 and later versions do include this support, and can be used by rebuilding the package from upstream source as a static binary. This is not ideal, but a workaround.

[Test Case]

Install nvme-cli from Bionic, and attempt to update firmware for any Micron NVMe drive, using a command similar to the below. It will fail, as the drive is not supported.

$ nvme micron select-download /dev/<nvme_device>n1 --fw ./Micron_9200_FW-101008S0.tar --select=ALL

With the upstream commit patched into place, we can verify the subcommands function with:

$ nvme micron

This will display the help screen and a list of supported commands.

$ nvme micron select-download

This will show the help page for firmware updating, and required arguments. Running again with the arguments from the first example will update the firmware on the drives successfully.

You can find a test package for Bionic here:

https://launchpad.net/~mruffell/+archive/ubuntu/sf237119-test

[Regression Potential]

The opportunity for regression is low. The Micron support is implemented as a plugin for the application and the changes are more or less standalone. The code paths can only be accessed via "nvme micron" subcommands.

If a regression happens, then users should refrain from running "nvme micron" commands while the package is fixed.

[Other Info]

The commit that adds support for Micron drives is:

commit 0124daa3331602365d009a9e8229454c41931c07
Author: Stephen Tubbs <email address hidden>
Date: Wed May 9 07:06:03 2018 -0700
Subject: Add support for Micron plugin

https://github.com/linux-nvme/nvme-cli/commit/0124daa3331602365d009a9e8229454c41931c07

This commit landed in version 1.6, and is present in the following distros:

$ rmadison nvme-cli -a amd64
nvme-cli | 1.5-1 | bionic/universe | amd64
nvme-cli | 1.6-1 | cosmic/universe | amd64
nvme-cli | 1.7-1 | disco/universe | amd64
nvme-cli | 1.7-1 | eoan/universe | amd64

There is a minor backport required for the commit into version 1.5, and that is in the Makefile. Some other plugins which are not currently present are in the patch, and needed to be removed from the OBJS line.

[Matthew Ruffell (mruffell)]

Test package was successfully validated against Micron NVMe devices.

Attached is the debdiff for nvme-cli for bionic.

[Matthew Ruffell (mruffell)]

Fixed tab use in debian/changelog. Whoops.

[Eric Desrochers (slashd)]

[sts-sponsor]

Since this is a "HWE/new feature", please read https://wiki.ubuntu.com/StableReleaseUpdates#Other_safe_cases

and see if your patch fits inside the policy.

Also, I would like to see more justification/testings (detailed steps, output, ...) using a micron nmve drive type, local autopkgtest (if any), software testsuite report (if any), ...

Anything that would support that the regression potential is low, and that the micro plugin is working as expected.

I would also suggest to look upstream, and see if there was any major bugfix, CVE, ... after the micron add support, that could be a red flag or simply worth be adding inside this SRU to-be.

Regards,
Eric

[Dave Chiluk (chiluk)]

@slashd ,

Reading through the patch, it's exactly as @mruffell says it is. It's sequestered into a code path that only exists for micron drives. A code path which previously did not exist. This satisfies the intro, and the first three bullet points of Other_safe_cases. This patch is upstream and AFAICT from git log and comparing of the enablement patch and the file on master of the public project micron-nvme.c has not changed in any consequential way.

IMHO. there should be no issue with pushing this in as an SRU into bionic, and does significantly improve the user experience for those with Micron drives. This simultaneously has little to no impact on other users.

[Dave Chiluk (chiluk)]

I got my act together and sponsored this bug today. Now you just need SRU approval.

@mruffell, welcome to the party.

[Matthew Ruffell (mruffell)]

Hi @slashd and @chiluk,

As @chiluk mentioned, this feature is implemented as a standalone plugin which has no influence over any of the existing source code, and instead adds code which is only reachable through a new subcommand. Even if a regression was introduced, it would be limited to users of the "nvme micron" subcommand only.

I did a diff of the proposed patch to the current upstream master branch, and the new changes are all more or less cosmetic:

https://paste.ubuntu.com/p/QPkVkNrzVf/

If you would like to review each commit individually, I have provided them below:

1d37bc9 nvme-cli: Code cleanup
507ded5 nvme-vendor: fix c99 declarations in vendor plugins
c36e848 nvme: Unify min(), max() macro as a common one
661ad0f micron-nvme: Replace direct use of ioctl
f392181 Refactor plugins in a file hierarchy

None of these are direct bug fixes, and are not necessary to carry in this SRU.

I went and performed autopkgtest on the updated package, but unfortunately there are no autopkgtests included with this package:

https://paste.ubuntu.com/p/Ky7V4Mr8fM/

The code being introduced is compatible across kernels, as we are only opening a device interface up for writing, and is compatible across the default and subsequent HWE kernels.

As for "Other safe cases", I believe that this SRU fulfils the first three cases for applicability while also improving the user experience of those with Micron NVMe devices.

For case one:
1.) "Has an obviously safe patch" - we are adding a standalone plugin which has no impact on existing code, since it implements a new subcommand, specifically for use of Micron hardware.
2.) "affects an application" - changes are limited to nvme-cli only. Kernel support is already present.

For case two:
This SRU will enable the use of Micron NVMe devices, while not affecting any existing NVMe devices from other manufacturers, since the subcommand is implemented alongside other manufactures subcommands, and does not modify any existing code.

For case three:
The changes are unintrusive, again, to being limited to plugin which implements a new subcommand. The code itself is simplistic, and has not been dramatically modified since it first landed in upstream compared to the present day. The feature landed in nvme-cli version 1.6, which appears in cosmic onwards, meaning upgrading from bionic to a newer release will not cause upgrade regressions.

The test package in the SRU template has been tested by the customer on a Micron NVMe device, although I do not have any actual output of the commands ran. The customer ran the "nvme micron select-download" command to update the device firmware, and managed to format a drive to 4K block size.

When the package hits -proposed, I can ask the customer for explicit output of commands ran if that is necessary, during the verification process.

Thanks!

[Łukasz Zemczak (sil2100)]

This looks okay. Every new HWE enablement comes with risk and there's not much we can do about it - here at least the regression potential feels to be limited. I will accept this but I'd probably feel much better if we could get the verification done on more than just one device. If we don't have other people with Micron NVMe drives to test this on, that's fine, but the more verification cases the better.

[Łukasz Zemczak (sil2100)]

Hello David, or anyone else affected,

Accepted nvme-cli into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/nvme-cli/1.5-1ubuntu1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Matthew Ruffell (mruffell)]

The following is from my own testing of the package in -proposed. Note, I do not have access to a Micron NVMe device, So all commands are more or less printing their help documentation.

https://paste.ubuntu.com/p/PsGYQpj359/

We see that Micron is listed as a plugin, and sub commands can be called and their help printed.

I have asked the customer to install the package in -proposed to a test environment, and to test against one of their Micron NVMe devices, for both updating the firmware and formatting the disk to 4K block size.

I have asked them to provide output from their terminal of their verification, and I will attach it here when they get back to me.

[Eric Desrochers (slashd)]

Quick update:

Package reached day 21 in -proposed.

We haven't been able to have someone to test the pkg using a micron nvme drive so far.
We have someone willing to test, but it may takes weeks before they get access to the hardware.

- Eric

[Matthew Ruffell (mruffell)]

The following is from the customer testing the package in -proposed, after swapping out a failed Micron NVMe drive for a new replacement Micron NVMe drive:

Begins:

I've tested the version of the package in `bionic-proposed` and it seems to be fine. Here are the relevant terminal outputs in ASCII on a system with a replacement NVME drive:

Firmware update
===============
Script started on 2020-01-07 10:52:02+0000
[root@<hostname> ~]# apt-cache policy nvme-cli | grep Installed
Installed: 1.5-1ubuntu1
[root@<hostname> ~] nvme micron select-download /dev/nvme13n1 --fw ./Micron_9200_FW-101008S0.tar --select=ALL
Update successful! Please power cycle for changes to take effect
[root@<hostname> ~]# exit

Script done on 2020-01-07 10:55:02+0000

Format
======
Script started on 2020-01-07 11:10:39+0000
[root@<hostname> ~]# apt-cache policy nvme-cli | grep Installed
Installed: 1.5-1ubuntu1
[root@<hostname> ~]# nvme list | egrep 1853207DCEB4
/dev/nvme2n1 1853207DCEB4 MTFDHAL3T8TCT 1
3.84 TB / 3.84 TB 512 B + 0 B 101008S0
[root@<hostname> ~]# nvme format -l 1 --namespace-id=1 --ses=1 /dev/nvme2n1
Success formatting namespace:1
[root@<hostname> ~]# exit

Script done on 2020-01-07 11:13:48+0000

Ends.

With confirmation from the customer that nvme-cli 1.5-1ubuntu1 works as expected on a Micron drive, and from my own testing that the submenus work, I am happy to mark this as verified.

Thanks,
Matthew

[Łukasz Zemczak (sil2100)]

The verification of the Stable Release Update for nvme-cli has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package nvme-cli - 1.5-1ubuntu1

---------------
nvme-cli (1.5-1ubuntu1) bionic; urgency=medium

  * debian/patches/Add-support-for-Micron-plugin.patch:
    - Add upstream patch to enable support for Micron NVMe devices,
      specifically enabling the feature to update firmware. (LP: #1838555)

 -- Matthew Ruffell <email address hidden> Sat, 05 Oct 2019 23:26:53 -0500