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/[email protected]/
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
Probe the dExtendedUFSFeaturesSupport register for the device's
temperature notification support.
Signed-off-by: Avri Altman <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
+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 <[email protected]>
+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 <[email protected]>
+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
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.
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 <[email protected]>
> +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.
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 <[email protected]>
---
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
>
> 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 <[email protected]>
> > +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.
>
> 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.
> > 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.