[PATCH 0/3] Add temperature notification support

[PATCH 0/3] Add temperature notification support

[Avri Altman]

UFS3.0 allows using the ufs device as a temperature sensor. The purpose
of this optional 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.

A previous attempt [1] tried a comprehensive approach.  Still, it was
unsuccessful. Here is a more modest approach that introduces just the
bare minimum to support temperature notification.

Thanks,
Avri

[1] https://lore.kernel.org/lkml/1582450522-13256-1-git-send-email-avi.shchislowski@wdc.com/


Avri Altman (3):
  scsi: ufs: Probe for temperature notification support
  scsi: ufs: Add temperature notification exception handling
  scsi: ufs-sysfs: Add sysfs entries for temperature notification

 Documentation/ABI/testing/sysfs-driver-ufs | 38 +++++++++++++
 drivers/scsi/ufs/ufs-sysfs.c               | 63 +++++++++++++++++++++-
 drivers/scsi/ufs/ufs.h                     | 12 +++++
 drivers/scsi/ufs/ufshcd.c                  | 28 ++++++++++
 drivers/scsi/ufs/ufshcd.h                  | 29 ++++++++++
 5 files changed, 169 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/3] scsi: ufs: Probe for temperature notification support

[Avri Altman]

Probe the dExtendedUFSFeaturesSupport register for the device's
temperature notification support.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufs.h    |  7 +++++++
 drivers/scsi/ufs/ufshcd.c | 18 ++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h | 22 ++++++++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 8c6b38b1b142..dee897ef9631 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -338,6 +338,9 @@ enum {
 
 /* Possible values for dExtendedUFSFeaturesSupport */
 enum {
+	UFS_DEV_LOW_TEMP_NOTIF		= BIT(4),
+	UFS_DEV_HIGH_TEMP_NOTIF		= BIT(5),
+	UFS_DEV_EXT_TEMP_NOTIF		= BIT(6),
 	UFS_DEV_HPB_SUPPORT		= BIT(7),
 	UFS_DEV_WRITE_BOOSTER_SUP	= BIT(8),
 };
@@ -604,6 +607,10 @@ struct ufs_dev_info {
 
 	bool	b_rpm_dev_flush_capable;
 	u8	b_presrv_uspc_en;
+
+	/* temperature notification */
+	bool high_temp_notif;
+	bool low_temp_notif;
 };
 
 /*
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3841ab49f556..6ad51ae764c5 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7469,6 +7469,22 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
 	hba->caps &= ~UFSHCD_CAP_WB_EN;
 }
 
+static void ufshcd_temp_notif_probe(struct ufs_hba *hba, u8 *desc_buf)
+{
+	struct ufs_dev_info *dev_info = &hba->dev_info;
+	u32 ext_ufs_feature;
+
+	if (!(hba->caps & UFSHCD_CAP_TEMP_NOTIF) ||
+	    dev_info->wspecversion < 0x300)
+		return;
+
+	ext_ufs_feature = get_unaligned_be32(desc_buf +
+					DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
+
+	dev_info->low_temp_notif = ext_ufs_feature & UFS_DEV_LOW_TEMP_NOTIF;
+	dev_info->high_temp_notif = ext_ufs_feature & UFS_DEV_HIGH_TEMP_NOTIF;
+}
+
 void ufshcd_fixup_dev_quirks(struct ufs_hba *hba, struct ufs_dev_fix *fixups)
 {
 	struct ufs_dev_fix *f;
@@ -7564,6 +7580,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 
 	ufshcd_wb_probe(hba, desc_buf);
 
+	ufshcd_temp_notif_probe(hba, desc_buf);
+
 	/*
 	 * ufshcd_read_string_desc returns size of the string
 	 * reset the error value
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 52ea6f350b18..467affbaec80 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -653,6 +653,12 @@ enum ufshcd_caps {
 	 * in order to exit DeepSleep state.
 	 */
 	UFSHCD_CAP_DEEPSLEEP				= 1 << 10,
+
+	/*
+	 * This capability allows the host controller driver to use temperature
+	 * notification if it is supported by the UFS device.
+	 */
+	UFSHCD_CAP_TEMP_NOTIF				= 1 << 11,
 };
 
 struct ufs_hba_variant_params {
@@ -972,6 +978,22 @@ static inline bool ufshcd_is_user_access_allowed(struct ufs_hba *hba)
 	return !hba->shutting_down;
 }
 
+static inline bool ufshcd_is_high_temp_notif_allowed(struct ufs_hba *hba)
+{
+	return hba->dev_info.high_temp_notif;
+}
+
+static inline bool ufshcd_is_low_temp_notif_allowed(struct ufs_hba *hba)
+{
+	return hba->dev_info.low_temp_notif;
+}
+
+static inline bool ufshcd_is_temp_notif_allowed(struct ufs_hba *hba)
+{
+	return ufshcd_is_high_temp_notif_allowed(hba) ||
+	       ufshcd_is_high_temp_notif_allowed(hba);
+}
+
 #define ufshcd_writel(hba, val, reg)	\
 	writel((val), (hba)->mmio_base + (reg))
 #define ufshcd_readl(hba, reg)	\
-- 
2.17.1

[PATCH 2/3] scsi: ufs: Add temperature notification exception handling

[Avri Altman]

The device may notify the host of an extreme temperature by using the
exception event mechanism. The exception can be raised when the device’s
Tcase temperature is either too high or too low.

It is essentially up to the platform to decide what further actions need
to be taken. So add a designated vop for that.  Each chipset vendor can
decide if it wants to use the thermal subsystem, hw monitor, or some
Privet implementation.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufs.h    |  2 ++
 drivers/scsi/ufs/ufshcd.c | 18 ++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h |  7 +++++++
 3 files changed, 27 insertions(+)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index dee897ef9631..4f2a2fe0c84a 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -374,6 +374,8 @@ enum {
 	MASK_EE_PERFORMANCE_THROTTLING	= BIT(6),
 };
 
+#define MASK_EE_URGENT_TEMP (MASK_EE_TOO_HIGH_TEMP | MASK_EE_TOO_LOW_TEMP)
+
 /* Background operation status */
 enum bkops_status {
 	BKOPS_STATUS_NO_OP               = 0x0,
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 6ad51ae764c5..5f1fce21b655 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5642,6 +5642,11 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
 				__func__, err);
 }
 
+static void ufshcd_temp_exception_event_handler(struct ufs_hba *hba, u16 status)
+{
+	ufshcd_vops_temp_notify(hba, status);
+}
+
 static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn)
 {
 	u8 index;
@@ -5821,6 +5826,9 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 	if (status & hba->ee_drv_mask & MASK_EE_URGENT_BKOPS)
 		ufshcd_bkops_exception_event_handler(hba);
 
+	if (status & hba->ee_drv_mask & MASK_EE_URGENT_TEMP)
+		ufshcd_temp_exception_event_handler(hba, status);
+
 	ufs_debugfs_exception_event(hba, status);
 out:
 	ufshcd_scsi_unblock_requests(hba);
@@ -7473,6 +7481,7 @@ static void ufshcd_temp_notif_probe(struct ufs_hba *hba, u8 *desc_buf)
 {
 	struct ufs_dev_info *dev_info = &hba->dev_info;
 	u32 ext_ufs_feature;
+	u16 mask = 0;
 
 	if (!(hba->caps & UFSHCD_CAP_TEMP_NOTIF) ||
 	    dev_info->wspecversion < 0x300)
@@ -7483,6 +7492,15 @@ static void ufshcd_temp_notif_probe(struct ufs_hba *hba, u8 *desc_buf)
 
 	dev_info->low_temp_notif = ext_ufs_feature & UFS_DEV_LOW_TEMP_NOTIF;
 	dev_info->high_temp_notif = ext_ufs_feature & UFS_DEV_HIGH_TEMP_NOTIF;
+
+	if (dev_info->low_temp_notif)
+		mask |= MASK_EE_TOO_LOW_TEMP;
+
+	if (dev_info->high_temp_notif)
+		mask |= MASK_EE_TOO_HIGH_TEMP;
+
+	if (mask)
+		ufshcd_enable_ee(hba, mask);
 }
 
 void ufshcd_fixup_dev_quirks(struct ufs_hba *hba, struct ufs_dev_fix *fixups)
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 467affbaec80..98ac7e7c8ec3 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -356,6 +356,7 @@ struct ufs_hba_variant_ops {
 			       const union ufs_crypto_cfg_entry *cfg, int slot);
 	void	(*event_notify)(struct ufs_hba *hba,
 				enum ufs_event_type evt, void *data);
+	void	(*temp_notify)(struct ufs_hba *hba, u16 status);
 };
 
 /* clock gating state  */
@@ -1355,6 +1356,12 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
 		hba->vops->config_scaling_param(hba, profile, data);
 }
 
+static inline void ufshcd_vops_temp_notify(struct ufs_hba *hba, u16 status)
+{
+	if (hba->vops && hba->vops->temp_notify)
+		hba->vops->temp_notify(hba, status);
+}
+
 extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];
 
 /*
-- 
2.17.1

[PATCH 3/3] scsi: ufs-sysfs: Add sysfs entries for temperature notification

[Avri Altman]

The temperature readings are in degrees Celsius, and to allow negative
temperature, need to subtract 80 from the reported value. Make sure to
enforce the legal range of those values, and properly document it as
well.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 38 ++++++++++
 drivers/scsi/ufs/ufs-sysfs.c               | 86 ++++++++++++++++++++++
 drivers/scsi/ufs/ufs.h                     |  3 +
 3 files changed, 127 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index ec3a7149ced5..ff2294971e44 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1534,3 +1534,41 @@ Contact:	Avri Altman <avri.altman@wdc.com>
 Description:	In host control mode the host is the originator of map requests.
 		To avoid flooding the device with map requests, use a simple throttling
 		mechanism that limits the number of inflight map requests.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/attributes/case_rough_temp
+Date:		September 2021
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:	The device case rough temperature (bDeviceCaseRoughTemperature
+		attribute). It is termed "rough" due to the inherent inaccuracy
+		of the temperature sensor inside a semiconductor device,
+		e.g. +- 10 degrees centigrade error range.
+		allowable range is [-79..170].
+		The temperature readings are in decimal degrees Celsius.
+
+		Please note that the Tcase validity depends on the state of the
+		wExceptionEventControl attribute: it is up to the user to
+		verify that the applicable mask (TOO_HIGH_TEMP_EN, and / or
+		TOO_LOW_TEMP_EN) is set for the exception handling control.
+		This can be either done by ufs-bsg or ufs-debugfs.
+
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/attributes/high_temp_bound
+Date:		September 2021
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:	The high temperature (bDeviceTooHighTempBoundary attribute)
+		from which TOO_HIGH_TEMP in wExceptionEventStatus is turned on.
+		The temperature readings are in decimal degrees Celsius.
+		allowable range is [20..170].
+
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/attributes/low_temp_bound
+Date:		September 2021
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:	The low temperature (bDeviceTooLowTempBoundary attribute)
+		from which TOO_LOW_TEMP in wExceptionEventStatus is turned on.
+		The temperature readings are in decimal degrees Celsius.
+		allowable range is [-79..0].
+
+		The file is read only.
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 5c405ff7b6ea..a9abe33c40e4 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -1047,6 +1047,86 @@ static inline bool ufshcd_is_wb_attrs(enum attr_idn idn)
 		idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE;
 }
 
+static inline bool ufshcd_is_temp_attrs(enum attr_idn idn)
+{
+	return idn >= QUERY_ATTR_IDN_CASE_ROUGH_TEMP &&
+	       idn <= QUERY_ATTR_IDN_LOW_TEMP_BOUND;
+}
+
+static bool ufshcd_case_temp_legal(struct ufs_hba *hba)
+{
+	u32 ee_mask;
+	int ret;
+
+	ufshcd_rpm_get_sync(hba);
+	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+				QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &ee_mask);
+	ufshcd_rpm_put_sync(hba);
+	if (ret)
+		return false;
+
+	return (ufshcd_is_high_temp_notif_allowed(hba) &&
+		(ee_mask & MASK_EE_TOO_HIGH_TEMP)) ||
+	       (ufshcd_is_low_temp_notif_allowed(hba) &&
+		(ee_mask & MASK_EE_TOO_HIGH_TEMP));
+}
+
+static bool ufshcd_temp_legal(struct ufs_hba *hba, enum attr_idn idn,
+			      u32 value)
+{
+	return (idn == QUERY_ATTR_IDN_CASE_ROUGH_TEMP && value >= 1 &&
+		value <= 250 && ufshcd_case_temp_legal(hba)) ||
+	       (idn == QUERY_ATTR_IDN_HIGH_TEMP_BOUND && value >= 100 &&
+		value <= 250) ||
+	       (idn == QUERY_ATTR_IDN_LOW_TEMP_BOUND && value >= 1 &&
+		value <= 80);
+}
+
+#define UFS_ATTRIBUTE_DEC(_name, _uname)				\
+static ssize_t _name##_show(struct device *dev,				\
+	struct device_attribute *attr, char *buf)			\
+{									\
+	struct ufs_hba *hba = dev_get_drvdata(dev);			\
+	u32 value;							\
+	int dec_value;							\
+	int ret;							\
+	u8 index = 0;							\
+									\
+	down(&hba->host_sem);						\
+	if (!ufshcd_is_user_access_allowed(hba)) {			\
+		up(&hba->host_sem);					\
+		return -EBUSY;						\
+	}								\
+	if (ufshcd_is_temp_attrs(QUERY_ATTR_IDN##_uname)) {		\
+		if (!ufshcd_is_temp_notif_allowed(hba)) {		\
+			up(&hba->host_sem);				\
+			return -EOPNOTSUPP;				\
+		}							\
+	}								\
+	ufshcd_rpm_get_sync(hba);					\
+	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,	\
+		QUERY_ATTR_IDN##_uname, index, 0, &value);		\
+	ufshcd_rpm_put_sync(hba);					\
+	if (ret) {							\
+		ret = -EINVAL;						\
+		goto out;						\
+	}								\
+	dec_value = value;						\
+	if (ufshcd_is_temp_attrs(QUERY_ATTR_IDN##_uname)) {		\
+		if (!ufshcd_temp_legal(hba, QUERY_ATTR_IDN##_uname,	\
+				       value)) {			\
+			ret = -EINVAL;					\
+			goto out;					\
+		}							\
+		dec_value -= 80;					\
+	}								\
+	ret = sysfs_emit(buf, "%d\n", dec_value);			\
+out:									\
+	up(&hba->host_sem);						\
+	return ret;							\
+}									\
+static DEVICE_ATTR_RO(_name)
+
 #define UFS_ATTRIBUTE(_name, _uname)					\
 static ssize_t _name##_show(struct device *dev,				\
 	struct device_attribute *attr, char *buf)			\
@@ -1100,6 +1180,9 @@ UFS_ATTRIBUTE(wb_avail_buf, _AVAIL_WB_BUFF_SIZE);
 UFS_ATTRIBUTE(wb_life_time_est, _WB_BUFF_LIFE_TIME_EST);
 UFS_ATTRIBUTE(wb_cur_buf, _CURR_WB_BUFF_SIZE);
 
+UFS_ATTRIBUTE_DEC(case_rough_temp, _CASE_ROUGH_TEMP);
+UFS_ATTRIBUTE_DEC(high_temp_bound, _HIGH_TEMP_BOUND);
+UFS_ATTRIBUTE_DEC(low_temp_bound, _LOW_TEMP_BOUND);
 
 static struct attribute *ufs_sysfs_attributes[] = {
 	&dev_attr_boot_lun_enabled.attr,
@@ -1119,6 +1202,9 @@ static struct attribute *ufs_sysfs_attributes[] = {
 	&dev_attr_ffu_status.attr,
 	&dev_attr_psa_state.attr,
 	&dev_attr_psa_data_size.attr,
+	&dev_attr_case_rough_temp.attr,
+	&dev_attr_high_temp_bound.attr,
+	&dev_attr_low_temp_bound.attr,
 	&dev_attr_wb_flush_status.attr,
 	&dev_attr_wb_avail_buf.attr,
 	&dev_attr_wb_life_time_est.attr,
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 4f2a2fe0c84a..03d08fc1bd68 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -152,6 +152,9 @@ enum attr_idn {
 	QUERY_ATTR_IDN_PSA_STATE		= 0x15,
 	QUERY_ATTR_IDN_PSA_DATA_SIZE		= 0x16,
 	QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME	= 0x17,
+	QUERY_ATTR_IDN_CASE_ROUGH_TEMP		= 0x18,
+	QUERY_ATTR_IDN_HIGH_TEMP_BOUND		= 0x19,
+	QUERY_ATTR_IDN_LOW_TEMP_BOUND		= 0x1A,
 	QUERY_ATTR_IDN_WB_FLUSH_STATUS	        = 0x1C,
 	QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE       = 0x1D,
 	QUERY_ATTR_IDN_WB_BUFF_LIFE_TIME_EST    = 0x1E,
-- 
2.17.1

Re: [PATCH 2/3] scsi: ufs: Add temperature notification exception handling

[Asutosh Das (asd)]

On 9/1/2021 5:37 AM, Avri Altman wrote:
> The device may notify the host of an extreme temperature by using the
> exception event mechanism. The exception can be raised when the device’s
> Tcase temperature is either too high or too low.
> 
> It is essentially up to the platform to decide what further actions need
> to be taken. So add a designated vop for that.  Each chipset vendor can
> decide if it wants to use the thermal subsystem, hw monitor, or some
> Privet implementation.
The sensors of thermal subsystem would essentially get the skin 
temperature and invoke the registered handlers to throttle, if possible.

I'm not sure how the vendor drivers can use thermal subsystem now if the 
ufs device doesn't inform the thermal subsystem about its increase in 
temperature.

Meaning, (FWIU):
  -> ufs device senses an increase in temperature
     -> informs thermal subsystem
       -> registered vendor driver cb() is invoked by thermal subsystem.

Please let me know if I'm missing something.

> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>   drivers/scsi/ufs/ufs.h    |  2 ++
>   drivers/scsi/ufs/ufshcd.c | 18 ++++++++++++++++++
>   drivers/scsi/ufs/ufshcd.h |  7 +++++++
>   3 files changed, 27 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index dee897ef9631..4f2a2fe0c84a 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -374,6 +374,8 @@ enum {
>   	MASK_EE_PERFORMANCE_THROTTLING	= BIT(6),
>   };
>   
> +#define MASK_EE_URGENT_TEMP (MASK_EE_TOO_HIGH_TEMP | MASK_EE_TOO_LOW_TEMP)
> +
>   /* Background operation status */
>   enum bkops_status {
>   	BKOPS_STATUS_NO_OP               = 0x0,
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 6ad51ae764c5..5f1fce21b655 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5642,6 +5642,11 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
>   				__func__, err);
>   }
>   
> +static void ufshcd_temp_exception_event_handler(struct ufs_hba *hba, u16 status)
> +{
> +	ufshcd_vops_temp_notify(hba, status);
> +}
> +
>   static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn)
>   {
>   	u8 index;
> @@ -5821,6 +5826,9 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
>   	if (status & hba->ee_drv_mask & MASK_EE_URGENT_BKOPS)
>   		ufshcd_bkops_exception_event_handler(hba);
>   
> +	if (status & hba->ee_drv_mask & MASK_EE_URGENT_TEMP)
> +		ufshcd_temp_exception_event_handler(hba, status);
> +
>   	ufs_debugfs_exception_event(hba, status);
>   out:
>   	ufshcd_scsi_unblock_requests(hba);
> @@ -7473,6 +7481,7 @@ static void ufshcd_temp_notif_probe(struct ufs_hba *hba, u8 *desc_buf)
>   {
>   	struct ufs_dev_info *dev_info = &hba->dev_info;
>   	u32 ext_ufs_feature;
> +	u16 mask = 0;
>   
>   	if (!(hba->caps & UFSHCD_CAP_TEMP_NOTIF) ||
>   	    dev_info->wspecversion < 0x300)
> @@ -7483,6 +7492,15 @@ static void ufshcd_temp_notif_probe(struct ufs_hba *hba, u8 *desc_buf)
>   
>   	dev_info->low_temp_notif = ext_ufs_feature & UFS_DEV_LOW_TEMP_NOTIF;
>   	dev_info->high_temp_notif = ext_ufs_feature & UFS_DEV_HIGH_TEMP_NOTIF;
> +
> +	if (dev_info->low_temp_notif)
> +		mask |= MASK_EE_TOO_LOW_TEMP;
> +
> +	if (dev_info->high_temp_notif)
> +		mask |= MASK_EE_TOO_HIGH_TEMP;
> +
> +	if (mask)
> +		ufshcd_enable_ee(hba, mask);
>   }
>   
>   void ufshcd_fixup_dev_quirks(struct ufs_hba *hba, struct ufs_dev_fix *fixups)
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 467affbaec80..98ac7e7c8ec3 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -356,6 +356,7 @@ struct ufs_hba_variant_ops {
>   			       const union ufs_crypto_cfg_entry *cfg, int slot);
>   	void	(*event_notify)(struct ufs_hba *hba,
>   				enum ufs_event_type evt, void *data);
> +	void	(*temp_notify)(struct ufs_hba *hba, u16 status);
>   };
>   
>   /* clock gating state  */
> @@ -1355,6 +1356,12 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
>   		hba->vops->config_scaling_param(hba, profile, data);
>   }
>   
> +static inline void ufshcd_vops_temp_notify(struct ufs_hba *hba, u16 status)
> +{
> +	if (hba->vops && hba->vops->temp_notify)
> +		hba->vops->temp_notify(hba, status);
> +}
> +
>   extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];
>   
>   /*
> 


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

Re: [PATCH 1/3] scsi: ufs: Probe for temperature notification support

[Bart Van Assche]

On 9/1/21 5:37 AM, Avri Altman wrote:
> +static inline bool ufshcd_is_high_temp_notif_allowed(struct ufs_hba *hba)
> +{
> +	return hba->dev_info.high_temp_notif;
> +}
> +
> +static inline bool ufshcd_is_low_temp_notif_allowed(struct ufs_hba *hba)
> +{
> +	return hba->dev_info.low_temp_notif;
> +}

Please do not introduce single line inline functions.

> +static inline bool ufshcd_is_temp_notif_allowed(struct ufs_hba *hba)
> +{
> +	return ufshcd_is_high_temp_notif_allowed(hba) ||
> +	       ufshcd_is_high_temp_notif_allowed(hba);
> +}

Since this function is not in any hot path (command processing), 
shouldn't it be moved into ufshcd.c?

Thanks,

Bart.

Re: [PATCH 2/3] scsi: ufs: Add temperature notification exception handling

[Bart Van Assche]

On 9/1/21 5:37 AM, Avri Altman wrote:
> It is essentially up to the platform to decide what further actions need
> to be taken. So add a designated vop for that.  Each chipset vendor can
> decide if it wants to use the thermal subsystem, hw monitor, or some
> Privet implementation.

Why to make chipset vendors define what to do in case of extreme 
temperatures? I'd prefer a single implementation in ufshcd.c instead of 
making each vendor come up with a different implementation.

> +	void	(*temp_notify)(struct ufs_hba *hba, u16 status);

Please do not add new vops without adding at least one implementation of 
that vop.

Thanks,

Bart.

Re: [PATCH 3/3] scsi: ufs-sysfs: Add sysfs entries for temperature notification

[Bart Van Assche]

On 9/1/21 5:37 AM, Avri Altman wrote:
> +What:		/sys/bus/platform/drivers/ufshcd/*/attributes/case_rough_temp
> +Date:		September 2021
> +Contact:	Avri Altman <avri.altman@wdc.com>
> +Description:	The device case rough temperature (bDeviceCaseRoughTemperature
> +		attribute). It is termed "rough" due to the inherent inaccuracy
> +		of the temperature sensor inside a semiconductor device,
> +		e.g. +- 10 degrees centigrade error range.

My understanding is that the word Celsius is more common than centigrade 
so please use Celsius instead of centigrade. See also 
https://www.brannan.co.uk/celsius-centigrade-and-fahrenheit/

> +		allowable range is [-79..170].
> +		The temperature readings are in decimal degrees Celsius.
> +
> +		Please note that the Tcase validity depends on the state of the
> +		wExceptionEventControl attribute: it is up to the user to
> +		verify that the applicable mask (TOO_HIGH_TEMP_EN, and / or
> +		TOO_LOW_TEMP_EN) is set for the exception handling control.
> +		This can be either done by ufs-bsg or ufs-debugfs.

Instead of making the user verify whether case_rough_temp is valid, 
please modify the kernel code such that case_rough_temp only reports a 
value if that value is valid. One possible approach is to make the show 
method return an error code if case_rough_temp is not valid. Another and 
probably better approach is to define a sysfs attribute group and to 
make case_rough_temp visible only if it is valid.

> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 5c405ff7b6ea..a9abe33c40e4 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -1047,6 +1047,86 @@ static inline bool ufshcd_is_wb_attrs(enum attr_idn idn)
>   		idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE;
>   }
>   
> +static inline bool ufshcd_is_temp_attrs(enum attr_idn idn)
> +{
> +	return idn >= QUERY_ATTR_IDN_CASE_ROUGH_TEMP &&
> +	       idn <= QUERY_ATTR_IDN_LOW_TEMP_BOUND;
> +}

Modern compilers are good at deciding when to inline a function so 
please leave out the 'inline' keyword from the above function.

> +static bool ufshcd_case_temp_legal(struct ufs_hba *hba)\

Please use another word than "legal" since the primary meaning of 
"legal" is "of or relating to law".

> +	ufshcd_rpm_get_sync(hba);
> +	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +				QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &ee_mask);
> +	ufshcd_rpm_put_sync(hba);

Are there any ufshcd_query_attr() calls that are not surrounded by 
ufshcd_rpm_{get,put}_sync()? If not, please move the 
ufshcd_rpm_{get,put}_sync() calls into ufshcd_query_attr().

Thanks,

Bart.

Re: [PATCH 2/3] scsi: ufs: Add temperature notification exception handling

[Asutosh Das (asd)]

On 9/1/2021 9:39 AM, Bart Van Assche wrote:
> On 9/1/21 5:37 AM, Avri Altman wrote:
>> It is essentially up to the platform to decide what further actions need
>> to be taken. So add a designated vop for that.  Each chipset vendor can
>> decide if it wants to use the thermal subsystem, hw monitor, or some
>> Privet implementation.
> 
> Why to make chipset vendors define what to do in case of extreme 
> temperatures? I'd prefer a single implementation in ufshcd.c instead of 
> making each vendor come up with a different implementation.
> 
I think it should be either i.e. if a vendor specific implementation is 
defined use that else use the generic implementation in ufshcd.
There may be a bunch of things that each vendor may need/want do 
depending upon use-case, I imagine.

>> +    void    (*temp_notify)(struct ufs_hba *hba, u16 status);
> 
> Please do not add new vops without adding at least one implementation of 
> that vop.
> 
> Thanks,
> 
> Bart.


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

RE: [PATCH 1/3] scsi: ufs: Probe for temperature notification support

[Avri Altman]

> 
> On 9/1/21 5:37 AM, Avri Altman wrote:
> > +static inline bool ufshcd_is_high_temp_notif_allowed(struct ufs_hba *hba)
> > +{
> > +     return hba->dev_info.high_temp_notif;
> > +}
> > +
> > +static inline bool ufshcd_is_low_temp_notif_allowed(struct ufs_hba *hba)
> > +{
> > +     return hba->dev_info.low_temp_notif;
> > +}
> 
> Please do not introduce single line inline functions.
Done.

> 
> > +static inline bool ufshcd_is_temp_notif_allowed(struct ufs_hba *hba)
> > +{
> > +     return ufshcd_is_high_temp_notif_allowed(hba) ||
> > +            ufshcd_is_high_temp_notif_allowed(hba);
> > +}
> 
> Since this function is not in any hot path (command processing),
> shouldn't it be moved into ufshcd.c?
Done.

> 
> Thanks,
> 
> Bart.

RE: [PATCH 2/3] scsi: ufs: Add temperature notification exception handling

[Avri Altman]

> On 9/1/2021 9:39 AM, Bart Van Assche wrote:
> > On 9/1/21 5:37 AM, Avri Altman wrote:
> >> It is essentially up to the platform to decide what further actions need
> >> to be taken. So add a designated vop for that.  Each chipset vendor can
> >> decide if it wants to use the thermal subsystem, hw monitor, or some
> >> Privet implementation.
> >
> > Why to make chipset vendors define what to do in case of extreme
> > temperatures? I'd prefer a single implementation in ufshcd.c instead of
> > making each vendor come up with a different implementation.
The storage device is merely acting as a temperature sensor.
This info, jointly with other temperature sensors of the system,
Should be used elsewhere in a much broader scope - probably by Android.
Either way, ufshcd is hardly the place for those decisions.

> >
> I think it should be either i.e. if a vendor specific implementation is
> defined use that else use the generic implementation in ufshcd.
> There may be a bunch of things that each vendor may need/want do
> depending upon use-case, I imagine.
I agree, and this is why I wanted to allow that that flexibility.
But I get Bart's point. I will register the sensor in some subsystem.
It should allow the required degrees of freedom.

Thanks,
Avri
> 
> >> +    void    (*temp_notify)(struct ufs_hba *hba, u16 status);
> >
> > Please do not add new vops without adding at least one implementation of
> > that vop.
> >
> > Thanks,
> >
> > Bart.
> 
> 
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> Linux Foundation Collaborative Project

RE: [PATCH 3/3] scsi: ufs-sysfs: Add sysfs entries for temperature notification

[Avri Altman]

> 
> On 9/1/21 5:37 AM, Avri Altman wrote:
> > +What:
> /sys/bus/platform/drivers/ufshcd/*/attributes/case_rough_temp
> > +Date:                September 2021
> > +Contact:     Avri Altman <avri.altman@wdc.com>
> > +Description: The device case rough temperature
> (bDeviceCaseRoughTemperature
> > +             attribute). It is termed "rough" due to the inherent inaccuracy
> > +             of the temperature sensor inside a semiconductor device,
> > +             e.g. +- 10 degrees centigrade error range.
> 
> My understanding is that the word Celsius is more common than centigrade
> so please use Celsius instead of centigrade. See also
> https://www.brannan.co.uk/celsius-centigrade-and-fahrenheit/
Done.

> 
> > +             allowable range is [-79..170].
> > +             The temperature readings are in decimal degrees Celsius.
> > +
> > +             Please note that the Tcase validity depends on the state of the
> > +             wExceptionEventControl attribute: it is up to the user to
> > +             verify that the applicable mask (TOO_HIGH_TEMP_EN, and / or
> > +             TOO_LOW_TEMP_EN) is set for the exception handling control.
> > +             This can be either done by ufs-bsg or ufs-debugfs.
> 
> Instead of making the user verify whether case_rough_temp is valid,
> please modify the kernel code such that case_rough_temp only reports a
> value if that value is valid. One possible approach is to make the show
> method return an error code if case_rough_temp is not valid.
But it does.
Just wanted to document that exception control is controlled from user space,
And avoid the eyebrow raises when getting invalid temperature reading.

>Another and
> probably better approach is to define a sysfs attribute group and to
> make case_rough_temp visible only if it is valid.
> 
> > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> > index 5c405ff7b6ea..a9abe33c40e4 100644
> > --- a/drivers/scsi/ufs/ufs-sysfs.c
> > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > @@ -1047,6 +1047,86 @@ static inline bool ufshcd_is_wb_attrs(enum
> attr_idn idn)
> >               idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE;
> >   }
> >
> > +static inline bool ufshcd_is_temp_attrs(enum attr_idn idn)
> > +{
> > +     return idn >= QUERY_ATTR_IDN_CASE_ROUGH_TEMP &&
> > +            idn <= QUERY_ATTR_IDN_LOW_TEMP_BOUND;
> > +}
> 
> Modern compilers are good at deciding when to inline a function so
> please leave out the 'inline' keyword from the above function.
Done.

> 
> > +static bool ufshcd_case_temp_legal(struct ufs_hba *hba)\
> 
> Please use another word than "legal" since the primary meaning of
> "legal" is "of or relating to law".
Done.

> 
> > +     ufshcd_rpm_get_sync(hba);
> > +     ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> > +                             QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &ee_mask);
> > +     ufshcd_rpm_put_sync(hba);
> 
> Are there any ufshcd_query_attr() calls that are not surrounded by
> ufshcd_rpm_{get,put}_sync()? If not, please move the
> ufshcd_rpm_{get,put}_sync() calls into ufshcd_query_attr().
Will check.

Thanks,
Avri
> 
> Thanks,
> 
> Bart.

RE: [PATCH 3/3] scsi: ufs-sysfs: Add sysfs entries for temperature notification

[Avri Altman]

> > Are there any ufshcd_query_attr() calls that are not surrounded by
> > ufshcd_rpm_{get,put}_sync()? If not, please move the
> > ufshcd_rpm_{get,put}_sync() calls into ufshcd_query_attr().
> Will check.
Apparently there are.  Adding, a @user_access  argument e.g. 
@user_access: Does ufshcd_rpm_{get,put}_sync need to be called
Doesn't really save anything, so lets leave it be for now.

Thanks,
Avri

> 
> Thanks,
> Avri
> >
> > Thanks,
> >
> > Bart.