[PATCH] NVMe: Log Sense Temperature doesn't handle negative values

[PATCH] NVMe: Log Sense Temperature doesn't handle negative values

[Jeffrey Lien]

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>

[PATCH] NVMe: Log Sense Temperature doesn't handle negative values

[Keith Busch]

On Thu, 8 Oct 2015, Jeffrey Lien wrote:
> 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]

> @@ -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;
> +		/* 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.

[PATCH] NVMe: Log Sense Temperature doesn't handle negative values

[Jon Derrick]

> >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]
> 
> >@@ -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;
> >+		/* 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

[PATCH] NVMe: Log Sense Temperature doesn't handle negative values

[Jon Derrick]

On Fri, Oct 09, 2015@02:15:33PM -0600, Jon Derrick wrote:
> > >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]
> > 
> > >@@ -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;
> > >+		/* 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
> 

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;

[PATCH] NVMe: Log Sense Temperature doesn't handle negative values

[Jeffrey Lien]

-----Original Message-----
From: Jon Derrick [mailto:jonathan.derrick@intel.com] 
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

On Fri, Oct 09, 2015@02:15:33PM -0600, Jon Derrick wrote:
>> > >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]
>> > 
>> > >@@ -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;
>> > >+		/* 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
>> 

>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.

[PATCH] NVMe: Log Sense Temperature doesn't handle negative values

[Jeffrey Lien]

Attached an updated patch with change Jon recommended below.   


-----Original Message-----
From: Linux-nvme [mailto:linux-nvme-bounces@lists.infradead.org] 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



-----Original Message-----
From: Jon Derrick [mailto:jonathan.derrick@intel.com]
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

On Fri, Oct 09, 2015@02:15:33PM -0600, Jon Derrick wrote:
>> > >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]
>> > 
>> > >@@ -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;
>> > >+		/* 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
>> 

>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.


_______________________________________________
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>

[PATCH] NVMe: Log Sense Temperature doesn't handle negative values

[Jon Derrick]

On Wed, Oct 14, 2015@03:50:32PM +0000, Jeffrey Lien wrote:
> Attached an updated patch with change Jon recommended below.   
> 
> 

Looks good.

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

[PATCH] NVMe: Log Sense Temperature doesn't handle negative values

[Jeffrey Lien]

Keith,
Are you going to push this patch?   

-----Original Message-----
From: Jon Derrick [mailto:jonathan.derrick@intel.com] 
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

On Wed, Oct 14, 2015@03:50:32PM +0000, Jeffrey Lien wrote:
> Attached an updated patch with change Jon recommended below.   
> 
> 

Looks good.

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