Re: [RFC PATCH] nvmet: passthru: use Invalid Field, not Invalid Opcode, for unhandled Get/Set Features FIDs

Hey Marko,

> Hello all,
>
> My first kernel patch, be gentle ;)
>
> nvmet_passthru_get_set_features() rejects any FID that reaches its
> default case - whether intentionally blocked (IRQ_COALESCE,
> IRQ_CONFIG, HOST_MEM_BUF, SW_PROGRESS, RESV_MASK, RESV_PERSIST) or
> simply unlisted (e.g. FID 0, unassigned by the spec) - with
> NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR. That's the wrong class: the
> Get/Set Features opcode is always valid here, it's the FID that's
> rejected, so NVME_SC_INVALID_FIELD is correct.
>
> This surfaced while debugging why nvme-cli's "dump all known
> features" mode (get-feature -f 0) completes against a native
> controller but aborts over nvmet passthru: natively, FID 0 gets a
> plain Invalid Field rejection (no DNR) that nvme-cli tolerates and
> continues past; nvmet's Invalid Opcode + DNR is not tolerated the
> same way.
>
> Verified with this fix applied: get-feature -f 0 now enumerates every
> FID over nvmet passthru (NVMe/TCP) without aborting early. The DNR
> flag was not the obstacle - only the status class was.
>
> This is a status-code fix only, not a policy change: Get/Set Features
> FIDs are still intercepted at more than one layer before reaching
> this function (nvmet_parse_admin_cmd()/nvmet_parse_passthru_admin_cmd()
> already service ASYNC_EVENT, KATO, NUM_QUEUES, HOST_ID and FDP with
> nvmet's own emulation), so native and passthru queries still won't
> return identical results for every FID - only this early-abort
> failure mode is fixed. I'd rather leave any broader forwarding-policy
> change to a separate discussion.
>
> Unrelated aside noticed while reading this code: the
> NVME_FEAT_ASYNC_EVENT case (and propably some other) inside this
> function is dead code, since the callers above already intercept
> that FID before it can be reached.
>
> Signed-off-by: Marko Ahvenainen <[email protected]>
> ---
>   drivers/nvme/target/passthru.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
> index e27f84e3cf2b..df1396081f87 100644
> --- a/drivers/nvme/target/passthru.c
> +++ b/drivers/nvme/target/passthru.c
> @@ -483,7 +483,7 @@ static u16 nvmet_passthru_get_set_features(struct nvmet_req *req)
>          case NVME_FEAT_RESV_PERSIST:
>                  /* No reservations, see nvmet_parse_passthru_io_cmd() */
>          default:
> -               return NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR;
> +               return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;

I think you want to set the error_loc such that it can be viewed from 
the error log page.

>          }
>   }
>
> --
> 2.43.0

You should make it a proper (non-rfc) patch and send it.
The below should be removed from the actual submission.

>
> This e-mail and any attachments may contain privileged or confidential information and is for the sole use of the intended recipient(s). Any unauthorized use or disclosure of this communication is prohibited. If you believe that you have received this e-mail in error, please notify the sender immediately and destroy all copies.