* [PATCH] NVMe: Log Sense Temperature doesn't handle negative values @ 2015-10-08 18:35 Jeffrey Lien 2015-10-09 19:52 ` Keith Busch 0 siblings, 1 reply; 8+ messages in thread From: Jeffrey Lien @ 2015-10-08 18:35 UTC (permalink / raw) 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: Log Sense Temperature doesn't handle negative values 2015-10-08 18:35 [PATCH] NVMe: Log Sense Temperature doesn't handle negative values Jeffrey Lien @ 2015-10-09 19:52 ` Keith Busch 2015-10-09 20:15 ` Jon Derrick 0 siblings, 1 reply; 8+ messages in thread From: Keith Busch @ 2015-10-09 19:52 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: Log Sense Temperature doesn't handle negative values 2015-10-09 19:52 ` Keith Busch @ 2015-10-09 20:15 ` Jon Derrick 2015-10-09 20:41 ` Jon Derrick 0 siblings, 1 reply; 8+ messages in thread From: Jon Derrick @ 2015-10-09 20:15 UTC (permalink / raw) > >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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: Log Sense Temperature doesn't handle negative values 2015-10-09 20:15 ` Jon Derrick @ 2015-10-09 20:41 ` Jon Derrick 2015-10-12 20:24 ` Jeffrey Lien 0 siblings, 1 reply; 8+ messages in thread From: Jon Derrick @ 2015-10-09 20:41 UTC (permalink / raw) 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; ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: Log Sense Temperature doesn't handle negative values 2015-10-09 20:41 ` Jon Derrick @ 2015-10-12 20:24 ` Jeffrey Lien 2015-10-14 15:50 ` Jeffrey Lien 0 siblings, 1 reply; 8+ messages in thread From: Jeffrey Lien @ 2015-10-12 20:24 UTC (permalink / raw) -----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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: Log Sense Temperature doesn't handle negative values 2015-10-12 20:24 ` Jeffrey Lien @ 2015-10-14 15:50 ` Jeffrey Lien 2015-10-14 16:56 ` Jon Derrick 0 siblings, 1 reply; 8+ messages in thread From: Jeffrey Lien @ 2015-10-14 15:50 UTC (permalink / raw) 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: Log Sense Temperature doesn't handle negative values 2015-10-14 15:50 ` Jeffrey Lien @ 2015-10-14 16:56 ` Jon Derrick 2015-10-19 22:46 ` Jeffrey Lien 0 siblings, 1 reply; 8+ messages in thread From: Jon Derrick @ 2015-10-14 16:56 UTC (permalink / raw) 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: Log Sense Temperature doesn't handle negative values 2015-10-14 16:56 ` Jon Derrick @ 2015-10-19 22:46 ` Jeffrey Lien 0 siblings, 0 replies; 8+ messages in thread From: Jeffrey Lien @ 2015-10-19 22:46 UTC (permalink / raw) 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-10-19 22:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-08 18:35 [PATCH] NVMe: Log Sense Temperature doesn't handle negative values Jeffrey Lien 2015-10-09 19:52 ` Keith Busch 2015-10-09 20:15 ` Jon Derrick 2015-10-09 20:41 ` Jon Derrick 2015-10-12 20:24 ` Jeffrey Lien 2015-10-14 15:50 ` Jeffrey Lien 2015-10-14 16:56 ` Jon Derrick 2015-10-19 22:46 ` Jeffrey Lien
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.