On 3/13/2025 11:14 AM, Bart Van Assche wrote:
On 3/6/25 9:31 AM, Bao D. Nguyen wrote:
+What: /sys/bus/platform/drivers/ufshcd/*/device_lvl_exception
+What: /sys/bus/platform/devices/*.ufs/device_lvl_exception
+Date: March 2025
+Contact: Bao D. Nguyen <quic_nguyenb@xxxxxxxxxxx>
+Description:
+ The device_lvl_exception is a counter indicating the number
+ of times the device level exceptions have occurred since the
+ last time this variable is reset. Read the
device_lvl_exception_id
+ sysfs node to know more information about the exception.
+ The file is read only.
Shouldn't this sysfs attribute have a "_count" suffix to make it clear
that it represents a count?
Thank you, Bart. I agree. Will make the change.
Additionally, here and below, please change "file" into "attribute".
+What: /sys/bus/platform/drivers/ufshcd/*/device_lvl_exception_id
+What: /sys/bus/platform/devices/*.ufs/device_lvl_exception_id
+Date: March 2025
+Contact: Bao D. Nguyen <quic_nguyenb@xxxxxxxxxxx>
+Description:
+ Reading the device_lvl_exception_id returns the device JEDEC
+ standard's qDeviceLevelExceptionID attribute. The definition
of the
+ qDeviceLevelExceptionID is the ufs device vendor specific
design.
+ Refer to the device manufacturer datasheet for more information
+ on the meaning of the qDeviceLevelExceptionID attribute value.
+ The file is read only.
I'm not sure it is useful to export vendor-specific information to
sysfs.
Because each ufs vendor defines the data differently according to the
device spec, we probably can't have a defined handling on this event in
the kernel. For some applications such as automobile, the information is
useful. If you have suggestions for the user applications to access this
data, I am all ears.
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 90b5ab6..0248288a 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -466,6 +466,56 @@ static ssize_t critical_health_show(struct device
*dev,
return sysfs_emit(buf, "%d\n", hba->critical_health_count);
}
+static ssize_t device_lvl_exception_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+
+ if (hba->dev_info.wspecversion < 0x410)
+ return -EOPNOTSUPP;
+
+ return sysfs_emit(buf, "%u\n", hba->dev_lvl_exception_count);
+}
The preferred approach for sysfs attributes that are not supported is to
make these invisible rather than returning an error code.I was thinking it would be useful for the user application to know the
ufs device does not support this feature, so that it would not keep
trying to read the data.
+static ssize_t device_lvl_exception_id_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+ u64 exception_id;
+ int err;
+
+ ufshcd_rpm_get_sync(hba);
+ err = ufshcd_read_device_lvl_exception_id(hba, &exception_id);
+ ufshcd_rpm_put_sync(hba);
+
+ if (err)
+ return err;
+
+ hba->dev_lvl_exception_id = exception_id;
+ return sysfs_emit(buf, "%llu\n", exception_id);
+}
Just like device_lvl_exception, this attribute shouldn't be visible if
device level exceptions are not supported.
Same reasoning, to inform the user application the feature is not
supported so that it does not keep trying.
+ if (status & hba->ee_drv_mask & MASK_EE_DEV_LVL_EXCEPTION) {
+ hba->dev_lvl_exception_count++;
+ sysfs_notify(&hba->dev->kobj, NULL, "device_lvl_exception");
+ }
sysfs_notify() may sleep and hence must not be called from an interrupt
handler.
I will look into this.
Thanks, Bao
Thanks,
Bart.
