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

[Christoph Hellwig <hch@lst.de>]

On Tue, Feb 01, 2022 at 04:50:50PM -0800, Alan Adamson wrote:
> +nvme-core-y				:= core.o ioctl.o errors.o

I still don't like the errors.c/o name.  This isn't really about
errors per se, but about pretty printing protocol constants.  I think
I suggested pretty_print.c before, but maybe just following drivers/scsi/
and calling this constants.c might be best.

> +	const unsigned char *op_str  = "I/O Cmd";
> +	const unsigned char *admin_op_str  = "Admin Cmd";

No nee for these constants, just put this into the main format string.

> +	const unsigned char *err_str = NULL;
> +
> +	err_str = nvme_get_error_status_str(nr->status);
> +
> +	if (ns) {
> +		op_str = nvme_get_opcode_str(nr->cmd->common.opcode);

And not real need for err_str and op_str either, just put the calls
into the argument list of the pr_err_ratelimited calls.

Also please mark nvme_log_err static and move it so that there is no
need for a forward declaration.

> +const unsigned char *nvme_get_error_status_str(u16 status)
> +{
> +	const unsigned char *err_str;
> +
> +	if ((status & 0x7ff) < ARRAY_SIZE(nvme_statuses) &&
> +	    nvme_statuses[status & 0x7ff] != NULL)
> +		err_str = nvme_statuses[status & 0x7ff];
> +	else
> +		err_str = "Unknown";

This can be simplified to:

	status &= 0x7ff;
	if (status < ARRAY_SIZE(nvme_statuses) nvme_statuses[status])
		return nvme_statuses[status];
	return "Unknown";

Similar for the other helpers.

> +EXPORT_SYMBOL_GPL(nvme_get_error_status_str);

No need to export any of the function added in this patch, as they are
all used only inside of nvme-core.ko