Re: [PATCH v1 1/1] scsi: ufs: core: add device level exception support

["Bao D. Nguyen" <quic_nguyenb@quicinc.com>]

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@quicinc.com>
>> +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@quicinc.com>
>> +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.