Workaround proposition for pre-2.0 controllers

Dmytro Bagrii <[email protected]>
Newsgroups org.infradead.lists.linux-nvme
Message-ID <[email protected]>
Hi All,

There is a known issue with pre-2.0 controllers that causes non-volatile 
ErrCount to increment at every boot. An example of such controller is 
144d:a808 SM981/PM981/PM983 NVMe SSD Controller found in Samsung SSD 970 
PRO.

This is definitely a flaw of firmware implementations that treat Invalid 
Field in Command as an error worth loging and incrementing the counter, 
rather than simply return an error code. However, it is unlikely that 
firmwares for existing devices will ever be updated, especially by 
manufacturers that no longer active.

The issue confuses userspace disk health monitoring tools and make users 
prone to overlooking real errors.

My proposition is to allow to workaround the issue with a quirk flag. 
Since it is known that specific controller does not support 
nvme_admin_identify opcode with cns=5 and cns=6, issuing these commands 
can be omitted. Using quirk module option allows users to decide whether 
to workaround the issue or not.

Whether to add a quirk to nvme_id_table[] for known affected controllers 
is out of scope of this proposition.

Option 1: re-use NVME_QUIRK_IDENTIFY_CNS by extending its effect for 
versions >= 1.2.0, not only for 1.1.0 <= version < 1.2.0.
Pros: Minor code changes.
Cons:
    It's not yet clear to me whether the driver can issue 
nvme_admin_identify with cns > 6 if controller already rejected cns=5 
and cns=6.
    The impact for devices that already have NVME_QUIRK_IDENTIFY_CNS set 
but claim support for version > 1.1.0 is unclear, if any such devices exist.

By the way, the comment for NVME_QUIRK_IDENTIFY_CNS in 
drivers/nvme/host/nvme.h is misleading: it states that "The controller 
doesn't handle Identify value others than 0 or 1 correctly" but 
nvme_id_cns_ok() actually allows cns values up to 3.

Option 2: introduce another quirk flag.
Pros: Independent control, with no devices potentially affected by default.
Cons: More code changes.

I would also prefer to leave ioctl() behavior as is, allowing userspace 
to issue nvme_admin_identify with any cns value.

I could try to prepare a patch if the idea isn't rejected outright.

-- 
With best regards,
Dmytro Bagrii
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.