NVMe: Log Sense Temperature doesn't handle negative values

PATCH New Jeffrey Lien 3 participants 8 messages Oct 8, 2015 – Oct 19, 2015 NVMe

lore

Share & tools

lore

Jeffrey Lien

author

2015-10-08 18:35 UTC #0 lore raw

PATCH 1/1 NVMe: Log Sense Temperature doesn't handle negative values

Attached the patch.

Jeff Lien

Jeff.Lien at hgst.com
507 322-2416/23-2416
3605 Hwy 52 N.
Rochester, MN 55901

HGST E-mail Confidentiality Notice & Disclaimer:
This e-mail and any files transmitted with it may contain confidential or legally privileged information of HGST and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system. -------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-NVMe-Log-Sense-Temperature-doesn-t-handle-negative-v.patch
Type: application/octet-stream
Size: 2898 bytes
Desc: 0001-NVMe-Log-Sense-Temperature-doesn-t-handle-negative-v.patch
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20151008/f2d03761/attachment.obj>

Keith Busch

2015-10-09 19:52 UTC #1 lore raw

PATCH 1/1 NVMe: Log Sense Temperature doesn't handle negative values

Quoted 21 lines · Jeffrey Lien

From 4f8b0146b5d310b944cb99b5a68deed08ed89b6b Mon Sep 17 00:00:00 2001
From: Jeff Lien <jlien at ddtest-jeff.hgst.com>
Date: Wed, 7 Oct 2015 14:25:07 -0500
Subject: [PATCH] NVMe: Log Sense Temperature doesn't handle negative values
properly.
To meet the SCSI Command Spec (T10 SPC-4 r37), the log sense temperature cannot
be negative. From the spec:
The TEMPERATURE field indicates the temperature of the SCSI target device in degrees Celsius at the time the LOG SENSE command is performed. Temperatures equal to or less than zero degrees Celsius shall cause the TEMPERATURE field to be set to zero. If the device server is unable to detect a valid temperature because of a sensor failure or other condition, then the TEMPERATURE field shall be set to FFh. The temperature should be reported with an accuracy of plus or minus three Celsius degrees while the SCSI target device is operating at a steady state within its environmental limits.
With the current code, a value of -2 C will be shown as 254 C since it was not written to handle negative vales. This same issue also exists in nvme_trans_log_info_exceptions.

[snip]

Quoted 9 lines

(quoted diff) +1 -1

@@ -1097,7 +1101,7 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	dma_addr_t dma_addr;
 	void *mem;
 	u32 feature_resp;
-	u8 temp_c_cur, temp_c_thresh;
+	s8 temp_c_cur, temp_c_thresh;
 	u16 temp_k;
 	log_response = kzalloc(LOG_TEMP_PAGE_LENGTH, GFP_KERNEL);

Review static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr, · line 1139

Quoted 7 lines

(quoted diff) +4 -0

@@ -1129,6 +1133,10 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 		temp_k = (smart_log->temperature[1] << 8) +
 				(smart_log->temperature[0]);
 		temp_c_cur = temp_k - KELVIN_TEMP_FACTOR;
+		/* if temp_c_cur is negative,          */
+		/* set to 0 to meet Scsi Command Spec  */
+		if (temp_c_cur < 0)
+			temp_c_cur = 0;

Interesting, < 0C is colder than I might have expected. What if they can get hotter than expected, like 128C? I don't think we want to report that as 0.

Jon Derrick

2015-10-09 20:15 UTC #2 lore raw

PATCH 1/1 NVMe: Log Sense Temperature doesn't handle negative values

Quoted 30 lines

Quoted 15 lines
To meet the SCSI Command Spec (T10 SPC-4 r37), the log sense temperature cannot
be negative. From the spec:
The TEMPERATURE field indicates the temperature of the SCSI target device in degrees Celsius at the time the LOG SENSE command is performed. Temperatures equal to or less than zero degrees Celsius shall cause the TEMPERATURE field to be set to zero. If the device server is unable to detect a valid temperature because of a sensor failure or other condition, then the TEMPERATURE field shall be set to FFh. The temperature should be reported with an accuracy of plus or minus three Celsius degrees while the SCSI target device is operating at a steady state within its environmental limits.
With the current code, a value of -2 C will be shown as 254 C since it was not written to handle negative vales. This same issue also exists in nvme_trans_log_info_exceptions.
[snip]
Quoted 12 lines
(quoted diff) +1 -1
@@ -1097,7 +1101,7 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	dma_addr_t dma_addr;
 	void *mem;
 	u32 feature_resp;
-	u8 temp_c_cur, temp_c_thresh;
+	s8 temp_c_cur, temp_c_thresh;
 	u16 temp_k;
 	log_response = kzalloc(LOG_TEMP_PAGE_LENGTH, GFP_KERNEL);
@@ -1129,6 +1133,10 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 		temp_k = (smart_log->temperature[1] << 8) +
 				(smart_log->temperature[0]);
 		temp_c_cur = temp_k - KELVIN_TEMP_FACTOR;

Quoted 9 lines

Review static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr, · line 1139
Quoted 4 lines
(quoted diff) +4 -0
+		/* if temp_c_cur is negative,          */
+		/* set to 0 to meet Scsi Command Spec  */
+		if (temp_c_cur < 0)
+			temp_c_cur = 0;
Interesting, < 0C is colder than I might have expected. What if they can get hotter than expected, like 128C? I don't think we want to report that as 0.

It's unfortunate the spec is written that way. Aerospace silicon is often rated for -55/-40C to +125C junction temp +/- a few %. Thats a big negative range missing if it has to be 0. I would imagine over 128C is not meaningful so we would want to report that as 128C

Jon Derrick

2015-10-09 20:41 UTC #3 lore raw

PATCH 1/1 NVMe: Log Sense Temperature doesn't handle negative values

Quoted 40 lines · Jon Derrick

Quoted 30 lines
Quoted 15 lines
To meet the SCSI Command Spec (T10 SPC-4 r37), the log sense temperature cannot
be negative. From the spec:
The TEMPERATURE field indicates the temperature of the SCSI target device in degrees Celsius at the time the LOG SENSE command is performed. Temperatures equal to or less than zero degrees Celsius shall cause the TEMPERATURE field to be set to zero. If the device server is unable to detect a valid temperature because of a sensor failure or other condition, then the TEMPERATURE field shall be set to FFh. The temperature should be reported with an accuracy of plus or minus three Celsius degrees while the SCSI target device is operating at a steady state within its environmental limits.
With the current code, a value of -2 C will be shown as 254 C since it was not written to handle negative vales. This same issue also exists in nvme_trans_log_info_exceptions.
[snip]
Quoted 12 lines
(quoted diff) +1 -1
@@ -1097,7 +1101,7 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	dma_addr_t dma_addr;
 	void *mem;
 	u32 feature_resp;
-	u8 temp_c_cur, temp_c_thresh;
+	s8 temp_c_cur, temp_c_thresh;
 	u16 temp_k;
 	log_response = kzalloc(LOG_TEMP_PAGE_LENGTH, GFP_KERNEL);
@@ -1129,6 +1133,10 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 		temp_k = (smart_log->temperature[1] << 8) +
 				(smart_log->temperature[0]);
 		temp_c_cur = temp_k - KELVIN_TEMP_FACTOR;
Quoted 9 lines
Review static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr, · line 1139
Quoted 4 lines
(quoted diff) +4 -0
+		/* if temp_c_cur is negative,          */
+		/* set to 0 to meet Scsi Command Spec  */
+		if (temp_c_cur < 0)
+			temp_c_cur = 0;
Interesting, < 0C is colder than I might have expected. What if they can get hotter than expected, like 128C? I don't think we want to report that as 0.

Quoted 2 lines

It's unfortunate the spec is written that way. Aerospace silicon is often rated for -55/-40C to +125C junction temp +/- a few %. Thats a big negative range missing if it has to be 0. I would imagine over 128C is not meaningful so we would want to report that as 128C

Thinking about it again, I would much rather see the u8 being used so we get the full range.

something like this should do:
temp_c_cur = temp_k < KELVIN_TEMP_FACTOR ? 0 : temp_k - KELVIN_TEMP_FACTOR;

Jeffrey Lien

author

2015-10-12 20:24 UTC #4 lore raw

PATCH 1/1 NVMe: Log Sense Temperature doesn't handle negative values

-----Original Message-----
From: Jon Derrick [mailto:[email protected]]
Sent: Friday, October 9, 2015 3:41 PM
To: Keith Busch <keith.busch at intel.com>
Cc: Jeffrey Lien <Jeff.Lien at hgst.com>; axboe at fb.com; David Darrington <david.darrington at hgst.com>; linux-nvme at lists.infradead.org
Subject: Re: [PATCH] NVMe: Log Sense Temperature doesn't handle negative values

Quoted 46 lines · Jon Derrick

Quoted 40 lines
Quoted 30 lines
Quoted 15 lines
To meet the SCSI Command Spec (T10 SPC-4 r37), the log sense temperature cannot
be negative. From the spec:
The TEMPERATURE field indicates the temperature of the SCSI target
device in degrees Celsius at the time the LOG SENSE command is
performed. Temperatures equal to or less than zero degrees Celsius
shall cause the TEMPERATURE field to be set to zero. If the device
server is unable to detect a valid temperature because of a sensor
failure or other condition, then the TEMPERATURE field shall be set
to FFh. The temperature should be reported with an accuracy of plus
or minus three Celsius degrees while the SCSI target device is operating at a steady state within its environmental limits.
With the current code, a value of -2 C will be shown as 254 C since it was not written to handle negative vales. This same issue also exists in nvme_trans_log_info_exceptions.
[snip]
Quoted 12 lines
(quoted diff) +1 -2
@@ -1097,7 +1101,7 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	dma_addr_t dma_addr;
 	void *mem;
 	u32 feature_resp;
-	u8 temp_c_cur, temp_c_thresh;
+	s8 temp_c_cur, temp_c_thresh;
 	u16 temp_k;
 	log_response = kzalloc(LOG_TEMP_PAGE_LENGTH, GFP_KERNEL); @@ 
-1129,6 +1133,10 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
		temp_k = (smart_log->temperature[1] << 8) +
				(smart_log->temperature[0]);
		temp_c_cur = temp_k - KELVIN_TEMP_FACTOR;
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+		/* if temp_c_cur is negative,          */
+		/* set to 0 to meet Scsi Command Spec  */
+		if (temp_c_cur < 0)
+			temp_c_cur = 0;
Interesting, < 0C is colder than I might have expected. What if they can get hotter than expected, like 128C? I don't think we want to report that as 0.
Quoted 5 lines
It's unfortunate the spec is written that way. Aerospace silicon is often rated for -55/-40C to +125C junction temp +/- a few %. Thats a big negative range missing if it has to be 0. I would imagine over 128C is not meaningful so we would want to report that as 128C

Quoted 4 lines

Thinking about it again, I would much rather see the u8 being used so we get the full range.
something like this should do:
temp_c_cur = temp_k < KELVIN_TEMP_FACTOR ? 0 : temp_k - KELVIN_TEMP_FACTOR;

I like that solution; simple and straight forward. Want me to resubmit the patch with Jon's suggested change?
HGST E-mail Confidentiality Notice & Disclaimer:
This e-mail and any files transmitted with it may contain confidential or legally privileged information of HGST and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.

Jeffrey Lien

author

2015-10-14 15:50 UTC #5 lore raw

PATCH 1/1 NVMe: Log Sense Temperature doesn't handle negative values

Attached an updated patch with change Jon recommended below.

-----Original Message-----
From: Linux-nvme [mailto:[email protected]] On Behalf Of Jeffrey Lien
Sent: Monday, October 12, 2015 3:25 PM
To: Jon Derrick <jonathan.derrick at intel.com>; Keith Busch <keith.busch at intel.com>
Cc: axboe at fb.com; David Darrington <david.darrington at hgst.com>; linux-nvme at lists.infradead.org
Subject: RE: [PATCH] NVMe: Log Sense Temperature doesn't handle negative values

Quoted 46 lines · Jon Derrick

Quoted 40 lines
Quoted 30 lines
Quoted 15 lines
To meet the SCSI Command Spec (T10 SPC-4 r37), the log sense temperature cannot
be negative. From the spec:
The TEMPERATURE field indicates the temperature of the SCSI target
device in degrees Celsius at the time the LOG SENSE command is
performed. Temperatures equal to or less than zero degrees Celsius
shall cause the TEMPERATURE field to be set to zero. If the device
server is unable to detect a valid temperature because of a sensor
failure or other condition, then the TEMPERATURE field shall be
set to FFh. The temperature should be reported with an accuracy of
plus or minus three Celsius degrees while the SCSI target device is operating at a steady state within its environmental limits.
With the current code, a value of -2 C will be shown as 254 C since it was not written to handle negative vales. This same issue also exists in nvme_trans_log_info_exceptions.
[snip]
Quoted 12 lines
(quoted diff) +1 -2
@@ -1097,7 +1101,7 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	dma_addr_t dma_addr;
 	void *mem;
 	u32 feature_resp;
-	u8 temp_c_cur, temp_c_thresh;
+	s8 temp_c_cur, temp_c_thresh;
 	u16 temp_k;
 	log_response = kzalloc(LOG_TEMP_PAGE_LENGTH, GFP_KERNEL); @@
-1129,6 +1133,10 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
		temp_k = (smart_log->temperature[1] << 8) +
				(smart_log->temperature[0]);
		temp_c_cur = temp_k - KELVIN_TEMP_FACTOR;
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+		/* if temp_c_cur is negative,          */
+		/* set to 0 to meet Scsi Command Spec  */
+		if (temp_c_cur < 0)
+			temp_c_cur = 0;
Interesting, < 0C is colder than I might have expected. What if they can get hotter than expected, like 128C? I don't think we want to report that as 0.
Quoted 5 lines
It's unfortunate the spec is written that way. Aerospace silicon is often rated for -55/-40C to +125C junction temp +/- a few %. Thats a big negative range missing if it has to be 0. I would imagine over 128C is not meaningful so we would want to report that as 128C

Quoted 5 lines

Thinking about it again, I would much rather see the u8 being used so we get the full range.
something like this should do:
temp_c_cur = temp_k < KELVIN_TEMP_FACTOR ? 0 : temp_k - KELVIN_TEMP_FACTOR;

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-NVMe-Log-Sense-Temperature-doesn-t-handle-negative-v.patch
Type: application/octet-stream
Size: 2261 bytes
Desc: 0001-NVMe-Log-Sense-Temperature-doesn-t-handle-negative-v.patch
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20151014/5790a444/attachment.obj>

Jon Derrick

2015-10-14 16:56 UTC #6 lore raw

PATCH 1/1 NVMe: Log Sense Temperature doesn't handle negative values

Quoted 3 lines · Jeffrey Lien

Attached an updated patch with change Jon recommended below.

Looks good.

Acked-by Jon Derrick <jonathan.derrick at intel.com>

Jeffrey Lien

author

2015-10-19 22:46 UTC #7 lore raw

PATCH 1/1 NVMe: Log Sense Temperature doesn't handle negative values

Keith,
Are you going to push this patch?

-----Original Message-----
From: Jon Derrick [mailto:[email protected]]
Sent: Wednesday, October 14, 2015 11:56 AM
To: Jeffrey Lien <Jeff.Lien at hgst.com>
Cc: Keith Busch <keith.busch at intel.com>; axboe at fb.com; David Darrington <david.darrington at hgst.com>; linux-nvme at lists.infradead.org
Subject: Re: [PATCH] NVMe: Log Sense Temperature doesn't handle negative values

Quoted 3 lines · Jeffrey Lien

Attached an updated patch with change Jon recommended below.

Looks good.

Acked-by Jon Derrick <jonathan.derrick at intel.com>