> > +config SCSI_UFS_HWMON
> > + bool "UFS Temperature Notification"
> > + depends on SCSI_UFSHCD && HWMON
> > + help
> > + This provides support for UFS hardware monitoring. If enabled,
> > + a hardware monitoring device will be created for the UFS device.
> > +
> > + If unsure, say N.
> > +
>
> git complains about blank line at EOF.
Done.
>
> > diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
> > index c407da9b5171..966048875b50 100644
> > --- a/drivers/scsi/ufs/Makefile
> > +++ b/drivers/scsi/ufs/Makefile
> > @@ -10,6 +10,7 @@ ufshcd-core-$(CONFIG_SCSI_UFS_BSG) += ufs_bsg.o
> > ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO) += ufshcd-crypto.o
> > ufshcd-core-$(CONFIG_SCSI_UFS_HPB) += ufshpb.o
> > ufshcd-core-$(CONFIG_SCSI_UFS_FAULT_INJECTION) +=
> > ufs-fault-injection.o
> > +ufshcd-core-$(CONFIG_SCSI_UFS_HWMON) += ufs-hwmon.o
> >
> > obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o
> tc-dwc-g210.o
> > obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o
> > ufshcd-dwc.o tc-dwc-g210.o diff --git a/drivers/scsi/ufs/ufs-hwmon.c
> > b/drivers/scsi/ufs/ufs-hwmon.c new file mode 100644 index
> > 000000000000..a50e83f645f4
> > --- /dev/null
> > +++ b/drivers/scsi/ufs/ufs-hwmon.c
> > @@ -0,0 +1,179 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * UFS hardware monitoring support
> > + * Copyright (c) 2021, Western Digital Corporation */
> > +
> > +#include <linux/hwmon.h>
> > +
> > +#include "ufshcd.h"
> > +
> > +struct ufs_hwmon_data {
> > + struct ufs_hba *hba;
> > + u8 mask;
> > +};
> > +
> > +static bool ufs_temp_enabled(struct ufs_hba *hba, u8 mask) {
> > + u32 ee_mask;
> > +
> > + if (ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> > + QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &ee_mask))
> > + return false;
> > +
> > + return (mask & ee_mask & MASK_EE_TOO_HIGH_TEMP) ||
> > + (mask & ee_mask & MASK_EE_TOO_LOW_TEMP); }
> > +
> > +static bool ufs_temp_valid(struct ufs_hba *hba, u8 mask,
> > + enum attr_idn idn, u32 value) {
> > + return (idn == QUERY_ATTR_IDN_CASE_ROUGH_TEMP && value >= 1
> &&
> > + value <= 250 && ufs_temp_enabled(hba, mask)) ||
> > + (idn == QUERY_ATTR_IDN_HIGH_TEMP_BOUND && value >= 100 &&
> > + value <= 250) ||
> > + (idn == QUERY_ATTR_IDN_LOW_TEMP_BOUND && value >= 1 &&
> > + value <= 80);
> > +}
> > +
> The value ranges checed above suggest that the temperature is reported in
> degrees C (or maybe degrees C with an offset).
Yes. No offset.
>The hwmon API expects
> temperatures to be reported in milli-degrees C, and I don't see a conversion in
> the actual read functions. What does the "sensors" command report ?
I missed that (Although it is well documented) - sorry about that.
I wasn't aware of the sensors command. I don't have it in my arm64 android platform image (galaxy s21).
Will try to get it.
I was reading the temperature using hwmon sysfs entries, which indicate the correct temperature.
e.g
t2s:/ # ls -la /sys/class/hwmon/
total 0
drwxr-xr-x 2 root root 0 2020-12-20 10:16 .
drwxr-xr-x 104 root root 0 2020-12-19 19:05 ..
lrwxrwxrwx 1 root root 0 2020-12-20 10:16 hwmon0 -> ../../devices/platform/13100000.ufs/hwmon/hwmon0
.....
t2s:/ # cat /sys/class/hwmon/hwmon0/temp1_input
25
Will fix it. Thanks.
>
> > +static int ufs_get_temp(struct ufs_hba *hba, u8 mask, enum attr_idn
> > +idn) {
> > + u32 value;
> > +
> > + if (ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR, idn, 0, 0,
> > + &value))
>
> checkpatch states that alignment is off, and I am quite sure this fits into one
> line anyway (with the 100-column limit). There are more instances with bad
> alignment according to checkpatch.
I wasn't aware that the Linux Kernel deprecates the 80 Character Line Coding Style.
Will try to make it full 100-characters lines.
I didn't get any alignment complaints from checkpatch.
>
> Also, ufshcd_query_attr() returns a valid Linux error code. That should be
> returned to the caller and not be replaced. More on that below.
>
> > + return 0;
> > +
> > + if (ufs_temp_valid(hba, mask, idn, value))
> > + return value - 80;
> > +
>
> This again suggests that the temperature is not milli-degrees C.
>
> Is there reason to believe that this validation is necessary ?
> Note that this reports an "error" if the returned temperature value happens to
> have a value of 80. Again, more on that below.
Data->mask holds the temperature related bits in the ufs features register: TOO_LOW_TEMPERATURE and TOO_HIGH_TEMPERATURE.
This is set for the device by the flash vendor and can't be changed by the OEMs.
If the device doesn't support any of that, then hwmon_probe is not even called, see ufshcd_temp_notif_probe.
So data->mask is not 0, and never changes.
When the device returns a 0 temperature value, it means that it is not valid.
The spec say about the Device’s rough package case surface temperature:
"
This value shall be valid when (TOO_HIGH_TEMPERATURE is supported and TOO_HIGH_TEMP_EN is enabled) or
( TOO_LOW_TEMPERATURE is supported and TOO_LOW_TEMP_EN is enabled ).
0 : Unknown Temperature , 1~250 : ( this value – 80 ) degrees in Celsius. ( i.e., -79 ºC ~ 170 ºC )
Others: Reserved
"
data->mask is not 0, but the temperature exception bits: TOO_HIGH_TEMP_EN and TOO_LOW_TEMP_EN are of type read/volatile,
Meaning it can be written many times, e.g. by debugfs or ufs-utils.
To sum up:
- yes, checking the temperature against the spec boundaries is useless.
The device will return 0 if it is not valid.
ufs_temp_valid() can be removed, and just need to check that the temperature is not 0.
- The return value of querry_attr is of less interest.
if it failed or temp == 0, then the temperature is invalid and the proper return value should be -EINVAL.
>
> > + return 0;
> > +}
> > +
> > +static int ufs_hwmon_read(struct device *dev, enum hwmon_sensor_types
> type,
> > + u32 attr, int channel, long *val) {
> > + struct ufs_hwmon_data *data = dev_get_drvdata(dev);
> > + struct ufs_hba *hba = data->hba;
> > + u8 mask = data->mask;
> > + int err = 0;
> > + bool get_temp = true;
> > +
> > + if (type != hwmon_temp)
> > + return 0;
> > +
> > + down(&hba->host_sem);
> > +
> > + if (!ufshcd_is_user_access_allowed(hba)) {
> > + up(&hba->host_sem);
> > + return -EBUSY;
> > + }
> > +
> > + ufshcd_rpm_get_sync(hba);
> > +
> > + switch (attr) {
> > + case hwmon_temp_enable:
> > + *val = ufs_temp_enabled(hba, mask);
> > + get_temp = false;
> > +
>
> This seems to be read-only, and the mask only affects the limit registers as far
> as I con see. If so, this is wrong: The mask should be used to enable or hide the
> limit attributes as needed. If the mask is 0, and if this means that the current
> temperature is not reported either, the driver should not instantiate at all.
>
> The "enable" attribute only makes sense if it can be used to actually enable or
> disable a specific sensor, and is not tied to limit attributes but to the actual
> sensor values.
See explanation above.
Will make it writable as well.
>
> > + break;
> > + case hwmon_temp_max_alarm:
> > + *val = ufs_get_temp(hba, mask,
> > + QUERY_ATTR_IDN_HIGH_TEMP_BOUND);
> > +
> > + break;
> > + case hwmon_temp_min_alarm:
> > + *val = ufs_get_temp(hba, mask,
> > + QUERY_ATTR_IDN_LOW_TEMP_BOUND);
> > +
> > + break;
> > + case hwmon_temp_input:
> > + *val = ufs_get_temp(hba, mask,
> > + QUERY_ATTR_IDN_CASE_ROUGH_TEMP);
> > +
> If an enable attribute exists and is 0 (disabled), this should return -ENODATA.
> In this case, that would imply that the driver should not be instantiated in the
> first place since it has nothing to report.
See explanation above.
Will fix it so the error value will make more sense.
>
> > + break;
> > + default:
> > + err = -EOPNOTSUPP;
> > +
> > + break;
> > + }
> > +
> > + ufshcd_rpm_put_sync(hba);
> > +
> > + up(&hba->host_sem);
> > +
> > + if (get_temp && !err && *val == 0)
> > + err = -EINVAL;
> > +
> That is an odd way of detection errors. If it was in the hwmon subsystem, I'd ask
> for the error handling to be moved into the case statements. On top of that,
> interpreting a return value of "0" as error seems wrong.
> ufs_get_temp() returns 0 if the measured temperature or the reported limit
> happens to have a value of 80, and that is perfectly valid. If ufs_get_temp()
> reports an error, it should report that as error.
>
> Also, EINVAL is "invalid argument", which I am quite sure does not apply here.
Ditto.
EINVAL implies that the temperature is invalid.
> >
> > +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;
> > + u8 mask = 0;
> > +
> > + if (!(hba->caps & UFSHCD_CAP_TEMP_NOTIF) ||
> > + dev_info->wspecversion < 0x300)
>
> I am quite sure this fits a single line.
Done.
