[PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Avi Shchislowski]

UFS3.0 allows using the ufs device as a temperature sensor. The
purpose of this feature is to provide notification to the host of the
UFS device case temperature. It allows reading of a rough estimate
(+-10 degrees centigrade) of the current case temperature, And
setting a lower and upper temperature bounds, in which the device
will trigger an applicable exception event.

We added the capability of responding to such notifications, while
notifying the kernel's thermal core, which further exposes the thermal
zone attributes to user space. UFS temperature attributes are all
read-only, so only thermal read ops (.get_xxx) can be implemented.

Avi Shchislowski (5):
  scsi: ufs: Add ufs thermal support
  scsi: ufs: export ufshcd_enable_ee
  scsi: ufs: enable thermal exception event
  scsi: ufs-thermal: implement thermal file ops
  scsi: ufs: temperature atrributes add to ufs_sysfs

 drivers/scsi/ufs/Kconfig       |  11 ++
 drivers/scsi/ufs/Makefile      |   1 +
 drivers/scsi/ufs/ufs-sysfs.c   |   6 +
 drivers/scsi/ufs/ufs-thermal.c | 247 +++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs-thermal.h |  25 +++++
 drivers/scsi/ufs/ufs.h         |  20 +++-
 drivers/scsi/ufs/ufshcd.c      |   9 +-
 drivers/scsi/ufs/ufshcd.h      |  12 ++
 8 files changed, 329 insertions(+), 2 deletions(-)
 create mode 100644 drivers/scsi/ufs/ufs-thermal.c
 create mode 100644 drivers/scsi/ufs/ufs-thermal.h

-- 
1.9.1

[PATCH 1/5] scsi: ufs: Add ufs thermal support

[Avi Shchislowski]

Support the new temperature notification attributes introduced in
UFSv3.0. Add exception event mask, and ufs features attributes.

Signed-off-by: Uri Yanai <uri.yanai@wdc.com>
Signed-off-by: Avi Shchislowski <avi.shchislowski@wdc.com>
---
 drivers/scsi/ufs/Kconfig       |  11 ++++
 drivers/scsi/ufs/Makefile      |   1 +
 drivers/scsi/ufs/ufs-thermal.c | 123 +++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs-thermal.h |  19 +++++++
 drivers/scsi/ufs/ufs.h         |  11 ++++
 drivers/scsi/ufs/ufshcd.c      |   3 +
 drivers/scsi/ufs/ufshcd.h      |  10 ++++
 7 files changed, 178 insertions(+)
 create mode 100644 drivers/scsi/ufs/ufs-thermal.c
 create mode 100644 drivers/scsi/ufs/ufs-thermal.h

diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index d14c224..bed56ee 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -160,3 +160,14 @@ config SCSI_UFS_BSG
 
 	  Select this if you need a bsg device node for your UFS controller.
 	  If unsure, say N.
+
+config THERMAL_UFS
+	bool "Thermal UFS"
+	depends on THERMAL && SCSI_UFSHCD
+	help
+	  A UFS3.0 feature that allows using the ufs device as a temperature
+	  sensor. it provide notification to the host when the UFS device
+	  case temperature approaches its pre-defined boundaries.
+
+	  Select Y to enable this feature, otherwise say N.
+	  If unsure, say N.
\ No newline at end of file
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 94c6c5d..fd35941 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
 obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
 obj-$(CONFIG_SCSI_UFS_MEDIATEK) += ufs-mediatek.o
 obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o
+obj-$(CONFIG_THERMAL_UFS) += ufs-thermal.o
diff --git a/drivers/scsi/ufs/ufs-thermal.c b/drivers/scsi/ufs/ufs-thermal.c
new file mode 100644
index 0000000..469c1ed
--- /dev/null
+++ b/drivers/scsi/ufs/ufs-thermal.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * thermal ufs
+ *
+ * Copyright (C) 2020 Western Digital Corporation
+ */
+#include <linux/thermal.h>
+#include "ufs-thermal.h"
+
+enum {
+	UFS_THERM_MAX_TEMP,
+	UFS_THERM_HIGH_TEMP,
+	UFS_THERM_LOW_TEMP,
+	UFS_THERM_MIN_TEMP,
+
+	/* keep last */
+	UFS_THERM_MAX_TRIPS
+};
+
+/**
+ *struct ufs_thermal - thermal zone related data
+ * @tzone: thermal zone device data
+ */
+static struct ufs_thermal {
+	struct thermal_zone_device *zone;
+} thermal;
+
+static  struct thermal_zone_device_ops ufs_thermal_ops = {
+	.get_temp = NULL,
+	.get_trip_temp = NULL,
+	.get_trip_type = NULL,
+};
+
+static int ufs_thermal_enable_ee(struct ufs_hba *hba)
+{
+	/* later */
+	return -EINVAL;
+}
+
+static void ufs_thermal_zone_unregister(struct ufs_hba *hba)
+{
+	if (thermal.zone) {
+		dev_dbg(hba->dev, "Thermal zone device unregister\n");
+		thermal_zone_device_unregister(thermal.zone);
+		thermal.zone = NULL;
+	}
+}
+
+static int ufs_thermal_register(struct ufs_hba *hba)
+{
+	int err = 0;
+	char name[THERMAL_NAME_LENGTH] = {};
+
+	snprintf(name, THERMAL_NAME_LENGTH, "ufs_storage_%d",
+			hba->host->host_no);
+
+	thermal.zone = thermal_zone_device_register(name, UFS_THERM_MAX_TRIPS,
+			0, hba, &ufs_thermal_ops, NULL, 0, 0);
+	if (IS_ERR(thermal.zone)) {
+		err = PTR_ERR(thermal.zone);
+		dev_err(hba->dev, "Failed to register to thermal zone, err %d\n",
+				err);
+		thermal.zone = NULL;
+		goto out;
+	}
+
+	 /* thermal support is enabled only after successful
+	  * enablement of thermal exception
+	  */
+	if (ufs_thermal_enable_ee(hba)) {
+		dev_info(hba->dev, "Failed to enable thermal exception\n");
+		ufs_thermal_zone_unregister(hba);
+		err = -EINVAL;
+	}
+
+out:
+	return err;
+}
+
+int ufs_thermal_probe(struct ufs_hba *hba)
+{
+	u8 ufs_features;
+	u8 *desc_buf = NULL;
+	int err = -EINVAL;
+
+	if (!ufshcd_thermal_management_enabled(hba))
+		goto out;
+
+	desc_buf = kzalloc(hba->desc_size.dev_desc, GFP_KERNEL);
+	if (!desc_buf) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	if (ufshcd_read_desc_param(hba, QUERY_DESC_IDN_DEVICE, 0, 0, desc_buf,
+			hba->desc_size.dev_desc))
+		goto out;
+
+
+	ufs_features = desc_buf[DEVICE_DESC_PARAM_UFS_FEAT] &
+			(UFS_FEATURE_HTEMP | UFS_FEATURE_LTEMP);
+	if (!ufs_features)
+		goto out;
+
+	err = ufs_thermal_register(hba);
+	if (err)
+		goto out;
+
+	hba->thermal_features = ufs_features;
+
+out:
+	kfree(desc_buf);
+	return err;
+}
+
+void ufs_thermal_remove(struct ufs_hba *hba)
+{
+	if (!ufshcd_thermal_management_enabled(hba))
+		return;
+
+	 ufs_thermal_zone_unregister(hba);
+	 hba->thermal_features = 0;
+}
diff --git a/drivers/scsi/ufs/ufs-thermal.h b/drivers/scsi/ufs/ufs-thermal.h
new file mode 100644
index 0000000..7c0fcbe
--- /dev/null
+++ b/drivers/scsi/ufs/ufs-thermal.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Western Digital Corporation
+ */
+#ifndef UFS_THERMAL_H
+#define UFS_THERMAL_H
+
+#include "ufshcd.h"
+#include "ufs.h"
+
+#ifdef CONFIG_THERMAL_UFS
+void ufs_thermal_remove(struct ufs_hba *hba);
+int ufs_thermal_probe(struct ufs_hba *hba);
+#else
+static inline void ufs_thermal_remove(struct ufs_hba *hba) {}
+static inline int ufs_thermal_probe(struct ufs_hba *hba) {return 0; }
+#endif /* CONFIG_THERMAL_UFS */
+
+#endif /* UFS_THERMAL_H */
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index dde2eb0..eb729cc 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -332,6 +332,17 @@ enum {
 	UFSHCD_AMP		= 3,
 };
 
+/* UFS Features - to decode bUFSFeaturesSupport */
+enum {
+	UFS_FEATURE_FFU		= BIT(0),
+	UFS_FEATURE_PSA		= BIT(1),
+	UFS_FEATURE_LIFE		= BIT(2),
+	UFS_FEATURE_REFRESH		= BIT(3),
+	UFS_FEATURE_HTEMP		= BIT(4),
+	UFS_FEATURE_LTEMP		= BIT(5),
+	UFS_FEATURE_ETEMP		= BIT(6),
+};
+
 #define POWER_DESC_MAX_SIZE			0x62
 #define POWER_DESC_MAX_ACTV_ICC_LVLS		16
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index abd0e6b..099d2de 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -47,6 +47,7 @@
 #include "unipro.h"
 #include "ufs-sysfs.h"
 #include "ufs_bsg.h"
+#include "ufs-thermal.h"
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/ufs.h>
@@ -7111,6 +7112,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool async)
 
 	/* Enable Auto-Hibernate if configured */
 	ufshcd_auto_hibern8_enable(hba);
+	ufs_thermal_probe(hba);
 
 out:
 
@@ -8278,6 +8280,7 @@ int ufshcd_shutdown(struct ufs_hba *hba)
  */
 void ufshcd_remove(struct ufs_hba *hba)
 {
+	ufs_thermal_remove(hba);
 	ufs_bsg_remove(hba);
 	ufs_sysfs_remove_nodes(hba->dev);
 	blk_cleanup_queue(hba->tmf_queue);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 2ae6c7c..28c0063 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -730,6 +730,11 @@ struct ufs_hba {
 
 	struct device		bsg_dev;
 	struct request_queue	*bsg_queue;
+
+#define UFSHCD_CAP_THERMAL_MANAGEMENT (1 << 7)
+
+	u8 thermal_features;
+
 };
 
 /* Returns true if clocks can be gated. Otherwise false */
@@ -754,6 +759,11 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba)
 	return hba->caps & UFSHCD_CAP_RPM_AUTOSUSPEND;
 }
 
+static inline bool ufshcd_thermal_management_enabled(struct ufs_hba *hba)
+{
+	return hba->caps & UFSHCD_CAP_THERMAL_MANAGEMENT;
+}
+
 static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba)
 {
 /* DWC UFS Core has the Interrupt aggregation feature but is not detectable*/
-- 
1.9.1

[PATCH 2/5] scsi: ufs: export ufshcd_enable_ee

[Avi Shchislowski]

export ufshcd_enable_ee so that other modules can use it. this will
come handy in the next patch where we will need it to enable thermal
support

Signed-off-by: Uri Yanai <uri.yanai@wdc.com>
Signed-off-by: Avi Shchislowski <avi.shchislowski@wdc.com>
---
 drivers/scsi/ufs/ufshcd.c | 2 +-
 drivers/scsi/ufs/ufshcd.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 099d2de..f25b93c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4923,7 +4923,7 @@ static int ufshcd_disable_ee(struct ufs_hba *hba, u16 mask)
  *
  * Returns zero on success, non-zero error value on failure.
  */
-static int ufshcd_enable_ee(struct ufs_hba *hba, u16 mask)
+int ufshcd_enable_ee(struct ufs_hba *hba, u16 mask)
 {
 	int err = 0;
 	u32 val;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 28c0063..6d2a5fd 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -960,6 +960,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 			     u8 *desc_buff, int *buff_len,
 			     enum query_opcode desc_op);
 
+int ufshcd_enable_ee(struct ufs_hba *hba, u16 mask);
+
 /* Wrapper functions for safely calling variant operations */
 static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
 {
-- 
1.9.1

[PATCH 3/5] scsi: ufs: enable thermal exception event

[Avi Shchislowski]

The host might need to be aware of the device's temperature, when it's
too high or too low. Should such event occur, the device is expected
to notify it to the host by using the exception event mechanism.

E.g. when TOO_HIGH_TEMP in wExceptionEventStatus is raised, it is
recommended to perform thermal throttling or other cooling activities
for lowering the device Tcase temperature. Similarly, when
TOO_LOW_TEMP is raised, it is recommended to take an applicable
actions to increase the device’s Tcase temperature.

Signed-off-by: Uri Yanai <uri.yanai@wdc.com>
Signed-off-by: Avi Shchislowski <avi.shchislowski@wdc.com>
---
 drivers/scsi/ufs/ufs-thermal.c | 28 ++++++++++++++++++++++++----
 drivers/scsi/ufs/ufs-thermal.h |  6 ++++++
 drivers/scsi/ufs/ufs.h         |  6 +++++-
 drivers/scsi/ufs/ufshcd.c      |  4 ++++
 4 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-thermal.c b/drivers/scsi/ufs/ufs-thermal.c
index 469c1ed..dfa5d68 100644
--- a/drivers/scsi/ufs/ufs-thermal.c
+++ b/drivers/scsi/ufs/ufs-thermal.c
@@ -19,11 +19,32 @@ enum {
 
 /**
  *struct ufs_thermal - thermal zone related data
- * @tzone: thermal zone device data
+ * @trip: trip array
  */
 static struct ufs_thermal {
 	struct thermal_zone_device *zone;
-} thermal;
+	int trip[UFS_THERM_MAX_TRIPS];
+} thermal = {
+		.trip = {
+			[UFS_THERM_MAX_TEMP] = 170 * 1000,
+			[UFS_THERM_MIN_TEMP] = -79 * 1000
+		}
+};
+
+void ufs_thermal_exception_event_handler(struct ufs_hba *hba,
+		u32 exception_status)
+{
+	if (WARN_ON_ONCE(!hba->thermal_features))
+		return;
+
+	if (exception_status & MASK_EE_TOO_HIGH_TEMP) {
+		thermal_notify_framework(thermal.zone, UFS_THERM_HIGH_TEMP);
+		dev_info(hba->dev, "High temperature raised\n");
+	} else if (exception_status & MASK_EE_TOO_LOW_TEMP) {
+		thermal_notify_framework(thermal.zone, UFS_THERM_LOW_TEMP);
+		dev_info(hba->dev, "Low temperature raised\n");
+	}
+}
 
 static  struct thermal_zone_device_ops ufs_thermal_ops = {
 	.get_temp = NULL,
@@ -33,8 +54,7 @@ enum {
 
 static int ufs_thermal_enable_ee(struct ufs_hba *hba)
 {
-	/* later */
-	return -EINVAL;
+	return ufshcd_enable_ee(hba, MASK_EE_URGENT_TEMP);
 }
 
 static void ufs_thermal_zone_unregister(struct ufs_hba *hba)
diff --git a/drivers/scsi/ufs/ufs-thermal.h b/drivers/scsi/ufs/ufs-thermal.h
index 7c0fcbe..285049e 100644
--- a/drivers/scsi/ufs/ufs-thermal.h
+++ b/drivers/scsi/ufs/ufs-thermal.h
@@ -11,9 +11,15 @@
 #ifdef CONFIG_THERMAL_UFS
 void ufs_thermal_remove(struct ufs_hba *hba);
 int ufs_thermal_probe(struct ufs_hba *hba);
+void ufs_thermal_exception_event_handler(struct ufs_hba *hba,
+		u32 exception_status);
 #else
 static inline void ufs_thermal_remove(struct ufs_hba *hba) {}
 static inline int ufs_thermal_probe(struct ufs_hba *hba) {return 0; }
+void ufs_thermal_exception_event_handler(struct ufs_hba *hba,
+		u32 exception_status)
+{
+}
 #endif /* CONFIG_THERMAL_UFS */
 
 #endif /* UFS_THERMAL_H */
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index eb729cc..8fc0b0c 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -363,7 +363,9 @@ enum power_desc_param_offset {
 /* Exception event mask values */
 enum {
 	MASK_EE_STATUS		= 0xFFFF,
-	MASK_EE_URGENT_BKOPS	= (1 << 2),
+	MASK_EE_URGENT_BKOPS	= BIT(2),
+	MASK_EE_TOO_HIGH_TEMP	= BIT(3),
+	MASK_EE_TOO_LOW_TEMP	= BIT(4),
 };
 
 /* Background operation status */
@@ -375,6 +377,8 @@ enum bkops_status {
 	BKOPS_STATUS_MAX		 = BKOPS_STATUS_CRITICAL,
 };
 
+#define MASK_EE_URGENT_TEMP (MASK_EE_TOO_HIGH_TEMP | MASK_EE_TOO_LOW_TEMP)
+
 /* UTP QUERY Transaction Specific Fields OpCode */
 enum query_opcode {
 	UPIU_QUERY_OPCODE_NOP		= 0x0,
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f25b93c..45fb52d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -42,6 +42,7 @@
 #include <linux/nls.h>
 #include <linux/of.h>
 #include <linux/bitfield.h>
+#include <linux/thermal.h>
 #include "ufshcd.h"
 #include "ufs_quirks.h"
 #include "unipro.h"
@@ -5183,6 +5184,9 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 	if (status & MASK_EE_URGENT_BKOPS)
 		ufshcd_bkops_exception_event_handler(hba);
 
+	if (status & MASK_EE_URGENT_TEMP)
+		ufs_thermal_exception_event_handler(hba, status);
+
 out:
 	ufshcd_scsi_unblock_requests(hba);
 	pm_runtime_put_sync(hba->dev);
-- 
1.9.1

[PATCH 4/5] scsi: ufs-thermal: implement thermal file ops

[Avi Shchislowski]

The thermal interface adds a new thermal zone device sensor under
/sys/class/thermal/ folder.

Signed-off-by: Uri Yanai <uri.yanai@wdc.com>
Signed-off-by: Avi Shchislowski <avi.shchislowski@wdc.com>
---
 drivers/scsi/ufs/ufs-thermal.c | 122 ++++++++++++++++++++++++++++++++++++++---
 drivers/scsi/ufs/ufs.h         |   3 +
 2 files changed, 117 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-thermal.c b/drivers/scsi/ufs/ufs-thermal.c
index dfa5d68..23e4ac1 100644
--- a/drivers/scsi/ufs/ufs-thermal.c
+++ b/drivers/scsi/ufs/ufs-thermal.c
@@ -31,6 +31,99 @@ enum {
 		}
 };
 
+#define attr2milicelcius(attr) (((0xFF & attr) - 80) * 1000)
+
+static int ufs_thermal_get_temp(struct thermal_zone_device *device,
+				  int *temperature)
+{
+	struct ufs_hba *hba = (struct ufs_hba *)device->devdata;
+	u32 temp;
+	int err;
+
+	err = ufshcd_query_attr(hba,
+			UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_ROUGH_TEMP,
+			0, 0, &temp);
+	if (err)
+		return -EINVAL;
+
+	*temperature = attr2milicelcius(temp);
+	return 0;
+}
+
+static int ufs_thermal_get_trip_temp(
+		struct thermal_zone_device *device,
+				 int trip, int *temp)
+{
+
+	if (trip < 0 || trip >= UFS_THERM_MAX_TRIPS)
+		return -EINVAL;
+
+	*temp = thermal.trip[trip];
+
+	return 0;
+}
+
+static int ufs_thermal_get_trip_type(
+		struct thermal_zone_device *device,
+		int trip, enum thermal_trip_type *type)
+{
+	if (trip < 0 || trip >= UFS_THERM_MAX_TRIPS)
+		return -EINVAL;
+
+	*type = THERMAL_TRIP_PASSIVE;
+
+	return 0;
+}
+
+static int ufs_thermal_get_boundary(struct ufs_hba *hba,
+					int trip, int *boundary)
+{
+	enum attr_idn idn;
+	int err = 0;
+	u32 val;
+
+	idn = trip == UFS_THERM_HIGH_TEMP ?
+			QUERY_ATTR_IDN_TOO_HIGH_TEMP :
+			QUERY_ATTR_IDN_TOO_LOW_TEMP;
+
+	err = ufshcd_query_attr(hba,
+			UPIU_QUERY_OPCODE_READ_ATTR,
+			idn, 0, 0, &val);
+	if (err) {
+		dev_err(hba->dev,
+		"Failed to get device too %s temperature boundary\n",
+		trip == UFS_THERM_HIGH_TEMP ? "high" : "low");
+		goto out;
+	}
+
+	if (val < 1 || val > 250) {
+		dev_err(hba->dev, "out of device temperature boundary\n");
+		err = -EINVAL;
+		goto out;
+	}
+
+	*boundary = attr2milicelcius(val);
+
+out:
+	return err;
+}
+
+static int ufs_thermal_set_trip(struct ufs_hba *hba, int trip)
+{
+	int temp;
+	int err = 0;
+
+	err = ufs_thermal_get_boundary(hba, trip, &temp);
+	if (err)
+		return err;
+
+	thermal.trip[trip] = temp;
+
+	return err;
+
+}
+
 void ufs_thermal_exception_event_handler(struct ufs_hba *hba,
 		u32 exception_status)
 {
@@ -46,17 +139,12 @@ void ufs_thermal_exception_event_handler(struct ufs_hba *hba,
 	}
 }
 
-static  struct thermal_zone_device_ops ufs_thermal_ops = {
-	.get_temp = NULL,
-	.get_trip_temp = NULL,
-	.get_trip_type = NULL,
-};
-
 static int ufs_thermal_enable_ee(struct ufs_hba *hba)
 {
 	return ufshcd_enable_ee(hba, MASK_EE_URGENT_TEMP);
 }
 
+
 static void ufs_thermal_zone_unregister(struct ufs_hba *hba)
 {
 	if (thermal.zone) {
@@ -66,7 +154,13 @@ static void ufs_thermal_zone_unregister(struct ufs_hba *hba)
 	}
 }
 
-static int ufs_thermal_register(struct ufs_hba *hba)
+static  struct thermal_zone_device_ops ufs_thermal_ops = {
+	.get_temp = ufs_thermal_get_temp,
+	.get_trip_temp = ufs_thermal_get_trip_temp,
+	.get_trip_type = ufs_thermal_get_trip_type,
+};
+
+static int ufs_thermal_register(struct ufs_hba *hba, u8 ufs_features)
 {
 	int err = 0;
 	char name[THERMAL_NAME_LENGTH] = {};
@@ -74,6 +168,18 @@ static int ufs_thermal_register(struct ufs_hba *hba)
 	snprintf(name, THERMAL_NAME_LENGTH, "ufs_storage_%d",
 			hba->host->host_no);
 
+	if (ufs_features & UFS_FEATURE_HTEMP) {
+		err = ufs_thermal_set_trip(hba, UFS_THERM_HIGH_TEMP);
+		if (err)
+			goto out;
+	}
+
+	if (ufs_features & UFS_FEATURE_LTEMP) {
+		err = ufs_thermal_set_trip(hba, UFS_THERM_LOW_TEMP);
+		if (err)
+			goto out;
+	}
+
 	thermal.zone = thermal_zone_device_register(name, UFS_THERM_MAX_TRIPS,
 			0, hba, &ufs_thermal_ops, NULL, 0, 0);
 	if (IS_ERR(thermal.zone)) {
@@ -122,7 +228,7 @@ int ufs_thermal_probe(struct ufs_hba *hba)
 	if (!ufs_features)
 		goto out;
 
-	err = ufs_thermal_register(hba);
+	err = ufs_thermal_register(hba, ufs_features);
 	if (err)
 		goto out;
 
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 8fc0b0c..9f8224b 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -167,6 +167,9 @@ enum attr_idn {
 	QUERY_ATTR_IDN_FFU_STATUS		= 0x14,
 	QUERY_ATTR_IDN_PSA_STATE		= 0x15,
 	QUERY_ATTR_IDN_PSA_DATA_SIZE		= 0x16,
+	QUERY_ATTR_IDN_ROUGH_TEMP		= 0x18,
+	QUERY_ATTR_IDN_TOO_HIGH_TEMP		= 0x19,
+	QUERY_ATTR_IDN_TOO_LOW_TEMP		= 0x1A,
 };
 
 /* Descriptor idn for Query requests */
-- 
1.9.1

[PATCH 5/5] scsi: ufs: temperature atrributes add to ufs_sysfs

[Avi Shchislowski]

From: Avi Shchislowski <avi.shchislowski@swdc.com>

Signed-off-by: Uri Yanai <uri.yanai@wdc.com>
Signed-off-by: Avi Shchislowski <avi.shchislowski@wdc.com>
---
 drivers/scsi/ufs/ufs-sysfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index dbdf8b0..180f4db 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -667,6 +667,9 @@ static DEVICE_ATTR_RO(_name)
 UFS_ATTRIBUTE(ffu_status, _FFU_STATUS);
 UFS_ATTRIBUTE(psa_state, _PSA_STATE);
 UFS_ATTRIBUTE(psa_data_size, _PSA_DATA_SIZE);
+UFS_ATTRIBUTE(rough_temp, _ROUGH_TEMP);
+UFS_ATTRIBUTE(too_high_temp, _TOO_HIGH_TEMP);
+UFS_ATTRIBUTE(too_low_temp, _TOO_LOW_TEMP);
 
 static struct attribute *ufs_sysfs_attributes[] = {
 	&dev_attr_boot_lun_enabled.attr,
@@ -685,6 +688,9 @@ static DEVICE_ATTR_RO(_name)
 	&dev_attr_ffu_status.attr,
 	&dev_attr_psa_state.attr,
 	&dev_attr_psa_data_size.attr,
+	&dev_attr_rough_temp.attr,
+	&dev_attr_too_high_temp.attr,
+	&dev_attr_too_low_temp.attr,
 	NULL,
 };
 
-- 
1.9.1

Re: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Guenter Roeck]

On Sun, Feb 02, 2020 at 12:46:54PM +0200, Avi Shchislowski wrote:
> UFS3.0 allows using the ufs device as a temperature sensor. The
> purpose of this feature is to provide notification to the host of the
> UFS device case temperature. It allows reading of a rough estimate
> (+-10 degrees centigrade) of the current case temperature, And
> setting a lower and upper temperature bounds, in which the device
> will trigger an applicable exception event.
> 
> We added the capability of responding to such notifications, while
> notifying the kernel's thermal core, which further exposes the thermal
> zone attributes to user space. UFS temperature attributes are all
> read-only, so only thermal read ops (.get_xxx) can be implemented.
> 

Can you add an explanation why this can't be added to the just-introduced
'drivetemp' driver in the hwmon subsystem, and why it make sense to
have proprietary attributes for temperature and temperature limits ?

Thanks,
Guenter

> Avi Shchislowski (5):
>   scsi: ufs: Add ufs thermal support
>   scsi: ufs: export ufshcd_enable_ee
>   scsi: ufs: enable thermal exception event
>   scsi: ufs-thermal: implement thermal file ops
>   scsi: ufs: temperature atrributes add to ufs_sysfs

attributes

RE: [EXT] [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Bean Huo (beanhuo)]

Hi, Avi
Please add "resend" prefix or version in your subject when you re-send your patches for the next time, 
Such make us easier follow your changes.

Thanks,

//Bean

> Subject: [EXT] [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor
> 
> Avi Shchislowski (5):
>   scsi: ufs: Add ufs thermal support
>   scsi: ufs: export ufshcd_enable_ee
>   scsi: ufs: enable thermal exception event
>   scsi: ufs-thermal: implement thermal file ops
>   scsi: ufs: temperature atrributes add to ufs_sysfs
> 
>  drivers/scsi/ufs/Kconfig       |  11 ++
>  drivers/scsi/ufs/Makefile      |   1 +
>  drivers/scsi/ufs/ufs-sysfs.c   |   6 +
>  drivers/scsi/ufs/ufs-thermal.c | 247
> +++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufs-thermal.h |  25 +++++
>  drivers/scsi/ufs/ufs.h         |  20 +++-
>  drivers/scsi/ufs/ufshcd.c      |   9 +-
>  drivers/scsi/ufs/ufshcd.h      |  12 ++
>  8 files changed, 329 insertions(+), 2 deletions(-)  create mode 100644
> drivers/scsi/ufs/ufs-thermal.c  create mode 100644 drivers/scsi/ufs/ufs-
> thermal.h
> 
> --
> 1.9.1

RE: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Avi Shchislowski]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Sunday, February 2, 2020 9:21 PM
> To: Avi Shchislowski <Avi.Shchislowski@wdc.com>
> Cc: Alim Akhtar <alim.akhtar@samsung.com>; Avri Altman
> <Avri.Altman@wdc.com>; James E.J. Bottomley <jejb@linux.ibm.com>;
> Martin K. Petersen <martin.petersen@oracle.com>; linux-
> kernel@vger.kernel.org; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor
> 
 
> On Sun, Feb 02, 2020 at 12:46:54PM +0200, Avi Shchislowski wrote:
> > UFS3.0 allows using the ufs device as a temperature sensor. The
> > purpose of this feature is to provide notification to the host of the
> > UFS device case temperature. It allows reading of a rough estimate
> > (+-10 degrees centigrade) of the current case temperature, And setting
> > a lower and upper temperature bounds, in which the device will trigger
> > an applicable exception event.
> >
> > We added the capability of responding to such notifications, while
> > notifying the kernel's thermal core, which further exposes the thermal
> > zone attributes to user space. UFS temperature attributes are all
> > read-only, so only thermal read ops (.get_xxx) can be implemented.
> >
> 
> Can you add an explanation why this can't be added to the just-introduced
> 'drivetemp' driver in the hwmon subsystem, and why it make sense to have
> proprietary attributes for temperature and temperature limits ?
> 
> Thanks,
> Guenter
> 
Hi Guenter

Thank you for your comment

The ufs device is not a temperature sensor per-se.  It is, first and foremost, a storage device.
Reporting the device case temperature is a feature added in a recently released UFS spec (UFS3.0).
Therefore, adding a thermal-core module, in opposed to hwmon module, seemed more appropriate.
Registering a hwmon device look excessive, as no other hw-monitoring attribute is available - aside temperature.

Using Martin's tree, I wasn't able to locate the 'drivetemp' module, nor any reference to  it in the hwmon documentation.

> > Avi Shchislowski (5):
> >   scsi: ufs: Add ufs thermal support
> >   scsi: ufs: export ufshcd_enable_ee
> >   scsi: ufs: enable thermal exception event
> >   scsi: ufs-thermal: implement thermal file ops
> >   scsi: ufs: temperature atrributes add to ufs_sysfs
> 
> attributes

Re: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Guenter Roeck]

On 2/3/20 3:57 AM, Avi Shchislowski wrote:
> 
> 
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Sunday, February 2, 2020 9:21 PM
>> To: Avi Shchislowski <Avi.Shchislowski@wdc.com>
>> Cc: Alim Akhtar <alim.akhtar@samsung.com>; Avri Altman
>> <Avri.Altman@wdc.com>; James E.J. Bottomley <jejb@linux.ibm.com>;
>> Martin K. Petersen <martin.petersen@oracle.com>; linux-
>> kernel@vger.kernel.org; linux-scsi@vger.kernel.org
>> Subject: Re: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor
>>
>   
>> On Sun, Feb 02, 2020 at 12:46:54PM +0200, Avi Shchislowski wrote:
>>> UFS3.0 allows using the ufs device as a temperature sensor. The
>>> purpose of this feature is to provide notification to the host of the
>>> UFS device case temperature. It allows reading of a rough estimate
>>> (+-10 degrees centigrade) of the current case temperature, And setting
>>> a lower and upper temperature bounds, in which the device will trigger
>>> an applicable exception event.
>>>
>>> We added the capability of responding to such notifications, while
>>> notifying the kernel's thermal core, which further exposes the thermal
>>> zone attributes to user space. UFS temperature attributes are all
>>> read-only, so only thermal read ops (.get_xxx) can be implemented.
>>>
>>
>> Can you add an explanation why this can't be added to the just-introduced
>> 'drivetemp' driver in the hwmon subsystem, and why it make sense to have
>> proprietary attributes for temperature and temperature limits ?
>>
>> Thanks,
>> Guenter
>>
> Hi Guenter
> 
> Thank you for your comment
> 
> The ufs device is not a temperature sensor per-se.  It is, first and foremost, a storage device.

Huh ? Many non-temperature-sensor devices in the kernel report their temperature
through the hardware monitoring subsystem.

> Reporting the device case temperature is a feature added in a recently released UFS spec (UFS3.0).
> Therefore, adding a thermal-core module, in opposed to hwmon module, seemed more appropriate.
> Registering a hwmon device look excessive, as no other hw-monitoring attribute is available - aside temperature.
> 
Really ? Interesting position. Are you saying that instantiating a thermal core
module, plus providing non-standard sysfs attributes to report the temperature,
is less complex ? Are you sure ? Can you provide evidence that this is indeed
the case ?

> Using Martin's tree, I wasn't able to locate the 'drivetemp' module, nor any reference to  it in the hwmon documentation.
> 

You might want to consider looking in the mainline kernel.

Guenter

RE: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Avri Altman]

> >> Can you add an explanation why this can't be added to the just-
> introduced
> >> 'drivetemp' driver in the hwmon subsystem, and why it make sense to
> have
> >> proprietary attributes for temperature and temperature limits ?


Guenter hi,
Yeah - I see your point. But here is the thing - 
UFS devices support only a subset of scsi commands.
It does not support ATA_16 nor SMART attributes.
Moreover, you can't read UFS attributes using any other scsi/ATA/SATA
Commands, nor it obey the ATA temperature sensing conventions.
So unless you want to totally break the newly born drivetemp - 
Better to leave ufs devices out of it.

Another option is to put a ufs module under hwmon.
Do you see why would that be more advantageous to using the thermal core?
Provided that you are not going to deprecate it (Intel's wifi card is still using it)...

As for adding those attributes to ufs-sysfs - 
this is just a supplementary as all other attributes are exposed this way.

Thanks,
Avri 

Re: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Guenter Roeck]

On Mon, Feb 03, 2020 at 09:29:57PM +0000, Avri Altman wrote:
> > >> Can you add an explanation why this can't be added to the just-
> > introduced
> > >> 'drivetemp' driver in the hwmon subsystem, and why it make sense to
> > have
> > >> proprietary attributes for temperature and temperature limits ?
> 
> 
> Guenter hi,
> Yeah - I see your point. But here is the thing - 
> UFS devices support only a subset of scsi commands.
> It does not support ATA_16 nor SMART attributes.
> Moreover, you can't read UFS attributes using any other scsi/ATA/SATA
> Commands, nor it obey the ATA temperature sensing conventions.
> So unless you want to totally break the newly born drivetemp - 
> Better to leave ufs devices out of it.
> 

drivetemp is written with extensibility in mind. For example, Martin has a
prototype enhancement which supports SCSI drive temperature sensors. 
As long as a device can be identified as ufs device, and as long as there
is a means to pass-through commands, adding a new type would be easy.

> Another option is to put a ufs module under hwmon.
> Do you see why would that be more advantageous to using the thermal core?
> Provided that you are not going to deprecate it (Intel's wifi card is still using it)...
> 

Deprecate what, and what does this discussion have to do with Intel's wifi
card ?

Either case, the hardware monitoring subsystem provides standard attributes,
and it provides a bridge to the thermal subsystem. The question should be
why _not_ to use the hwmon subsystem, and this question should be answered
as part of this patch series.

Guenter

RE: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Avri Altman]

 
> On Mon, Feb 03, 2020 at 09:29:57PM +0000, Avri Altman wrote:
> > > >> Can you add an explanation why this can't be added to the just-
> > > introduced
> > > >> 'drivetemp' driver in the hwmon subsystem, and why it make sense to
> > > have
> > > >> proprietary attributes for temperature and temperature limits ?
> >
> >
> > Guenter hi,
> > Yeah - I see your point. But here is the thing -
> > UFS devices support only a subset of scsi commands.
> > It does not support ATA_16 nor SMART attributes.
> > Moreover, you can't read UFS attributes using any other scsi/ATA/SATA
> > Commands, nor it obey the ATA temperature sensing conventions.
> > So unless you want to totally break the newly born drivetemp -
> > Better to leave ufs devices out of it.
> >
> 
> drivetemp is written with extensibility in mind. For example, Martin has a
> prototype enhancement which supports SCSI drive temperature sensors.
> As long as a device can be identified as ufs device, and as long as there
The ufs device does not identifies as such, e.g. by INQUIRY or other.

> is a means to pass-through commands, adding a new type would be easy.
I am unaware of any such option.
Device management commands are privet to the ufs driver.

Thanks,
Avri

RE: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Avi Shchislowski]

As it become evident that the hwmon is not a viable option to implement ufs thermal notification, I would appreciate some concrete comments of this series.

Thanks,
Avi

> 
> > On Mon, Feb 03, 2020 at 09:29:57PM +0000, Avri Altman wrote:
> > > > >> Can you add an explanation why this can't be added to the just-
> > > > introduced
> > > > >> 'drivetemp' driver in the hwmon subsystem, and why it make
> > > > >> sense to
> > > > have
> > > > >> proprietary attributes for temperature and temperature limits ?
> > >
> > >
> > > Guenter hi,
> > > Yeah - I see your point. But here is the thing - UFS devices support
> > > only a subset of scsi commands.
> > > It does not support ATA_16 nor SMART attributes.
> > > Moreover, you can't read UFS attributes using any other
> > > scsi/ATA/SATA Commands, nor it obey the ATA temperature sensing
> conventions.
> > > So unless you want to totally break the newly born drivetemp -
> > > Better to leave ufs devices out of it.
> > >
> >
> > drivetemp is written with extensibility in mind. For example, Martin
> > has a prototype enhancement which supports SCSI drive temperature
> sensors.
> > As long as a device can be identified as ufs device, and as long as
> > there
> The ufs device does not identifies as such, e.g. by INQUIRY or other.
> 
> > is a means to pass-through commands, adding a new type would be easy.
> I am unaware of any such option.
> Device management commands are privet to the ufs driver.
> 
> Thanks,
> Avri

Re: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Julian Calaby]

Hi Avi,

On Thu, Feb 6, 2020 at 9:48 PM Avi Shchislowski
<Avi.Shchislowski@wdc.com> wrote:
>
> As it become evident that the hwmon is not a viable option to implement ufs thermal notification, I would appreciate some concrete comments of this series.

That isn't my reading of this thread.

You have two options:
1. extend drivetemp if that makes sense for this particular application.
2. follow the model of other devices that happen to have a built-in
temperature sensor and expose the hwmon compatible attributes as a
subdevice

It appears that option 1 isn't viable, so what about option 2?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

RE: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Avri Altman]

 
> 
> Hi Avi,
> 
> On Thu, Feb 6, 2020 at 9:48 PM Avi Shchislowski
> <Avi.Shchislowski@wdc.com> wrote:
> >
> > As it become evident that the hwmon is not a viable option to implement
> ufs thermal notification, I would appreciate some concrete comments of this
> series.
> 
> That isn't my reading of this thread.
> 
> You have two options:
> 1. extend drivetemp if that makes sense for this particular application.
> 2. follow the model of other devices that happen to have a built-in
> temperature sensor and expose the hwmon compatible attributes as a
> subdevice
> 
> It appears that option 1 isn't viable, so what about option 2?
This will require to export the ufs device management commands,
Which is privet to the ufs driver.

This is not a viable option as well, because it will allow unrestricted access
(Including format etc.) to the storage device.

Sorry for not making it clearer before.

Thanks,
Avri

> 
> Thanks,
> 
> --
> Julian Calaby
> 
> Email: julian.calaby@gmail.com
> Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Julian Calaby]

Hi Avri,

On Thu, Feb 6, 2020 at 11:08 PM Avri Altman <Avri.Altman@wdc.com> wrote:
>
>
> >
> > Hi Avi,
> >
> > On Thu, Feb 6, 2020 at 9:48 PM Avi Shchislowski
> > <Avi.Shchislowski@wdc.com> wrote:
> > >
> > > As it become evident that the hwmon is not a viable option to implement
> > ufs thermal notification, I would appreciate some concrete comments of this
> > series.
> >
> > That isn't my reading of this thread.
> >
> > You have two options:
> > 1. extend drivetemp if that makes sense for this particular application.
> > 2. follow the model of other devices that happen to have a built-in
> > temperature sensor and expose the hwmon compatible attributes as a
> > subdevice
> >
> > It appears that option 1 isn't viable, so what about option 2?
> This will require to export the ufs device management commands,
> Which is privet to the ufs driver.
>
> This is not a viable option as well, because it will allow unrestricted access
> (Including format etc.) to the storage device.
>
> Sorry for not making it clearer before.

I should have clarified further: I meant having the UFS device
register a HWMON driver using this API:
https://www.kernel.org/doc/html/latest/hwmon/hwmon-kernel-api.html

*Not* writing a separate HWMON driver that uses some private interface.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

RE: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Avri Altman]

> 
> Hi Avri,
> 
> On Thu, Feb 6, 2020 at 11:08 PM Avri Altman <Avri.Altman@wdc.com>
> wrote:
> >
> >
> > >
> > > Hi Avi,
> > >
> > > On Thu, Feb 6, 2020 at 9:48 PM Avi Shchislowski
> > > <Avi.Shchislowski@wdc.com> wrote:
> > > >
> > > > As it become evident that the hwmon is not a viable option to
> implement
> > > ufs thermal notification, I would appreciate some concrete comments of
> this
> > > series.
> > >
> > > That isn't my reading of this thread.
> > >
> > > You have two options:
> > > 1. extend drivetemp if that makes sense for this particular application.
> > > 2. follow the model of other devices that happen to have a built-in
> > > temperature sensor and expose the hwmon compatible attributes as a
> > > subdevice
> > >
> > > It appears that option 1 isn't viable, so what about option 2?
> > This will require to export the ufs device management commands,
> > Which is privet to the ufs driver.
> >
> > This is not a viable option as well, because it will allow unrestricted access
> > (Including format etc.) to the storage device.
> >
> > Sorry for not making it clearer before.
> 
> I should have clarified further: I meant having the UFS device
> register a HWMON driver using this API:
> https://www.kernel.org/doc/html/latest/hwmon/hwmon-kernel-api.html
> 
> *Not* writing a separate HWMON driver that uses some private interface.
Ok.
Just one last question:
The ufs spec requires to be able to react upon an exception event from the device.
The thermal core provides an api in the form of thermal_notify_framework().
What would be the hwmon equivalent for that?

Thanks,
Avri

Re: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Julian Calaby]

Hi Avri,

On Fri, Feb 7, 2020 at 12:41 AM Avri Altman <Avri.Altman@wdc.com> wrote:
>
> >
> > Hi Avri,
> >
> > On Thu, Feb 6, 2020 at 11:08 PM Avri Altman <Avri.Altman@wdc.com>
> > wrote:
> > >
> > >
> > > >
> > > > Hi Avi,
> > > >
> > > > On Thu, Feb 6, 2020 at 9:48 PM Avi Shchislowski
> > > > <Avi.Shchislowski@wdc.com> wrote:
> > > > >
> > > > > As it become evident that the hwmon is not a viable option to
> > implement
> > > > ufs thermal notification, I would appreciate some concrete comments of
> > this
> > > > series.
> > > >
> > > > That isn't my reading of this thread.
> > > >
> > > > You have two options:
> > > > 1. extend drivetemp if that makes sense for this particular application.
> > > > 2. follow the model of other devices that happen to have a built-in
> > > > temperature sensor and expose the hwmon compatible attributes as a
> > > > subdevice
> > > >
> > > > It appears that option 1 isn't viable, so what about option 2?
> > > This will require to export the ufs device management commands,
> > > Which is privet to the ufs driver.
> > >
> > > This is not a viable option as well, because it will allow unrestricted access
> > > (Including format etc.) to the storage device.
> > >
> > > Sorry for not making it clearer before.
> >
> > I should have clarified further: I meant having the UFS device
> > register a HWMON driver using this API:
> > https://www.kernel.org/doc/html/latest/hwmon/hwmon-kernel-api.html
> >
> > *Not* writing a separate HWMON driver that uses some private interface.
> Ok.
> Just one last question:
> The ufs spec requires to be able to react upon an exception event from the device.
> The thermal core provides an api in the form of thermal_notify_framework().
> What would be the hwmon equivalent for that?

My understanding is that HWMON is just a standardised way to report
hardware sensor data to userspace. There are "alarm" files that can be
used to report fault conditions, so any action taken would have to be
either managed by userspace or configured using thermal zones
configured in the hardware's devicetree.

thermal_notify_framework() is a way to notify the "other side" of a
thermal zone to do something to reduce the temperature of that zone.
E.g. spin up a fan or switch to a lower-power state to cool a CPU.
Looking at your code, you're only implementing the "sensor" side of
the thermal zone functionality, so your calls to
thermal_notify_framework() won't do anything.

Thanks,

--
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

RE: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Avri Altman]

Hi Julian,

> 
> 
> Hi Avri,
> 
> On Fri, Feb 7, 2020 at 12:41 AM Avri Altman <Avri.Altman@wdc.com> wrote:
> >
> > >
> > > Hi Avri,
> > >
> > > On Thu, Feb 6, 2020 at 11:08 PM Avri Altman <Avri.Altman@wdc.com>
> > > wrote:
> > > >
> > > >
> > > > >
> > > > > Hi Avi,
> > > > >
> > > > > On Thu, Feb 6, 2020 at 9:48 PM Avi Shchislowski
> > > > > <Avi.Shchislowski@wdc.com> wrote:
> > > > > >
> > > > > > As it become evident that the hwmon is not a viable option to
> > > implement
> > > > > ufs thermal notification, I would appreciate some concrete comments
> of
> > > this
> > > > > series.
> > > > >
> > > > > That isn't my reading of this thread.
> > > > >
> > > > > You have two options:
> > > > > 1. extend drivetemp if that makes sense for this particular application.
> > > > > 2. follow the model of other devices that happen to have a built-in
> > > > > temperature sensor and expose the hwmon compatible attributes as
> a
> > > > > subdevice
> > > > >
> > > > > It appears that option 1 isn't viable, so what about option 2?
> > > > This will require to export the ufs device management commands,
> > > > Which is privet to the ufs driver.
> > > >
> > > > This is not a viable option as well, because it will allow unrestricted
> access
> > > > (Including format etc.) to the storage device.
> > > >
> > > > Sorry for not making it clearer before.
> > >
> > > I should have clarified further: I meant having the UFS device
> > > register a HWMON driver using this API:
> > > https://www.kernel.org/doc/html/latest/hwmon/hwmon-kernel-
> api.html
> > >
> > > *Not* writing a separate HWMON driver that uses some private
> interface.
> > Ok.
> > Just one last question:
> > The ufs spec requires to be able to react upon an exception event from the
> device.
> > The thermal core provides an api in the form of
> thermal_notify_framework().
> > What would be the hwmon equivalent for that?
> 
> My understanding is that HWMON is just a standardised way to report
> hardware sensor data to userspace. There are "alarm" files that can be
> used to report fault conditions, so any action taken would have to be
> either managed by userspace or configured using thermal zones
> configured in the hardware's devicetree.
Those "alarms" are  implemented as part of the modules under drivers/hwmon/ isn't it?
We already established that this is not an option for the ufs driver.

> 
> thermal_notify_framework() is a way to notify the "other side" of a
> thermal zone to do something to reduce the temperature of that zone.
> E.g. spin up a fan or switch to a lower-power state to cool a CPU.
> Looking at your code, you're only implementing the "sensor" side of
> the thermal zone functionality, so your calls to
> thermal_notify_framework() won't do anything.
Right.  The thermal core allows to react to such notifications,
Provided that the thermal zone device has a governor defined,
And/or notify ops etc.

Should the current patches implement those callbacks or not,
Can be discussed during their review process.
But the important thing is that the thermal core support it in an intuitive and simple way,
While the hwmon doesn't.

We are indifference with respect of which subsystem to use.
The thermal core was our first choice because we bumped into it,
Looking for a way to raise thermal exceptions.
It provides the functionality we need, and other devices uses it,
Why the insistence not to use it?


Thanks,
Avri
 
> 
> Thanks,
> 
> --
> Julian Calaby
> 
> Email: julian.calaby@gmail.com
> Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Guenter Roeck]

On Thu, Feb 06, 2020 at 07:32:12PM +0000, Avri Altman wrote:
> Hi Julian,
> 
> > 
> > 
> > Hi Avri,
> > 
> > On Fri, Feb 7, 2020 at 12:41 AM Avri Altman <Avri.Altman@wdc.com> wrote:
> > >
> > > >
> > > > Hi Avri,
> > > >
> > > > On Thu, Feb 6, 2020 at 11:08 PM Avri Altman <Avri.Altman@wdc.com>
> > > > wrote:
> > > > >
> > > > >
> > > > > >
> > > > > > Hi Avi,
> > > > > >
> > > > > > On Thu, Feb 6, 2020 at 9:48 PM Avi Shchislowski
> > > > > > <Avi.Shchislowski@wdc.com> wrote:
> > > > > > >
> > > > > > > As it become evident that the hwmon is not a viable option to
> > > > implement
> > > > > > ufs thermal notification, I would appreciate some concrete comments
> > of
> > > > this
> > > > > > series.
> > > > > >
> > > > > > That isn't my reading of this thread.
> > > > > >
> > > > > > You have two options:
> > > > > > 1. extend drivetemp if that makes sense for this particular application.
> > > > > > 2. follow the model of other devices that happen to have a built-in
> > > > > > temperature sensor and expose the hwmon compatible attributes as
> > a
> > > > > > subdevice
> > > > > >
> > > > > > It appears that option 1 isn't viable, so what about option 2?
> > > > > This will require to export the ufs device management commands,
> > > > > Which is privet to the ufs driver.
> > > > >
> > > > > This is not a viable option as well, because it will allow unrestricted
> > access
> > > > > (Including format etc.) to the storage device.
> > > > >
> > > > > Sorry for not making it clearer before.
> > > >
> > > > I should have clarified further: I meant having the UFS device
> > > > register a HWMON driver using this API:
> > > > https://www.kernel.org/doc/html/latest/hwmon/hwmon-kernel-
> > api.html
> > > >
> > > > *Not* writing a separate HWMON driver that uses some private
> > interface.
> > > Ok.
> > > Just one last question:
> > > The ufs spec requires to be able to react upon an exception event from the
> > device.
> > > The thermal core provides an api in the form of
> > thermal_notify_framework().
> > > What would be the hwmon equivalent for that?
> > 
> > My understanding is that HWMON is just a standardised way to report
> > hardware sensor data to userspace. There are "alarm" files that can be
> > used to report fault conditions, so any action taken would have to be
> > either managed by userspace or configured using thermal zones
> > configured in the hardware's devicetree.
> Those "alarms" are  implemented as part of the modules under drivers/hwmon/ isn't it?
> We already established that this is not an option for the ufs driver.

You have established nothing. What exactly is not an option ?
To create alarm attributes ? No one forces you to create any of those
if you don't want to.

> 
> > 
> > thermal_notify_framework() is a way to notify the "other side" of a
> > thermal zone to do something to reduce the temperature of that zone.
> > E.g. spin up a fan or switch to a lower-power state to cool a CPU.
> > Looking at your code, you're only implementing the "sensor" side of
> > the thermal zone functionality, so your calls to
> > thermal_notify_framework() won't do anything.
> Right.  The thermal core allows to react to such notifications,
> Provided that the thermal zone device has a governor defined,
> And/or notify ops etc.
> 
> Should the current patches implement those callbacks or not,
> Can be discussed during their review process.
> But the important thing is that the thermal core support it in an intuitive and simple way,
> While the hwmon doesn't.
> 
> We are indifference with respect of which subsystem to use.

Not really. Quite the opposite; you are quite obviously heavily
opposed to a hwmon driver.

> The thermal core was our first choice because we bumped into it,
> Looking for a way to raise thermal exceptions.
> It provides the functionality we need, and other devices uses it,
> Why the insistence not to use it?
> 

As mentioned before, the hwmon subsystem lets you create a bridge
to the thermal subsystem, it creates standard attributes to report
temperatures instead of the private ones your patch provides,
and it would result in simpler code.

Why the insistence to _not_ use the hwmon subsystem ?

Guenter

RE: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Avri Altman]

> 
> On Thu, Feb 06, 2020 at 07:32:12PM +0000, Avri Altman wrote:
> > Hi Julian,
> >
> > >
> > >
> > > Hi Avri,
> > >
> > > On Fri, Feb 7, 2020 at 12:41 AM Avri Altman <Avri.Altman@wdc.com>
> wrote:
> > > >
> > > > >
> > > > > Hi Avri,
> > > > >
> > > > > On Thu, Feb 6, 2020 at 11:08 PM Avri Altman
> <Avri.Altman@wdc.com>
> > > > > wrote:
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Hi Avi,
> > > > > > >
> > > > > > > On Thu, Feb 6, 2020 at 9:48 PM Avi Shchislowski
> > > > > > > <Avi.Shchislowski@wdc.com> wrote:
> > > > > > > >
> > > > > > > > As it become evident that the hwmon is not a viable option to
> > > > > implement
> > > > > > > ufs thermal notification, I would appreciate some concrete
> comments
> > > of
> > > > > this
> > > > > > > series.
> > > > > > >
> > > > > > > That isn't my reading of this thread.
> > > > > > >
> > > > > > > You have two options:
> > > > > > > 1. extend drivetemp if that makes sense for this particular
> application.
> > > > > > > 2. follow the model of other devices that happen to have a built-in
> > > > > > > temperature sensor and expose the hwmon compatible attributes
> as
> > > a
> > > > > > > subdevice
> > > > > > >
> > > > > > > It appears that option 1 isn't viable, so what about option 2?
> > > > > > This will require to export the ufs device management commands,
> > > > > > Which is privet to the ufs driver.
> > > > > >
> > > > > > This is not a viable option as well, because it will allow unrestricted
> > > access
> > > > > > (Including format etc.) to the storage device.
> > > > > >
> > > > > > Sorry for not making it clearer before.
> > > > >
> > > > > I should have clarified further: I meant having the UFS device
> > > > > register a HWMON driver using this API:
> > > > > https://www.kernel.org/doc/html/latest/hwmon/hwmon-kernel-
> > > api.html
> > > > >
> > > > > *Not* writing a separate HWMON driver that uses some private
> > > interface.
> > > > Ok.
> > > > Just one last question:
> > > > The ufs spec requires to be able to react upon an exception event from
> the
> > > device.
> > > > The thermal core provides an api in the form of
> > > thermal_notify_framework().
> > > > What would be the hwmon equivalent for that?
> > >
> > > My understanding is that HWMON is just a standardised way to report
> > > hardware sensor data to userspace. There are "alarm" files that can be
> > > used to report fault conditions, so any action taken would have to be
> > > either managed by userspace or configured using thermal zones
> > > configured in the hardware's devicetree.
> > Those "alarms" are  implemented as part of the modules under
> drivers/hwmon/ isn't it?
> > We already established that this is not an option for the ufs driver.
> 
> You have established nothing. What exactly is not an option ?
> To create alarm attributes ? No one forces you to create any of those
> if you don't want to.
> 
> >
> > >
> > > thermal_notify_framework() is a way to notify the "other side" of a
> > > thermal zone to do something to reduce the temperature of that zone.
> > > E.g. spin up a fan or switch to a lower-power state to cool a CPU.
> > > Looking at your code, you're only implementing the "sensor" side of
> > > the thermal zone functionality, so your calls to
> > > thermal_notify_framework() won't do anything.
> > Right.  The thermal core allows to react to such notifications,
> > Provided that the thermal zone device has a governor defined,
> > And/or notify ops etc.
> >
> > Should the current patches implement those callbacks or not,
> > Can be discussed during their review process.
> > But the important thing is that the thermal core support it in an intuitive
> and simple way,
> > While the hwmon doesn't.
> >
> > We are indifference with respect of which subsystem to use.
> 
> Not really. Quite the opposite; you are quite obviously heavily
> opposed to a hwmon driver.
> 
> > The thermal core was our first choice because we bumped into it,
> > Looking for a way to raise thermal exceptions.
> > It provides the functionality we need, and other devices uses it,
> > Why the insistence not to use it?
> >
> 
> As mentioned before, the hwmon subsystem lets you create a bridge
> to the thermal subsystem, it creates standard attributes to report
> temperatures instead of the private ones your patch provides,
> and it would result in simpler code.
> 
> Why the insistence to _not_ use the hwmon subsystem ?
I am not. Please, guide me through it - 
We'll register a hwmon device and implement its read ops.
Since read is confined to the ufs driver, we'll still need to have the ufs-thermal module that we've created.
Next we need to respond to a thermal exception event raised by the device - 
We are expected to pass such notification onward, for whatever governor/mind to  take an applicable action,
Should such mind exists.  How do I do that?

> 
> Guenter

Re: [PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Julian Calaby]

Hi Avri,

On Fri, Feb 7, 2020 at 6:32 AM Avri Altman <Avri.Altman@wdc.com> wrote:
>
> Hi Julian,
>
> >
> >
> > Hi Avri,
> >
> > On Fri, Feb 7, 2020 at 12:41 AM Avri Altman <Avri.Altman@wdc.com> wrote:
> > >
> > > >
> > > > Hi Avri,
> > > >
> > > > On Thu, Feb 6, 2020 at 11:08 PM Avri Altman <Avri.Altman@wdc.com>
> > > > wrote:
> > > > >
> > > > >
> > > > > >
> > > > > > Hi Avi,
> > > > > >
> > > > > > On Thu, Feb 6, 2020 at 9:48 PM Avi Shchislowski
> > > > > > <Avi.Shchislowski@wdc.com> wrote:
> > > > > > >
> > > > > > > As it become evident that the hwmon is not a viable option to
> > > > implement
> > > > > > ufs thermal notification, I would appreciate some concrete comments
> > of
> > > > this
> > > > > > series.
> > > > > >
> > > > > > That isn't my reading of this thread.
> > > > > >
> > > > > > You have two options:
> > > > > > 1. extend drivetemp if that makes sense for this particular application.
> > > > > > 2. follow the model of other devices that happen to have a built-in
> > > > > > temperature sensor and expose the hwmon compatible attributes as
> > a
> > > > > > subdevice
> > > > > >
> > > > > > It appears that option 1 isn't viable, so what about option 2?
> > > > > This will require to export the ufs device management commands,
> > > > > Which is privet to the ufs driver.
> > > > >
> > > > > This is not a viable option as well, because it will allow unrestricted
> > access
> > > > > (Including format etc.) to the storage device.
> > > > >
> > > > > Sorry for not making it clearer before.
> > > >
> > > > I should have clarified further: I meant having the UFS device
> > > > register a HWMON driver using this API:
> > > > https://www.kernel.org/doc/html/latest/hwmon/hwmon-kernel-
> > api.html
> > > >
> > > > *Not* writing a separate HWMON driver that uses some private
> > interface.
> > > Ok.
> > > Just one last question:
> > > The ufs spec requires to be able to react upon an exception event from the
> > device.
> > > The thermal core provides an api in the form of
> > thermal_notify_framework().
> > > What would be the hwmon equivalent for that?
> >
> > My understanding is that HWMON is just a standardised way to report
> > hardware sensor data to userspace. There are "alarm" files that can be
> > used to report fault conditions, so any action taken would have to be
> > either managed by userspace or configured using thermal zones
> > configured in the hardware's devicetree.
> Those "alarms" are  implemented as part of the modules under drivers/hwmon/ isn't it?
> We already established that this is not an option for the ufs driver.

The HWMON API I pointed you to is a way for _any_ driver to implement
the necessary bits and pieces to report temperatures, alarms, etc.

It is _not_ restricted to modules under drivers/hwmon/ - you can
implement all parts of the HWMON interface from any driver anywhere.

Which means that all that is required to report alarms is to simply
implement support for that particular type of file.

> > thermal_notify_framework() is a way to notify the "other side" of a
> > thermal zone to do something to reduce the temperature of that zone.
> > E.g. spin up a fan or switch to a lower-power state to cool a CPU.
> > Looking at your code, you're only implementing the "sensor" side of
> > the thermal zone functionality, so your calls to
> > thermal_notify_framework() won't do anything.
> Right.  The thermal core allows to react to such notifications,
> Provided that the thermal zone device has a governor defined,
> And/or notify ops etc.

Yes, but you don't define a cooling device, so there's nothing to
react to those notifications.

> Should the current patches implement those callbacks or not,
> Can be discussed during their review process.
> But the important thing is that the thermal core support it in an intuitive and simple way,
> While the hwmon doesn't.
>
> We are indifference with respect of which subsystem to use.
> The thermal core was our first choice because we bumped into it,
> Looking for a way to raise thermal exceptions.
> It provides the functionality we need, and other devices uses it,
> Why the insistence not to use it?

Other devices using the thermal zone subsystem implement both "sides"
of the zone: a sensor that monitors the device and cooling device(s)
which should be able to react to that. E.g. lowering the clock speed
of a CPU, slowing the transmission speed of a WiFi card, etc.

Your implementation doesn't do that.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Avi Shchislowski]

UFS3.0 allows using the ufs device as a temperature sensor. The
purpose of this feature is to provide notification to the host of the
UFS device case temperature. It allows reading of a rough estimate
(+-10 degrees centigrade) of the current case temperature, And
setting a lower and upper temperature bounds, in which the device
will trigger an applicable exception event.

We added the capability of responding to such notifications, while
notifying the kernel's thermal core, which further exposes the thermal
zone attributes to user space. UFS temperature attributes are all
read-only, so only thermal read ops (.get_xxx) can be implemented.

Avi Shchislowski (5):
  scsi: ufs: Add ufs thermal support
  scsi: ufs: export ufshcd_enable_ee
  scsi: ufs: enable thermal exception event
  scsi: ufs-thermal: implement thermal file ops
  scsi: ufs: temperature atrributes add to ufs_sysfs

 drivers/scsi/ufs/Kconfig       |  11 ++
 drivers/scsi/ufs/Makefile      |   1 +
 drivers/scsi/ufs/ufs-sysfs.c   |   6 +
 drivers/scsi/ufs/ufs-thermal.c | 247 +++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs-thermal.h |  25 +++++
 drivers/scsi/ufs/ufs.h         |  20 +++-
 drivers/scsi/ufs/ufshcd.c      |   9 +-
 drivers/scsi/ufs/ufshcd.h      |  12 ++
 8 files changed, 329 insertions(+), 2 deletions(-)
 create mode 100644 drivers/scsi/ufs/ufs-thermal.c
 create mode 100644 drivers/scsi/ufs/ufs-thermal.h

-- 
1.9.1

[PATCH 0/5] scsi: ufs: ufs device as a temperature sensor

[Avi Shchislowski]

From: Avi Shchislowski <avi.shchislowski@sandisk.com>

UFS3.0 allows using the ufs device as a temperature sensor. The
purpose of this feature is to provide notification to the host of the
UFS device case temperature. It allows reading of a rough estimate
(+-10 degrees centigrade) of the current case temperature, And
setting a lower and upper temperature bounds, in which the device
will trigger an applicable exception event.

We added the capability of responding to such notifications, while
notifying the kernel's thermal core, which further exposes the thermal
zone attributes to user space. UFS temperature attributes are all
read-only, so only thermal read ops (.get_xxx) can be implemented.

Avi Shchislowski (5):
  scsi: ufs: Add ufs thermal support
  scsi: ufs: export ufshcd_enable_ee
  scsi: ufs: enable thermal exception event
  scsi: ufs-thermal: implement thermal file ops
  scsi: ufs: temperature atrributes add to ufs_sysfs

 drivers/scsi/ufs/Kconfig       |  11 ++
 drivers/scsi/ufs/Makefile      |   1 +
 drivers/scsi/ufs/ufs-sysfs.c   |   6 +
 drivers/scsi/ufs/ufs-thermal.c | 247 +++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs-thermal.h |  25 +++++
 drivers/scsi/ufs/ufs.h         |  20 +++-
 drivers/scsi/ufs/ufshcd.c      |   9 +-
 drivers/scsi/ufs/ufshcd.h      |  12 ++
 8 files changed, 329 insertions(+), 2 deletions(-)
 create mode 100644 drivers/scsi/ufs/ufs-thermal.c
 create mode 100644 drivers/scsi/ufs/ufs-thermal.h

-- 
1.9.1