[PATCH 3/3] nvme: add KConfig options for debug features

[Sagi Grimberg]

>>>>> Add KConfig menu option to enable and disable gencounter debug
>>>>> feature that uses config NVME_DEBUG_USE_CID_GENCTR.
>>>>>
>>>>> Signed-off-by: Chaitanya Kulkarni <kch at nvidia.com>
>>>>> ---
>>>>>     drivers/nvme/host/Kconfig | 10 ++++++++++
>>>>>     1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
>>>>> index dc0450ca23a3..dfa2609b7006 100644
>>>>> --- a/drivers/nvme/host/Kconfig
>>>>> +++ b/drivers/nvme/host/Kconfig
>>>>> @@ -1,4 +1,14 @@
>>>>>     # SPDX-License-Identifier: GPL-2.0-only
>>>>> +menu "Debug (Enable driver debug features)"
>>>>> +config NVME_DEBUG_USE_CID_GENCTR
>>>>> +     bool "Enable command ID gen counter for spurious request
>>>>> completion"
>>>>> +     depends on NVME_CORE
>>>>> +     help
>>>>> +       The NVM Express driver will use generation counter
>>>>> +       when calculating the command id. This is needed to debug the
>>>>> +       spurious request completions coming from a buggy controller.
>>>>
>>>> This is not just to debug - it is also to protect against such a
>>>> controller. What is the purpose of this config option anyways?
>>>> The main distributions will (as they should) enable it anyways...
>>>
>>> I can rewrite the text and rename it to "driver features".
>>
>> It is not a feature.
> 
> that is what I thought, hence I kept it under debug.

It is not debug, I mean it can certainly be used for debug, but
it is protecting against kernel-crash (or worse) with such controllers.
The fact that a buggy controller can crash the kernel (or worse) has
been raised on this list by users multiple times in the past.

>>> We are protecting against such a controller which is not stable
>>> (buggy), i.e. it is doing things which it shouldn't be
>>> doing at the first place.
>>
>> Yes, this is exactly what we are doing.
>>
>>> Consider a case if controller is not
>>> buggy then it adds instructions in the fast path which are not
>>> needed at all.
>>
>> We keep repeating this, if the controller assumes _anything_ on
>> the command identifier then it _is_ buggy, period.
>>
> 
> Completely agree on this.
> 
> What I mean buggy in this context is not related to assuming anything
> with command identifier population, it meant controller that is
> causing spurious completions -> buggy. The controller that is not
> able to process non-seq command ids and assumes anything is out of
> picture for this patch-series (for that nvme-cli skip approach is
> fine), see below.
> 
>> Again, the fact that the controller is buggy is perfectly fine
>> really, that's why we have quirks.
>>
> 
> A controller which is suffering from spurious completions needs
> gencounter to debug the issue, using quirk and masking gencounter
> should be avoided in this case.

Well of course, the quirk is for controllers that don't suffer
from this but happen to break because the host is doing something
unexpected with the command id (even though it is allowed to).

> 
>> What happened to your nvme-cli argument? that would make it
>> specific to that controller.
>>
> 
> That is completely different issue, the nvme-cli argument tries
> to fix the issue with a controller that fails to process
> non-sequntial command-id such as Apple ctrl and needs quirk.
> 
> This path-series has nothing to do with that issue.
> This patch-series is only focused on keeping the fast path lean
> and allowing user(s) to configure gencounter in the environment
> where :-
> 
> 1. Controller is stable.
> 2. Able to process the non-sequential command ids
>      (hence no quirk needed) and does not assume anything about the
>      command id population unlike Apple controller.
> 3. Does not suffer from spurious completion problem.
> 
> For this controller, user can disable gencounter and keep the fast
> path lean.

I understand, although I'd argue that a vendor may tell a customer
to disable that globally... That is not necessarily something we want
to allow.

> 
>> The host side cost is very cheap as shown before.
>>
>>> A controller(s) that is used in the production environment goes
>>> through qualification process from vendors and from the consumers
>>> to make sure they are stable, something like spurious completions
>>> detection is a basic part of the qualification,
>>
>> I don't think that assuming that a production controller does not
>> have any hidden bugs is a great practice, and controllers evolve during
>> their production lifetime.
>>
> 
> That is why we should keep it configurable and let user decide.

OK, so this allows a user that is absolutely set on shaving few
nano-seconds to sacrifice a level of protection against buggy
controllers. Is this a real use-case?

For that, I guess the config option is fine, but this at the very least
needs to default to y.