[PATCH] nvme: log additional message for controller status

[PATCH] nvme: log additional message for controller status

[Rupesh Girase]

	Logging the controller fatal and ready status would help to
	identfy if issue lies within kernel nvme subsytem or
	controller is unhealthy.

Signed-off-by: Rupesh Girase <rgirase@redhat.com>
---
 drivers/nvme/host/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ada59df..2dfca9d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2079,8 +2079,9 @@ static int nvme_wait_ready(struct nvme_ctrl *ctrl, u64 cap, bool enabled)
 			return -EINTR;
 		if (time_after(jiffies, timeout)) {
 			dev_err(ctrl->device,
-				"Device not ready; aborting %s\n", enabled ?
-						"initialisation" : "reset");
+				"Device not ready; aborting %s, RDY=0x%x, CFS=0x%x\n",
+				enabled ? "initialisation" : "reset",
+				csts & NVME_CSTS_RDY, csts & NVME_CSTS_CFS);
 			return -ENODEV;
 		}
 	}
-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: log additional message for controller status

[Christoph Hellwig]

On Mon, Feb 24, 2020 at 10:02:04PM +0530, Rupesh Girase wrote:
> 	Logging the controller fatal and ready status would help to
> 	identfy if issue lies within kernel nvme subsytem or
> 	controller is unhealthy.
> 
> Signed-off-by: Rupesh Girase <rgirase@redhat.com>

There should be no tabs before the commit message.

Otherwise this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: log additional message for controller status

[Chaitanya Kulkarni]

Looks good, with tabs in commit log fixed at the time of applying
patch.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 2/24/20 8:34 AM, Rupesh Girase wrote:
> 	Logging the controller fatal and ready status would help to
> 	identfy if issue lies within kernel nvme subsytem or
> 	controller is unhealthy.
>
> Signed-off-by: Rupesh Girase <rgirase@redhat.com>



_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: log additional message for controller status

[Keith Busch]

On Mon, Feb 24, 2020 at 10:02:04PM +0530, Rupesh Girase wrote:
>  			dev_err(ctrl->device,
> -				"Device not ready; aborting %s\n", enabled ?
> -						"initialisation" : "reset");
> +				"Device not ready; aborting %s, RDY=0x%x, CFS=0x%x\n",
> +				enabled ? "initialisation" : "reset",
> +				csts & NVME_CSTS_RDY, csts & NVME_CSTS_CFS);

The RDY bit is a bit redundant. If "initialisation", we know it's not 1,
and if "reset", we know it's not 0. We also know it's not all 1's if we
reach here.

CFS may be useful, but the print will look a strange. If CFS is set,
you'll see "CFS=0x2", but the CFS field can only be either 0 or 1. I say
just print out the entire csts field in case SHST or NSSRO may provide
insights to the vendor.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: log additional message for controller status

[Sagi Grimberg]

> On Mon, Feb 24, 2020 at 10:02:04PM +0530, Rupesh Girase wrote:
>>   			dev_err(ctrl->device,
>> -				"Device not ready; aborting %s\n", enabled ?
>> -						"initialisation" : "reset");
>> +				"Device not ready; aborting %s, RDY=0x%x, CFS=0x%x\n",
>> +				enabled ? "initialisation" : "reset",
>> +				csts & NVME_CSTS_RDY, csts & NVME_CSTS_CFS);
> 
> The RDY bit is a bit redundant. If "initialisation", we know it's not 1,
> and if "reset", we know it's not 0. We also know it's not all 1's if we
> reach here.
> 
> CFS may be useful, but the print will look a strange. If CFS is set,
> you'll see "CFS=0x2", but the CFS field can only be either 0 or 1. I say
> just print out the entire csts field in case SHST or NSSRO may provide
> insights to the vendor.

Suggestions sound fine, looks reasonable either way.

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme