Re: [PATCH V5 1/1] nvme: Add verbose error logging

[Christoph Hellwig <hch@lst.de>]

On Wed, Jan 26, 2022 at 10:33:13AM -0800, Alan Adamson wrote:
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -354,6 +354,9 @@ void nvme_complete_rq(struct request *req)
>  
>  	switch (nvme_decide_disposition(req)) {
>  	case COMPLETE:
> +		if (unlikely(nvme_req(req)->status != NVME_SC_SUCCESS))
> +			nvme_error_log(req);

nvme_log_error would seem a more natural name.

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * NVM Express device driver verbose errors
> + * Copyright (c) 2021, Oracle and/or its affiliates
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */

No more new license boilerplates, please.  That's what we have the
SPDX tag for.


> +	[nvme_cmd_dsm] = "Deallocate (DSM)",

This should be "Dataset Management" as in the spec.

> +	if (ns) {
> +#ifdef CONFIG_NVME_VERBOSE_ERRORS
> +		if (nr->cmd->common.opcode < ARRAY_SIZE(nvme_ops) &&
> +		    nvme_ops[nr->cmd->common.opcode] != NULL)
> +			op_str = nvme_ops[nr->cmd->common.opcode];
> +		else
> +			op_str = "Unknown";
> +#endif

> +	} else {
> +#ifdef CONFIG_NVME_VERBOSE_ERRORS
> +		if (nr->cmd->common.opcode < ARRAY_SIZE(nvme_admin_ops) &&
> +		    nvme_admin_ops[nr->cmd->common.opcode] != NULL)
> +			admin_op_str = nvme_admin_ops[nr->cmd->common.opcode];
> +		else
> +			admin_op_str = "Unknown";
> +#endif

Can't we factor the ifdefs bits of code into little helpers that are
stubbed out if CONFIG_NVME_VERBOSE_ERRORS is not set?  That way the main
logging helper could stay in core.c, and we could have a pretty_print.c
with just the constants and these helpers.

> +extern void nvme_error_log(struct request *req);

No need for the extern.