| Message ID | 20210901123707.5014-4-avri.altman@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] scsi: ufs: Probe for temperature notification support | expand |
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.
> > 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.
> > 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.
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,
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(+)