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

On 9/1/21 5:37 AM, Avri Altman wrote:

quoted

+What:

/sys/bus/platform/drivers/ufshcd/*/attributes/case_rough_temp

quoted

+Date:                September 2021
+Contact:     Avri Altman [off-list ref]
+Description: The device case rough temperature

(bDeviceCaseRoughTemperature

quoted

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

quoted

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

quoted

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)

quoted

              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.

quoted

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

quoted

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